No Useless Increment

ID

csharp.no_useless_increment

Severity

high

Remediation Complexity

trivial

Remediation Risk

low

Remediation Effort

low

Resource

Reliability

Language

CSharp

Tags

dead-code, reliability, suspicious-assignment

Description

Reports a postfix ` or `--` whose effect is discarded: `return i; (or the expression body int Bump(int i) ⇒ i;`) on a local variable or a by-value parameter, where the returned value is the one from before the increment, and `i = i;, which writes the old value straight back over the incremented one.

Rationale

The postfix operator yields the value the variable had before it was changed. In return i;` the caller therefore receives the old value, and the variable that was just incremented dies with the method — the increment has no observable effect at all. In `i = i; the same old value is assigned back to i, so the statement as a whole does nothing. Both shapes compile without a warning and both read as if they did something, which is what makes them worth reporting.

The returned-value shape is reported only when the operand is a local variable or a by-value parameter, that is, storage that dies with the call. A field keeps the incremented value, so return _next++; is the ordinary way to write an id generator, and a ref or out parameter hands the new value back to the caller. Neither is reported.

private int counter;

public int NextBroken()
{
    int i = counter;
    return i++;                       // FLAW — returns the old value; the increment is lost
}

public void ResetBroken(int i)
{
    i = i++;                          // FLAW — no-op: the old value overwrites the new one
}

public int Bump(int i) => i++;         // FLAW — same defect in an expression body

public int NextOk()
{
    return ++counter;                 // OK — the incremented value is returned
}

public int NextIdOk()
{
    return counter++;                 // OK — the field keeps the incremented value
}

public int TakeOk(ref int cursor)
{
    return cursor++;                  // OK — the caller observes the increment
}

public void CountOk(int[] items)
{
    int n = 0;
    foreach (var item in items)
    {
        n++;                          // OK — the normal idiom
    }

    Use(n);
}

Remediation

For return i;` decide what the caller should get: `return i + 1;` to return the successor without touching `i`, or `return i; when the update itself was the point and should be visible in the returned value. If the counter is meant to survive the call, promote it to a field or a ref parameter. For i = i;` drop the assignment and write `i;, or i = i + 1; if the intent was to make the update explicit.

References