No Static Write From Instance Member

ID

csharp.no_static_write_from_instance_member

Severity

high

Remediation Complexity

medium

Remediation Risk

low

Remediation Effort

medium

Resource

Code Smell

Language

CSharp

Tags

code_smell, concurrency, field, shared-state

Description

Reports an instance method, property accessor or event accessor that writes to a static field of its own type. One shared storage location is being mutated from code that runs once per object, and in most processes on several threads at once.

Rationale

A static field has exactly one value for the whole program. An instance member has one execution per object, and in a web request pipeline, a background worker or a test run, several of those executions overlap in time. Writing the static field from there mixes the two lifetimes, and the field’s value ends up determined by which caller happened to run last.

The first consequence is lost updates. count++, total += amount, and any "read it, decide, assign it" sequence compile to a read followed by a write with a gap in between. Two instances running concurrently both read the old value and both write back their own result, so one update disappears. Nothing throws, and the shortfall is proportional to load — which is why it is usually noticed in production rather than in tests.

The second is cross-instance interference. A field that reads like per-object state — the last processed item, a cached lookup, a current user — is in fact one slot, so object A silently overwrites what object B is still working with. In a test suite this shows up as tests that pass alone and fail together, or that depend on execution order.

A write from a static method, from a static constructor or from a field initializer is not reported: those run once, under the runtime’s initialization guarantees. Constructors, destructors and operators are out of scope, so counting instances in a constructor is left alone.

A write held inside a lock block is reported. The check is deliberately syntactic: a lock makes one update atomic, but it does not make the field per-object, so the cross-instance interference above survives it. Where the shared state is intended and the lock covers the whole read-modify-write, the finding is one to accept at triage rather than a defect to fix.

using System;

public class Tracker
{
    private static readonly object SyncRoot = new object();
    private static int processed;
    private int instanceCount;

    public void Record()
    {
        processed++;                          // FLAW — two instances can lose an update
    }

    public int Total
    {
        get { return processed; }             // OK — a read is not a write
    }

    public void RecordSafely()
    {
        lock (SyncRoot)
        {
            processed++;                      // FLAW — atomic, but still one slot for every instance
        }
    }

    public void RecordAtomically()
    {
        System.Threading.Interlocked.Increment(ref processed);   // OK — single atomic operation
    }

    public void Count()
    {
        instanceCount++;                      // OK — per-object state
    }

    public static void Reset()
    {
        processed = 0;                        // OK — static member, shared state on purpose
    }
}

Remediation

Decide whether the state is per-object or global, then make the code say so.

  • Per-object state: make the field an instance field. This is the fix in the majority of cases — the static was a mistake or a leftover.

  • A genuinely global counter or aggregate: keep it static and make the update atomic. Interlocked.Increment / Interlocked.Add / Interlocked.Exchange cover counters and single-slot values without a lock; a lock around the whole read-modify-write covers anything more involved. For a shared collection, use a type from System.Collections.Concurrent. A write already synchronised this way is still reported — accept the finding at triage rather than rewriting it.

  • State that is per-caller rather than per-object or global — an ambient context — belongs in AsyncLocal<T> or in the request scope of the dependency-injection container, not in a static field.

References