Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not flag imports from import container as unused #2471

Merged
merged 2 commits into from
Jun 5, 2024
Merged

fix: do not flag imports from import container as unused #2471

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7397483
fix: do not flag imports from import container as unused
0xalpharush Jun 5, 2024
91d5f89
lint
0xalpharush Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 52 additions & 6 deletions slither/detectors/statements/unused_import.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from typing import List
from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification, Output
from slither.core.scope.scope import FileScope

# pylint: disable=protected-access,too-many-nested-blocks
class UnusedImport(AbstractDetector):
Expand Down Expand Up @@ -30,26 +31,71 @@ class UnusedImport(AbstractDetector):
"Remove the unused import. If the import is needed later, it can be added back."
)

@staticmethod
def _is_import_container(scope: FileScope) -> bool: # pylint: disable=too-many-branches
"""
Returns True if a given file (provided as a `FileScope` object) contains only `import` directives (and pragmas).
Such a file doesn't need the imports it contains, but its purpose is to aggregate certain correlated imports.
"""
for c in scope.contracts.values():
if c.file_scope == scope:
return False
for err in scope.custom_errors:
if err.file_scope == scope:
return False
for en in scope.enums.values():
if en.file_scope == scope:
return False
for f in scope.functions:
if f.file_scope == scope:
return False
for st in scope.structures.values():
if st.file_scope == scope:
return False
for ct in scope.type_aliases.values():
if ct.source_mapping and ct.source_mapping.filename == scope.filename:
return False
for uf in scope.using_for_directives:
if uf.file_scope == scope:
return False
for v in scope.variables.values():
if v.file_scope == scope:
return False
return True
Comment on lines +34 to +64
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of _is_import_container method looks robust and well-documented.

Consider adding unit tests to ensure this method accurately identifies import containers under various scenarios.

Would you like me to help by writing some unit tests or opening a GitHub issue for this task?


def _detect(self) -> List[Output]:
results: List[Output] = []
# This is computed lazily and then memoized so we need to trigger the computation.
self.slither._compute_offsets_to_ref_impl_decl()

for unit in self.slither.compilation_units:
for filename, scope in unit.scopes.items():
for filename, current_scope in unit.scopes.items():
# Skip files that are dependencies
if unit.crytic_compile.is_dependency(filename.absolute):
continue

unused = []
for i in scope.imports:
for i in current_scope.imports:
# `scope.imports` contains all transitive imports so we need to filter out imports not explicitly imported in the file.
# Otherwise, we would recommend removing an import that is used by a leaf contract and cause compilation errors.
if i.scope != scope:
if i.scope != current_scope:
continue

# If a scope doesn't define any contract, function, etc., it is an import container.
# The second case accounts for importing from an import container as a reference will only be in the definition's file.
if self._is_import_container(i.scope) or self._is_import_container(
unit.get_scope(i.filename)
):
continue

import_path = self.slither.crytic_compile.filename_lookup(i.filename)
imported_path = self.slither.crytic_compile.filename_lookup(i.filename)

use_found = False
# Search through all references to the imported file
for _, refs in self.slither._offset_to_references[import_path].items():
for ref in refs:
for _, refs_to_imported_path in self.slither._offset_to_references[
imported_path
].items():
for ref in refs_to_imported_path:
# If there is a reference in this file to the imported file, it is used.
if ref.filename == filename:
use_found = True
Expand Down