diff --git a/lib/rules/visibility-first.js b/lib/rules/visibility-first.js index 472b829..5a1c093 100644 --- a/lib/rules/visibility-first.js +++ b/lib/rules/visibility-first.js @@ -10,41 +10,41 @@ module.exports = { docs: { recommended: true, type: "warning", - description: - "Ensure that the visibility modifier for a function should come before any custom modifiers" + description: "Ensure that the visibility modifier for a function should come before any custom modifiers" }, schema: [] }, create(context) { - function inspect(emitted) { - const visibilityModifiers = ["public", "external", "internal", "private"]; - const { node } = emitted; - const modifiers = (node.modifiers || []).map(m => m.name); - - if (emitted.exit) { + // Find index of the first visibility modifier in declaration. + // Find the first non-VM before this first VM found above. + // If non-VM found, report the VM. + function inspectFD(emitted) { + const { node } = emitted, + visibilityModifiers = ["public", "external", "internal", "private"]; + const modifiers = (node.modifiers || []), + firstVisibilityModifierIndex = modifiers.findIndex(m => visibilityModifiers.includes(m.name)); + + // If no visibility modifiers exist in function declaration, exit now + if (emitted.exit || firstVisibilityModifierIndex === -1) { return; } - modifiers.forEach((m, index) => { - if (!visibilityModifiers.includes(m)) { - return; - } - - const misplacedModifier = modifiers - .slice(0, index) - .find(e => !visibilityModifiers.includes(e)); - - if (misplacedModifier) { - context.report({ - node, - message: `FunctionDeclaration name "${node.name}" doesn't follow the function visibility modifier order` - }); - } - }); + const firstNonVisModifBeforeFirstVisModif = modifiers.slice(0, firstVisibilityModifierIndex).find(m => !visibilityModifiers.includes(m.name)); + + // TODO: Add fix() for this rule + if (firstNonVisModifBeforeFirstVisModif) { + const issue = { + node: modifiers[firstVisibilityModifierIndex], + message: `Visibility modifier "${modifiers[firstVisibilityModifierIndex].name}" should come before other modifiers.` + }; + context.report(issue); + } } - return { FunctionDeclaration: inspect }; + return { + FunctionDeclaration: inspectFD + }; } }; diff --git a/test/lib/rules/visibility-first/visibility-first.js b/test/lib/rules/visibility-first/visibility-first.js index 0aea60e..3f9fa28 100644 --- a/test/lib/rules/visibility-first/visibility-first.js +++ b/test/lib/rules/visibility-first/visibility-first.js @@ -5,9 +5,8 @@ "use strict"; -const Solium = require("../../../../lib/solium"); -const wrappers = require("../../../utils/wrappers"); -const toContract = wrappers.toContract; +const Solium = require("../../../../lib/solium"), + { toContract } = require("../../../utils/wrappers"); const userConfig = { "rules": { @@ -18,6 +17,7 @@ const userConfig = { describe("[RULE] visibility-first: Acceptances", () => { it("accepts valid contract names", done => { let code = [ + "function test() public onlyOwner modA modB modC modD private modE {}", "function test() public onlyOwner {}", "function test() external onlyOwner {}", "function test() internal onlyOwner {}", @@ -33,45 +33,11 @@ describe("[RULE] visibility-first: Acceptances", () => { code = code.map(item => toContract(item)); - errors = Solium.lint(code [0], userConfig); - errors.constructor.name.should.equal("Array"); - errors.length.should.equal(0); - - errors = Solium.lint(code [1], userConfig); - errors.constructor.name.should.equal("Array"); - errors.length.should.equal(0); - - errors = Solium.lint(code [2], userConfig); - errors.constructor.name.should.equal("Array"); - errors.length.should.equal(0); - - errors = Solium.lint(code [3], userConfig); - errors.constructor.name.should.equal("Array"); - errors.length.should.equal(0); - - errors = Solium.lint(code [4], userConfig); - errors.constructor.name.should.equal("Array"); - errors.length.should.equal(0); - - errors = Solium.lint(code [5], userConfig); - errors.constructor.name.should.equal("Array"); - errors.length.should.equal(0); - - errors = Solium.lint(code [6], userConfig); - errors.constructor.name.should.equal("Array"); - errors.length.should.equal(0); - - errors = Solium.lint(code [7], userConfig); - errors.constructor.name.should.equal("Array"); - errors.length.should.equal(0); - - errors = Solium.lint(code [8], userConfig); - errors.constructor.name.should.equal("Array"); - errors.length.should.equal(0); - - errors = Solium.lint(code [9], userConfig); - errors.constructor.name.should.equal("Array"); - errors.length.should.equal(0); + code.forEach(snip => { + errors = Solium.lint(snip, userConfig); + errors.should.be.Array(); + errors.should.be.empty(); + }); Solium.reset(); done(); @@ -82,6 +48,9 @@ describe("[RULE] visibility-first: Acceptances", () => { describe("[RULE] visibility-first: Rejections", () => { it("rejects invalid struct names", done => { let code = [ + "function test() onlyOwner modA modB modC public {}", + "function test() onlyOwner modA modB modC external modD modE {}", + "function test() onlyOwner modA modB modC internal modD modE private {}", "function test() onlyOwner public {}", "function test() onlyOwner external {}", "function test() onlyOwner internal {}", @@ -91,21 +60,11 @@ describe("[RULE] visibility-first: Rejections", () => { code = code.map(item => toContract(item)); - errors = Solium.lint(code [0], userConfig); - errors.constructor.name.should.equal("Array"); - errors.length.should.equal(1); - - errors = Solium.lint(code [1], userConfig); - errors.constructor.name.should.equal("Array"); - errors.length.should.equal(1); - - errors = Solium.lint(code [2], userConfig); - errors.constructor.name.should.equal("Array"); - errors.length.should.equal(1); - - errors = Solium.lint(code [3], userConfig); - errors.constructor.name.should.equal("Array"); - errors.length.should.equal(1); + code.forEach(snip => { + errors = Solium.lint(snip, userConfig); + errors.should.be.Array(); + errors.should.have.size(1); + }); Solium.reset(); done();