fix: do not flag imports from import container as unused #2471
Conversation
WalkthroughWalkthroughThe recent changes in Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant UnusedImport
participant FileScope
User->>UnusedImport: Run unused import detector
UnusedImport->>FileScope: Analyze file for imports
FileScope-->>UnusedImport: Return file scope details
UnusedImport->>UnusedImport: Check if file is an import container (_is_import_container)
alt File is an import container
UnusedImport->>UnusedImport: Skip unused import detection
else File is not an import container
UnusedImport->>UnusedImport: Continue with unused import detection
end
UnusedImport-->>User: Return unused import detection results
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- slither/detectors/statements/unused_import.py (2 hunks)
Additional comments not posted (1)
slither/detectors/statements/unused_import.py (1)
72-98: The updates to the_detectmethod to handle import containers and skip dependencies are aligned with the PR objectives.Ensure that these changes integrate smoothly with the rest of the system, particularly in how they interact with other components of the
slithertool.
| @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 |
There was a problem hiding this comment.
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?
closes #2466
Tested on https://github.com/0xalpharush/2466
Summary by CodeRabbit