From 5be33e43e3432e279ad31824337cb31756043044 Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Wed, 22 Feb 2023 10:58:23 +0100 Subject: [PATCH] Review --- analyzers/rspec/cs/S2445_c#.html | 8 +- .../Rules/LockedFieldPrivateReadonly.cs | 61 ---------------- .../LockedFieldShouldBePrivateAndReadonly.cs | 73 +++++++++++++++++++ ...kedFieldShouldBePrivateAndReadonlyTest.cs} | 23 ++++-- ...eldShouldBePrivateAndReadonly.CSharp11.cs} | 0 ...ieldShouldBePrivateAndReadonly.CSharp8.cs} | 15 ++-- ...FieldShouldBePrivateAndReadonly.CSharp9.cs | 23 ++++++ ... LockedFieldShouldBePrivateAndReadonly.cs} | 38 +++++++--- 8 files changed, 155 insertions(+), 86 deletions(-) delete mode 100644 analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldPrivateReadonly.cs create mode 100644 analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBePrivateAndReadonly.cs rename analyzers/tests/SonarAnalyzer.UnitTest/Rules/{LockedFieldPrivateReadonlyTest.cs => LockedFieldShouldBePrivateAndReadonlyTest.cs} (61%) rename analyzers/tests/SonarAnalyzer.UnitTest/TestCases/{LockedFieldPrivateReadonly.CSharp11.cs => LockedFieldShouldBePrivateAndReadonly.CSharp11.cs} (100%) rename analyzers/tests/SonarAnalyzer.UnitTest/TestCases/{LockedFieldPrivateReadonly.CSharp8.cs => LockedFieldShouldBePrivateAndReadonly.CSharp8.cs} (80%) create mode 100644 analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp9.cs rename analyzers/tests/SonarAnalyzer.UnitTest/TestCases/{LockedFieldPrivateReadonly.cs => LockedFieldShouldBePrivateAndReadonly.cs} (80%) diff --git a/analyzers/rspec/cs/S2445_c#.html b/analyzers/rspec/cs/S2445_c#.html index a4cc24ea031..0119f6dfac7 100644 --- a/analyzers/rspec/cs/S2445_c#.html +++ b/analyzers/rspec/cs/S2445_c#.html @@ -1,8 +1,12 @@

Locking on a class field synchronizes not on the field itself, but on the object assigned to it. So locking on a non-readonly 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.

-

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.

+

Locking on a readonly field of a class which is not private allows external code to lock the field, potentially +interfering with synchronization by methods of that class.

+

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.

+

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.

Noncompliant Code Example

 private string color = "red";
diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldPrivateReadonly.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldPrivateReadonly.cs
deleted file mode 100644
index 5929b808723..00000000000
--- a/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldPrivateReadonly.cs
+++ /dev/null
@@ -1,61 +0,0 @@
-/*
- * 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 LockedFieldPrivateReadonly : SonarDiagnosticAnalyzer
-{
-    private const string DiagnosticId = "S2445";
-
-    private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, "{0}");
-
-    public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule);
-
-    protected override void Initialize(SonarAnalysisContext context) =>
-        context.RegisterNodeAction(c =>
-            {
-                var expression = ((LockStatementSyntax)c.Node).Expression;
-                if (expression is ObjectCreationExpressionSyntax
-                    or AnonymousObjectCreationExpressionSyntax
-                    or ArrayCreationExpressionSyntax
-                    or ImplicitArrayCreationExpressionSyntax
-                    or QueryExpressionSyntax)
-                {
-                    c.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), "Locking on a new instance is a no-op."));
-                }
-                else if (expression.IsAnyKind(SyntaxKind.StringLiteralExpression, SyntaxKind.InterpolatedStringExpression))
-                {
-                    c.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), "Strings can be interned, and should not be used for locking."));
-                }
-                else if (expression is IdentifierNameSyntax
-                    && c.SemanticModel.GetSymbolInfo(expression).Symbol is ILocalSymbol lockedSymbol)
-                {
-                    c.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), $"'{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))
-                {
-                    c.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), $"'{lockedField.Name}' is not 'private readonly', and should not be used for locking."));
-                }
-            },
-            SyntaxKind.LockStatement);
-}
diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBePrivateAndReadonly.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBePrivateAndReadonly.cs
new file mode 100644
index 00000000000..324e02c2a2c
--- /dev/null
+++ b/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 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);
+}
diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldPrivateReadonlyTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldShouldBePrivateAndReadonlyTest.cs
similarity index 61%
rename from analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldPrivateReadonlyTest.cs
rename to analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldShouldBePrivateAndReadonlyTest.cs
index 61cf3ea09f0..ed6a414f847 100644
--- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldPrivateReadonlyTest.cs
+++ b/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();
+    private readonly VerifierBuilder builder = new VerifierBuilder();
 
     [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();
 }
diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.CSharp11.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp11.cs
similarity index 100%
rename from analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.CSharp11.cs
rename to analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp11.cs
diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.CSharp8.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp8.cs
similarity index 80%
rename from analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.CSharp8.cs
rename to analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp8.cs
index 2bad6133323..ffb6bbd01fe 100644
--- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.CSharp8.cs
+++ b/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
     }
 }
 
diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp9.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp9.cs
new file mode 100644
index 00000000000..f90b56d9fb0
--- /dev/null
+++ b/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();
+}
diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.cs
similarity index 80%
rename from analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.cs
rename to analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.cs
index 4fb8a46f7d4..8675c562d26 100644
--- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.cs
+++ b/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(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,6 +80,16 @@ 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;
@@ -85,7 +97,7 @@ void OnAssignment()
         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);
 }