From 62981dea40af2399a8823d09c38cc4fcdcfc7450 Mon Sep 17 00:00:00 2001 From: junderw Date: Fri, 31 Jul 2020 07:41:46 +0900 Subject: [PATCH] Support no-visibility constructors in >=0.7.0 Co-authored-by: Franco Victorio --- docs/rules/security/func-visibility.md | 11 ++++- lib/rules/security/func-visibility.js | 43 +++++++++++++++++-- lib/rules/security/index.js | 2 +- package-lock.json | 10 ++--- package.json | 2 +- .../security/functions-with-visibility.js | 3 +- test/rules/security/func-visibility.js | 26 +++++++++++ 7 files changed, 83 insertions(+), 14 deletions(-) diff --git a/docs/rules/security/func-visibility.md b/docs/rules/security/func-visibility.md index 61ced878..97e13645 100644 --- a/docs/rules/security/func-visibility.md +++ b/docs/rules/security/func-visibility.md @@ -15,13 +15,19 @@ title: "func-visibility | Solhint" Explicitly mark visibility in function. ## Options -This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warn. +This rule accepts an array of options: + +| Index | Description | Default Value | +| ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------- | +| 0 | Rule severity. Must be one of "error", "warn", "off". | warn | +| 1 | A JSON object with a single property "ignoreConstructors" specifying if the rule should ignore constructors. (**Note: This is required to be true for Solidity >=0.7.0 and false for <0.7.0**) | {"ignoreConstructors":false} | + ### Example Config ```json { "rules": { - "func-visibility": "warn" + "func-visibility": ["warn",{"ignoreConstructors":false}] } } ``` @@ -37,6 +43,7 @@ function b() internal { } function b() external { } function b() private { } function b() public { } +constructor() public { } ``` ### 👎 Examples of **incorrect** code for this rule diff --git a/lib/rules/security/func-visibility.js b/lib/rules/security/func-visibility.js index b96f5d87..9f7d8fbb 100644 --- a/lib/rules/security/func-visibility.js +++ b/lib/rules/security/func-visibility.js @@ -1,4 +1,9 @@ const BaseChecker = require('./../base-checker') +const { severityDescription } = require('../../doc/utils') + +const DEFAULT_SEVERITY = 'warn' +const DEFAULT_IGNORE_CONSTRUCTORS = false +const DEFAULT_OPTION = { ignoreConstructors: DEFAULT_IGNORE_CONSTRUCTORS } const ruleId = 'func-visibility' const meta = { @@ -7,6 +12,17 @@ const meta = { docs: { description: `Explicitly mark visibility in function.`, category: 'Security Rules', + options: [ + { + description: severityDescription, + default: DEFAULT_SEVERITY + }, + { + description: + 'A JSON object with a single property "ignoreConstructors" specifying if the rule should ignore constructors. (**Note: This is required to be true for Solidity >=0.7.0 and false for <0.7.0**)', + default: JSON.stringify(DEFAULT_OPTION) + } + ], examples: { good: [ { @@ -25,19 +41,38 @@ const meta = { isDefault: false, recommended: true, - defaultSetup: 'warn', + defaultSetup: [DEFAULT_SEVERITY, DEFAULT_OPTION], - schema: null + schema: { + type: 'object', + properties: { + ignoreConstructors: { + type: 'boolean' + } + } + } } class FuncVisibilityChecker extends BaseChecker { - constructor(reporter) { + constructor(reporter, config) { super(reporter, ruleId, meta) + + this.ignoreConstructors = + config && config.getObjectPropertyBoolean(ruleId, 'ignoreConstructors', false) } FunctionDefinition(node) { + if (node.isConstructor && this.ignoreConstructors) { + return + } + if (node.visibility === 'default') { - this.warn(node, 'Explicitly mark visibility in function') + this.warn( + node, + node.isConstructor + ? 'Explicitly mark visibility in function (Set ignoreConstructors to true if using solidity >=0.7.0)' + : 'Explicitly mark visibility in function' + ) } } } diff --git a/lib/rules/security/index.js b/lib/rules/security/index.js index 5b928b00..cdb709c6 100644 --- a/lib/rules/security/index.js +++ b/lib/rules/security/index.js @@ -26,7 +26,7 @@ module.exports = function security(reporter, config, inputSrc) { new AvoidTxOriginChecker(reporter), new CheckSendResultChecker(reporter), new CompilerVersionChecker(reporter, config), - new FuncVisibilityChecker(reporter), + new FuncVisibilityChecker(reporter, config), new MarkCallableContractsChecker(reporter), new MultipleSendsChecker(reporter), new NoComplexFallbackChecker(reporter), diff --git a/package-lock.json b/package-lock.json index 688f7449..ed101bdf 100644 --- a/package-lock.json +++ b/package-lock.json @@ -391,9 +391,9 @@ "dev": true }, "@solidity-parser/parser": { - "version": "0.6.0", - "resolved": "https://registry.npmjs.org/@solidity-parser/parser/-/parser-0.6.0.tgz", - "integrity": "sha512-RiJXfS22frulogcfQCFhbKrd5ATu6P4tYUv/daChiIh6VHyKQ1kkVZVfX6aP7c2YGU/Bf9RwGNKdwLjfpaoqYQ==" + "version": "0.7.0", + "resolved": "https://registry.npmjs.org/@solidity-parser/parser/-/parser-0.7.0.tgz", + "integrity": "sha512-YJ333ezgd9slnwCpFQVfsBcYsTcLWZRpVswlKgS82YDZPzzNtVnkEs5DX5+jMsu8PNnVxwZuxC6ucukima9x6w==" }, "@stryker-mutator/api": { "version": "2.3.0", @@ -1280,7 +1280,7 @@ "dependencies": { "callsites": { "version": "2.0.0", - "resolved": "http://registry.npmjs.org/callsites/-/callsites-2.0.0.tgz", + "resolved": "https://registry.npmjs.org/callsites/-/callsites-2.0.0.tgz", "integrity": "sha1-BuuE8A7qQT2oav/vrL/7Ngk7PFA=" } } @@ -2376,7 +2376,7 @@ }, "fast-deep-equal": { "version": "1.1.0", - "resolved": "http://registry.npmjs.org/fast-deep-equal/-/fast-deep-equal-1.1.0.tgz", + "resolved": "https://registry.npmjs.org/fast-deep-equal/-/fast-deep-equal-1.1.0.tgz", "integrity": "sha1-wFNHeBfIa1HaqFPIHgWbcz0CNhQ=", "dev": true }, diff --git a/package.json b/package.json index 770ac428..ad31ddd0 100644 --- a/package.json +++ b/package.json @@ -29,7 +29,7 @@ "author": "Ilya Drabenia ", "license": "MIT", "dependencies": { - "@solidity-parser/parser": "^0.6.0", + "@solidity-parser/parser": "^0.7.0", "ajv": "^6.6.1", "antlr4": "4.7.1", "ast-parents": "0.0.1", diff --git a/test/fixtures/security/functions-with-visibility.js b/test/fixtures/security/functions-with-visibility.js index fd74aa3e..95b31b34 100644 --- a/test/fixtures/security/functions-with-visibility.js +++ b/test/fixtures/security/functions-with-visibility.js @@ -2,5 +2,6 @@ module.exports = [ 'function b() internal { }', 'function b() external { }', 'function b() private { }', - 'function b() public { }' + 'function b() public { }', + 'constructor() public { }' ] diff --git a/test/rules/security/func-visibility.js b/test/rules/security/func-visibility.js index 4c80e1eb..cba2a821 100644 --- a/test/rules/security/func-visibility.js +++ b/test/rules/security/func-visibility.js @@ -27,4 +27,30 @@ describe('Linter - func-visibility', () => { assert.equal(report.warningCount, 0) }) }) + + describe("when 'ignoreConstructors' is enabled", () => { + it('should ignore constructors without visibility', () => { + const code = contractWith('constructor () {}') + + const report = linter.processStr(code, { + rules: { + 'func-visibility': ['warn', { ignoreConstructors: true }] + } + }) + + assert.equal(report.warningCount, 0) + }) + + it('should still report functions without visibility', () => { + const code = contractWith('function foo() {}') + + const report = linter.processStr(code, { + rules: { + 'func-visibility': ['warn', { ignoreConstructors: true }] + } + }) + + assert.equal(report.warningCount, 1) + }) + }) })