add: non-state-vars-leading-underscore

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

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  |

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)

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 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

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)