diff --git a/lib/coverage.js b/lib/coverage.js index 8d5d6a6..a1a1391 100644 --- a/lib/coverage.js +++ b/lib/coverage.js @@ -69,15 +69,19 @@ 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 'assertPre': this.assertData[contractPath][id].preEvents = hits; break; - case 'assertPost': this.assertData[contractPath][id].postEvents = 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 'assertPre': this.assertData[contractPath][id].preEvents = hits; break; + case 'assertPost': this.assertData[contractPath][id].postEvents = hits; break; } } diff --git a/lib/injector.js b/lib/injector.js index b722ae5..d77366a 100644 --- a/lib/injector.js +++ b/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. - * 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} 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}`; } }; diff --git a/lib/instrumenter.js b/lib/instrumenter.js index 9b8aa4f..5aab842 100644 --- a/lib/instrumenter.js +++ b/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); }); diff --git a/lib/parse.js b/lib/parse.js index 3d561fb..180392d 100644 --- a/lib/parse.js +++ b/lib/parse.js @@ -20,17 +20,40 @@ 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.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 === '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); diff --git a/lib/registrar.js b/lib/registrar.js index 8187584..4c6b6c4 100644 --- a/lib/registrar.js +++ b/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 diff --git a/test/sources/solidity/contracts/assert/Require-fn-reason.sol b/test/sources/solidity/contracts/assert/Require-fn-reason.sol new file mode 100644 index 0000000..0a010b3 --- /dev/null +++ b/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"); + } +} diff --git a/test/sources/solidity/contracts/assert/Require-fn.sol b/test/sources/solidity/contracts/assert/Require-fn.sol new file mode 100644 index 0000000..c07dfdc --- /dev/null +++ b/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)); + } +} diff --git a/test/sources/solidity/contracts/or/return-or.sol b/test/sources/solidity/contracts/or/return-or.sol index d0ba26d..aed8962 100644 --- a/test/sources/solidity/contracts/or/return-or.sol +++ b/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) || - (x == 2 && true); + return (x == 1 && true) || (x == 2 && true); } } diff --git a/test/units/assert.js b/test/units/assert.js index ecb2c88..524b79c 100644 --- a/test/units/assert.js +++ b/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 + }); + }); }); diff --git a/test/units/or.js b/test/units/or.js index a52e6a1..3414731 100644 --- a/test/units/or.js +++ b/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,