diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBePrivateAndReadonly.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBePrivateAndReadonly.cs index b5d73f011e8..7137fc7773a 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBePrivateAndReadonly.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBePrivateAndReadonly.cs @@ -30,40 +30,44 @@ public sealed class LockedFieldShouldBePrivateAndReadonly : SonarDiagnosticAnaly public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); protected override void Initialize(SonarAnalysisContext context) => - context.RegisterNodeAction(c => + context.RegisterNodeAction(CheckLockStatement, SyntaxKind.LockStatement); + + private static void CheckLockStatement(SonarSyntaxNodeReportingContext context) + { + var expression = ((LockStatementSyntax)context.Node).Expression?.RemoveParentheses(); + if (IsCreation(expression)) + { + ReportIssue("Locking on a new instance is a no-op."); + } + else + { + var symbol = context.SemanticModel.GetSymbolInfo(expression).Symbol; + if (IsOfTypeString(expression, symbol)) { - var expression = ((LockStatementSyntax)c.Node).Expression?.RemoveParentheses(); - if (IsCreation(expression)) - { - ReportIssue("Locking on a new instance is a no-op."); - } - else if (IsOfTypeString(c.SemanticModel, expression)) - { - ReportIssue("Strings can be interned, and should not be used for locking."); - } - else if (expression is IdentifierNameSyntax - && c.SemanticModel.GetSymbolInfo(expression).Symbol is ILocalSymbol lockedSymbol) + ReportIssue("Strings can be interned, and should not be used for locking."); + } + else if (expression is IdentifierNameSyntax && symbol is ILocalSymbol lockedSymbol) + { + ReportIssue($"'{lockedSymbol.Name}' is a local variable, and should not be used for locking."); + } + else if (expression is (IdentifierNameSyntax or MemberAccessExpressionSyntax) + && symbol is IFieldSymbol lockedField + && (!lockedField.IsReadOnly || lockedField.GetEffectiveAccessibility() != Accessibility.Private)) + { + if (lockedField.ContainingType is { } lockedFieldType && context.ContainingSymbol?.ContainingType is { } containingType && !lockedFieldType.Equals(containingType)) { - ReportIssue($"'{lockedSymbol.Name}' is a local variable, and should not be used for locking."); + ReportIssue($"Use members from '{containingType.ToMinimalDisplayString(context.SemanticModel, expression.SpanStart)}' for locking."); } - else if (expression is (IdentifierNameSyntax or MemberAccessExpressionSyntax) - && c.SemanticModel.GetSymbolInfo(expression).Symbol is IFieldSymbol lockedField - && (!lockedField.IsReadOnly || lockedField.GetEffectiveAccessibility() != Accessibility.Private)) + else { - if (lockedField.ContainingType is { } lockedFieldType && c.ContainingSymbol?.ContainingType is { } containingType && !lockedFieldType.Equals(containingType)) - { - ReportIssue($"Use fields from '{containingType.ToMinimalDisplayString(c.SemanticModel, expression.SpanStart)}' for locking."); - } - else - { - ReportIssue($"'{lockedField.Name}' is not 'private readonly', and should not be used for locking."); - } + ReportIssue($"'{lockedField.Name}' is not 'private readonly', and should not be used for locking."); } + } + } - void ReportIssue(string message) => - c.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), message)); - }, - SyntaxKind.LockStatement); + void ReportIssue(string message) => + context.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), message)); + } private static bool IsCreation(ExpressionSyntax expression) => expression is ObjectCreationExpressionSyntax @@ -72,7 +76,7 @@ or ArrayCreationExpressionSyntax or ImplicitArrayCreationExpressionSyntax or QueryExpressionSyntax; - private static bool IsOfTypeString(SemanticModel model, ExpressionSyntax expression) => + private static bool IsOfTypeString(ExpressionSyntax expression, ISymbol symbol) => expression.IsAnyKind(SyntaxKind.StringLiteralExpression, SyntaxKind.InterpolatedStringExpression) - || (model.GetTypeInfo(expression).Type is { } type && type.Is(KnownType.System_String)); + || (symbol.GetSymbolType() is { } type && type.Is(KnownType.System_String)); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.cs index 92419aabc29..f8d713321c5 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.cs @@ -83,9 +83,9 @@ void OnANewInstance() void OnAStringInstance() { lock ("a string") { } // Noncompliant {{Strings can be interned, and should not be used for locking.}} - lock ($"an interpolated {"string"}") { } // Noncompliant {{Strings can be interned, and should not be used for locking.}} - lock ("a" + "string") { } // Noncompliant {{Strings can be interned, and should not be used for locking.}} - lock (MethodReturningString()) { } // Noncompliant {{Strings can be interned, and should not be used for locking.}} + lock ($"an interpolated {"string"}") { } // Noncompliant + lock ("a" + "string") { } // Noncompliant + lock (MethodReturningString()) { } // Noncompliant string MethodReturningString() => "a string"; }