Detector for unused imports

[bart1e]

Detector for unused imports implemented. Fixes #1531.
Files with with cyclic imports and files with import {...} from directives are not supported.

Apart from telling the user which imports to remove, it may tell to add more specific imports. For instance if we have:

C.sol:
    import "./B.sol"; // but uses only things from A.sol
B.sol:
    import "./A.sol"

, then it will tell that import "./B.sol" should be removed in C.sol and import "./A.sol" should be added.

For each file, it searches for all items used there and updates the list of needed imports.
Unfortunately, it isn't currently possible to iterate over user defined types in Slither (aliases like type something is uint), so I had to handle them "manually".
Additionally, some items don't have their references set up correctly. Some examples of that include:

• references to contract member functions when they are called
• references aren't added to contract when some other contract inherits from it
• references to top level functions when no contract is declared in a source file

Some issues regarding the implementation:

• since custom types (aliases) had to be treated in a specific way, it's possible that the detector will not detect all of their uses (although I've done my best to handle most common use cases)
• sometimes (for instance in OpenZeppelin repository) there are "import containers" - files that simply contain import directives with related files. The detector will tell that they contain unused imports, which is correct, but creating such files is intentional. You may see it by running the detector on @openzeppelin/contracts/interfaces.
• I have created many tests for the detector, especially to test it against false positives - I feel that they are important, but if you feel there are too many of them, just let me know.

You are welcome to test the detector, and if you have any suggestion on how to improve it, or any question regarding the implementation, please let me know.

[0xalpharush]

Thanks for tackling this! We definitely want to have this, but will likely need to address the issues you outline.

Unfortunately, it isn't currently possible to iterate over user defined types in Slither (aliases like type something is uint), so I had to handle them "manually".

You may be able to iterate over the compilation unit and contract scope to get this info.

Wrt references, some of this has to do with the AST references being inaccurate/ missing (see here), but we are working to improve the parsing here #1565. More broadly, we will probably overhaul the current design of top level declarations as it will make features like this easier to implement.

[bart1e]

@0xalpharush Thanks a lot for the feedback! I will try to make use of these _user_defined_value_types you mentioned - this will definitely make the code shorter and easier to read. I will try to change it tomorrow - I'm a little bit tired a the moment.

An additional problem that I just realised is within the function _import_to_absolute_path, where I try to unify imports by converting them into the absolute paths, but it seems to be failing on some tests. Is there any easy way to convert the Import class instance to an absolute path? Because my approach doesn't seem to work.

[bart1e]

I have just checked both options (using compilation unit and scope) and indeed, I can iterate over user defined types (aliases), but their references aren't updated correctly.
So there are two options:

• I may either edit the code assuming that the references are filled correctly and wait until it's fixed
• or just leave it as is for now

I would prefer the first option, because the code will be easier to read and maintain in the future + we will already have tests for the references in user defined types.
The only question is, how long it will take to improve the parsing (to have correct references).

[0xalpharush]

You may need to do something like Path(...).absolute() and then use filename_loop in _import_to_absolute_path

[0xalpharush]

but their references aren't updated correctly

@bart1e I'm not exactly sure what's incorrect but you're welcome to make changes that fix it and help make your detector more accurate
EDIT: It looks like top level type aliases are being considered here

slither/slither/core/slither_core.py

Line 236 in 4c976d5

def _compute_offsets_to_ref_impl_decl(self): # pylint: disable=too-many-branches

[bart1e]

@0xalpharush, I will try to make the changes as you suggested. Can I do them in this PR, or create a separate one?

And, perhaps, I have expressed myself wrong previously - by "but their references aren't updated correctly" I meant that references list in user defined types (aliases) doesn't reflect all their uses.
Nonetheless, I see that missing references may be added here:

slither/slither/solc_parsing/solidity_types/type_parsing.py

