Accuracy for && and ||

[rmeissner]

Hey,
in Java when the coverage is calculated it also checks if only one and or or statement in an if is called.
Currently as far as I can see this is not the case for solifidy coverage.

function check(string typeString, uint8 currentType)
        private
        pure
        returns (bool)
    {
        return
            (mapType(typeString, 2) == currentType && mapType(typeString, 4) == currentType ) ||
            (mapType(typeString, 1) == currentType && mapType(typeString, 3) == currentType);
    }

This is satisfied if it returns true and false once, even it has not been triggered with all possible combinations.

Maybe this is also just not displayed in the html report.

[cgewecke]

Hi @rmeissner, thanks for raising this. (FWIW, a similar issue is open at Istanbul here.)

Agree this would increase accuracy and the return snippet you've provided is a nice example of where this could be useful.

However, it would be quite difficult to implement with our current design which relies on injecting Solidity event statements into the source code before compilation and tracking the tests' execution path at the log opcode in the VM. We're constrained by Solidity's grammar for conditionals.

That said, we're working on re-writing solidity-coverage to use in-memory source mappings and passively track the vm's opcode step with a websocket subscription. This additional level of detail should be possible if that approach succeeds - definitely worth aiming for.

[rmeissner]

Hey, thanks for the update. Looking forward to v2 😄

[cgewecke]

Leaving some notes on a possible solution...

0.7.0 instruments as shown below. We track line/branch hits by inspecting the vm stack at each opcode step and pick out hashes we've passed to an injected "coverage" function:

contract Test {
  function coverage_0xab123(bytes32 c__0xab123) public pure {}

  function a(uint x) public {

    coverage_0xab123(0xa9065...549e2); /* line */         
  
    if (x == 1) {
  
      coverage_0xab123(0x85e7e...afacf); /* branch */ 
      coverage_0xab123(0xfa262...4fb28); /* line */  
 
      x = 3;
  
    } else { 
      coverage_0xab123(0xb54c1...3ef6b); /* branch */ 
    }
}

If we modified the coverage function definition to return bool...

function covFn(bytes32 hash) public pure returns (bool) {
  return true; 
}

...we might be able to detect these additional branches by injecting like this:

if ( a || b ) { ... } 

// Transformed to:
if ( a && covFn(h) || b && covFn(h) ) { ... }

Additional Resources:

Gas Distortion (solc v0.5.11)	Cost
fn(bytes32 hash) public pure {}	114
fn(bytes32 hash) public pure returns (bool){ return true; }	134

• Istanbul's coberatura reporter.

[cgewecke]

Example from bZx arb

require ((
         loanDataBytes.length == 0 && // Kyber only
         sentAmounts[6] == sentAmounts[1]) || // newLoanAmount
   !OracleInterface(oracle).shouldLiquidate(
         loanOrder,
         loanPosition
   ),
   "unhealthy position"
);

[frangio]

Should be closed as fixed by #499.

[cgewecke]

Thanks @frangio ;)