fix(coverage): track nested ternary branches - #16828
Open
gomesalexandre wants to merge 2 commits into
Open
gomesalexandre wants to merge 2 commits into
gomesalexandre wants to merge 2 commits into
Conversation
gomesalexandre
requested review from
0xrusowsky,
DaniPopes,
figtracer,
grandizzy,
mablr,
mattsse and
stevencartavia
as code owners
September 12, 2026 14:31
Contributor
✅ Changelog foundThe deterministic check will validate the changed entry. |
Discover ternary decisions with a branch-only AST traversal and match their jumps by exact source-map spans. This prevents nested decisions from sharing anchors and falsely reporting full branch coverage, while avoiding duplicate statement items.
Allow duplicate exact source spans from modifier inlining to use the last matching jump. Restore existing ternary statement counting and cover shared modifiers with a CLI regression test.
gomesalexandre
force-pushed
the
fix_coverage_ternary_branch
branch
from
September 13, 2026 10:34
2aa0ed4 to
f56a9b9
Compare
Contributor
Author
|
Rebased onto master - conflicted with #16835 ("track both Yul branch outcomes"), which restructured this exact function/match arm. Reconciled both: kept the exact-match ternary threading ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #16826
forge coveragenever gave ternary expressions (cond ? a : b) their own branch coverage item, inany of the shapes they show up in (bare statement, assignment RHS, return, declaration initializer,
call argument) - only a
Statement/line hit that fires whenever either arm runs. A test suite thatonly ever exercised one arm was reported as 100% branch coverage, no signal the other arm exists.
What changed
TernaryVisitorwalks every function body once, discoveringExprKind::Ternarynodes wherever they're nested (assign RHS/LHS, return, decl init, call args, another ternary's own
condition/arms) and pushing two
Branchcoverage items per node. It only pushesBranchitems -statement coverage stays owned by the pre-existing statement-level visitor, unchanged, so
% Statementsbehavior for ternaries is byte-identical tomain.crates/evm/coverage/src/anchors.rs's anchor resolution gets anexactmode used only forternary branches: instead of "does this JUMPI's source-mapped range overlap/fall inside a
containing span" (the existing heuristic used for
if/require, which resolves to the lastmatching JUMPI in the span), a ternary branch requires the JUMPI's own source-map element to match
the ternary's full expression span by exact
(source_id, offset, length)equality.Why the naive fix (tried first, not part of this diff) was rejected
The obvious approach - reuse
require()'s pattern verbatim, anchoring twoBranchitems on theternary's span via the existing (non-exact)
find_anchor_branch- works for a simple, non-nestedternary, but breaks on nesting:
Because the inner ternary's own JUMPI is textually contained inside the outer ternary's span, the
"last matching JUMPI in a containing range" heuristic resolves BOTH the outer and inner branch items
to the same (inner) jump. Running only
run(true,true)/run(true,false)(outer-false path neverexercised) printed a false 100% (4/4) branch coverage - worse than the original gap, since it's
confidently wrong rather than merely absent. That failure mode does not occur with this diff (see
receipts below) because exact span equality naturally disambiguates nested decisions: solc emits a
condition-JUMPI mapped to the ternary's own full expression span, and a nested ternary's span is by
construction a strict sub-range, never identical to its parent's.
One real edge case this DOES hit: Solidity inlines modifier bodies into every function using them,
so a ternary inside a shared modifier produces multiple bytecode copies of the same source node,
each with an identical exact span (not ambiguous nesting, just literal duplication of the same
decision). The fix takes the last such match, matching
require()'s existing behavior for the samescenario - covered by a new
ternary_modifierregression test.One semantics note for anyone comparing against
if's branch convention: solc's ternary JUMPI jumpsto the true arm, so path 0 = the JUMPI's fallthrough (false arm) and path 1 = the jump target
(true arm) - inverted from how
if/requirelay out path 0/1. Noted in a code comment at theTernaryVisitorloop.Receipts
Real
forge coverageoutput against real fixture contracts (solc 0.8.10, 0.8.30, 0.8.35 allchecked), covering every shape from the issue plus nesting in both arms, nesting in the condition
position, 3-level-deep nesting, and modifier inlining:
LCOV (
BRDA) confirms the exact original failure case: outer-false shows 0 hits while everygenuinely-exercised branch shows real hit counts.
Full existing coverage regression suite (
cargo test -p forge --test cli -- coverage --test-threads 4):test result: ok. 59 passed; 0 failed- every pre-existing coverage test(
branch,branch_with_code_free_else,branch_with_storage_bytes_reads,identical_bytecodes,require,try_catch, etc.) passes unchanged, plus the 6 new tests this PR adds(
ternary_return,ternary_nested_partial,ternary_nested_outer_only,ternary_nested_false_partial,ternary_expression_contexts,ternary_modifier). New anchor unittest
ternary_anchor_rejects_missing_node_mappingpasses.Review
This went through a second independent model pass before opening (cross-vendor review gate): it
flagged the modifier-inlining ambiguity issue above (an earlier draft errored instead of taking the
last match, which would have made a modifier's ternary permanently stuck at 0/2 coverage - fixed)
and an unrelated statement-count behavior change (an earlier draft double-removed vs. reverted the
existing enclosing-statement + ternary-expression double statement-item count - reverted to keep
% Statementsbehavior identical tomain, out of scope for this fix).🤖 Generated with Claude Code