Add ConstructorParamShouldMatchPropNames analyzer #4877

psxvoid · 2021-02-22T05:35:34Z

Partial implementation of #33796: Constructor parameters should match property names

Notes/Issues/Questions:

the analyzer does not respect naming style options yet
the analyzer does not contain the code fix yet
the category of the analyzer is set to "Design" but the analyzer is placed in Microsoft.NetCore.Analyzers/Runtime. Should it be placed in Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines instead?

@mavasani Most likely we have to port NamingStyleOptions from Roslyn repository because naming styles may be set to user-defined prefixes in .editorconfig for properties and fields. But NamingStyleOptions are internal to Roslyn repo. Is my assumption correct? If yes, could you please help define the approach to port NamingStyleOptions to RoslynAnalyzers?
Fixes dotnet/runtime#33796

dnfadmin · 2021-02-22T05:35:50Z

All CLA requirements met.

src/Test.Utilities/Test.Utilities.csproj

Youssef1313 · 2021-02-22T11:50:25Z

...ts/Microsoft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyNamesTests.cs

+               .WithArguments(arguments);
+
+        private DiagnosticResult CA2243BasicFieldResultAt(int line, int column, params string[] arguments)
+#pragma warning disable RS0030 // Do not used banned APIs


New tests shouldn't use banned APIs.

I've quickly tried to use WithLocation(markupKey) (not banned API) here instead but without luck yet - tests failed with System.InvalidOperationException : The markup location '#0' was not found in the input.. And I'm not sure how to fix it yet. It may be related to #624 in RoslynSDK issue. I'll come back to it later after adding a code fix.

