Assert / Require are implicit conditionals but do not receive branch coverage

[cgewecke]

A PR discussion at Open-Zeppelin caught this. And here is Istanbul on the same issue.

I wonder if we could (reliably) pre-process / instrument these into conditionals somehow. . .

[cgewecke]

for require(xyz)

if(!(xyz)){ assembly{ jump(invalidJumpLabel) } }

Assembly will stop solidity from warning about throw? And how should highlighting be done?

[area]

Oh-ho! Very nice spot. We should consider that valid solidity includes:

function test(){
      var () = assert(1==1);
}

function test2(){
    assert(
      return1() 
      !=
      return2()
    );
}

The first is very unlikely to be seen, but I could believe people would use something like the second.

In theory, the general solution of turning these in to conditionals works. It's going to require some extra magic floating around though, I think. Currently, the source mapping we generate comes from the original source file, which doesn't recognise these as conditionals; as a result, all conditional ids in the map are going to be wrong.

I tried typing up what I think the procedure should be here, but I kept finding problems with each of my attempts. I'll keep thinking!

In terms of highlighting, I assume it would be the same as how we show something like:

if (x==true) { doThing() }

i.e. show the 'E' in the margin indicating that the 'Else' branch hasn't been run, even though that branch isn't 'in' the source code? There would need to be a bit added to the README to explain why asserts were being counted as branches, but I do think this is justified.

[area]

Here's a half-baked idea that might make things easier:

PreAssertEvent();
assert(1==2);
PostAssertEvent();

If nPreAssertEvents==nPostAssertEvents, then the branch where the assert fails is missing, and if nPostAssertEvents==0 then the branch where the assert succeeds is missing.

[cgewecke]

The events strategy is a really clever way of detecting here. I have implementation questions:

1. With this approach how will we include require and assert statements in the branch coverage if we're detecting this behavior at the end of the run? We should also remember to add those events to the filter file in order to keep event pollution down.

2. If (1) is tricky, what are the consequences of pre-processing these statements before we generate the initial mapping? There might be a way of writing the conditional so that it encoded position information that was detectable at instrumenter.js. i.e we looked into the consequent block statement to see variables called sc_implicit_conditional_start = startColVal etc, or something. And then coerced the necessary values there? We would also need to not parse the body of that conditional? Not sure this idea is coherent or correct . . .

3. In the multiline function calls edge case above, do you feel those should be getting line coverage? Other coverage?

[cgewecke]

Also do you want to be assigned this? You don't have to take it (but you do have to figure out how to do it, sorry).

[area]

If Istanbul doesn't make a fuss about the ordering of the branchIds (i.e. branch with id 6 is allowed to be earlier in the source code than branch with id 5), then we could plausibly add on to the branchMap object at the end of the run... maybe 😄 . I need to remind myself of so many things...!

I definitely want to try this approach before experimenting with pre-processing, though, which I think could easily get out of hand from an implementation perspective quite quickly.

Yeah, I'll take this. Given this won't rely on npm stuff, I'm much happier with this sort of thing.

Oh, my kingdom for a , operator...

[cgewecke]

Ok great! I'll happily write tests for this too, don't worry about that unless you it's helpful to you to do them.

[area]

So my rough draft of what needs doing is now on the feature/assert-require-branches branch. The test for a failing assert currently doesn't work, because we're using the 'standard' etheruemjs-vm in our tests, and so when the assert fails, all changes are reverted, and we don't have any events!

I'll come back to this tomorrow, but if you wanted to throw some tests in there to break what I've already done, I'd definitely welcome that - and any other comments you might have are more than welcome, too!

[cgewecke]

LGTM! Beautiful.

Pushed to that branch:

• Added vm-sc to the dev dependencies (it's about to get swallowed by webpack)
• Made the test vm read from allFiredEvents
• Fixed old function tests that we're passing but shouldn't have been.
• Added code to ignore solc warnings
• Removed the process.env test filter
• Strengthened events filter test to include assertion events and conditional events.
• Added gratuitous multiline require statement test.

Everything's passing! I vote merge.