diff --git a/analyzers/rspec/cs/S2223_c#.html b/analyzers/rspec/cs/S2223_c#.html index 7757a715351..a255243326e 100644 --- a/analyzers/rspec/cs/S2223_c#.html +++ b/analyzers/rspec/cs/S2223_c#.html @@ -1,7 +1,10 @@ -
A static
field that is neither constant nor read-only is not thread-safe. Correctly accessing these fields from different threads
-needs synchronization with lock
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 readonly
or
-converted to constants.
Non-private static
fields that are neither const
nor readonly
can lead to errors and unpredictable
+behavior.
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 lock
. Improper synchronization may lead to
+unexpected results.
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
+readonly
or converted to const
.
public class Math diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs index 6a412c10be3..8d9c852d685 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs +++ b/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 IEnumerableGetDiagnostics(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 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 (); - 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)); } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.CSharp9.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.CSharp9.cs index 52f4a2b135c..801c4867761 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.CSharp9.cs +++ b/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] diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs index 441f2428d9c..5df9b1628be 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs +++ b/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