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

bug: coverage report misses return-declared variables if returned as-is #7476

Closed
2 tasks done
stevieraykatz opened this issue Mar 22, 2024 · 3 comments
Closed
2 tasks done
Labels
C-forge Command: forge Cmd-forge-coverage Command: forge coverage T-bug Type: bug

Comments

@stevieraykatz
Copy link

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 (545cd0b 2024-03-14T00:20:00.210934000Z)

What command(s) is the bug in?

forge coverage

Operating System

macOS (Apple Silicon)

Describe the bug

If a variable is defined as part of a return statement and then returns uninitialized, foundry coverage misses this. I found this issue when improving test coverage for this method in ERC1271.sol. It was trivial to recreate this issue in a stripped down experiment:

Contract

contract Coverage {
    function notCovered() public pure returns (uint256 a, bytes32 b, bytes[] memory c) {
        a = 1;
        b = b;
        c = c;
    }

    function covered() public pure returns (uint256 a, bytes32 b, bytes[] memory c) {
        a = 1;
        b = bytes32(0);
        c = new bytes[](0);
    }
}

Test

contract CoverageTest is Test {
    Coverage public coverage;
    function setUp() public {
        coverage = new Coverage();
    }

    function test_notCovered() public {
        (uint256 a, bytes32 b, bytes[] memory c) = coverage.notCovered();
        assertEq(a, 1);
        assertEq(b, bytes32(0));
        assertEq(c, new bytes[](0));
    }

    function test_covered() public {
        (uint256 a, bytes32 b, bytes[] memory c) = coverage.covered();
        assertEq(a, 1);
        assertEq(b, bytes32(0));
        assertEq(c, new bytes[](0));
    }
}

Coverage report

Uncovered for src/Coverage.sol:
- Line (location: source ID 25, line 8, chars 198-203, hits: 0)
- Statement (location: source ID 25, line 8, chars 198-203, hits: 0)
- Line (location: source ID 25, line 9, chars 213-218, hits: 0)
- Statement (location: source ID 25, line 9, chars 213-218, hits: 0)

where lines 8 and 9 refer to b = b; and c = c; in method notCovered().

@stevieraykatz stevieraykatz added the T-bug Type: bug label Mar 22, 2024
@zerosnacks zerosnacks added C-forge Command: forge Cmd-forge-coverage Command: forge coverage labels Jul 12, 2024
@zerosnacks zerosnacks changed the title Coverage report misses return-declared variables if returned as-is bug: coverage report misses return-declared variables if returned as-is Jul 12, 2024
@zerosnacks
Copy link
Member

Hi @stevieraykatz thanks for reporting this

Screenshot from 2024-07-12 16-03-08

I'm able to reproduce the bug, added a runnable repro here: https://github.com/zerosnacks/foundry-bug-7476-repro

@grandizzy
Copy link
Collaborator

yep, that's similar with #4311 where no-effect statements (b = b / c = c) are not ignored, will provide a fix for this scenario

@grandizzy
Copy link
Collaborator

A PR was made for this edge case, however per comment here #8442 (comment) there are other ways to replicate same behavior. Fixing all these scenarios will introduce code complexity that we'd rather avoid. More, having coverage reporting such lines should contribute to keeping code clean. We ack the limitation and close this as won't fix, please reopen if you think it's something critical and should be addressed. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge Cmd-forge-coverage Command: forge coverage T-bug Type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants