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.

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

Unused-imports bug fix #2479

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Unused-imports bug fix #2479

wants to merge 5 commits into from

Conversation

DarkaMaul
Copy link
Contributor

@DarkaMaul DarkaMaul commented Jun 10, 2024

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.sol
upgradable/X.sol
Counter.sol
Counter2.sol

original/X,sol and upgradable/X.sol:
contract Initializable { }

Counter.sol:

import { Initializable } from "./original/Initializable.sol";
contract Counter is Initializable {}

Counter2.sol

import {Counter} from "./Counter.sol";
import {Initializable} from "./upgradable/Initializable.sol";
contract SuperCounter is Counter, Initializable {}

In the Counter2.sol scope, we have both Initializable from original/X and upgradable/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_symbols list in the FileScope class. 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 :

for refId in scope.exported_symbols:
    if refId in sol_parser.contracts_by_id:
        # Only add elements if they are not present. Since we kept the exported symbols in
        # we resolve from the most local imports first.
        if contract.name not in scope.contracts:
            scope.contracts[contract.name] = contract

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

    • Introduced several new Solidity contracts for resource metering, cross-chain messaging, and contract initialization.
  • Bug Fixes

    • Enhanced the logic for detecting unused imports, ensuring more accurate identification.
  • Tests

    • Added new test cases for the UnusedImport detector.
    • Included new Solidity test files to support end-to-end testing of import detection.
  • Refactor

    • Improved the handling and merging of exported symbols to maintain order and avoid duplicates.

Copy link

coderabbitai bot commented Jun 10, 2024

Walkthrough

Walkthrough

The recent changes focus on improving the handling of exported symbols within the FileScope class by switching from a set to a list, ensuring order preservation. It also enhances the _update_file_scopes function to prioritize local imports and refines the detection of unused imports. Additionally, new Solidity contracts and libraries for various functionalities are introduced, along with corresponding test cases for the UnusedImport detector.

Changes

Files/Paths Change Summary
slither/core/scope/scope.py Modified FileScope class to use a list for exported_symbols instead of a set.
slither/slither.py Updated _update_file_scopes function to check for existing contract names before adding.
slither/detectors/statements/unused_import.py Changed iteration logic in _detect method to iterate over values instead of items.
slither/solc_parsing/slither_compilation_unit_solc.py Adjusted parse_top_level_items method to filter existing symbols before extending the list.
tests/e2e/detectors/test_data/unused-import/... Introduced various new Solidity contracts and libraries for different functionalities.
tests/e2e/detectors/test_detectors.py Added new test case for UnusedImport detector and imported shutil module.

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
Loading

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@DarkaMaul DarkaMaul changed the base branch from master to dev June 10, 2024 15:35
@DarkaMaul DarkaMaul marked this pull request as draft June 10, 2024 15:37
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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_init

Repeat 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_analyses method.

- except Exception as e:
+ except Exception as e:
+     raise e from None

Use raise ... from err to provide more context in the traceback, which can help in debugging.

slither/core/slither_core.py (1)

Line range hint 478-479: Consider simplifying nested if statements into a single if statement.

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 of canonicalName using 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 get method with a default value.


Line range hint 434-440: Consider merging nested if statements into a single if statement for clarity.

This change would simplify the control flow and enhance the readability of the code.


Line range hint 444-444: Use not in for 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 combining if statements.

-    if skip_existing:
-        if os.path.isfile(zip_artifact_path):
+    if skip_existing and os.path.isfile(zip_artifact_path):
Tools
Ruff

2-2: shutil imported but unused (F401)

Remove unused import: shutil

GitHub Check: Lint Code Base

[warning] 2-2:
W0611: Unused import shutil (unused-import)

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 798c1f6 and 1fd3989.

