Overload (and other ambiguities) resolution and many fixes in the new binder (#1426)

This PR adds functionality to resolve function overloads and many other
ambiguities that were not possible to resolve with the stack graph
binder. It also fixes a lot of issues found via running the new binder
against contracts from sourcify (see #1419) as well as comparing the
bindings output by `solc`. The resulting binding information should be
much better than that obtained by the stack graph binder. Notably:

- function overloads are resolved by matching the type (and count) of
arguments
- `super` calls should resolve considering the current contract as the
target
- `using` directives consider the full type to which they apply
(previously eg. function types were not discernible)
- full and correct public getters types are created for complex data
types
- all built-ins should type correctly, even in tricky cases such as
`.push()` for arrays
- `abi.decode` can now be typed correctly

We're still not doing full validation, so the binder may return
incorrect results eg. for cases with no ambiguity but a type mismatch.
But the valid Solidity code, the expectation is that the new binder is
correct.

A lot of new snapshot cases are added in this PR, mostly to test
specific functionality or cases that the new binder handles. In many of
these, the old binder will produce incorrect output, so the diff reports
are expected to grow in number significantly.

---------

Co-authored-by: Beta Ziliani <beta@manas.tech>

Author: ggiraldez

Cargo.lock

@@ -74,11 +74,20 @@ pub struct EventDefinition {
     pub parameters_scope_id: ScopeId,
 }
 
+#[derive(Debug, Eq, PartialEq)]
+pub enum FunctionVisibility {
+    External,
+    Internal,
+    Private,
+    Public,
+}
+
 #[derive(Debug)]
 pub struct FunctionDefinition {
     pub node_id: NodeId,
     pub identifier: Rc<TerminalNode>,
     pub parameters_scope_id: ScopeId,
+    pub visibility: FunctionVisibility,
 }
 
 #[derive(Debug)]
@@ -121,11 +130,19 @@ pub struct ParameterDefinition {
     pub identifier: Rc<TerminalNode>,
 }
 
+#[derive(Debug, Eq, PartialEq)]
+pub enum StateVariableVisibility {
+    Internal,
+    Private,
+    Public,
+}
+
 #[derive(Debug)]
 pub struct StateVariableDefinition {
     pub node_id: NodeId,
     pub identifier: Rc<TerminalNode>,
     pub getter_type_id: Option<TypeId>,
+    pub visibility: StateVariableVisibility,
 }
 
 #[derive(Debug)]
@@ -240,6 +257,40 @@ impl Definition {
         }
     }
 
+    pub(crate) fn is_private_or_internally_visible(&self) -> bool {
+        if let Self::Function(function_definition) = self {
+            function_definition.visibility != FunctionVisibility::External
+        } else {
+            true
+        }
+    }
+
+    pub(crate) fn is_internally_visible(&self) -> bool {
+        match self {
+            Self::Function(function_definition) => {
+                function_definition.visibility == FunctionVisibility::Internal
+                    || function_definition.visibility == FunctionVisibility::Public
+            }
+            Self::StateVariable(variable_definition) => {
+                variable_definition.visibility != StateVariableVisibility::Private
+            }
+            _ => true,
+        }
+    }
+
+    pub(crate) fn is_externally_visible(&self) -> bool {
+        match self {
+            Self::Function(function_definition) => {
+                function_definition.visibility == FunctionVisibility::Public
+                    || function_definition.visibility == FunctionVisibility::External
+            }
+            Self::StateVariable(variable_definition) => {
+                variable_definition.visibility == StateVariableVisibility::Public
+            }
+            _ => true,
+        }
+    }
+
     pub(crate) fn new_constant(node_id: NodeId, identifier: &Rc<TerminalNode>) -> Self {
         Self::Constant(ConstantDefinition {
             node_id,
@@ -298,11 +349,13 @@ impl Definition {
         node_id: NodeId,
         identifier: &Rc<TerminalNode>,
         parameters_scope_id: ScopeId,
+        visibility: FunctionVisibility,
     ) -> Self {
         Self::Function(FunctionDefinition {
             node_id,
             identifier: Rc::clone(identifier),
             parameters_scope_id,
+            visibility,
         })
     }
 
@@ -361,11 +414,16 @@ impl Definition {
         })
     }
 
-    pub(crate) fn new_state_variable(node_id: NodeId, identifier: &Rc<TerminalNode>) -> Self {
+    pub(crate) fn new_state_variable(
+        node_id: NodeId,
+        identifier: &Rc<TerminalNode>,
+        visibility: StateVariableVisibility,
+    ) -> Self {
         Self::StateVariable(StateVariableDefinition {
             node_id,
             identifier: Rc::clone(identifier),
             getter_type_id: None,
+            visibility,
         })
     }
 

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

@@ -10,11 +10,14 @@ mod scopes;
 
 pub use definitions::Definition;
 pub(crate) use definitions::{
-    ContractDefinition, ImportDefinition, InterfaceDefinition, LibraryDefinition,
+    ContractDefinition, FunctionVisibility, ImportDefinition, InterfaceDefinition,
+    LibraryDefinition, StateVariableVisibility,
 };
 pub use references::{Reference, Resolution};
 use scopes::ContractScope;
-pub(crate) use scopes::{EitherIter, FileScope, Scope, UsingDirective};
+pub(crate) use scopes::{
+    EitherIter, FileScope, ParameterDefinition, ParametersScope, Scope, UsingDirective,
+};
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub struct ScopeId(usize);
@@ -108,14 +111,21 @@ pub struct Binder {
     linearisations: HashMap<NodeId, Vec<NodeId>>,
 }
 
-// This controls how to use the linearisation when performing a contract scope
-// resolution. `Normal` uses all the linearised bases in order; `This(node_id)`
-// starts at `node_id` and proceeds in order; `Super(node_id)` starts at the
-// contract _following_ `node_id` (ie. its parent)
+/// This controls visibility filtering and how to use the linearisation when
+/// performing a contract scope resolution.
 #[derive(Clone, Copy, Eq, PartialEq)]
 pub(crate) enum ResolveOptions {
-    Normal,
+    /// Use all the linearised bases in order for an internal lookup.
+    Internal,
+    /// Use all the linearised bases for an external lookup.
+    External,
+    /// Use all bases and only excludes private members in bases types (used for
+    /// general qualified member access).
+    Qualified,
+    /// Starts at `node_id` and proceeds in order for an external lookup.
     This(NodeId),
+    /// Starts at the contract _following_ `node_id` (ie. its parent) for an
+    /// internal lookup.
     Super(NodeId),
 }
 
@@ -212,6 +222,11 @@ impl Binder {
         }
     }
 
+    #[cfg(feature = "__private_testing_utils")]
+    pub(crate) fn get_linearised_bases(&self, node_id: NodeId) -> Option<&Vec<NodeId>> {
+        self.linearisations.get(&node_id)
+    }
+
     #[cfg(feature = "__private_testing_utils")]
     pub fn linearisations(&self) -> &HashMap<NodeId, Vec<NodeId>> {
         &self.linearisations
@@ -222,6 +237,11 @@ impl Binder {
         self.references.insert(node_id, reference);
     }
 
+    pub(crate) fn fixup_reference(&mut self, node_id: NodeId, resolution: Resolution) {
+        let reference = self.references.get_mut(&node_id).unwrap();
+        reference.resolution = resolution;
+    }
+
     pub fn find_reference_by_identifier_node_id(&self, node_id: NodeId) -> Option<&Reference> {
         self.references.get(&node_id)
     }
@@ -300,6 +320,14 @@ impl Binder {
         }
     }
 
+    pub(crate) fn fixup_node_typing(&mut self, node_id: NodeId, typing: Typing) {
+        assert!(
+            self.node_typing.contains_key(&node_id),
+            "typing information for node {node_id:?} not set"
+        );
+        self.node_typing.insert(node_id, typing);
+    }
+
     // File scope resolution context
 
     fn get_file_scope(&self, file_id: &str) -> &FileScope {
@@ -344,7 +372,9 @@ impl Binder {
         // Search for the symbol in the linearised bases *in-order*.
         if let Some(linearisations) = self.linearisations.get(&contract_scope.node_id) {
             let linearisations = match options {
-                ResolveOptions::Normal => linearisations.as_slice(),
+                ResolveOptions::Internal | ResolveOptions::External | ResolveOptions::Qualified => {
+                    linearisations.as_slice()
+                }
                 ResolveOptions::This(node_id) => {
                     if let Some(index) = linearisations.iter().position(|id| *id == node_id) {
                         &linearisations[index..]
@@ -361,7 +391,7 @@ impl Binder {
                 }
             };
             let mut results = Vec::new();
-            for node_id in linearisations {
+            for (index, node_id) in linearisations.iter().enumerate() {
                 let Some(base_scope_id) = self.scope_id_for_node_id(*node_id) else {
                     continue;
                 };
@@ -372,7 +402,30 @@ impl Binder {
                 let Some(definitions) = base_contract_scope.definitions.get(symbol) else {
                     continue;
                 };
-                results.extend(definitions);
+                for definition_id in definitions {
+                    let definition = &self.definitions[definition_id];
+                    let visible = match options {
+                        ResolveOptions::Internal => {
+                            if index == 0 {
+                                definition.is_private_or_internally_visible()
+                            } else {
+                                definition.is_internally_visible()
+                            }
+                        }
+                        ResolveOptions::Qualified => {
+                            index == 0
+                                || definition.is_internally_visible()
+                                || definition.is_externally_visible()
+                        }
+                        ResolveOptions::This(_) | ResolveOptions::External => {
+                            definition.is_externally_visible()
+                        }
+                        ResolveOptions::Super(_) => definition.is_internally_visible(),
+                    };
+                    if visible {
+                        results.push(*definition_id);
+                    }
+                }
             }
             Resolution::from(results)
         } else if let Some(definitions) = contract_scope.definitions.get(symbol) {
@@ -395,7 +448,7 @@ impl Binder {
                 self.resolve_in_contract_scope_internal(
                     contract_scope,
                     symbol,
-                    ResolveOptions::Normal,
+                    ResolveOptions::Internal,
                 )
                 .or_else(|| {
                     // Otherwise, delegate to the containing file scope.
@@ -423,7 +476,9 @@ impl Binder {
                     )
                 }
             }
-            Scope::Parameters(parameters_scope) => parameters_scope.definitions.get(symbol).into(),
+            Scope::Parameters(parameters_scope) => {
+                parameters_scope.lookup_definition(symbol).into()
+            }
             Scope::Struct(struct_scope) => struct_scope.definitions.get(symbol).into(),
             Scope::Using(using_scope) => using_scope
                 .symbols
@@ -524,7 +579,7 @@ impl Binder {
             Scope::Contract(contract_scope) => self.resolve_in_contract_scope_internal(
                 contract_scope,
                 symbol,
-                ResolveOptions::Normal,
+                ResolveOptions::Qualified,
             ),
             Scope::Enum(enum_scope) => enum_scope.definitions.get(symbol).into(),
             Scope::Struct(struct_scope) => struct_scope.definitions.get(symbol).into(),
@@ -533,7 +588,9 @@ impl Binder {
                 .get(symbol)
                 .cloned()
                 .map_or(Resolution::Unresolved, Resolution::from),
-            Scope::Parameters(parameters_scope) => parameters_scope.definitions.get(symbol).into(),
+            Scope::Parameters(parameters_scope) => {
+                parameters_scope.lookup_definition(symbol).into()
+            }
 
             Scope::Block(_)
             | Scope::File(_)