Merge pull request #725 from 0xProject/feature/revert-trace-code-snippets

Include source code snippets in revert stack traces

8 files changed, 279 insertions, 36 deletions

diff --git a/packages/sol-cov/CHANGELOG.json b/packages/sol-cov/CHANGELOG.json
index e61201a42..3b6071801 100644
--- a/packages/sol-cov/CHANGELOG.json
+++ b/packages/sol-cov/CHANGELOG.json
@@ -3,6 +3,22 @@
         "version": "0.2.0",
         "changes": [
             {
+                "note": "Add artifact adapter as a parameter for CoverageSubprovider. Export AbstractArtifactAdapter",
+                "pr": 589
+            },
+            {
+                "note": "Implement SolCompilerArtifactAdapter and TruffleArtifactAdapter",
+                "pr": 589
+            },
+            {
+                "note": "Properly parse multi-level traces",
+                "pr": 589
+            },
+            {
+                "note": "Add support for solidity libraries",
+                "pr": 589
+            },
+            {
                 "note": "Fixed a bug causing RegExp to crash if contract code is longer that 32767 characters",
                 "pr": 675
             },
@@ -51,20 +67,12 @@
                 "pr": 690
             },
             {
-                "note": "Add artifact adapter as a parameter for CoverageSubprovider. Export AbstractArtifactAdapter",
-                "pr": 589
-            },
-            {
-                "note": "Implement SolCompilerArtifactAdapter and TruffleArtifactAdapter",
-                "pr": 589
-            },
-            {
-                "note": "Properly parse multi-level traces",
-                "pr": 589
+                "note": "Create `RevertTraceSubprovider` which prints a stack trace when a REVERT is detected",
+                "pr": 705
             },
             {
-                "note": "Add support for solidity libraries",
-                "pr": 589
+                "note": "Add source code snippets to stack traces printed by `RevertTraceSubprovider`",
+                "pr": 725
             }
         ]
     },
diff --git a/packages/sol-cov/package.json b/packages/sol-cov/package.json
index 39618eacf..f489edc97 100644
--- a/packages/sol-cov/package.json
+++ b/packages/sol-cov/package.json
@@ -73,7 +73,7 @@
         "@types/istanbul": "^0.4.30",
         "@types/loglevel": "^1.5.3",
         "@types/mkdirp": "^0.5.1",
-        "@types/solidity-parser-antlr": "^0.2.0",
+        "@types/solidity-parser-antlr": "^0.2.1",
         "@types/mocha": "^2.2.42",
         "@types/node": "^8.0.53",
         "@types/rimraf": "^2.0.2",
