From e65f78dcb7e6b9e37098f4d4e3ca478ad724913f Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Tue, 24 Oct 2023 17:33:22 +0200 Subject: [PATCH 1/7] First part: redesign and initial support --- .../Rules/VariableShadowsField.cs | 116 +++++++++++------- .../VariableShadowsField.CSharp12.cs | 32 +++-- 2 files changed, 94 insertions(+), 54 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs index fc337bb8043..213ea22d0c0 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs @@ -28,75 +28,97 @@ public sealed class VariableShadowsField : SonarDiagnosticAnalyzer private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat); + private static readonly SyntaxKind[] TypesWithPrimaryConstructorDeclarations = + { + SyntaxKind.ClassDeclaration, + SyntaxKind.StructDeclaration, + SyntaxKindEx.RecordClassDeclaration, + SyntaxKindEx.RecordStructDeclaration + }; + public override ImmutableArray 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); - }, + 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); + context.RegisterNodeAction(c => Process(c, c.Node), 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 void ProcessVariableDesignation(SonarSyntaxNodeReportingContext context, VariableDesignationSyntaxWrapper variableDesignation) + private static SyntaxNode GetDeclarationOrDesignation(SyntaxNode node) => + node switch + { + LocalDeclarationStatementSyntax localDeclaration => localDeclaration.Declaration, + ForStatementSyntax forStatement => forStatement.Declaration, + UsingStatementSyntax usingStatement => usingStatement.Declaration, + FixedStatementSyntax fixedStatement => fixedStatement.Declaration, + _ 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 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 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 ImmutableArray GetContextSymbols(SonarSyntaxNodeReportingContext context) + { + var symbols = context.ContainingSymbol.ContainingSymbol.GetSymbolType().GetMembers(); + var relevantSymbols = symbols.Where(x => x is IPropertySymbol or IFieldSymbol).ToImmutableArray(); + var primaryCtor = symbols.FirstOrDefault(x => x is IMethodSymbol && IsPrimaryCtor(x)); + var primaryCtorParameters = primaryCtor?.GetParameters(); + return primaryCtor is null ? relevantSymbols : relevantSymbols.AddRange(primaryCtorParameters); } + private static bool IsPrimaryCtor(ISymbol methodSymbol) => + methodSymbol.IsConstructor() + && methodSymbol.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is { } syntax + && TypesWithPrimaryConstructorDeclarations.Contains(syntax.Kind()); + private static void ReportOnVariableMatchingField(SonarSyntaxNodeReportingContext context, IEnumerable 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 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/TestCases/VariableShadowsField.CSharp12.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs index 1de9ae91e50..33c91f20684 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs @@ -1,11 +1,29 @@ -class MyClass(int first, int second, int third, int fourth) -{ - private int first = first; // Compliant - private int second; // FN +using System; - void MyMethod() +class MyParentClass(int parentClassParam) +{ + class MyClass(int a, int b, int c, int d, int e, int f, int g) { - int third = 1; // FN - _ = first is object fourth; // FN + private int a = a; // Compliant + private int b; // FN + private int c { get; set; } = c; // Compliant + private int d { get; set; } // FN + + class MyChildClass(int childClassParam) { } + + MyClass(int a, int b) : this(a, b, 1, 1, 1, 1, 1) { } // Compliant + + void MyMethod() + { + int parentClassParam = 42; // Compliant + int childClassParam = 42; // Compliant + int ctorParam = 42; // Compliant + + int e = 42; // Noncompliant {{Rename 'e' which hides the primary constructor parameter with the same name.}} + _ = a is object f; // Noncompliant + for (int g = 0; g < 10; g++) { } // Noncompliant + for (int a = 0; a < 10; a++) { } // Noncompliant FP + var lambda = (int g = 1) => g; // Compliant + } } } From aed918c32e7db5d9260c4ccc5801ebedf923f900 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Tue, 24 Oct 2023 19:26:40 +0200 Subject: [PATCH 2/7] Add partial classes unit tests --- .../Rules/VariableShadowsFieldTest.cs | 6 ++++++ .../VariableShadowsField_PartialClass1.cs | 7 +++++++ .../VariableShadowsField_PartialClass2.cs | 19 +++++++++++++++++++ 3 files changed, 32 insertions(+) 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/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_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 + // ^ + } +} From dd66875b1c44a172d2f262162834103c065d3b9e Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Tue, 24 Oct 2023 19:27:15 +0200 Subject: [PATCH 3/7] Fix FP on assignments --- .../Rules/VariableShadowsField.cs | 39 +++++++++++++++++-- .../VariableShadowsField.CSharp12.cs | 26 +++++++------ 2 files changed, 51 insertions(+), 14 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs index 213ea22d0c0..92dd53faa09 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs @@ -90,13 +90,46 @@ _ when VariableDesignationSyntaxWrapper.IsInstance(node) => ((VariableDesignatio _ => new() }; - private static ImmutableArray GetContextSymbols(SonarSyntaxNodeReportingContext context) + private static List GetContextSymbols(SonarSyntaxNodeReportingContext context) { var symbols = context.ContainingSymbol.ContainingSymbol.GetSymbolType().GetMembers(); - var relevantSymbols = symbols.Where(x => x is IPropertySymbol or IFieldSymbol).ToImmutableArray(); var primaryCtor = symbols.FirstOrDefault(x => x is IMethodSymbol && IsPrimaryCtor(x)); var primaryCtorParameters = primaryCtor?.GetParameters(); - return primaryCtor is null ? relevantSymbols : relevantSymbols.AddRange(primaryCtorParameters); + var relevantSymbols = new List(); + foreach (var relevant in symbols.Where(x => x is IPropertySymbol or IFieldSymbol)) + { + if (primaryCtorParameters is not null + && primaryCtorParameters.Any() + && TryGetAssignedValue(relevant, out var assignedValue) + && primaryCtorParameters.Any(x => x.Name == assignedValue)) + { + primaryCtorParameters = primaryCtorParameters.Where(x => x.Name != assignedValue).ToImmutableArray(); + } + else + { + relevantSymbols.Add(relevant); + } + } + return primaryCtor is null ? relevantSymbols : relevantSymbols.Concat(primaryCtorParameters).ToList(); + } + + private static bool TryGetAssignedValue(ISymbol symbol, out string assignedValue) + { + assignedValue = string.Empty; + if (symbol.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is { } syntax) + { + if (syntax is VariableDeclaratorSyntax variableDeclarator && variableDeclarator.Initializer is { Value: IdentifierNameSyntax { } fieldValue }) + { + assignedValue = fieldValue.Identifier.ValueText; + return true; + } + else if (syntax is PropertyDeclarationSyntax propertyDeclaration && propertyDeclaration.Initializer is { Value: IdentifierNameSyntax { } propertyValue }) + { + assignedValue = propertyValue.Identifier.ValueText; + return true; + } + } + return false; } private static bool IsPrimaryCtor(ISymbol methodSymbol) => diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs index 33c91f20684..09ba5810b95 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs @@ -2,16 +2,19 @@ class MyParentClass(int parentClassParam) { - class MyClass(int a, int b, int c, int d, int e, int f, int g) + class MyClass(int a, int b, int c, int d, int e, int f, int g, int h) { - private int a = a; // Compliant - private int b; // FN - private int c { get; set; } = c; // Compliant - private int d { get; set; } // FN + 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 a, int b) : this(a, b, 1, 1, 1, 1, 1) { } // Compliant + MyClass(int ctorParam, int f) : this(ctorParam, f, 1, 1, 1, 1, 1, 1) { } // Compliant void MyMethod() { @@ -19,11 +22,12 @@ void MyMethod() int childClassParam = 42; // Compliant int ctorParam = 42; // Compliant - int e = 42; // Noncompliant {{Rename 'e' which hides the primary constructor parameter with the same name.}} - _ = a is object f; // Noncompliant - for (int g = 0; g < 10; g++) { } // Noncompliant - for (int a = 0; a < 10; a++) { } // Noncompliant FP - var lambda = (int g = 1) => g; // Compliant + 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 + for (int a = 0; a < 10; a++) { } // Compliant + for (int c = 0; c < 10; c++) { } // Compliant + var lambda = (int h = 1) => h; // Compliant } } } From 4e691423bc19402e996078f540c591b22bed0db6 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Wed, 25 Oct 2023 12:16:26 +0200 Subject: [PATCH 4/7] Address first round of comments --- .../Rules/VariableShadowsField.cs | 48 +++---------------- .../VariableShadowsField.CSharp12.cs | 12 +++-- 2 files changed, 13 insertions(+), 47 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs index 92dd53faa09..f56f6d4d12e 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs @@ -38,8 +38,7 @@ public sealed class VariableShadowsField : SonarDiagnosticAnalyzer public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); - protected override void Initialize(SonarAnalysisContext context) - { + protected override void Initialize(SonarAnalysisContext context) => context.RegisterNodeAction(c => Process(c, GetDeclarationOrDesignation(c.Node)), SyntaxKind.LocalDeclarationStatement, SyntaxKind.ForStatement, @@ -49,10 +48,8 @@ protected override void Initialize(SonarAnalysisContext context) SyntaxKindEx.RecursivePattern, SyntaxKindEx.VarPattern, SyntaxKindEx.DeclarationPattern, - SyntaxKindEx.ListPattern); - context.RegisterNodeAction(c => Process(c, c.Node), + SyntaxKindEx.ListPattern, SyntaxKind.ForEachStatement); - } private static SyntaxNode GetDeclarationOrDesignation(SyntaxNode node) => node switch @@ -61,6 +58,7 @@ private static SyntaxNode GetDeclarationOrDesignation(SyntaxNode node) => 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, @@ -92,44 +90,10 @@ _ when VariableDesignationSyntaxWrapper.IsInstance(node) => ((VariableDesignatio private static List GetContextSymbols(SonarSyntaxNodeReportingContext context) { - var symbols = context.ContainingSymbol.ContainingSymbol.GetSymbolType().GetMembers(); + var symbols = ((INamespaceOrTypeSymbol)context.ContainingSymbol.ContainingSymbol).GetMembers(); var primaryCtor = symbols.FirstOrDefault(x => x is IMethodSymbol && IsPrimaryCtor(x)); - var primaryCtorParameters = primaryCtor?.GetParameters(); - var relevantSymbols = new List(); - foreach (var relevant in symbols.Where(x => x is IPropertySymbol or IFieldSymbol)) - { - if (primaryCtorParameters is not null - && primaryCtorParameters.Any() - && TryGetAssignedValue(relevant, out var assignedValue) - && primaryCtorParameters.Any(x => x.Name == assignedValue)) - { - primaryCtorParameters = primaryCtorParameters.Where(x => x.Name != assignedValue).ToImmutableArray(); - } - else - { - relevantSymbols.Add(relevant); - } - } - return primaryCtor is null ? relevantSymbols : relevantSymbols.Concat(primaryCtorParameters).ToList(); - } - - private static bool TryGetAssignedValue(ISymbol symbol, out string assignedValue) - { - assignedValue = string.Empty; - if (symbol.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is { } syntax) - { - if (syntax is VariableDeclaratorSyntax variableDeclarator && variableDeclarator.Initializer is { Value: IdentifierNameSyntax { } fieldValue }) - { - assignedValue = fieldValue.Identifier.ValueText; - return true; - } - else if (syntax is PropertyDeclarationSyntax propertyDeclaration && propertyDeclaration.Initializer is { Value: IdentifierNameSyntax { } propertyValue }) - { - assignedValue = propertyValue.Identifier.ValueText; - return true; - } - } - return false; + var relevantSymbols = symbols.Where(x => x is IPropertySymbol or IFieldSymbol).ToList(); + return primaryCtor is null ? relevantSymbols : relevantSymbols.Concat(primaryCtor.GetParameters()).ToList(); } private static bool IsPrimaryCtor(ISymbol methodSymbol) => diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs index 09ba5810b95..fd890452f01 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs @@ -2,7 +2,7 @@ class MyParentClass(int parentClassParam) { - class MyClass(int a, int b, int c, int d, int e, int f, int g, int h) + 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 @@ -14,7 +14,7 @@ class MyClass(int a, int b, int c, int d, int e, int f, int g, int h) class MyChildClass(int childClassParam) { } - MyClass(int ctorParam, int f) : this(ctorParam, f, 1, 1, 1, 1, 1, 1) { } // Compliant + MyClass(int ctorParam, int f) : this(ctorParam, f, 1, 1, 1, 1, 1, 1, 1, 1) { } // Compliant void MyMethod() { @@ -24,10 +24,12 @@ 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 - for (int a = 0; a < 10; a++) { } // Compliant - for (int c = 0; c < 10; c++) { } // Compliant + 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 } } } From f70b23472f5e1b3288059ae30ad66f71eac40979 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Thu, 26 Oct 2023 15:53:42 +0200 Subject: [PATCH 5/7] Add reproducer for 8260 --- .../VariableShadowsField.CSharp12.cs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs index fd890452f01..1046e63080d 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/VariableShadowsField.CSharp12.cs @@ -33,3 +33,22 @@ void MyMethod() } } } + +// 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() + { + 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 + } +} From c23a507d4bd2a3855270ea246fd48e2e3e8de7c9 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Thu, 26 Oct 2023 15:59:15 +0200 Subject: [PATCH 6/7] Address comments --- .../Extensions/ISymbolExtensions.cs | 14 +++++++++++++ .../Rules/VariableShadowsField.cs | 21 ++++--------------- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Extensions/ISymbolExtensions.cs b/analyzers/src/SonarAnalyzer.CSharp/Extensions/ISymbolExtensions.cs index 2565b9ffd46..56252bfea87 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Extensions/ISymbolExtensions.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Extensions/ISymbolExtensions.cs @@ -20,11 +20,20 @@ using SonarAnalyzer.SymbolicExecution; using SonarAnalyzer.SymbolicExecution.Sonar; +using System.Linq; 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 +101,10 @@ public static ImmutableArray 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 f56f6d4d12e..c347124dad8 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/VariableShadowsField.cs @@ -28,14 +28,6 @@ public sealed class VariableShadowsField : SonarDiagnosticAnalyzer private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat); - private static readonly SyntaxKind[] TypesWithPrimaryConstructorDeclarations = - { - SyntaxKind.ClassDeclaration, - SyntaxKind.StructDeclaration, - SyntaxKindEx.RecordClassDeclaration, - SyntaxKindEx.RecordStructDeclaration - }; - public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); protected override void Initialize(SonarAnalysisContext context) => @@ -90,17 +82,12 @@ _ when VariableDesignationSyntaxWrapper.IsInstance(node) => ((VariableDesignatio private static List GetContextSymbols(SonarSyntaxNodeReportingContext context) { - var symbols = ((INamespaceOrTypeSymbol)context.ContainingSymbol.ContainingSymbol).GetMembers(); - var primaryCtor = symbols.FirstOrDefault(x => x is IMethodSymbol && IsPrimaryCtor(x)); - var relevantSymbols = symbols.Where(x => x is IPropertySymbol or IFieldSymbol).ToList(); - return primaryCtor is null ? relevantSymbols : relevantSymbols.Concat(primaryCtor.GetParameters()).ToList(); + 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 bool IsPrimaryCtor(ISymbol methodSymbol) => - methodSymbol.IsConstructor() - && methodSymbol.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is { } syntax - && TypesWithPrimaryConstructorDeclarations.Contains(syntax.Kind()); - private static void ReportOnVariableMatchingField(SonarSyntaxNodeReportingContext context, IEnumerable members, SyntaxToken identifier) { if (members.FirstOrDefault(x => x.Name == identifier.ValueText) is { } matchingMember) From d48852d8d2b8239ea3c5c01b7fd31888961b971d Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Thu, 26 Oct 2023 17:24:10 +0200 Subject: [PATCH 7/7] Remove unnecessary using --- .../src/SonarAnalyzer.CSharp/Extensions/ISymbolExtensions.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Extensions/ISymbolExtensions.cs b/analyzers/src/SonarAnalyzer.CSharp/Extensions/ISymbolExtensions.cs index 56252bfea87..b4a1e569097 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Extensions/ISymbolExtensions.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Extensions/ISymbolExtensions.cs @@ -20,7 +20,6 @@ using SonarAnalyzer.SymbolicExecution; using SonarAnalyzer.SymbolicExecution.Sonar; -using System.Linq; namespace SonarAnalyzer.Extensions {