add private var autofix and backup suggestion when fixing

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
 <br><br>
 ## Configuration
 

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.                                                                                  |              | $~~~~~~~$✔️ |

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

e2e/formatters-test.js

@@ -112,7 +112,7 @@ describe('e2e', function () {
         expect(strExpected).to.equal(strOutput)
         expect(code).to.equal(0)
       })
-      it('should make the output report with unix formatter for Foo and Foo2 and Foo3', () => {
+      it('should make the output report with json formatter for Foo and Foo2 and Foo3', () => {
         const { code, stdout } = shell.exec(
           `solhint ${PATH}contracts/Foo.sol ${PATH}contracts/Foo2.sol ${PATH}contracts/Foo3.sol -f ${formatterType}`
         )

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,25 +103,26 @@ 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) {
       return
     }
 
     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,33 +137,51 @@ 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
     }
 
     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

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()
+      })
+    }
   }
 }
 

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