Unused-imports bug fix

slither/core/scope/scope.py

@@ -29,7 +29,7 @@ class FileScope:
     def __init__(self, filename: Filename) -> None:
         self.filename = filename
         self.accessible_scopes: List[FileScope] = []
-        self.exported_symbols: Set[int] = set()
+        self.exported_symbols: List[int] = []
 
         self.contracts: Dict[str, Contract] = {}
         # Custom error are a list instead of a dict
@@ -82,8 +82,16 @@ def add_accessible_scopes(self) -> bool:  # pylint: disable=too-many-branches
             # To get around this bug for aliases https://github.com/ethereum/solidity/pull/11881,
             # we propagate the exported_symbols from the imported file to the importing file
             # See tests/e2e/solc_parsing/test_data/top-level-nested-import-0.7.1.sol
-            if not new_scope.exported_symbols.issubset(self.exported_symbols):
-                self.exported_symbols |= new_scope.exported_symbols
+            if not set(new_scope.exported_symbols).issubset(self.exported_symbols):
+                # We are using lists and specifically extending them to keep the order in which
+                # elements are added. This will come handy when we have name collisions.
+                # See issue : https://github.com/crytic/slither/issues/2477
+                new_symbols = [
+                    symbol
+                    for symbol in new_scope.exported_symbols
+                    if symbol not in self.exported_symbols
+                ]
+                self.exported_symbols.extend(new_symbols)
                 learn_something = True
 
             # This is need to support aliasing when we do a late lookup using SolidityImportPlaceholder

slither/detectors/statements/unused_import.py

@@ -92,9 +92,9 @@ def _detect(self) -> List[Output]:  # pylint: disable=too-many-branches
 
                     use_found = False
                     # Search through all references to the imported file
-                    for _, refs_to_imported_path in self.slither._offset_to_references[
+                    for refs_to_imported_path in self.slither._offset_to_references[
                         imported_path
-                    ].items():
+                    ].values():
                         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:

slither/slither.py

@@ -56,7 +56,13 @@ def _update_file_scopes(
         for refId in scope.exported_symbols:
             if refId in sol_parser.contracts_by_id:
                 contract = sol_parser.contracts_by_id[refId]
-                scope.contracts[contract.name] = contract
+
+                # Add elements only if they are not already present. By keeping the exported symbols
+                # in the order they were encountered, we ensure that the most local imports are
+                # resolved first.
+                if contract.name not in scope.contracts:
+                    scope.contracts[contract.name] = contract
+
             elif refId in sol_parser.functions_by_id:
                 functions = sol_parser.functions_by_id[refId]
                 assert len(functions) == 1

slither/solc_parsing/slither_compilation_unit_solc.py

@@ -3,6 +3,7 @@
 import logging
 import os
 import re
+from itertools import chain
 from pathlib import Path
 from typing import List, Dict
 
@@ -256,8 +257,13 @@ def parse_top_level_items(self, data_loaded: Dict, filename: str) -> None:
         scope = self.compilation_unit.get_scope(filename)
         # Exported symbols includes a reference ID to all top-level definitions the file exports,
         # including def's brought in by imports (even transitively) and def's local to the file.
-        for refId in exported_symbols.values():
-            scope.exported_symbols |= set(refId)
+
+        new_symbols = [
+            symbol
+            for symbol in chain.from_iterable(exported_symbols.values())
+            if symbol not in scope.exported_symbols
+        ]
+        scope.exported_symbols.extend(new_symbols)
 
         for top_level_data in data_loaded[self.get_children()]:
             if top_level_data[self.get_key()] == "ContractDefinition":

tests/e2e/detectors/snapshots/detectors__detector_UnusedImport_0_8_16_CrossDomainMessenger_sol__0.txt

@@ -0,0 +1,3 @@
+The following unused import(s) in tests/e2e/detectors/test_data/unused-import/0.8.16/CrossDomainMessenger.sol should be removed:
+	-import "./utils/console.sol"; (tests/e2e/detectors/test_data/unused-import/0.8.16/CrossDomainMessenger.sol#6)
+

tests/e2e/detectors/test_data/unused-import/0.8.16/Constants.sol

@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.16;
+
+import { ResourceMetering } from "./lib/ResourceMetering.sol";
+
+/// @title Constants
+/// @notice Constants is a library for storing constants. Simple! Don't put everything in here, just
+///         the stuff used in multiple contracts. Constants that only apply to a single contract
+///         should be defined in that contract instead.
+library Constants {
+    /// @notice Special address to be used as the tx origin for gas estimation calls in the
+    ///         OptimismPortal and CrossDomainMessenger calls. You only need to use this address if
+    ///         the minimum gas limit specified by the user is not actually enough to execute the
+    ///         given message and you're attempting to estimate the actual necessary gas limit. We
+    ///         use address(1) because it's the ecrecover precompile and therefore guaranteed to
+    ///         never have any code on any EVM chain.
+    address internal constant ESTIMATION_ADDRESS = address(1);
+
+    /// @notice Value used for the L2 sender storage slot in both the OptimismPortal and the
+    ///         CrossDomainMessenger contracts before an actual sender is set. This value is
+    ///         non-zero to reduce the gas cost of message passing transactions.
+    address internal constant DEFAULT_L2_SENDER = 0x000000000000000000000000000000000000dEaD;
+
+    /// @notice The storage slot that holds the address of a proxy implementation.
+    /// @dev `bytes32(uint256(keccak256('eip1967.proxy.implementation')) - 1)`
+    bytes32 internal constant PROXY_IMPLEMENTATION_ADDRESS =
+        0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;
+
+    /// @notice The storage slot that holds the address of the owner.
+    /// @dev `bytes32(uint256(keccak256('eip1967.proxy.admin')) - 1)`
+    bytes32 internal constant PROXY_OWNER_ADDRESS = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103;
+
+    /// @notice The address that represents ether when dealing with ERC20 token addresses.
+    address internal constant ETHER = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
+
+    /// @notice The address that represents the system caller responsible for L1 attributes
+    ///         transactions.
+    address internal constant DEPOSITOR_ACCOUNT = 0xDeaDDEaDDeAdDeAdDEAdDEaddeAddEAdDEAd0001;
+
+    /// @notice Returns the default values for the ResourceConfig. These are the recommended values
+    ///         for a production network.
+    function DEFAULT_RESOURCE_CONFIG() internal pure returns (ResourceMetering.ResourceConfig memory) {
+        ResourceMetering.ResourceConfig memory config = ResourceMetering.ResourceConfig({
+            maxResourceLimit: 20_000_000,
+            elasticityMultiplier: 10,
+            baseFeeMaxChangeDenominator: 8,
+            minimumBaseFee: 1 gwei,
+            systemTxMaxGas: 1_000_000,
+            maximumBaseFee: type(uint128).max
+        });
+        return config;
+    }
+}