Skip to content

Commit d1b2513

Browse files
grandizzygrandizzy
authored
fix(coverage): proper single path branch support (#8552)
1 parent d5ff3d3 commit d1b2513

File tree

4 files changed

+158
-62
lines changed

4 files changed

+158
-62
lines changed

‎crates/evm/coverage/src/analysis.rs

Lines changed: 65 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -211,59 +211,64 @@ impl<'a> ContractVisitor<'a> {
211211
// branch ID as we do.
212212
self.branch_id += 1;
213213

214-
// The relevant source range for the true branch is the `if(...)` statement itself
215-
// and the true body of the if statement. The false body of the
216-
// statement (if any) is processed as its own thing. If this source
217-
// range is not processed like this, it is virtually impossible to
218-
// correctly map instructions back to branches that include more
219-
// complex logic like conditional logic.
220-
let true_branch_loc = &ast::LowFidelitySourceLocation {
221-
start: node.src.start,
222-
length: true_body
223-
.src
224-
.length
225-
.map(|length| true_body.src.start - node.src.start + length),
226-
index: node.src.index,
227-
};
228-
229-
// Add the coverage item for branch 0 (true body).
230-
self.push_item(CoverageItem {
231-
kind: CoverageItemKind::Branch { branch_id, path_id: 0 },
232-
loc: self.source_location_for(true_branch_loc),
233-
hits: 0,
234-
});
235-
236214
match node.attribute::<Node>("falseBody") {
237215
// Both if/else statements.
238216
Some(false_body) => {
239-
// Add the coverage item for branch 1 (false body).
240-
// The relevant source range for the false branch is the `else` statement
241-
// itself and the false body of the else statement.
242-
self.push_item(CoverageItem {
243-
kind: CoverageItemKind::Branch { branch_id, path_id: 1 },
244-
loc: self.source_location_for(&ast::LowFidelitySourceLocation {
245-
start: node.src.start,
246-
length: false_body.src.length.map(|length| {
247-
false_body.src.start - true_body.src.start + length
217+
// Add branch coverage items only if one of true/branch bodies contains
218+
// statements.
219+
if has_statements(&true_body) || has_statements(&false_body) {
220+
// Add the coverage item for branch 0 (true body).
221+
// The relevant source range for the true branch is the `if(...)`
222+
// statement itself and the true body of the if statement.
223+
//
224+
// The false body of the statement is processed as its own thing.
225+
// If this source range is not processed like this, it is virtually
226+
// impossible to correctly map instructions back to branches that
227+
// include more complex logic like conditional logic.
228+
self.push_item(CoverageItem {
229+
kind: CoverageItemKind::Branch { branch_id, path_id: 0 },
230+
loc: self.source_location_for(&ast::LowFidelitySourceLocation {
231+
start: node.src.start,
232+
length: true_body.src.length.map(|length| {
233+
true_body.src.start - node.src.start + length
234+
}),
235+
index: node.src.index,
248236
}),
249-
index: node.src.index,
250-
}),
251-
hits: 0,
252-
});
253-
// Process the true body.
254-
self.visit_block_or_statement(&true_body)?;
255-
// Process the false body.
256-
self.visit_block_or_statement(&false_body)?;
237+
hits: 0,
238+
});
239+
// Add the coverage item for branch 1 (false body).
240+
// The relevant source range for the false branch is the `else`
241+
// statement itself and the false body of the else statement.
242+
self.push_item(CoverageItem {
243+
kind: CoverageItemKind::Branch { branch_id, path_id: 1 },
244+
loc: self.source_location_for(&ast::LowFidelitySourceLocation {
245+
start: node.src.start,
246+
length: false_body.src.length.map(|length| {
247+
false_body.src.start - true_body.src.start + length
248+
}),
249+
index: node.src.index,
250+
}),
251+
hits: 0,
252+
});
253+
254+
// Process the true body.
255+
self.visit_block_or_statement(&true_body)?;
256+
// Process the false body.
257+
self.visit_block_or_statement(&false_body)?;
258+
}
257259
}
258260
None => {
259-
// Add the coverage item for branch 1 (same true body).
260-
self.push_item(CoverageItem {
261-
kind: CoverageItemKind::Branch { branch_id, path_id: 1 },
262-
loc: self.source_location_for(true_branch_loc),
263-
hits: 0,
264-
});
265-
// Process the true body.
266-
self.visit_block_or_statement(&true_body)?;
261+
// Add single branch coverage only if it contains statements.
262+
if has_statements(&true_body) {
263+
// Add the coverage item for branch 0 (true body).
264+
self.push_item(CoverageItem {
265+
kind: CoverageItemKind::SinglePathBranch { branch_id },
266+
loc: self.source_location_for(&true_body.src),
267+
hits: 0,
268+
});
269+
// Process the true body.
270+
self.visit_block_or_statement(&true_body)?;
271+
}
267272
}
268273
}
269274

@@ -321,9 +326,7 @@ impl<'a> ContractVisitor<'a> {
321326

322327
// Add coverage for clause body only if it is not empty.
323328
if let Some(block) = clause.attribute::<Node>("block") {
324-
let statements: Vec<Node> =
325-
block.attribute("statements").unwrap_or_default();
326-
if !statements.is_empty() {
329+
if has_statements(&block) {
327330
self.push_item(CoverageItem {
328331
kind: CoverageItemKind::Statement,
329332
loc: self.source_location_for(&block.src),
@@ -502,8 +505,12 @@ impl<'a> ContractVisitor<'a> {
502505
let source_location = &item.loc;
503506

504507
// Push a line item if we haven't already
505-
if matches!(item.kind, CoverageItemKind::Statement | CoverageItemKind::Branch { .. }) &&
506-
self.last_line < source_location.line
508+
if matches!(
509+
item.kind,
510+
CoverageItemKind::Statement |
511+
CoverageItemKind::Branch { .. } |
512+
CoverageItemKind::SinglePathBranch { .. }
513+
) && self.last_line < source_location.line
507514
{
508515
self.items.push(CoverageItem {
509516
kind: CoverageItemKind::Line,
@@ -527,6 +534,12 @@ impl<'a> ContractVisitor<'a> {
527534
}
528535
}
529536

537+
/// Helper function to check if a given node contains any statement.
538+
fn has_statements(node: &Node) -> bool {
539+
let statements: Vec<Node> = node.attribute("statements").unwrap_or_default();
540+
!statements.is_empty()
541+
}
542+
530543
/// [`SourceAnalyzer`] result type.
531544
#[derive(Debug)]
532545
pub struct SourceAnalysis {

‎crates/evm/coverage/src/lib.rs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,11 @@ pub enum CoverageItemKind {
295295
/// The first path has ID 0, the next ID 1, and so on.
296296
path_id: usize,
297297
},
298+
/// A branch in the code.
299+
SinglePathBranch {
300+
/// The ID that identifies the branch.
301+
branch_id: usize,
302+
},
298303
/// A function in the code.
299304
Function {
300305
/// The name of the function.
@@ -324,6 +329,9 @@ impl Display for CoverageItem {
324329
CoverageItemKind::Branch { branch_id, path_id } => {
325330
write!(f, "Branch (branch: {branch_id}, path: {path_id})")?;
326331
}
332+
CoverageItemKind::SinglePathBranch { branch_id } => {
333+
write!(f, "Branch (branch: {branch_id}, path: 0)")?;
334+
}
327335
CoverageItemKind::Function { name } => {
328336
write!(f, r#"Function "{name}""#)?;
329337
}
@@ -408,7 +416,7 @@ impl AddAssign<&CoverageItem> for CoverageSummary {
408416
self.statement_hits += 1;
409417
}
410418
}
411-
CoverageItemKind::Branch { .. } => {
419+
CoverageItemKind::Branch { .. } | CoverageItemKind::SinglePathBranch { .. } => {
412420
self.branch_count += 1;
413421
if item.hits > 0 {
414422
self.branch_hits += 1;

‎crates/forge/src/coverage.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,13 @@ impl<'a> CoverageReporter for LcovReporter<'a> {
116116
if hits == 0 { "-".to_string() } else { hits.to_string() }
117117
)?;
118118
}
119+
CoverageItemKind::SinglePathBranch { branch_id } => {
120+
writeln!(
121+
self.destination,
122+
"BRDA:{line},{branch_id},0,{}",
123+
if hits == 0 { "-".to_string() } else { hits.to_string() }
124+
)?;
125+
}
119126
// Statements are not in the LCOV format.
120127
// We don't add them in order to avoid doubling line hits.
121128
_ => {}

‎crates/forge/tests/cli/coverage.rs

Lines changed: 77 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -464,6 +464,27 @@ contract Foo {
464464
}
465465
return true;
466466
}
467+
468+
function checkEmptyStatements(uint256 number, uint256[] memory arr) external pure returns (bool) {
469+
// Check that empty statements are covered.
470+
if (number >= arr[0]) {
471+
// Do nothing
472+
} else {
473+
// Do nothing.
474+
}
475+
if (number >= arr[0]) {}
476+
477+
return true;
478+
}
479+
480+
function singlePathCoverage(uint256 number) external pure {
481+
if (number < 10) {
482+
if (number < 5) {
483+
number++;
484+
}
485+
number++;
486+
}
487+
}
467488
}
468489
"#,
469490
)
@@ -562,23 +583,70 @@ contract FooTest is DSTest {
562583
bool result = foo.checkLt(number, arr);
563584
assertTrue(result);
564585
}
586+
587+
function test_issue_4314() external {
588+
uint256[] memory arr = new uint256[](1);
589+
arr[0] = 1;
590+
foo.checkEmptyStatements(0, arr);
591+
}
592+
593+
function test_single_path_child_branch() external {
594+
foo.singlePathCoverage(1);
595+
}
596+
597+
function test_single_path_parent_branch() external {
598+
foo.singlePathCoverage(9);
599+
}
600+
601+
function test_single_path_branch() external {
602+
foo.singlePathCoverage(15);
603+
}
565604
}
566605
"#,
567606
)
568607
.unwrap();
569608

570-
// TODO: fix following issues for 100% coverage
571-
// https://github.com/foundry-rs/foundry/issues/4309
572-
// https://github.com/foundry-rs/foundry/issues/4310
573-
// https://github.com/foundry-rs/foundry/issues/4315
574-
cmd.arg("coverage").args(["--summary".to_string()]).assert_success().stdout_eq(str![[r#"
609+
// Assert no coverage for single path branch. 2 branches (parent and child) not covered.
610+
cmd.arg("coverage")
611+
.args([
612+
"--nmt".to_string(),
613+
"test_single_path_child_branch|test_single_path_parent_branch".to_string(),
614+
])
615+
.assert_success()
616+
.stdout_eq(str![[r#"
575617
...
576-
| File | % Lines | % Statements | % Branches | % Funcs |
577-
|-------------|-----------------|-----------------|----------------|---------------|
578-
| src/Foo.sol | 100.00% (20/20) | 100.00% (23/23) | 83.33% (15/18) | 100.00% (7/7) |
579-
| Total | 100.00% (20/20) | 100.00% (23/23) | 83.33% (15/18) | 100.00% (7/7) |
618+
| File | % Lines | % Statements | % Branches | % Funcs |
619+
|-------------|----------------|----------------|----------------|---------------|
620+
| src/Foo.sol | 88.89% (24/27) | 90.00% (27/30) | 87.50% (14/16) | 100.00% (9/9) |
621+
| Total | 88.89% (24/27) | 90.00% (27/30) | 87.50% (14/16) | 100.00% (9/9) |
580622
581623
"#]]);
624+
625+
// Assert no coverage for single path child branch. 1 branch (child) not covered.
626+
cmd.forge_fuse()
627+
.arg("coverage")
628+
.args(["--nmt".to_string(), "test_single_path_child_branch".to_string()])
629+
.assert_success()
630+
.stdout_eq(str![[r#"
631+
...
632+
| File | % Lines | % Statements | % Branches | % Funcs |
633+
|-------------|----------------|----------------|----------------|---------------|
634+
| src/Foo.sol | 96.30% (26/27) | 96.67% (29/30) | 93.75% (15/16) | 100.00% (9/9) |
635+
| Total | 96.30% (26/27) | 96.67% (29/30) | 93.75% (15/16) | 100.00% (9/9) |
636+
637+
"#]]);
638+
639+
// Assert 100% coverage.
640+
cmd.forge_fuse().arg("coverage").args(["--summary".to_string()]).assert_success().stdout_eq(
641+
str![[r#"
642+
...
643+
| File | % Lines | % Statements | % Branches | % Funcs |
644+
|-------------|-----------------|-----------------|-----------------|---------------|
645+
| src/Foo.sol | 100.00% (27/27) | 100.00% (30/30) | 100.00% (16/16) | 100.00% (9/9) |
646+
| Total | 100.00% (27/27) | 100.00% (30/30) | 100.00% (16/16) | 100.00% (9/9) |
647+
648+
"#]],
649+
);
582650
});
583651

584652
forgetest!(test_function_call_coverage, |prj, cmd| {

0 commit comments

Comments
 (0)