No Unconditional Element Replace

ID

csharp.no_unconditional_element_replace

Severity

high

Remediation Complexity

trivial

Remediation Risk

low

Remediation Effort

low

Resource

Reliability

Language

CSharp

Tags

collection, dead-code, reliability, suspicious-assignment

Description

Reports a statement that writes a collection at a key an earlier statement in the same run of element writes has already written, where the later value does not depend on the earlier one. The earlier store is dead — nothing can ever observe it — so the work that produced it, including any side effect, is discarded.

Rationale

Storing a value and then replacing it without looking at it is never what the author meant. Either the key of one of the two writes is wrong (they were supposed to fill two slots), or the second write belongs behind a condition that was lost during an edit, or the first computation is a leftover that should simply go. In all three readings the code behaves differently from how it reads, and the expensive or side-effecting call on the first line runs for nothing.

The two writes do not have to be adjacent: the common shape is a block of key assignments in which one key appears twice. A block is therefore walked as a run of consecutive element writes, and each write is compared backwards against the writes already in the run. The run ends at the first statement that is not an element write, and the backward comparison stops as soon as an earlier write mentions the collection — so a read placed between the two writes is not reported.

A pair is reported only when the receivers and the keys are structurally equal and the later value never mentions the collection. That last condition is what keeps read-modify-write shapes such as counts[key] = counts[key] + 1 out of the report. Compound assignments are not treated as writes at all, since they read the previous value by definition.

Because keys are compared structurally, a key that changes as it is evaluated would compare equal to itself while addressing a different element: slots[i] = a; slots[i] = b; fills two slots. Only a key that is a simple name, a literal or a member access is therefore compared.

public class Registry
{
    private readonly Dictionary<string, int> counts = new Dictionary<string, int>();
    private readonly Dictionary<string, string> headers = new Dictionary<string, string>();
    private readonly string[] slots = new string[4];

    public void Seed(string key, int index)
    {
        counts.Add(key, Compute(key));
        counts[key] = 0;                       // FLAW — the computed value is discarded

        slots[index] = Load(index);
        slots[index] = "unknown";              // FLAW — the load never reaches anyone
    }

    public void SetHeaders(string json, string xml)
    {
        headers["Accept"] = json;
        headers["Content-Type"] = json;
        headers["Accept"] = xml;               // FLAW — the run of writes repeats one key
    }

    public void FillTwoSlots(int index)
    {
        slots[index] = Load(index);
        slots[index + 1] = "unknown";          // OK — different keys

        slots[index++] = "first";
        slots[index++] = "second";             // OK — each write addresses a different element
    }

    public void Bump(string key, int index)
    {
        counts[key] = counts[key] + 1;         // OK — the previous value is read

        slots[index] = Load(index);
        Report(slots[index]);
        slots[index] = "unknown";              // OK — the value is read in between
    }

    private int Compute(string key) => key.Length;
    private string Load(int index) => index.ToString();
    private void Report(string value) { }
}

Remediation

Work out which of the two writes was meant to survive and keep only that one. If the two values belong to different keys, fix the key. If the second write was supposed to apply only in some situation, restore the condition around it — a keyed collection also offers direct ways to say "only if absent", such as TryAdd for a dictionary.

References