diff --git a/README.md b/README.md index 338d1fd1..1963c4c4 100644 --- a/README.md +++ b/README.md @@ -64,6 +64,7 @@ Options: --ignore-path [file_name] file to use as your .solhintignore --fix automatically fix problems. Skip report --fixShow automatically fix problems. Show report + --noPrompt do not suggest to backup files when any `fix` option is selected --init create configuration file for solhint --disc do not check for solhint updates --save save report to file on current folder @@ -76,8 +77,15 @@ Commands: ``` ### Notes - Solhint checks if there are newer versions. The `--disc` option avoids that check. -- `--fix` option currently works only on "avoid-throw" and "avoid-sha3" rules. - `--save` option will create a file named as `YYYYMMDDHHMMSS_solhintReport.txt` on current folder with default or specified format + +### Fix +This option currently works on: +- avoid-throw +- avoid-sha3 +- no-console +- explicit-types +- private-vars-underscore

## Configuration diff --git a/docs/rules.md b/docs/rules.md index 66d12655..64185cd1 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -48,7 +48,7 @@ title: "Rule Index of Solhint" | [modifier-name-mixedcase](./rules/naming/modifier-name-mixedcase.md) | Modifier name must be in mixedCase. | | | | [named-parameters-mapping](./rules/naming/named-parameters-mapping.md) | Solidity v0.8.18 introduced named parameters on the mappings definition. | | | | [named-return-values](./rules/naming/named-return-values.md) | Enforce the return values of a function to be named | | | -| [private-vars-leading-underscore](./rules/naming/private-vars-leading-underscore.md) | Private and internal names must start with a single underscore. | | | +| [private-vars-leading-underscore](./rules/naming/private-vars-leading-underscore.md) | Non-external functions and state variables should start with a single underscore. Others, shouldn't | | | | [use-forbidden-name](./rules/naming/use-forbidden-name.md) | Avoid to use letters 'I', 'l', 'O' as identifiers. | $~~~~~~~~$✔️ | | | [var-name-mixedcase](./rules/naming/var-name-mixedcase.md) | Variable name must be in mixedCase. (Does not check IMMUTABLES, use immutable-vars-naming) | $~~~~~~~~$✔️ | | | [func-order](./rules/order/func-order.md) | Function order is incorrect. | | $~~~~~~~$✔️ | diff --git a/docs/rules/naming/private-vars-leading-underscore.md b/docs/rules/naming/private-vars-leading-underscore.md index 1bb34921..6bcbaee6 100644 --- a/docs/rules/naming/private-vars-leading-underscore.md +++ b/docs/rules/naming/private-vars-leading-underscore.md @@ -9,15 +9,15 @@ title: "private-vars-leading-underscore | Solhint" ![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow) ## Description -Private and internal names must start with a single underscore. +Non-external functions and state variables should start with a single underscore. Others, shouldn't. ## Options 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 "strict" specifying if the rule should apply to non state variables. Default: { strict: false }. | {"strict":false} | +| Index | Description | Default Value | +| ----- | ----------------------------------------------------------------------------------------------------------------------------------------- | ---------------- | +| 0 | Rule severity. Must be one of "error", "warn", "off". | warn | +| 1 | A JSON object with a single property "strict" specifying if the rule should apply to ALL non state variables. Default: { strict: false }. | {"strict":false} | ### Example Config @@ -30,6 +30,7 @@ This rule accepts an array of options: ``` ### Notes +- This rule considers functions and variables in Libraries as well - This rule skips external and public functions - This rule skips external and public state variables - See [here](https://docs.soliditylang.org/en/latest/style-guide.html#underscore-prefix-for-non-external-functions-and-variables) for further information diff --git a/lib/rules/naming/private-vars-leading-underscore.js b/lib/rules/naming/private-vars-leading-underscore.js index 6c02bd05..bcbfdff3 100644 --- a/lib/rules/naming/private-vars-leading-underscore.js +++ b/lib/rules/naming/private-vars-leading-underscore.js @@ -11,7 +11,8 @@ const meta = { type: 'naming', docs: { - description: 'Private and internal names must start with a single underscore.', + description: + "Non-external functions and state variables should start with a single underscore. Others, shouldn't", category: 'Style Guide Rules', options: [ { @@ -20,7 +21,7 @@ const meta = { }, { description: - 'A JSON object with a single property "strict" specifying if the rule should apply to non state variables. Default: { strict: false }.', + 'A JSON object with a single property "strict" specifying if the rule should apply to ALL non state variables. Default: { strict: false }.', default: JSON.stringify(DEFAULT_OPTION), }, ], @@ -65,6 +66,9 @@ const meta = { ], }, notes: [ + { + note: 'This rule considers functions and variables in Libraries as well', + }, { note: 'This rule skips external and public functions', }, @@ -80,6 +84,7 @@ const meta = { isDefault: false, recommended: false, defaultSetup: [DEFAULT_SEVERITY, DEFAULT_OPTION], + fixable: true, schema: { type: 'object', @@ -98,15 +103,15 @@ class PrivateVarsLeadingUnderscoreChecker extends BaseChecker { this.isStrict = config && config.getObjectPropertyBoolean(ruleId, 'strict', DEFAULT_STRICTNESS) } - ContractDefinition(node) { - if (node.kind === 'library') { - this.inLibrary = true - } - } + // ContractDefinition(node) { + // if (node.kind === 'library') { + // this.inLibrary = true + // } + // } - 'ContractDefinition:exit'() { - this.inLibrary = false - } + // 'ContractDefinition:exit'() { + // this.inLibrary = false + // } FunctionDefinition(node) { if (!node.name) { @@ -114,9 +119,10 @@ class PrivateVarsLeadingUnderscoreChecker extends BaseChecker { } const isPrivate = node.visibility === 'private' - const isInternal = node.visibility === 'internal' - const shouldHaveLeadingUnderscore = isPrivate || (!this.inLibrary && isInternal) - this.validateName(node, shouldHaveLeadingUnderscore) + const isInternal = node.visibility === 'internal' || node.visibility === 'default' + // const shouldHaveLeadingUnderscore = isPrivate || (!this.inLibrary && isInternal) + const shouldHaveLeadingUnderscore = isPrivate || isInternal + this.validateName(node, shouldHaveLeadingUnderscore, 'function') } StateVariableDeclaration() { @@ -131,7 +137,7 @@ class PrivateVarsLeadingUnderscoreChecker extends BaseChecker { if (!this.inStateVariableDeclaration) { // if strict is enabled, non-state vars should not start with leading underscore if (this.isStrict) { - this.validateName(node, false) + this.validateName(node, false, 'variable') } return } @@ -139,25 +145,43 @@ class PrivateVarsLeadingUnderscoreChecker extends BaseChecker { const isPrivate = node.visibility === 'private' const isInternal = node.visibility === 'internal' || node.visibility === 'default' const shouldHaveLeadingUnderscore = isPrivate || isInternal - this.validateName(node, shouldHaveLeadingUnderscore) + this.validateName(node, shouldHaveLeadingUnderscore, 'variable') } - validateName(node, shouldHaveLeadingUnderscore) { + validateName(node, shouldHaveLeadingUnderscore, type) { if (node.name === null) { return } if (naming.hasLeadingUnderscore(node.name) !== shouldHaveLeadingUnderscore) { - this._error(node, node.name, shouldHaveLeadingUnderscore) + this._error(node, node.name, shouldHaveLeadingUnderscore, type) } } - _error(node, name, shouldHaveLeadingUnderscore) { + _error(node, name, shouldHaveLeadingUnderscore, type) { this.error( node, - `'${name}' ${shouldHaveLeadingUnderscore ? 'should' : 'should not'} start with _` + `'${name}' ${shouldHaveLeadingUnderscore ? 'should' : 'should not'} start with _`, + this.fixStatement(node, shouldHaveLeadingUnderscore, type) ) } + + fixStatement(node, shouldHaveLeadingUnderscore, type) { + let range + + if (type === 'function') { + range = node.range + range[0] += 8 + } else { + range = node.identifier.range + range[0] -= 1 + } + + return (fixer) => + shouldHaveLeadingUnderscore + ? fixer.insertTextBeforeRange(range, ' _') + : fixer.removeRange([range[0] + 1, range[0] + 1]) + } } module.exports = PrivateVarsLeadingUnderscoreChecker diff --git a/solhint.js b/solhint.js index 4b932b85..5f8d5b4a 100644 --- a/solhint.js +++ b/solhint.js @@ -4,6 +4,7 @@ const program = require('commander') const _ = require('lodash') const fs = require('fs') const process = require('process') +const readline = require('readline') const linter = require('./lib/index') const { loadConfig } = require('./lib/config/config-file') @@ -29,6 +30,7 @@ function init() { .option('--ignore-path [file_name]', 'file to use as your .solhintignore') .option('--fix', 'automatically fix problems. Skips fixes in report') .option('--fixShow', 'automatically fix problems. Show fixes in report') + .option('--noPrompt', 'do not suggest to backup files when any `fix` option is selected') .option('--init', 'create configuration file for solhint') .option('--disc', 'do not check for solhint updates') .option('--save', 'save report to file on current folder') @@ -60,15 +62,50 @@ function init() { program.parse(process.argv) } +function askUserToContinue(callback) { + const rl = readline.createInterface({ + input: process.stdin, + output: process.stdout, + }) + + rl.question( + '\nFIX option detected. Solhint will modify your files whenever it finds a fix for a rule error. Please BACKUP your contracts first. \nContinue ? (y/n) ', + (answer) => { + // Close the readline interface. + rl.close() + + // Normalize and pass the user's answer to the callback function. + const normalizedAnswer = answer.trim().toLowerCase() + callback(normalizedAnswer) + } + ) +} + function execMainAction() { - if (!program.opts().disc) { - // Call checkForUpdate and wait for it to complete using .then() - checkForUpdate().then(() => { - // This block runs after checkForUpdate is complete - executeMainActionLogic() + if ((program.opts().fix || program.opts().fixShow) && !program.opts().noPrompt) { + askUserToContinue((userAnswer) => { + if (userAnswer !== 'y') { + console.log('\nProcess terminated by user') + } else { + // User agreed, continue with the operation. + continueExecution() + } }) } else { - executeMainActionLogic() + // No need for user input, continue with the operation. + continueExecution() + } + + function continueExecution() { + if (program.opts().disc) { + executeMainActionLogic() + } else { + // Call checkForUpdate and wait for it to complete using .then() + checkForUpdate().then(() => { + // This block runs after checkForUpdate is complete + executeMainActionLogic() + }) + } } } diff --git a/test/rules/naming/private-vars-leading-underscore.js b/test/rules/naming/private-vars-leading-underscore.js index 5a8d0e51..dbf02a20 100644 --- a/test/rules/naming/private-vars-leading-underscore.js +++ b/test/rules/naming/private-vars-leading-underscore.js @@ -4,60 +4,66 @@ const { contractWith, libraryWith } = require('../../common/contract-builder') describe('Linter - private-vars-leading-underscore', () => { const SHOULD_WARN_CASES = [ - // warn when private/internal names don't start with _ + // warn when private/internal/default names don't start with _ contractWith('uint foo;'), contractWith('uint private foo;'), contractWith('uint internal foo;'), + contractWith('function foo() {}'), contractWith('function foo() private {}'), contractWith('function foo() internal {}'), + libraryWith('function foo() {}'), libraryWith('function foo() private {}'), + libraryWith('function foo() internal {}'), - // warn when public/external/default names start with _ + // warn when public/external names start with _ contractWith('uint public _foo;'), - contractWith('function _foo() {}'), + contractWith('uint external _foo;'), contractWith('function _foo() public {}'), contractWith('function _foo() external {}'), - libraryWith('function _foo() {}'), libraryWith('function _foo() public {}'), - libraryWith('function _foo() internal {}'), + libraryWith('function _foo() external {}'), ] + const SHOULD_WARN_STRICT_CASES = [ - contractWith('function foo() { uint _bar; }'), - contractWith('function foo(uint _bar) {}'), - contractWith('function foo() returns (uint256 _bar) {}'), - libraryWith('function foo() returns (uint256 _bar) {}'), - libraryWith('function foo(uint _bar) {}'), + contractWith('function _foo() internal { uint _bar; }'), + contractWith('function foo(uint _bar) external {}'), + contractWith('function foo() public returns (uint256 _bar) {}'), + libraryWith('function _foo() returns (uint256 _bar) {}'), + libraryWith('function _foo(uint _bar) private {}'), ] + const SHOULD_NOT_WARN_CASES = [ - // don't warn when private/internal names start with _ + // don't warn when private/internal/default names start with _ contractWith('uint _foo;'), contractWith('uint private _foo;'), contractWith('uint internal _foo;'), + contractWith('function _foo() {}'), contractWith('function _foo() private {}'), contractWith('function _foo() internal {}'), + libraryWith('function _foo() {}'), + libraryWith('function _foo() internal {}'), libraryWith('function _foo() private {}'), - // don't warn when public/external/default names don't start with _ + // don't warn when public/external names don't start with _ contractWith('uint public foo;'), - contractWith('function foo() {}'), + contractWith('uint public foo = 2;'), contractWith('function foo() public {}'), contractWith('function foo() external {}'), - libraryWith('function foo() {}'), libraryWith('function foo() public {}'), - libraryWith('function foo() internal {}'), + libraryWith('function foo() external {}'), // don't warn for constructors contractWith('constructor() public {}'), // other names (variables, parameters, returns) shouldn't be affected by this rule - contractWith('function foo(uint bar) {}'), - contractWith('function foo() { uint bar; }'), - contractWith('function foo() returns (uint256) {}'), - contractWith('function foo() returns (uint256 bar) {}'), - libraryWith('function foo(uint bar) {}'), - libraryWith('function foo() { uint bar; }'), - libraryWith('function foo() returns (uint256) {}'), - libraryWith('function foo() returns (uint256 bar) {}'), + contractWith('function foo(uint bar) external {}'), + contractWith('function foo() public { uint bar; }'), + contractWith('function _foo() returns (uint256) {}'), + contractWith('function _foo() returns (uint256 bar) {}'), + libraryWith('function foo(uint bar) external {}'), + libraryWith('function foo() public { uint bar; }'), + libraryWith('function _foo() returns (uint256) {}'), + libraryWith('function _foo() internal returns (uint256 bar) {}'), ] SHOULD_WARN_CASES.forEach((code, index) => { @@ -71,7 +77,7 @@ describe('Linter - private-vars-leading-underscore', () => { }) SHOULD_WARN_STRICT_CASES.concat(SHOULD_NOT_WARN_CASES).forEach((code, index) => { - it(`should not emit a warning (strict) (${index})`, () => { + it(`should not emit a warning (not strict) (${index})`, () => { const report = linter.processStr(code, { rules: { 'private-vars-leading-underscore': 'error' }, }) @@ -91,7 +97,7 @@ describe('Linter - private-vars-leading-underscore', () => { }) SHOULD_WARN_STRICT_CASES.forEach((code, index) => { - it(`should not emit a warning (strict) (${index})`, () => { + it(`should emit a warning (strict) (${index})`, () => { const report = linter.processStr(code, { rules: { 'private-vars-leading-underscore': ['error', { strict: true }] }, })