Autofix private-vars-leading-underscore and Backup prompt 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/06-formatters/helpers/helpers.js

@@ -32,7 +32,6 @@ const foo1Output = [
     severity: 'Warning',
     message: "'TEST2' should start with _",
     ruleId: 'private-vars-leading-underscore',
-    fix: null,
     filePath: 'contracts/Foo.sol',
   },
   {

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/best-practises/no-console.js

@@ -28,6 +28,7 @@ const meta = {
   isDefault: true,
   recommended: true,
   defaultSetup: 'error',
+  fixable: true,
   schema: null,
 }
 
@@ -38,13 +39,13 @@ class NoConsoleLogChecker extends BaseChecker {
 
   ImportDirective(node) {
     if (this.isConsoleImport(node)) {
-      this.error(node, 'Unexpected import of console file')
+      this.error(node, 'Unexpected import of console file', this.fixStatement(node, 'import'))
     }
   }
 
   FunctionCall(node) {
     if (this.isConsoleLog(node)) {
-      this.error(node, 'Unexpected console statement')
+      this.error(node, 'Unexpected console statement', this.fixStatement(node, 'call'))
     }
   }
 
@@ -64,6 +65,18 @@ class NoConsoleLogChecker extends BaseChecker {
       node.path === 'forge-std/console2.sol'
     )
   }
+
+  fixStatement(node, type) {
+    const range = node.range
+    // to remove the ';'
+    if (type === 'call') range[1] += 1
+
+    // // to remove the ';' and the 'enter'
+    // if (type === 'call') range[1] += 2
+    // else range[1] += 1 // to remove the 'enter'
+
+    return (fixer) => fixer.removeRange(range)
+  }
 }
 
 module.exports = NoConsoleLogChecker

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/best-practises/no-console.js

@@ -35,7 +35,7 @@ describe('Linter - no-console', () => {
 
   it('should raise console.logBytes12() is not allowed', () => {
     const code = funcWith(`
-      console.logString('test');
+      console.logBytes12('test');
     `)
 
     const report = linter.processStr(code, {