Merge pull request #219 from hbeckeri/master

Rule for visibility before modifiers

config/rulesets/solium-all.js

@@ -37,6 +37,7 @@ module.exports = {
         "no-experimental": "warning",
         "max-len": "warning",
         "error-reason": "warning",
+        "visibility-first": "warning",
 
         // Turn OFF all deprecated rules
         "double-quotes": "off",

config/rulesets/solium-recommended.js

@@ -28,6 +28,7 @@ module.exports = {
         "no-constant": "warning",
         "max-len": "warning",
         "error-reason": "warning",
+        "visibility-first": "warning",
 
         "lbrace": "off",
         "mixedcase": "off",

config/solium.json

@@ -209,7 +209,13 @@
 			"recommended": true,
 			"type": "warning",
 			"description": "Ensure that error message is provided for revert and require statements"
-		}
+		},
 
+		"visibility-first": {
+			"enabled": true,
+			"recommended": true,
+			"type": "warning",
+			"description": "Ensure that the visibility modifier for a function should come before any custom modifiers"
+		}
 	}
 }

docs/user-guide.rst

@@ -373,6 +373,8 @@ For eg- your choice of indentation might be Tab or 4 spaces or 2 spaces. What in
 +----------------------------+--------------------------------------------------------------------------------------------------------------+-----------------------------------------------------------------------------------+-------------------------------------+-------+
 | error-reason               | Ensure that error message is provided for revert and require statements                                      |            Object with "revert" and "require" keys with boolean values            | { "revert": true, "require": true } |       |
 +----------------------------+--------------------------------------------------------------------------------------------------------------+-----------------------------------------------------------------------------------+-------------------------------------+-------+
+| visibility-first           | Ensure that the visibility modifier for a function should come before any custom modifiers                   |                                         -                                         |                                     |       |
++----------------------------+--------------------------------------------------------------------------------------------------------------+-----------------------------------------------------------------------------------+-------------------------------------+-------+
 
 .. index:: IDE and Editor integrations
 

lib/rules/visibility-first.js

@@ -0,0 +1,50 @@
+/**
+ * @fileoverview Ensure that the visibility modifier for a function should come before any custom modifiers.
+ * @author Harrison Beckerich <https://github.com/hbeckeri>
+ */
+
+"use strict";
+
+module.exports = {
+    meta: {
+        docs: {
+            recommended: true,
+            type: "warning",
+            description: "Ensure that the visibility modifier for a function should come before any custom modifiers"
+        },
+
+        schema: []
+    },
+
+    create(context) {
+        // 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;
+            }
+
+            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: inspectFD
+        };
+    }
+};

lib/utils/rule-inspector.js

@@ -46,4 +46,4 @@ module.exports = {
         return validateOptionsList(options);
     }
 
-};
+};

test/lib/rules.js

@@ -212,7 +212,7 @@ describe("Checking exported rules object", function() {
         // We extend ALL solium core rules and eliminate a few by setting their severity to 0.
         // The rest of the rules should all be available.
         // The below count will keep changing with every change in the number of core rules that exist in solium.
-        Object.keys(ruleDescriptions).length.should.equal(26);
+        Object.keys(ruleDescriptions).length.should.equal(27);
 
         done();
     });

test/lib/rules/visibility-first/visibility-first.js

@@ -0,0 +1,72 @@
+/**
+ * @fileoverview Tests for visibility-first rule.
+ * @author Harrison Beckerich <https://github.com/hbeckeri>
+ */
+
+"use strict";
+
+const Solium = require("../../../../lib/solium"),
+    { toContract } = require("../../../utils/wrappers");
+
+const userConfig = {
+    "rules": {
+        "visibility-first": "warning"
+    }
+};
+
+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 {}",
+            "function test() private onlyOwner {}",
+            "function test() onlyOwner {}",
+            "function test() public {}",
+            "function test() external {}",
+            "function test() internal {}",
+            "function test() private {}",
+            "function test() {}"
+        ];
+        let errors;
+
+        code = code.map(item => toContract(item));
+
+        code.forEach(snip => {
+            errors = Solium.lint(snip, userConfig);
+            errors.should.be.Array();
+            errors.should.be.empty();
+        });
+
+        Solium.reset();
+        done();
+    });
+});
+
+
+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 {}",
+            "function test() onlyOwner private {}"
+        ];
+        let errors;
+
+        code = code.map(item => toContract(item));
+
+        code.forEach(snip => {
+            errors = Solium.lint(snip, userConfig);
+            errors.should.be.Array();
+            errors.should.have.size(1);
+        });
+
+        Solium.reset();
+        done();
+    });
+});