Merge pull request #587 from protofire/import-order-rule

ADD: Import order rule
protofire · Jul 25, 2024 · e31d090 · e31d090
2 parents efa15b5 + efea5d8
commit e31d090
Show file tree

Hide file tree

Showing 14 changed files with 512 additions and 9 deletions.
diff --git a/.prettierrc b/.prettierrc
@@ -1,5 +1,6 @@
 {
   "semi": false,
   "singleQuote": true,
-  "printWidth": 100
+  "printWidth": 100,
+  "bracketSpacing": true
 }
diff --git 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/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
@@ -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/e2e/08-autofix/imports-order/.solhint.json 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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