Add branch coverage for logical OR conditions (#499)

* Add initial OR coverage test cases

* Add logicalOR coverage for "require" conditions

* Add logicalOR coverage for "while" conditions

* Add logicalOR coverage for "return" conditions

* Add logicalOR coverage for "if" conditions

* Add logicalOR branch highlighting for html report

Author: cgewecke

lib/coverage.js

@@ -69,13 +69,17 @@ 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){
-        case 'line':       this.data[contractPath].l[id] = hits;                   break;
-        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 'line':       this.data[contractPath].l[id] = hits;                       break;
+        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 'requirePre':  this.requireData[contractPath][id].preEvents = hits;     break;
         case 'requirePost': this.requireData[contractPath][id].postEvents = hits;    break;
       }

lib/injector.js

@@ -13,18 +13,29 @@ class Injector {
   }
 
   _getInjectable(id, hash, type){
-    return `${this._getMethodIdentifier(id)}(${hash}); /* ${type} */ \n`;
+    switch(type){
+      case 'logicalOR':
+        return ` && ${this._getTrueMethodIdentifier(id)}(${hash}))`;
+      default:
+        return `${this._getDefaultMethodIdentifier(id)}(${hash}); /* ${type} */ \n`;
+    }
   }
 
   _getHash(id) {
     this.hashCounter++;
     return web3Utils.keccak256(`${id}:${this.hashCounter}`);
   }
 
