Add logicalOR coverage for "require" conditions

lib/coverage.js

@@ -69,6 +69,9 @@ class Coverage {
       const data = collectedData[hash];
       const contractPath = collectedData[hash].contractPath;
       const id = collectedData[hash].id;
+
+      // NB: Any branch using the injected fn which returns boolean will have artificially
+      // doubled hits (because of something internal to Solidity about how the stack is managed)
       const hits = collectedData[hash].hits;
 
       switch(collectedData[hash].type){
@@ -76,6 +79,7 @@ class Coverage {
         case 'function':   this.data[contractPath].f[id] = hits;                       break;
         case 'statement':  this.data[contractPath].s[id] = hits;                       break;
         case 'branch':     this.data[contractPath].b[id][data.locationIdx] = hits;     break;
+        case 'logicalOR':  this.data[contractPath].b[id][data.locationIdx] = hits / 2; break;
         case 'assertPre':  this.assertData[contractPath][id].preEvents = hits;         break;
         case 'assertPost': this.assertData[contractPath][id].postEvents = hits;        break;
       }

lib/injector.js

@@ -13,7 +13,12 @@ class Injector {
   }
 
   _getInjectable(fileName, hash, type){
-    return `${this._getMethodIdentifier(fileName)}(${hash}); /* ${type} */ \n`;
+    switch(type){
+      case 'logicalOR':
+        return ` && ${this._getTrueMethodIdentifier(fileName)}(${hash}))`;
+      default:
+        return `${this._getDefaultMethodIdentifier(fileName)}(${hash}); /* ${type} */ \n`;
+    }
   }
 
   _getHash(fileName) {
@@ -21,10 +26,16 @@ class Injector {
     return web3Utils.keccak256(`${fileName}:${this.hashCounter}`);
   }
 
-  _getMethodIdentifier(fileName){
+  // Method returns void
+  _getDefaultMethodIdentifier(fileName){
     return `coverage_${web3Utils.keccak256(fileName).slice(0,10)}`
   }
 
+  // Method returns boolean true
+  _getTrueMethodIdentifier(fileName){
+    return `coverage_true${web3Utils.keccak256(fileName).slice(0,10)}`
+  }
+
   _getInjectionComponents(contract, injectionPoint, fileName, type){
     const { start, end } = this._split(contract, injectionPoint);
     const hash = this._getHash(fileName)
@@ -39,17 +50,29 @@ class Injector {
   }
 
   /**
-   * Generates a solidity statement injection. Declared once per fn.
+   * Generates a solidity statement injection defining a method
-   * Definition is the same for every fn in file.
+   * *which returns void* to pass instrumentation hash to.
    * @param  {String} fileName
    * @return {String}          ex: bytes32[1] memory _sc_82e0891
    */
-  _getHashMethodDefinition(fileName){
+  _getDefaultMethodDefinition(fileName){
     const hash = web3Utils.keccak256(fileName).slice(0,10);
-    const method = this._getMethodIdentifier(fileName);
+    const method = this._getDefaultMethodIdentifier(fileName);
     return `\nfunction ${method}(bytes32 c__${hash}) public pure {}\n`;
   }
 
+  /**
+   * Generates a solidity statement injection defining a method
+   * *which returns boolean true* to pass instrumentation hash to.
+   * @param  {String} fileName
+   * @return {String}          ex: bytes32[1] memory _sc_82e0891
+   */
+  _getTrueMethodDefinition(fileName){
+    const hash = web3Utils.keccak256(fileName).slice(0,10);
+    const method = this._getTrueMethodIdentifier(fileName);
+    return `\nfunction ${method}(bytes32 c__${hash}) public pure returns (bool){ return true; }\n`;
+  }
+
   injectLine(contract, fileName, injectionPoint, injection, instrumentation){
     const type = 'line';
     const { start, end } = this._split(contract, injectionPoint);
@@ -196,7 +219,37 @@ class Injector {
   injectHashMethod(contract, fileName, injectionPoint, injection, instrumentation){
     const start = contract.instrumented.slice(0, injectionPoint);
     const end = contract.instrumented.slice(injectionPoint);
-    contract.instrumented = `${start}${this._getHashMethodDefinition(fileName)}${end}`;
+    contract.instrumented = `${start}` +
+                            `${this._getDefaultMethodDefinition(fileName)}` +
+                            `${this._getTrueMethodDefinition(fileName)}` +
+                            `${end}`;
+  }
+
+  injectOpenParen(contract, fileName, injectionPoint, injection, instrumentation){
+    const start = contract.instrumented.slice(0, injectionPoint);
+    const end = contract.instrumented.slice(injectionPoint);
+    contract.instrumented = `${start}(${end}`;
+  }
+
+  injectLogicalOR(contract, fileName, injectionPoint, injection, instrumentation){
+    const type = 'logicalOR';
+
+    const {
+      start,
+      end,
+      hash,
+      injectable
+    } = this._getInjectionComponents(contract, injectionPoint, fileName, type);
+
+    instrumentation[hash] = {
+      id: injection.branchId,
+      locationIdx: injection.locationIdx,
+      type: type,
+      contractPath: fileName,
+      hits: 0
+    }
+
+    contract.instrumented = `${start}${injectable}${end}`;
   }
 };
 

lib/instrumenter.js

@@ -84,7 +84,12 @@ class Instrumenter {
 
       // Line instrumentation has to happen first
       contract.injectionPoints[injectionPoint].sort((a, b) => {
-        const injections = ['injectBranch', 'injectEmptyBranch', 'injectLine'];
+        const injections = [
+          'injectLogicalOR',
+          'injectBranch',
+          'injectEmptyBranch',
+          'injectLine'
+        ];
         return injections.indexOf(b.type) - injections.indexOf(a.type);
       });
 

lib/parse.js

@@ -20,17 +20,40 @@ parse.Block = function(contract, expression) {
   }
 };
 
-parse.BinaryOperation = function(contract, expression) {
+parse.BinaryOperation = function(contract, expression, skipStatementRegistry) {
+  // Regular binary operation
+  if (!skipStatementRegistry){
     register.statement(contract, expression);
+
+  // LogicalOR conditional search...
+  } else {
+    parse[expression.left.type] &&
+    parse[expression.left.type](contract, expression.left, true);
+
+    parse[expression.right.type] &&
+    parse[expression.right.type](contract, expression.right, true);
+
+    if (expression.operator === '||'){
+      register.logicalOR(contract, expression);
+    }
+  }
 }
 
-parse.FunctionCall = function(contract, expression) {
+parse.FunctionCall = function(contract, expression, skipStatementRegistry) {
   // In any given chain of call expressions, only the last one will fail this check.
   // This makes sure we don't instrument a chain of expressions multiple times.
   if (expression.expression.type !== 'FunctionCall') {
+
+    // Don't register sub-expressions (like intermediate method calls)
+    if (!skipStatementRegistry){
       register.statement(contract, expression);
+    }
+
     if (expression.expression.name === 'assert' || expression.expression.name === 'require') {
       register.assertOrRequire(contract, expression);
+      expression.arguments.forEach(arg => {
+        parse[arg.type] && parse[arg.type](contract, arg, true);
+      });
     }
     parse[expression.expression.type] &&
     parse[expression.expression.type](contract, expression.expression);

lib/registrar.js

@@ -176,6 +176,86 @@ class Registrar {
     };
   };
 
+  /**
+   * Registers injections for branch measurements. Used by logicalOR registration method.
+   * @param  {Object} contract   instrumentation target
+   * @param  {Object} expression AST node
+   */
+  addNewLogicalORBranch(contract, expression) {
+    const startContract = contract.instrumented.slice(0, expression.range[0]);
+    const startline = ( startContract.match(/\n/g) || [] ).length + 1;
+
+    contract.branchId += 1;
+
+    // NB locations for if branches in istanbul are zero
+    // length and associated with the start of the if.
+    contract.branchMap[contract.branchId] = {
+      line: startline,
+      type: 'cond-expr',
+      locations: [{
+        start: {
+          line: startline, column: expression.left.range[0],
+        },
+        end: {
+          line: startline, column: expression.left.range[1],
+        },
+      }, {
+        start: {
+          line: startline, column: expression.right.range[0],
+        },
+        end: {
+          line: startline, column: expression.right.range[1],
+        },
+      }],
+    };
+  };
+
+  /**
+   * Registers injections for logicalOR clause measurements (branches)
+   * @param  {Object} contract     instrumentation target
+   * @param  {Object} expression   AST node
+   * @param  {Number} injectionIdx pre/post branch index (left=0, right=1)
+   */
+  logicalOR(contract, expression) {
+    this.addNewLogicalORBranch(contract, expression);
+
+    // Left
+    this._createInjectionPoint(
+      contract,
+      expression.left.range[0],
+      {
+        type: 'injectOpenParen',
+      }
+    );
+    this._createInjectionPoint(
+      contract,
+      expression.left.range[1] + 1,
+      {
+        type: 'injectLogicalOR',
+        branchId: contract.branchId,
+        locationIdx: 0
+      }
+    );
+
+    // Right
+    this._createInjectionPoint(
+      contract,
+      expression.right.range[0],
+      {
+        type: 'injectOpenParen',
+      }
+    );
+    this._createInjectionPoint(
+      contract,
+      expression.right.range[1] + 1,
+      {
+        type: 'injectLogicalOR',
+        branchId: contract.branchId,
+        locationIdx: 1
+      }
+    );
+  }
+
   /**
    * Registers injections for assert/require statement measurements (branches)
    * @param  {Object} contract   instrumentation target

test/sources/solidity/contracts/assert/Require-fn-reason.sol

@@ -0,0 +1,11 @@
+pragma solidity ^0.5.0;
+
+contract Test {
+  function getBool(bool _b) public pure returns (bool){
+    return _b;
+  }
+
+  function a(bool _a) public {
+    require(getBool(_a), "mi ritrovai per una selva oscura");
+  }
+}

test/sources/solidity/contracts/assert/Require-fn.sol

@@ -0,0 +1,11 @@
+pragma solidity ^0.5.0;
+
+contract Test {
+  function getBool(bool _b) public pure returns (bool){
+    return _b;
+  }
+
+  function a(bool _a) public {
+    require(getBool(_a));
+  }
+}

test/sources/solidity/contracts/or/return-or.sol

@@ -2,7 +2,6 @@ pragma solidity ^0.5.0;
 
 contract Test {
     function a(uint x) public pure returns (bool) {
-        return (x == 1 && true) ||
+        return (x == 1 && true) || (x == 2 && true);
-               (x == 2 && true);
     }
 }

test/units/assert.js

@@ -99,4 +99,44 @@ describe('asserts and requires', () => {
       1: 2,
     });
   });
+
+  it('should cover require statements with method arguments', async function() {
+    const contract = await util.bootstrapCoverage('assert/Require-fn', provider, collector);
+    coverage.addContract(contract.instrumented, util.filePath);
+    await contract.instance.a(true);
+    const mapping = coverage.generate(contract.data, util.pathPrefix);
+
+    assert.deepEqual(mapping[util.filePath].l, {
+      5: 1, 9: 1,
+    });
+    assert.deepEqual(mapping[util.filePath].b, {
+      1: [1, 0],
+    });
+    assert.deepEqual(mapping[util.filePath].s, {
+      1: 1, 2: 1
+    });
+    assert.deepEqual(mapping[util.filePath].f, {
+      1: 1, 2: 1
+    });
+  });
+
+  it('should cover require statements with method arguments & reason string', async function() {
+    const contract = await util.bootstrapCoverage('assert/Require-fn-reason', provider, collector);
+    coverage.addContract(contract.instrumented, util.filePath);
+    await contract.instance.a(true);
+    const mapping = coverage.generate(contract.data, util.pathPrefix);
+
+    assert.deepEqual(mapping[util.filePath].l, {
+      5: 1, 9: 1,
+    });
+    assert.deepEqual(mapping[util.filePath].b, {
+      1: [1, 0],
+    });
+    assert.deepEqual(mapping[util.filePath].s, {
+      1: 1, 2: 1
+    });
+    assert.deepEqual(mapping[util.filePath].f, {
+      1: 1, 2: 1
+    });
+  });
 });

test/units/or.js

@@ -69,7 +69,7 @@ describe('logical OR branches', () => {
       5: 1,
     });
     assert.deepEqual(mapping[util.filePath].b, {
-      1: [1, 0],
+      1: [1, 0], 2: [1, 0]
     });
     assert.deepEqual(mapping[util.filePath].s, {
       1: 1,
@@ -92,7 +92,7 @@ describe('logical OR branches', () => {
       5: 2,
     });
     assert.deepEqual(mapping[util.filePath].b, {
-      1: [2, 0],
+      1: [2, 0], 2: [1, 1]
     });
     assert.deepEqual(mapping[util.filePath].s, {
       1: 2,
@@ -336,7 +336,7 @@ describe('logical OR branches', () => {
       5: 1, 9: 1
     });
     assert.deepEqual(mapping[util.filePath].b, {
-      1: [1, 0],
+      1: [1, 0], 2: [0, 1]
     });
     assert.deepEqual(mapping[util.filePath].s, {
       1: 1, 2: 1,