Add modifier-invocation-as-branch coverage (#588)

Author: cgewecke

README.md

@@ -98,6 +98,7 @@ module.exports = {
 | skipFiles | *Array* | `['Migrations.sol']` | Array of contracts or folders (with paths expressed relative to the `contracts` directory) that should be skipped when doing instrumentation. |
 | measureStatementCoverage | *boolean* | `true` | Computes statement (in addition to line) coverage. [More...][34] |
 | measureFunctionCoverage | *boolean* | `true` | Computes function coverage. [More...][34] |
+| measureModifierCoverage | *boolean* | `true` | Computes each modifier invocation as a code branch. [More...][34] |
 | istanbulFolder | *String* | `./coverage` |  Folder location for Istanbul coverage reports. |
 | istanbulReporter | *Array* | `['html', 'lcov', 'text', 'json']` | [Istanbul coverage reporters][2]  |
 | mocha | *Object* | `{ }` | [Mocha options][3] to merge into existing mocha config. `grep` and `invert` are useful for skipping certain tests under coverage using tags in the test descriptions.|

lib/injector.js

@@ -3,6 +3,7 @@ const web3Utils = require("web3-utils");
 class Injector {
   constructor(){
     this.hashCounter = 0;
+    this.modifiers = {};
   }
 
   _split(contract, injectionPoint){
@@ -18,6 +19,8 @@ class Injector {
         return ` && ${this._getTrueMethodIdentifier(id)}(${hash}))`;
       case 'or-false':
         return ` || ${this._getFalseMethodIdentifier(id)}(${hash}))`;
+      case 'modifier':
+        return ` ${this._getModifierIdentifier(id)} `;
       default:
         return `${this._getDefaultMethodIdentifier(id)}(${hash}); /* ${type} */ \n`;
     }
@@ -43,6 +46,10 @@ class Injector {
     return `c_false${web3Utils.keccak256(id).slice(0,10)}`
   }
 
+  _getModifierIdentifier(id){
+    return `c_mod${web3Utils.keccak256(id).slice(0,10)}`
+  }
+
   _getInjectionComponents(contract, injectionPoint, id, type){
     const { start, end } = this._split(contract, injectionPoint);
     const hash = this._getHash(id)
@@ -104,6 +111,57 @@ class Injector {
     return `function ${method}(bytes32 c__${hash}) public pure returns (bool){ return false; }\n`;
   }
 
+  _getModifierDefinitions(contractId, instrumentation){
+    let injection = '';
+
+    if (this.modifiers[contractId]){
+
+      for (const item of this.modifiers[contractId]){
+        injection += `modifier ${this._getModifierIdentifier(item.modifierId)}{ `;
+
+        let hash = this._getHash(item.modifierId);
+        let injectable = this._getInjectable(item.contractId, hash, 'modifier-pre');
+
+        instrumentation[hash] = {
+          id: item.branchId,
+          type: 'requirePre',
+          contractPath: item.fileName,
+          hits: 0
+        }
+
+        injection += injectable;
+        injection += `_;`
+
+        hash = this._getHash(item.modifierId);
+        injectable = this._getInjectable(item.contractId, hash, 'modifier-post');
+
+        instrumentation[hash] = {
+          id: item.branchId,
+          type: 'requirePost',
+          contractPath: item.fileName,
+          hits: 0
+        }
+
+        injection += injectable;
+        injection += ` }\n`;
+      }
+    }
+
+    return injection;
+  }
+
+  _cacheModifier(injection){
+    if (!this.modifiers[injection.contractId]) {
+      this.modifiers[injection.contractId] = [];
+    }
+
+    this.modifiers[injection.contractId].push(injection);
+  }
+
+  resetModifierMapping(){
+    this.modifiers = {};
+  }
+
   injectLine(contract, fileName, injectionPoint, injection, instrumentation){
     const type = 'line';
     const { start, end } = this._split(contract, injectionPoint);
@@ -267,6 +325,7 @@ class Injector {
                             `${defaultMethodDefinition}` +
                             `${this._getTrueMethodDefinition(id)}` +
                             `${this._getFalseMethodDefinition(id)}` +
+                            `${this._getModifierDefinitions(id, instrumentation)}` +
                             `${end}`;
   }
 
@@ -319,6 +378,29 @@ class Injector {
 
     contract.instrumented = `${start}${injectable}${end}`;
   }
+
+  injectModifier(contract, fileName, injectionPoint, injection, instrumentation){
+    const type = 'modifier';
+    const contractId = `${fileName}:${injection.contractName}`;
+    const modifierId = `${fileName}:${injection.contractName}:` +
+                       `${injection.modifierName}:${injection.fnId}`;
+
+    const {
+      start,
+      end,
+      hash,
+      injectable
+    } = this._getInjectionComponents(contract, injectionPoint, modifierId, type);
+
+    this._cacheModifier({
+      contractId,
+      modifierId,
+      fileName,
+      ...injection
+    });
+
+    contract.instrumented = `${start}${injectable}${end}`;
+  }
 };
 
 module.exports = Injector;

lib/instrumenter.js

@@ -17,6 +17,7 @@ class Instrumenter {
     this.injector = new Injector();
     this.measureStatementCoverage = (config.measureStatementCoverage === false) ? false : true;
     this.measureFunctionCoverage = (config.measureFunctionCoverage === false) ? false: true;
+    this.measureModifierCoverage = (config.measureModifierCoverage === false) ? false: true;
   }
 
   _isRootNode(node){
@@ -56,16 +57,19 @@ class Instrumenter {
   instrument(contractSource, fileName) {
     const contract = {};
 
+    this.injector.resetModifierMapping();
+    parse.configureStatementCoverage(this.measureStatementCoverage)
+    parse.configureFunctionCoverage(this.measureFunctionCoverage)
+    parse.configureModifierCoverage(this.measureModifierCoverage)
+
     contract.source = contractSource;
     contract.instrumented = contractSource;
 
     this._initializeCoverageFields(contract);
-    parse.configureStatementCoverage(this.measureStatementCoverage)
-    parse.configureFunctionCoverage(this.measureFunctionCoverage)
 
     // First, we run over the original contract to get the source mapping.
     let ast = SolidityParser.parse(contract.source, {loc: true, range: true});
-    //console.log(JSON.stringify(ast, null, ' '))
+
     parse[ast.type](contract, ast);
     const retValue = JSON.parse(JSON.stringify(contract)); // Possibly apotropaic.
 

lib/parse.js

@@ -19,6 +19,10 @@ parse.configureFunctionCoverage = function(val){
   register.measureFunctionCoverage = val;
 }
 
+parse.configureModifierCoverage = function(val){
+  register.measureModifierCoverage = val;
+}
+
 // Nodes
 parse.AssignmentExpression = function(contract, expression) {
   register.statement(contract, expression);

lib/registrar.js

@@ -14,6 +14,7 @@ class Registrar {
     // These are set by user option and enable/disable the measurement completely
     this.measureStatementCoverage = true;
     this.measureFunctionCoverage = true;
+    this.measureModifierCoverage = true;
   }
 
   /**
@@ -110,14 +111,31 @@ class Registrar {
     if (!this.measureFunctionCoverage) return;
 
     let start = 0;
+    contract.fnId += 1;
 
-    // It's possible functions will have modifiers that take string args
-    // which contains an open curly brace. Skip ahead...
     if (expression.modifiers && expression.modifiers.length){
       for (let modifier of expression.modifiers ){
+
+        // It's possible functions will have modifiers that take string args
+        // which contains an open curly brace. Skip ahead...
         if (modifier.range[1] > start){
           start = modifier.range[1];
         }
+
+        // Add modifier branch coverage
+        if (!this.measureModifierCoverage) continue;
+
+        this.addNewModifierBranch(contract, modifier);
+        this._createInjectionPoint(
+          contract,
+          modifier.range[0],
+          {
+            type: 'injectModifier',
+            branchId: contract.branchId,
+            modifierName: modifier.name,
+            fnId: contract.fnId
+          }
+        );
       }
     } else {
       start = expression.range[0];
@@ -133,7 +151,6 @@ class Registrar {
       start + endlineDelta
     );
 
-    contract.fnId += 1;
     contract.fnMap[contract.fnId] = {
       name: expression.isConstructor ? 'constructor' : expression.name,
       line: startline,
@@ -186,6 +203,41 @@ class Registrar {
     };
   };
 
+  /**
+   * Registers injections for modifier branch measurements.
+   * @param  {Object} contract   instrumentation target
+   * @param  {Object} expression AST node
+   */
+  addNewModifierBranch(contract, expression) {
+    const startContract = contract.instrumented.slice(0, expression.range[0]);
+    const startline = ( startContract.match(/\n/g) || [] ).length + 1;
+    const startcol = expression.range[0] - startContract.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: startline,
+      type: 'if',
+      locations: [{
+        start: {
+          line: startline, column: startcol,
+        },
+        end: {
+          line: startline, column: startcol,
+        },
+      }, {
+        start: {
+          line: startline, column: startcol,
+        },
+        end: {
+          line: startline, column: startcol,
+        },
+      }],
+    };
+  };
+
   addNewConditionalBranch(contract, expression){
     let start;
     // Instabul HTML highlighting location data...

lib/validator.js

@@ -22,6 +22,7 @@ const configSchema = {
     istanbulFolder:       {type: "string"},
     measureStatementCoverage: {type: "boolean"},
     measureFunctionCoverage: {type: "boolean"},
+    measureModifierCoverage: {type: "boolean"},
 
     // Hooks:
     onServerReady:        {type: "function", format: "isFunction"},

test/integration/projects/modifiers/.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/modifiers/contracts/ModifiersA.sol

@@ -0,0 +1,45 @@
+pragma solidity ^0.6.0;
+
+import "./ModifiersB.sol";
+
+/**
+ * New syntaxes in solc 0.6.x
+ */
+contract ModifiersA is ModifiersB {
+  uint counter;
+  bool flag = true;
+
+  modifier flippable {
+    require(flag);
+    _;
+  }
+
+  modifier overridden() override {
+    require(true);
+    _;
+  }
+
+  function flip() public {
+    flag = !flag;
+  }
+
+  function simpleSet(uint i)
+    public
+    override(ModifiersB)
+  {
+    counter = counter + i;
+  }
+
+  function simpleView(uint i)
+    view
+    overridden
+    external
+    returns (uint, bool)
+  {
+    return (counter + i, true);
+  }
+
+  function simpleSetFlip(uint i) flippable public {
+    counter = counter + i;
+  }
+}

test/integration/projects/modifiers/contracts/ModifiersB.sol

@@ -0,0 +1,19 @@
+pragma solidity ^0.6.0;
+
+
+contract ModifiersB {
+  uint value;
+  uint b;
+
+  constructor() public {
+  }
+
+  modifier overridden() virtual {
+    require(true);
+    _;
+  }
+
+  function simpleSet(uint i) public virtual {
+    value = 5;
+  }
+}

test/integration/projects/modifiers/contracts/ModifiersC.sol

@@ -0,0 +1,43 @@
+pragma solidity ^0.6.0;
+
+import "./ModifiersB.sol";
+
+/**
+ * New syntaxes in solc 0.6.x
+ */
+contract ModifiersC {
+  uint counter;
+  address owner;
+  bool flag = true;
+
+  constructor() public {
+    owner = msg.sender;
+  }
+
+  modifier flippable {
+    require(flag);
+    _;
+  }
+
+  function flip() public {
+    flag = !flag;
+  }
+
+  function simpleSetFlip(uint i) flippable public {
+    counter = counter + i;
+  }
+
+  modifier onlyOwner {
+    require(msg.sender == owner);
+    _;
+  }
+
+  function set(uint i)
+    onlyOwner
+    public
+    payable
+    virtual
+  {
+    counter = counter + i;
+  }
+}