diff --git a/config/rulesets/solium-all.js b/config/rulesets/solium-all.js index 158a5ba..efd4b5b 100644 --- a/config/rulesets/solium-all.js +++ b/config/rulesets/solium-all.js @@ -37,6 +37,7 @@ module.exports = { "no-experimental": "warning", "max-len": "warning", "error-reason": "warning", + "magic-to-const": "warning", // Turn OFF all deprecated rules "double-quotes": "off", diff --git a/config/rulesets/solium-recommended.js b/config/rulesets/solium-recommended.js index d2ab762..ca0d309 100644 --- a/config/rulesets/solium-recommended.js +++ b/config/rulesets/solium-recommended.js @@ -28,6 +28,7 @@ module.exports = { "no-constant": "warning", "max-len": "warning", "error-reason": "warning", + "magic-to-const": "warning", "lbrace": "off", "mixedcase": "off", diff --git a/config/solium.json b/config/solium.json index a984084..027ce72 100644 --- a/config/solium.json +++ b/config/solium.json @@ -99,6 +99,13 @@ "description": "Ensure that no empty blocks {} exist" }, + "magic-to-const": { + "enabled": true, + "recommended": true, + "type": "warning", + "description": "Ensure that magic numbers are extracted to constants" + }, + "no-unused-vars": { "enabled": true, "recommended": true, diff --git a/docs/user-guide.rst b/docs/user-guide.rst index 752d02b..510a634 100644 --- a/docs/user-guide.rst +++ b/docs/user-guide.rst @@ -373,6 +373,9 @@ 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 } | | +----------------------------+--------------------------------------------------------------------------------------------------------------+-----------------------------------------------------------------------------------+-------------------------------------+-------+ +| magic-to-const | Ensure that magic numbers are extracted to constants | - | | | ++----------------------------+--------------------------------------------------------------------------------------------------------------+-----------------------------------------------------------------------------------+-------------------------------------+-------+ + .. index:: IDE and Editor integrations diff --git a/lib/rules/magic-to-const.js b/lib/rules/magic-to-const.js new file mode 100644 index 0000000..3783220 --- /dev/null +++ b/lib/rules/magic-to-const.js @@ -0,0 +1,57 @@ +/** + * @fileoverview Ensure that magic numbers are extracted to constants + * @author Ivan Mushketyk + */ + +"use strict"; + +const allowedMagicNumbers = [0, 1]; + +module.exports = { + + meta: { + + docs: { + recommended: true, + type: "warning", + description: "Ensure that magic numbers are extracted to constants" + }, + + schema: [] + }, + + create(context) { + + function inspectLiteral(emitted) { + if (emitted.exit) { + return; + } + + const { node } = emitted, + parentNode = context.getSourceCode().getParent(node); + + if (isNumber(node.value) && !isAConstantInitialization(parentNode) && !isAllowed(node.value)) { + context.report({ + node, + message: `Value '${node.value}' should be extracted to a constant` + }); + } + } + + function isNumber(value) { + return typeof(value) === "number"; + } + + function isAConstantInitialization(parentNode) { + return (parentNode.type === "StateVariableDeclaration" && parentNode.is_constant === true); + } + + function isAllowed(value) { + return allowedMagicNumbers.includes(value); + } + + return { + Literal: inspectLiteral + }; + } +}; diff --git a/lib/rules/no-constant.js b/lib/rules/no-constant.js index 7d81a2e..8c73e02 100644 --- a/lib/rules/no-constant.js +++ b/lib/rules/no-constant.js @@ -9,7 +9,7 @@ function create(context) { function reportIfConstant(emitted) { const { node } = emitted, enclosingFunction = context.getSourceCode().getParent(node); - + if (emitted.exit || node.name !== "constant" || enclosingFunction.type !== "FunctionDeclaration") { return; } diff --git a/test/lib/rules.js b/test/lib/rules.js index 32bd983..b1cce27 100644 --- a/test/lib/rules.js +++ b/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(); }); diff --git a/test/lib/rules/magic-to-const/magic-to-const.js b/test/lib/rules/magic-to-const/magic-to-const.js new file mode 100644 index 0000000..3a5b5c1 --- /dev/null +++ b/test/lib/rules/magic-to-const/magic-to-const.js @@ -0,0 +1,32 @@ +/** + * @fileoverview Tests for magic numbers checking rule + * @author Ivan Mushketyk + */ + +"use strict"; + +let wrappers = require("../../../utils/wrappers"); +let acceptanceCases = wrappers.acceptanceCases; +let rejectionCases = wrappers.rejectionCases; + +let userConfig = { + "custom-rules-filename": null, + "rules": { + "magic-to-const": true + } +}; + +acceptanceCases("magic-to-const", userConfig, + [ + "uint constant x = 123;", + "uint a = 1; uint b = 0; uint c = -1;", + "string a = \"abc\";", + "string a = \"1\";" + ] +); + +rejectionCases("magic-to-const", userConfig, + [ + "uint a = 5;" + ] +); \ No newline at end of file diff --git a/test/lib/rules/no-constant/no-constant.js b/test/lib/rules/no-constant/no-constant.js index 35d0236..28780c1 100644 --- a/test/lib/rules/no-constant/no-constant.js +++ b/test/lib/rules/no-constant/no-constant.js @@ -6,62 +6,36 @@ "use strict"; const Solium = require("../../../../lib/solium"); +let wrappers = require("../../../utils/wrappers"); +let acceptanceCases = wrappers.acceptanceCases; +let rejectionCases = wrappers.rejectionCases; const userConfig = { "rules": { "no-constant": "error" } }; - -describe("[RULE] emit: Acceptances", () => { - - it("should accept function declarations that don't have constant modifier", done => { - const declarations = [ - "function foo() view pure returns(bool);", - "function foo() view pure returns(bool) {}", - "function foo() {}", - "function foo();", - "function(){}", - "function foo() myModifier(100, \"hello\") boo;" - ]; - - declarations.forEach(func => { - const issues = Solium.lint(`contract Foo { ${func} }`, userConfig); - - issues.should.be.Array(); - issues.should.be.empty(); - }); - - done(); - }); - -}); - - -describe("[RULE] emit: Rejections", () => { - - it("should reject function declarations that have constant modifier", done => { - const declarations = [ - "function foo() view pure constant returns(bool);", - "function foo() constant pure returns(bool) {}", - "function foo() constant {}", - "function foo() constant;", - "function()constant{}", - "function foo() myModifier(100, \"hello\") constant boo;" - ]; - - declarations.forEach(func => { - const issues = Solium.lint(`contract Foo { ${func} }`, userConfig); - - issues.should.be.Array(); - issues.should.have.size(1); - }); - - done(); - }); - -}); - +acceptanceCases("no-constant", userConfig, + [ + "function foo() view pure returns(bool);", + "function foo() view pure returns(bool) {}", + "function foo() {}", + "function foo();", + "function(){}", + "function foo() myModifier(100, \"hello\") boo;" + ] +); + +rejectionCases("no-constant", userConfig, + [ + "function foo() view pure constant returns(bool);", + "function foo() constant pure returns(bool) {}", + "function foo() constant {}", + "function foo() constant;", + "function()constant{}", + "function foo() myModifier(100, \"hello\") constant boo;" + ] +); describe("[RULE] emit: fixes", () => { diff --git a/test/lib/solium.js b/test/lib/solium.js index 30660aa..f8c51f9 100644 --- a/test/lib/solium.js +++ b/test/lib/solium.js @@ -26,7 +26,7 @@ describe("Checking Exported Solium API", function() { Solium.should.be.type("object"); Solium.should.be.instanceof(EventEmitter); Solium.should.have.size(11); - + Solium.should.have.ownProperty("reset"); Solium.reset.should.be.type("function"); Solium.should.have.ownProperty("lint"); @@ -862,7 +862,7 @@ describe("Solium.lint() comment directives", () => { const errors = Solium.lint(code, { "extends": "solium:all" }); errors.should.be.Array(); - errors.should.have.size(10); // This no. can change if changes are made in any rules from solium:all ruleset + errors.should.have.size(12); // This no. can change if changes are made in any rules from solium:all ruleset Solium.reset(); done(); diff --git a/test/utils/wrappers.js b/test/utils/wrappers.js index f325773..2b40e23 100644 --- a/test/utils/wrappers.js +++ b/test/utils/wrappers.js @@ -7,6 +7,8 @@ "use strict"; +let Solium = require("../../lib/solium"); + const { EOL } = require("os"); module.exports = { @@ -40,5 +42,47 @@ module.exports = { addPragma: function(code) { let pre = `pragma solidity ^0.4.3;${EOL.repeat(3)}`; return pre + code; + }, + + noErrors: function(code, userConfig) { + let errors = Solium.lint(code, userConfig); + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(0); + }, + + expectError: function(code, userConfig) { + let errors = Solium.lint(code, userConfig); + errors.constructor.name.should.equal("Array"); + errors.length.should.equal(1); + }, + + acceptanceCases: function(ruleName, userConfig, codeSnippets) { + describe(`[RULE] ${ruleName}: Acceptances`, function() { + codeSnippets.forEach((code) => { + it(`should accept - ${code}`, function(done) { + let contractCode = module.exports.toContract(code); + + module.exports.noErrors(contractCode, userConfig); + + Solium.reset(); + done(); + }); + }); + }); + }, + + rejectionCases: function(ruleName, userConfig, codeSnippets) { + describe(`[RULE] ${ruleName}: Rejections`, function() { + codeSnippets.forEach((code) => { + it(`should reject - ${code}`, function(done) { + let contractCode = module.exports.toContract(code); + + module.exports.expectError(contractCode, userConfig); + + Solium.reset(); + done(); + }); + }); + }); } }; \ No newline at end of file