Lines 359 to 365 in 4c976d5

	if t[key] == "UserDefinedTypeName":
	if is_compact_ast:
	name = t["typeDescriptions"]["typeString"]
	if name in renaming:
	name = renaming[name]
	if name in user_defined_types:
	return user_defined_types[name]

[0xalpharush]

@bart1e A separate PR would make it a little easier to review/ test in isolation

[bart1e]

@0xalpharush OK, I will try to submit a separate PR tomorrow.

[shafu0x]

Would also be very helpful to detect unused custom errors and modifiers.

[0xalpharush]

Not 100% sure, but I think we can get rid of the local/CI file path discrepancies by using PurePath(k).as_posix() here

[bart1e]

I have made the change you suggested, however, for some reason, tests weren't run on the code, so I am not sure if it passes the tests that were failing.

[bart1e]

@0xalpharush Before this will be merged, I have to know how the detector should deal with the following things:

• as I have mentioned before, sometimes there are files that just aggregate imports (like https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/interfaces/IERC3156.sol). The question is, how the detector should handle these cases. Right now, for the file I have linked, it will output that 2 files imported there are unneeded. Optionally, I may try to detect (and ignore) such "import containers" by checking whether a file contains only import directives.
• the second question is about which unused imports should be listed by the detector. Consider the following scenario:

// file C.sol uses B.sol and A.sol and contains the following imports:
import "./A.sol";
import "./B.sol";
// file B.sol doesn't use anything from A.sol, but contains the import:
import "./A.sol";

Currently, the detector will say that import "./A.sol" is unneeded in C.sol (since B.sol that is already imported contains it), and that import "./A.sol" is unneeded in B.sol. So, if the user removes A.sol import from both files at the same time, the project will not compile. Should we leave that behaviour or should the detector's output print that import "./A.sol" is unneeded only in B.sol? I would opt for leaving it as is.

[0xalpharush]

I think it'd make sense to ignore files that only contain imports but if a file imports this "container" we'd still want to warn for unused ones.

I think leaving it as is for right now is fine. If it becomes a nuisance for users down the line, we can improve the heuristic.

[bart1e]

I have added a code that will ignore "import containers" while printing the output.

In the future, I may also handle cyclic imports (right now, if cyclic imports are detected, the detector simply omits the files that contain them) - I think that would be too much for one PR as the code is already complicated enough.

[montyly]

This is an amazing work, thanks @bart1e for the effort.

I made minor modifications (mostly python related). My main concern is around the class variable (see the comment)

[0xalpharush]

We determined that we don't want to identify "redundant" imports i.e. where a dependency can be satisfied transitively and instead only recommend removing absolutely unused imports

[bart1e]

I've removed the code responsible for detecting "redundant" imports.

Now, only imports whose own import graph doesn't contain any needed import will be listed.

So, for example, if we have A.sol importing X_1, X_2, ..., X_10 and X_i imports B.sol and A.sol uses something from B.sol, but nothing from X_i, then nothing will be printed by the detector.

So, the detector will now print something a lot less frequently than with the previous implementation.

[mds1]

Just wanted to bump this and see if there are plans to get this merged anytime soon :)

[0xalpharush]

I will try to get this updated and ready for the next release (probably a couple weeks). In addition to some testing, I want to change the way the file names are displayed to be relative to the project and not include the user's home as well as change the format a bit.
From:

Consider removing the following imports:
@openzeppelin/contracts/utils/Address.sol
@openzeppelin/contracts/token/ERC721/ERC721.sol
@openzeppelin/contracts/utils/Strings.sol
hardhat/console.sol
@openzeppelin/contracts/utils/Base64.sol

To:

Consider removing the following imports:
- @openzeppelin/contracts/utils/Address.sol
- @openzeppelin/contracts/token/ERC721/ERC721.sol
- @openzeppelin/contracts/utils/Strings.sol
- hardhat/console.sol
- @openzeppelin/contracts/utils/Base64.sol