From 39895c1987d1dddf2aa16ab28ba73fc988090bc9 Mon Sep 17 00:00:00 2001 From: duaraghav8 Date: Sat, 14 Sep 2019 12:03:06 +0530 Subject: [PATCH 01/18] 1.2.5 --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 640ba9f..f570ca9 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "ethlint", - "version": "1.2.4", + "version": "1.2.5", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 0f8b872..27212e4 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "ethlint", - "version": "1.2.4", + "version": "1.2.5", "description": "Linter to identify and fix Style & Security issues in Solidity", "main": "./lib/solium.js", "bin": { From 4a6306573d597ad561e8cabfe50343a797f410ff Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 06:01:00 +0200 Subject: [PATCH 02/18] preliminary assembly indent support --- lib/rules/indentation.js | 41 +++++++++++++ .../lib/rules/indentation/accept/assembly.sol | 13 +++++ .../indentation/accept/one-line-assembly.sol | 7 +++ test/lib/rules/indentation/indentation.js | 57 +++++++++++++++++++ .../lib/rules/indentation/reject/assembly.sol | 13 +++++ 5 files changed, 131 insertions(+) create mode 100644 test/lib/rules/indentation/accept/assembly.sol create mode 100644 test/lib/rules/indentation/accept/one-line-assembly.sol create mode 100644 test/lib/rules/indentation/reject/assembly.sol diff --git a/lib/rules/indentation.js b/lib/rules/indentation.js index a2feaef..696ef94 100644 --- a/lib/rules/indentation.js +++ b/lib/rules/indentation.js @@ -291,6 +291,46 @@ module.exports = { + function inspectAssemblyBlock(emitted) { + let node = emitted.node, + body = node.body || [], + endingLineNum = sourceCode.getEndingLine(node); + let assemblyDeclarationLineText, currentIndent, currentIndentLevel; + + // No need to lint further if entire assembly declaration is on single line + if (emitted.exit || sourceCode.getLine(node) === endingLineNum) { + return; + } + + assemblyDeclarationLineText = sourceCode.getTextOnLine(sourceCode.getLine(node)); + + currentIndent = assemblyDeclarationLineText.slice( + 0, + assemblyDeclarationLineText.indexOf(assemblyDeclarationLineText.trim()) + ); + + currentIndentLevel = (currentIndent.match(BASE_INDENTATION_STYLE_REGEXP_GLOBAL) || []).length; + + //ensure that there is only whitespace of correct level on the line containing assembly declaration + if (getIndentString(BASE_INDENTATION_STYLE, currentIndentLevel) !== currentIndent) { + return; //exit now, we can' proceed further unless this is fixed + } + + const assemblyIndent = getIndentString(BASE_INDENTATION_STYLE, currentIndentLevel + 1); + const assemblyIndentDesc = getIndentDescription(BASE_INDENTATION_STYLE, currentIndentLevel + 1); + + let indentRegExp = new RegExp("^" + assemblyIndent + "[^\\s(\/\*)]"); + for (let line = sourceCode.getLine(node) + 1; line < endingLineNum; ++line) { + let codeLineText = sourceCode.getTextOnLine(line); + !indentRegExp.test(codeLineText) && context.report({ + node: node, + message: `Only use indent of ${assemblyIndentDesc}.` + }); + } + } + + + function inspectStructDeclaration(emitted) { let node = emitted.node, body = node.body || [], @@ -482,6 +522,7 @@ module.exports = { ArrayExpression: inspectArrayExpression, StructDeclaration: inspectStructDeclaration, BlockStatement: inspectBlockStatement, + InlineAssemblyStatement: inspectAssemblyBlock, Program: inspectProgram }; diff --git a/test/lib/rules/indentation/accept/assembly.sol b/test/lib/rules/indentation/accept/assembly.sol new file mode 100644 index 0000000..2cc335a --- /dev/null +++ b/test/lib/rules/indentation/accept/assembly.sol @@ -0,0 +1,13 @@ +pragma solidity ^0.4.4; + +library GetCode { + function at(address _addr) public view returns (bytes memory o_code) { + assembly { + let size := extcodesize(_addr) + o_code := mload(0x40) + mstore(0x40, add(o_code, and(add(add(size, 0x20), 0x1f), not(0x1f)))) + mstore(o_code, size) + extcodecopy(_addr, add(o_code, 0x20), 0, size) + } + } +} \ No newline at end of file diff --git a/test/lib/rules/indentation/accept/one-line-assembly.sol b/test/lib/rules/indentation/accept/one-line-assembly.sol new file mode 100644 index 0000000..220df16 --- /dev/null +++ b/test/lib/rules/indentation/accept/one-line-assembly.sol @@ -0,0 +1,7 @@ +pragma solidity ^0.4.4; + +library GetCode { + function at(address _addr) public view returns (bytes memory o_code) { + assembly { o_code := mload(0x40) } + } +} \ No newline at end of file diff --git a/test/lib/rules/indentation/indentation.js b/test/lib/rules/indentation/indentation.js index a0dfa7c..ee30fa8 100644 --- a/test/lib/rules/indentation/indentation.js +++ b/test/lib/rules/indentation/indentation.js @@ -165,6 +165,44 @@ describe("[RULE] indentation: Acceptances", function() { done(); }); + it("should accept a file with an inline assembly", function(done) { + let userConfig = { + "rules": { + "indentation": "error" + } + }; + + let file = "assembly.sol"; + let code = fs.readFileSync(path.join(acceptDir, file), "utf8"); + + let errors = Solium.lint(code, userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + + Solium.reset(); + done(); + }); + + it("should accept a file with an assembly in one line", function(done) { + let userConfig = { + "rules": { + "indentation": "error" + } + }; + + let file = "one-line-struct.sol"; + let code = fs.readFileSync(path.join(acceptDir, file), "utf8"); + + let errors = Solium.lint(code, userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + + Solium.reset(); + done(); + }); + it("should accept a file with an array in one line", function(done) { let userConfig = { "rules": { @@ -357,6 +395,25 @@ describe("[RULE] indentation: Rejections", function() { done(); }); + it("should reject an invalid file with an assembly", function(done) { + let userConfig = { + "rules": { + "indentation": "error" + } + }; + + let file = "assembly.sol"; + let code = fs.readFileSync(path.join(rejectDir, file), "utf8"); + + let errors = Solium.lint(code, userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(2); + + Solium.reset(); + done(); + }); + it("should reject top level indentation", function(done) { let userConfig = { "rules": { diff --git a/test/lib/rules/indentation/reject/assembly.sol b/test/lib/rules/indentation/reject/assembly.sol new file mode 100644 index 0000000..805189a --- /dev/null +++ b/test/lib/rules/indentation/reject/assembly.sol @@ -0,0 +1,13 @@ +pragma solidity ^0.4.4; + +library GetCode { + function at(address _addr) public view returns (bytes memory o_code) { + assembly { + let size := extcodesize(_addr) + o_code := mload(0x40) + mstore(0x40, add(o_code, and(add(add(size, 0x20), 0x1f), not(0x1f)))) + mstore(o_code, size) + extcodecopy(_addr, add(o_code, 0x20), 0, size) + } + } +} \ No newline at end of file From ca4c4523e8566b64641f72eb9b30db9c8a5976b2 Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 06:02:02 +0200 Subject: [PATCH 03/18] bug fix --- test/lib/rules/indentation/indentation.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/lib/rules/indentation/indentation.js b/test/lib/rules/indentation/indentation.js index ee30fa8..1058026 100644 --- a/test/lib/rules/indentation/indentation.js +++ b/test/lib/rules/indentation/indentation.js @@ -191,7 +191,7 @@ describe("[RULE] indentation: Acceptances", function() { } }; - let file = "one-line-struct.sol"; + let file = "one-line-assembly.sol"; let code = fs.readFileSync(path.join(acceptDir, file), "utf8"); let errors = Solium.lint(code, userConfig); From efc4a702c67356654750982e1f342d4853579346 Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 06:27:56 +0200 Subject: [PATCH 04/18] support for intentation of control structures in inline assemblies --- lib/rules/indentation.js | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/rules/indentation.js b/lib/rules/indentation.js index 696ef94..ef76b45 100644 --- a/lib/rules/indentation.js +++ b/lib/rules/indentation.js @@ -316,16 +316,24 @@ module.exports = { return; //exit now, we can' proceed further unless this is fixed } - const assemblyIndent = getIndentString(BASE_INDENTATION_STYLE, currentIndentLevel + 1); - const assemblyIndentDesc = getIndentDescription(BASE_INDENTATION_STYLE, currentIndentLevel + 1); - - let indentRegExp = new RegExp("^" + assemblyIndent + "[^\\s(\/\*)]"); + // Instead of parsing all kinds of assembly constructs, I will just increase + // indent on { and decrease on } (skipping strings) + let subLevel = 1; for (let line = sourceCode.getLine(node) + 1; line < endingLineNum; ++line) { + // maybe, a little slow + const assemblyIndent = getIndentString(BASE_INDENTATION_STYLE, currentIndentLevel + subLevel); + const assemblyIndentDesc = getIndentDescription(BASE_INDENTATION_STYLE, currentIndentLevel + subLevel); + const indentRegExp = new RegExp("^" + assemblyIndent + "[^\\s(\/\*)]"); + let codeLineText = sourceCode.getTextOnLine(line); !indentRegExp.test(codeLineText) && context.report({ node: node, message: `Only use indent of ${assemblyIndentDesc}.` }); + + let codeLineWithoutStrings = codeLineText.replace(/"([^"]|\\")*"|'([^']|\\')*'/g, ""); + subLevel += (codeLineWithoutStrings.match(/{/g) || []).length - + (codeLineWithoutStrings.match(/}/g) || []).length; } } From e1174e2eae4a98e1b82d438913d1f9fc70f06e4d Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 07:06:02 +0200 Subject: [PATCH 05/18] bug fixes --- lib/rules/indentation.js | 11 +++++++---- test/lib/rules/indentation/accept/assembly.sol | 16 +++++++++++----- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/lib/rules/indentation.js b/lib/rules/indentation.js index ef76b45..61c67c1 100644 --- a/lib/rules/indentation.js +++ b/lib/rules/indentation.js @@ -320,20 +320,23 @@ module.exports = { // indent on { and decrease on } (skipping strings) let subLevel = 1; for (let line = sourceCode.getLine(node) + 1; line < endingLineNum; ++line) { + let codeLineText = sourceCode.getTextOnLine(line); + + if (codeLineText == "") continue; + let codeLineWithoutStrings = codeLineText.replace(/"([^"]|\\")*"|'([^']|\\')*'/g, ""); + subLevel -= (codeLineWithoutStrings.match(/}/g) || []).length; + // maybe, a little slow const assemblyIndent = getIndentString(BASE_INDENTATION_STYLE, currentIndentLevel + subLevel); const assemblyIndentDesc = getIndentDescription(BASE_INDENTATION_STYLE, currentIndentLevel + subLevel); const indentRegExp = new RegExp("^" + assemblyIndent + "[^\\s(\/\*)]"); - let codeLineText = sourceCode.getTextOnLine(line); !indentRegExp.test(codeLineText) && context.report({ node: node, message: `Only use indent of ${assemblyIndentDesc}.` }); - let codeLineWithoutStrings = codeLineText.replace(/"([^"]|\\")*"|'([^']|\\')*'/g, ""); - subLevel += (codeLineWithoutStrings.match(/{/g) || []).length - - (codeLineWithoutStrings.match(/}/g) || []).length; + subLevel += (codeLineWithoutStrings.match(/{/g) || []).length; } } diff --git a/test/lib/rules/indentation/accept/assembly.sol b/test/lib/rules/indentation/accept/assembly.sol index 2cc335a..3eac3af 100644 --- a/test/lib/rules/indentation/accept/assembly.sol +++ b/test/lib/rules/indentation/accept/assembly.sol @@ -3,11 +3,17 @@ pragma solidity ^0.4.4; library GetCode { function at(address _addr) public view returns (bytes memory o_code) { assembly { - let size := extcodesize(_addr) - o_code := mload(0x40) - mstore(0x40, add(o_code, and(add(add(size, 0x20), 0x1f), not(0x1f)))) - mstore(o_code, size) - extcodecopy(_addr, add(o_code, 0x20), 0, size) + let len := mload(_data) + + let data := add(_data, 0x20) + + for + { let end := add(data, mul(len, 0x20)) } + lt(data, end) + { data := add(data, 0x20) } + { + sum := add(sum, mload(data)) + } } } } \ No newline at end of file From b832d9b84ce67b0d834b40ec39da0614a5c3da06 Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 08:10:58 +0200 Subject: [PATCH 06/18] bug fixes --- lib/rules/indentation.js | 32 +++++++++++++++---- .../lib/rules/indentation/accept/assembly.sol | 4 --- test/lib/rules/indentation/indentation.js | 29 +++++++++-------- 3 files changed, 41 insertions(+), 24 deletions(-) diff --git a/lib/rules/indentation.js b/lib/rules/indentation.js index 61c67c1..84cb5fd 100644 --- a/lib/rules/indentation.js +++ b/lib/rules/indentation.js @@ -319,16 +319,22 @@ module.exports = { // Instead of parsing all kinds of assembly constructs, I will just increase // indent on { and decrease on } (skipping strings) let subLevel = 1; + let remainingForLines = 0; // how many lines remain to indent additionally after "for" standing on a separate line + let forArgumentSubLevel = 0; // count open/close braces inside a "for" argument for (let line = sourceCode.getLine(node) + 1; line < endingLineNum; ++line) { - let codeLineText = sourceCode.getTextOnLine(line); + const codeLineText = sourceCode.getTextOnLine(line); if (codeLineText == "") continue; - let codeLineWithoutStrings = codeLineText.replace(/"([^"]|\\")*"|'([^']|\\')*'/g, ""); - subLevel -= (codeLineWithoutStrings.match(/}/g) || []).length; + const codeLineWithoutStrings = codeLineText.replace(/"([^"]|\\")*"|'([^']|\\')*'/g, ""); + const numberOfOpenBraces = (codeLineWithoutStrings.match(/{/g) || []).length; + const numberOfClosingBraces = (codeLineWithoutStrings.match(/}/g) || []).length; + subLevel += numberOfOpenBraces - numberOfClosingBraces; + let effectiveSubLevel = subLevel; + if (/^\s*}\s*$/.test(codeLineText)) --effectiveSubLevel; // Don't indent closing brace. // maybe, a little slow - const assemblyIndent = getIndentString(BASE_INDENTATION_STYLE, currentIndentLevel + subLevel); - const assemblyIndentDesc = getIndentDescription(BASE_INDENTATION_STYLE, currentIndentLevel + subLevel); + const assemblyIndent = getIndentString(BASE_INDENTATION_STYLE, currentIndentLevel + effectiveSubLevel); + const assemblyIndentDesc = getIndentDescription(BASE_INDENTATION_STYLE, currentIndentLevel + effectiveSubLevel); const indentRegExp = new RegExp("^" + assemblyIndent + "[^\\s(\/\*)]"); !indentRegExp.test(codeLineText) && context.report({ @@ -336,7 +342,21 @@ module.exports = { message: `Only use indent of ${assemblyIndentDesc}.` }); - subLevel += (codeLineWithoutStrings.match(/{/g) || []).length; + // Special processing for "for" standing on a separate line. + // After this we expect 3 indented lines (or more if there are blocks). + if(remainingForLines) { + forArgumentSubLevel += numberOfOpenBraces - numberOfClosingBraces; + if (!forArgumentSubLevel) { // A "for" argument ends here. + --remainingForLines; + if (!remainingForLines) --subLevel; + } + } + + if (/^\s*for\s*$/.test(codeLineText)) { + // TODO: Support for "for" loop inside a for-initialization or for-increment (needs a stack). + remainingForLines = 3; + ++subLevel; + } } } diff --git a/test/lib/rules/indentation/accept/assembly.sol b/test/lib/rules/indentation/accept/assembly.sol index 3eac3af..1a67469 100644 --- a/test/lib/rules/indentation/accept/assembly.sol +++ b/test/lib/rules/indentation/accept/assembly.sol @@ -3,10 +3,6 @@ pragma solidity ^0.4.4; library GetCode { function at(address _addr) public view returns (bytes memory o_code) { assembly { - let len := mload(_data) - - let data := add(_data, 0x20) - for { let end := add(data, mul(len, 0x20)) } lt(data, end) diff --git a/test/lib/rules/indentation/indentation.js b/test/lib/rules/indentation/indentation.js index 1058026..f3c120c 100644 --- a/test/lib/rules/indentation/indentation.js +++ b/test/lib/rules/indentation/indentation.js @@ -395,24 +395,25 @@ describe("[RULE] indentation: Rejections", function() { done(); }); - it("should reject an invalid file with an assembly", function(done) { - let userConfig = { - "rules": { - "indentation": "error" - } - }; + // FIXME: Uncomment. + // it("should reject an invalid file with an inline assembly", function(done) { + // let userConfig = { + // "rules": { + // "indentation": "error" + // } + // }; - let file = "assembly.sol"; - let code = fs.readFileSync(path.join(rejectDir, file), "utf8"); + // let file = "assembly.sol"; + // let code = fs.readFileSync(path.join(rejectDir, file), "utf8"); - let errors = Solium.lint(code, userConfig); + // let errors = Solium.lint(code, userConfig); - errors.constructor.name.should.equal("Array"); - errors.length.should.equal(2); + // errors.constructor.name.should.equal("Array"); + // errors.length.should.equal(2); - Solium.reset(); - done(); - }); + // Solium.reset(); + // done(); + // }); it("should reject top level indentation", function(done) { let userConfig = { From 57a2c8e644b8d7f517313204825e00257c6477a2 Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 08:22:22 +0200 Subject: [PATCH 07/18] bug fix --- lib/rules/indentation.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rules/indentation.js b/lib/rules/indentation.js index 84cb5fd..e40b4df 100644 --- a/lib/rules/indentation.js +++ b/lib/rules/indentation.js @@ -330,7 +330,7 @@ module.exports = { const numberOfClosingBraces = (codeLineWithoutStrings.match(/}/g) || []).length; subLevel += numberOfOpenBraces - numberOfClosingBraces; let effectiveSubLevel = subLevel; - if (/^\s*}\s*$/.test(codeLineText)) --effectiveSubLevel; // Don't indent closing brace. + if (/^\s*{\s*$/.test(codeLineText)) --effectiveSubLevel; // Don't indent closing brace. // maybe, a little slow const assemblyIndent = getIndentString(BASE_INDENTATION_STYLE, currentIndentLevel + effectiveSubLevel); From 58fb8bf26aec25ce5261080954f7c47b5f409115 Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 08:23:32 +0200 Subject: [PATCH 08/18] more tests --- test/lib/rules/indentation/accept/assembly.sol | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/lib/rules/indentation/accept/assembly.sol b/test/lib/rules/indentation/accept/assembly.sol index 1a67469..81f7fde 100644 --- a/test/lib/rules/indentation/accept/assembly.sol +++ b/test/lib/rules/indentation/accept/assembly.sol @@ -3,6 +3,10 @@ pragma solidity ^0.4.4; library GetCode { function at(address _addr) public view returns (bytes memory o_code) { assembly { + let len := mload(_data) + + let data := add(_data, 0x20) + for { let end := add(data, mul(len, 0x20)) } lt(data, end) @@ -10,6 +14,11 @@ library GetCode { { sum := add(sum, mload(data)) } + + for { let end := add(data, mul(len, 0x20)) } lt(data, end) { data := add(data, 0x20) } + { + sum := add(sum, mload(data)) + } } } } \ No newline at end of file From fbe29131ed09c5fa50540fb81f50aa67696328ca Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 08:24:15 +0200 Subject: [PATCH 09/18] test uncommented --- test/lib/rules/indentation/indentation.js | 29 +++++++++++------------ 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/test/lib/rules/indentation/indentation.js b/test/lib/rules/indentation/indentation.js index f3c120c..77ef9eb 100644 --- a/test/lib/rules/indentation/indentation.js +++ b/test/lib/rules/indentation/indentation.js @@ -395,25 +395,24 @@ describe("[RULE] indentation: Rejections", function() { done(); }); - // FIXME: Uncomment. - // it("should reject an invalid file with an inline assembly", function(done) { - // let userConfig = { - // "rules": { - // "indentation": "error" - // } - // }; + it("should reject an invalid file with an inline assembly", function(done) { + let userConfig = { + "rules": { + "indentation": "error" + } + }; - // let file = "assembly.sol"; - // let code = fs.readFileSync(path.join(rejectDir, file), "utf8"); + let file = "assembly.sol"; + let code = fs.readFileSync(path.join(rejectDir, file), "utf8"); - // let errors = Solium.lint(code, userConfig); + let errors = Solium.lint(code, userConfig); - // errors.constructor.name.should.equal("Array"); - // errors.length.should.equal(2); + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(2); - // Solium.reset(); - // done(); - // }); + Solium.reset(); + done(); + }); it("should reject top level indentation", function(done) { let userConfig = { From 4f9aa4f961081402305584084fe030103d315398 Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 08:25:41 +0200 Subject: [PATCH 10/18] more indentation rejecting testing --- test/lib/rules/indentation/indentation.js | 2 +- test/lib/rules/indentation/reject/assembly.sol | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/test/lib/rules/indentation/indentation.js b/test/lib/rules/indentation/indentation.js index 77ef9eb..9adbaf6 100644 --- a/test/lib/rules/indentation/indentation.js +++ b/test/lib/rules/indentation/indentation.js @@ -408,7 +408,7 @@ describe("[RULE] indentation: Rejections", function() { let errors = Solium.lint(code, userConfig); errors.constructor.name.should.equal("Array"); - errors.length.should.equal(2); + errors.length.should.equal(4); Solium.reset(); done(); diff --git a/test/lib/rules/indentation/reject/assembly.sol b/test/lib/rules/indentation/reject/assembly.sol index 805189a..9597630 100644 --- a/test/lib/rules/indentation/reject/assembly.sol +++ b/test/lib/rules/indentation/reject/assembly.sol @@ -8,6 +8,13 @@ library GetCode { mstore(0x40, add(o_code, and(add(add(size, 0x20), 0x1f), not(0x1f)))) mstore(o_code, size) extcodecopy(_addr, add(o_code, 0x20), 0, size) + for + { let end := add(data, mul(len, 0x20)) } + lt(data, end) + { data := add(data, 0x20) } + { + sum := add(sum, mload(data)) + } } } } \ No newline at end of file From 464bc1170693dbb401c0db30c7c6646f10014fcd Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 08:38:48 +0200 Subject: [PATCH 11/18] error in comment --- lib/rules/indentation.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rules/indentation.js b/lib/rules/indentation.js index e40b4df..f82d2c5 100644 --- a/lib/rules/indentation.js +++ b/lib/rules/indentation.js @@ -330,7 +330,7 @@ module.exports = { const numberOfClosingBraces = (codeLineWithoutStrings.match(/}/g) || []).length; subLevel += numberOfOpenBraces - numberOfClosingBraces; let effectiveSubLevel = subLevel; - if (/^\s*{\s*$/.test(codeLineText)) --effectiveSubLevel; // Don't indent closing brace. + if (/^\s*{\s*$/.test(codeLineText)) --effectiveSubLevel; // Don't indent opening brace. // maybe, a little slow const assemblyIndent = getIndentString(BASE_INDENTATION_STYLE, currentIndentLevel + effectiveSubLevel); From 482332ae9aa9fd6489a1e25c1f2ae309cc522732 Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 08:39:34 +0200 Subject: [PATCH 12/18] more syntax testing --- test/lib/rules/indentation/accept/assembly.sol | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/lib/rules/indentation/accept/assembly.sol b/test/lib/rules/indentation/accept/assembly.sol index 81f7fde..a428692 100644 --- a/test/lib/rules/indentation/accept/assembly.sol +++ b/test/lib/rules/indentation/accept/assembly.sol @@ -15,6 +15,18 @@ library GetCode { sum := add(sum, mload(data)) } + for + { + let end := add(data, mul(len, 0x20)) + } + lt(data, end) + { + data := add(data, 0x20) + } + { + sum := add(sum, mload(data)) + } + for { let end := add(data, mul(len, 0x20)) } lt(data, end) { data := add(data, 0x20) } { sum := add(sum, mload(data)) From 103f1c3d0ae5378e4abb7c6c9e209d33d24f20c8 Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 11:54:34 +0200 Subject: [PATCH 13/18] tests for contructors --- lib/rules/lbrace.js | 1 + test/lib/rules/lbrace/lbrace.js | 96 ++++++++++++++++++++++++++++++++- test/lib/utils/wrappers.js | 20 +++++++ 3 files changed, 116 insertions(+), 1 deletion(-) diff --git a/lib/rules/lbrace.js b/lib/rules/lbrace.js index 8da84c3..1692b12 100644 --- a/lib/rules/lbrace.js +++ b/lib/rules/lbrace.js @@ -409,6 +409,7 @@ module.exports = { return { FunctionDeclaration: inspectFunctionDeclaration, + ConstructorDeclaration: inspectFunctionDeclaration, LibraryStatement: inspectLibraryStatement, ContractStatement: inspectContractStatement, StructDeclaration: inspectStructDeclaration, diff --git a/test/lib/rules/lbrace/lbrace.js b/test/lib/rules/lbrace/lbrace.js index df942cb..7bce253 100644 --- a/test/lib/rules/lbrace/lbrace.js +++ b/test/lib/rules/lbrace/lbrace.js @@ -93,6 +93,12 @@ describe("[RULE] lbrace: Acceptances", function() { errors.constructor.name.should.equal("Array"); errors.length.should.equal(0); + code = `constructor (bytes32 name) {${EOL}\t/*body*/${EOL}}`; + errors = Solium.lint(toContract(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + code = `struct Student {${EOL}\tstring name;${EOL}\tuint age;${EOL}\taddress account;${EOL}}`; errors = Solium.lint(toContract(code), userConfig); @@ -146,6 +152,31 @@ describe("[RULE] lbrace: Acceptances", function() { errors.constructor.name.should.equal("Array"); errors.length.should.equal(0); + + code = "if (true) { hello (); }", + errors = Solium.lint(toConstructor(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + + code = `if (true) {${EOL}\thello ();${EOL}} else if (true) {${EOL}\tworld ();${EOL}} else {${EOL}\tfoobar ();${EOL}}`; + errors = Solium.lint(toConstructor(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + + code = "constructor () {hello ();}"; + errors = Solium.lint(toContract(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + + code = `constructor () {${EOL}\thello ();${EOL}}`; + errors = Solium.lint(toContract(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + Solium.reset(); done(); }); @@ -172,6 +203,26 @@ describe("[RULE] lbrace: Acceptances", function() { errors.constructor.name.should.equal("Array"); errors.length.should.equal(0); + + //since this rule doesn't lint for indentation, only ${EOL} without \t should also be valid + code = `if (true)${EOL}newNumber = (10 * 78 + 982 % 6**2);${EOL}else${EOL}newNumber = 0;`; + errors = Solium.lint(toConstructor(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + + code = `if (true)${EOL}\tlaunchEvent (\"foo bar!\");`; + errors = Solium.lint(toConstructor(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + + code = `if (true)${EOL}createStructObject ({ name: \"Chuck Norris\", age: \"inf\" });`; + errors = Solium.lint(toConstructor(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + Solium.reset(); done(); @@ -184,6 +235,12 @@ describe("[RULE] lbrace: Acceptances", function() { errors.constructor.name.should.equal("Array"); errors.length.should.equal(0); + code = `constructor (${EOL}\tuint x,${EOL}\tstring y,${EOL}\taddress z${EOL}) {${EOL}\tfoobar ();${EOL}}`, + errors = Solium.lint(toContract(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + Solium.reset(); done(); }); @@ -270,7 +327,13 @@ describe("[RULE] lbrace: Acceptances", function() { it("should allow opening brace to be on its own line in case a function has base constructor arguments", function(done) { let code = `function baseArgs ()${EOL}\tA (10)${EOL}\tB (\"hello\")${EOL}\tC (0x0)${EOL}\tD (frodo)${EOL}{${EOL}\tfoobar ();${EOL}}`, - errors = Solium.lint(toContract(code), userConfig); + errors = Solium.lint(toContract(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + + code = `constructor ()${EOL}\tA (10)${EOL}\tB (\"hello\")${EOL}\tC (0x0)${EOL}\tD (frodo)${EOL}{${EOL}\tfoobar ();${EOL}}`, + errors = Solium.lint(toContract(code), userConfig); errors.constructor.name.should.equal("Array"); errors.length.should.equal(0); @@ -310,6 +373,36 @@ describe("[RULE] lbrace: Acceptances", function() { errors.constructor.name.should.equal("Array"); errors.length.should.equal(0); + code = "if (true) {h();} else if (true) {h();} else {h();}", + errors = Solium.lint(toConstructor(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + + code = "if (true) {h();} else {h();}"; + errors = Solium.lint(toConstructor(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + + code = "if (true) {h();} else if (true) {h();}"; + errors = Solium.lint(toConstructor(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + + code = `if (true) {h();} else if (true)${EOL}hello ();`; + errors = Solium.lint(toConstructor(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + + code = `if (true) {h();} else if (true)${EOL}hello ();`; + errors = Solium.lint(toConstructor(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + Solium.reset(); done(); }); @@ -317,6 +410,7 @@ describe("[RULE] lbrace: Acceptances", function() { }); +// For rejections do not test for constructors, because it is the same code, no need to test twice describe("[RULE] lbrace: Rejections", function() { it("should reject any opening brace which is not preceded by EXACTLY single space (exception: functions with modifiers)", function(done) { diff --git a/test/lib/utils/wrappers.js b/test/lib/utils/wrappers.js index 590ca64..c6d4426 100644 --- a/test/lib/utils/wrappers.js +++ b/test/lib/utils/wrappers.js @@ -68,6 +68,26 @@ describe("Test wrappers", function() { done(); }); + it("toConstructor: should correctly wrap a solidity statement in contract/constructor code", function(done) { + let toConstructor = wrappers.toConstructor; + let statement = "uint x = 1;"; + let expected = + `pragma solidity ^0.4.3;${EOL.repeat(3)}` + + `contract Wrap {${EOL}` + + `\tconstructor() {${EOL}` + + "\t\t" + statement + EOL + + `\t}${EOL}` + + "}"; + + let errors = Solium.lint(expected, userConfig); + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + toConstructor(statement).should.equal(expected); + + Solium.reset(); + done(); + }); + it("addPragma: should correctly pre-pend a pragma statement to a solidity contract or library", function(done) { let addPragma = wrappers.addPragma; let contract = "contract Abc { }"; From 1f8ac2a02433ec425ae81ea1e7cf66e21bb84232 Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 12:17:09 +0200 Subject: [PATCH 14/18] lbrace.js: constructors --- lib/rules/lbrace.js | 16 ++++++++++------ test/lib/rules/lbrace/lbrace.js | 11 ++++++++++- test/utils/wrappers.js | 11 +++++++++++ 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/lib/rules/lbrace.js b/lib/rules/lbrace.js index 1692b12..245731a 100644 --- a/lib/rules/lbrace.js +++ b/lib/rules/lbrace.js @@ -324,7 +324,7 @@ module.exports = { } - function inspectFunctionDeclaration(emitted) { + function inspectFunctionOrConstructorDeclaration(emitted) { let node = emitted.node; if (emitted.exit || node.is_abstract) { @@ -341,13 +341,17 @@ module.exports = { declarationStartingLine = sourceCode.getLine(node), bodyStartingLine = sourceCode.getLine(node.body); + // Don't write like this: + // if (node.returnParams !== null) + // because for contructor they may be undefined. + //If the node just before function's body is on the start line itself, then the opening brace must too be on the start line - if (node.returnParams !== null) { + if (node.returnParams) { declarationEndingLine = sourceCode.getEndingLine(node.returnParams); - } else if (node.modifiers !== null) { + } else if (node.modifiers) { //if modifiers is non-null (,i.e., array), compare bodyStartingLine with ending line of the last modifier declaration (last array element) declarationEndingLine = sourceCode.getEndingLine(node.modifiers.slice(-1) [0]); - } else if (node.params !== null) { + } else if (node.params) { //if params is non-null (,i.e., array), compare bodyStartingLine with ending line of the last param declaration (last array element) declarationEndingLine = sourceCode.getEndingLine(node.params.slice(-1) [0]); } else { @@ -408,8 +412,8 @@ module.exports = { return { - FunctionDeclaration: inspectFunctionDeclaration, - ConstructorDeclaration: inspectFunctionDeclaration, + FunctionDeclaration: inspectFunctionOrConstructorDeclaration, + ConstructorDeclaration: inspectFunctionOrConstructorDeclaration, LibraryStatement: inspectLibraryStatement, ContractStatement: inspectContractStatement, StructDeclaration: inspectStructDeclaration, diff --git a/test/lib/rules/lbrace/lbrace.js b/test/lib/rules/lbrace/lbrace.js index 7bce253..b740c25 100644 --- a/test/lib/rules/lbrace/lbrace.js +++ b/test/lib/rules/lbrace/lbrace.js @@ -10,6 +10,7 @@ const Solium = require("../../../../lib/solium"), const toContract = wrappers.toContract, toFunction = wrappers.toFunction, + toConstructor = wrappers.toConstructor, addPragma = wrappers.addPragma, { EOL } = require("os"); @@ -410,7 +411,8 @@ describe("[RULE] lbrace: Acceptances", function() { }); -// For rejections do not test for constructors, because it is the same code, no need to test twice +// For rejections do not test everything for constructors, because it is the same code in lbrace.js, +// no need to test twice. Let make just one test for a constructor to ensure it is parsed correctly. describe("[RULE] lbrace: Rejections", function() { it("should reject any opening brace which is not preceded by EXACTLY single space (exception: functions with modifiers)", function(done) { @@ -681,6 +683,13 @@ describe("[RULE] lbrace: Rejections", function() { errors.constructor.name.should.equal("Array"); errors.length.should.equal(1); + + // just one test for constructors (see above) + code = `constructor (${EOL}\tuint x,${EOL}\tstring y,${EOL}\taddress z${EOL}){${EOL}\tfoobar ();${EOL}}`; + errors = Solium.lint(toContract(code), userConfig); + + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(1); code = `function lotsOfArgs (${EOL}\tuint x,${EOL}\tstring y,${EOL}\taddress z${EOL}){${EOL}\tfoobar ();${EOL}}`; errors = Solium.lint(toContract(code), userConfig); diff --git a/test/utils/wrappers.js b/test/utils/wrappers.js index f325773..e240ce7 100644 --- a/test/utils/wrappers.js +++ b/test/utils/wrappers.js @@ -32,6 +32,17 @@ module.exports = { return pre + code + post; }, + /** + * Wrap a solidity statement in valid contract and constructor boilerplate. + * @param {String} code Solidity snippet + * @return {String} wrapped snippet + */ + toConstructor: function(code) { + let pre = `pragma solidity ^0.4.3;${EOL.repeat(3)}contract Wrap {${EOL}\tconstructor() {${EOL}\t\t`; + let post = `${EOL}\t}${EOL}}`; + return pre + code + post; + }, + /** * Prepend solidity contract / library with a pragma statement * @param {String} code Solidity snippet From c48d149352ead0dffbe23f1b0da58810a7bf1346 Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 13:52:09 +0200 Subject: [PATCH 15/18] work about empty function/contructor bodies (does not pass tests now) --- lib/rules/no-empty-blocks.js | 68 +++++-------------- .../rules/no-empty-blocks/no-empty-blocks.js | 18 ++++- 2 files changed, 33 insertions(+), 53 deletions(-) diff --git a/lib/rules/no-empty-blocks.js b/lib/rules/no-empty-blocks.js index c137480..d5e4acf 100644 --- a/lib/rules/no-empty-blocks.js +++ b/lib/rules/no-empty-blocks.js @@ -70,80 +70,46 @@ module.exports = { const { node } = emitted, { body } = node, parent = context.getSourceCode().getParent(node); - if (emitted.exit || isFallbackFunc(parent) || isPayableFuncOrCons(parent)) { + // Historical note: No need to check (as suggested in #264) that a modifier is calling a base constructor, + // because ANY modifier in constructor should make its body not required. + if (emitted.exit || isConstructor(parent) || isFunc(parent)) { + // Use inspectFunctionDeclaration or inspectConstructorDeclaration logic instead, + // not to report the error twice. return; } if (Array.isArray(body) && body.length === 0) { - if (isConstructor(parent)) { - // If an empty block is a constructor func's body, then it shouldn't - // be reported if the constructor is calling a base constructor. - // The cons. must have a modifier with a name that is also present - // in its enclosing contract's inheritance declaration. Eg- - // - // contract Foo is Bar { - // constructor() Bar("hello") {} - // } - // - // See issue #264 - // - // Since we can't fetch enclosing contract from the BlockStatement - // node or its parent (due to current limitations), we use a - // workaround to not report such a constructor. - constructorsToCheckForBaseCall.push(parent.start); - return; - } - report(node); } } - function inspectConstructorDeclaration(emitted) { + function inspectFunctionDeclaration(emitted) { const { node } = emitted; - if (!emitted.exit) { - // Because parent of a node is not accessible during exit phase, - // cache the parents of all constructors during entry so they - // can be used during exit. - enclosingContractsOfConstructors[node.start] = context.getSourceCode().getParent(node); - return; - } - - // No need to check the constructor currently being exited if it - // isn't even flagged for empty block. - if (!constructorsToCheckForBaseCall.includes(node.start)) { - return; + // If node.modifiers is null, it means no modifiers exist for this + // function and it should therefore be reported. + if (node.body.body.length === 0 && !node.modifiers && !isFallbackFunc(node) && !isPayable(node)) { + report(node.body); } + } - // Run constructor inspection while exiting nodes. - // By this time, the constructorsToCheckForBaseCall list has been - // populated. - const enclosingContract = enclosingContractsOfConstructors[node.start]; + function inspectConstructorDeclaration(emitted) { + const { node } = emitted; // If node.modifiers is null, it means no modifiers exist for this // constructor and it should therefore be reported. - for (let i = 0; i < (node.modifiers || []).length; i++) { - const functionModif = node.modifiers[i].name; - - for (let j = 0; j < enclosingContract.is.length; j++) { - // The constructor is calling a base cons, no need - // to report it. - if (enclosingContract.is[j].name === functionModif) { - return; - } - } + if (node.body.body.length === 0 && !node.modifiers && !isPayable(node)) { + report(node.body); } - - report(node.body); } - let constructorsToCheckForBaseCall = [], enclosingContractsOfConstructors = {}; const similarNodeTypes = ["ContractStatement", "LibraryStatement", "InterfaceStatement"]; const response = { BlockStatement: inspectBlockStatement, - ConstructorDeclaration: inspectConstructorDeclaration + ConstructorDeclaration: inspectConstructorDeclaration, + FunctionDeclaration: inspectFunctionDeclaration }; similarNodeTypes.forEach(function(nodeName) { diff --git a/test/lib/rules/no-empty-blocks/no-empty-blocks.js b/test/lib/rules/no-empty-blocks/no-empty-blocks.js index 6115c3c..8aed0bd 100644 --- a/test/lib/rules/no-empty-blocks/no-empty-blocks.js +++ b/test/lib/rules/no-empty-blocks/no-empty-blocks.js @@ -55,13 +55,15 @@ describe("[RULE] no-empty-blocks: Acceptances", function() { }); it("should allow fallback and payable functions & payable constructors to have empty bodies", done => { + // https://gitcoin.co/issue/duaraghav8/Solium/169/780 says: + // "It should only ignore fallaback functions". So I commented them out to ignore. let snippets = [ - "function(string address) {}", + //"function(string address) {}", "function foo(string address) payable external {}", "function(string address) payable public {}", "constructor(uint x) payable {}", - "function(string address) { /* hello world */ }", + //"function(string address) { /* hello world */ }", "function foo(string address) payable external {\t\t\t\t\t\n\n\t}", "function(string address) payable public { }", "constructor(uint x) payable { /* testing */ }" @@ -76,6 +78,18 @@ describe("[RULE] no-empty-blocks: Acceptances", function() { done(); }); + it("should allow functions with modifiers to have empty bodies", done => { + let code = `contract Foo is Bar { + function f(uint _y) m {} + modifier m() { int x = 0; } +}`; + let errors = Solium.lint(code, userConfig); + errors.should.be.empty(); + + Solium.reset(); + done(); + }); + it("should allow constructors calling base constructors to have empty bodies", done => { const code = ` contract Foo is Bar { From 4a6f724e7c281425fca32ea2a86559d1d18f724f Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 14:01:51 +0200 Subject: [PATCH 16/18] bug fix --- lib/rules/no-empty-blocks.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/rules/no-empty-blocks.js b/lib/rules/no-empty-blocks.js index d5e4acf..220e8a5 100644 --- a/lib/rules/no-empty-blocks.js +++ b/lib/rules/no-empty-blocks.js @@ -86,6 +86,10 @@ module.exports = { function inspectFunctionDeclaration(emitted) { const { node } = emitted; + if (emitted.exit) { + return; + } + // If node.modifiers is null, it means no modifiers exist for this // function and it should therefore be reported. if (node.body.body.length === 0 && !node.modifiers && !isFallbackFunc(node) && !isPayable(node)) { @@ -96,6 +100,10 @@ module.exports = { function inspectConstructorDeclaration(emitted) { const { node } = emitted; + if (emitted.exit) { + return; + } + // If node.modifiers is null, it means no modifiers exist for this // constructor and it should therefore be reported. if (node.body.body.length === 0 && !node.modifiers && !isPayable(node)) { From 4a127e72e5e2bbfb6c038c735c024e2fd9b38134 Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 14:16:40 +0200 Subject: [PATCH 17/18] bug fix --- lib/rules/no-empty-blocks.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/rules/no-empty-blocks.js b/lib/rules/no-empty-blocks.js index 220e8a5..7c27d5f 100644 --- a/lib/rules/no-empty-blocks.js +++ b/lib/rules/no-empty-blocks.js @@ -53,6 +53,13 @@ module.exports = { return isFuncOrConstructor(node) && isPayable(node); } + function hasNonTrivialModifiers(node) { + if (!node.modifiers) return false; + + const re = /^(external|public|internal|private|pure|view|payable)$/; + return node.modifiers.filter(m => !re.test(m.name)).length !== 0; + } + function inspectCLI(emitted) { let node = emitted.node, sourceCode = context.getSourceCode(), text = sourceCode.getText(node); @@ -92,7 +99,7 @@ module.exports = { // If node.modifiers is null, it means no modifiers exist for this // function and it should therefore be reported. - if (node.body.body.length === 0 && !node.modifiers && !isFallbackFunc(node) && !isPayable(node)) { + if (node.body.body.length === 0 && !hasNonTrivialModifiers(node) && !isFallbackFunc(node) && !isPayable(node)) { report(node.body); } } @@ -106,7 +113,7 @@ module.exports = { // If node.modifiers is null, it means no modifiers exist for this // constructor and it should therefore be reported. - if (node.body.body.length === 0 && !node.modifiers && !isPayable(node)) { + if (node.body.body.length === 0 && !hasNonTrivialModifiers(node) && !isPayable(node)) { report(node.body); } } From 00b823c09c1ac4cbe6d2dab30afeece86a02aea1 Mon Sep 17 00:00:00 2001 From: Victor Porton Date: Mon, 13 Jan 2020 14:30:19 +0200 Subject: [PATCH 18/18] better to ESLint --- lib/rules/indentation.js | 1 - lib/rules/no-empty-blocks.js | 8 -------- 2 files changed, 9 deletions(-) diff --git a/lib/rules/indentation.js b/lib/rules/indentation.js index f82d2c5..bc8018f 100644 --- a/lib/rules/indentation.js +++ b/lib/rules/indentation.js @@ -293,7 +293,6 @@ module.exports = { function inspectAssemblyBlock(emitted) { let node = emitted.node, - body = node.body || [], endingLineNum = sourceCode.getEndingLine(node); let assemblyDeclarationLineText, currentIndent, currentIndentLevel; diff --git a/lib/rules/no-empty-blocks.js b/lib/rules/no-empty-blocks.js index 7c27d5f..d12e504 100644 --- a/lib/rules/no-empty-blocks.js +++ b/lib/rules/no-empty-blocks.js @@ -34,10 +34,6 @@ module.exports = { return node.type === "ConstructorDeclaration"; } - function isFuncOrConstructor(node) { - return isFunc(node) || isConstructor(node); - } - function isFallbackFunc(node) { return isFunc(node) && node.name === null; } @@ -49,10 +45,6 @@ module.exports = { return false; } - function isPayableFuncOrCons(node) { - return isFuncOrConstructor(node) && isPayable(node); - } - function hasNonTrivialModifiers(node) { if (!node.modifiers) return false;