diff --git a/packages/sol-cov/src/ast_visitor.ts b/packages/sol-cov/src/ast_visitor.ts
index 16984b5ec..564f0f7d2 100644
--- a/packages/sol-cov/src/ast_visitor.ts
+++ b/packages/sol-cov/src/ast_visitor.ts
@@ -116,8 +116,9 @@ export class ASTVisitor {
         this._statementMap[this._entryId++] = this._getExpressionRange(ast);
     }
     private _getExpressionRange(ast: Parser.ASTNode): SingleFileSourceRange {
-        const start = this._locationByOffset[ast.range[0]];
-        const end = this._locationByOffset[ast.range[1] + 1];
+        const astRange = ast.range as [number, number];
+        const start = this._locationByOffset[astRange[0]];
+        const end = this._locationByOffset[astRange[1] + 1];
         const range = {
             start,
             end,
diff --git a/packages/sol-cov/src/get_source_range_snippet.ts b/packages/sol-cov/src/get_source_range_snippet.ts
new file mode 100644
index 000000000..30d6ec802
--- /dev/null
+++ b/packages/sol-cov/src/get_source_range_snippet.ts
@@ -0,0 +1,180 @@
+import * as ethUtil from 'ethereumjs-util';
+import * as _ from 'lodash';
+import * as Parser from 'solidity-parser-antlr';
+
+import { SingleFileSourceRange, SourceRange, SourceSnippet } from './types';
+import { utils } from './utils';
+
+interface ASTInfo {
+    type: string;
+    node: Parser.ASTNode;
+    name: string | null;
+    range?: SingleFileSourceRange;
+}
+
+// Parsing source code for each transaction/code is slow and therefore we cache it
+const parsedSourceByHash: { [sourceHash: string]: Parser.ASTNode } = {};
+
+export function getSourceRangeSnippet(sourceRange: SourceRange, sourceCode: string): SourceSnippet | null {
+    const sourceHash = ethUtil.sha3(sourceCode).toString('hex');
+    if (_.isUndefined(parsedSourceByHash[sourceHash])) {
+        parsedSourceByHash[sourceHash] = Parser.parse(sourceCode, { loc: true });
+    }
+    const astNode = parsedSourceByHash[sourceHash];
+    const visitor = new ASTInfoVisitor();
+    Parser.visit(astNode, visitor);
+    const astInfo = visitor.getASTInfoForRange(sourceRange);
+    if (astInfo === null) {
+        return null;
+    }
+    const sourceCodeInRange = utils.getRange(sourceCode, sourceRange.location);
+    return {
+        ...astInfo,
+        range: astInfo.range as SingleFileSourceRange,
+        source: sourceCodeInRange,
+        fileName: sourceRange.fileName,
+    };
+}
+
+// A visitor which collects ASTInfo for most nodes in the AST.
+class ASTInfoVisitor {
+    private _astInfos: ASTInfo[] = [];
+    public getASTInfoForRange(sourceRange: SourceRange): ASTInfo | null {
+        // HACK(albrow): Sometimes the source range doesn't exactly match that
+        // of astInfo. To work around that we try with a +/-1 offset on
+        // end.column. If nothing matches even with the offset, we return null.
+        const offset = {
+            start: {
+                line: 0,
+                column: 0,
+            },
+            end: {
+                line: 0,
+                column: 0,
+            },
+        };
+        let astInfo = this._getASTInfoForRange(sourceRange, offset);
+        if (astInfo !== null) {
+            return astInfo;
+        }
+        offset.end.column += 1;
+        astInfo = this._getASTInfoForRange(sourceRange, offset);
+        if (astInfo !== null) {
+            return astInfo;
+        }
+        offset.end.column -= 2;
+        astInfo = this._getASTInfoForRange(sourceRange, offset);
+        if (astInfo !== null) {
+            return astInfo;
+        }
+        return null;
+    }
+    public ContractDefinition(ast: Parser.ContractDefinition): void {
+        this._visitContractDefinition(ast);
+    }
+    public IfStatement(ast: Parser.IfStatement): void {
+        this._visitStatement(ast);
+    }
+    public FunctionDefinition(ast: Parser.FunctionDefinition): void {
+        this._visitFunctionLikeDefinition(ast);
+    }
+    public ModifierDefinition(ast: Parser.ModifierDefinition): void {
+        this._visitFunctionLikeDefinition(ast);
+    }
+    public ForStatement(ast: Parser.ForStatement): void {
+        this._visitStatement(ast);
+    }
+    public ReturnStatement(ast: Parser.ReturnStatement): void {
+        this._visitStatement(ast);
+    }
+    public BreakStatement(ast: Parser.BreakStatement): void {
+        this._visitStatement(ast);
+    }
+    public ContinueStatement(ast: Parser.ContinueStatement): void {
+        this._visitStatement(ast);
+    }
+    public EmitStatement(ast: any /* TODO: Parser.EmitStatement */): void {
+        this._visitStatement(ast);
+    }
+    public VariableDeclarationStatement(ast: Parser.VariableDeclarationStatement): void {
+        this._visitStatement(ast);
+    }
+    public Statement(ast: Parser.Statement): void {
+        this._visitStatement(ast);
+    }
+    public WhileStatement(ast: Parser.WhileStatement): void {
+        this._visitStatement(ast);
+    }
+    public SimpleStatement(ast: Parser.SimpleStatement): void {
+        this._visitStatement(ast);
+    }
+    public ThrowStatement(ast: Parser.ThrowStatement): void {
+        this._visitStatement(ast);
+    }
+    public DoWhileStatement(ast: Parser.DoWhileStatement): void {
+        this._visitStatement(ast);
+    }
+    public ExpressionStatement(ast: Parser.ExpressionStatement): void {
+        this._visitStatement(ast.expression);
+    }
+    public InlineAssemblyStatement(ast: Parser.InlineAssemblyStatement): void {
+        this._visitStatement(ast);
+    }
+    public ModifierInvocation(ast: Parser.ModifierInvocation): void {
+        const BUILTIN_MODIFIERS = ['public', 'view', 'payable', 'external', 'internal', 'pure', 'constant'];
+        if (!_.includes(BUILTIN_MODIFIERS, ast.name)) {
+            this._visitStatement(ast);
+        }
+    }
+    private _visitStatement(ast: Parser.ASTNode): void {
+        this._astInfos.push({
+            type: ast.type,
+            node: ast,
+            name: null,
+            range: ast.loc,
+        });
+    }
+    private _visitFunctionLikeDefinition(ast: Parser.ModifierDefinition | Parser.FunctionDefinition): void {
+        this._astInfos.push({
+            type: ast.type,
+            node: ast,
+            name: ast.name,
+            range: ast.loc,
+        });
+    }
+    private _visitContractDefinition(ast: Parser.ContractDefinition): void {
+        this._astInfos.push({
+            type: ast.type,
+            node: ast,
+            name: ast.name,
+            range: ast.loc,
+        });
+    }
+    private _getASTInfoForRange(sourceRange: SourceRange, offset: SingleFileSourceRange): ASTInfo | null {
+        const offsetSourceRange = {
+            ...sourceRange,
+            location: {
+                start: {
+                    line: sourceRange.location.start.line + offset.start.line,
+                    column: sourceRange.location.start.column + offset.start.column,
+                },
+                end: {
+                    line: sourceRange.location.end.line + offset.end.line,
+                    column: sourceRange.location.end.column + offset.end.column,
+                },
+            },
+        };
+        for (const astInfo of this._astInfos) {
+            const astInfoRange = astInfo.range as SingleFileSourceRange;
+            if (
+                astInfoRange.start.column === offsetSourceRange.location.start.column &&
+                astInfoRange.start.line === offsetSourceRange.location.start.line &&
+                astInfoRange.end.column === offsetSourceRange.location.end.column &&
+                astInfoRange.end.line === offsetSourceRange.location.end.line
+            ) {
+                return astInfo;
+            }
+        }
+        return null;
+    }
+}
diff --git a/packages/sol-cov/src/revert_trace_subprovider.ts b/packages/sol-cov/src/revert_trace_subprovider.ts
index b1d4da10c..fb2215eaa 100644
--- a/packages/sol-cov/src/revert_trace_subprovider.ts
+++ b/packages/sol-cov/src/revert_trace_subprovider.ts
@@ -4,10 +4,11 @@ import { getLogger, levels, Logger } from 'loglevel';
 
 import { AbstractArtifactAdapter } from './artifact_adapters/abstract_artifact_adapter';
 import { constants } from './constants';
+import { getSourceRangeSnippet } from './get_source_range_snippet';
 import { getRevertTrace } from './revert_trace';
 import { parseSourceMap } from './source_maps';
 import { TraceCollectionSubprovider } from './trace_collection_subprovider';
-import { ContractData, EvmCallStack, SourceRange } from './types';
+import { ContractData, EvmCallStack, SourceRange, SourceSnippet } from './types';
 import { utils } from './utils';
 
 /**
@@ -53,7 +54,7 @@ export class RevertTraceSubprovider extends TraceCollectionSubprovider {
         }
     }
     private async _printStackTraceAsync(evmCallStack: EvmCallStack): Promise<void> {
-        const sourceRanges: SourceRange[] = [];
+        const sourceSnippets: SourceSnippet[] = [];
         if (_.isUndefined(this._contractsData)) {
             this._contractsData = await this._artifactAdapter.collectContractsDataAsync();
         }
@@ -97,18 +98,62 @@ export class RevertTraceSubprovider extends TraceCollectionSubprovider {
                     continue;
                 }
             }
-            sourceRanges.push(sourceRange);
+            const fileIndex = contractData.sources.indexOf(sourceRange.fileName);
+            const sourceSnippet = getSourceRangeSnippet(sourceRange, contractData.sourceCodes[fileIndex]);
+            if (sourceSnippet !== null) {
+                sourceSnippets.push(sourceSnippet);
+            }
         }
-        if (sourceRanges.length > 0) {
+        const filteredSnippets = filterSnippets(sourceSnippets);
+        if (filteredSnippets.length > 0) {
             this._logger.error('\n\nStack trace for REVERT:\n');
-            _.forEach(_.reverse(sourceRanges), sourceRange => {
-                this._logger.error(
-                    `${sourceRange.fileName}:${sourceRange.location.start.line}:${sourceRange.location.start.column}`,
-                );
+            _.forEach(_.reverse(filteredSnippets), snippet => {
+                const traceString = getStackTraceString(snippet);
+                this._logger.error(traceString);
             });
             this._logger.error('\n');
         } else {
-            this._logger.error('Could not determine stack trace');
+            this._logger.error('REVERT detected but could not determine stack trace');
+        }
+    }
+}
+
+// removes duplicates and if statements
+function filterSnippets(sourceSnippets: SourceSnippet[]): SourceSnippet[] {
+    if (sourceSnippets.length === 0) {
+        return [];
+    }
+    const results: SourceSnippet[] = [sourceSnippets[0]];
+    let prev = sourceSnippets[0];
+    for (const sourceSnippet of sourceSnippets) {
+        if (sourceSnippet.type === 'IfStatement') {
+            continue;
+        } else if (sourceSnippet.source === prev.source) {
+            prev = sourceSnippet;
+            continue;
         }
+        results.push(sourceSnippet);
+        prev = sourceSnippet;
+    }
+    return results;
+}
+
+function getStackTraceString(sourceSnippet: SourceSnippet): string {
+    let result = `${sourceSnippet.fileName}:${sourceSnippet.range.start.line}:${sourceSnippet.range.start.column}`;
+    const snippetString = getSourceSnippetString(sourceSnippet);
+    if (snippetString !== '') {
+        result += `:\n        ${snippetString}`;
+    }
+    return result;
+}
+
+function getSourceSnippetString(sourceSnippet: SourceSnippet): string {
+    switch (sourceSnippet.type) {
+        case 'ContractDefinition':
+            return `contract ${sourceSnippet.name}`;
+        case 'FunctionDefinition':
+            return `function ${sourceSnippet.name}`;
+        default:
+            return `${sourceSnippet.source}`;
     }
 }
diff --git a/packages/sol-cov/src/types.ts b/packages/sol-cov/src/types.ts
index cef7141cb..54ade0400 100644
--- a/packages/sol-cov/src/types.ts
+++ b/packages/sol-cov/src/types.ts
@@ -1,4 +1,5 @@
 import { StructLog } from 'ethereum-types';
+import * as Parser from 'solidity-parser-antlr';
 
 export interface LineColumn {
     line: number;
@@ -114,3 +115,12 @@ export interface EvmCallStackEntry {
 }
 
 export type EvmCallStack = EvmCallStackEntry[];
+
+export interface SourceSnippet {
+    source: string;
+    fileName: string;
+    type: string;
+    node: Parser.ASTNode;
+    name: string | null;
+    range: SingleFileSourceRange;
+}
diff --git a/packages/sol-cov/src/utils.ts b/packages/sol-cov/src/utils.ts
index 4f16a1cda..d31696636 100644
--- a/packages/sol-cov/src/utils.ts
+++ b/packages/sol-cov/src/utils.ts
@@ -66,4 +66,10 @@ export const utils = {
         }
         return structLogs;
     },
+    getRange(sourceCode: string, range: SingleFileSourceRange): string {
+        const lines = sourceCode.split('\n').slice(range.start.line - 1, range.end.line);
+        lines[lines.length - 1] = lines[lines.length - 1].slice(0, range.end.column);
+        lines[0] = lines[0].slice(range.start.column);
+        return lines.join('\n');
+    },
 };
diff --git a/packages/sol-cov/test/collect_coverage_entries_test.ts b/packages/sol-cov/test/collect_coverage_entries_test.ts
index a03be19cd..5eedfd55c 100644
--- a/packages/sol-cov/test/collect_coverage_entries_test.ts
+++ b/packages/sol-cov/test/collect_coverage_entries_test.ts
@@ -6,17 +6,10 @@ import 'mocha';
 import * as path from 'path';
 
 import { collectCoverageEntries } from '../src/collect_coverage_entries';
-import { SingleFileSourceRange } from '../src/types';
+import { utils } from '../src/utils';
 
 const expect = chai.expect;
 
-const getRange = (sourceCode: string, range: SingleFileSourceRange) => {
-    const lines = sourceCode.split('\n').slice(range.start.line - 1, range.end.line);
-    lines[lines.length - 1] = lines[lines.length - 1].slice(0, range.end.column);
-    lines[0] = lines[0].slice(range.start.column);
-    return lines.join('\n');
-};
-
 describe('Collect coverage entries', () => {
     describe('#collectCoverageEntries', () => {
         it('correctly collects coverage entries for Simplest contract', () => {
@@ -45,20 +38,20 @@ describe('Collect coverage entries', () => {
             const setFunction = `function set(uint x) {
         storedData = x;
     }`;
-            expect(getRange(simpleStorageContract, coverageEntries.fnMap[fnIds[0]].loc)).to.be.equal(setFunction);
+            expect(utils.getRange(simpleStorageContract, coverageEntries.fnMap[fnIds[0]].loc)).to.be.equal(setFunction);
             expect(coverageEntries.fnMap[fnIds[1]].name).to.be.equal('get');
             // tslint:disable-next-line:custom-no-magic-numbers
             expect(coverageEntries.fnMap[fnIds[1]].line).to.be.equal(8);
             const getFunction = `function get() constant returns (uint retVal) {
         return storedData;
     }`;
-            expect(getRange(simpleStorageContract, coverageEntries.fnMap[fnIds[1]].loc)).to.be.equal(getFunction);
+            expect(utils.getRange(simpleStorageContract, coverageEntries.fnMap[fnIds[1]].loc)).to.be.equal(getFunction);
             expect(coverageEntries.branchMap).to.be.deep.equal({});
             const statementIds = _.keys(coverageEntries.statementMap);
-            expect(getRange(simpleStorageContract, coverageEntries.statementMap[statementIds[1]])).to.be.equal(
+            expect(utils.getRange(simpleStorageContract, coverageEntries.statementMap[statementIds[1]])).to.be.equal(
                 'storedData = x',
             );
-            expect(getRange(simpleStorageContract, coverageEntries.statementMap[statementIds[3]])).to.be.equal(
+            expect(utils.getRange(simpleStorageContract, coverageEntries.statementMap[statementIds[3]])).to.be.equal(
                 'return storedData;',
             );
             expect(coverageEntries.modifiersStatementIds).to.be.deep.equal([]);