Skip to content

Commit 5e881bd

Browse files
dbale-altorosdbale-altoros
authored
Merge pull request #511 from protofire/autofix-private-var-underscore
Autofix private-vars-leading-underscore and Backup prompt when fixing
2 parents 23833d5 + 1242e6d commit 5e881bd

File tree

10 files changed

+150
-62
lines changed

10 files changed

+150
-62
lines changed

‎README.md

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ Options:
6464
--ignore-path [file_name] file to use as your .solhintignore
6565
--fix automatically fix problems. Skip report
6666
--fixShow automatically fix problems. Show report
67+
--noPrompt do not suggest to backup files when any `fix` option is selected
6768
--init create configuration file for solhint
6869
--disc do not check for solhint updates
6970
--save save report to file on current folder
@@ -76,8 +77,15 @@ Commands:
7677
```
7778
### Notes
7879
- Solhint checks if there are newer versions. The `--disc` option avoids that check.
79-
- `--fix` option currently works only on "avoid-throw" and "avoid-sha3" rules.
8080
- `--save` option will create a file named as `YYYYMMDDHHMMSS_solhintReport.txt` on current folder with default or specified format
81+
82+
### Fix
83+
This option currently works on:
84+
- avoid-throw
85+
- avoid-sha3
86+
- no-console
87+
- explicit-types
88+
- private-vars-underscore
8189
<br><br>
8290
## Configuration
8391

‎docs/rules.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ title: "Rule Index of Solhint"
4848
| [modifier-name-mixedcase](./rules/naming/modifier-name-mixedcase.md) | Modifier name must be in mixedCase. | | |
4949
| [named-parameters-mapping](./rules/naming/named-parameters-mapping.md) | Solidity v0.8.18 introduced named parameters on the mappings definition. | | |
5050
| [named-return-values](./rules/naming/named-return-values.md) | Enforce the return values of a function to be named | | |
51-
| [private-vars-leading-underscore](./rules/naming/private-vars-leading-underscore.md) | Private and internal names must start with a single underscore. | | |
51+
| [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 | | |
5252
| [use-forbidden-name](./rules/naming/use-forbidden-name.md) | Avoid to use letters 'I', 'l', 'O' as identifiers. | $~~~~~~~~$✔️ | |
5353
| [var-name-mixedcase](./rules/naming/var-name-mixedcase.md) | Variable name must be in mixedCase. (Does not check IMMUTABLES, use immutable-vars-naming) | $~~~~~~~~$✔️ | |
5454
| [func-order](./rules/order/func-order.md) | Function order is incorrect. | | $~~~~~~~$✔️ |

‎docs/rules/naming/private-vars-leading-underscore.md

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,15 @@ title: "private-vars-leading-underscore | Solhint"
99
![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow)
1010

1111
## Description
12-
Private and internal names must start with a single underscore.
12+
Non-external functions and state variables should start with a single underscore. Others, shouldn't.
1313

1414
## Options
1515
This rule accepts an array of options:
1616

17-
| Index | Description | Default Value |
18-
| ----- | ------------------------------------------------------------------------------------------------------------------------------------- | ---------------- |
19-
| 0 | Rule severity. Must be one of "error", "warn", "off". | warn |
20-
| 1 | A JSON object with a single property "strict" specifying if the rule should apply to non state variables. Default: { strict: false }. | {"strict":false} |
17+
| Index | Description | Default Value |
18+
| ----- | ----------------------------------------------------------------------------------------------------------------------------------------- | ---------------- |
19+
| 0 | Rule severity. Must be one of "error", "warn", "off". | warn |
20+
| 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} |
2121

2222

2323
### Example Config
@@ -30,6 +30,7 @@ This rule accepts an array of options:
3030
```
3131

