From ef2233657a16c190228c1665ed9218705cc83b35 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 28 Nov 2024 01:28:03 -0800 Subject: [PATCH 1/2] Add unit test for consistent compilation diagnostic --- .../Test_SourceGeneratorsDiagnostics.cs | 54 ++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4120.UnitTests/Test_SourceGeneratorsDiagnostics.cs b/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4120.UnitTests/Test_SourceGeneratorsDiagnostics.cs index 0cadc6439..fc18d3e2c 100644 --- a/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4120.UnitTests/Test_SourceGeneratorsDiagnostics.cs +++ b/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4120.UnitTests/Test_SourceGeneratorsDiagnostics.cs @@ -468,7 +468,7 @@ namespace MyApp { public partial class SampleViewModel : ObservableObject { - [{|MVVMTK0051:ObservableProperty|}] + [{|MVVMTK0051:ObservableProperty|}] private string {|MVVMTK0045:name|}; } } @@ -480,6 +480,58 @@ await CSharpAnalyzerWithLanguageVersionTest.VerifyAnalyzerAsync( + source, + LanguageVersion.CSharp12, + editorconfig: [("_MvvmToolkitIsUsingWindowsRuntimePack", true), ("CsWinRTAotOptimizerEnabled", "auto")]); + } + [TestMethod] public async Task WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer_TargetingWindows_CsWinRTAotOptimizerEnabled_True_NoXaml_Level1_DoesNotWarn() { From 421ad363f3fe4e2cb7bf62aedd617fbf06b1a984 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 28 Nov 2024 10:58:35 -0800 Subject: [PATCH 2/2] Update analyzer to emit consistent locations --- ...pertyOnFieldsIsNotAotCompatibleAnalyzer.cs | 65 +++++++++++++++++-- .../Test_SourceGeneratorsDiagnostics.cs | 12 ++-- 2 files changed, 68 insertions(+), 9 deletions(-) diff --git a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs index 4fa8040ba..1a925e7a3 100644 --- a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs +++ b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs @@ -4,7 +4,9 @@ #if ROSLYN_4_12_0_OR_GREATER +using System; using System.Collections.Immutable; +using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Threading; using CommunityToolkit.Mvvm.SourceGenerators.Extensions; @@ -68,11 +70,11 @@ public override void Initialize(AnalysisContext context) fieldSymbol.ContainingType, fieldSymbol.Name)); - // Notify that we did produce at least one diagnostic. Note: callbacks can run in parallel, so the order - // is not guaranteed. As such, there's no point in using an interlocked compare exchange operation here, - // since we couldn't rely on the value being written actually being the "first" occurrence anyway. - // So we can just do a normal volatile read for better performance. - Volatile.Write(ref firstObservablePropertyAttribute, observablePropertyAttribute); + // Track the attribute data to use as target for the diagnostic. This method takes care of effectively + // sorting all incoming values, so that the final one is deterministic across runs. This ensures that + // the actual location will be the same across recompilations, instead of jumping around all over the + // place. This also makes it possible to more easily suppress it, since its location would not change. + SetOrUpdateAttributeDataBySourceLocation(ref firstObservablePropertyAttribute, observablePropertyAttribute); } }, SymbolKind.Field); @@ -95,6 +97,59 @@ public override void Initialize(AnalysisContext context) }); }); } + + /// + /// Sets or updates the instance to use for compilation diagnostics, sorting by source location. + /// + /// The location of the previous value to potentially overwrite. + /// Thew new instance. + private static void SetOrUpdateAttributeDataBySourceLocation([NotNull] ref AttributeData? oldAttributeDataLocation, AttributeData newAttributeData) + { + bool hasReplacedOriginalValue; + + do + { + AttributeData? oldAttributeData = Volatile.Read(ref oldAttributeDataLocation); + + // If the old attribute data is null, it means this is the first time we called this method with a new + // attribute candidate. In that case, there is nothing to check: we should always store the new instance. + if (oldAttributeData is not null) + { + // Sort by file paths, alphabetically + int filePathRelativeSortIndex = string.Compare( + newAttributeData.ApplicationSyntaxReference?.SyntaxTree.FilePath, + oldAttributeData.ApplicationSyntaxReference?.SyntaxTree.FilePath, + StringComparison.OrdinalIgnoreCase); + + // Also sort by location (this is a tie-breaker if two values are from the same file) + bool isTextSpanLowerSorted = + (newAttributeData.ApplicationSyntaxReference?.Span.Start ?? 0) < + (oldAttributeData.ApplicationSyntaxReference?.Span.Start ?? 0); + + // The new candidate can be dropped if it's from a file that's alphabetically sorted after + // the old value, or whether the location is after the previous one, within the same file. + if (filePathRelativeSortIndex == 1 || (filePathRelativeSortIndex == 0 && !isTextSpanLowerSorted)) + { + break; + } + } + + // Attempt to actually replace the old value, without taking locks + AttributeData? originalValue = Interlocked.CompareExchange( + location1: ref oldAttributeDataLocation, + value: newAttributeData, + comparand: oldAttributeData); + + // This call might have raced against other threads, since all symbol actions can run in parallel. + // If the original value is the old value we read at the start of the method, it means no other + // thread raced against this one, so we are done. If it's different, then we failed to write the + // new candidate. We can discard the work done in this iteration and simply try again. + hasReplacedOriginalValue = oldAttributeData == originalValue; + } + while (!hasReplacedOriginalValue); +#pragma warning disable CS8777 // The loop always ensures that 'oldAttributeDataLocation' is not null on exit + } +#pragma warning restore CS8777 } #endif diff --git a/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4120.UnitTests/Test_SourceGeneratorsDiagnostics.cs b/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4120.UnitTests/Test_SourceGeneratorsDiagnostics.cs index fc18d3e2c..186fe98c8 100644 --- a/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4120.UnitTests/Test_SourceGeneratorsDiagnostics.cs +++ b/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4120.UnitTests/Test_SourceGeneratorsDiagnostics.cs @@ -526,10 +526,14 @@ public partial class YetAnotherViewModel : ObservableObject } """; - await CSharpAnalyzerWithLanguageVersionTest.VerifyAnalyzerAsync( - source, - LanguageVersion.CSharp12, - editorconfig: [("_MvvmToolkitIsUsingWindowsRuntimePack", true), ("CsWinRTAotOptimizerEnabled", "auto")]); + // This test is non deterministic, so run it 10 times to ensure the likelihood of it passing just by luck is almost 0 + for (int i = 0; i < 10; i++) + { + await CSharpAnalyzerWithLanguageVersionTest.VerifyAnalyzerAsync( + source, + LanguageVersion.CSharp12, + editorconfig: [("_MvvmToolkitIsUsingWindowsRuntimePack", true), ("CsWinRTAotOptimizerEnabled", "auto")]); + } } [TestMethod]