-  _getMethodIdentifier(id){
+  // Method returns void
+  _getDefaultMethodIdentifier(id){
     return `c_${web3Utils.keccak256(id).slice(0,10)}`
   }
 
+  // Method returns boolean true
+  _getTrueMethodIdentifier(id){
+    return `c_true${web3Utils.keccak256(id).slice(0,10)}`
+  }
+
   _getInjectionComponents(contract, injectionPoint, id, type){
     const { start, end } = this._split(contract, injectionPoint);
     const hash = this._getHash(id)
@@ -39,17 +50,29 @@ class Injector {
   }
 
   /**
-   * Generates a solidity statement injection. Declared once per fn.
-   * Definition is the same for every fn in file.
+   * Generates a solidity statement injection defining a method
+   * *which returns void* to pass instrumentation hash to.
    * @param  {String} id
    * @return {String}          ex: bytes32[1] memory _sc_82e0891
    */
-  _getHashMethodDefinition(id, contract){
+  _getDefaultMethodDefinition(id){
     const hash = web3Utils.keccak256(id).slice(0,10);
-    const method = this._getMethodIdentifier(id);
+    const method = this._getDefaultMethodIdentifier(id);
     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(id){
+    const hash = web3Utils.keccak256(id).slice(0,10);
+    const method = this._getTrueMethodIdentifier(id);
+    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);
@@ -204,7 +227,38 @@ class Injector {
     const start = contract.instrumented.slice(0, injectionPoint);
     const end = contract.instrumented.slice(injectionPoint);
     const id = `${fileName}:${injection.contractName}`;
-    contract.instrumented = `${start}${this._getHashMethodDefinition(id)}${end}`;
+    contract.instrumented = `${start}` +
+                            `${this._getDefaultMethodDefinition(id)}` +
+                            `${this._getTrueMethodDefinition(id)}` +
+                            `${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 id = `${fileName}:${injection.contractName}`;
+
+    const {
+      start,
+      end,
+      hash,
+      injectable
+    } = this._getInjectionComponents(contract, injectionPoint, id, type);
+
+    instrumentation[hash] = {
+      id: injection.branchId,
+      locationIdx: injection.locationIdx,
+      type: type,
+      contractPath: fileName,
+      hits: 0
+    }
+
+    contract.instrumented = `${start}${injectable}${end}`;
   }
 };
 

lib/instrumenter.js

@@ -88,7 +88,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

@@ -30,17 +30,47 @@ parse.Block = function(contract, expression) {
   }
 };
 
-parse.BinaryOperation = function(contract, expression) {
-  register.statement(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.TupleExpression = function(contract, expression, skipStatementRegistry) {
+  expression.components.forEach(component => {
+    parse[component.type] &&
+    parse[component.type](contract, component, skipStatementRegistry);
+  });
 }
 
-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') {
-    register.statement(contract, expression);
+
+    // Don't register sub-expressions (like intermediate method calls)
+    if (!skipStatementRegistry){
+      register.statement(contract, expression);
+    }
+
     if (expression.expression.name === 'require') {
       register.requireBranch(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);
@@ -50,10 +80,13 @@ parse.FunctionCall = function(contract, expression) {
   }
 };
 
-parse.Conditional = function(contract, expression) {
-  register.statement(contract, expression);
-  // TODO: Investigate node structure
-  // There are potential substatements here we aren't measuring
+parse.Conditional = function(contract, expression, skipStatementRegistry) {
+  if (!skipStatementRegistry){
+    register.statement(contract, expression);
+  }
+
+  parse[expression.condition.type] &&
+  parse[expression.condition.type](contract, expression.condition, skipStatementRegistry);
 };
 
 parse.ContractDefinition = function(contract, expression) {
@@ -130,6 +163,10 @@ parse.FunctionDefinition = function(contract, expression) {
 parse.IfStatement = function(contract, expression) {
   register.statement(contract, expression);
   register.ifStatement(contract, expression);
+
+  parse[expression.condition.type] &&
+  parse[expression.condition.type](contract, expression.condition, true);
+
   parse[expression.trueBody.type] &&
   parse[expression.trueBody.type](contract, expression.trueBody);
 
@@ -173,6 +210,10 @@ parse.SourceUnit = function(contract, expression) {
 
 parse.ReturnStatement = function(contract, expression) {
   register.statement(contract, expression);
+
+  expression.expression &&
+  parse[expression.expression.type] &&
+  parse[expression.expression.type](contract, expression.expression, true);
 };
 
 // TODO:Investigate node structure
@@ -203,6 +244,10 @@ parse.VariableDeclarationStatement = function (contract, expression) {
 
 parse.WhileStatement = function (contract, expression) {
   register.statement(contract, expression);
+
+  parse[expression.condition.type] &&
+  parse[expression.condition.type](contract, expression.condition, true);
+
   parse[expression.body.type] &&
   parse[expression.body.type](contract, expression.body);
 };

lib/registrar.js

@@ -186,6 +186,107 @@ 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) {
+    let start;
+
+    // Instabul HTML highlighting location data...
+    const leftZero = expression.left.range[0];
+    const leftOne = expression.left.range[1];
+    const rightZero = expression.right.range[0];
+    const rightOne = expression.right.range[1];
+
+    start = contract.instrumented.slice(0, leftZero);
+    const leftStartLine = ( start.match(/\n/g) || [] ).length + 1;
+    const leftStartCol = leftZero - start.lastIndexOf('\n') - 1;
+
+    start = contract.instrumented.slice(0, leftOne);
+    const leftEndLine = ( start.match(/\n/g) || [] ).length + 1;
+    const leftEndCol = leftOne - start.lastIndexOf('\n') - 1;
+
+    start = contract.instrumented.slice(0, rightZero);
+    const rightStartLine = ( start.match(/\n/g) || [] ).length + 1;
+    const rightStartCol = rightZero - start.lastIndexOf('\n') - 1;
+
+    start = contract.instrumented.slice(0, rightOne);
+    const rightEndLine = ( start.match(/\n/g) || [] ).length + 1;
+    const rightEndCol = rightOne - start.lastIndexOf('\n') - 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: leftStartLine,
+      type: 'cond-expr',
+      locations: [{
+        start: {
+          line: leftStartLine, column: leftStartCol,
+        },
+        end: {
+          line: leftEndLine, column: leftEndCol,
+        },
+      }, {
+        start: {
+          line: rightStartLine, column: rightStartCol,
+        },
+        end: {
+          line: rightEndLine, column: rightEndCol,
+        },
+      }],
+    };
+  };
+
+  /**
+   * 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 require statement measurements (branches)
    * @param  {Object} contract   instrumentation target

test/integration/projects/logical-or/.solcover.js

@@ -0,0 +1,8 @@
+// Testing hooks
+const fn = (msg, config) => config.logger.log(msg);
+
+module.exports = {
+  skipFiles: ['Migrations.sol'],
+  silent: process.env.SILENT ? true : false,
+  istanbulReporter: ['json-summary', 'text'],
+}

test/integration/projects/logical-or/buidler.config.js

@@ -0,0 +1,7 @@
+const { loadPluginFile } = require("@nomiclabs/buidler/plugins-testing");
+loadPluginFile(__dirname + "/../plugins/buidler.plugin");
+usePlugin("@nomiclabs/buidler-truffle5");
+
+module.exports={
+  defaultNetwork: "buidlerevm"
+};