Skip to content
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

Merged
merged 9 commits into from
Jul 10, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions .solhint.json

This file was deleted.

18 changes: 18 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
"rimraf": "^3.0.2",
"semver": "^7.3.5",
"solhint": "^3.3.6",
"solhint-plugin-openzeppelin": "file:scripts/solhint-custom",
"solidity-ast": "^0.4.25",
"solidity-coverage": "^0.8.0",
"solidity-docgen": "^0.6.0-beta.29",
Expand Down
82 changes: 82 additions & 0 deletions scripts/solhint-custom/index.js
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');
}
}
Copy link
Collaborator

@Amxx Amxx Jul 6, 2023

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 ?

Suggested change
}
} else if (node.isStateVar && constantOrImmutable) {
if (/^_/.test(node.name)) {
this.error(node, 'Constant and immutable variables should not be prefixed with underscore');
}
}

Copy link
Contributor

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?

Copy link
Collaborator

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 :

  • enable the rule
  • fix the code
    ?

That sounds ok

Copy link
Contributor

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.

Copy link
Collaborator

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.

Copy link
Contributor

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, specifically const-name-snakecase.

}

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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');
// }
// }
// },
];
4 changes: 4 additions & 0 deletions scripts/solhint-custom/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "solhint-plugin-openzeppelin",
"private": true
}
20 changes: 20 additions & 0 deletions solhint.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
const customRules = require('./scripts/solhint-custom').map(r => r.ruleId);

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

module.exports = {
plugins: ['openzeppelin'],
rules: Object.fromEntries(rules.map(r => [r, 'error'])),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not define it like an object?

const rules = {
  'no-unused-vars': 'error',
  ...
}

Not a blocker because everything shows an "error", I just wanted to point out that the array doesn't seem to be needed

Copy link
Contributor

@frangio frangio Jul 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to use fromEntries in some way because we get the names of the custom rules dynamically:

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.

};