org.sonar.plugins.csharp.S2223.html Maven / Gradle / Ivy

Go to download
Show more of this group Show more artifacts with this name
Show all versions of sonar-csharp-plugin Show documentation
Code Analyzer for C#
There is a newer version: 10.2.0.105762
Why is this an issue?
Unlike instance fields, which can only be accessed by code having a hold on the instance, static fields can be accessed by any code
having visibility of the field and its type.
public class Math
{
    public static double Pi = 3.14;  // Noncompliant
}

// Somewhere else, where Math and Math.Pi are visible
var pi = Math.Pi; // Reading the value
Math.Pi = 3.1416; // Mutating the value

Another typical scenario of the use of a non-private mutable static field is the following:
public class Shape
{
    public static Shape Empty = new EmptyShape();  // Noncompliant

    private class EmptyShape : Shape
    {
    }
}

Non-private static fields that are neither const nor readonly, like the ones in the examples above, 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. class Counters
{
    public static int ErrorCounter = 0;
}

class Program
{
    public static void Thread1()
    {
        // ...
        Counters.ErrorCounter = 0; // Error counter reset
        // ...
    }

    public static void Thread2()
    {
        // ...
        if (Counters.ErrorCounter > 0)
        {
            Trace.TraceError($"There are {Counters.ErrorCounter} errors"); // It may print "There are 0 errors"
        }
        // ...
    }
}
 
   Correctly accessing these fields from different threads needs synchronization with lock or equivalent mechanisms. Improper synchronization may lead to unexpected results. class Counters
{
    public static volatile int ErrorCounter;
}

class Program
{
    public static void ImproperSynchronization()
    {
        Counters.ErrorCounter = 0;
        Parallel.ForEach(Enumerable.Range(0, 1000), _ => Counters.ErrorCounter++); // Volatile is not enough
        Console.WriteLine(Counters.ErrorCounter); // May print less than 1000
    }

    public static void ProperSynchronization()
    {
        Counters.ErrorCounter = 0;
        Parallel.ForEach(Enumerable.Range(0, 1000), _ => Interlocked.Increment(ref Counters.ErrorCounter));
        Console.WriteLine(Counters.ErrorCounter); // Always prints 1000
    }
}
 

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
{
    public const double Pi = 3.14;
}

public class Shape
{
    public static readonly Shape Empty = new EmptyShape();

    private class EmptyShape : Shape
    {
    }
}

Resources
Documentation

   static (C# Reference) 
   Overview of synchronization
  primitives 
   volatile (C# Reference) 

Articles & blog posts

   Stack Exchange - Mutable global state is evil and alternatives to it 
   Fabulous adventures in coding - Eric Lippert:
  Immutability in C# 
   Stack Overflow - Eric Lippert: ++ is not "threadsafe"