Improve Rule S2223: cleanup and performance (#6761)

analyzers/rspec/cs/S2223_c#.html

@@ -1,7 +1,10 @@
-<p>A <code>static</code> field that is neither constant nor read-only is not thread-safe. Correctly accessing these fields from different threads
-needs synchronization with <code>lock</code>s. Improper synchronization may lead to unexpected results, thus publicly visible static fields are best
-suited for storing non-changing data shared by many consumers. To enforce this intent, these fields should be marked <code>readonly</code> or
-converted to constants.</p>
+<p>Non-private <code>static</code> fields that are neither <code>const</code> nor <code>readonly</code> can lead to errors and unpredictable
+behavior.</p>
+<p>This can happen because: * Any object can modify these fields and alter the global state. This makes the code more difficult to read, debug and
+test. * Correctly accessing these fields from different threads needs synchronization with <code>lock</code>. Improper synchronization may lead to
+unexpected results.</p>
+<p>Publicly visible static fields should only be used to store shared data that does not change. To enforce this intent, these fields should be marked
+<code>readonly</code> or converted to <code>const</code>.</p>
 <h2>Noncompliant Code Example</h2>
 <pre>
 public class Math

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

@@ -34,22 +34,28 @@ public sealed class StaticFieldVisible : SonarDiagnosticAnalyzer
             context.RegisterNodeAction(
                 c =>
                 {
-                    var fieldDeclaration = (FieldDeclarationSyntax)c.Node;
-                    foreach (var diagnostic in GetDiagnostics(fieldDeclaration.Declaration, c.SemanticModel))
+                    foreach (var diagnostic in GetDiagnostics(c.SemanticModel, (FieldDeclarationSyntax)c.Node))
                     {
                         c.ReportIssue(diagnostic);
                     }
                 },
                 SyntaxKind.FieldDeclaration);
 
-        private static IEnumerable<Diagnostic> GetDiagnostics(VariableDeclarationSyntax declaration, SemanticModel semanticModel) =>
-            declaration.Variables
-                       .Where(x => FieldIsRelevant(semanticModel.GetDeclaredSymbol(x) as IFieldSymbol))
-                       .Select(x => Diagnostic.Create(Rule, x.Identifier.GetLocation(), x.Identifier.ValueText));
+        private static IEnumerable<Diagnostic> GetDiagnostics(SemanticModel model, FieldDeclarationSyntax declaration) =>
+            FieldIsRelevant(declaration)
+                ? declaration.Declaration.Variables
+                    .Where(x => !FieldIsThreadSafe(model.GetDeclaredSymbol(x) as IFieldSymbol))
+                    .Select(x => Diagnostic.Create(Rule, x.Identifier.GetLocation(), x.Identifier.ValueText))
+                : Enumerable.Empty<Diagnostic>();
 
-        private static bool FieldIsRelevant(IFieldSymbol fieldSymbol) =>
-            fieldSymbol is {IsStatic: true, IsConst: false, IsReadOnly: false}
-            && fieldSymbol.DeclaredAccessibility != Accessibility.Private
-            && !fieldSymbol.GetAttributes().Any(x => x.AttributeClass.Is(KnownType.System_ThreadStaticAttribute));
+        private static bool FieldIsRelevant(FieldDeclarationSyntax node) =>
+            node.Modifiers.Count > 1
+            && node.Modifiers.Any(SyntaxKind.StaticKeyword)
+            && !node.Modifiers.Any(SyntaxKind.VolatileKeyword)
+            && (!node.Modifiers.Any(SyntaxKind.PrivateKeyword) || node.Modifiers.Any(SyntaxKind.ProtectedKeyword))
+            && !node.Modifiers.Any(SyntaxKind.ReadOnlyKeyword);
+
+        private static bool FieldIsThreadSafe(IFieldSymbol fieldSymbol) =>
+            fieldSymbol.GetAttributes().Any(x => x.AttributeClass.Is(KnownType.System_ThreadStaticAttribute));
     }
 }

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

@@ -6,6 +6,9 @@ public record StaticFieldVisible
 //                       ^^
     public const double Pi2 = 3.14;
     public double Pi3 = 3.14;
+    private protected static double Pi4 = 3.14; // Noncompliant
+    protected private static double Pi15 = 3.14; // Noncompliant
+
     public static Shape Empty = Shape.Empty; // Noncompliant {{Change the visibility of 'Empty' or make it 'const' or 'readonly'.}}
 
     [ThreadStatic]

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

@@ -6,11 +6,32 @@ public class StaticFieldVisible
     {
         public static double Pi = 3.14;  // Noncompliant
 //                           ^^
+        public static int X = 1, Y, Z = 100;
+//                        ^
+//                               ^@-1
+//                                  ^@-2
         public const double Pi2 = 3.14;
         public double Pi3 = 3.14;
 
+        protected static double Pi4 = 3.14;             // Noncompliant
+        internal static double Pi5 = 3.14;              // Noncompliant
+        internal static double Pi6 = 3.14;              // Noncompliant
+        protected internal static double Pi7 = 3.14;    // Noncompliant
+
+        private static double Pi8 = 3.14;
+        private double Pi9 = 3.14;
+        static double Pi10 = 3.14; // Compliant - if no access modifier exists, the field is private
+        double Pi11 = 3.14;
+        public readonly double Pi12 = 3.14;
+
+        public static volatile int VolatileValue = 3; // Compliant - see: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/volatile
+
         [ThreadStatic]
-        public static int value; // Compliant, thread static field values are not shared between threads
+        public static int Value; // Compliant, thread static field values are not shared between threads
+
+        [NonSerialized]
+        public static int Value2; // Noncompliant
+
     }
 
     public class Shape