From c4d904163bac028efe6ec3829dafa5af8edf5f0e Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 26 Jan 2023 19:45:28 +0100 Subject: [PATCH 1/3] Add Compilation.TryBuildNamedTypeSymbolMap extension --- .../Extensions/CompilationExtensions.cs | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/CommunityToolkit.Mvvm.SourceGenerators/Extensions/CompilationExtensions.cs b/src/CommunityToolkit.Mvvm.SourceGenerators/Extensions/CompilationExtensions.cs index 878b62188..ac05bdff6 100644 --- a/src/CommunityToolkit.Mvvm.SourceGenerators/Extensions/CompilationExtensions.cs +++ b/src/CommunityToolkit.Mvvm.SourceGenerators/Extensions/CompilationExtensions.cs @@ -2,6 +2,10 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Diagnostics.CodeAnalysis; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; @@ -87,4 +91,37 @@ public static bool HasAccessibleTypeWithMetadataName(this Compilation compilatio return false; } + + /// + /// Tries to build a map of instances form the input mapping of names. + /// + /// The type of keys for each symbol. + /// The to consider for analysis. + /// The input mapping of keys to fully qualified type names. + /// The resulting mapping of keys to resolved instances. + /// Whether all requested instances could be resolved. + public static bool TryBuildNamedTypeSymbolMap( + this Compilation compilation, + IEnumerable> typeNames, + [NotNullWhen(true)] out ImmutableDictionary? typeSymbols) + where T : IEquatable + { + ImmutableDictionary.Builder builder = ImmutableDictionary.CreateBuilder(); + + foreach (KeyValuePair pair in typeNames) + { + if (compilation.GetTypeByMetadataName(pair.Value) is not INamedTypeSymbol attributeSymbol) + { + typeSymbols = null; + + return false; + } + + builder.Add(pair.Key, attributeSymbol); + } + + typeSymbols = builder.ToImmutable(); + + return true; + } } From 857b4b871a07fc77647c9a500b2b9af1f383c1d3 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 26 Jan 2023 19:47:57 +0100 Subject: [PATCH 2/3] Resolve symbols before registering actions in analyzers --- ...ngAttributeInsteadOfInheritanceAnalyzer.cs | 52 +++++++----- ...renceForObservablePropertyFieldAnalyzer.cs | 80 ++++++++++--------- ...entObservablePropertyAttributesAnalyzer.cs | 20 +++-- ...pertyChangedRecipientsAttributeAnalyzer.cs | 2 +- ...nsupportedCSharpLanguageVersionAnalyzer.cs | 15 ++-- 5 files changed, 99 insertions(+), 70 deletions(-) diff --git a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/ClassUsingAttributeInsteadOfInheritanceAnalyzer.cs b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/ClassUsingAttributeInsteadOfInheritanceAnalyzer.cs index 1dbacc7b9..a28c86cdf 100644 --- a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/ClassUsingAttributeInsteadOfInheritanceAnalyzer.cs +++ b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/ClassUsingAttributeInsteadOfInheritanceAnalyzer.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Collections.Immutable; using System.Linq; +using CommunityToolkit.Mvvm.SourceGenerators.Extensions; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; using static CommunityToolkit.Mvvm.SourceGenerators.Diagnostics.DiagnosticDescriptors; @@ -56,37 +57,44 @@ public override void Initialize(AnalysisContext context) context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); context.EnableConcurrentExecution(); - context.RegisterSymbolAction(static context => + context.RegisterCompilationStartAction(static context => { - // We're looking for class declarations - if (context.Symbol is not INamedTypeSymbol { TypeKind: TypeKind.Class, IsRecord: false, IsStatic: false, IsImplicitlyDeclared: false } classSymbol) + // Try to get all necessary type symbols + if (!context.Compilation.TryBuildNamedTypeSymbolMap(GeneratorAttributeNamesToFullyQualifiedNamesMap, out ImmutableDictionary? typeSymbols)) { return; } - foreach (AttributeData attribute in context.Symbol.GetAttributes()) + context.RegisterSymbolAction(context => { - // Same logic as in FieldWithOrphanedDependentObservablePropertyAttributesAnalyzer to find target attributes - if (attribute.AttributeClass is { Name: string attributeName } attributeClass && - GeneratorAttributeNamesToFullyQualifiedNamesMap.TryGetValue(attributeName, out string? fullyQualifiedAttributeName) && - context.Compilation.GetTypeByMetadataName(fullyQualifiedAttributeName) is INamedTypeSymbol attributeSymbol && - SymbolEqualityComparer.Default.Equals(attributeClass, attributeSymbol)) + // We're looking for class declarations that don't have any base type + if (context.Symbol is not INamedTypeSymbol { TypeKind: TypeKind.Class, IsRecord: false, IsStatic: false, IsImplicitlyDeclared: false } classSymbol) { - // The type is annotated with either [ObservableObject] or [INotifyPropertyChanged]. - // Next, we need to check whether it isn't already inheriting from another type. - if (classSymbol.BaseType is { SpecialType: SpecialType.System_Object }) + return; + } + + foreach (AttributeData attribute in context.Symbol.GetAttributes()) + { + // Same logic as in FieldWithOrphanedDependentObservablePropertyAttributesAnalyzer to find target attributes + if (attribute.AttributeClass is { Name: string attributeName } attributeClass && + typeSymbols.TryGetValue(attributeName, out INamedTypeSymbol? attributeSymbol) && + SymbolEqualityComparer.Default.Equals(attributeClass, attributeSymbol)) { - // This type is using the attribute when it could just inherit from ObservableObject, which is preferred - context.ReportDiagnostic(Diagnostic.Create( - GeneratorAttributeNamesToDiagnosticsMap[attributeClass.Name], - context.Symbol.Locations.FirstOrDefault(), - ImmutableDictionary.Create() - .Add(TypeNameKey, classSymbol.Name) - .Add(AttributeTypeNameKey, attributeName), - context.Symbol)); + // The type is annotated with either [ObservableObject] or [INotifyPropertyChanged]. + if (classSymbol.BaseType is { SpecialType: SpecialType.System_Object }) + { + // This type is using the attribute when it could just inherit from ObservableObject, which is preferred + context.ReportDiagnostic(Diagnostic.Create( + GeneratorAttributeNamesToDiagnosticsMap[attributeClass.Name], + context.Symbol.Locations.FirstOrDefault(), + ImmutableDictionary.Create() + .Add(TypeNameKey, classSymbol.Name) + .Add(AttributeTypeNameKey, attributeName), + context.Symbol)); + } } } - } - }, SymbolKind.NamedType); + }, SymbolKind.NamedType); + }); } } diff --git a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/FieldReferenceForObservablePropertyFieldAnalyzer.cs b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/FieldReferenceForObservablePropertyFieldAnalyzer.cs index dd7885ed2..599fa7cdb 100644 --- a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/FieldReferenceForObservablePropertyFieldAnalyzer.cs +++ b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/FieldReferenceForObservablePropertyFieldAnalyzer.cs @@ -35,51 +35,59 @@ public override void Initialize(AnalysisContext context) context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); context.EnableConcurrentExecution(); - context.RegisterOperationAction(static context => + context.RegisterCompilationStartAction(static context => { - // We're only looking for references to fields that could potentially be observable properties - if (context.Operation is not IFieldReferenceOperation - { - Field: IFieldSymbol { IsStatic: false, IsConst: false, IsImplicitlyDeclared: false, ContainingType: INamedTypeSymbol } fieldSymbol, - Instance.Type: ITypeSymbol typeSymbol - }) + // Get the symbol for [ObservableProperty] + if (context.Compilation.GetTypeByMetadataName("CommunityToolkit.Mvvm.ComponentModel.ObservablePropertyAttribute") is not INamedTypeSymbol observablePropertySymbol) { return; } - // Special case field references from within a constructor and don't ever emit warnings for them. The point of this - // analyzer is to prevent mistakes when users assign a field instead of a property and then get confused when the - // property changed event is not raised. But this would never be the case from a constructur anyway, given that - // no handler for that event would possibly be present. Suppressing warnings in this cases though will help to - // avoid scenarios where people get nullability warnings they cannot suppress, in case they were pushed by the - // analyzer in the MVVM Toolkit to not assign a field marked with a non-nullable reference type. Ideally this - // would be solved by habing the generated setter be marked with [MemberNotNullIfNotNull("field", "value")], - // but such an annotation does not currently exist. - if (context.ContainingSymbol is IMethodSymbol { MethodKind: MethodKind.Constructor, ContainingType: INamedTypeSymbol instanceType } && - SymbolEqualityComparer.Default.Equals(instanceType, typeSymbol)) + context.RegisterOperationAction(context => { - return; - } - - foreach (AttributeData attribute in fieldSymbol.GetAttributes()) - { - // Look for the [ObservableProperty] attribute (there can only ever be one per field) - if (attribute.AttributeClass is { Name: "ObservablePropertyAttribute" } attributeClass && - context.Compilation.GetTypeByMetadataName("CommunityToolkit.Mvvm.ComponentModel.ObservablePropertyAttribute") is INamedTypeSymbol attributeSymbol && - SymbolEqualityComparer.Default.Equals(attributeClass, attributeSymbol)) + // We're only looking for references to fields that could potentially be observable properties + if (context.Operation is not IFieldReferenceOperation + { + Field: IFieldSymbol { IsStatic: false, IsConst: false, IsImplicitlyDeclared: false, ContainingType: INamedTypeSymbol } fieldSymbol, + Instance.Type: ITypeSymbol typeSymbol + }) { - // Emit a warning to redirect users to access the generated property instead - context.ReportDiagnostic(Diagnostic.Create( - FieldReferenceForObservablePropertyFieldWarning, - context.Operation.Syntax.GetLocation(), - ImmutableDictionary.Create() - .Add(FieldNameKey, fieldSymbol.Name) - .Add(PropertyNameKey, ObservablePropertyGenerator.Execute.GetGeneratedPropertyName(fieldSymbol)), - fieldSymbol)); + return; + } + // Special case field references from within a constructor and don't ever emit warnings for them. The point of this + // analyzer is to prevent mistakes when users assign a field instead of a property and then get confused when the + // property changed event is not raised. But this would never be the case from a constructur anyway, given that + // no handler for that event would possibly be present. Suppressing warnings in this cases though will help to + // avoid scenarios where people get nullability warnings they cannot suppress, in case they were pushed by the + // analyzer in the MVVM Toolkit to not assign a field marked with a non-nullable reference type. Ideally this + // would be solved by habing the generated setter be marked with [MemberNotNullIfNotNull("field", "value")], + // but such an annotation does not currently exist. + if (context.ContainingSymbol is IMethodSymbol { MethodKind: MethodKind.Constructor, ContainingType: INamedTypeSymbol instanceType } && + SymbolEqualityComparer.Default.Equals(instanceType, typeSymbol)) + { return; } - } - }, OperationKind.FieldReference); + + foreach (AttributeData attribute in fieldSymbol.GetAttributes()) + { + // Look for the [ObservableProperty] attribute (there can only ever be one per field) + if (attribute.AttributeClass is { Name: "ObservablePropertyAttribute" } attributeClass && + SymbolEqualityComparer.Default.Equals(attributeClass, observablePropertySymbol)) + { + // Emit a warning to redirect users to access the generated property instead + context.ReportDiagnostic(Diagnostic.Create( + FieldReferenceForObservablePropertyFieldWarning, + context.Operation.Syntax.GetLocation(), + ImmutableDictionary.Create() + .Add(FieldNameKey, fieldSymbol.Name) + .Add(PropertyNameKey, ObservablePropertyGenerator.Execute.GetGeneratedPropertyName(fieldSymbol)), + fieldSymbol)); + + return; + } + } + }, OperationKind.FieldReference); + }); } } diff --git a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/FieldWithOrphanedDependentObservablePropertyAttributesAnalyzer.cs b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/FieldWithOrphanedDependentObservablePropertyAttributesAnalyzer.cs index 44962f0b9..1505b6865 100644 --- a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/FieldWithOrphanedDependentObservablePropertyAttributesAnalyzer.cs +++ b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/FieldWithOrphanedDependentObservablePropertyAttributesAnalyzer.cs @@ -48,7 +48,19 @@ public override void Initialize(AnalysisContext context) return; } - context.RegisterSymbolAction(static context => + // Try to get all necessary type symbols to map + if (!context.Compilation.TryBuildNamedTypeSymbolMap(GeneratorAttributeNamesToFullyQualifiedNamesMap, out ImmutableDictionary? typeSymbols)) + { + return; + } + + // We also need the symbol for [ObservableProperty], separately + if (context.Compilation.GetTypeByMetadataName("CommunityToolkit.Mvvm.ComponentModel.ObservablePropertyAttribute") is not INamedTypeSymbol observablePropertySymbol) + { + return; + } + + context.RegisterSymbolAction(context => { ImmutableArray attributes = context.Symbol.GetAttributes(); @@ -61,17 +73,15 @@ public override void Initialize(AnalysisContext context) foreach (AttributeData dependentAttribute in attributes) { // Go over each attribute on the target symbol, anche check if any of them matches one of the trigger attributes. - // The logic here is the same as the one in UnsupportedCSharpLanguageVersionAnalyzer, to minimize retrieving symbols. + // The logic here is the same as the one in UnsupportedCSharpLanguageVersionAnalyzer. if (dependentAttribute.AttributeClass is { Name: string attributeName } dependentAttributeClass && - GeneratorAttributeNamesToFullyQualifiedNamesMap.TryGetValue(attributeName, out string? fullyQualifiedDependentAttributeName) && - context.Compilation.GetTypeByMetadataName(fullyQualifiedDependentAttributeName) is INamedTypeSymbol dependentAttributeSymbol && + typeSymbols.TryGetValue(attributeName, out INamedTypeSymbol? dependentAttributeSymbol) && SymbolEqualityComparer.Default.Equals(dependentAttributeClass, dependentAttributeSymbol)) { // If the attribute matches, iterate over the attributes to try to find [ObservableProperty] foreach (AttributeData attribute in attributes) { if (attribute.AttributeClass is { Name: "ObservablePropertyAttribute" } attributeSymbol && - context.Compilation.GetTypeByMetadataName("CommunityToolkit.Mvvm.ComponentModel.ObservablePropertyAttribute") is INamedTypeSymbol observablePropertySymbol && SymbolEqualityComparer.Default.Equals(attributeSymbol, observablePropertySymbol)) { // If [ObservableProperty] is found, then this field is valid in that it doesn't have orphaned dependent attributes diff --git a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/InvalidClassLevelNotifyPropertyChangedRecipientsAttributeAnalyzer.cs b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/InvalidClassLevelNotifyPropertyChangedRecipientsAttributeAnalyzer.cs index d1062ba42..c9525ed3d 100644 --- a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/InvalidClassLevelNotifyPropertyChangedRecipientsAttributeAnalyzer.cs +++ b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/InvalidClassLevelNotifyPropertyChangedRecipientsAttributeAnalyzer.cs @@ -44,7 +44,7 @@ public override void Initialize(AnalysisContext context) return; } - // Emit a diagnstic for types that use [NotifyPropertyChangedRecipients] but are neither inheriting from ObservableRecipient nor using [ObservableRecipient] + // Emit a diagnostic for types that use [NotifyPropertyChangedRecipients] but are neither inheriting from ObservableRecipient nor using [ObservableRecipient] if (classSymbol.HasAttributeWithType(notifyPropertyChangedRecipientsAttributeSymbol) && !classSymbol.InheritsFromType(observableRecipientSymbol) && !classSymbol.HasOrInheritsAttributeWithType(observableRecipientAttributeSymbol)) diff --git a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/UnsupportedCSharpLanguageVersionAnalyzer.cs b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/UnsupportedCSharpLanguageVersionAnalyzer.cs index fd5f20b72..af81bcac4 100644 --- a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/UnsupportedCSharpLanguageVersionAnalyzer.cs +++ b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/UnsupportedCSharpLanguageVersionAnalyzer.cs @@ -54,7 +54,13 @@ public override void Initialize(AnalysisContext context) return; } - context.RegisterSymbolAction(static context => + // Try to get all necessary type symbols + if (!context.Compilation.TryBuildNamedTypeSymbolMap(GeneratorAttributeNamesToFullyQualifiedNamesMap, out ImmutableDictionary? typeSymbols)) + { + return; + } + + context.RegisterSymbolAction(context => { // The possible attribute targets are only fields, classes and methods if (context.Symbol is not (IFieldSymbol or INamedTypeSymbol { TypeKind: TypeKind.Class, IsImplicitlyDeclared: false } or IMethodSymbol)) @@ -65,12 +71,9 @@ public override void Initialize(AnalysisContext context) foreach (AttributeData attribute in context.Symbol.GetAttributes()) { // Go over each attribute on the target symbol, and check if the attribute type name is a candidate. - // If it is, double check by actually resolving the symbol from the compilation and comparing against it. - // This minimizes the calls to CompilationGetTypeByMetadataName(string) to only cases where it's almost - // guaranteed we'll actually get a match. If we do have one, then we can emit the diagnostic for the symbol. + // If it is, double check by actually resolving the symbol from the mapping and comparing against it. if (attribute.AttributeClass is { Name: string attributeName } attributeClass && - GeneratorAttributeNamesToFullyQualifiedNamesMap.TryGetValue(attributeName, out string? fullyQualifiedAttributeName) && - context.Compilation.GetTypeByMetadataName(fullyQualifiedAttributeName) is INamedTypeSymbol attributeSymbol && + typeSymbols.TryGetValue(attributeName, out INamedTypeSymbol? attributeSymbol) && SymbolEqualityComparer.Default.Equals(attributeClass, attributeSymbol)) { context.ReportDiagnostic(Diagnostic.Create(UnsupportedCSharpLanguageVersionError, context.Symbol.Locations.FirstOrDefault())); From afad5d2b20a141e17924bf36744b0ce5ac9e3af7 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 26 Jan 2023 21:33:52 +0100 Subject: [PATCH 3/3] Do base type check early in ClassUsingAttributeInsteadOfInheritanceAnalyzer --- ...ngAttributeInsteadOfInheritanceAnalyzer.cs | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/ClassUsingAttributeInsteadOfInheritanceAnalyzer.cs b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/ClassUsingAttributeInsteadOfInheritanceAnalyzer.cs index a28c86cdf..39419d2b0 100644 --- a/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/ClassUsingAttributeInsteadOfInheritanceAnalyzer.cs +++ b/src/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/Analyzers/ClassUsingAttributeInsteadOfInheritanceAnalyzer.cs @@ -68,7 +68,7 @@ public override void Initialize(AnalysisContext context) context.RegisterSymbolAction(context => { // We're looking for class declarations that don't have any base type - if (context.Symbol is not INamedTypeSymbol { TypeKind: TypeKind.Class, IsRecord: false, IsStatic: false, IsImplicitlyDeclared: false } classSymbol) + if (context.Symbol is not INamedTypeSymbol { TypeKind: TypeKind.Class, IsRecord: false, IsStatic: false, IsImplicitlyDeclared: false, BaseType.SpecialType: SpecialType.System_Object } classSymbol) { return; } @@ -80,18 +80,15 @@ public override void Initialize(AnalysisContext context) typeSymbols.TryGetValue(attributeName, out INamedTypeSymbol? attributeSymbol) && SymbolEqualityComparer.Default.Equals(attributeClass, attributeSymbol)) { - // The type is annotated with either [ObservableObject] or [INotifyPropertyChanged]. - if (classSymbol.BaseType is { SpecialType: SpecialType.System_Object }) - { - // This type is using the attribute when it could just inherit from ObservableObject, which is preferred - context.ReportDiagnostic(Diagnostic.Create( - GeneratorAttributeNamesToDiagnosticsMap[attributeClass.Name], - context.Symbol.Locations.FirstOrDefault(), - ImmutableDictionary.Create() - .Add(TypeNameKey, classSymbol.Name) - .Add(AttributeTypeNameKey, attributeName), - context.Symbol)); - } + // The type is annotated with either [ObservableObject] or [INotifyPropertyChanged], and we already validated + // that it has no other base type, so emit a diagnostic to suggest inheriting from ObservableObject instead. + context.ReportDiagnostic(Diagnostic.Create( + GeneratorAttributeNamesToDiagnosticsMap[attributeClass.Name], + context.Symbol.Locations.FirstOrDefault(), + ImmutableDictionary.Create() + .Add(TypeNameKey, classSymbol.Name) + .Add(AttributeTypeNameKey, attributeName), + context.Symbol)); } } }, SymbolKind.NamedType);