Review

analyzers/rspec/cs/S2445_c#.html

@@ -1,8 +1,12 @@
 <p>Locking on a class field synchronizes not on the field itself, but on the object assigned to it. So locking on a non-<code>readonly</code> field
 makes it possible for the field’s value to change while a thread is in a block locked on the old value. That would allow a second thread, locked on
 the new value, to enter the block at the same time.</p>
-<p>The story is very similar for locking on a local variable or on a new instance; two different threads running the method in parallel would lock on
-two different object instances, completely undermining the synchronization.</p>
+<p>Locking on a <code>readonly</code> field of a class which is not <code>private</code> allows external code to lock the field, potentially
+interfering with synchronization by methods of that class.</p>
+<p>Locking on a local variable or on a new instance undermines the synchronization: two different threads running the method in parallel would lock on
+two different object instances.</p>
+<p>Locking on a string literal is even more dangerous: depending on whether the string is interned or not, different threads may or may not
+synchronize on the same object instance.</p>
 <h2>Noncompliant Code Example</h2>
 <pre>
 private string color = "red";

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

@@ -0,0 +1,73 @@
+/*
+ * SonarAnalyzer for .NET
+ * Copyright (C) 2015-2023 SonarSource SA
+ * mailto: contact AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+namespace SonarAnalyzer.Rules.CSharp;
+
+[DiagnosticAnalyzer(LanguageNames.CSharp)]
+public sealed class LockedFieldShouldBePrivateAndReadonly : SonarDiagnosticAnalyzer
+{
+    private const string DiagnosticId = "S2445";
+
+    private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, "{0}");
+
+    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
+
+    protected override void Initialize(SonarAnalysisContext context) =>
+        context.RegisterNodeAction(c =>
+            {
+                var expression = ((LockStatementSyntax)c.Node).Expression?.RemoveParentheses();
+                if (expression is ObjectCreationExpressionSyntax
+                    or AnonymousObjectCreationExpressionSyntax
+                    or ArrayCreationExpressionSyntax
+                    or ImplicitArrayCreationExpressionSyntax
+                    or QueryExpressionSyntax)
+                {
+                    ReportIssue("Locking on a new instance is a no-op.");
+                }
+                else
+                {
+                    var type = c.SemanticModel.GetTypeInfo(expression).Type;
+
+                    if (expression.IsAnyKind(SyntaxKind.StringLiteralExpression, SyntaxKind.InterpolatedStringExpression)
+                        || (type is not null && type.Is(KnownType.System_String)))
+                    {
+                        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($"'{lockedSymbol.Name}' is a local variable, and should not be used for locking.");
+                    }
+                    else if (expression is (IdentifierNameSyntax or MemberAccessExpressionSyntax)
+                        && c.SemanticModel.GetSymbolInfo(expression).Symbol is IFieldSymbol lockedField
+                        && (!lockedField.IsReadOnly || lockedField.GetEffectiveAccessibility() != Accessibility.Private))
+                    {
+                        ReportIssue(lockedField.ContainingType is { } lockedFieldType
+                            && c.ContainingSymbol?.ContainingType is { } containingType
+                            && !lockedFieldType.Equals(containingType)
+                                ? $"Use field from '{containingType.ToMinimalDisplayString(c.SemanticModel, expression.SpanStart)}' for locking."
+                                : $"'{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);
+}

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

@@ -23,25 +23,32 @@
 namespace SonarAnalyzer.UnitTest.Rules;
 
 [TestClass]
-public class LockedFieldPrivateReadonlyTest
+public class LockedFieldShouldBePrivateAndReadonlyTest
 {
-    private readonly VerifierBuilder builder = new VerifierBuilder<LockedFieldPrivateReadonly>();
+    private readonly VerifierBuilder builder = new VerifierBuilder<LockedFieldShouldBePrivateAndReadonly>();
 
     [TestMethod]
-    public void LockedFieldPrivateReadonly_CS() =>
-        builder.AddPaths("LockedFieldPrivateReadonly.cs").Verify();
+    public void LockedFieldShouldBePrivateAndReadonly_CS() =>
+        builder.AddPaths("LockedFieldShouldBePrivateAndReadonly.cs").Verify();
 
     [TestMethod]
-    public void LockedFieldPrivateReadonly_CSharp8() =>
+    public void LockedFieldShouldBePrivateAndReadonly_CSharp8() =>
         builder
-            .AddPaths("LockedFieldPrivateReadonly.CSharp8.cs")
+            .AddPaths("LockedFieldShouldBePrivateAndReadonly.CSharp8.cs")
             .WithOptions(ParseOptionsHelper.FromCSharp8)
             .Verify();
 
     [TestMethod]
-    public void LockedFieldPrivateReadonly_CSharp11() =>
+    public void LockedFieldShouldBePrivateAndReadonly_CSharp9() =>
         builder
-            .AddPaths("LockedFieldPrivateReadonly.CSharp11.cs")
+            .AddPaths("LockedFieldShouldBePrivateAndReadonly.CSharp9.cs")
+            .WithOptions(ParseOptionsHelper.FromCSharp9)
+            .Verify();
+
+    [TestMethod]
+    public void LockedFieldShouldBePrivateAndReadonly_CSharp11() =>
+        builder
+            .AddPaths("LockedFieldShouldBePrivateAndReadonly.CSharp11.cs")
             .WithOptions(ParseOptionsHelper.FromCSharp11)
             .Verify();
 }

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.CSharp8.cs → analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp8.cs

@@ -1,6 +1,4 @@
-using System;
-
-class Test
+class Test
 {
     static readonly object staticReadonlyField = null;
     static object staticReadWriteField = null;
@@ -32,13 +30,18 @@ void ReadonlyReferences()
 
     void OnANewInstanceOnStack()
     {
-        lock (stackalloc int[] { }) { }   // Error [CS0185]
-        lock (stackalloc [] { 1 }) { }    // Error [CS0185]
+        lock (stackalloc int[] { }) { }              // Error [CS0185]
+        lock (stackalloc [] { 1 }) { }               // Error [CS0185]
     }
 
     void CoalescingAssignment(object oPar)
     {
-        lock (oPar ??= readonlyField) { } // FN, null conditional assignment not supported
+        lock (oPar ??= readonlyField) { }            // FN, null conditional assignment not supported
+    }
+
+    void SwitchExpression(object oPar)
+    {
+        lock (oPar switch { _ => new object() }) { } // FN, switch expression not supported
     }
 }
 

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

@@ -0,0 +1,23 @@
+class Test
+{
+    readonly ARecord readonlyField = new();
+    ARecord readWriteField = new();
+
+    static readonly ARecord staticReadonlyField = new();
+    static ARecord staticReadWriteField = new();
+
+    void OnAFieldOfTypeRecord()
+    {
+        lock (readonlyField) { }
+        lock (readWriteField) { }       // Noncompliant
+        lock (staticReadonlyField) { }
+        lock (staticReadWriteField) { } // Noncompliant
+    }
+
+    void OnANewRecordInstance()
+    {
+        lock (new ARecord()) { }  // Noncompliant
+    }
+
+    record ARecord();
+}

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

@@ -24,18 +24,21 @@ 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 {{'staticReadonlyField' is not 'private readonly', and should not be used for locking.}}
-        lock (AnotherClass.staticReadWriteField) { }     // Noncompliant {{'staticReadWriteField' is not 'private readonly', and should not be used for locking.}}
+        lock (AnotherClass.staticReadonlyField) { }      // Noncompliant {{Use field from 'Test' for locking.}}
+        lock (AnotherClass.staticReadWriteField) { }     // Noncompliant {{Use field from 'Test' for locking.}}
         //    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     }
 
     void OnAFieldOfSameInstance()
     {
         lock (readonlyField) { }
-        lock (readonlyStringField) { }
+        lock ((readonlyField)) { }
+        lock (readonlyStringField) { }                   // Noncompliant {{Strings can be interned, and should not be used for locking.}}
         lock (readWriteField) { }                        // Noncompliant {{'readWriteField' is not 'private readonly', and should not be used for locking.}}
+        lock ((readWriteField)) { }                      // Noncompliant {{'readWriteField' is not 'private readonly', and should not be used for locking.}}
         lock (this.readonlyField) { }
         lock (this.readWriteField) { }                   // Noncompliant {{'readWriteField' is not 'private readonly', and should not be used for locking.}}
+        lock ((this.readWriteField)) { }                 // Noncompliant {{'readWriteField' is not 'private readonly', and should not be used for locking.}}
     }
 
     void OnAFieldOfDifferentInstance()
@@ -44,6 +47,7 @@ void OnAFieldOfDifferentInstance()
         lock (anotherInstance.readonlyField) { }
         lock (anotherInstance.readWriteField) { }        // Noncompliant {{'readWriteField' is not 'private readonly', and should not be used for locking.}}
         lock (anotherInstance.readonlyField) { }
+        lock (anotherInstance?.readWriteField) { }       // FN: ?. not supported
     }
 
     void OnALocalVariable()
@@ -60,13 +64,11 @@ void OnANewInstance()
     {
         lock (new object()) { }                          // Noncompliant {{Locking on a new instance is a no-op.}}
         lock (new ANamespace.AClass()) { }               // Noncompliant
-        lock (new Test[] { })                            // Noncompliant
-        lock (new[] { readonlyField}) { }                // Noncompliant
+        lock (new Test[] { }) { }                        // Noncompliant
+        lock (new[] { readonlyField }) { }               // Noncompliant
         lock (new Tuple<object>(readonlyField)) { }      // Noncompliant
-        lock (new { })                                   // Noncompliant
+        lock (new { }) { }                               // Noncompliant
 
-        lock ("a string") { }                            // Noncompliant
-        lock ($"an interpolated {"string"}") { }         // Noncompliant
         lock (1) { }                                     // Error [CS0185]
         lock ((a: readonlyField, b: readonlyField)) { }  // Error [CS0185]
 
@@ -78,14 +80,24 @@ void OnANewInstance()
         lock (from x in new object[2] select x) { }      // Noncompliant
     }
 
+    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.}}
+
+        string MethodReturningString() => "a string";
+    }
+
     void OnAssignment()
     {
         object x;
         lock (x = readonlyField) { }
         lock (x = readWriteField) { }                    // FN, assignment not supported
     }
 
-    void OtherCases(object oPar, bool bPar)
+    void OtherCases(object oPar, bool bPar, object[] arrayPar)
     {
         lock (null) { }
 
@@ -109,6 +121,8 @@ void OtherCases(object oPar, bool bPar)
 
         lock (oPar ?? readonlyField) { }
         lock (oPar = readonlyField) { }
+
+        lock (arrayPar[0]) { }
     }
 
     void ReadWriteReferences()
@@ -120,6 +134,12 @@ void ReadWriteReferences()
         ref object RefReturnStaticReadonlyField(Test instance) => ref Test.staticReadWriteField;
     }
 
+    void NoIdentifier()
+    {
+        lock () { }   // Error
+        lock (()) { } // Error
+    }
+
     delegate object ADelegate(object oPar);
 }