Unused-imports bug fix #2479
Unused-imports bug fix #2479
Conversation
DarkaMaul
requested review from
0xalpharush,
montyly and
smonicas
as code owners
WalkthroughWalkthroughThe recent changes focus on improving the handling of exported symbols within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Slither
participant FileScope
participant UnusedImportDetector
User->>Slither: Run analysis
Slither->>FileScope: Initialize and process files
FileScope->>FileScope: Update exported symbols (list)
Slither->>UnusedImportDetector: Detect unused imports
UnusedImportDetector->>FileScope: Check references
FileScope->>UnusedImportDetector: Return references
UnusedImportDetector->>Slither: Report results
Slither->>User: Display analysis results
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
Outside diff range and nitpick comments (9)
slither/detectors/statements/unused_import.py (1)
Line range hint
61-64: Optimize the loop for checking if all variables in a scope are not in the given scope.- for v in scope.variables.values(): - if v.file_scope == scope: - return False - return True + return all(v.file_scope != scope for v in scope.variables.values())slither/core/scope/scope.py (2)
Line range hint
24-24: Optimize dictionary key check.- return all(item in d2_keys for item in d1.keys()) + return all(item in d2 for item in d1)Remove
.keys()for a more Pythonic and efficient check.
Line range hint
145-145: Replace lambda expressions with function definitions.- getter: Callable[[SourceUnit], Dict[str, str]] = lambda x: x.bytecodes_init + def get_bytecodes_init(x: SourceUnit) -> Dict[str, str]: + return x.bytecodes_initRepeat this pattern for all instances where lambda expressions are assigned to variables. This change improves readability and adheres to Python best practices.
Also applies to: 163-163, 181-181, 199-199, 215-215
slither/slither.py (1)
Line range hint
144-144: Improve exception handling in the_init_parsing_and_analysesmethod.- except Exception as e: + except Exception as e: + raise e from NoneUse
raise ... from errto provide more context in the traceback, which can help in debugging.slither/core/slither_core.py (1)
Line range hint
478-479: Consider simplifying nestedifstatements into a singleifstatement.This change would enhance readability and maintainability of the code. If you need assistance with this refactor, please let me know.
slither/solc_parsing/slither_compilation_unit_solc.py (3)
Line range hint
198-201: Simplify the assignment ofcanonicalNameusing a more concise approach.- if "canonicalName" in top_level_data["attributes"]: - canonicalName = top_level_data["attributes"]["canonicalName"] - else: - canonicalName = name + canonicalName = top_level_data["attributes"].get("canonicalName", name)This change reduces the lines of code and improves readability by using the
getmethod with a default value.
Line range hint
434-440: Consider merging nestedifstatements into a singleifstatement for clarity.This change would simplify the control flow and enhance the readability of the code.
Line range hint
444-444: Usenot infor membership tests to improve readability.- if i in contract_parser.remapping: + if i not in contract_parser.remapping:This change adheres to Pythonic practices and makes the condition more intuitive.
Also applies to: 448-448
tests/e2e/detectors/test_detectors.py (1)
Line range hint
1949-1950: Optimize the condition check by combiningifstatements.- if skip_existing: - if os.path.isfile(zip_artifact_path): + if skip_existing and os.path.isfile(zip_artifact_path):Tools
Ruff
2-2:
shutilimported but unused (F401)Remove unused import:
shutilGitHub Check: Lint Code Base
[warning] 2-2:
W0611: Unused import shutil (unused-import)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
tests/e2e/detectors/test_data/unused-import/0.8.16/CrossDomainMessenger.sol-0.8.16.zipis excluded by!**/*.zip
Files selected for processing (13)
- slither/core/scope/scope.py (2 hunks)
- slither/core/slither_core.py (1 hunks)
- slither/detectors/statements/unused_import.py (1 hunks)
- slither/slither.py (1 hunks)
- slither/solc_parsing/slither_compilation_unit_solc.py (2 hunks)
- tests/e2e/detectors/snapshots/detectors__detector_UnusedImport_0_8_16_CrossDomainMessenger_sol__0.txt (1 hunks)
- tests/e2e/detectors/test_data/unused-import/0.8.16/Constants.sol (1 hunks)
- tests/e2e/detectors/test_data/unused-import/0.8.16/CrossDomainMessenger.sol (1 hunks)
- tests/e2e/detectors/test_data/unused-import/0.8.16/lib/ResourceMetering.sol (1 hunks)
- tests/e2e/detectors/test_data/unused-import/0.8.16/utils/console.sol (1 hunks)
- tests/e2e/detectors/test_data/unused-import/0.8.16/utils/original/Initializable.sol (1 hunks)
- tests/e2e/detectors/test_data/unused-import/0.8.16/utils/upgradeable/Initializable.sol (1 hunks)
- tests/e2e/detectors/test_detectors.py (2 hunks)
Files skipped from review due to trivial changes (2)
- tests/e2e/detectors/snapshots/detectors__detector_UnusedImport_0_8_16_CrossDomainMessenger_sol__0.txt
- tests/e2e/detectors/test_data/unused-import/0.8.16/utils/console.sol
Additional context used
Ruff
slither/detectors/statements/unused_import.py
61-64: Use
return all(v.file_scope != scope for v in scope.variables.values())instead offorloop (SIM110)Replace with
return all(v.file_scope != scope for v in scope.variables.values())slither/core/scope/scope.py
24-24: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
145-145: Do not assign a
lambdaexpression, use adef(E731)Rewrite
getteras adef
163-163: Do not assign a
lambdaexpression, use adef(E731)Rewrite
getteras adef
181-181: Do not assign a
lambdaexpression, use adef(E731)Rewrite
getteras adef
199-199: Do not assign a
lambdaexpression, use adef(E731)Rewrite
getteras adef
215-215: Do not assign a
lambdaexpression, use adef(E731)Rewrite
getteras adefslither/slither.py
144-144: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)slither/core/slither_core.py
478-479: Use a single
ifstatement instead of nestedifstatements (SIM102)slither/solc_parsing/slither_compilation_unit_solc.py
198-201: Use
canonicalName = top_level_data["attributes"].get("canonicalName", name)instead of anifblock (SIM401)Replace with
canonicalName = top_level_data["attributes"].get("canonicalName", name)
434-440: Use a single
ifstatement instead of nestedifstatements (SIM102)
444-444: Test for membership should be
not in(E713)Convert to
not in
448-448: Test for membership should be
not in(E713)Convert to
not intests/e2e/detectors/test_detectors.py
2-2:
shutilimported but unused (F401)Remove unused import:
shutil
1949-1950: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
GitHub Check: Lint Code Base
tests/e2e/detectors/test_detectors.py
[warning] 2-2:
W0611: Unused import shutil (unused-import)
Additional comments not posted (15)
tests/e2e/detectors/test_data/unused-import/0.8.16/Constants.sol (2)
42-52: TheDEFAULT_RESOURCE_CONFIGfunction is well-defined and returns a struct with appropriate values.
4-4: Ensure the importedResourceMeteringlibrary is utilized within this file.Verification successful
The
ResourceMeteringlibrary is indeed utilized within theConstants.solfile. The search results show that it is used in theDEFAULT_RESOURCE_CONFIGfunction.
ResourceMetering.ResourceConfig memory config = ResourceMetering.ResourceConfig({ ... });This confirms that the import is necessary and correctly used.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the imported `ResourceMetering` library is utilized in `Constants.sol`. # Test: Search for the usage of `ResourceMetering` in `Constants.sol`. Expect: At least one usage. rg --type solidity 'ResourceMetering' tests/e2e/detectors/test_data/unused-import/0.8.16/Constants.solLength of output: 135
Script:
#!/bin/bash # Description: Verify that the imported `ResourceMetering` library is utilized in `Constants.sol`. # Test: Search for the usage of `ResourceMetering` in `Constants.sol`. Expect: At least one usage. rg 'ResourceMetering' tests/e2e/detectors/test_data/unused-import/0.8.16/Constants.solLength of output: 343
slither/detectors/statements/unused_import.py (1)
Line range hint
95-126: The updated logic in_detectmethod efficiently checks for unused imports and handles multiple edge cases, such as import containers and transitive imports.tests/e2e/detectors/test_data/unused-import/0.8.16/utils/original/Initializable.sol (3)
54-82: Theinitializermodifier is correctly implemented to ensure that initialization can only happen once or during construction.
102-114: Thereinitializermodifier properly handles reinitialization with version control, ensuring that reinitialization can only occur if the contract hasn't been initialized to a greater version.
142-152: The_disableInitializersfunction effectively locks the contract from any future reinitialization, which is crucial for security in upgradeable contracts.tests/e2e/detectors/test_data/unused-import/0.8.16/lib/ResourceMetering.sol (1)
61-131: Themeteredmodifier and the_meteredfunction are well-implemented to handle resource metering based on EIP-1559. The logic for updating gas parameters and handling resource costs is correctly handled.slither/core/scope/scope.py (2)
32-32: Changeexported_symbolsfrom a set to a list to maintain order.This change ensures that the order of symbols is preserved, which is crucial for resolving symbols correctly in the presence of duplicates.
85-94: Ensure that duplicate symbols are not added toexported_symbols.This logic prevents duplicate entries in
exported_symbolsby checking if a symbol is already present before adding it. This is a crucial fix for the bug described in the PR.tests/e2e/detectors/test_data/unused-import/0.8.16/utils/upgradeable/Initializable.sol (1)
1-228: Review of the entireInitializable.solfile.The file introduces a robust mechanism for handling contract upgrades and initializations, which is critical for upgradeable smart contracts. The use of versioning and initialization checks is well-implemented to prevent re-initialization vulnerabilities.
slither/slither.py (1)
59-64: Ensure contracts are added only if they are not already present.This change prevents duplicate contracts from being added to the scope, which aligns with the PR's goal to handle symbol resolution correctly.
tests/e2e/detectors/test_data/unused-import/0.8.16/CrossDomainMessenger.sol (1)
1-260: Review of the entireCrossDomainMessenger.solfile.The contract provides a detailed and well-documented implementation for cross-chain messaging. The use of constants and events is appropriate, and the security considerations are well addressed.
slither/core/slither_core.py (1)
254-254: Refactor to useaddfor clarity and safety in adding single items to a set.This change avoids potential errors when
thing.source_mappingis not iterable, which would cause the previous|=operation to fail.slither/solc_parsing/slither_compilation_unit_solc.py (1)
260-266: Efficiently handle duplicate symbols by filtering before extending the list.This change prevents duplicate symbols from being added to
scope.exported_symbols, aligning with the PR's objectives to handle such cases more effectively.tests/e2e/detectors/test_detectors.py (1)
1904-1908: Addition of a new test case forUnusedImportdetector usingCrossDomainMessenger.sol. Ensure the test file and version are correctly referenced and exist.
| @@ -1,9 +1,11 @@ | |||
| import os | |||
| import shutil | |||
There was a problem hiding this comment.
Remove unused import: shutil.
- import shutilCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import shutil |
Tools
Ruff
2-2:
shutilimported but unused (F401)Remove unused import:
shutil
GitHub Check: Lint Code Base
[warning] 2-2:
W0611: Unused import shutil (unused-import)
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
slither/core/scope/scope.py (2)
Line range hint
24-24: Optimize dictionary key check.- return all(item in d2_keys for item in d1.keys()) + return all(item in d2 for item in d1)This change simplifies the check by removing unnecessary calls to
.keys(), which is more Pythonic and slightly improves performance.
Line range hint
145-145: Replace lambda expressions with function definitions.- getter: Callable[[SourceUnit], Dict[str, str]] = lambda x: x.bytecodes_init + def get_bytecodes_init(x: SourceUnit) -> Dict[str, str]: + return x.bytecodes_init - getter: Callable[[SourceUnit], Dict[str, str]] = lambda x: x.bytecodes_runtime + def get_bytecodes_runtime(x: SourceUnit) -> Dict[str, str]: + return x.bytecodes_runtime - getter: Callable[[SourceUnit], Dict[str, List[str]]] = lambda x: x.srcmaps_init + def get_srcmaps_init(x: SourceUnit) -> Dict[str, List[str]]: + return x.srcmaps_init - getter: Callable[[SourceUnit], Dict[str, List[str]]] = lambda x: x.srcmaps_runtime + def get_srcmaps_runtime(x: SourceUnit) -> Dict[str, List[str]]: + return x.srcmaps_runtime - getter: Callable[[SourceUnit], Dict[str, List[str]]] = lambda x: x.abis + def get_abis(x: SourceUnit) -> Dict[str, List[str]]: + return x.abisConverting lambda expressions to function definitions improves readability and maintainability of the code.
Also applies to: 163-163, 181-181, 199-199, 215-215
slither/slither.py (1)
Line range hint
145-145: Useraise ... fromto provide context for exceptions.- raise SlitherError(f"Invalid compilation: \n{str(e)}") + raise SlitherError(f"Invalid compilation: \n{str(e)}") from eUsing
raise ... fromprovides better error context and helps in debugging by making the chain of exceptions clear.tests/e2e/detectors/test_detectors.py (1)
Line range hint
1948-1949: Optimize conditional checks in_generate_compilefunction.- if skip_existing: - if os.path.isfile(zip_artifact_path): + if skip_existing and os.path.isfile(zip_artifact_path):This change combines the nested
ifstatements into a single line, improving readability and reducing the complexity of the code.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- slither/core/scope/scope.py (2 hunks)
- slither/slither.py (1 hunks)
- tests/e2e/detectors/test_detectors.py (2 hunks)
Additional context used
Ruff
slither/core/scope/scope.py
24-24: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
145-145: Do not assign a
lambdaexpression, use adef(E731)Rewrite
getteras adef
163-163: Do not assign a
lambdaexpression, use adef(E731)Rewrite
getteras adef
181-181: Do not assign a
lambdaexpression, use adef(E731)Rewrite
getteras adef
199-199: Do not assign a
lambdaexpression, use adef(E731)Rewrite
getteras adef
215-215: Do not assign a
lambdaexpression, use adef(E731)Rewrite
getteras adefslither/slither.py
145-145: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)tests/e2e/detectors/test_detectors.py
1948-1949: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
Additional comments not posted (4)
slither/core/scope/scope.py (2)
32-32: Changeexported_symbolsfrom a set to a list to maintain order.This change is crucial for ensuring that symbols are processed in the order they are imported, which is necessary for the correct resolution of symbols when there are name collisions.
85-94: Ensure that duplicate symbols are not added toexported_symbols.This modification prevents the addition of duplicate symbols, which is essential for maintaining the integrity of the symbol table and avoiding potential bugs related to symbol resolution.
slither/slither.py (1)
59-65: Ensure contracts are added to the scope only if they are not already present.This change prevents the same contract from being added multiple times to the scope, which is essential for maintaining the correctness of the scope's state.
tests/e2e/detectors/test_detectors.py (1)
1903-1907: Added test case forUnusedImportdetector usingCrossDomainMessenger.sol.This addition aligns with the PR's objective to enhance the detection of unused imports. Ensure that
CrossDomainMessenger.solis correctly set up for this test.
PR Review: #2479 - Unused-imports bug fixSummaryThis PR addresses issue #2477, fixing a false positive in the Key Changes
Critical Issue (Blocking CI)The PR adds 39 test cases referencing # from .statements.unused_import import UnusedImportThis causes CI to fail with: Fix required: Uncomment the import in Performance ConsiderationThe change from
For projects with many exported symbols, this could impact performance. Consider maintaining both a list (for order) and a set (for O(1) lookups): self.exported_symbols: List[int] = []
self._exported_symbols_set: Set[int] = set()Additionally, line 84 creates a new set on each iteration, which is inefficient. A generator expression would be more efficient: if any(symbol not in self.exported_symbols for symbol in new_scope.exported_symbols):Test CoverageThe snapshot shows the detector correctly identifies the unused VerdictThe fix correctly addresses the symbol shadowing issue by preserving insertion order. However, CI cannot pass until the |
- Uncomment UnusedImport import in all_detectors.py to fix CI - Replace inefficient set().issubset() with any() generator in scope.py Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add documentation explaining that exported_symbols uses a list (not set) to preserve insertion order for correct name resolution when multiple files export symbols with the same name. Co-Authored-By: Claude Opus 4.5 <[email protected]>
dguido
force-pushed
the
fix/unused-imports
branch
from
864b67d to
623d90a
Compare
The bug
It is possible to import twice the same symbol in Solidity. This was uncovered with this pattern:
With the following layout
original/X,solandupgradable/X.sol:contract Initializable { }Counter.sol:Counter2.solIn the
Counter2.solscope, we have bothInitializablefromoriginal/Xandupgradable/X.The Fix
I went for a simple fix, that's maybe not perfect :
We are keeping the order in which symbols are seen within the
exported_symbolslist in theFileScopeclass. Because Python list keep the insertion order in place, we know which symbols came first.When resolving contract names, we are doing this in the same order and only adding it to the list of seen contracts if no other exist with the same name :
The test
A test is added to the repository to make sure this situation does not happen again. It follows the same pattern as the one described above.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
UnusedImportdetector.Refactor