Retain references to ImportDeconstructionSymbols instead of following to the aliased definition (#1510)

If an identifier resolves lexically to an `ImportDeconstructionSymbol`
definition, register that instead of skipping it and registering to the
actual aliased definition. This allows keeping better track of
referenced imports, which would otherwise appear to have no references
pointing to them.

The drawback is that we need to explicitly follow through the aliases
whenever we need access to the actual definition, eg. when resolving
types, members, etc.

---------

Co-authored-by: Teodoro Freund <tfreund95@gmail.com>

Author: ggiraldez

crates/solidity/outputs/cargo/crate/src/backend/binder/mod.rs

@@ -481,11 +481,14 @@ impl Binder {
         }
     }
 
-    fn resolve_in_scope_internal(&self, scope_id: ScopeId, symbol: &str) -> Resolution {
+    // This function attempts to lexically resolve `symbol` starting from the
+    // given scope. Certain scopes can delegate to their "parent" scopes if
+    // the symbol is not found there.
+    pub(crate) fn resolve_in_scope(&self, scope_id: ScopeId, symbol: &str) -> Resolution {
         let scope = self.get_scope_by_id(scope_id);
         match scope {
             Scope::Block(block_scope) => block_scope.definitions.get(symbol).copied().map_or_else(
-                || self.resolve_in_scope_internal(block_scope.parent_scope_id, symbol),
+                || self.resolve_in_scope(block_scope.parent_scope_id, symbol),
                 Resolution::Definition,
             ),
             Scope::Contract(contract_scope) => {
@@ -496,7 +499,7 @@ impl Binder {
                 )
                 .or_else(|| {
                     // Otherwise, delegate to the containing file scope.
-                    self.resolve_in_scope_internal(contract_scope.file_scope_id, symbol)
+                    self.resolve_in_scope(contract_scope.file_scope_id, symbol)
                 })
             }
             Scope::Enum(enum_scope) => enum_scope.definitions.get(symbol).into(),
@@ -506,16 +509,16 @@ impl Binder {
                 .get(symbol)
                 .copied()
                 .map_or_else(
-                    || self.resolve_in_scope_internal(function_scope.parameters_scope_id, symbol),
+                    || self.resolve_in_scope(function_scope.parameters_scope_id, symbol),
                     Resolution::Definition,
                 )
-                .or_else(|| self.resolve_in_scope_internal(function_scope.parent_scope_id, symbol)),
+                .or_else(|| self.resolve_in_scope(function_scope.parent_scope_id, symbol)),
             Scope::Modifier(modifier_scope) => {
                 if symbol == "_" {
                     Resolution::BuiltIn(BuiltIn::ModifierUnderscore)
                 } else {
                     modifier_scope.definitions.get(symbol).copied().map_or_else(
-                        || self.resolve_in_scope_internal(modifier_scope.parent_scope_id, symbol),
+                        || self.resolve_in_scope(modifier_scope.parent_scope_id, symbol),
                         Resolution::Definition,
                     )
                 }
@@ -534,7 +537,7 @@ impl Binder {
                 .get(symbol)
                 .copied()
                 .map_or_else(
-                    || self.resolve_in_scope_internal(yul_block_scope.parent_scope_id, symbol),
+                    || self.resolve_in_scope(yul_block_scope.parent_scope_id, symbol),
                     Resolution::Definition,
                 ),
             Scope::YulFunction(yul_function_scope) => {
@@ -545,40 +548,33 @@ impl Binder {
                     .get(symbol)
                     .copied()
                     .map_or_else(
-                        || {
-                            self.resolve_in_scope_internal(
-                                yul_function_scope.parent_scope_id,
-                                symbol,
-                            )
-                        },
+                        || self.resolve_in_scope(yul_function_scope.parent_scope_id, symbol),
                         Resolution::Definition,
                     )
             }
         }
     }
 
-    // This will attempt to lexically resolve `symbol` starting from the given
-    // scope. This means that scopes can delegate to their "parent" scopes if
-    // the symbol is not found there, and also that imported symbols are
-    // followed recursively. This function handles the latter (following
-    // imported symbols recursively), while `resolve_in_scope_internal`
-    // delegates to parent or otherwise linked scopes.
-    pub(crate) fn resolve_in_scope(&self, scope_id: ScopeId, symbol: &str) -> Resolution {
+    // If the resolution points to definitions that are symbols aliases (eg.
+    // import deconstruction symbols) this function will recursively follow them
+    // and return an appropriate resulting resolution. Ambiguities and missing
+    // definitions can occur along the followed aliases, so there is no
+    // guarantee that a `Resolved` resolution will yield another `Resolved`
+    // resolution.
+    pub(crate) fn follow_symbol_aliases(&self, resolution: &Resolution) -> Resolution {
+        match resolution {
+            Resolution::Unresolved | Resolution::BuiltIn(_) => return resolution.clone(),
+            _ => {}
+        }
+
         // TODO: since this function uses the results from other resolution
         // functions, we making more allocations than necessary; it may be worth
         // it to try and avoid them by returning iterators from the delegated
         // resolution functions
         let mut found_ids = Vec::new();
-        let mut working_set = Vec::new();
         let mut seen_ids = HashSet::new();
+        let mut working_set = resolution.get_definition_ids();
 
-        let initial_resolution = self.resolve_in_scope_internal(scope_id, symbol);
-        match initial_resolution {
-            Resolution::Unresolved | Resolution::BuiltIn(_) => return initial_resolution,
-            _ => {}
-        }
-
-        working_set.extend(initial_resolution.get_definition_ids().iter().rev());
         while let Some(definition_id) = working_set.pop() {
             if !seen_ids.insert(definition_id) {
                 // we already processed this definition
@@ -597,16 +593,17 @@ impl Binder {
                     continue;
                 };
                 working_set.extend(
-                    self.resolve_in_scope_internal(scope_id, &imported_symbol.symbol)
-                        .get_definition_ids()
-                        .iter()
-                        .rev(),
+                    self.resolve_in_scope(scope_id, &imported_symbol.symbol)
+                        .get_definition_ids(),
                 );
             } else {
                 found_ids.push(definition_id);
             }
         }
 
+        // reverse the result to maintain the original ordering, since we
+        // resolved from last to first in the loop above
+        found_ids.reverse();
         Resolution::from(found_ids)
     }
 

crates/solidity/outputs/cargo/crate/src/backend/ir/ast/node_extensions/identifiers.rs

@@ -39,13 +39,26 @@ impl IdentifierStruct {
             .is_some()
     }
 
-    // only makes sense if `is_reference()` is true
+    /// Attempts to resolve the identifier to a definition, following symbol
+    /// aliases (import deconstructions). Returns `None` if the identifier is
+    /// not acting as a reference, or the reference cannot be resolved.
     pub fn resolve_to_definition(&self) -> Option<Definition> {
-        let reference = self
+        let definition_id = self
             .semantic
-            .binder()
-            .find_reference_by_identifier_node_id(self.ir_node.id())?;
-        let definition_id = reference.resolution.as_definition_id()?;
+            .resolve_reference_identifier_to_definition_id(self.ir_node.id())?;
+        Definition::try_create(definition_id, &self.semantic)
+    }
+
+    /// Attempts to resolve the identifier to an immediate definition, without
+    /// following symbol aliases, possibly returning import deconstruction
+    /// symbols. If the identifier refers to a direct definition, this is
+    /// equivalent to `resolve_to_definition()`. Returns `None` if the
+    /// identifier is not acting as a reference, or the reference cannot be
+    /// resolved.
+    pub fn resolve_to_immediate_definition(&self) -> Option<Definition> {
+        let definition_id = self
+            .semantic
+            .resolve_reference_identifier_to_immediate_definition_id(self.ir_node.id())?;
         Definition::try_create(definition_id, &self.semantic)
     }
 
@@ -137,6 +150,14 @@ impl Reference {
             }
         }
     }
+
+    pub fn resolve_to_immediate_definition(&self) -> Option<Definition> {
+        match self {
+            Reference::Identifier(identifier) | Reference::YulIdentifier(identifier) => {
+                identifier.resolve_to_immediate_definition()
+            }
+        }
+    }
 }
 
 impl SemanticAnalysis {
@@ -152,6 +173,25 @@ impl SemanticAnalysis {
             })
             .collect()
     }
+
+    fn resolve_reference_identifier_to_definition_id(&self, node_id: NodeId) -> Option<NodeId> {
+        let reference = self
+            .binder()
+            .find_reference_by_identifier_node_id(node_id)?;
+        self.binder()
+            .follow_symbol_aliases(&reference.resolution)
+            .as_definition_id()
+    }
+
+    fn resolve_reference_identifier_to_immediate_definition_id(
+        &self,
+        node_id: NodeId,
+    ) -> Option<NodeId> {
+        let reference = self
+            .binder()
+            .find_reference_by_identifier_node_id(node_id)?;
+        reference.resolution.as_definition_id()
+    }
 }
 
 impl IdentifierPathStruct {
@@ -163,13 +203,25 @@ impl IdentifierPathStruct {
             .join(".")
     }
 
+    /// Attempts to resolve the identifier path to a definition, following
+    /// symbol aliases (import deconstructions).
     pub fn resolve_to_definition(&self) -> Option<Definition> {
         let ir_node = self.ir_nodes.last()?;
-        let reference = self
+        let definition_id = self
             .semantic
-            .binder()
-            .find_reference_by_identifier_node_id(ir_node.id())?;
-        let definition_id = reference.resolution.as_definition_id()?;
+            .resolve_reference_identifier_to_definition_id(ir_node.id())?;
+        Definition::try_create(definition_id, &self.semantic)
+    }
+
+    /// Attempts to resolve the identifier path to an immediate definition,
+    /// without following symbol aliases, possibly returning import
+    /// deconstruction symbols. If the path refers to a direct definition, this
+    /// is equivalent to `resolve_to_definition`.
+    pub fn resolve_to_immediate_definition(&self) -> Option<Definition> {
+        let ir_node = self.ir_nodes.last()?;
+        let definition_id = self
+            .semantic
+            .resolve_reference_identifier_to_immediate_definition_id(ir_node.id())?;
         Definition::try_create(definition_id, &self.semantic)
     }
 }

crates/solidity/outputs/cargo/crate/src/backend/passes/p3_linearise_contracts/mod.rs

@@ -122,14 +122,26 @@ impl<'a> Pass<'a> {
             } else {
                 Resolution::Unresolved
             };
-            let definition_id = resolution.as_definition_id();
 
             let reference = Reference::new(Rc::clone(identifier), resolution.clone());
             self.binder.insert_reference(reference);
 
+            // Unless we used namespace resolution and in order to continue
+            // resolving the identifier path, we should ensure we've followed
+            // through any symbol alias (ie. import deconstruction symbol). This
+            // is not needed for namespaced resolution because there cannot be
+            // import directives inside contracts, interfaces or libraries which
+            // changes the lookup mode (see below).
+            let resolution = if use_lexical_resolution {
+                self.binder.follow_symbol_aliases(&resolution)
+            } else {
+                resolution
+            };
+
             // recurse into file scopes pointed by the resolved definition
             // to resolve the next identifier in the path
-            scope_id = definition_id
+            scope_id = resolution
+                .as_definition_id()
                 .and_then(|node_id| self.binder.find_definition_by_id(node_id))
                 .and_then(|definition| match definition {
                     Definition::Import(ImportDefinition {

crates/solidity/outputs/cargo/crate/src/backend/passes/p4_type_definitions/resolution.rs

@@ -32,14 +32,26 @@ impl Pass<'_> {
             } else {
                 Resolution::Unresolved
             };
-            let definition_id = resolution.as_definition_id();
 
             let reference = Reference::new(Rc::clone(identifier), resolution.clone());
             self.binder.insert_reference(reference);
 
+            // Unless we used namespace resolution and in order to continue
+            // resolving the identifier path, we should ensure we've followed
+            // through any symbol alias (ie. import deconstruction symbol). This
+            // is not needed for namespaced resolution because there cannot be
+            // import directives inside contracts, interfaces or libraries which
+            // changes the lookup mode (see below).
+            let resolution = if use_lexical_resolution {
+                self.binder.follow_symbol_aliases(&resolution)
+            } else {
+                resolution
+            };
+
             // recurse into file scopes pointed by the resolved definition
             // to resolve the next identifier in the path
-            scope_id = definition_id
+            scope_id = resolution
+                .as_definition_id()
                 .and_then(|node_id| self.binder.find_definition_by_id(node_id))
                 .and_then(|definition| match definition {
                     Definition::Import(ImportDefinition {
@@ -147,7 +159,13 @@ impl Pass<'_> {
                 self.binder
                     .find_reference_by_identifier_node_id(identifier.id())
             })
-            .and_then(|reference| reference.resolution.as_definition_id())?;
+            .and_then(|reference| {
+                // reference may resolve to an imported library, so we need to
+                // follow aliases
+                self.binder
+                    .follow_symbol_aliases(&reference.resolution)
+                    .as_definition_id()
+            })?;
 
         let Some(Definition::Library(_)) = self.binder.find_definition_by_id(definition_id) else {
             // the referenced definition is not a library

crates/solidity/outputs/cargo/crate/src/backend/passes/p4_type_definitions/typing.rs

@@ -16,7 +16,14 @@ impl Pass<'_> {
                 self.binder
                     .find_reference_by_identifier_node_id(identifier.id())
             })
-            .and_then(|reference| reference.resolution.as_definition_id())
+            .and_then(|reference| {
+                // We should follow symbol aliases here. This is only relevant
+                // if the path has a single element, as in other cases symbols
+                // cannot be aliased.
+                self.binder
+                    .follow_symbol_aliases(&reference.resolution)
+                    .as_definition_id()
+            })
             .and_then(|node_id| self.type_of_definition(node_id, data_location))
     }
 

crates/solidity/outputs/cargo/crate/src/backend/passes/p4_type_definitions/visitor.rs

@@ -87,15 +87,22 @@ impl Visitor for Pass<'_> {
 
     fn enter_import_deconstruction(&mut self, node: &input_ir::ImportDeconstruction) -> bool {
         for symbol in &node.symbols {
-            let target_symbol = if let Some(alias) = &symbol.alias {
-                alias
-            } else {
-                &symbol.name
+            // find the associated definition to get the imported file ID
+            let Some(Definition::ImportedSymbol(imported_symbol)) =
+                self.binder.find_definition_by_id(symbol.node_id)
+            else {
+                unreachable!("expected to find definition associated to imported symbol");
             };
-            let resolution = self.binder.resolve_in_scope(
-                self.current_contract_or_file_scope_id(),
-                &target_symbol.unparse(),
-            );
+            // now we can get the target scope ID
+            let scope_id = imported_symbol
+                .resolved_file_id
+                .as_ref()
+                .and_then(|file_id| self.binder.scope_id_for_file_id(file_id));
+
+            let resolution = scope_id.map_or(Resolution::Unresolved, |scope_id| {
+                self.binder
+                    .resolve_in_scope(scope_id, &symbol.name.unparse())
+            });
             let reference = Reference::new(Rc::clone(&symbol.name), resolution);
             self.binder.insert_reference(reference);
         }

crates/solidity/outputs/cargo/crate/src/backend/passes/p5_resolve_references/resolution.rs

@@ -411,7 +411,10 @@ impl Pass<'_> {
                 resolution
             };
 
-            scope_id = resolution
+            // NOTE: we need to follow symbol aliases to resolve the next scope to use
+            scope_id = self
+                .binder
+                .follow_symbol_aliases(&resolution)
                 .as_definition_id()
                 .and_then(|definition_id| self.binder.scope_id_for_node_id(definition_id));
 

crates/solidity/outputs/cargo/crate/src/backend/passes/p5_resolve_references/typing.rs

@@ -139,12 +139,14 @@ impl Pass<'_> {
     }
 
     fn typing_of_identifier(&self, identifier: &Rc<TerminalNode>) -> Typing {
-        let resolution = self
+        let resolution = &self
             .binder
             .find_reference_by_identifier_node_id(identifier.id())
             .unwrap()
-            .resolution
-            .clone();
+            .resolution;
+        // The resolution may point to an imported symbol, so we need to follow
+        // through in order to get to the actual typing
+        let resolution = self.binder.follow_symbol_aliases(resolution);
         self.typing_of_resolution(&resolution)
     }
 

crates/solidity/outputs/cargo/crate/src/backend/passes/p5_resolve_references/visitor.rs

@@ -635,7 +635,13 @@ impl Visitor for Pass<'_> {
             let definition_id = self
                 .binder
                 .find_reference_by_identifier_node_id(node.error.last().unwrap().id())
-                .and_then(|reference| reference.resolution.as_definition_id());
+                .and_then(|reference| {
+                    // Follow symbol aliases as the error type argument to
+                    // revert could be an imported symbol
+                    self.binder
+                        .follow_symbol_aliases(&reference.resolution)
+                        .as_definition_id()
+                });
             self.resolve_named_arguments(named_arguments, definition_id);
         }
     }