Code review 2

analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBePrivateAndReadonly.cs

@@ -42,26 +42,19 @@ private static void CheckLockStatement(SonarSyntaxNodeReportingContext context)
         else
         {
             var symbol = context.SemanticModel.GetSymbolInfo(expression).Symbol;
-            if (IsOfTypeString(expression, symbol))
+            if (IsOfTypeString(context.SemanticModel, expression))
             {
                 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))
+            else if (FieldNotReadonlyOrNotPrivate(expression, symbol) is { } lockedField)
             {
-                if (lockedField.ContainingType is { } lockedFieldType && context.ContainingSymbol?.ContainingType is { } containingType && !lockedFieldType.Equals(containingType))
-                {
-                    ReportIssue($"Use members from '{containingType.ToMinimalDisplayString(context.SemanticModel, expression.SpanStart)}' for locking.");
-                }
-                else
-                {
-                    ReportIssue($"'{lockedField.Name}' is not 'private readonly', and should not be used for locking.");
-                }
+                ReportIssue(FieldInSameTypeAs(lockedField, context.ContainingSymbol?.ContainingType) is { } containingType
+                    ? $"Use members from '{containingType.ToMinimalDisplayString(context.SemanticModel, expression.SpanStart)}' for locking."
+                    : $"'{lockedField.Name}' is not 'private readonly', and should not be used for locking.");
             }
         }
 
@@ -70,13 +63,26 @@ private static void CheckLockStatement(SonarSyntaxNodeReportingContext context)
     }
 
     private static bool IsCreation(ExpressionSyntax expression) =>
-        expression is ObjectCreationExpressionSyntax
-            or AnonymousObjectCreationExpressionSyntax
-            or ArrayCreationExpressionSyntax
-            or ImplicitArrayCreationExpressionSyntax
-            or QueryExpressionSyntax;
+        expression.IsAnyKind(
+            SyntaxKind.ObjectCreationExpression,
+            SyntaxKind.AnonymousObjectCreationExpression,
+            SyntaxKind.ArrayCreationExpression,
+            SyntaxKind.ImplicitArrayCreationExpression,
+            SyntaxKind.QueryExpression);
 
-    private static bool IsOfTypeString(ExpressionSyntax expression, ISymbol symbol) =>
+    private static bool IsOfTypeString(SemanticModel model, ExpressionSyntax expression) =>
         expression.IsAnyKind(SyntaxKind.StringLiteralExpression, SyntaxKind.InterpolatedStringExpression)
-            || (symbol.GetSymbolType() is { } type && type.Is(KnownType.System_String));
+            || expression.IsKnownType(KnownType.System_String, model);
+
+    private static IFieldSymbol FieldNotReadonlyOrNotPrivate(ExpressionSyntax expression, ISymbol symbol) =>
+        expression.IsAnyKind(SyntaxKind.IdentifierName, SyntaxKind.SimpleMemberAccessExpression)
+            && symbol is IFieldSymbol lockedField
+            && (!lockedField.IsReadOnly || lockedField.GetEffectiveAccessibility() != Accessibility.Private)
+                ? lockedField
+                : null;
+
+    private static ITypeSymbol FieldInSameTypeAs(IFieldSymbol field, INamedTypeSymbol type) =>
+        field.ContainingType is { } fieldType && type is { } && !fieldType.Equals(type)
+            ? type
+            : null;
 }

analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldShouldBePrivateAndReadonlyTest.cs

@@ -45,6 +45,14 @@ public class LockedFieldShouldBePrivateAndReadonlyTest
             .WithOptions(ParseOptionsHelper.FromCSharp9)
             .Verify();
 
+    [TestMethod]
+    public void LockedFieldShouldBePrivateAndReadonly_CSharp9_TopLevelStatements() =>
+        builder
+            .AddPaths("LockedFieldShouldBePrivateAndReadonly.CSharp9.TopLevelStatements.cs")
+            .WithOptions(ParseOptionsHelper.FromCSharp9)
+            .WithTopLevelStatements()
+            .Verify();
+
     [TestMethod]
     public void LockedFieldShouldBePrivateAndReadonly_CSharp11() =>
         builder

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp9.TopLevelStatements.cs

@@ -0,0 +1,40 @@
+lock ("a string") { }              // Noncompliant
+//    ^^^^^^^^^^
+lock (MethodReturningString()) { } // Noncompliant
+lock (new object()) { }            // Noncompliant
+
+string MethodReturningString() => "a string";
+
+class ClassNestedAtTopLevel
+{
+    readonly object readonlyField = new();
+    object readWriteField = new();
+
+    static readonly object staticReadonlyField = new();
+    static object staticReadWriteField = new();
+
+    void Test()
+    {
+        lock (readonlyField) { }
+        lock (readWriteField) { }           // Noncompliant
+        lock (staticReadonlyField) { }
+        lock (staticReadWriteField) { }     // Noncompliant
+    }
+
+    class SecondLevelNesting
+    {
+        readonly object readonlyField = new();
+        object readWriteField = new();
+
+        static readonly object staticReadonlyField = new();
+        static object staticReadWriteField = new();
+
+        void Test()
+        {
+            lock (readonlyField) { }
+            lock (readWriteField) { }       // Noncompliant
+            lock (staticReadonlyField) { }
+            lock (staticReadWriteField) { } // Noncompliant
+        }
+    }
+}

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.cs

@@ -24,8 +24,8 @@ void OnAStaticField()
         lock (Test.staticReadonlyField) { }
         lock (Test.staticReadWriteField) { }             // Noncompliant {{'staticReadWriteField' is not 'private readonly', and should not be used for locking.}}
         //    ^^^^^^^^^^^^^^^^^^^^^^^^^
-        lock (AnotherClass.staticReadonlyField) { }      // Noncompliant {{Use fields from 'Test' for locking.}}
-        lock (AnotherClass.staticReadWriteField) { }     // Noncompliant {{Use fields from 'Test' for locking.}}
+        lock (AnotherClass.staticReadonlyField) { }      // Noncompliant {{Use members from 'Test' for locking.}}
+        lock (AnotherClass.staticReadWriteField) { }     // Noncompliant {{Use members from 'Test' for locking.}}
         //    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     }
 
@@ -149,7 +149,7 @@ class TestExplicitCast
 
     void Test()
     {
-        lock ((AnotherClass)readonlyField) { } // Compliant, the cast operator can build
+        lock ((AnotherClass)readonlyField) { } // Compliant, the cast operator can run arbitrary code
     }
 }