@psxvoid Did you add the span where the diagnostic is expected between {|#0: and |}

Yep, you are right, I was missing those. Thank you, fixed.

src/Utilities/Compiler/WellKnownTypeNames.cs

Youssef1313

New analyzers shouldn't target master branch. Please rebase on 6.0.1xx-preview2 branch.

buyaa-n

Thank you @psxvoid, overall looks you are solving it well, left some comments

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

...rs/Core/Microsoft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyNames.cs

...etAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SerializationRulesDiagnosticAnalyzer.cs

src/Test.Utilities/AdditionalMetadataReferences.cs

src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs

...ts/Microsoft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyNamesTests.cs

src/NetAnalyzers/UnitTests/Microsoft.CodeAnalysis.NetAnalyzers.UnitTests.csproj

codecov · 2021-02-25T11:31:56Z

Codecov Report

Merging #4877 (aafe0de) into main (96ae2f9) will decrease coverage by 0.07%.
The diff coverage is 98.88%.

@@            Coverage Diff             @@
##             main    #4877      +/-   ##
==========================================
- Coverage   95.69%   95.61%   -0.08%     
==========================================
  Files        1197     1204       +7     
  Lines      271784   277965    +6181     
  Branches    16433    16723     +290     
==========================================
+ Hits       260078   265774    +5696     
- Misses       9599     9996     +397     
- Partials     2107     2195      +88

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

Youssef1313 · 2021-02-25T11:45:44Z

...ts/Microsoft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyNamesTests.cs

+                    Public Sub New(firstIField as Integer, secondIField as Object)
+                        Me.firstField = firstIField
+                        Me.secondField = secondIField


Consider adding another test without 'Me' to ensure proper codefix behavior when it's implemented.

Is it something that can happen? I haven't written a code fix yet, but I certainly will add another test if it something that can go wrong.

...rs/Core/Microsoft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyNames.cs

Youssef1313 · 2021-02-26T08:09:17Z

...rs/Core/Microsoft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyNames.cs

+        private sealed class ParameterAnalyzer
+        {
+            private readonly INamedTypeSymbol _jsonConstructorAttributeInfoType;
+
+            public ParameterAnalyzer(INamedTypeSymbol jsonConstructorAttributeInfoType)
+            {
+                _jsonConstructorAttributeInfoType = jsonConstructorAttributeInfoType;
+            }


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.

Youssef1313 · 2021-02-26T08:11:58Z

...rs/Core/Microsoft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyNames.cs

+            {
+                var operation = (IParameterReferenceOperation)context.Operation;
+
+                if (operation.Parent is not IAssignmentOperation assignment)


This is likely to fail for tuple assignments (deconstruction assignment) like (_x, _y) = (x, y);

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, JsonConstructor will throw a runtime exception. That's why diagnostic must still be reported anyway for this case.

@psxvoid The tests in that commit are no diagnostic tests. I meant to confirm that diagnostic cases are working correctly for tuple assignments.

Yep, sorry, it doesn't work with tuple assignments. I'll come back to it a bit later.

Done. Though, I'm not sure whether it's possible in VB.

Youssef1313 · 2021-06-28T07:32:57Z

New analyzers shouldn't target master branch. Please rebase on 6.0.1xx-preview2 branch.

You're still targeting main. It should target release/6.0.1xx at this time.

psxvoid · 2021-06-28T07:44:34Z

New analyzers shouldn't target master branch. Please rebase on 6.0.1xx-preview2 branch.

You're still targeting main. It should target release/6.0.1xx at this time.

I'm aware of it. I want to finish some fixes and cleanups, implement some missing features; and then create another non-draft PR to avoid force-push and preserve review comments. But thank you for helping to find the latest branch, very appreciated.

Youssef1313 · 2021-06-28T08:04:57Z

...Microsoft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyAndFieldNames.cs

+            context.EnableConcurrentExecution();
+            context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
+
+            context.RegisterCompilationStartAction((context) =>


nit: parentheses are not needed.

Suggested change

context.RegisterCompilationStartAction((context) =>

context.RegisterCompilationStartAction(context =>

Youssef1313 · 2021-06-28T08:05:38Z

...Microsoft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyAndFieldNames.cs

+
+            context.RegisterCompilationStartAction((context) =>
+            {
+                INamedTypeSymbol? jsonConstructorAttributeNamedSymbol = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemTextJsonSerializationJsonConstructorAttribute);


nit: Use context.Compilation.TryGetOrCreateTypeByMetadataName

Youssef1313 · 2021-07-08T07:33:07Z

...Microsoft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyAndFieldNames.cs

+                    .SetItem(ReferencedFieldOrPropertyNameKey, prop.Name)
+                    .SetItem(DiagnosticReasonKey, reason.ToString());


Since we know the two keys being added are unique.

Suggested change

.SetItem(ReferencedFieldOrPropertyNameKey, prop.Name)

.SetItem(DiagnosticReasonKey, reason.ToString());

.Add(ReferencedFieldOrPropertyNameKey, prop.Name)

.Add(DiagnosticReasonKey, reason.ToString());

Consider using constant strings instead of an enum, which avoids doing Enum.Parse in codefix, and also matches what's done in other analyzers.

Youssef1313 · 2021-07-08T07:36:15Z

...Microsoft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyAndFieldNames.cs

+                        diagnosticDescriptor,
+                        reason == ParameterDiagnosticReason.PropertyInappropriateVisibility ? prop.Locations : ImmutableArray<Location>.Empty,
+                        properties,
+                        param.ContainingType.ToDisplayString(SymbolDisplayFormats.ShortSymbolDisplayFormat),


Suggested change

param.ContainingType.ToDisplayString(SymbolDisplayFormats.ShortSymbolDisplayFormat),

param.ContainingType.Name,

Youssef1313 · 2021-07-08T07:38:45Z

...etCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyAndFieldNamesTests.Fixer.cs

+                using System.Text.Json.Serialization;
+
+                public class C1
+                {
+                    public int FirstProp { get; }
+
+                    public object SecondProp { get; }
+
+                    [JsonConstructor]
+                    public C1(int [|firstDrop|], object secondProp)
+                    {
+                        this.FirstProp = firstDrop;
+                        this.SecondProp = secondProp;
+                    }
+                }",


Same for all strings.

Suggested change

using System.Text.Json.Serialization;

public class C1

{

public int FirstProp { get; }

public object SecondProp { get; }

[JsonConstructor]

public C1(int [|firstDrop|], object secondProp)

{

this.FirstProp = firstDrop;

this.SecondProp = secondProp;

}

}",

using System.Text.Json.Serialization;

public class C1

{

public int FirstProp { get; }

public object SecondProp { get; }

[JsonConstructor]

public C1(int [|firstDrop|], object secondProp)

{

this.FirstProp = firstDrop;

this.SecondProp = secondProp;

}

}",

Youssef1313 · 2021-07-08T07:41:06Z

...soft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyAndFieldNamesTests.cs

+            await VerifyCSharpAnalyzerAsync(@"
+                using System.Text.Json.Serialization;
+
+                public class C1
+                {
+                    public int FirstProp { get; }
+                    public object SecondProp { get; }
+
+                    [JsonConstructor]
+                    public C1(int {|#0:firstDrop|}, object {|#1:secondDrop|})
+                    {
+                        this.FirstProp = firstDrop;
+                        this.SecondProp = secondDrop;
+                    }
+                }",
+                CA1071CSharpPropertyOrFieldResultAt(0, "C1", "firstDrop", "FirstProp"),
+                CA1071CSharpPropertyOrFieldResultAt(1, "C1", "secondDrop", "SecondProp"));


As long as the source has diagnostics, codefix should be tested, even if it's not offered.

Youssef1313 · 2021-07-08T07:42:00Z

...etCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyAndFieldNamesTests.Fixer.cs

+
+namespace Microsoft.NetCore.Analyzers.Runtime.UnitTests
+{
+    public class ConstructorParametersShouldMatchPropertyAndFieldNamesFixerTests


⚠️ ⚠️ There shouldn't be separate test files for analyzer and codefix.

Why so? It seems like a lot of analyzers have separate files for an analyzer and a code fix. For example:

TestForEmptyStringsUsingStringLengthTests
TestForEmptyStringsUsingStringLengthTests.Fixer

UseOrdinalStringComparisonTests
UseOrdinalStringComparisonTests.Fixer

MarkAllNonSerializableFieldsTests
MarkAllNonSerializableFieldsTests.Fixer
etc.

For me it's OK to have separate files, especially for the scenarios where codefix is not suggested, testing no diagnostic /false positives scenarios.

Though for the scenarios that has a codefix do not need to repeat the test in the analyzers test and codefix test. For example I see you have a same test in analyzer and code fix:

[Fact] public async Task CA1071_ClassPropsDoNotMatch_ConstructorParametersShouldMatchPropertyNames_CSharp() { await VerifyCSharpAnalyzerAsync(@" using System.Text.Json.Serialization; public class C1 { public int FirstProp { get; } public object SecondProp { get; } [JsonConstructor] public C1(int {|#0:firstDrop|}, object {|#1:secondDrop|}) { this.FirstProp = firstDrop; this.SecondProp = secondDrop; } }", CA1071CSharpPropertyOrFieldResultAt(0, "C1", "firstDrop", "FirstProp"), CA1071CSharpPropertyOrFieldResultAt(1, "C1", "secondDrop", "SecondProp")); } ``` And in code fix tests: ```cs [Fact] public async Task CA1071_ClassSinglePropDoesNotMatch_CSharp() { await VerifyCSharpCodeFixAsync(@" using System.Text.Json.Serialization; public class C1 { public int FirstProp { get; } public object SecondProp { get; } [JsonConstructor] public C1(int [|firstDrop|], object secondProp) { this.FirstProp = firstDrop; this.SecondProp = secondProp; } }", @" using System.Text.Json.Serialization; public class C1 { public int FirstProp { get; } public object SecondProp { get; } [JsonConstructor] public C1(int firstProp, object secondProp) { this.FirstProp = firstProp; this.SecondProp = secondProp; } }"); } ``` These should be just in code fix tests

Youssef1313 · 2021-07-08T07:45:02Z

...Microsoft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyAndFieldNames.cs

+                        diagnosticDescriptor,
+                        reason == ParameterDiagnosticReason.FieldInappropriateVisibility ? field.Locations : ImmutableArray<Location>.Empty,
+                        properties,
+                        param.ContainingType.ToDisplayString(SymbolDisplayFormats.ShortSymbolDisplayFormat),


Suggested change

param.ContainingType.ToDisplayString(SymbolDisplayFormats.ShortSymbolDisplayFormat),

param.ContainingType.Name,

Youssef1313 · 2021-07-08T07:45:16Z

...Microsoft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyAndFieldNames.cs

+                    .SetItem(ReferencedFieldOrPropertyNameKey, field.Name)
+                    .SetItem(DiagnosticReasonKey, reason.ToString());


Suggested change

.SetItem(ReferencedFieldOrPropertyNameKey, field.Name)

.SetItem(DiagnosticReasonKey, reason.ToString());

.Add(ReferencedFieldOrPropertyNameKey, field.Name)

.Add(DiagnosticReasonKey, reason.ToString());

Youssef1313 · 2021-07-08T07:48:02Z

...Microsoft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyAndFieldNames.cs

+                INamedTypeSymbol? jsonConstructorAttributeNamedSymbol = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemTextJsonSerializationJsonConstructorAttribute);
+                if (jsonConstructorAttributeNamedSymbol == null)


Suggested change

INamedTypeSymbol? jsonConstructorAttributeNamedSymbol = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemTextJsonSerializationJsonConstructorAttribute);

if (jsonConstructorAttributeNamedSymbol == null)

if (!context.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemTextJsonSerializationJsonConstructorAttribute, out var jsonConstructorAttributeNamedSymbol))

...etCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyAndFieldNamesTests.Fixer.cs

buyaa-n · 2023-05-08T17:02:22Z

Hey @psxvoid, what is left for this analyzer? Could you finish the remaining work?

Or if this is done and ready for review, please resolve conflicts and mark it as ready for review. Thanks

psxvoid · 2023-05-09T05:09:33Z

Hi @buyaa-n

As far as I remember, it was almost done but it's stuck on the code review phase. I haven't understood why There shouldn't be separate test files for analyzer and codefix while many other analyzers do have separate ones. And I've never got a reply.

buyaa-n · 2023-05-09T17:26:49Z

I haven't understood why #4877 (comment) while many other analyzers do have separate ones.

It's OK to have a separate file, but please do not add repeated tests in both files, I

As far as I remember, it was almost done but it's stuck on the code review phase.

Please merge conflicts and mark it ready for review if its done, we don't normally review draft PRs

psxvoid · 2023-05-10T10:10:13Z

It's OK to have a separate file, but please do not add repeated tests in both files
Please merge conflicts and mark it ready for review if its done, we don't normally review draft PRs

Got it. Thanks a lot!

Add ConstructorParamShouldMatchPropNames analyzer

3dedbb2

psxvoid requested a review from a team as a code owner February 22, 2021 05:35

psxvoid commented Feb 22, 2021

View reviewed changes

src/Test.Utilities/Test.Utilities.csproj Outdated Show resolved Hide resolved

psxvoid marked this pull request as draft February 22, 2021 06:13

Youssef1313 reviewed Feb 22, 2021

View reviewed changes

src/Utilities/Compiler/WellKnownTypeNames.cs Outdated Show resolved Hide resolved

Youssef1313 suggested changes Feb 22, 2021

View reviewed changes

buyaa-n reviewed Feb 24, 2021

View reviewed changes

psxvoid added 8 commits February 25, 2021 13:59

Fix - remove unused package reference

6dc3ef9

Fix json-ctor order in well-known-names

6b71577

Fix code alignment in tests

52360bb

Mark the diagnostic as non-fx-cop-ported

d9308a4

Add missing msbuild autogenerated assets

700c4ac

Fix - remove unused package reference, part 2

d3769d6

Update rule's message

5c82803

Rollback unrelated analyzer to the original state

43ad9b1

Youssef1313 reviewed Feb 25, 2021

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved

Youssef1313 reviewed Feb 25, 2021

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved

Youssef1313 reviewed Feb 25, 2021

View reviewed changes

...rs/Core/Microsoft.NetCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyNames.cs Outdated Show resolved Hide resolved

psxvoid added 6 commits February 26, 2021 09:20

Update the description message for the analyzer

41d44b2

Fix misplaced comma and update the message

e3db946

Fix incorrect helper test method name prefix

e9bc939

Update the message title with the recommended one

481f5ed

Use net50 references instead netstandard20 in tests

93c0429

Replace IsJsonCtor extension-method with private method

3efee13

Youssef1313 reviewed Feb 26, 2021

View reviewed changes

Fixer - inline ExportFixProvider and Shared attributes (fixed)

8dacfcb

Analyzer - use single diagnostic rule per analyzer

ab492ca

Youssef1313 reviewed Jun 28, 2021

View reviewed changes

psxvoid added 2 commits July 8, 2021 10:26

CodeFix - make bound field public (no message, not aligned)

b1deb98

Analyzer - remove unreferenced parameter diagnostic reason

2008bfe

Youssef1313 reviewed Jul 8, 2021

View reviewed changes

psxvoid added 2 commits July 13, 2021 06:06

Analyzer - fix broken tests due to additional diagnostic location

194c3a2

CodeFix - add tests for single private property

a487621

psxvoid commented Jul 13, 2021

View reviewed changes

...etCore.Analyzers/Runtime/ConstructorParametersShouldMatchPropertyAndFieldNamesTests.Fixer.cs Show resolved Hide resolved

psxvoid added 5 commits July 13, 2021 06:45

CodeFix - add tests for multiple private fields and props

f38390d

Analyzer - use "Add" instead of "SetItem"

051d067

Analyzer - init jsonConstructorAttribute symbol using out var

f86bf43

Analyzer - Use ContainingType.Name instead of ToDisplayString

35b2346

Analyzer - Remove parentheses from lambda param while passing context

aafe0de

buyaa-n mentioned this pull request Nov 16, 2022

.NET 8 developers can verify more APIs for correct usage to speed up their development dotnet/runtime#78442

Closed

buyaa-n mentioned this pull request Jul 5, 2023

Constructor parameters should match property names dotnet/runtime#33796

Open

	context.RegisterCompilationStartAction((context) =>
	context.RegisterCompilationStartAction(context =>

		.SetItem(ReferencedFieldOrPropertyNameKey, prop.Name)
		.SetItem(DiagnosticReasonKey, reason.ToString());

	param.ContainingType.ToDisplayString(SymbolDisplayFormats.ShortSymbolDisplayFormat),
	param.ContainingType.Name,

		INamedTypeSymbol? jsonConstructorAttributeNamedSymbol = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemTextJsonSerializationJsonConstructorAttribute);
		if (jsonConstructorAttributeNamedSymbol == null)

	INamedTypeSymbol? jsonConstructorAttributeNamedSymbol = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemTextJsonSerializationJsonConstructorAttribute);
	if (jsonConstructorAttributeNamedSymbol == null)
	if (!context.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemTextJsonSerializationJsonConstructorAttribute, out var jsonConstructorAttributeNamedSymbol))

Add ConstructorParamShouldMatchPropNames analyzer #4877

Are you sure you want to change the base?

Add ConstructorParamShouldMatchPropNames analyzer #4877

Conversation

psxvoid commented Feb 22, 2021 • edited by buyaa-n Loading

dnfadmin commented Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 left a comment

Choose a reason for hiding this comment

buyaa-n left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 25, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 Feb 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psxvoid Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

Youssef1313 commented Jun 28, 2021

psxvoid commented Jun 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psxvoid Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buyaa-n commented May 8, 2023

psxvoid commented May 9, 2023

buyaa-n commented May 9, 2023

psxvoid commented May 10, 2023 • edited Loading

psxvoid commented Feb 22, 2021 •

edited by buyaa-n

Loading

dnfadmin commented Feb 22, 2021 •

edited

Loading

codecov bot commented Feb 25, 2021 •

edited

Loading

Youssef1313 Feb 26, 2021 •

edited

Loading

psxvoid Mar 4, 2021 •

edited

Loading

psxvoid Jul 27, 2021 •

edited

Loading

psxvoid commented May 10, 2023 •

edited

Loading