Files ignored due to path filters (1)
  • tests/e2e/detectors/test_data/unused-import/0.8.16/CrossDomainMessenger.sol-0.8.16.zip is 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 of for loop (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 dict instead of key in dict.keys() (SIM118)

Remove .keys()


145-145: Do not assign a lambda expression, use a def (E731)

Rewrite getter as a def


163-163: Do not assign a lambda expression, use a def (E731)

Rewrite getter as a def


181-181: Do not assign a lambda expression, use a def (E731)

Rewrite getter as a def


199-199: Do not assign a lambda expression, use a def (E731)

Rewrite getter as a def


215-215: Do not assign a lambda expression, use a def (E731)

Rewrite getter as a def

slither/slither.py

144-144: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling (B904)

slither/core/slither_core.py

478-479: Use a single if statement instead of nested if statements (SIM102)

slither/solc_parsing/slither_compilation_unit_solc.py

198-201: Use canonicalName = top_level_data["attributes"].get("canonicalName", name) instead of an if block (SIM401)

Replace with canonicalName = top_level_data["attributes"].get("canonicalName", name)


434-440: Use a single if statement instead of nested if statements (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 in

tests/e2e/detectors/test_detectors.py

2-2: shutil imported but unused (F401)

Remove unused import: shutil


1949-1950: Use a single if statement instead of nested if statements (SIM102)

Combine if statements using and

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: The DEFAULT_RESOURCE_CONFIG function is well-defined and returns a struct with appropriate values.


4-4: Ensure the imported ResourceMetering library is utilized within this file.

Verification successful

The ResourceMetering library is indeed utilized within the Constants.sol file. The search results show that it is used in the DEFAULT_RESOURCE_CONFIG function.

  • 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.sol

Length 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.sol

Length of output: 343

slither/detectors/statements/unused_import.py (1)

Line range hint 95-126: The updated logic in _detect method 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: The initializer modifier is correctly implemented to ensure that initialization can only happen once or during construction.


102-114: The reinitializer modifier 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 _disableInitializers function 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: The metered modifier and the _metered function 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: Change exported_symbols from 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 to exported_symbols.

This logic prevents duplicate entries in exported_symbols by 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 entire Initializable.sol file.

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 entire CrossDomainMessenger.sol file.

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 use add for clarity and safety in adding single items to a set.

This change avoids potential errors when thing.source_mapping is 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 for UnusedImport detector using CrossDomainMessenger.sol. Ensure the test file and version are correctly referenced and exist.

@@ -1,9 +1,11 @@
import os
import shutil
Copy link

Choose a reason for hiding this comment

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

Remove unused import: shutil.

- import shutil
Committable 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.

Suggested change
import shutil
Tools
Ruff

2-2: shutil imported but unused (F401)

Remove unused import: shutil

GitHub Check: Lint Code Base

[warning] 2-2:
W0611: Unused import shutil (unused-import)

@DarkaMaul DarkaMaul changed the title Unused bug fix Unused-imports bug fix Jun 10, 2024
@DarkaMaul DarkaMaul marked this pull request as ready for review June 11, 2024 10:16
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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.abis

Converting 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: Use raise ... from to provide context for exceptions.

- raise SlitherError(f"Invalid compilation: \n{str(e)}")
+ raise SlitherError(f"Invalid compilation: \n{str(e)}") from e

Using raise ... from provides 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_compile function.

- 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 if statements into a single line, improving readability and reducing the complexity of the code.

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 1fd3989 and 89af1d8.

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 dict instead of key in dict.keys() (SIM118)

Remove .keys()


145-145: Do not assign a lambda expression, use a def (E731)

Rewrite getter as a def


163-163: Do not assign a lambda expression, use a def (E731)

Rewrite getter as a def


181-181: Do not assign a lambda expression, use a def (E731)

Rewrite getter as a def


199-199: Do not assign a lambda expression, use a def (E731)

Rewrite getter as a def


215-215: Do not assign a lambda expression, use a def (E731)

Rewrite getter as a def

slither/slither.py

145-145: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling (B904)

tests/e2e/detectors/test_detectors.py

1948-1949: Use a single if statement instead of nested if statements (SIM102)

Combine if statements using and

Additional comments not posted (4)
slither/core/scope/scope.py (2)

32-32: Change exported_symbols from 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 to exported_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 for UnusedImport detector using CrossDomainMessenger.sol.

This addition aligns with the PR's objective to enhance the detection of unused imports. Ensure that CrossDomainMessenger.sol is correctly set up for this test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants