Merge pull request #1009 from CommunityToolkit/dev/consistent-compilation-diagnostic-location

Emit consistent locations for 'MVVMTK0051' diagnostics

Author: Sergio0694

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)
             });
         });
     }
+
+    /// <summary>
+    /// Sets or updates the <see cref="AttributeData"/> instance to use for compilation diagnostics, sorting by source location.
+    /// </summary>
+    /// <param name="oldAttributeDataLocation">The location of the previous value to potentially overwrite.</param>
+    /// <param name="newAttributeData">Thew new <see cref="AttributeData"/> instance.</param>
+    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

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<WinRTObservablePropertyOnFieldsIsNot
             editorconfig: [("_MvvmToolkitIsUsingWindowsRuntimePack", true), ("CsWinRTAotOptimizerEnabled", "auto")]);
     }
 
+    [TestMethod]
+    public async Task WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer_TargetingWindows_CsWinRTAotOptimizerEnabled_Auto_NotCSharpPreview_MultipleFields_Warns_WithCompilationWarning_ConsistentLocation()
+    {
+        const string source = """
+            using CommunityToolkit.Mvvm.ComponentModel;
+            
+            namespace MyApp
+            {
+                public partial class SampleViewModel : ObservableObject
+                {            
+                    [{|MVVMTK0051:ObservableProperty|}]
+                    private string {|MVVMTK0045:f1|};
+
+                    [ObservableProperty]
+                    private string {|MVVMTK0045:f2|};
+
+                    [ObservableProperty]
+                    private string {|MVVMTK0045:f3|};
+                }
+
+                public partial class OtherViewModel : ObservableObject
+                {            
+                    [ObservableProperty]
+                    private string {|MVVMTK0045:f1|};
+
+                    [ObservableProperty]
+                    private string {|MVVMTK0045:f2|};
+
+                    [ObservableProperty]
+                    private string {|MVVMTK0045:f3|};
+                }
+            }
+
+            namespace OtherNamespace
+            {
+                public partial class YetAnotherViewModel : ObservableObject
+                {            
+                    [ObservableProperty]
+                    private string {|MVVMTK0045:f1|};
+
+                    [ObservableProperty]
+                    private string {|MVVMTK0045:f2|};
+                }
+            }
+            """;
+
+        // 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<WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer>.VerifyAnalyzerAsync(
+                source,
+                LanguageVersion.CSharp12,
+                editorconfig: [("_MvvmToolkitIsUsingWindowsRuntimePack", true), ("CsWinRTAotOptimizerEnabled", "auto")]);
+        }
+    }
+
     [TestMethod]
     public async Task WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer_TargetingWindows_CsWinRTAotOptimizerEnabled_True_NoXaml_Level1_DoesNotWarn()
     {