Skip to content

fix(coverage): ternary expressions never get branch coverage tracking #16826

Description

@gomesalexandre

forge coverage's branch-coverage tracking has a real gap: a ternary expression (cond ? a : b)
never gets a Branch coverage item, in any of the shapes it commonly appears in - a bare
expression-statement, an assignment RHS, a return expression, or a declaration initializer. It
only ever contributes a Statement/line hit, which fires whenever either arm executes. This
means a test suite that only ever exercises one side of a ternary is reported as 100% branch
coverage for that file, with no signal that the untested arm exists.

Repro

contract Assignment {
    uint x;
    function run(bool cond) external { x = cond ? 1 : 2; }
}
contract Returned {
    function run(bool cond) external pure returns (uint) { return cond ? 1 : 2; }
}
contract Declaration {
    function run(bool cond) external pure returns (uint) { uint x = cond ? 1 : 2; return x; }
}

Running forge coverage --mt testTrue (a test that only ever calls run(true)) against all three
prints N/A (0/0) branches for every file - i.e. the tool has no branch item at all for the
ternary, in any shape - while % Statements/% Lines read 100%. Compare with the same shape
written as if/else, which correctly reports 50% branches when only one side is exercised.

Full output tables (produced by an actual local build, Solc 0.8.35, default coverage settings):

| File                | % Lines       | % Statements  | % Branches | % Funcs       |
| src/Assignment.sol  | 100.00% (1/1) | 100.00% (1/1) | N/A (0/0)  | 100.00% (1/1) |
| src/Returned.sol    | 100.00% (1/1) | 100.00% (2/2) | N/A (0/0)  | 100.00% (1/1) |
| src/Declaration.sol | 100.00% (1/1) | 100.00% (3/3) | N/A (0/0)  | 100.00% (1/1) |

Root cause

crates/evm/coverage/src/analysis.rs's SourceVisitor::visit_expr groups ExprKind::Ternary with
Assign/Unary/Binary and only calls self.push_stmt(expr.span) (a Statement item) - unlike
StmtKind::If a few lines above, or the require(...) call handling in the same function, both of
which allocate a next_branch_id() and push two CoverageItemKind::Branch items (one per path).
Additionally, Assign/Unary/Ternary (unlike Binary) never call self.walk_expr(expr), so a
Ternary nested inside an Assign/Return/DeclSingle is never independently visited at all
today - the enclosing node swallows the whole span as one Statement.

Why a naive fix is unsafe (tried and rejected)

The obvious fix - reuse the same technique as require(): allocate two Branch items over the
ternary's span and let the existing find_anchor_branch (crates/evm/coverage/src/anchors.rs)
locate the two JUMPI-adjacent PCs - works for a simple, non-nested ternary (verified: correctly
reports 50%/50%/100% for one-arm/one-arm/both-arm runs on the shapes above). But find_anchor_branch
resolves to the last matching PUSH+JUMPI within the given span. For a nested ternary:

function run(bool outer, bool inner) external pure returns (uint) {
    return outer ? (inner ? 1 : 2) : 3;
}

the outer and inner Branch items both resolve to the same inner jump, so:

  • run(false, false) (outer branch never taken) prints 0.00% (0/4) - undercounting a path
    that was exercised.
  • Running only run(true, true) and run(true, false) (the outer path exercised, but the outer
    false
    path never exercised at all) prints 100.00% (4/4) - a false-positive full-coverage
    claim while a real path was never executed.

That's worse than the current gap: it would trade "no signal" for "confidently wrong signal" on
any nested ternary. A correct fix needs a ternary-aware association between each AST decision node
and its own bytecode jump (not "last matching JUMPI in a byte range"), which is more than a
minimal patch.

Suggested scope for whoever picks this up

  • Extend the AST visitor so a Ternary is reachable wherever it's nested (Assign RHS/LHS,
    Return, DeclSingle/DeclMulti initializers, call arguments) without turning on unrestricted
    expression recursion everywhere (that risks double-counting/exploding item counts elsewhere).
  • Give each ternary (including nested ones) its own disambiguated anchor resolution instead of
    reusing find_anchor_branch's "last matching JUMPI in span" heuristic verbatim - likely needs to
    track jump destinations per-ternary-node rather than per-span.
  • A regression test belongs in crates/forge/tests/cli/coverage.rs alongside the existing
    branch/branch_with_code_free_else tests, covering both a simple and a nested ternary.

Happy to share the full investigation (four fixture shapes + nested counterexample, all with real
forge coverage output) if useful - didn't want to open a PR with a fix I couldn't verify was
correct on nested cases.

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