Skip to content

fix(lint): uninitialized-local false-positives on a for-loop's own init counter #16641

Description

@gomesalexandre

uninitialized-local's zero-iteration loop restore doesn't cover a for-loop's own init counter, so for (uint256 i; i < n; ++i) false-positives as an uninitialized read of i in the loop condition.

Mechanism

Solar lowers a for loop's init declaration (uint256 i) into a wrapper block outside the Loop HIR node itself — the declaration lives in the parent block, not inside the loop body. crates/lint/src/sol/med/uninitialized_local.rs's StmtKind::Loop arm restores self.uninitialized to account for the loop body possibly running zero times, but that restore only covers state set inside the loop body — it can't reach a variable declared in the parent block. Because the check reasons at the Loop node in isolation, it has no way to special-case a [DeclSingle, Loop] sibling shape at the parent Block level, so the counter never gets marked initialized before the loop condition reads it.

Separately, findings (the recorded warnings) is never rolled back the way self.uninitialized is on the zero-iteration path, so once a warning is recorded during the body walk it survives even if later analysis would otherwise suppress it.

Counter-evidence, raised proactively

The compoundRead fixture (crates/lint/testdata/UninitializedLocal.sol, uint256 x; x += 1; //~WARN) pins that warning on a read of a defaulted value-type variable is intentional — so this isn't simply "never warn on defaulted reads." The distinction I'd draw: x += 1 on a genuinely fresh x is a plausible real mistake with no idiomatic defence, whereas for (uint256 i; i < n; ++i) is a deliberate, universally-understood gas-optimization idiom (skip the explicit = 0 since the type already defaults to zero) — and the loop arm's own implementation comments reason only about writes being discarded on the zero-iteration path, never mentioning counters, suggesting this specific shape wasn't considered rather than being a deliberate design choice.

Blast radius

The for (uintN i; ...) (no explicit initializer) idiom appears in 24 .sol files inside foundry's own repo, including files under crates/lint/testdata/ and several crates/fmt/testdata/ fixtures — so this is a false positive on a pattern the repo's own test corpus uses repeatedly.

Why this is an issue, not a PR

A fix isn't a trivial one-liner: the for-init variable isn't distinguishable at the Loop node itself (it lives in the parent Block), so a correct fix needs the StmtKind::Block arm to recognize solar's [DeclSingle, Loop] sibling shape and treat the declared variable as initialized-by-the-loop-condition before entering the loop analysis — not a change scoped entirely within the Loop arm.

Would a maintainer want a carve-out specifically for a for-init counter whose only pre-write read is the loop's own condition/update expression? Happy to open a PR along those lines if that's the right shape — wanted to check framing first given the compoundRead precedent above.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    • Status
      Backlog

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions