diff --git a/.changelog/lint-defaulted-loop-counters.md b/.changelog/lint-defaulted-loop-counters.md new file mode 100644 index 0000000000000..9d45ab0aff48f --- /dev/null +++ b/.changelog/lint-defaulted-loop-counters.md @@ -0,0 +1,6 @@ +--- +forge: patch +forge-lint: patch +--- + +Avoid uninitialized-local warnings for unsigned counters that implicitly start at zero in conventional for-loop headers. diff --git a/crates/lint/docs/uninitialized-local.md b/crates/lint/docs/uninitialized-local.md index 209aa044dd2bb..78b29ff2b11c2 100644 --- a/crates/lint/docs/uninitialized-local.md +++ b/crates/lint/docs/uninitialized-local.md @@ -9,6 +9,12 @@ Flags local variables that are declared without an initializer and then read bef Reports any local variable of `VarKind::Statement` (i.e., a variable declared inside a function body, not a parameter or state variable) whose first use is a read and which has never been explicitly assigned prior to that read on at least one execution path. +Unsigned counters declared in a `for` initializer may intentionally start at zero, as in +`for (uint256 i; i < n; ++i)`. The lint exempts these counters when the condition compares +them against an upper bound and the header update uses `++i` or `i++`. Other uninitialized +locals read by the condition or body still produce warnings. Counters declared outside the +header or incremented inside the body retain their existing diagnostics. + ## Why is this bad? Reading an uninitialized variable means the code silently depends on a language-level zero-default rather than an explicit value chosen by the developer. Common consequences include: diff --git a/crates/lint/src/sol/med/uninitialized_local.rs b/crates/lint/src/sol/med/uninitialized_local.rs index 647e228fc2480..f2724969ee419 100644 --- a/crates/lint/src/sol/med/uninitialized_local.rs +++ b/crates/lint/src/sol/med/uninitialized_local.rs @@ -7,12 +7,13 @@ use crate::{ }, }; use solar::{ + ast::ElementaryType, interface::{Span, data_structures::Never}, sema::{ Gcx, Hir, hir::{ - Expr, ExprKind, Function, LoopSource, Res, Stmt, StmtKind, TypeKind, VarKind, - VariableId, Visit, + BinOpKind, Block, Expr, ExprKind, Function, LoopSource, Res, Stmt, StmtKind, TypeKind, + UnOpKind, VarKind, VariableId, Visit, }, }, }; @@ -64,6 +65,36 @@ impl Checker<'_> { } } +/// Recognizes an unsigned counter whose implicit zero is intentional in a conventional `for` +/// header. Matching the lowered wrapper's span keeps declarations outside the header distinct. +fn defaulted_counter_loop<'hir>( + hir: &Hir<'hir>, + block: &'hir Block<'hir>, +) -> Option<&'hir Stmt<'hir>> { + if let [Stmt { kind: StmtKind::DeclSingle(vid), .. }, loop_stmt] = block.stmts + && let StmtKind::Loop(body, LoopSource::ForWithUpdate) = &loop_stmt.kind + && block.span == loop_stmt.span + && hir.variable(*vid).initializer.is_none() + && matches!(hir.variable(*vid).ty.kind, TypeKind::Elementary(ElementaryType::UInt(_))) + && let [Stmt { kind: StmtKind::If(condition, then, Some(else_)), .. }] = body.stmts + && matches!(else_.kind, StmtKind::Break) + && let ExprKind::Binary(left, op, right) = &condition.peel_parens().kind + && ((matches!(op.kind, BinOpKind::Lt | BinOpKind::Le) && left.as_variable() == Some(*vid)) + || (matches!(op.kind, BinOpKind::Gt | BinOpKind::Ge) + && right.as_variable() == Some(*vid))) + && let StmtKind::Block(inner) = &then.kind + && inner.span == body.span + && let [_, Stmt { kind: StmtKind::Expr(update), .. }] = inner.stmts + && let ExprKind::Unary(op, target) = &update.peel_parens().kind + && matches!(op.kind, UnOpKind::PreInc | UnOpKind::PostInc) + && target.as_variable() == Some(*vid) + { + Some(loop_stmt) + } else { + None + } +} + impl<'hir> Visit<'hir> for Checker<'hir> { type BreakValue = Never; @@ -73,6 +104,13 @@ impl<'hir> Visit<'hir> for Checker<'hir> { fn visit_stmt(&mut self, stmt: &'hir Stmt<'hir>) -> ControlFlow { match &stmt.kind { + StmtKind::Block(block) => { + if let Some(loop_stmt) = defaulted_counter_loop(self.hir, block) { + // Skip only the counter's declaration; all reads in the loop still run + // through the ordinary checker, including reads of other locals. + return self.visit_stmt(loop_stmt); + } + } StmtKind::DeclSingle(vid) => { let v = self.hir.variable(*vid); if v.kind == VarKind::Statement diff --git a/crates/lint/testdata/UninitializedLocalFor.sol b/crates/lint/testdata/UninitializedLocalFor.sol new file mode 100644 index 0000000000000..362b101ffa196 --- /dev/null +++ b/crates/lint/testdata/UninitializedLocalFor.sol @@ -0,0 +1,99 @@ +//@compile-flags: --only-lint uninitialized-local + +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +contract UninitializedLocalFor { + // Header counters deliberately start at zero, including reads in the loop body. + function counters(uint256 n) public pure returns (uint256 sum) { + for (uint256 i; i < n; ++i) { + sum += i; + } + for (uint8 j; j <= 10; j++) { + sum += j; + } + for (uint256 k; n > (k); (k)++) { + for (uint256 m; n >= m; ++m) { + sum += k + m; + } + } + } + + // A declaration outside the header must not match the synthetic wrapper. + function outsideHeader(uint256 n) public pure { + { + uint256 i; + for (; i < n; ++i) {} //~WARN: local variable is read before being initialized + } + } + + // Only the loop counter is exempt; an uninitialized bound is still a read. + function missingBound() public pure { + uint256 n; + for (uint256 i; i < n; ++i) {} //~WARN: local variable is read before being initialized + } + + // The loop might run, so findings in its body must not be rolled back. + function bodyRead(uint256 n) public pure returns (uint256 sum) { + for (uint256 i; i < n; ++i) { + uint256 amount; + sum += amount; //~WARN: local variable is read before being initialized + } + } + + // The loop might not run, so its writes do not initialize another local afterwards. + function zeroIterations(uint256 n) public pure returns (uint256) { + uint256 amount; + for (uint256 i; i < n; ++i) { + amount = i; + } + return amount; //~WARN: local variable is read before being initialized + } + + // A header declaration alone does not make an unrelated local a counter. + function unrelatedHeader(uint256 n) public pure returns (uint256 sum) { + uint256 i = 0; + for (uint256 amount; i < n; ++i) { + sum += amount; //~WARN: local variable is read before being initialized + } + } + + // Descending from an implicit zero is not the ascending-counter idiom. + function decrement(uint256 n) public pure { + for (uint256 i; i < n; --i) {} //~WARN: local variable is read before being initialized + } + + // A condition that happens to read a local is not enough without its increment. + function differentUpdate(uint256 n) public pure { + for (uint256 i; i < n; --n) {} //~WARN: local variable is read before being initialized + } + + // Loops without a header update keep their existing diagnostics. + function bodyUpdate(uint256 n) public pure { + for (uint256 i; i < n;) { //~WARN: local variable is read before being initialized + ++i; + } + } + + // A standalone compound read still relies on an unintended default. + function compoundRead() public pure returns (uint256) { + uint256 amount; + amount += 1; //~WARN: local variable is read before being initialized + return amount; + } + + // Nested counter exemptions must not suppress another local's read. + function nestedRead(uint256 n) public pure returns (uint256 sum) { + for (uint256 i; i < n; ++i) { + for (uint256 j; j < n; ++j) { + uint256 amount; + sum += amount; //~WARN: local variable is read before being initialized + } + } + } + + // The exception is limited to unsigned counters. + function signedCounter(int256 n) public pure { + for (int256 i; i < n; ++i) {} //~WARN: local variable is read before being initialized + } +} diff --git a/crates/lint/testdata/UninitializedLocalFor.stderr b/crates/lint/testdata/UninitializedLocalFor.stderr new file mode 100644 index 0000000000000..33a314c9019fa --- /dev/null +++ b/crates/lint/testdata/UninitializedLocalFor.stderr @@ -0,0 +1,88 @@ +warning[uninitialized-local]: local variable is read before being initialized + ╭▸ ROOT/testdata/UninitializedLocalFor.sol:LL:CC + │ +LL │ for (; i < n; ++i) {} + │ ━ + │ + ╰ help: https://getfoundry.sh/forge/linting/uninitialized-local + +warning[uninitialized-local]: local variable is read before being initialized + ╭▸ ROOT/testdata/UninitializedLocalFor.sol:LL:CC + │ +LL │ for (uint256 i; i < n; ++i) {} + │ ━ + │ + ╰ help: https://getfoundry.sh/forge/linting/uninitialized-local + +warning[uninitialized-local]: local variable is read before being initialized + ╭▸ ROOT/testdata/UninitializedLocalFor.sol:LL:CC + │ +LL │ sum += amount; + │ ━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/uninitialized-local + +warning[uninitialized-local]: local variable is read before being initialized + ╭▸ ROOT/testdata/UninitializedLocalFor.sol:LL:CC + │ +LL │ return amount; + │ ━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/uninitialized-local + +warning[uninitialized-local]: local variable is read before being initialized + ╭▸ ROOT/testdata/UninitializedLocalFor.sol:LL:CC + │ +LL │ sum += amount; + │ ━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/uninitialized-local + +warning[uninitialized-local]: local variable is read before being initialized + ╭▸ ROOT/testdata/UninitializedLocalFor.sol:LL:CC + │ +LL │ for (uint256 i; i < n; --i) {} + │ ━ + │ + ╰ help: https://getfoundry.sh/forge/linting/uninitialized-local + +warning[uninitialized-local]: local variable is read before being initialized + ╭▸ ROOT/testdata/UninitializedLocalFor.sol:LL:CC + │ +LL │ for (uint256 i; i < n; --n) {} + │ ━ + │ + ╰ help: https://getfoundry.sh/forge/linting/uninitialized-local + +warning[uninitialized-local]: local variable is read before being initialized + ╭▸ ROOT/testdata/UninitializedLocalFor.sol:LL:CC + │ +LL │ for (uint256 i; i < n;) { + │ ━ + │ + ╰ help: https://getfoundry.sh/forge/linting/uninitialized-local + +warning[uninitialized-local]: local variable is read before being initialized + ╭▸ ROOT/testdata/UninitializedLocalFor.sol:LL:CC + │ +LL │ amount += 1; + │ ━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/uninitialized-local + +warning[uninitialized-local]: local variable is read before being initialized + ╭▸ ROOT/testdata/UninitializedLocalFor.sol:LL:CC + │ +LL │ sum += amount; + │ ━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/uninitialized-local + +warning[uninitialized-local]: local variable is read before being initialized + ╭▸ ROOT/testdata/UninitializedLocalFor.sol:LL:CC + │ +LL │ for (int256 i; i < n; ++i) {} + │ ━ + │ + ╰ help: https://getfoundry.sh/forge/linting/uninitialized-local +