New Rule: non-state-vars-leading-underscore

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',
       {

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

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)

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),
   ]
 }

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