From eefdd2d7c217b1626fdb6a10161bc881f105bd47 Mon Sep 17 00:00:00 2001 From: dbale-altoros Date: Fri, 8 Mar 2024 17:33:54 -0300 Subject: [PATCH] add: non-state-vars-leading-underscore --- conf/rulesets/solhint-all.js | 1 + docs/rules.md | 7 + .../non-state-vars-leading-underscore.md | 137 ++++++++++++++++++ lib/rules/naming/index.js | 2 + .../non-state-vars-leading-underscore.js | 132 +++++++++++++++++ .../gas-named-return-values.js | 7 +- .../non-state-vars-leading-underscore.js | 111 ++++++++++++++ 7 files changed, 396 insertions(+), 1 deletion(-) create mode 100644 docs/rules/naming/non-state-vars-leading-underscore.md create mode 100644 lib/rules/naming/non-state-vars-leading-underscore.js create mode 100644 test/rules/naming/non-state-vars-leading-underscore.js diff --git a/conf/rulesets/solhint-all.js b/conf/rulesets/solhint-all.js index de747a7c..1a68867a 100644 --- a/conf/rulesets/solhint-all.js +++ b/conf/rulesets/solhint-all.js @@ -49,6 +49,7 @@ module.exports = Object.freeze({ ], 'modifier-name-mixedcase': '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 41b6af80..fb924553 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -69,6 +69,13 @@ title: "Rule Index of Solhint" | [visibility-modifier-order](./rules/order/visibility-modifier-order.md) | Visibility modifier must be first in list of modifiers. | $~~~~~~~~$✔️ | | +## Best Practice Rules + +| Rule Id | Error | Recommended | Deprecated | +| ---------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------- | ---------- | +| [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 | | | + + ## Security Rules | Rule Id | Error | Recommended | Deprecated | 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..05943232 --- /dev/null +++ b/docs/rules/naming/non-state-vars-leading-underscore.md @@ -0,0 +1,137 @@ +--- +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%20Practice%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 an array of options: + +| Index | Description | Default Value | +| ----- | ----------------------------------------------------- | ------------- | +| 0 | Rule severity. Must be one of "error", "warn", "off". | 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..72a485e0 --- /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 Practice 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/gas-consumption/gas-named-return-values.js b/test/rules/gas-consumption/gas-named-return-values.js index ccd2e69a..3aa73ff0 100644 --- a/test/rules/gas-consumption/gas-named-return-values.js +++ b/test/rules/gas-consumption/gas-named-return-values.js @@ -84,7 +84,12 @@ describe('Linter - gas-named-return-values', () => { const report = linter.processStr(code, { extends: 'solhint:all', - rules: { 'compiler-version': 'off' }, + rules: { + 'compiler-version': 'off', + 'comprehensive-interface': 'off', + 'foundry-test-functions': 'off', + 'non-state-vars-leading-underscore': 'off', + }, }) assertWarnsCount(report, 2) 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) + }) + }) +})