diff --git a/.prettierrc b/.prettierrc index 75a894a2..c8e7be8d 100644 --- a/.prettierrc +++ b/.prettierrc @@ -1,5 +1,6 @@ { "semi": false, "singleQuote": true, - "printWidth": 100 + "printWidth": 100, + "bracketSpacing": true } diff --git a/CHANGELOG.md b/CHANGELOG.md index ce343433..8e91fe0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,13 @@ +## [5.0.2] - 2024-07-25 +### Fixed +- `func-named-parameters` exclude abi.encodeX from the rule [#583](https://github.com/protofire/solhint/pull/583) (Thanks to [@0xCLARITY](https://github.com/0xCLARITY)) +- Several typos in comments [#586](https://github.com/protofire/solhint/pull/586) (Thanks to [@dropbigfish](https://github.com/dropbigfish)) + +### Added +- New Rule: Imports order [#587](https://github.com/protofire/solhint/pull/587) + +

+ ## [5.0.1] - 2024-05-13 ### BREAKING CHANGES (refer to v5.0.0) Fixed an issue on the returining values where only was evaluating the first report instead of all of them. diff --git a/conf/rulesets/solhint-all.js b/conf/rulesets/solhint-all.js index b10c1e1a..06b9fc5a 100644 --- a/conf/rulesets/solhint-all.js +++ b/conf/rulesets/solhint-all.js @@ -50,6 +50,7 @@ module.exports = Object.freeze({ immutablesAsConstants: true, }, ], + 'imports-order': 'warn', 'modifier-name-mixedcase': 'warn', 'named-parameters-mapping': 'warn', 'private-vars-leading-underscore': [ diff --git a/docker/Dockerfile b/docker/Dockerfile index e5d2f8c0..e7af3f49 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -1,5 +1,5 @@ FROM node:20-alpine LABEL maintainer="diego.bale@protofire.io" -ENV VERSION=5.0.1 +ENV VERSION=5.0.2 RUN npm install -g solhint@"$VERSION" \ No newline at end of file diff --git a/docs/rules.md b/docs/rules.md index 61bc0c8b..8c644b5f 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -37,6 +37,7 @@ title: "Rule Index of Solhint" | [func-named-parameters](./rules/naming/func-named-parameters.md) | Enforce named parameters for function calls with 4 or more arguments. This rule may have some false positives | | | | [func-param-name-mixedcase](./rules/naming/func-param-name-mixedcase.md) | Function param name must be in mixedCase. | | | | [immutable-vars-naming](./rules/naming/immutable-vars-naming.md) | Check Immutable variables. Capitalized SNAKE_CASE or mixedCase depending on configuration. | $~~~~~~~~$✔️ | | +| [imports-order](./rules/naming/imports-order.md) | Order the imports of the contract to follow a certain hierarchy (read "Notes section") | | | | [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. | | | | [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 | | | diff --git a/docs/rules/naming/imports-order.md b/docs/rules/naming/imports-order.md new file mode 100644 index 00000000..b5322754 --- /dev/null +++ b/docs/rules/naming/imports-order.md @@ -0,0 +1,42 @@ +--- +warning: "This is a dynamically generated file. Do not edit manually." +layout: "default" +title: "imports-order | Solhint" +--- + +# imports-order +![Category Badge](https://img.shields.io/badge/-Style%20Guide%20Rules-informational) +![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow) + +## Description +Order the imports of the contract to follow a certain hierarchy (read "Notes section") + +## Options +This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warn. + +### Example Config +```json +{ + "rules": { + "imports-order": "warn" + } +} +``` + +### Notes +- Paths starting with "@" like "@openzeppelin/" and urls ("http" and "https") will go first +- Order by hierarchy of directories first, e.g. ./../../ comes before ./../, which comes before ./, which comes before ./foo +- Order alphabetically for each path at the same level, e.g. ./contract/Zbar.sol comes before ./interface/Ifoo.sol +- Rule does NOT support this kind of import "import * as Alias from "./filename.sol" +- When "--fix", rule will re-write this notation "../folder/file.sol" or this one "../file.sol" to "./../folder/file.sol" or this one "./../file.sol" + +## Examples +This rule does not have examples. + +## Version +This rule is introduced in the latest version. + +## Resources +- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/naming/imports-order.js) +- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/naming/imports-order.md) +- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/naming/imports-order.js) diff --git a/docs/rules/naming/named-parameters-mapping.md b/docs/rules/naming/named-parameters-mapping.md index 1f3d1a81..fe793ea6 100644 --- a/docs/rules/naming/named-parameters-mapping.md +++ b/docs/rules/naming/named-parameters-mapping.md @@ -39,7 +39,7 @@ mapping(string name => uint256 balance) public users; mapping(address owner => mapping(address token => uint256 balance)) public tokenBalances; ``` -#### Main key of mapping is enforced. On nested mappings other naming are not neccesary +#### Main key of mapping is enforced. On nested mappings other naming are not necessary ```solidity mapping(address owner => mapping(address => uint256)) public tokenBalances; diff --git a/e2e/08-autofix/imports-order/.solhint.json b/e2e/08-autofix/imports-order/.solhint.json new file mode 100644 index 00000000..b17afc51 --- /dev/null +++ b/e2e/08-autofix/imports-order/.solhint.json @@ -0,0 +1,5 @@ +{ + "rules": { + "imports-order": "error" + } +} diff --git a/e2e/08-autofix/imports-order/Foo1.sol b/e2e/08-autofix/imports-order/Foo1.sol new file mode 100644 index 00000000..a15c2fb7 --- /dev/null +++ b/e2e/08-autofix/imports-order/Foo1.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.0; + +import './ThisIsAVeryLongFileOnPurposeToTestTheFirstPathShorterThanTheLastOnelooooooooooong.sol'; +import { Unauthorized, add as func, Point } from './Foo.sol'; +import 'https://github.com/owner/repo/blob/branch/path/to/Contract.sol'; +import { Initializable } from './openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; +import '../../../../token/interfaces/FakeContract1.sol'; +import { FakeContract3 } from '../../../token/interfaces/FakeContract3.sol'; +import './../../../../token/interfaces/AFakeContract1.sol'; +import './../token/interfaces/IXTokenWrapper.sol'; +import { IXTokenFactory, holaquetal } from '../../token/interfaces/IXTokenFactory.sol'; +import './../../bpath/otherfolder/otherfolder/aContract.sol'; +import '@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol'; +import { FakeContract2 } from '../../../token/interfaces/FakeContract2.sol'; +import '../../apath/zContract.sol'; +import 'http://github.com/owner/repo/blob/branch/path/to/Contract2.sol'; +import { Afool1 } from './Afool1.sol'; +import './Ownable.sol'; +import { IXTokenWrapper2 } from '../token/interfaces/IXTokenWrapper2.sol'; +import { ReentrancyGuardUpgradeable2 } from '@apenzeppelin/ReentrancyGuardUpgradeable2.sol'; + +contract ImportsOrder { + constructor() {} +} diff --git a/e2e/08-autofix/imports-order/Foo1AfterFix.sol b/e2e/08-autofix/imports-order/Foo1AfterFix.sol new file mode 100644 index 00000000..e835d7dd --- /dev/null +++ b/e2e/08-autofix/imports-order/Foo1AfterFix.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.0; + +import { ReentrancyGuardUpgradeable2 } from '@apenzeppelin/ReentrancyGuardUpgradeable2.sol'; +import '@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol'; +import 'http://github.com/owner/repo/blob/branch/path/to/Contract2.sol'; +import 'https://github.com/owner/repo/blob/branch/path/to/Contract.sol'; +import './../../../../token/interfaces/AFakeContract1.sol'; +import './../../../../token/interfaces/FakeContract1.sol'; +import { FakeContract2 } from './../../../token/interfaces/FakeContract2.sol'; +import { FakeContract3 } from './../../../token/interfaces/FakeContract3.sol'; +import './../../apath/zContract.sol'; +import './../../bpath/otherfolder/otherfolder/aContract.sol'; +import { IXTokenFactory, holaquetal } from './../../token/interfaces/IXTokenFactory.sol'; +import './../token/interfaces/IXTokenWrapper.sol'; +import { IXTokenWrapper2 } from './../token/interfaces/IXTokenWrapper2.sol'; +import { Afool1 } from './Afool1.sol'; +import { Unauthorized, add as func, Point } from './Foo.sol'; +import { Initializable } from './openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; +import './Ownable.sol'; +import './ThisIsAVeryLongFileOnPurposeToTestTheFirstPathShorterThanTheLastOnelooooooooooong.sol'; + +contract ImportsOrder { + constructor() {} +} diff --git a/e2e/08-autofix/imports-order/Foo1BeforeFix.sol b/e2e/08-autofix/imports-order/Foo1BeforeFix.sol new file mode 100644 index 00000000..a15c2fb7 --- /dev/null +++ b/e2e/08-autofix/imports-order/Foo1BeforeFix.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.0; + +import './ThisIsAVeryLongFileOnPurposeToTestTheFirstPathShorterThanTheLastOnelooooooooooong.sol'; +import { Unauthorized, add as func, Point } from './Foo.sol'; +import 'https://github.com/owner/repo/blob/branch/path/to/Contract.sol'; +import { Initializable } from './openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; +import '../../../../token/interfaces/FakeContract1.sol'; +import { FakeContract3 } from '../../../token/interfaces/FakeContract3.sol'; +import './../../../../token/interfaces/AFakeContract1.sol'; +import './../token/interfaces/IXTokenWrapper.sol'; +import { IXTokenFactory, holaquetal } from '../../token/interfaces/IXTokenFactory.sol'; +import './../../bpath/otherfolder/otherfolder/aContract.sol'; +import '@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol'; +import { FakeContract2 } from '../../../token/interfaces/FakeContract2.sol'; +import '../../apath/zContract.sol'; +import 'http://github.com/owner/repo/blob/branch/path/to/Contract2.sol'; +import { Afool1 } from './Afool1.sol'; +import './Ownable.sol'; +import { IXTokenWrapper2 } from '../token/interfaces/IXTokenWrapper2.sol'; +import { ReentrancyGuardUpgradeable2 } from '@apenzeppelin/ReentrancyGuardUpgradeable2.sol'; + +contract ImportsOrder { + constructor() {} +} diff --git a/e2e/08-autofix/imports-order/Foo2.sol b/e2e/08-autofix/imports-order/Foo2.sol new file mode 100644 index 00000000..4959ef29 --- /dev/null +++ b/e2e/08-autofix/imports-order/Foo2.sol @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.0; + +import './ThisIsAVeryLongFileOnPurposeToTestTheFirstPathShorterThanTheLastOnelooooooooooong.sol'; +import '../../../../../token/interfaces/IXTokenWrapper3.sol'; +import {IXTokenFactory2} from '../../atoken/interfaces/IXTokenFactory2.sol'; +import {Initializable} from './openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; +import '../../../../token/interfaces/FakeContract1.sol'; +import '../token/interfaces/IXTokenWrapper.sol'; +import {IXTokenFactory, holaquetal} from '../../token/interfaces/IXTokenFactory.sol'; +import '@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol'; +import {FakeContract2} from '../../../token/interfaces/FakeContract2.sol'; +import {Afool1} from './Afool1.sol'; +import {IXTokenWrapper2} from '../token/interfaces/IXTokenWrapper2.sol'; +import {ReentrancyGuardUpgradeable2} from '@apenzeppelin/ReentrancyGuardUpgradeable2.sol'; + +contract ImportsOrder2 { + constructor() {} +} diff --git a/e2e/08-autofix/imports-order/Foo2AfterFix.sol b/e2e/08-autofix/imports-order/Foo2AfterFix.sol new file mode 100644 index 00000000..8e9a3120 --- /dev/null +++ b/e2e/08-autofix/imports-order/Foo2AfterFix.sol @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.0; + +import {ReentrancyGuardUpgradeable2} from '@apenzeppelin/ReentrancyGuardUpgradeable2.sol'; +import '@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol'; +import '../../../../../token/interfaces/IXTokenWrapper3.sol'; +import '../../../../token/interfaces/FakeContract1.sol'; +import {FakeContract2} from '../../../token/interfaces/FakeContract2.sol'; +import {IXTokenFactory2} from '../../atoken/interfaces/IXTokenFactory2.sol'; +import {IXTokenFactory, holaquetal} from '../../token/interfaces/IXTokenFactory.sol'; +import '../token/interfaces/IXTokenWrapper.sol'; +import {IXTokenWrapper2} from '../token/interfaces/IXTokenWrapper2.sol'; +import {Initializable} from './openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; +import {Afool1} from './Afool1.sol'; +import './ThisIsAVeryLongFileOnPurposeToTestTheFirstPathShorterThanTheLastOnelooooooooooong.sol'; + +contract ImportsOrder2 { + constructor() {} +} diff --git a/e2e/08-autofix/imports-order/Foo2BeforeFix.sol b/e2e/08-autofix/imports-order/Foo2BeforeFix.sol new file mode 100644 index 00000000..4959ef29 --- /dev/null +++ b/e2e/08-autofix/imports-order/Foo2BeforeFix.sol @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.0; + +import './ThisIsAVeryLongFileOnPurposeToTestTheFirstPathShorterThanTheLastOnelooooooooooong.sol'; +import '../../../../../token/interfaces/IXTokenWrapper3.sol'; +import {IXTokenFactory2} from '../../atoken/interfaces/IXTokenFactory2.sol'; +import {Initializable} from './openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; +import '../../../../token/interfaces/FakeContract1.sol'; +import '../token/interfaces/IXTokenWrapper.sol'; +import {IXTokenFactory, holaquetal} from '../../token/interfaces/IXTokenFactory.sol'; +import '@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol'; +import {FakeContract2} from '../../../token/interfaces/FakeContract2.sol'; +import {Afool1} from './Afool1.sol'; +import {IXTokenWrapper2} from '../token/interfaces/IXTokenWrapper2.sol'; +import {ReentrancyGuardUpgradeable2} from '@apenzeppelin/ReentrancyGuardUpgradeable2.sol'; + +contract ImportsOrder2 { + constructor() {} +} diff --git a/e2e/autofix-test.js b/e2e/autofix-test.js index 3edaaa29..0b7812ca 100644 --- a/e2e/autofix-test.js +++ b/e2e/autofix-test.js @@ -26,6 +26,17 @@ function retrieveParams(subpath) { function compareTextFiles(file1Path, file2Path) { const file1Content = fs.readFileSync(file1Path, 'utf-8') const file2Content = fs.readFileSync(file2Path, 'utf-8') + + // console.log('=================================================================================') + // console.log('=================================================================================') + // console.log('file1Content: ', file1Content) + // console.log('=================================================================================') + // console.log('=================================================================================') + // console.log('=================================================================================') + // console.log('=================================================================================') + // console.log('file2Content: ', file2Content) + // console.log('=================================================================================') + // console.log('=================================================================================') return file1Content === file2Content } @@ -458,7 +469,7 @@ describe('e2e', function () { expect(reportLines[reportLines.length - 3]).to.contain(finalLine) }) }) - + it('should check FOO1 does not change after test (7)', () => { result = compareTextFiles(currentFile, beforeFixFile) expect(result).to.be.true @@ -504,7 +515,7 @@ describe('e2e', function () { expect(reportLines[reportLines.length - 3]).to.contain(finalLine) }) }) - + it('should check FOO1 does not change after test (8)', () => { result = compareTextFiles(currentFile, beforeFixFile) expect(result).to.be.true @@ -526,12 +537,12 @@ describe('e2e', function () { }) describe('--fix with noPrompt', () => { - it('should compare Foo1 file with template BEFORE FIX file and they should match (8)', () => { + it('should compare Foo1 file with template BEFORE FIX file and they should match (9)', () => { result = compareTextFiles(currentFile, beforeFixFile) expect(result).to.be.true }) - it('should execute and compare Foo1 with template AFTER FIX and they should match (8)', () => { + it('should execute and compare Foo1 with template AFTER FIX and they should match (9)', () => { ;({ code, stdout } = shell.exec( `${params.command} ${params.param1} -c ${currentConfig} ${currentFile} --fix --disc --noPrompt` )) @@ -540,22 +551,114 @@ describe('e2e', function () { expect(result).to.be.true }) - it('should execute and exit with code 1 (8)', () => { + it('should execute and exit with code 1 (9)', () => { expect(code).to.equal(EXIT_CODES.REPORTED_ERRORS) }) - it('should get the right report (8)', () => { + it('should get the right report (9)', () => { const reportLines = stdout.split('\n') const finalLine = '6 problems (6 errors, 0 warnings)' expect(reportLines[reportLines.length - 3]).to.contain(finalLine) }) }) - - it('should check FOO1 does not change after test (8)', () => { + + it('should check FOO1 does not change after test (9)', () => { result = compareTextFiles(currentFile, beforeFixFile) expect(result).to.be.true }) }) + + describe('autofix rule: imports-order', () => { + describe('autofix rule: imports-order Foo1', () => { + before(function () { + params = retrieveParams('imports-order/') + currentConfig = `${params.path}${params.subpath}.solhint.json` + currentFile = `${params.path}${params.subpath}Foo1.sol` + beforeFixFile = `${params.path}${params.subpath}Foo1BeforeFix.sol` + afterFixFile = `${params.path}${params.subpath}Foo1AfterFix.sol` + }) + after(function () { + if (!E2E) { + copyFile(beforeFixFile, currentFile) + } + }) + + describe('--fix with noPrompt', () => { + it('should compare Foo1 file with template BEFORE FIX file and they should match (10)', () => { + result = compareTextFiles(currentFile, beforeFixFile) + expect(result).to.be.true + }) + + it('should execute and compare Foo1 with template AFTER FIX and they should match (10)', () => { + ;({ code, stdout } = shell.exec( + `${params.command} ${params.param1} -c ${currentConfig} ${currentFile} --fix --disc --noPrompt` + )) + result = compareTextFiles(currentFile, afterFixFile) + expect(result).to.be.true + }) + + it('should execute and exit with code 1 (10)', () => { + expect(code).to.equal(EXIT_CODES.REPORTED_ERRORS) + }) + + it('should get the right report (10)', () => { + const reportLines = stdout.split('\n') + const finalLine = '18 problems (18 errors, 0 warnings)' + expect(reportLines[reportLines.length - 3]).to.contain(finalLine) + }) + }) + + it('should check FOO1 does not change after test (10)', () => { + result = compareTextFiles(currentFile, beforeFixFile) + expect(result).to.be.true + }) + }) + // describe('autofix rule: imports-order Foo2', () => { + // before(function () { + // params = retrieveParams('imports-order/') + // currentConfig = `${params.path}${params.subpath}.solhint.json` + // currentFile = `${params.path}${params.subpath}Foo2.sol` + // beforeFixFile = `${params.path}${params.subpath}Foo2BeforeFix.sol` + // afterFixFile = `${params.path}${params.subpath}Foo2AfterFix.sol` + // }) + // after(function () { + // if (!E2E) { + // copyFile(beforeFixFile, currentFile) + // } + // }) + + // describe('--fix with noPrompt', () => { + // it('should compare Foo1 file with template BEFORE FIX file and they should match (11)', () => { + // result = compareTextFiles(currentFile, beforeFixFile) + // expect(result).to.be.true + // }) + + // it('should execute and compare Foo1 with template AFTER FIX and they should match (11)', () => { + // ;({ code, stdout } = shell.exec( + // `${params.command} ${params.param1} -c ${currentConfig} ${currentFile} --fix --disc --noPrompt` + // )) + + // result = compareTextFiles(currentFile, afterFixFile) + // expect(result).to.be.true + // }) + + // it('should execute and exit with code 1 (11)', () => { + // expect(code).to.equal(EXIT_CODES.REPORTED_ERRORS) + // }) + + // it('should get the right report (11)', () => { + // const reportLines = stdout.split('\n') + // const finalLine = '12 problems (12 errors, 0 warnings)' + // expect(reportLines[reportLines.length - 3]).to.contain(finalLine) + // }) + // }) + + // it('should check FOO1 does not change after test (11)', () => { + // result = compareTextFiles(currentFile, beforeFixFile) + // expect(result).to.be.true + // }) + // }) + }) }) // FALTA LA PRUEBA DEL STORE TO FILE diff --git a/e2e/test.js b/e2e/test.js index 2d77e757..59c2bfd7 100644 --- a/e2e/test.js +++ b/e2e/test.js @@ -96,7 +96,7 @@ describe('e2e', function () { const PATH = '03-no-empty-blocks' const { PREFIX, SUFFIX } = prepareContext(PATH) - it('No contracts to lint should fail with appropiate message', function () { + it('No contracts to lint should fail with appropriate message', function () { const { code, stderr } = shell.exec(`${NODE}solhint Foo1.sol ${SUFFIX}`) expect(code).to.equal(EXIT_CODES.BAD_OPTIONS) diff --git a/lib/rules/naming/func-named-parameters.js b/lib/rules/naming/func-named-parameters.js index 170ea4ba..3f821f84 100644 --- a/lib/rules/naming/func-named-parameters.js +++ b/lib/rules/naming/func-named-parameters.js @@ -36,6 +36,10 @@ const meta = { description: 'Function call with four NAMED parameters', code: 'functionName({ sender: _senderAddress, amount: 1e18, token: _tokenAddress, receiver: _receiverAddress })', }, + { + description: 'abi.encodeX call with four UNNAMED parameters', + code: 'abi.encodePacked(_senderAddress, 1e18, _tokenAddress, _receiverAddress )', + }, ], bad: [ { @@ -67,12 +71,24 @@ class FunctionNamedParametersChecker extends BaseChecker { const qtyNamed = node.names.length const qtyArgs = node.arguments.length - if (qtyArgs !== 0) { - if (qtyNamed === 0 && qtyArgs > this.maxUnnamedArguments) { - this.error( - node, - `Named parameters missing. MIN unnamed argumenst is ${this.maxUnnamedArguments}` - ) + if (!this.isAbiCall(node)) { + if (qtyArgs !== 0) { + if (qtyNamed === 0 && qtyArgs > this.maxUnnamedArguments) { + this.error( + node, + `Named parameters missing. MIN unnamed arguments is ${this.maxUnnamedArguments}` + ) + } + } + } + } + + isAbiCall(node) { + if (node.expression.type === 'MemberAccess') { + if (node.expression.expression.type === 'Identifier') { + if (node.expression.expression.name === 'abi') { + return true + } } } } diff --git a/lib/rules/naming/imports-order.js b/lib/rules/naming/imports-order.js new file mode 100644 index 00000000..6d3a255e --- /dev/null +++ b/lib/rules/naming/imports-order.js @@ -0,0 +1,216 @@ +const BaseChecker = require('../base-checker') +const { severityDescription } = require('../../doc/utils') + +const DEFAULT_SEVERITY = 'warn' + +const ruleId = 'imports-order' +const meta = { + type: 'naming', + + docs: { + description: `Order the imports of the contract to follow a certain hierarchy (read "Notes section")`, + category: 'Style Guide Rules', + options: [ + { + description: severityDescription, + default: DEFAULT_SEVERITY, + }, + ], + notes: [ + { + note: 'Paths starting with "@" like "@openzeppelin/" and urls ("http" and "https") will go first', + }, + { + note: 'Order by hierarchy of directories first, e.g. ./../../ comes before ./../, which comes before ./, which comes before ./foo', + }, + { + note: 'Order alphabetically for each path at the same level, e.g. ./contract/Zbar.sol comes before ./interface/Ifoo.sol', + }, + { + note: 'Rule does NOT support this kind of import "import * as Alias from "./filename.sol"', + }, + { + note: 'When "--fix", rule will re-write this notation "../folder/file.sol" or this one "../file.sol" to "./../folder/file.sol" or this one "./../file.sol"', + }, + ], + }, + + isDefault: false, + recommended: false, + defaultSetup: 'warn', + fixable: true, + schema: null, +} + +class ImportsOrderChecker extends BaseChecker { + constructor(reporter) { + super(reporter, ruleId, meta) + this.orderedImports = [] // This will hold the sorted imports + } + + SourceUnit(node) { + // console.log('node.children :>> ', node.children) + // get all the imports into one object + this.fromContractImports = node.children + .filter((child) => child.type === 'ImportDirective') + .map((child) => { + const normalizedPath = this.normalizePath(child.path) + const result = { + range: child.range, + path: normalizedPath, + fullSentence: child.symbolAliases + ? `${this.getFullSentence(child.symbolAliases)}'${normalizedPath}';` + : `import '${normalizedPath}';`, + } + return result + }) + + // Create a deep copy of fromContractImports for sorting + this.orderedImports = JSON.parse(JSON.stringify(this.fromContractImports)) + + // order the object to get the ordered and the contract order + this.orderedImports = this.sortImports(this.orderedImports) + + // console.log('this.orderedImports :>> ', this.fromContractImports) + // console.log('NO Order: \n') + // this.fromContractImports.forEach((importItem) => console.log(importItem.fullSentence)) + // console.log('\n\nOrdered: \n') + // this.orderedImports.forEach((importItem) => console.log(importItem.fullSentence)) + } + + 'SourceUnit:exit'(node) { + // when finish analyzing check if ordered import array is equal to the import contract order + const areEqual = areArraysEqual(this.fromContractImports, this.orderedImports) + + // if are equal do nothing, if not enter the if + if (!areEqual) { + // Find the lowest starting range to start the replacement + let currentStart = Math.min(...this.fromContractImports.map((imp) => imp.range[0])) + // Prepare replacements changing the range + const replacements = this.orderedImports.map((orderedImport) => { + const newText = orderedImport.fullSentence + const rangeEnd = currentStart + newText.length + + const replacement = { + range: [currentStart, rangeEnd], + newText, + } + + currentStart = rangeEnd + + return replacement + }) + + // get the name of the contract only to report the error + let name = '' + const loopQty = replacements.length - 1 + // loop through imports to report and correct if requested + for (let i = loopQty; i >= 0; i--) { + name = this.fromContractImports[i].path.split('/') + name = name[name.length - 1] + this.error( + node, + `Wrong Import Order for {${name}}`, + // replace from bottom to top all the imports in the right order + // flag the first one, which will be the last import + this.fixStatement(replacements[i], i === loopQty) + ) + } + } + } + + fixStatement(replacement, isLast) { + // the last import should replace all the chars up to the end + // this is for imports path longer than the one it was before + if (isLast) { + const lastRangeEnd = this.fromContractImports[this.fromContractImports.length - 1].range[1] + return (fixer) => + fixer.replaceTextRange([replacement.range[0], lastRangeEnd], replacement.newText) + } + return (fixer) => fixer.replaceTextRange(replacement.range, replacement.newText + '\n') + } + + sortImports(unorderedImports) { + // Helper function to determine the hierarchical level of a path + function getHierarchyLevel(path) { + // put very large numbers so these comes first in precedence + const protocolOrder = { + '@': -30000, + 'http://': -20000, + 'https://': -10000, + } + + // Check for protocol-specific paths and assign them their respective order levels + for (const protocol in protocolOrder) { + if (path.startsWith(protocol)) { + return protocolOrder[protocol] + } + } + + // Relative path handling + if (path.startsWith('./')) { + // Count the number of '../' sequences to determine depth + const depth = path.split('/').filter((part) => part === '..').length + // Return a negative value to ensure that lesser depth has higher precedence (closer to 0) + return -depth + } + + // Default catch-all for unhandled cases + return Infinity + } + + // Sort imports based on the hierarchy level and then alphabetically + const orderedImports = unorderedImports.sort((a, b) => { + const levelA = getHierarchyLevel(a.path) + const levelB = getHierarchyLevel(b.path) + + if (levelA !== levelB) { + return levelA - levelB + } + + // For same level, sort alphabetically by the entire path, case-insensitive + return a.path.localeCompare(b.path, undefined, { sensitivity: 'base' }) + }) + + return orderedImports + } + + // Map through each subarray to convert it to the desired format + getFullSentence(elements) { + const importParts = elements.map(([name, alias]) => { + // Check if there is an alias; if so, format it as 'name as alias', otherwise just return the name + return alias ? `${name} as ${alias}` : name + }) + + // Join all the parts with a comma and space, and format it into the final import string + return `import { ${importParts.join(', ')} } from ` + } + + normalizePath(path) { + if (path.startsWith('../')) { + return `./${path}` + } + return path + } +} + +// function to compare is both arrays are equal +function areArraysEqual(arr1, arr2) { + if (arr1.length !== arr2.length) { + return false + } + + for (let i = 0; i < arr1.length; i++) { + if ( + arr1[i].path !== arr2[i].path || + arr1[i].range[0] !== arr2[i].range[0] || + arr1[i].range[1] !== arr2[i].range[1] + ) { + return false + } + } + + return true +} + +module.exports = ImportsOrderChecker diff --git a/lib/rules/naming/index.js b/lib/rules/naming/index.js index 43ead75b..2949ad35 100644 --- a/lib/rules/naming/index.js +++ b/lib/rules/naming/index.js @@ -11,6 +11,7 @@ const NamedParametersMappingChecker = require('./named-parameters-mapping') const ImmutableVarsNamingChecker = require('./immutable-vars-naming') const FunctionNamedParametersChecker = require('./func-named-parameters') const FoundryTestFunctionsChecker = require('./foundry-test-functions') +const ImportsOrderChecker = require('./imports-order') module.exports = function checkers(reporter, config) { return [ @@ -27,5 +28,6 @@ module.exports = function checkers(reporter, config) { new ImmutableVarsNamingChecker(reporter, config), new FunctionNamedParametersChecker(reporter, config), new FoundryTestFunctionsChecker(reporter, config), + new ImportsOrderChecker(reporter, config), ] } diff --git a/lib/rules/naming/named-parameters-mapping.js b/lib/rules/naming/named-parameters-mapping.js index c170fa62..a798abc8 100644 --- a/lib/rules/naming/named-parameters-mapping.js +++ b/lib/rules/naming/named-parameters-mapping.js @@ -20,7 +20,7 @@ const meta = { }, { description: - 'Main key of mapping is enforced. On nested mappings other naming are not neccesary', + 'Main key of mapping is enforced. On nested mappings other naming are not necessary', code: 'mapping(address owner => mapping(address => uint256)) public tokenBalances;', }, { diff --git a/package.json b/package.json index 35199efd..bb17a54e 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "solhint", - "version": "5.0.1", + "version": "5.0.2", "description": "Solidity Code Linter", "main": "lib/index.js", "keywords": [ diff --git a/test/rules/naming/func-named-parameters.js b/test/rules/naming/func-named-parameters.js index c1102d2d..97389e69 100644 --- a/test/rules/naming/func-named-parameters.js +++ b/test/rules/naming/func-named-parameters.js @@ -27,7 +27,7 @@ describe('Linter - func-named-parameters', () => { assertErrorCount(report, 1) assertErrorMessage( report, - `Named parameters missing. MIN unnamed argumenst is ${ + `Named parameters missing. MIN unnamed arguments is ${ minUnnamed < DEFAULT_MIN_UNNAMED_ARGUMENTS ? DEFAULT_MIN_UNNAMED_ARGUMENTS : minUnnamed }` ) @@ -90,7 +90,7 @@ describe('Linter - func-named-parameters', () => { assertErrorMessage( report, - `Named parameters missing. MIN unnamed argumenst is ${DEFAULT_MIN_UNNAMED_ARGUMENTS}` + `Named parameters missing. MIN unnamed arguments is ${DEFAULT_MIN_UNNAMED_ARGUMENTS}` ) }) @@ -106,7 +106,7 @@ describe('Linter - func-named-parameters', () => { assertErrorMessage( report, - `Named parameters missing. MIN unnamed argumenst is ${DEFAULT_MIN_UNNAMED_ARGUMENTS}` + `Named parameters missing. MIN unnamed arguments is ${DEFAULT_MIN_UNNAMED_ARGUMENTS}` ) }) })