diff --git a/conf/rulesets/solhint-all.js b/conf/rulesets/solhint-all.js index 2344912c..ef674361 100644 --- a/conf/rulesets/solhint-all.js +++ b/conf/rulesets/solhint-all.js @@ -51,7 +51,8 @@ module.exports = Object.freeze({ }, ], 'modifier-name-mixedcase': 'warn', - 'named-parameters-mapping': 'warn', + 'named-parameters-mapping': 'off', + 'non-state-vars-leading-underscore': 'warn', 'private-vars-leading-underscore': [ 'warn', { diff --git a/docs/rules.md b/docs/rules.md index f1ed4375..a8876b6c 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -6,22 +6,23 @@ title: "Rule Index of Solhint" ## Best Practise Rules -| Rule Id | Error | Recommended | Deprecated | -| ------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------ | ------------ | ---------- | -| [code-complexity](./rules/best-practises/code-complexity.md) | Function has cyclomatic complexity "current" but allowed no more than maxcompl. | | | -| [explicit-types](./rules/best-practises/explicit-types.md) | Forbid or enforce explicit types (like uint256) that have an alias (like uint). | $~~~~~~~~$✔️ | | -| [function-max-lines](./rules/best-practises/function-max-lines.md) | Function body contains "count" lines but allowed no more than maxlines. | | | -| [max-line-length](./rules/best-practises/max-line-length.md) | Line length must be no more than maxlen. | | | -| [max-states-count](./rules/best-practises/max-states-count.md) | Contract has "some count" states declarations but allowed no more than maxstates. | $~~~~~~~~$✔️ | | -| [no-console](./rules/best-practises/no-console.md) | No console.log/logInt/logBytesX/logString/etc & No hardhat and forge-std console.sol import statements. | $~~~~~~~~$✔️ | | -| [no-empty-blocks](./rules/best-practises/no-empty-blocks.md) | Code block has zero statements inside. Exceptions apply. | $~~~~~~~~$✔️ | | -| [no-global-import](./rules/best-practises/no-global-import.md) | Import statement includes an entire file instead of selected symbols. | $~~~~~~~~$✔️ | | -| [no-unused-import](./rules/best-practises/no-unused-import.md) | Imported object name is not being used by the contract. | $~~~~~~~~$✔️ | | -| [no-unused-vars](./rules/best-practises/no-unused-vars.md) | Variable "name" is unused. | $~~~~~~~~$✔️ | | -| [one-contract-per-file](./rules/best-practises/one-contract-per-file.md) | Enforces the use of ONE Contract per file see [here](https://docs.soliditylang.org/en/v0.8.21/style-guide.html#contract-and-library-names) | $~~~~~~~~$✔️ | | -| [payable-fallback](./rules/best-practises/payable-fallback.md) | When fallback is not payable you will not be able to receive ethers. | $~~~~~~~~$✔️ | | -| [reason-string](./rules/best-practises/reason-string.md) | Require or revert statement must have a reason string and check that each reason string is at most N characters long. | $~~~~~~~~$✔️ | | -| [constructor-syntax](./rules/best-practises/constructor-syntax.md) | Constructors should use the new constructor keyword. | | | +| Rule Id | Error | Recommended | Deprecated | +| ---------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------ | ---------- | +| [code-complexity](./rules/best-practises/code-complexity.md) | Function has cyclomatic complexity "current" but allowed no more than maxcompl. | | | +| [explicit-types](./rules/best-practises/explicit-types.md) | Forbid or enforce explicit types (like uint256) that have an alias (like uint). | $~~~~~~~~$✔️ | | +| [function-max-lines](./rules/best-practises/function-max-lines.md) | Function body contains "count" lines but allowed no more than maxlines. | | | +| [max-line-length](./rules/best-practises/max-line-length.md) | Line length must be no more than maxlen. | | | +| [max-states-count](./rules/best-practises/max-states-count.md) | Contract has "some count" states declarations but allowed no more than maxstates. | $~~~~~~~~$✔️ | | +| [no-console](./rules/best-practises/no-console.md) | No console.log/logInt/logBytesX/logString/etc & No hardhat and forge-std console.sol import statements. | $~~~~~~~~$✔️ | | +| [no-empty-blocks](./rules/best-practises/no-empty-blocks.md) | Code block has zero statements inside. Exceptions apply. | $~~~~~~~~$✔️ | | +| [no-global-import](./rules/best-practises/no-global-import.md) | Import statement includes an entire file instead of selected symbols. | $~~~~~~~~$✔️ | | +| [no-unused-import](./rules/best-practises/no-unused-import.md) | Imported object name is not being used by the contract. | $~~~~~~~~$✔️ | | +| [no-unused-vars](./rules/best-practises/no-unused-vars.md) | Variable "name" is unused. | $~~~~~~~~$✔️ | | +| [one-contract-per-file](./rules/best-practises/one-contract-per-file.md) | Enforces the use of ONE Contract per file see [here](https://docs.soliditylang.org/en/v0.8.21/style-guide.html#contract-and-library-names) | $~~~~~~~~$✔️ | | +| [payable-fallback](./rules/best-practises/payable-fallback.md) | When fallback is not payable you will not be able to receive ethers. | $~~~~~~~~$✔️ | | +| [reason-string](./rules/best-practises/reason-string.md) | Require or revert statement must have a reason string and check that each reason string is at most N characters long. | $~~~~~~~~$✔️ | | +| [constructor-syntax](./rules/best-practises/constructor-syntax.md) | Constructors should use the new constructor keyword. | | | +| [non-state-vars-leading-underscore](./rules/naming/non-state-vars-leading-underscore.md) | Variables that are not in contract state should start with underscore. Conversely, variables that can cause an SLOAD/SSTORE should NOT start with an underscore. This makes it evident which operations cause expensive storage access when hunting for gas optimizations | | | ## Style Guide Rules diff --git a/docs/rules/naming/non-state-vars-leading-underscore.md b/docs/rules/naming/non-state-vars-leading-underscore.md new file mode 100644 index 00000000..98978ca5 --- /dev/null +++ b/docs/rules/naming/non-state-vars-leading-underscore.md @@ -0,0 +1,132 @@ +--- +warning: "This is a dynamically generated file. Do not edit manually." +layout: "default" +title: "non-state-vars-leading-underscore | Solhint" +--- + +# non-state-vars-leading-underscore +![Category Badge](https://img.shields.io/badge/-Best%20Practise%20Rules-informational) +![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow) + +## Description +Variables that are not in contract state should start with underscore. Conversely, variables that can cause an SLOAD/SSTORE should NOT start with an underscore. This makes it evident which operations cause expensive storage access when hunting for gas optimizations + +## Options +This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warn. + +### Example Config +```json +{ + "rules": { + "non-state-vars-leading-underscore": "warn" + } +} +``` + +### Notes +- event & custom error parameters and struct memer names are ignored since they do not define variables +- this rule is contradictory with private-vars-leading-underscore, only one of them should be enabled at the same time. + +## Examples +### 👍 Examples of **correct** code for this rule + +#### mutable variable should NOT start with underscore since they DO cause storage read/writes + +```solidity + + pragma solidity 0.4.4; + + + contract A { + uint256 public foo; + } + +``` + +#### immutable variable should start with underscore since they do not cause storage reads + +```solidity + + pragma solidity 0.4.4; + + + contract A { + uint256 immutable public _FOO; + } + +``` + +#### block variable with leading underscore + +```solidity + + pragma solidity 0.4.4; + + + contract A { + function foo() public { uint _myVar; } + } + +``` + +#### function parameter with leading underscore + +```solidity + + pragma solidity 0.4.4; + + + contract A { + function foo( uint256 _foo ) public {} + } + +``` + +### 👎 Examples of **incorrect** code for this rule + +#### mutable variable starting with underscore + +```solidity + + pragma solidity 0.4.4; + + + contract A { + uint256 public _foo; + } + +``` + +#### block variable without leading underscore + +```solidity + + pragma solidity 0.4.4; + + + contract A { + function foo() public { uint myVar; } + } + +``` + +#### function parameter without leading underscore + +```solidity + + pragma solidity 0.4.4; + + + contract A { + function foo( uint256 foo ) public {} + } + +``` + +## Version +This rule is introduced in the latest version. + +## Resources +- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/naming/non-state-vars-leading-underscore.js) +- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/naming/non-state-vars-leading-underscore.md) +- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/naming/non-state-vars-leading-underscore.js) diff --git a/lib/rules/naming/index.js b/lib/rules/naming/index.js index 43ead75b..03706dfd 100644 --- a/lib/rules/naming/index.js +++ b/lib/rules/naming/index.js @@ -11,6 +11,7 @@ const NamedParametersMappingChecker = require('./named-parameters-mapping') const ImmutableVarsNamingChecker = require('./immutable-vars-naming') const FunctionNamedParametersChecker = require('./func-named-parameters') const FoundryTestFunctionsChecker = require('./foundry-test-functions') +const NonStateVarsLeadingUnderscoreChecker = require('./non-state-vars-leading-underscore') module.exports = function checkers(reporter, config) { return [ @@ -27,5 +28,6 @@ module.exports = function checkers(reporter, config) { new ImmutableVarsNamingChecker(reporter, config), new FunctionNamedParametersChecker(reporter, config), new FoundryTestFunctionsChecker(reporter, config), + new NonStateVarsLeadingUnderscoreChecker(reporter), ] } diff --git a/lib/rules/naming/non-state-vars-leading-underscore.js b/lib/rules/naming/non-state-vars-leading-underscore.js new file mode 100644 index 00000000..a0482393 --- /dev/null +++ b/lib/rules/naming/non-state-vars-leading-underscore.js @@ -0,0 +1,132 @@ +const BaseChecker = require('../base-checker') +const { contractWith } = require('../../../test/common/contract-builder') +const { hasLeadingUnderscore } = require('../../common/identifier-naming') +const { severityDescription } = require('../../doc/utils') + +const DEFAULT_SEVERITY = 'warn' + +const ruleId = 'non-state-vars-leading-underscore' +const meta = { + type: 'naming', + docs: { + description: + 'Variables that are not in contract state should start with underscore. Conversely, variables that can cause an SLOAD/SSTORE should NOT start with an underscore. This makes it evident which operations cause expensive storage access when hunting for gas optimizations', + category: 'Best Practise Rules', + examples: { + good: [ + { + description: + 'mutable variable should NOT start with underscore since they DO cause storage read/writes', + code: contractWith('uint256 public foo;'), + }, + { + description: + 'immutable variable should start with underscore since they do not cause storage reads', + code: contractWith('uint256 immutable public _FOO;'), + }, + { + description: 'block variable with leading underscore', + code: contractWith('function foo() public { uint _myVar; }'), + }, + { + description: 'function parameter with leading underscore', + code: contractWith('function foo( uint256 _foo ) public {}'), + }, + ], + bad: [ + { + description: 'mutable variable starting with underscore', + code: contractWith('uint256 public _foo;'), + }, + { + description: 'block variable without leading underscore', + code: contractWith('function foo() public { uint myVar; }'), + }, + { + description: 'function parameter without leading underscore', + code: contractWith('function foo( uint256 foo ) public {}'), + }, + ], + }, + notes: [ + { + note: 'event & custom error parameters and struct memer names are ignored since they do not define variables', + }, + { + note: 'this rule is contradictory with private-vars-leading-underscore, only one of them should be enabled at the same time.', + }, + ], + options: [ + { + description: severityDescription, + default: DEFAULT_SEVERITY, + }, + ], + }, + isDefault: false, + recommended: false, + schema: null, + defaultSetup: DEFAULT_SEVERITY, +} + +class NonStateVarsLeadingUnderscoreChecker extends BaseChecker { + constructor(reporter) { + super(reporter, ruleId, meta) + this.definingSubName = false + } + + FileLevelConstant(node) { + this.validateName(node, true) + } + + StructDefinition() { + this.definingSubName = true + } + + 'StructDefinition:exit'() { + this.definingSubName = false + } + + EventDefinition() { + this.definingSubName = true + } + + 'EventDefinition:exit'() { + this.definingSubName = false + } + + CustomErrorDefinition() { + this.definingSubName = true + } + + 'CustomErrorDefinition:exit'() { + this.definingSubName = false + } + + VariableDeclaration(node) { + if (this.definingSubName) return + if (node.isStateVar) { + this.validateName(node, node.isDeclaredConst || node.isImmutable) + } else { + this.validateName(node, true) + } + } + + validateName(node, shouldHaveLeadingUnderscore) { + if (node.name === null) { + return + } + + if (hasLeadingUnderscore(node.name) !== shouldHaveLeadingUnderscore) { + this._error(node, node.name, shouldHaveLeadingUnderscore) + } + } + + _error(node, name, shouldHaveLeadingUnderscore) { + this.error( + node, + `'${name}' ${shouldHaveLeadingUnderscore ? 'should' : 'should not'} start with _` + ) + } +} +module.exports = NonStateVarsLeadingUnderscoreChecker diff --git a/test/rules/naming/non-state-vars-leading-underscore.js b/test/rules/naming/non-state-vars-leading-underscore.js new file mode 100644 index 00000000..64603ed3 --- /dev/null +++ b/test/rules/naming/non-state-vars-leading-underscore.js @@ -0,0 +1,111 @@ +const assert = require('assert') +const { processStr } = require('../../../lib/index') +const { contractWith } = require('../../common/contract-builder') + +const config = { + rules: { 'non-state-vars-leading-underscore': 'error' }, +} + +describe('non-state-vars-leading-underscore', () => { + ;[ + { + description: 'struct member', + underscore: contractWith('struct Foo { uint256 _foo; }'), + noUnderscore: contractWith('struct Foo { uint256 _foo; }'), + }, + { + description: 'event parameter', + underscore: contractWith('event Foo ( uint256 _foo );'), + noUnderscore: contractWith('event Foo ( uint256 foo );'), + }, + { + description: 'custom error parameter', + underscore: contractWith('error Foo ( uint256 _foo );'), + noUnderscore: contractWith('error Foo ( uint256 foo );'), + }, + ].forEach(({ description, underscore, noUnderscore }) => { + describe(`${description} names should be ignored since they just define a type`, function () { + it('they can start with underscore', function () { + const report = processStr(underscore, config) + assert.equal(report.errorCount, 0) + }) + it('they can start without underscore', function () { + const report = processStr(noUnderscore, config) + assert.equal(report.errorCount, 0) + }) + }) + }) + describe("'state' vars that dont actually use storage should start with underscore", function () { + ;[ + { + description: 'immutable variable', + underscore: contractWith('uint256 immutable public _FOO;'), + noUnderscore: contractWith('uint256 immutable public FOO;'), + }, + { + description: 'constant variable', + underscore: contractWith('uint256 constant public _FOO;'), + noUnderscore: contractWith('uint256 constant public FOO;'), + }, + ].forEach(({ description, underscore, noUnderscore }) => { + it(`should raise an error if a ${description} does not start with an underscore`, () => { + const report = processStr(noUnderscore, config) + assert.equal(report.errorCount, 1) + assert(report.messages[0].message.includes('should start with _')) + }) + + it(`should NOT raise an error if a ${description} starts with an underscore`, () => { + const report = processStr(underscore, config) + assert.equal(report.errorCount, 0) + }) + }) + }) + describe('non state vars should have a leading underscore', function () { + ;[ + { + description: 'block variable', + underscore: contractWith('function foo() public { uint _myVar; }'), + noUnderscore: contractWith('function foo() public { uint myVar; }'), + }, + { + description: 'return variable', + underscore: contractWith('function foo() public returns (uint256 _foo){}'), + noUnderscore: contractWith('function foo() public returns (uint256 foo){}'), + }, + { + description: 'function param', + underscore: contractWith('function foo( uint256 _foo ) public {}'), + noUnderscore: contractWith('function foo( uint256 foo ) public {}'), + }, + { + description: 'file-level constant', + underscore: 'uint256 constant _IMPORTANT_VALUE = 420;', + noUnderscore: 'uint256 constant IMPORTANT_VALUE = 420;', + }, + ].forEach(({ description, underscore, noUnderscore }) => { + it(`should raise an error if a ${description} does not start with an underscore`, () => { + const report = processStr(noUnderscore, config) + assert.equal(report.errorCount, 1) + assert(report.messages[0].message.includes('should start with _')) + }) + + it(`should NOT raise an error if a ${description} starts with an underscore`, () => { + const report = processStr(underscore, config) + assert.equal(report.errorCount, 0) + }) + }) + }) + + describe('state-using state vars should NOT have a leading underscore', function () { + it(`should raise an error if a mutablestate variable starts with an underscore`, () => { + const report = processStr(contractWith('uint256 _foo;'), config) + assert.equal(report.errorCount, 1) + assert(report.messages[0].message.includes('should not start with _')) + }) + + it(`should NOT raise an error if a mutable state variable doesnt start with an underscore`, () => { + const report = processStr(contractWith('uint256 foo;'), config) + assert.equal(report.errorCount, 0) + }) + }) +})