Line Coverage Decreases in v0.8.9 + viaIR=true

[doublesharp]

Using [email protected] with this repo https://github.com/PaintSwap/samwitch-vrf the line coverage is decreased from 100%.

.solcover.js

module.exports = {
  configureYulOptimizer: true,
  solcOptimizerDetails: {
    peephole: false,
    inliner: false,
    jumpdestRemover: false,
    orderLiterals: true, // <-- TRUE! Stack too deep when false
    deduplicate: false,
    cse: false,
    constantOptimizer: false,
    yul: true, // <-- required to compile
  },
  skipFiles: ["BokkyPooBahsRedBlackTreeLibrary.sol", "test/"],
};

The lines that are "missing" coverage are:

[cgewecke]

@doublesharp LOL samwitch is what I used to test that the 0.8.9 fix works and now it's broken again?

Could you see if this resolves when you remove all the .solcover.js compiler config? (those options are deprecated since 0.8.7). Like this:

module.exports = {
  skipFiles: ["BokkyPooBahsRedBlackTreeLibrary.sol", "test/"],
};

If viaIR is set to true in your regular solc settings the coverage plugin should be ok.

(Be sure to run npx hardhat clean whenever you update .solcover.js as well).

[doublesharp]

@cgewecke that was our VRF repo, this is the on-chain ERC1155 order book I made with @0xSamWitch for an idle RPG called Estfor Kingdom on Fantom :)

The contracts do compile with the .solcover.js empty (except the skipFiles), but the line coverage is still off. With the other issue it was the branch coverage but here it is lines. I haven't been able to pin down anything unique about those lines except that splitting it up shows additional lines uncovered.

[cgewecke]

@doublesharp Ok cool will take a look...

[cgewecke]

@doublesharp Yes it looks like viaIR is stripping the instrumentation in those locations - it must be getting swept out across a multi-line optimization.

I guess if things were working in 0.8.6 with the special .solcover config I would downgrade to that. Am adding this issue to the tracking in #861 and will try to make minimal cases out of these - thanks so much for reporting.

[cgewecke]

@doublesharp Update - I patched (#871) and published an experimental fix for this on the rc tag which you can test with

npm install solidity-coverage@rc --save-dev

It covers line 230 correctly...

However, line 970 is still un-hit.

I've added console.logs around it and they never fire (on test or coverage) as can be seen in the CI job for this commit on my fork of the orderbook

Can you verify that the tests actually exercise that code? (It might be a legit coverage gap.)

[cgewecke]

Have published the patch in 0.8.10.

Went back to the order book code and logged the values going into the for loop above line 970

It looks like they are always equal & the condition to enter the loop is never met, e.g this state or equivalent:

_index ==>: 0
_segments.length.sub(1) ==>: 0 

...for this:

for (uint i = _index; i < _segments.length.sub(1); ++i) {
  _segments[i] = _segments[i.inc()];
}

Closing via #871 but please just ping if you have more info about this / that analysis is incorrect.

Thanks for finding and reporting this bug.

[doublesharp]

@cgewecke - thanks, those lines were actually uncovered. We missed them because the previous config I was using showed 100%, but I think that was incorrect.

I have since updated to [email protected] but the lines coverage is still off for https://github.com/PaintSwap/samwitch-orderbook/. The console.log I added is running in the tests, and logically the lines should be covered based on the others. Let me know if I should open a new issue or if you need more info than the screenshots below, thanks!

[cgewecke]

Yep, that's a bug. Patched in the latest release - could you try 0.8.11 (after running hardhat clean) when you have a chance?

I got the result below locally for that section of code (and 100%). I also commented out the unneeded solc stuff in .solcover.js and got rid of the condtional logic about solidity-coverage in the hardhat.config.ts in case that makes a difference.

[doublesharp]

0.8.11 shows 100% coverage 👍