A static field that is neither constant nor read-only is not thread-safe. Correctly accessing these fields from different threads
-needs synchronization with locks. 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 IEnumerable 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 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