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 0cadc6439..186fe98c8 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,62 @@ await CSharpAnalyzerWithLanguageVersionTest.VerifyAnalyzerAsync( + source, + LanguageVersion.CSharp12, + editorconfig: [("_MvvmToolkitIsUsingWindowsRuntimePack", true), ("CsWinRTAotOptimizerEnabled", "auto")]); + } + } + [TestMethod] public async Task WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer_TargetingWindows_CsWinRTAotOptimizerEnabled_True_NoXaml_Level1_DoesNotWarn() {