Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(coverage): treat assert/require as lines instead branch #8413

Merged
merged 6 commits into from
Jul 11, 2024

Conversation

grandizzy
Copy link
Collaborator

@grandizzy grandizzy commented Jul 11, 2024

Motivation

Closes #4294

  • assert/require are treated as branches hence always reporting less coverage (branch 0 not hit)

Solution

  • remove assert/require check (do not add branches but treat them as regular lines instead, already added as line kind when self.push_item as a statement)
  • remove duped code, group function node type with others
  • unit test to make sure assert/require does not add branches

@grandizzy grandizzy marked this pull request as ready for review July 11, 2024 09:47
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great!

Comment on lines -332 to -340
let expr: Option<Node> = node.attribute("expression");
if let Some(NodeType::Identifier) = expr.as_ref().map(|expr| &expr.node_type) {
// Might be a require/assert call
let name: Option<String> = expr.and_then(|expr| expr.attribute("name"));
if let Some("assert" | "require") = name.as_deref() {
self.push_branches(&node.src, self.branch_id);
self.branch_id += 1;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah okay, this is now no longer required because all this did was check for assert | require

crates/forge/tests/cli/coverage.rs Outdated Show resolved Hide resolved
@grandizzy grandizzy requested a review from DaniPopes July 11, 2024 14:46
@grandizzy grandizzy merged commit 309a2f9 into foundry-rs:master Jul 11, 2024
20 checks passed
@grandizzy grandizzy deleted the issue-4294 branch July 11, 2024 17:10
@KholdStare
Copy link
Contributor

KholdStare commented Jul 11, 2024

@mattsse @grandizzy @DaniPopes I think require should still be a branch. Can you check my reasoning?

There is a subtle difference between require and assert:

  • assert deals with asserting invariants that should hold no matter what the input is. Hence why it shouldn't be possible to hit "the other branch" - that would indicate a bug in the code.
  • require deals with pre/post conditions that may get violated due to erroneous/malicious inputs. Both sides of the branch need to be checked as both cases can occur. That's why we have expectRevert in tests.

There is a semantic difference and it's very well highlighted in the solidity docs:

Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. Language analysis tools can evaluate your contract to identify the conditions and function calls which will cause a Panic.

There is also a concrete/practical difference - the SMTChecker in solc relies on this distinction:

The SMTChecker module automatically tries to prove that the code satisfies the specification given by require and assert statements. That is, it considers require statements as assumptions and tries to prove that the conditions inside assert statements are always true. If an assertion failure is found, a counterexample may be given to the user showing how the assertion can be violated.

Does this make sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(forge): disable coverage for assertions
4 participants