From 388e7f8603eb15ebace64b4d4e8a35df638bf17d Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini <114916336+cristian-ambrosini-sonarsource@users.noreply.github.com> Date: Fri, 27 Oct 2023 09:03:15 +0200 Subject: [PATCH] Fix S1117 FN: support primary constructors (#8251) --- .../Extensions/ISymbolExtensions.cs | 13 +++ .../Rules/VariableShadowsField.cs | 104 +++++++++--------- .../Rules/VariableShadowsFieldTest.cs | 6 + .../VariableShadowsField.CSharp12.cs | 55 ++++++++- .../VariableShadowsField_PartialClass1.cs | 7 ++ .../VariableShadowsField_PartialClass2.cs | 19 ++++ 6 files changed, 149 insertions(+), 55 deletions(-) create mode 100644 analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField_PartialClass1.cs create mode 100644 analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField_PartialClass2.cs diff --git a/analyzers/src/SonarAnalyzer.CSharp/Extensions/ISymbolExtensions.cs b/analyzers/src/SonarAnalyzer.CSharp/Extensions/ISymbolExtensions.cs index 2565b9ffd46..b4a1e569097 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Extensions/ISymbolExtensions.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Extensions/ISymbolExtensions.cs @@ -25,6 +25,14 @@ namespace SonarAnalyzer.Extensions { internal static class ISymbolExtensions { + private static readonly SyntaxKind[] DeclarationsTypesWithPrimaryConstructor = + { + SyntaxKind.ClassDeclaration, + SyntaxKind.StructDeclaration, + SyntaxKindEx.RecordClassDeclaration, + SyntaxKindEx.RecordStructDeclaration + }; + public static bool HasConstraint(this ISymbol symbol, SymbolicConstraint constraint, ProgramState programState) { var symbolicValue = programState.GetSymbolValue(symbol); @@ -92,5 +100,10 @@ public static ImmutableArray<SyntaxToken> DeclaringReferenceIdentifiers(this ISy .Select(x => x.GetSyntax().GetIdentifier()) .WhereNotNull() .ToImmutableArray(); + + public static bool IsPrimaryConstructor(this ISymbol symbol) => + symbol.IsConstructor() + && symbol.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is { } syntax + && DeclarationsTypesWithPrimaryConstructor.Contains(syntax.Kind()); } } diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs index fc337bb8043..c347124dad8 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs @@ -30,73 +30,79 @@ public sealed class VariableShadowsField : SonarDiagnosticAnalyzer public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); - protected override void Initialize(SonarAnalysisContext context) - { - context.RegisterNodeAction(c => - { - var declaration = (ForEachStatementSyntax)c.Node; - - var variableSymbol = c.SemanticModel.GetDeclaredSymbol(declaration); - if (variableSymbol == null) - { - return; - } - - var members = GetMembers(variableSymbol.ContainingType); - - ReportOnVariableMatchingField(c, members, declaration.Identifier); - }, + protected override void Initialize(SonarAnalysisContext context) => + context.RegisterNodeAction(c => Process(c, GetDeclarationOrDesignation(c.Node)), + SyntaxKind.LocalDeclarationStatement, + SyntaxKind.ForStatement, + SyntaxKind.UsingStatement, + SyntaxKind.FixedStatement, + SyntaxKindEx.DeclarationExpression, + SyntaxKindEx.RecursivePattern, + SyntaxKindEx.VarPattern, + SyntaxKindEx.DeclarationPattern, + SyntaxKindEx.ListPattern, SyntaxKind.ForEachStatement); - context.RegisterNodeAction(c => ProcessVariableDeclaration(c, ((LocalDeclarationStatementSyntax)c.Node).Declaration), SyntaxKind.LocalDeclarationStatement); - context.RegisterNodeAction(c => ProcessVariableDeclaration(c, ((ForStatementSyntax)c.Node).Declaration), SyntaxKind.ForStatement); - context.RegisterNodeAction(c => ProcessVariableDeclaration(c, ((UsingStatementSyntax)c.Node).Declaration), SyntaxKind.UsingStatement); - context.RegisterNodeAction(c => ProcessVariableDeclaration(c, ((FixedStatementSyntax)c.Node).Declaration), SyntaxKind.FixedStatement); - - context.RegisterNodeAction(c => ProcessVariableDesignation(c, ((DeclarationExpressionSyntaxWrapper)c.Node).Designation), SyntaxKindEx.DeclarationExpression); - context.RegisterNodeAction(c => ProcessVariableDesignation(c, ((RecursivePatternSyntaxWrapper)c.Node).Designation), SyntaxKindEx.RecursivePattern); - context.RegisterNodeAction(c => ProcessVariableDesignation(c, ((VarPatternSyntaxWrapper)c.Node).Designation), SyntaxKindEx.VarPattern); - context.RegisterNodeAction(c => ProcessVariableDesignation(c, ((DeclarationPatternSyntaxWrapper)c.Node).Designation), SyntaxKindEx.DeclarationPattern); - context.RegisterNodeAction(c => ProcessVariableDesignation(c, ((ListPatternSyntaxWrapper)c.Node).Designation), SyntaxKindEx.ListPattern); - } + private static SyntaxNode GetDeclarationOrDesignation(SyntaxNode node) => + node switch + { + LocalDeclarationStatementSyntax localDeclaration => localDeclaration.Declaration, + ForStatementSyntax forStatement => forStatement.Declaration, + UsingStatementSyntax usingStatement => usingStatement.Declaration, + FixedStatementSyntax fixedStatement => fixedStatement.Declaration, + ForEachStatementSyntax forEachStatement => forEachStatement, + _ when DeclarationExpressionSyntaxWrapper.IsInstance(node) => ((DeclarationExpressionSyntaxWrapper)node).Designation, + _ when RecursivePatternSyntaxWrapper.IsInstance(node) => ((RecursivePatternSyntaxWrapper)node).Designation, + _ when VarPatternSyntaxWrapper.IsInstance(node) => ((VarPatternSyntaxWrapper)node).Designation, + _ when DeclarationPatternSyntaxWrapper.IsInstance(node) => ((DeclarationPatternSyntaxWrapper)node).Designation, + _ when ListPatternSyntaxWrapper.IsInstance(node) => ((ListPatternSyntaxWrapper)node).Designation, + _ => null + }; - private static void ProcessVariableDesignation(SonarSyntaxNodeReportingContext context, VariableDesignationSyntaxWrapper variableDesignation) + private static void Process(SonarSyntaxNodeReportingContext context, SyntaxNode node) { - if (variableDesignation.AllVariables() is { Length: > 0 } variables - && context.ContainingSymbol.ContainingType is { } containingType - && GetMembers(containingType) is var members) + if (ExtractIdentifiers(node) is { Count: > 0 } identifiers + && GetContextSymbols(context) is var members) { - foreach (var variable in variables) + foreach (var identifier in identifiers) { - ReportOnVariableMatchingField(context, members, variable.Identifier); + ReportOnVariableMatchingField(context, members, identifier); } } } - private static void ProcessVariableDeclaration(SonarSyntaxNodeReportingContext context, VariableDeclarationSyntax variableDeclaration) - { - if (variableDeclaration is { Variables: { Count: > 0 } variables } - && context.ContainingSymbol.ContainingType is { } containingType - && GetMembers(containingType) is var members) + private static List<SyntaxToken> ExtractIdentifiers(SyntaxNode node) => + node switch { - foreach (var variable in variables) - { - ReportOnVariableMatchingField(context, members, variable.Identifier); - } - } + VariableDeclarationSyntax variableDeclaration => variableDeclaration.Variables.Select(x => x.Identifier).ToList(), + ForEachStatementSyntax foreachStatement => new() { foreachStatement.Identifier }, + _ when VariableDesignationSyntaxWrapper.IsInstance(node) => ((VariableDesignationSyntaxWrapper)node).AllVariables().Select(x => x.Identifier).ToList(), + _ => new() + }; + + private static List<ISymbol> GetContextSymbols(SonarSyntaxNodeReportingContext context) + { + var members = context.ContainingSymbol.ContainingType.GetMembers(); + var primaryConstructorParameters = members.FirstOrDefault(x => x.IsPrimaryConstructor())?.GetParameters(); + var fieldsAndProperties = members.Where(x => x is IPropertySymbol or IFieldSymbol).ToList(); + return primaryConstructorParameters is null ? fieldsAndProperties : fieldsAndProperties.Concat(primaryConstructorParameters).ToList(); } private static void ReportOnVariableMatchingField(SonarSyntaxNodeReportingContext context, IEnumerable<ISymbol> members, SyntaxToken identifier) { - if (members.FirstOrDefault(m => m.Name == identifier.ValueText) is { } matchingMember) + if (members.FirstOrDefault(x => x.Name == identifier.ValueText) is { } matchingMember) { - context.ReportIssue(Diagnostic.Create(Rule, identifier.GetLocation(), identifier.Text, matchingMember is IFieldSymbol ? "field" : "property")); + context.ReportIssue(Diagnostic.Create(Rule, identifier.GetLocation(), identifier.Text, GetSymbolName(matchingMember))); } } - private static List<ISymbol> GetMembers(INamespaceOrTypeSymbol classSymbol) => - classSymbol.GetMembers() - .Where(member => member is IFieldSymbol or IPropertySymbol) - .ToList(); + private static string GetSymbolName(ISymbol symbol) => + symbol switch + { + IFieldSymbol => "field", + IPropertySymbol => "property", + IParameterSymbol => "primary constructor parameter", + _ => string.Empty + }; } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/VariableShadowsFieldTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/VariableShadowsFieldTest.cs index 2cb1455fa3a..d61f47ea824 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/VariableShadowsFieldTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/VariableShadowsFieldTest.cs @@ -60,6 +60,12 @@ public void VariableShadowsField_CSharp12() => .WithOptions(ParseOptionsHelper.FromCSharp12) .Verify(); + [TestMethod] + public void VariableShadowsField_PartialClass() => + builder.AddPaths("VariableShadowsField_PartialClass1.cs", "VariableShadowsField_PartialClass2.cs") + .WithOptions(ParseOptionsHelper.FromCSharp12) + .Verify(); + #endif } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs index 1de9ae91e50..1046e63080d 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs @@ -1,11 +1,54 @@ -class MyClass(int first, int second, int third, int fourth) +using System; + +class MyParentClass(int parentClassParam) { - private int first = first; // Compliant - private int second; // FN + class MyClass(int a, int b, int c, int d, int e, int f, int g, int h, int i, int j) + { + public int a = a; // Compliant + public int b; // FN + + public int c { get; set; } = c; // Compliant + public int d { get; set; } // FN + + public int g = a; // FN {{Rename 'g' which hides the primary constructor parameter with the same name.}} + + class MyChildClass(int childClassParam) { } + + MyClass(int ctorParam, int f) : this(ctorParam, f, 1, 1, 1, 1, 1, 1, 1, 1) { } // Compliant + + void MyMethod() + { + int parentClassParam = 42; // Compliant + int childClassParam = 42; // Compliant + int ctorParam = 42; // Compliant - void MyMethod() + int f = 42; // Noncompliant {{Rename 'f' which hides the primary constructor parameter with the same name.}} + _ = a is object g; // Noncompliant + for (int h = 0; h < 10; h++) { } // Noncompliant {{Rename 'h' which hides the primary constructor parameter with the same name.}} + for (int a = 0; a < 10; a++) { } // Noncompliant {{Rename 'a' which hides the field with the same name.}} + for (int c = 0; c < 10; c++) { } // Noncompliant {{Rename 'c' which hides the property with the same name.}} + var lambda = (int h = 1) => h; // Compliant + foreach (var (i, j) in new (int, int)[0]) { } // Noncompliant + // Noncompliant@-1 + } + } +} + +// Reproducer for https://github.com/SonarSource/sonar-dotnet/issues/8260 +class Repro_8260(int primaryCtorParam) +{ + private int instanceField = 1; + private int instanceProperty { get; set; } = 1; + private static int staticField = 1; + private static int staticProperty = 1; + + public static void StaticMethod() { - int third = 1; // FN - _ = first is object fourth; // FN + var instanceField = 2; // Noncompliant FP (instance field is not accessible from static method) + var instanceProperty = 2; // Noncompliant FP (instance property is not accessible from static method) + var primaryCtorParam = 2; // Noncompliant FP (primary ctor parameter is not accessible from static method) + + var staticField = 2; // Noncompliant + var staticProperty = 2; // Noncompliant } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField_PartialClass1.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField_PartialClass1.cs new file mode 100644 index 00000000000..eca17149b3b --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField_PartialClass1.cs @@ -0,0 +1,7 @@ +public partial class VariableShadowsField +{ + public int myField; + public int myProperty { get; set; } +} + +public partial class VariableShadowsFieldPrimaryConstructor(int a) { } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField_PartialClass2.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField_PartialClass2.cs new file mode 100644 index 00000000000..fdbf3e94fd3 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField_PartialClass2.cs @@ -0,0 +1,19 @@ +public partial class VariableShadowsField +{ + public void Method(object someParameter) + { + int myField = 0; // Noncompliant + // ^^^^^^^ + int myProperty = 0; // Noncompliant + // ^^^^^^^^^^ + } +} + +public partial class VariableShadowsFieldPrimaryConstructor +{ + void Method() + { + var a = 0; // Noncompliant + // ^ + } +}