foundry-rs / foundry

Foundry is a blazing fast, portable and modular toolkit for Ethereum application development written in Rust.
https://getfoundry.sh
Apache License 2.0
7.89k stars 1.58k forks source link

bug(forge): incorrect branch coverage when passing array elements to custom errors #4309

Open PaulRBerg opened 1 year ago

PaulRBerg commented 1 year ago

Component

Forge

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

What version of Foundry are you on?

forge 0.2.0 (249538f 2023-02-08T00:12:05.805004Z)

What command(s) is the bug in?

forge coverage

Operating System

macOS (Apple Silicon)

Describe the bug

This is a strange bug. Reproducing it in a repo with a clean slate took me hours, but I did eventually manage to get to the bottom of it.

Take the following unsuspecting contract:

contract Foo {
    error Gte1(uint256 number, uint256 firstElement);

    function checkLt(uint256 number, uint256[] memory arr) external pure returns (bool) {
        if (number >= arr[0]) {
            revert Gte1(number, arr[0]);
        }
        return true;
    }
}

And the following tests (hidden by default for brevity reasons, click the toggle below to collapse them):

Click me to toggle the tests ```solidity contract FooTest is Test { Foo internal foo = new Foo(); uint256[] internal arr; function setUp() public virtual { arr.push(1); } function test_RevertWhen_Gt() external { uint256 number = 2; vm.expectRevert(abi.encodeWithSelector(Foo.Gte1.selector, number, arr[0])); foo.checkLt(number, arr); } function test_RevertWhen_Eq() external { uint256 number = 1; vm.expectRevert(abi.encodeWithSelector(Foo.Gte1.selector, number, arr[0])); foo.checkLt(number, arr); } function test_Lt() external { uint256 number = 0; bool result = foo.checkLt(number, arr); assertTrue(result); } } ```

Now, run forge coverage. You will get this report:

| File        | % Lines       | % Statements  | % Branches   | % Funcs       |
|-------------|---------------|---------------|--------------|---------------|
| src/Foo.sol | 100.00% (3/3) | 100.00% (3/3) | 50.00% (1/2) | 100.00% (1/1) |
| Total       | 100.00% (3/3) | 100.00% (3/3) | 50.00% (1/2) | 100.00% (1/1) |

Notice that the branch coverage is 50% instead of 100%, even if we have tests that account for the full gamut of possibilities (>, ==, and <).

Now, here's the strangest thing - just by getting rid of the the firstElement argument of the custom error, branch coverage increases to 100%. Also, passing the number instead of arr[0] achieves the same 100% coverage, like this:

| File        | % Lines       | % Statements  | % Branches    | % Funcs       |
|-------------|---------------|---------------|---------------|---------------|
| src/Foo.sol | 100.00% (3/3) | 100.00% (3/3) | 100.00% (2/2) | 100.00% (1/1) |
| Total       | 100.00% (3/3) | 100.00% (3/3) | 100.00% (2/2) | 100.00% (1/1) |
zerosnacks commented 1 week ago

Able to reproduce w/ given instructions, very strange

Will require further investigation