protofire / solhint

Solhint is an open-source project to provide a linting utility for Solidity code.
https://protofire.github.io/solhint/
MIT License
1.03k stars 160 forks source link

Feature request: no-duplicated-import #588

Open fedgiac opened 1 month ago

fedgiac commented 1 month ago

I'd like to suggest a new rule to catch when something is imported twice in a Solidity file, example below.

File ./Imported.sol:

contract Imported {}

File ./Contract.sol:

// The following line should trigger the suggested linting rule.
import {Imported, Imported} from "./Imported.sol`;

[... Solidity code that actually uses `Imported` ...]

A repeating import like the one above compiles successfully. A repetition can also happen in different import lines and from different files, as long as the import resolves to the same object.

dbale-altoros commented 1 month ago

@fedgiac thanks a lot for posting... we could implement that, but only in the current file solhint does not work as a cross file checker I'm not sure if it has too much value when checking only one file...

Is that still good for you ?

fedgiac commented 1 month ago

Yes! The linting issue is always in one file: an object with the same name appears in some import statement more than once.

This is the more complicated variation of this rule that I was talking about (which happens when I merge files together and make imports work by copy-pasting). Here I also write the content of the Imported* files to give a minimal compiling example.

File ./Imported1.sol:

contract Imported {}

File ./Imported2.sol:

import {Imported} from "./Imported1.sol";

File ./Contract.sol:

import {Imported} from "./Imported1.sol";
// The following line should trigger the suggested linting rule.
import {Imported} from "./Imported2.sol";

contract Contract is Imported {}

The code above compiles with Solidity 0.8.26.

Overall I don't think cross-file knowledge matters much: it's true that the two Imported objects could be an instance of two different objects, but in this case the Solidity compiler would return an error (seen by replacing the content of ./Imported2.sol with contract Imported {} in the example).

Another note: import renaming syntax like the following in ./Contract.sol should not trigger the linting rule, since the contract may be trying to use a different object that just happens to have the same name from another source.

import {Imported} from "./Imported1.sol";
// Does not trigger the rule. The import is renamed and used.
import {Imported as Imported2} from "./Imported2.sol";

contract Contract is Imported, Imported2 {}