Feature request: rule for ordering imports topologically and alphabetically

[PaulRBerg]

It would be helpful if Solhint offered a rule for ordering imports topologically and alphabetically.

Rationale: this is a non-opinionated code design that lessens the cognitive load on users.

Example of PR where we manually ordered the imports: sablier-labs/v2-periphery#301

I propose the following logic:

• Order by hierarchy of directories first, e.g. ../../ comes before ../, which comes before ./, which comes before ./foo
• Order alphabetically for each file at the same level, e.g. ./bar comes before ./foo

[dbale-altoros]

thanks @PaulRBerg for posting
I'll check what can be done about this

[PaulRBerg]

Can we sponsor you with a bounty to work on this, @dbale-altoros? feel free to share your Ethereum address on Telegram.

[dbale-altoros]

Hi @PaulRBerg i'm on vacations. I'll contact you this friday o next monday

[dbale-altoros]

@PaulRBerg
I was starting to do this
Just to be on the same page
The rule will support these kind of imports

Import parts of a file

• import {IXTokenFactory, IXToken} from '../../token/interfaces/IXTokenFactory.sol';

Import parts and define alias

• import {IXTokenFactory, IXToken as Token} from '../../token/interfaces/IXTokenFactory.sol';

Import the whole file

• import '../../token/interfaces/IXTokenFactory.sol';

Regarding the hierarchy
I will put a sample of unsorted imports
Would you mind ordering in the way you think it should be

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 '@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol';
import {FakeContract2} from '../../../token/interfaces/FakeContract2.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';

take into account there are paths like this
./../../folder/file.sol
which are at the same level as
../../folder/file.sol

[PaulRBerg]

thank you @dbale-altoros

adding @smol-ninja and @andreivladbrg for help here

[smol-ninja]

Hi @dbale-altoros, thank you for working on this. This will save us a significant amount of time. Here are the answers to your questions:

1. When different files are imported from the same path, we prefer to order the files in alphabetical order, ignoring the alias. For example:

   • import { IXToken, IXTokenFactory } from '../../token/interfaces/IXTokenFactory.sol';
   • import { IXToken as Token, IXTokenFactory } from '../../token/interfaces/IXTokenFactory.sol';

2. If the filenames are the same, I will put ./../../folder/file.sol over ../../folder/file.sol.
   If the filenames are different, then I will put them in alphabetical order by file name. For example, ../../folder/aile.sol would come before ./../../folder/bile.sol because technically they are at the same directory level.

3. Ordering the imports in the sample provided:

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 { holaquetal, IXTokenFactory } from '../../token/interfaces/IXTokenFactory.sol';
import './../token/interfaces/IXTokenWrapper.sol';
import { IXTokenWrapper2 } from '../token/interfaces/IXTokenWrapper2.sol';
import { Afool1 } from './Afool1.sol';
import { add as func, Point, Unauthorized } from "./Foo.sol";
import { Initializable } from './openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol';
import "./Ownable.sol";
import './ThisIsAVeryLongFileOnPurposeToTestTheFirstPathShorterThanTheLastOnelooooooooooong.sol';
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";

Note that I have put two spaces inside the braces { ABC }. Please let me know if you need help with more samples. I'd be very happy to explain further.

[dbale-altoros]

@smol-ninja thanks for the feedback
I will be moving forward with this mostly on the weekend
One thing...
Linter or prettier removes spaces in the { ABC } import... leaving those like this {ABC} so not sure if leaving with the space will be ok ?... but most important... the capability on solhint to "replace" is kind of limited so i'll do my best

[smol-ninja]

Thanks @dbale-altoros for the update.

Linter or prettier removes spaces in the { ABC } import.

True and that's why we use bracketSpacing: true in our prettier settings so it does not remove them.

[dbale-altoros]

Thanks @dbale-altoros for the update.

Linter or prettier removes spaces in the { ABC } import.

True and that's why we use bracketSpacing: true in our prettier settings so it does not remove them.

oh jajaja i will steal that... never looked that up , so i left that as is

[dbale-altoros]

@smol-ninja
following your thought
why is ReentrancyGuardUpgradeable2.sol before ReentrancyGuardUpgradeable.sol' in the ordered imports ?

import { ReentrancyGuardUpgradeable2 } from '@apenzeppelin/ReentrancyGuardUpgradeable2.sol';
import '@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol';

[smol-ninja]

Its because @a < @o so @apenzeppelin/ReentrancyGuardUpgradeable2.sol would come before @openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol.

[dbale-altoros]

@smol-ninja
ok cool, so the path takes precedence over the filename ?
sorry to make you repeat things but i want to make sure

so for this

./../../apath/zContract.sol
./../../bpath/aContract.sol

zContract path will be first

and for this

./../../apath/otherfolder/otherfolder/zContract.sol
./../../bpath/aContract.sol

zContract path will be first

and for this

./../../apath/zContract.sol
./../../bpath/otherfolder/otherfolder/aContract.sol

zContract path will be first

but for this

./zContract.sol
./aContract.sol

aContract path will be first

is that right ?

[smol-ninja]

the path takes precedence over the filename ?

Yes.

And yes yes yes to all your questions. You are absolutely right. That's how we like to do it.

[dbale-altoros]

@smol-ninja @PaulRBerg
I think is done here:
#587

You can test it, if you clone the repo and execute
node solhint -c ./imports-order.json ./FooImports.sol --fix --noPrompt

Just make a file called imports-order.json
with this content:

{
  "rules": {
    "imports-order": "error",
  }
}

And of course replace FooImports.sol with the contract of your choice

I can put together a new version next week, if you can give me some feedback, that will be awesome

[smol-ninja]

