-
-
Notifications
You must be signed in to change notification settings - Fork 14
Add analyzer for missing DefaultValueAttribute on initialized members #780
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
Copilot
wants to merge
9
commits into
main
Choose a base branch
from
copilot/fix-enum-deserialization-issue
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
a551cda
Initial plan
Copilot 96cd080
Add analyzer and code fix for missing DefaultValueAttribute on initia…
Copilot f19320a
Add comprehensive tests for DefaultValueInitializer analyzer and code…
Copilot a93480a
Fix code fix provider to use original expression directly instead of …
Copilot d5ee933
Add test case demonstrating fix for original enum deserialization issue
Copilot 54aef2d
Fix code style issues in IssueReproTests
Copilot 077ecbd
Use PolyType.Roslyn NuGet package instead of embedding source files
Copilot 9c4e47e
Get PolyType.Roslyn working in the analyzer a bit
AArnott 872cc82
Placeholder for fixes
AArnott File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
107 changes: 107 additions & 0 deletions
107
src/Nerdbank.MessagePack.Analyzers.CodeFixes/DefaultValueInitializerCodeFix.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| // Copyright (c) Andrew Arnott. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using System.Composition; | ||
| using Microsoft.CodeAnalysis.CodeActions; | ||
| using Microsoft.CodeAnalysis.CodeFixes; | ||
| using Microsoft.CodeAnalysis.CSharp; | ||
| using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
|
|
||
| namespace Nerdbank.MessagePack.Analyzers.CodeFixes; | ||
|
|
||
| /// <summary> | ||
| /// Code fix provider to add DefaultValueAttribute to fields and properties with initializers. | ||
| /// </summary> | ||
| [ExportCodeFixProvider(LanguageNames.CSharp)] | ||
| [Shared] | ||
| public class DefaultValueInitializerCodeFix : CodeFixProvider | ||
| { | ||
| public override ImmutableArray<string> FixableDiagnosticIds => [DefaultValueInitializerAnalyzer.MissingDefaultValueAttributeDiagnosticId]; | ||
|
|
||
| public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; | ||
|
|
||
| public override async Task RegisterCodeFixesAsync(CodeFixContext context) | ||
| { | ||
| SyntaxNode? root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); | ||
| if (root is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| Diagnostic diagnostic = context.Diagnostics[0]; | ||
| Microsoft.CodeAnalysis.Text.TextSpan diagnosticSpan = diagnostic.Location.SourceSpan; | ||
|
|
||
| // Find the member declaration | ||
| SyntaxNode? node = root.FindNode(diagnosticSpan); | ||
| if (node is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Find the containing member (field or property) | ||
| VariableDeclaratorSyntax? variableDeclarator = node.AncestorsAndSelf().OfType<VariableDeclaratorSyntax>().FirstOrDefault(); | ||
| PropertyDeclarationSyntax? propertyDeclaration = node.AncestorsAndSelf().OfType<PropertyDeclarationSyntax>().FirstOrDefault(); | ||
|
|
||
| if (variableDeclarator?.Initializer is EqualsValueClauseSyntax fieldInitializer) | ||
| { | ||
| // It's a field | ||
| if (this.IsConstExpression(fieldInitializer.Value)) | ||
| { | ||
| context.RegisterCodeFix( | ||
| CodeAction.Create( | ||
| "Add [DefaultValue(...)]", | ||
| cancellationToken => this.AddDefaultValueAttributeAsync(context.Document, root, variableDeclarator.Parent?.Parent, fieldInitializer.Value, cancellationToken), | ||
| equivalenceKey: nameof(DefaultValueInitializerCodeFix)), | ||
| diagnostic); | ||
| } | ||
| } | ||
| else if (propertyDeclaration?.Initializer is EqualsValueClauseSyntax propertyInitializer) | ||
| { | ||
| // It's a property | ||
| if (this.IsConstExpression(propertyInitializer.Value)) | ||
| { | ||
| context.RegisterCodeFix( | ||
| CodeAction.Create( | ||
| "Add [DefaultValue(...)]", | ||
| cancellationToken => this.AddDefaultValueAttributeAsync(context.Document, root, propertyDeclaration, propertyInitializer.Value, cancellationToken), | ||
| equivalenceKey: nameof(DefaultValueInitializerCodeFix)), | ||
| diagnostic); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private bool IsConstExpression(ExpressionSyntax expression) | ||
| { | ||
| return expression is LiteralExpressionSyntax | ||
| || expression is MemberAccessExpressionSyntax // For enum values like TestEnum.Second | ||
| || expression is DefaultExpressionSyntax | ||
| || (expression is PrefixUnaryExpressionSyntax unary && this.IsConstExpression(unary.Operand)); // For negative numbers | ||
| } | ||
|
|
||
| private async Task<Document> AddDefaultValueAttributeAsync(Document document, SyntaxNode root, SyntaxNode? memberDeclaration, ExpressionSyntax initializerValue, CancellationToken cancellationToken) | ||
| { | ||
| if (memberDeclaration is null) | ||
| { | ||
| return document; | ||
| } | ||
|
|
||
| // Create the DefaultValue attribute using the original expression directly | ||
| AttributeArgumentSyntax[] args = [SyntaxFactory.AttributeArgument(initializerValue)]; | ||
|
|
||
| AttributeSyntax attribute = SyntaxFactory.Attribute( | ||
| SyntaxFactory.ParseName("System.ComponentModel.DefaultValue"), | ||
| SyntaxFactory.AttributeArgumentList(SyntaxFactory.SeparatedList(args))); | ||
|
|
||
| AttributeListSyntax attributeList = SyntaxFactory.AttributeList(SyntaxFactory.SingletonSeparatedList(attribute)); | ||
|
|
||
| SyntaxNode newMemberDeclaration = memberDeclaration switch | ||
| { | ||
| FieldDeclarationSyntax field => field.AddAttributeLists(attributeList), | ||
| PropertyDeclarationSyntax property => property.AddAttributeLists(attributeList), | ||
| _ => memberDeclaration, | ||
| }; | ||
|
|
||
| SyntaxNode newRoot = root.ReplaceNode(memberDeclaration, newMemberDeclaration); | ||
| return document.WithSyntaxRoot(newRoot); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| // Copyright (c) Andrew Arnott. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| global using System.Collections.Immutable; | ||
| global using Microsoft.CodeAnalysis; | ||
| global using Microsoft.CodeAnalysis.Diagnostics; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
210 changes: 210 additions & 0 deletions
210
src/Nerdbank.MessagePack.Analyzers/DefaultValueInitializerAnalyzer.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,210 @@ | ||
| // Copyright (c) Andrew Arnott. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
| using PolyType.Roslyn; | ||
|
|
||
| namespace Nerdbank.MessagePack.Analyzers; | ||
|
|
||
| /// <summary> | ||
| /// Analyzer that detects fields and properties with initializers but no DefaultValueAttribute | ||
| /// on types that have source-generated shapes. | ||
| /// </summary> | ||
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
| public class DefaultValueInitializerAnalyzer : DiagnosticAnalyzer | ||
| { | ||
| public const string MissingDefaultValueAttributeDiagnosticId = "NBMsgPack110"; | ||
|
|
||
| public static readonly DiagnosticDescriptor MissingDefaultValueAttributeDescriptor = new( | ||
| id: MissingDefaultValueAttributeDiagnosticId, | ||
| title: Strings.NBMsgPack110_Title, | ||
| messageFormat: Strings.NBMsgPack110_MessageFormat, | ||
| category: "Usage", | ||
| defaultSeverity: DiagnosticSeverity.Warning, | ||
| isEnabledByDefault: true, | ||
| helpLinkUri: AnalyzerUtilities.GetHelpLink(MissingDefaultValueAttributeDiagnosticId), | ||
| customTags: WellKnownDiagnosticTags.CompilationEnd); | ||
|
|
||
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => [ | ||
| MissingDefaultValueAttributeDescriptor, | ||
| ]; | ||
|
|
||
| public override void Initialize(AnalysisContext context) | ||
| { | ||
| context.EnableConcurrentExecution(); | ||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); | ||
|
|
||
| context.RegisterCompilationStartAction(context => | ||
| { | ||
| if (!ReferenceSymbols.TryCreate(context.Compilation, out ReferenceSymbols? referenceSymbols)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| KnownSymbols knownSymbols = new(context.Compilation); | ||
|
|
||
| // Get the DefaultValue attribute symbol | ||
| INamedTypeSymbol? defaultValueAttribute = context.Compilation.GetTypeByMetadataName("System.ComponentModel.DefaultValueAttribute"); | ||
| if (defaultValueAttribute is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Use PolyType.Roslyn's TypeDataModelGenerator to find all types transitively included in the shape | ||
| // TODO: What about finding TypeShapeAttribute, PropertyShapeAttribute, assembly-level attributes, etc.? | ||
| // Is the Include method thread-safe? | ||
| TypeDataModelGenerator generator = new(context.Compilation.Assembly, knownSymbols, context.CancellationToken); | ||
|
|
||
| context.RegisterSymbolAction( | ||
| symbolContext => this.CollectShapes(symbolContext, generator, referenceSymbols), | ||
| SymbolKind.NamedType); | ||
|
|
||
| context.RegisterCompilationEndAction( | ||
| context => | ||
| { | ||
| // Look over all shaped types. | ||
| // Get all generated models - these are all the types for which shapes are generated | ||
| IEnumerable<TypeDataModel> allModels = generator.GeneratedModels.Values; | ||
|
|
||
| // Analyze each type that has a shape generated | ||
| foreach (TypeDataModel model in allModels) | ||
| { | ||
| this.AnalyzeTypeModel(context, model, defaultValueAttribute, referenceSymbols); | ||
| } | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| private void CollectShapes(SymbolAnalysisContext context, TypeDataModelGenerator generator, ReferenceSymbols referenceSymbols) | ||
| { | ||
| INamedTypeSymbol typeSymbol = (INamedTypeSymbol)context.Symbol; | ||
|
|
||
| // Check if this type has a GenerateShapeAttribute or GenerateShapeForAttribute - this is our entry point | ||
| foreach (AttributeData attribute in typeSymbol.GetAttributes()) | ||
| { | ||
| // TODO: What about all the other arguments to these attributes? | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eiriktsarpalis Can you comment on this? |
||
| if (SymbolEqualityComparer.Default.Equals(attribute.AttributeClass, referenceSymbols.GenerateShapeForAttribute)) | ||
| { | ||
| if (attribute.ConstructorArguments is [{ Kind: TypedConstantKind.Type, Value: ITypeSymbol shapedType }]) | ||
| { | ||
| generator.IncludeType(shapedType); | ||
| } | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| // Look for generic variant. | ||
| if (attribute.AttributeClass?.TypeArguments is [{ } typeArg] && SymbolEqualityComparer.Default.Equals(attribute.AttributeClass.ConstructUnboundGenericType(), referenceSymbols.GenerateShapeForGenericAttribute)) | ||
| { | ||
| generator.IncludeType(typeArg); | ||
| continue; | ||
| } | ||
|
|
||
| // Look for ordinary GenerateShapeAttribute. | ||
| if (SymbolEqualityComparer.Default.Equals(attribute.AttributeClass, referenceSymbols.GenerateShapeAttribute)) | ||
| { | ||
| generator.IncludeType(typeSymbol); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void AnalyzeTypeModel(CompilationAnalysisContext context, TypeDataModel model, INamedTypeSymbol defaultValueAttribute, ReferenceSymbols referenceSymbols) | ||
| { | ||
| // Only analyze object types that have properties | ||
| if (model is not ObjectDataModel objectModel) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Check all properties from the model | ||
| foreach (PropertyDataModel property in objectModel.Properties) | ||
| { | ||
| ISymbol member = property.PropertySymbol; | ||
|
|
||
| if (member.IsStatic || member.IsImplicitlyDeclared) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| if (member is IFieldSymbol field) | ||
| { | ||
| this.AnalyzeField(context, field, defaultValueAttribute, referenceSymbols); | ||
| } | ||
| else if (member is IPropertySymbol propertySymbol) | ||
| { | ||
| this.AnalyzeProperty(context, propertySymbol, defaultValueAttribute, referenceSymbols); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void AnalyzeField(CompilationAnalysisContext context, IFieldSymbol field, INamedTypeSymbol defaultValueAttribute, ReferenceSymbols referenceSymbols) | ||
| { | ||
| // Skip static, const, or compiler-generated fields | ||
| if (field.IsConst) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Check if field has an initializer | ||
| if (!this.HasInitializer(field)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Check if field already has DefaultValueAttribute | ||
| if (field.GetAttributes().Any(attr => SymbolEqualityComparer.Default.Equals(attr.AttributeClass, defaultValueAttribute))) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Report diagnostic | ||
| Location location = field.Locations.FirstOrDefault() ?? Location.None; | ||
| context.ReportDiagnostic(Diagnostic.Create(MissingDefaultValueAttributeDescriptor, location, field.Name)); | ||
| } | ||
|
|
||
| private void AnalyzeProperty(CompilationAnalysisContext context, IPropertySymbol property, INamedTypeSymbol defaultValueAttribute, ReferenceSymbols referenceSymbols) | ||
| { | ||
| // Skip static, indexers, or compiler-generated properties | ||
| if (property.IsIndexer) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Check if property has an initializer | ||
| if (!this.HasInitializer(property)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Check if property already has DefaultValueAttribute | ||
| if (property.GetAttributes().Any(attr => SymbolEqualityComparer.Default.Equals(attr.AttributeClass, defaultValueAttribute))) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Report diagnostic | ||
| Location location = property.Locations.FirstOrDefault() ?? Location.None; | ||
| context.ReportDiagnostic(Diagnostic.Create(MissingDefaultValueAttributeDescriptor, location, property.Name)); | ||
| } | ||
|
|
||
| private bool HasInitializer(ISymbol symbol) | ||
| { | ||
| // Check if the symbol has a syntax reference (declaration) | ||
| foreach (SyntaxReference syntaxRef in symbol.DeclaringSyntaxReferences) | ||
| { | ||
| SyntaxNode node = syntaxRef.GetSyntax(); | ||
|
|
||
| if (node is VariableDeclaratorSyntax variableDeclarator && variableDeclarator.Initializer is not null) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| if (node is PropertyDeclarationSyntax propertyDeclaration && propertyDeclaration.Initializer is not null) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eiriktsarpalis Can you comment on this?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, it's not thread safe, and will throw if it detects being called concurrently:
https://github.com/AArnott/PolyType/blob/87921dc2d649b60c3d58b0c117f3a86236e539f9/src/PolyType.Roslyn/ModelGenerator/TypeDataModelGenerator.cs?plain=1#L82C1-L85C14
The other missing functionality requires me to derive from the
TypeDataModelGeneratorand override a bunch of methods. IMO this should be default behavior since I'd just be teaching it to honor its own PolyType attributes.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some overrides in
Parserfrom PolyType likeResolveConstructorsis anything but trivial, and likely has policy that changes over time. This really isn't something I want to source-copy into my own analyzer.I respect that you want to allow others to use this with their own attributes, etc. But could you either:
virtual, OR