Add ConstructorParamShouldMatchPropNames analyzer #4877
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,5 @@ | ||
| ; Please do not edit this file manually, it should only be updated through code fix application. | ||
| ### New Rules | ||
| Rule ID | Category | Severity | Notes | ||
| --------|----------|----------|------- | ||
| CA1071 | Design | Warning | ConstructorParametersShouldMatchPropertyNamesAnalyzer, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1071) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,194 @@ | ||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System.Collections.Immutable; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Linq; | ||
| using Analyzer.Utilities; | ||
| using Analyzer.Utilities.Extensions; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.Diagnostics; | ||
| using Microsoft.CodeAnalysis.Operations; | ||
|
|
||
| namespace Microsoft.NetCore.Analyzers.Runtime | ||
| { | ||
| /// <summary> | ||
| /// CA1071: Constructor parameters should match property and field names | ||
| /// </summary> | ||
| [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] | ||
| public sealed class ConstructorParametersShouldMatchPropertyNamesAnalyzer : DiagnosticAnalyzer | ||
psxvoid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| internal const string RuleId = "CA1071"; | ||
|
|
||
| private static readonly LocalizableString s_localizableTitle = new LocalizableResourceString(nameof(MicrosoftNetCoreAnalyzersResources.ConstructorParametersShouldMatchPropertyNamesTitle), MicrosoftNetCoreAnalyzersResources.ResourceManager, typeof(MicrosoftNetCoreAnalyzersResources)); | ||
|
|
||
| private static readonly LocalizableString s_localizableMessageProperty = new LocalizableResourceString(nameof(MicrosoftNetCoreAnalyzersResources.ConstructorParameterShouldMatchPropertyName), MicrosoftNetCoreAnalyzersResources.ResourceManager, typeof(MicrosoftNetCoreAnalyzersResources)); | ||
| private static readonly LocalizableString s_localizableMessageField = new LocalizableResourceString(nameof(MicrosoftNetCoreAnalyzersResources.ConstructorParameterShouldMatchFieldName), MicrosoftNetCoreAnalyzersResources.ResourceManager, typeof(MicrosoftNetCoreAnalyzersResources)); | ||
| private static readonly LocalizableString s_localizableDescription = new LocalizableResourceString(nameof(MicrosoftNetCoreAnalyzersResources.ConstructorParametersShouldMatchPropertyNamesDescription), MicrosoftNetCoreAnalyzersResources.ResourceManager, typeof(MicrosoftNetCoreAnalyzersResources)); | ||
|
|
||
| internal static DiagnosticDescriptor PropertyRule = DiagnosticDescriptorHelper.Create(RuleId, | ||
| s_localizableTitle, | ||
| s_localizableMessageProperty, | ||
| DiagnosticCategory.Design, | ||
| RuleLevel.BuildWarning, | ||
buyaa-n marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| description: s_localizableDescription, | ||
| isPortedFxCopRule: true, | ||
buyaa-n marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| isDataflowRule: false); | ||
| internal static DiagnosticDescriptor FieldRule = DiagnosticDescriptorHelper.Create(RuleId, | ||
| s_localizableTitle, | ||
| s_localizableMessageField, | ||
| DiagnosticCategory.Design, | ||
| RuleLevel.BuildWarning, | ||
| description: s_localizableDescription, | ||
| isPortedFxCopRule: true, | ||
| isDataflowRule: false); | ||
|
|
||
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(PropertyRule, FieldRule); | ||
|
|
||
| public override void Initialize(AnalysisContext context) | ||
| { | ||
| context.EnableConcurrentExecution(); | ||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); | ||
|
|
||
| context.RegisterCompilationStartAction( | ||
| (compilationStartContext) => | ||
| { | ||
| INamedTypeSymbol? jsonConstructorAttributeNamedSymbol = compilationStartContext.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemTextJsonSerializationJsonConstructorAttribute); | ||
| if (jsonConstructorAttributeNamedSymbol == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var paramAnalyzer = new ParameterAnalyzer(jsonConstructorAttributeNamedSymbol); | ||
|
|
||
| compilationStartContext.RegisterOperationBlockStartAction( | ||
|
There was a problem hiding this comment. I am not sure if we need to analyze each operation block for checking a constructor, probably better to register symbol action for named types There was a problem hiding this comment. I've tried it before using the current approach. The idea was to reduce the overall amount of checks/calls the analyzer should do before matching a correct type (to have as low an impact on the performance as possible). I thought that There was a problem hiding this comment.
Thanks, that was my concert too, by my understanding operation,
I am not sure if it is counted as a combination of actions, i would assume first Instead with the |
||
| (startOperationBlockContext) => | ||
| { | ||
| if (startOperationBlockContext.OwningSymbol is IMethodSymbol method | ||
| && !paramAnalyzer.ShouldAnalyzeMethod(method)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| startOperationBlockContext.RegisterOperationAction( | ||
|
There was a problem hiding this comment. You will not need to register another action as you can access all parameters from the |
||
| context => ParameterAnalyzer.AnalyzeOperationAndReport(context), | ||
| OperationKind.ParameterReference); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| private sealed class ParameterAnalyzer | ||
| { | ||
| private readonly INamedTypeSymbol _jsonConstructorAttributeInfoType; | ||
|
|
||
| public ParameterAnalyzer(INamedTypeSymbol jsonConstructorAttributeInfoType) | ||
| { | ||
| _jsonConstructorAttributeInfoType = jsonConstructorAttributeInfoType; | ||
| } | ||
|
There was a problem hiding this comment. Only one method is using the field. I personally prefer this being as a static class with jsonConstructorAttributeType being passed to the only method needs it. |
||
|
|
||
| public static void AnalyzeOperationAndReport(OperationAnalysisContext context) | ||
| { | ||
| var operation = (IParameterReferenceOperation)context.Operation; | ||
|
|
||
| if (operation.Parent is not IAssignmentOperation assignment) | ||
|
There was a problem hiding this comment. This is likely to fail for tuple assignments (deconstruction assignment) like There was a problem hiding this comment. I'm not sure whether I've completely got your point, but it seems that tuple assignment works fine with it, see the following commit: [POC] Verify tuple assignment is working with CA1071 Field names you've mentioned do not match parameter names, There was a problem hiding this comment. @psxvoid The tests in that commit are no diagnostic tests. I meant to confirm that diagnostic cases are working correctly for tuple assignments. There was a problem hiding this comment. Yep, sorry, it doesn't work with tuple assignments. I'll come back to it a bit later. There was a problem hiding this comment. Done. Though, I'm not sure whether it's possible in VB. |
||
| { | ||
| return; | ||
| } | ||
|
|
||
| IParameterSymbol param = operation.Parameter; | ||
| ISymbol? referencedSymbol = assignment.Target.GetReferencedMemberOrLocalOrParameter(); | ||
|
|
||
| if (referencedSymbol == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var field = referencedSymbol as IFieldSymbol; | ||
| var prop = referencedSymbol as IPropertySymbol; | ||
|
|
||
| if (field == null && prop == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Only process instance fields and properties | ||
| if (referencedSymbol.IsStatic) | ||
| { | ||
| return; | ||
| } | ||
|
There was a problem hiding this comment. This can be combined with the null check above as follows |
||
|
|
||
| if (IsSupportedField(field) && !IsParamMatchFieldName(param, field)) | ||
| { | ||
| context.ReportDiagnostic( | ||
| param.CreateDiagnostic( | ||
| FieldRule, | ||
| param.ContainingType.ToDisplayString(SymbolDisplayFormats.ShortSymbolDisplayFormat), | ||
| param.Name, | ||
| field.Name)); | ||
| } | ||
|
|
||
| if (IsSupportedProp(prop) && !IsParamMatchPropName(param, prop)) | ||
| { | ||
| context.ReportDiagnostic( | ||
| param.CreateDiagnostic( | ||
| PropertyRule, | ||
| param.ContainingType.ToDisplayString(SymbolDisplayFormats.ShortSymbolDisplayFormat), | ||
| param.Name, | ||
| prop.Name)); | ||
| } | ||
| } | ||
|
|
||
| private static bool IsSupportedProp([NotNullWhen(true)] IPropertySymbol? prop) | ||
| { | ||
| if (prop == null) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| private static bool IsSupportedField([NotNullWhen(true)] IFieldSymbol? field) | ||
| { | ||
| if (field == null) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| private static bool IsParamMatchFieldName(IParameterSymbol param, IFieldSymbol field) | ||
| { | ||
| var paramWords = WordParser.Parse(param.Name, WordParserOptions.SplitCompoundWords); | ||
| var fieldWords = WordParser.Parse(field.Name, WordParserOptions.SplitCompoundWords).ToImmutableArray(); | ||
|
|
||
| return paramWords.All(x => WordParser.ContainsWord(x, WordParserOptions.SplitCompoundWords, fieldWords)); | ||
| } | ||
|
|
||
| private static bool IsParamMatchPropName(IParameterSymbol param, IPropertySymbol prop) | ||
| { | ||
| var paramWords = WordParser.Parse(param.Name, WordParserOptions.SplitCompoundWords); | ||
| var propWords = WordParser.Parse(prop.Name, WordParserOptions.SplitCompoundWords).ToImmutableArray(); | ||
|
|
||
| return paramWords.All(x => WordParser.ContainsWord(x, WordParserOptions.SplitCompoundWords, propWords)); | ||
| } | ||
|
|
||
| public bool ShouldAnalyzeMethod(IMethodSymbol method) | ||
| { | ||
| // We only care about constructors with parameters. | ||
| if (method.Parameters.IsEmpty) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // We only care about constructors that are marked with JsonConstructor attribute. | ||
| if (!method.IsJsonConstructor(_jsonConstructorAttributeInfoType)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
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.
To Do: remove