Code review

SonarSource · Feb 23, 2023 · 9089b70 · 9089b70
1 parent cc24bfa
commit 9089b70
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 33 deletions.
diff --git 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<DiagnosticDescriptor> 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
@@ -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";
     }