Incredible. I just gave it go and realised that I made a mistake in my suggestion. I'd place the external imports above the relative imports and separate them with a "newline" to improve readability. So, the following will be placed at the top.

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";

The updated order would look like:

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 { holaquetal, IXTokenFactory } from '../../token/interfaces/IXTokenFactory.sol';
import './../token/interfaces/IXTokenWrapper.sol';
import { IXTokenWrapper2 } from '../token/interfaces/IXTokenWrapper2.sol';
import { Afool1 } from './Afool1.sol';
import { add as func, Point, Unauthorized } from "./Foo.sol";
import { Initializable } from './openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol';
import "./Ownable.sol";
import './ThisIsAVeryLongFileOnPurposeToTestTheFirstPathShorterThanTheLastOnelooooooooooong.sol';

The objective is to separate external imports from the relative imports. Sorry for making this mistake and increasing your work load. Does it require a major refactor in your PR?

Otherwise, I tested it locally, and very happy to confirm that everything is working perfect.

[dbale-altoros]

cool @smol-ninja
I can change the order, not sure about the new line
Need to check that

[dbale-altoros]

@smol-ninja please try it now...
I cannot make the new line between the groups...
The rest seems to be working to me

[smol-ninja]

Fantastic! I just gave it a go and it works. Don't worry about the new line, it doesn't matter much since can easily be added manually.

Thank you so much @dbale-altoros for doing this.

[PaulRBerg]

Thank you very much @dbale-altoros!

[dbale-altoros]

@PaulRBerg @smol-ninja i guess following days i will release a new version including this rule

[dbale-altoros]

new release with this feature launched
@PaulRBerg @smol-ninja

[smol-ninja]

Brilliant. Have a lovely weekend.

[smol-ninja]

Hi @dbale-altoros, I was testing the new rule and seems like there is a bug. It leads to the following sorting:

import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";
import { PRBMathUtils } from "@prb/math/test/utils/Utils.sol";
import { Constants } from "./Constants.sol";
import { CommonBase } from "forge-std/src/Base.sol";
import { SafeCastLib } from "solady/src/utils/SafeCastLib.sol";
import { Helpers } from "src/libraries/Helpers.sol";

We discussed that it would be a good idea to have external imports above the relative imports. It seems like the relative import, i.e. "./Constants.sol", is being ordered alphabetically relative to the external imports.

[dbale-altoros]

Hi' @smol-ninja , thanks for the feedback

And how do yo want that to be ordered ? please specify...
From solidity docs here:
https://docs.soliditylang.org/en/latest/path-resolution.html

Relative Imports
An import starting with ./ or ../ is a relative import. Such imports specify a path relative to the source unit name of the importing source unit

Direct Imports
An import that does not start with ./ or ../ is a direct import.

Is this what you mean ?

I will leave the code for testing before releasing new version, like I did before.
Please inform any other thing, because I need definitive answers previous the release

Thanks

[smol-ninja]

Is this what you mean ?

Yes.

And how do yo want that to be ordered?

I'd order the above example as the following:

import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";
import { PRBMathUtils } from "@prb/math/test/utils/Utils.sol";
import { CommonBase } from "forge-std/src/Base.sol";
import { SafeCastLib } from "solady/src/utils/SafeCastLib.sol";
import { Helpers } from "src/libraries/Helpers.sol";
import { Constants } from "./Constants.sol";

Let me know if you have any more questions.

[dbale-altoros]

@smol-ninja @PaulRBerg
I left a PR with the fix
#593

You can test it, if you clone the repo and go to that branch and execute
node solhint -c ./imports-order.json ./FooImports.sol --fix --noPrompt

Just make a file called imports-order.json
with this content:

  "rules": {
    "imports-order": "error",
  }
}

And of course replace FooImports.sol with the contract of your choice

I can put together a new version whenever you give me the ok on this one
Please test it the best you can so we can close this issue... and thanks a lot for your support!!!! really appreciate!!!

[smol-ninja]

Hi @dbale-altoros, I have just tested the new PR and it looks good. Looking forward to the patch release.

Thank you.

[dbale-altoros]

Resolved
#593

[PaulRBerg]

Hey @dbale-altoros, thank you so much for implementing this.

Tiny request: would it be possible to use double quotes instead of single quotes for the import paths?

I gather that this is an opinionated choice, but:

1. The Solidity docs use double quotes.
2. In my own experience, I've seen double quotes used way more than single quotes for import paths.

The current behavior is problematic for us because we need to run solhint --fix twice - once to order imports, and once to format the quotes.

[dbale-altoros]

@PaulRBerg sorry
I understand your point
I can do it, but i will add that in next version fix

[PierrickGT]

Thanks for this rule @dbale-altoros !

I've updated to the latest version and used the fix command.
I think there is an issue with relative paths that get prefixed with ./.

For example, import { IBridge } from "../interfaces/IBridge.sol"; becomes import { IBridge } from "./../interfaces/IBridge.sol"; which is unecessary.

[dbale-altoros]

@PierrickGT is this something that breaks the contracts ? or make a compilation error ?
This is done on purpose to normalize all imports and to get the order in an easier way

[PierrickGT]

@PierrickGT is this something that breaks the contracts ? or make a compilation error ? This is done on purpose to normalize all imports and to get the order in an easier way

It compiles properly but seems a bit redundant.
Not a big deal, it can easily be removed after running fix but if there is an easy way to remove it after ordering the imports, it would be great.

[dbale-altoros]

@PierrickGT feel free to push a pr for this
I don't mind having the ./ at the beggining... i think is more accurated to see all of the paths the same way
You can make the pr with a normalization flag which can remove or not that character

cheers!