From 089592527742041ccb7b487672b51cdef3d66abb Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 21 Nov 2024 23:22:44 -0800 Subject: [PATCH 1/3] Add diagnostic for '[ObservableProperty]' on WinRT AOT --- .../AnalyzerReleases.Shipped.md | 1 + ...pertyOnFieldsIsNotAotCompatibleAnalyzer.cs | 27 ++++++++++++++++++- .../Diagnostics/DiagnosticDescriptors.cs | 17 ++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/src/CommunityToolkit.Mvvm.SourceGenerators/AnalyzerReleases.Shipped.md b/src/CommunityToolkit.Mvvm.SourceGenerators/AnalyzerReleases.Shipped.md index e5a0fecb0..730713ab0 100644 --- a/src/CommunityToolkit.Mvvm.SourceGenerators/AnalyzerReleases.Shipped.md +++ b/src/CommunityToolkit.Mvvm.SourceGenerators/AnalyzerReleases.Shipped.md @@ -92,3 +92,4 @@ MVVMTK0047 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator MVVMTK0048 | CommunityToolkit.Mvvm.SourceGenerators.RelayCommandGenerator | Warning | See https://aka.ms/mvvmtoolkit/errors/mvvmtk0048 MVVMTK0049 | CommunityToolkit.Mvvm.SourceGenerators.INotifyPropertyChangedGenerator | Warning | See https://aka.ms/mvvmtoolkit/errors/mvvmtk0049 MVVMTK0050 | CommunityToolkit.Mvvm.SourceGenerators.ObservableObjectGenerator | Warning | See https://aka.ms/mvvmtoolkit/errors/mvvmtk0050 +MVVMTK0051 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Info | See https://aka.ms/mvvmtoolkit/errors/mvvmtk0050 diff --git a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs index f5d04d1bf..c1121edb4 100644 --- a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs +++ b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs @@ -6,6 +6,7 @@ using System.Collections.Immutable; using System.Linq; +using System.Threading; using CommunityToolkit.Mvvm.SourceGenerators.Extensions; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; @@ -20,7 +21,9 @@ namespace CommunityToolkit.Mvvm.SourceGenerators; public sealed class WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer : DiagnosticAnalyzer { /// - public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(WinRTObservablePropertyOnFieldsIsNotAotCompatible); + public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create( + WinRTObservablePropertyOnFieldsIsNotAotCompatible, + WinRTObservablePropertyOnFieldsIsNotAotCompatibleCompilationEndInfo); /// public override void Initialize(AnalysisContext context) @@ -42,6 +45,9 @@ public override void Initialize(AnalysisContext context) return; } + // Track whether we produced any diagnostics, for the compilation end scenario + bool hasProducedAnyDiagnostics = false; + context.RegisterSymbolAction(context => { // Ensure we do have a valid field @@ -61,8 +67,27 @@ public override void Initialize(AnalysisContext context) .Add(FieldReferenceForObservablePropertyFieldAnalyzer.PropertyNameKey, ObservablePropertyGenerator.Execute.GetGeneratedPropertyName(fieldSymbol)), fieldSymbol.ContainingType, fieldSymbol.Name)); + + // Notify that we did produce at least one diagnostic + Volatile.Write(ref hasProducedAnyDiagnostics, true); } }, SymbolKind.Field); + + // If C# preview is already in use, we can stop here. The last diagnostic is only needed when partial properties + // cannot be used, to inform developers that they'll need to bump the language version to enable the code fixer. + if (context.Compilation.IsLanguageVersionPreview()) + { + return; + } + + context.RegisterCompilationEndAction(context => + { + // If we have produced at least one diagnostic, also emit the info message + if (Volatile.Read(ref hasProducedAnyDiagnostics)) + { + context.ReportDiagnostic(Diagnostic.Create(WinRTObservablePropertyOnFieldsIsNotAotCompatibleCompilationEndInfo, location: null)); + } + }); }); } } diff --git a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs index 1bc3fcb44..d770e290f 100644 --- a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs +++ b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs @@ -844,4 +844,21 @@ internal static class DiagnosticDescriptors isEnabledByDefault: true, description: "Using the [ObservableObject] attribute on types is not AOT compatible in WinRT scenarios (such as UWP XAML and WinUI 3 apps), and they should derive from ObservableObject instead (as it allows the CsWinRT generators to correctly produce the necessary WinRT marshalling code).", helpLinkUri: "https://aka.ms/mvvmtoolkit/errors/mvvmtk0050"); + + /// + /// Gets a for when [ObservableProperty] is used on a field in WinRT scenarios. + /// + /// Format: "This project produced one or more 'MVVMTK0045' warnings due to [ObservableProperty] being used on fields, which is not AOT compatible in WinRT scenarios, but it can't enable partial properties and the associated code fixer because 'LangVersion' is not set to 'preview' (setting 'LangVersion=preview' is required to use [ObservableProperty] on partial properties and address these warnings)". + /// + /// + public static readonly DiagnosticDescriptor WinRTObservablePropertyOnFieldsIsNotAotCompatibleCompilationEndInfo = new( + id: "MVVMTK0051", + title: "Using [ObservableProperty] with WinRT and AOT requires 'LangVersion=preview'", + messageFormat: """This project produced one or more 'MVVMTK0045' warnings due to [ObservableProperty] being used on fields, which is not AOT compatible in WinRT scenarios, but it can't enable partial properties and the associated code fixer because 'LangVersion' is not set to 'preview' (setting 'LangVersion=preview' is required to use [ObservableProperty] on partial properties and address these warnings)""", + category: typeof(ObservablePropertyGenerator).FullName, + defaultSeverity: DiagnosticSeverity.Info, + isEnabledByDefault: true, + description: "Project producing one or more 'MVVMTK0045' warnings due to [ObservableProperty] being used on fields, which is not AOT compatible in WinRT scenarios, should set 'LangVersion' to 'preview' to enable partial properties and the associated code fixer because (setting 'LangVersion=preview' is required to use [ObservableProperty] on partial properties and address these warnings).", + helpLinkUri: "https://aka.ms/mvvmtoolkit/errors/mvvmtk0051", + customTags: WellKnownDiagnosticTags.CompilationEnd); } From f877f7cf5341e748a8349343846098b2f13e53e1 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 22 Nov 2024 11:37:14 -0800 Subject: [PATCH 2/3] Add unit test for new diagnostic --- ...pertyOnFieldsIsNotAotCompatibleAnalyzer.cs | 12 +++++----- .../Test_SourceGeneratorsDiagnostics.cs | 22 +++++++++++++++++++ 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs index c1121edb4..33f61ce53 100644 --- a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs +++ b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs @@ -46,7 +46,7 @@ public override void Initialize(AnalysisContext context) } // Track whether we produced any diagnostics, for the compilation end scenario - bool hasProducedAnyDiagnostics = false; + AttributeData? firstObservablePropertyAttribute = null; context.RegisterSymbolAction(context => { @@ -57,7 +57,7 @@ public override void Initialize(AnalysisContext context) } // Emit a diagnostic if the field is using the [ObservableProperty] attribute - if (fieldSymbol.HasAttributeWithType(observablePropertySymbol)) + if (fieldSymbol.TryGetAttributeWithType(observablePropertySymbol, out AttributeData? observablePropertyAttribute)) { context.ReportDiagnostic(Diagnostic.Create( WinRTObservablePropertyOnFieldsIsNotAotCompatible, @@ -69,7 +69,7 @@ public override void Initialize(AnalysisContext context) fieldSymbol.Name)); // Notify that we did produce at least one diagnostic - Volatile.Write(ref hasProducedAnyDiagnostics, true); + _ = Interlocked.CompareExchange(ref firstObservablePropertyAttribute, observablePropertyAttribute, null); } }, SymbolKind.Field); @@ -83,9 +83,11 @@ public override void Initialize(AnalysisContext context) context.RegisterCompilationEndAction(context => { // If we have produced at least one diagnostic, also emit the info message - if (Volatile.Read(ref hasProducedAnyDiagnostics)) + if (Volatile.Read(ref firstObservablePropertyAttribute) is { } observablePropertyAttribute) { - context.ReportDiagnostic(Diagnostic.Create(WinRTObservablePropertyOnFieldsIsNotAotCompatibleCompilationEndInfo, location: null)); + context.ReportDiagnostic(Diagnostic.Create( + WinRTObservablePropertyOnFieldsIsNotAotCompatibleCompilationEndInfo, + observablePropertyAttribute.GetLocation())); } }); }); diff --git a/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4110.UnitTests/Test_SourceGeneratorsDiagnostics.cs b/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4110.UnitTests/Test_SourceGeneratorsDiagnostics.cs index 66c6eea28..d1f55ddc1 100644 --- a/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4110.UnitTests/Test_SourceGeneratorsDiagnostics.cs +++ b/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4110.UnitTests/Test_SourceGeneratorsDiagnostics.cs @@ -434,6 +434,28 @@ await CSharpAnalyzerWithLanguageVersionTest.VerifyAnalyzerAsync( + source, + LanguageVersion.CSharp12, + editorconfig: [("_MvvmToolkitIsUsingWindowsRuntimePack", true), ("CsWinRTAotOptimizerEnabled", "auto")]); + } + [TestMethod] public async Task WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer_TargetingWindows_CsWinRTAotOptimizerEnabled_True_NoXaml_Level1_DoesNotWarn() { From b50a83a3cb7ef68772a697dcab8932465b4edcd8 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 22 Nov 2024 11:51:54 -0800 Subject: [PATCH 3/3] Use 'Volatile.Write' instead for better perf --- ...ObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs index 33f61ce53..0601499fd 100644 --- a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs +++ b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/WinRTObservablePropertyOnFieldsIsNotAotCompatibleAnalyzer.cs @@ -68,8 +68,11 @@ public override void Initialize(AnalysisContext context) fieldSymbol.ContainingType, fieldSymbol.Name)); - // Notify that we did produce at least one diagnostic - _ = Interlocked.CompareExchange(ref firstObservablePropertyAttribute, observablePropertyAttribute, null); + // 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); } }, SymbolKind.Field);