bug(coverage): with --ir-minimum enabled is showing wrong readings on require statements

[ChaituKReddy]

Component

Forge

Have you ensured that all of these are up to date?

• Foundry
• Foundryup

What version of Foundry are you on?

forge 0.2.0 (8549aad 2024-08-19T00:23:37.767039000Z)

What command(s) is the bug in?

forge coverage --ir-minimum

Operating System

macOS (Apple Silicon)

Describe the bug

From the counter contract example, the following code snippet and the test case for it yields 100% coverage with just

forge coverage

function addNum(uint256 _num) public {
     require(_num > 0, "Zero Number");
     number += _num;
}

function testAddNum() public {
        vm.expectRevert("Zero Number");
        counter.addNum(0);

        counter.addNum(1);
}

But the same with forge coverage --ir-minimum yields below report

The report is attached below

I'm trying to use the --ir-minimum in my project code base where I'm facing the same issue, this is a sample test to reproduce the issue.

[zerosnacks]

Hi @ChaituKReddy thanks for flagging this. Due to the AST transformation the via-ir pipeline performs this functionality is available as-is as stated. That said I think it is important that we keep track of issues, it is just that there likely is no structural solution yet in the short term.

Related: #3527

[VGabriel45]

@zerosnacks does this mean that using Solidity "0.8.26" with via-ir and "forge coverage" will output invalid coverage report ?
I am getting some unexpected behaviour when running "forge coverage --ir-minimum" in a sense that I don't get any error but instead some foundry tests are included in the coverage, un-covered lines output in the table are not correct and more.

[zerosnacks]

@zerosnacks does this mean that using Solidity "0.8.26" with via-ir and "forge coverage" will output invalid coverage report ? I am getting some unexpected behaviour when running "forge coverage --ir-minimum" in a sense that I don't get any error but instead some foundry tests are included in the coverage, un-covered lines output in the table are not correct and more.

via-ir always applies some methods of optimization which invalidates the AST making it very unreliable. -ir-minimum attempts to apply the least amount of optimizations but it is still at a best attempt and issues are to be expected. It will likely require a significant rewrite of forge coverage post 1.0 with a different instrumentation insertions that can persist through the optimizations to address this.

[DaniPopes]

cc @grandizzy

In general compiler optimizations should not be used at all for coverage for various reasons. There are no plans to support via-ir. We provide ir-minimum because of stack-too-deep errors, but it should be avoided when possible.

[VGabriel45]

cc @grandizzy

In general compiler optimizations should not be used at all for coverage for various reasons. There are no plans to support via-ir. We provide ir-minimum because of stack-too-deep errors, but it should be avoided when possible.

How can one run "forge coverage" with Solidity's version 0.8.26 and using the new require statement with custom errors sugar syntax in the contracts ?

[zerosnacks]

cc @grandizzy
In general compiler optimizations should not be used at all for coverage for various reasons. There are no plans to support via-ir. We provide ir-minimum because of stack-too-deep errors, but it should be avoided when possible.

How can one run "forge coverage" with Solidity's version 0.8.26 and using the new require statement with custom errors sugar syntax in the contracts ?

That is unfortunately a via-ir only feature therefore not compatible with forge coverage, ref: https://soliditylang.org/blog/2024/05/21/solidity-0.8.26-release-announcement/

[tmigone]

@VGabriel45 did you find a workaround? I'm facing the exact same problem.

[ChaituKReddy]

@tmigone

Based on my understanding, unless you're encountering a stack too deep issue, it's recommended not to use the --ir-minimum flag with forge coverage.

Instead, consider either:

1. Refactoring your code by replacing require statements with custom errors and using revert to handle exceptions, or
2. Waiting for future Solidity versions that will natively support require with custom errors, eliminating the need for the --via-ir flow.

This approach should help with maintaining coverage accuracy.

[VGabriel45]

@VGabriel45 did you find a workaround? I'm facing the exact same problem.

What we did is we created a side-branch with the same code but without the new require statement with custom errors which was forcing us to use via-ir in the main branch, now we are getting the coverage results for the code but this is a temporary work-around ... untill the Solidity devs do something about it.

[tmigone]

Waiting for future Solidity versions that will natively support require with custom errors, eliminating the need for the --via-ir flow.

@ChaituKReddy is there any indication for how long this could take? Do you know if it's been worked on?

@VGabriel45 thanks for your suggestion, seems like very cumbersome it's unfortunate that we have to resort to these techniques 😞

[zerosnacks]

That is unfortunately a via-ir only feature therefore not compatible with forge coverage, ref: https://soliditylang.org/blog/2024/05/21/solidity-0.8.26-release-announcement/

Update: 0.8.27 adds support for custom errors in require to the legacy pipeline: https://soliditylang.org/blog/2024/09/04/solidity-0.8.27-release-announcement/

[jenpaff]

duplicate of #8840