diff --git a/config/rulesets/solium-all.js b/config/rulesets/solium-all.js index d8ad91d..7b258fd 100644 --- a/config/rulesets/solium-all.js +++ b/config/rulesets/solium-all.js @@ -22,6 +22,7 @@ module.exports = { "indentation": "warning", "arg-overflow": "warning", "whitespace": "warning", + "magic-number": "warning", "function-whitespace": "error", "semicolon-whitespace": "error", "comma-whitespace": "error", diff --git a/config/rulesets/solium-recommended.js b/config/rulesets/solium-recommended.js index 9327b46..d8fe997 100644 --- a/config/rulesets/solium-recommended.js +++ b/config/rulesets/solium-recommended.js @@ -25,6 +25,7 @@ module.exports = { "operator-whitespace": "warning", "emit": "warning", "no-constant": "warning", + "magic-number": "warning", "lbrace": "off", "mixedcase": "off", diff --git a/config/solium.json b/config/solium.json index aec448e..77e0df9 100644 --- a/config/solium.json +++ b/config/solium.json @@ -99,6 +99,13 @@ "description": "Ensure that no empty blocks {} exist" }, + "magic-number": { + "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 9c7916a..0cae248 100644 --- a/docs/user-guide.rst +++ b/docs/user-guide.rst @@ -347,6 +347,8 @@ For eg- your choice of indentation might be Tab or 4 spaces or 2 spaces. What in +----------------------------+--------------------------------------------------------------------------------------------------------------+-----------------------------------------------------------------------------------+-----------------+-------+ | quotes | Ensure that all strings use only 1 style - either double quotes or single quotes | Single option - either "double" or "single" | double | YES | +----------------------------+--------------------------------------------------------------------------------------------------------------+-----------------------------------------------------------------------------------+-----------------+-------+ +| magic-number | Ensure that magic numbers are extracted to constants | - | | | ++----------------------------+--------------------------------------------------------------------------------------------------------------+-----------------------------------------------------------------------------------+-----------------+-------+ | blank-lines | Ensure that there is exactly a 2-line gap between Contract and Funtion declarations | - | | YES | +----------------------------+--------------------------------------------------------------------------------------------------------------+-----------------------------------------------------------------------------------+-----------------+-------+ | indentation | Ensure consistent indentation of 4 spaces per level | either "tab" or an integer representing the number of spaces | 4 spaces | | diff --git a/lib/rules/double-quotes.js b/lib/rules/double-quotes.js index 93d345e..d4df026 100644 --- a/lib/rules/double-quotes.js +++ b/lib/rules/double-quotes.js @@ -55,7 +55,7 @@ module.exports = { if (!doubleQuotesLiteralRegExp.test(nodeText)) { context.report({ node: node, - message: "'" + node.value + "': String Literals must be quoted with \"double quotes\" only." + message: `'${node.value}': String Literals must be quoted with "double quotes" only.` }); } } diff --git a/lib/rules/magic-number.js b/lib/rules/magic-number.js new file mode 100644 index 0000000..2498c32 --- /dev/null +++ b/lib/rules/magic-number.js @@ -0,0 +1,60 @@ +/** + * @fileoverview Ensure that magic numbers are extracted to constants + * @author Ivan Mushketyk + */ + +"use strict"; + +module.exports = { + + meta: { + + docs: { + recommended: true, + type: "warning", + description: "Ensure that magic numbers are extracted to constants", + replacedBy: ["quotes"] + }, + + schema: [], + deprecated: true + + }, + + create: function(context) { + + function inspectLiteral(emitted) { + if (emitted.exit) { + return; + } + + let parentNode = context.getSourceCode().getParent(emitted.node); + if (_isNumber(emitted.node.value) && + !_constantInitialization(parentNode) && + !_allowedMagicNumber(emitted.node.value)) { + + context.report({ + node: emitted.node, + message: `Value '${emitted.node.value}' should be extracted to a constant` + }); + } + } + + function _isNumber(value) { + return typeof(value) === "number"; + } + + function _constantInitialization(parentNode) { + return parentNode.type === "StateVariableDeclaration" && + parentNode.is_constant === true; + } + + function _allowedMagicNumber(value) { + return value === 0 || value === 1; + } + + 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 a819eb9..5c7c8db 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(22); + Object.keys(ruleDescriptions).length.should.equal(23); done(); }); diff --git a/test/lib/rules/magic-number/magic-number.js b/test/lib/rules/magic-number/magic-number.js new file mode 100644 index 0000000..e55ce52 --- /dev/null +++ b/test/lib/rules/magic-number/magic-number.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-number": true + } +}; + +acceptanceCases("magic-number", userConfig, + [ + "uint constant x = 123;", + "uint a = 1; uint b = 0; uint c = -1;", + "string a = \"abc\";", + "string a = \"1\";" + ] +); + +rejectionCases("magic-number", 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 1275261..da9721c 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