3232
### Notes
33+
- This rule considers functions and variables in Libraries as well
3334
- This rule skips external and public functions
3435
- This rule skips external and public state variables
3536
- 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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ const foo1Output = [
3232
severity: 'Warning',
3333
message: "'TEST2' should start with _",
3434
ruleId: 'private-vars-leading-underscore',
35-
fix: null,
3635
filePath: 'contracts/Foo.sol',
3736
},
3837
{

‎e2e/formatters-test.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ describe('e2e', function () {
112112
expect(strExpected).to.equal(strOutput)
113113
expect(code).to.equal(0)
114114
})
115-
it('should make the output report with unix formatter for Foo and Foo2 and Foo3', () => {
115+
it('should make the output report with json formatter for Foo and Foo2 and Foo3', () => {
116116
const { code, stdout } = shell.exec(
117117
`solhint ${PATH}contracts/Foo.sol ${PATH}contracts/Foo2.sol ${PATH}contracts/Foo3.sol -f ${formatterType}`
118118
)

‎lib/rules/best-practises/no-console.js

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ const meta = {
2828
isDefault: true,
2929
recommended: true,
3030
defaultSetup: 'error',
31+
fixable: true,
3132
schema: null,
3233
}
3334

@@ -38,13 +39,13 @@ class NoConsoleLogChecker extends BaseChecker {
3839

3940
ImportDirective(node) {
4041
if (this.isConsoleImport(node)) {
41-
this.error(node, 'Unexpected import of console file')
42+
this.error(node, 'Unexpected import of console file', this.fixStatement(node, 'import'))
4243
}
4344
}
4445

4546
FunctionCall(node) {
4647
if (this.isConsoleLog(node)) {
47-
this.error(node, 'Unexpected console statement')
48+
this.error(node, 'Unexpected console statement', this.fixStatement(node, 'call'))
4849
}
4950
}
5051

@@ -64,6 +65,18 @@ class NoConsoleLogChecker extends BaseChecker {
6465
node.path === 'forge-std/console2.sol'
6566
)
6667
}
68+
69+
fixStatement(node, type) {
70+
const range = node.range
71+
// to remove the ';'
72+
if (type === 'call') range[1] += 1
73+
74+
// // to remove the ';' and the 'enter'
75+
// if (type === 'call') range[1] += 2
76+
// else range[1] += 1 // to remove the 'enter'
77+
78+
return (fixer) => fixer.removeRange(range)
79+
}
6780
}
6881

6982
module.exports = NoConsoleLogChecker

‎lib/rules/naming/private-vars-leading-underscore.js

Lines changed: 43 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,8 @@ const meta = {
1111
type: 'naming',
1212

1313
docs: {
14-
description: 'Private and internal names must start with a single underscore.',
14+
description:
15+
"Non-external functions and state variables should start with a single underscore. Others, shouldn't",
1516
category: 'Style Guide Rules',
1617
options: [
1718
{
@@ -20,7 +21,7 @@ const meta = {
2021
},
2122
{
2223
description:
23-
'A JSON object with a single property "strict" specifying if the rule should apply to non state variables. Default: { strict: false }.',
24+
'A JSON object with a single property "strict" specifying if the rule should apply to ALL non state variables. Default: { strict: false }.',
2425
default: JSON.stringify(DEFAULT_OPTION),
2526
},
2627
],
@@ -65,6 +66,9 @@ const meta = {
6566
],
6667
},
6768
notes: [
69+
{
70+
note: 'This rule considers functions and variables in Libraries as well',
71+
},
6872
{
6973
note: 'This rule skips external and public functions',
7074
},
@@ -80,6 +84,7 @@ const meta = {
8084
isDefault: false,
8185
recommended: false,
8286
defaultSetup: [DEFAULT_SEVERITY, DEFAULT_OPTION],
87+
fixable: true,
8388

8489
schema: {
8590
type: 'object',
@@ -98,25 +103,26 @@ class PrivateVarsLeadingUnderscoreChecker extends BaseChecker {
98103
this.isStrict = config && config.getObjectPropertyBoolean(ruleId, 'strict', DEFAULT_STRICTNESS)
99104
}
100105

101-
ContractDefinition(node) {
102-
if (node.kind === 'library') {
103-
this.inLibrary = true
104-
}
105-
}
106+
// ContractDefinition(node) {
107+
// if (node.kind === 'library') {
108+
// this.inLibrary = true
109+
// }
110+
// }
106111

107-
'ContractDefinition:exit'() {
108-
this.inLibrary = false
109-
}
112+
// 'ContractDefinition:exit'() {
113+
// this.inLibrary = false
114+
// }
110115

111116
FunctionDefinition(node) {
112117
if (!node.name) {
113118
return
114119
}
115120

116121
const isPrivate = node.visibility === 'private'
117-
const isInternal = node.visibility === 'internal'
118-
const shouldHaveLeadingUnderscore = isPrivate || (!this.inLibrary && isInternal)
119-
this.validateName(node, shouldHaveLeadingUnderscore)
122+
const isInternal = node.visibility === 'internal' || node.visibility === 'default'
123+
// const shouldHaveLeadingUnderscore = isPrivate || (!this.inLibrary && isInternal)
124+
const shouldHaveLeadingUnderscore = isPrivate || isInternal
125+
this.validateName(node, shouldHaveLeadingUnderscore, 'function')
120126
}
121127

122128
StateVariableDeclaration() {
@@ -131,33 +137,51 @@ class PrivateVarsLeadingUnderscoreChecker extends BaseChecker {
131137
if (!this.inStateVariableDeclaration) {
132138
// if strict is enabled, non-state vars should not start with leading underscore
133139
if (this.isStrict) {
134-
this.validateName(node, false)
140+
this.validateName(node, false, 'variable')
135141
}
136142
return
137143
}
138144

139145
const isPrivate = node.visibility === 'private'
140146
const isInternal = node.visibility === 'internal' || node.visibility === 'default'
141147
const shouldHaveLeadingUnderscore = isPrivate || isInternal
142-
this.validateName(node, shouldHaveLeadingUnderscore)
148+
this.validateName(node, shouldHaveLeadingUnderscore, 'variable')
143149
}
144150

145-
validateName(node, shouldHaveLeadingUnderscore) {
151+
validateName(node, shouldHaveLeadingUnderscore, type) {
146152
if (node.name === null) {
147153
return
148154
}
149155

150156
if (naming.hasLeadingUnderscore(node.name) !== shouldHaveLeadingUnderscore) {
151-
this._error(node, node.name, shouldHaveLeadingUnderscore)
157+
this._error(node, node.name, shouldHaveLeadingUnderscore, type)
152158
}
153159
}
154160

155-
_error(node, name, shouldHaveLeadingUnderscore) {
161+
_error(node, name, shouldHaveLeadingUnderscore, type) {
156162
this.error(
157163
node,
158-
`'${name}' ${shouldHaveLeadingUnderscore ? 'should' : 'should not'} start with _`
164+
`'${name}' ${shouldHaveLeadingUnderscore ? 'should' : 'should not'} start with _`,
165+
this.fixStatement(node, shouldHaveLeadingUnderscore, type)
159166
)
160167
}
168+
169+
fixStatement(node, shouldHaveLeadingUnderscore, type) {
170+
let range
171+
172+
if (type === 'function') {
173+
range = node.range
174+
range[0] += 8
175+
} else {
176+
range = node.identifier.range
177+
range[0] -= 1
178+
}
179+
180+
return (fixer) =>
181+
shouldHaveLeadingUnderscore
182+
? fixer.insertTextBeforeRange(range, ' _')
183+
: fixer.removeRange([range[0] + 1, range[0] + 1])
184+
}
161185
}
162186

163187
module.exports = PrivateVarsLeadingUnderscoreChecker

‎solhint.js

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ const program = require('commander')
44
const _ = require('lodash')
55
const fs = require('fs')
66
const process = require('process')
7+
const readline = require('readline')
78

89
const linter = require('./lib/index')
910
const { loadConfig } = require('./lib/config/config-file')
@@ -29,6 +30,7 @@ function init() {
2930
.option('--ignore-path [file_name]', 'file to use as your .solhintignore')
3031
.option('--fix', 'automatically fix problems. Skips fixes in report')
3132
.option('--fixShow', 'automatically fix problems. Show fixes in report')
33+
.option('--noPrompt', 'do not suggest to backup files when any `fix` option is selected')
3234
.option('--init', 'create configuration file for solhint')
3335
.option('--disc', 'do not check for solhint updates')
3436
.option('--save', 'save report to file on current folder')
@@ -60,15 +62,50 @@ function init() {
6062
program.parse(process.argv)
6163
}
6264

65+
function askUserToContinue(callback) {
66+
const rl = readline.createInterface({
67+
input: process.stdin,
68+
output: process.stdout,
69+
})
70+
71+
rl.question(
72+
'\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) ',
73+
(answer) => {
74+
// Close the readline interface.
75+
rl.close()
76+
77+
// Normalize and pass the user's answer to the callback function.
78+
const normalizedAnswer = answer.trim().toLowerCase()
79+
callback(normalizedAnswer)
80+
}
81+
)
82+
}
83+
6384
function execMainAction() {
64-
if (!program.opts().disc) {
65-
// Call checkForUpdate and wait for it to complete using .then()
66-
checkForUpdate().then(() => {
67-
// This block runs after checkForUpdate is complete
68-
executeMainActionLogic()
85+
if ((program.opts().fix || program.opts().fixShow) && !program.opts().noPrompt) {
86+
askUserToContinue((userAnswer) => {
87+
if (userAnswer !== 'y') {
88+
console.log('\nProcess terminated by user')
89+
} else {
90+
// User agreed, continue with the operation.
91+
continueExecution()
92+
}
6993
})
7094
} else {
71-
executeMainActionLogic()
95+
// No need for user input, continue with the operation.
96+
continueExecution()
97+
}
98+
99+
function continueExecution() {
100+
if (program.opts().disc) {
101+
executeMainActionLogic()
102+
} else {
103+
// Call checkForUpdate and wait for it to complete using .then()
104+
checkForUpdate().then(() => {
105+
// This block runs after checkForUpdate is complete
106+
executeMainActionLogic()
107+
})
108+
}
72109
}
73110
}
74111

‎test/rules/best-practises/no-console.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ describe('Linter - no-console', () => {
3535

3636
it('should raise console.logBytes12() is not allowed', () => {
3737
const code = funcWith(`
38-
console.logString('test');
38+
console.logBytes12('test');
3939
`)
4040

4141
const report = linter.processStr(code, {

0 commit comments

Comments
 (0)