-
Notifications
You must be signed in to change notification settings - Fork 12k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add custom linting rules #4132
Add custom linting rules #4132
Changes from 8 commits
6c08f86
7e1fe6d
b3e6d79
61194ec
9c7dc19
cf8a1b1
8338d2a
2e54ac2
276dbae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
const path = require('path'); | ||
const minimatch = require('minimatch'); | ||
|
||
// Files matching these patterns will be ignored unless a rule has `static global = true` | ||
const ignore = ['contracts/mocks/**/*', 'test/**/*']; | ||
|
||
class Base { | ||
constructor(reporter, config, source, fileName) { | ||
this.reporter = reporter; | ||
this.ignored = this.constructor.global || ignore.some(p => minimatch(path.normalize(fileName), p)); | ||
this.ruleId = this.constructor.ruleId; | ||
if (this.ruleId === undefined) { | ||
throw Error('missing ruleId static property'); | ||
} | ||
} | ||
|
||
error(node, message) { | ||
if (!this.ignored) { | ||
this.reporter.error(node, this.ruleId, message); | ||
} | ||
} | ||
} | ||
|
||
module.exports = [ | ||
class extends Base { | ||
static ruleId = 'interface-names'; | ||
|
||
ContractDefinition(node) { | ||
if (node.kind === 'interface' && !/^I[A-Z]/.test(node.name)) { | ||
this.error(node, 'Interface names should have a capital I prefix'); | ||
} | ||
} | ||
}, | ||
|
||
class extends Base { | ||
static ruleId = 'private-variables'; | ||
|
||
VariableDeclaration(node) { | ||
const constantOrImmutable = node.isDeclaredConst || node.isImmutable; | ||
if (node.isStateVar && !constantOrImmutable && node.visibility !== 'private') { | ||
this.error(node, 'State variables must be private'); | ||
} | ||
} | ||
}, | ||
|
||
class extends Base { | ||
static ruleId = 'underscore-prefix'; | ||
|
||
VariableDeclaration(node) { | ||
const constantOrImmutable = node.isDeclaredConst || node.isImmutable; | ||
if (node.isStateVar && !constantOrImmutable && node.visibility === 'private') { | ||
if (!/^_/.test(node.name)) { | ||
this.error(node, 'Private variables must be prefixed with underscore'); | ||
} | ||
} | ||
} | ||
|
||
FunctionDefinition(node) { | ||
if (node.visibility === 'private' || (node.visibility === 'internal' && node.parent.kind !== 'library')) { | ||
if (!/^_/.test(node.name)) { | ||
this.error(node, 'Private and internal functions must be prefixed with underscore'); | ||
} | ||
} | ||
if (node.visibility === 'internal' && node.parent.kind === 'library') { | ||
if (/^_/.test(node.name)) { | ||
this.error(node, 'Library internal functions should not be prefixed with underscore'); | ||
} | ||
} | ||
} | ||
}, | ||
|
||
// TODO: re-enable and fix | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have some code that causes this rule to fail so I'm proposing to merge and enable it later. |
||
// class extends Base { | ||
// static ruleId = 'no-external-virtual'; | ||
// | ||
// FunctionDefinition(node) { | ||
// if (node.visibility == 'external' && node.isVirtual) { | ||
// this.error(node, 'Functions should not be external and virtual'); | ||
// } | ||
// } | ||
// }, | ||
]; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"name": "solhint-plugin-openzeppelin", | ||
"private": true | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
const customRules = require('./scripts/solhint-custom'); | ||
|
||
const rules = [ | ||
'no-unused-vars', | ||
'const-name-snakecase', | ||
'contract-name-camelcase', | ||
'event-name-camelcase', | ||
'func-name-mixedcase', | ||
'func-param-name-mixedcase', | ||
'modifier-name-mixedcase', | ||
'var-name-mixedcase', | ||
'imports-on-top', | ||
'no-global-import', | ||
...customRules.map(r => `openzeppelin/${r.ruleId}`), | ||
]; | ||
|
||
module.exports = { | ||
plugins: ['openzeppelin'], | ||
rules: Object.fromEntries(rules.map(r => [r, 'error'])), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not define it like an object?
Not a blocker because everything shows an "error", I just wanted to point out that the array doesn't seem to be needed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to use customRules.map(r => `openzeppelin/${r.ruleId}`) The rest could be an object, but I had a feeling that this was clearer? Let me know if it's too confusing. |
||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also have some rule that if the stateVar is constantOrImmutable, then it must NOT start with an underscore ? or do we accept both ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but this causes the rule to fail in a few places.
I added the rule but commented it out, and I'd propose to do another PR next week fixing those things. Does this sound ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we will have 2 follow up PRs that :
?
That sounds ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah or just one PR doing the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the rule should also check that the name is capitalized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be already handled by the casing rules in
solhint.config.js
, specificallyconst-name-snakecase
.