fix(semantic): Address remaining cases of undeclared identifier diagnostics

crates/solidity-v2/outputs/cargo/semantic/src/binder/definitions.rs

@@ -315,6 +315,34 @@ impl Definition {
         )
     }
 
+    /// Whether this is a function declared with a body.
+    pub(crate) fn is_implemented_function(&self) -> bool {
+        matches!(
+            self,
+            Self::Function(function_definition) if function_definition.ir_node.body.is_some()
+        )
+    }
+
+    /// Whether this is a function, state variable or constant declared
+    /// `private`.
+    pub(crate) fn is_private(&self) -> bool {
+        match self {
+            Self::Function(function_definition) => matches!(
+                function_definition.ir_node.attributes.visibility,
+                ir::FunctionVisibility::Private
+            ),
+            Self::StateVariable(variable_definition) => matches!(
+                variable_definition.ir_node.attributes.visibility,
+                ir::StateVariableVisibility::Private
+            ),
+            Self::Constant(constant_definition) => matches!(
+                constant_definition.ir_node.visibility,
+                Some(ir::StateVariableVisibility::Private)
+            ),
+            _ => false,
+        }
+    }
+
     pub(crate) fn is_private_or_internally_visible(&self) -> bool {
         if let Self::Function(function_definition) = self {
             !matches!(

crates/solidity-v2/outputs/cargo/semantic/src/binder/mod.rs

@@ -155,6 +155,34 @@ pub(crate) enum ResolveOptions {
     Super(NodeId),
 }
 
+impl ResolveOptions {
+    /// Whether a lookup under these options can see `definition`, declared in
+    /// the contract being searched (`in_own_contract`) or in one of its bases.
+    fn can_see(self, definition: &Definition, in_own_contract: bool) -> bool {
+        match self {
+            Self::Internal => {
+                if in_own_contract {
+                    definition.is_private_or_internally_visible()
+                } else {
+                    definition.is_internally_visible()
+                }
+            }
+            Self::Qualified => {
+                in_own_contract
+                    || definition.is_internally_visible()
+                    || definition.is_externally_visible()
+            }
+            Self::This(_) | Self::External => definition.is_externally_visible(),
+            // `super` reaches the implemented functions of the bases, and
+            // nothing else: not unimplemented functions, state variables,
+            // constants, or type definitions.
+            Self::Super(_) => {
+                definition.is_implemented_function() && definition.is_internally_visible()
+            }
+        }
+    }
+}
+
 /// A group of definitions, identified by their `NodeId`s. Held inline up to a
 /// handful of entries, which covers the sets that occur in practice: the
 /// definitions an ambiguous reference may name, and the working sets built
@@ -645,32 +673,7 @@ impl Binder {
                     continue;
                 };
                 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()
-                        }
-                        // `super` reaches the functions of the bases, and
-                        // nothing else: not state variables, constants, or
-                        // type definitions.
-                        ResolveOptions::Super(_) => {
-                            matches!(definition, Definition::Function(_))
-                                && definition.is_internally_visible()
-                        }
-                    };
-                    if visible {
+                    if options.can_see(&self.definitions[definition_id], index == 0) {
                         results.push(*definition_id);
                     }
                 }
@@ -697,9 +700,19 @@ impl Binder {
 
             Resolution::from(results)
         } else if let Some(definitions) = contract_scope.definitions.get(symbol) {
-            // This case shouldn't happen for valid Solidity, as all
-            // contracts should have a proper linearisation
-            Resolution::from(definitions.as_slice())
+            // Libraries are not linearised, and neither is a contract whose
+            // bases failed to linearise: search only the scope's own members.
+            // A qualified lookup sees all of a scope's own members.
+            let can_see =
+                |definition_id: &NodeId| options.can_see(&self.definitions[definition_id], true);
+            // NOTE(perf): the double check if we need to filter is cheaper than
+            // always copying and collecting the results.
+            if matches!(options, ResolveOptions::Qualified) || definitions.iter().all(can_see) {
+                Resolution::from(definitions.as_slice())
+            } else {
+                let results: DefinitionIds = definitions.iter().copied().filter(can_see).collect();
+                Resolution::from(results)
+            }
         } else {
             Resolution::Unresolved
         }
@@ -969,18 +982,30 @@ impl Binder {
     /// used when resolving members in an identifier path in a value context
     /// (eg. `emit C.E()`, or `C.f.selector`) as opposed to a typing context
     /// (eg. `C.S var`) where the member is searched in all inherited bases.
+    /// Neither a private member nor a modifier is reachable this way, not even
+    /// from within its own contract or library.
     pub(crate) fn resolve_in_scope_as_expression_member(
         &self,
         scope_id: ScopeId,
         symbol: &str,
     ) -> Resolution {
         match self.get_scope_by_id(scope_id) {
-            Scope::Contract(contract_scope) => contract_scope
-                .definitions
-                .get(symbol)
-                .map_or(Resolution::Unresolved, |definitions| {
+            Scope::Contract(contract_scope) => {
+                let Some(definitions) = contract_scope.definitions.get(symbol) else {
+                    return Resolution::Unresolved;
+                };
+                let is_reachable = |definition_id: &NodeId| {
+                    let definition = &self.definitions[definition_id];
+                    !definition.is_private() && !matches!(definition, Definition::Modifier(_))
+                };
+                if definitions.iter().all(is_reachable) {
                     Resolution::from(definitions.as_slice())
-                }),
+                } else {
+                    let results: DefinitionIds =
+                        definitions.iter().copied().filter(is_reachable).collect();
+                    Resolution::from(results)
+                }
+            }
             _ => self.resolve_in_scope_as_namespace(scope_id, symbol),
         }
     }

crates/solidity-v2/outputs/cargo/semantic/src/built_ins/mod.rs

@@ -362,10 +362,8 @@ impl<'a> BuiltInsResolver<'a> {
                 .binder
                 .find_definition_by_id(*definition_id)
                 .and_then(|definition| Self::lookup_member_of_user_definition(definition, symbol)),
-            Type::String(_) => match symbol {
-                "length" => Some(InternalBuiltIn::Length),
-                _ => None,
-            },
+            // A string has no members: `bytes(s).length` reads its length.
+            Type::String(_) => None,
             Type::Struct(_) => None,
             Type::Tuple(_) => None,
             Type::UserDefinedValue(_) => None,

crates/solidity-v2/outputs/cargo/semantic/src/passes/common/mod.rs

@@ -22,6 +22,7 @@ pub(crate) use node_extensions::node_location;
 pub(crate) use resolution::{
     IdentifierPathTarget, Overridable, PathResolution, filter_overridden_definitions,
     filter_overridden_definitions_by_selector, find_definition_namespace_scope_id,
-    resolve_identifier_path_as_expression_in_scope, resolve_identifier_path_as_type_in_scope,
+    namespace_member_not_found, resolve_identifier_path_as_expression_in_scope,
+    resolve_identifier_path_as_type_in_scope,
 };
 pub(crate) use scope_stack::ScopeStack;

crates/solidity-v2/outputs/cargo/semantic/src/passes/common/resolution.rs

@@ -76,15 +76,7 @@ impl PathResolution {
                 let kind = if index == 0 {
                     IdentifierNotFound.into()
                 } else {
-                    NamespaceMemberNotFound {
-                        name: identifier.unparse().to_owned(),
-                        namespace: identifier_path[..index]
-                            .iter()
-                            .map(|qualifier| qualifier.unparse())
-                            .collect::<Vec<_>>()
-                            .join("."),
-                    }
-                    .into()
+                    namespace_member_not_found(identifier_path, index).into()
                 };
                 Some((identifier, kind))
             }
@@ -130,6 +122,22 @@ impl PathResolution {
     }
 }
 
+/// The diagnostic for the identifier at `index` of `identifier_path` not being
+/// provided by the identifiers before it.
+pub(crate) fn namespace_member_not_found(
+    identifier_path: &ir::IdentifierPath,
+    index: usize,
+) -> NamespaceMemberNotFound {
+    NamespaceMemberNotFound {
+        name: identifier_path[index].unparse().to_owned(),
+        namespace: identifier_path[..index]
+            .iter()
+            .map(|qualifier| qualifier.unparse())
+            .collect::<Vec<_>>()
+            .join("."),
+    }
+}
+
 /// Resolves an `IdentifierPath` starting from the given scope, creating
 /// `Reference`s for all its elements. It will follow through
 /// contracts/interfaces/libraries as well as imports and treat them as

crates/solidity-v2/outputs/cargo/semantic/src/passes/p3_type_definitions/resolution.rs

@@ -19,8 +19,7 @@ use crate::passes::common::constant_evaluator::{
     ConstantResolver, EvaluationError, evaluate_compile_time_constant,
 };
 use crate::passes::common::{
-    IdentifierPathTarget, PathResolution, node_location,
-    resolve_identifier_path_as_expression_in_scope, resolve_identifier_path_as_type_in_scope,
+    IdentifierPathTarget, PathResolution, node_location, resolve_identifier_path_as_type_in_scope,
 };
 use crate::types::{
     ArrayType, DataLocation, FixedSizeArrayType, FunctionType, MappingType, TupleType, Type, TypeId,
@@ -56,27 +55,6 @@ impl Pass<'_> {
         }
     }
 
-    /// Resolves an `IdentifierPath` in an expression context starting at the
-    /// current lexical scope, and reports its first identifier that does not
-    /// name a single declaration. See [`PathResolution::unresolved_identifier`]
-    /// for `target`.
-    pub(super) fn resolve_and_check_identifier_path_as_expression(
-        &mut self,
-        identifier_path: &ir::IdentifierPath,
-        target: IdentifierPathTarget,
-    ) {
-        let path_resolution = resolve_identifier_path_as_expression_in_scope(
-            self.binder,
-            identifier_path,
-            self.current_scope_id(),
-        );
-        if let Some((identifier, kind)) =
-            path_resolution.unresolved_identifier(identifier_path, target)
-        {
-            self.push_diagnostic(identifier, kind);
-        }
-    }
-
     pub(super) fn resolve_parameter_types(
         &mut self,
         parameters: &ir::Parameters,

crates/solidity-v2/outputs/cargo/semantic/src/passes/p3_type_definitions/visitor.rs

@@ -548,11 +548,14 @@ impl Visitor for Pass<'_> {
                                     .entry(operator.into())
                                     .or_default()
                                     .extend_from_slice(&definition_ids);
+                            } else {
+                                // Only a function listed without an operator
+                                // is attached as a member.
+                                symbols.insert(
+                                    symbol_name.unparse().to_string(),
+                                    ScopeDefinitionIds::from_slice(&definition_ids),
+                                );
                             }
-                            symbols.insert(
-                                symbol_name.unparse().to_string(),
-                                ScopeDefinitionIds::from_slice(&definition_ids),
-                            );
                         }
 
                         let scope = Scope::new_using(node.id(), symbols);
@@ -594,32 +597,6 @@ impl Visitor for Pass<'_> {
         }
     }
 
-    fn enter_revert_statement(&mut self, node: &ir::RevertStatement) -> bool {
-        self.resolve_and_check_identifier_path_as_expression(
-            &node.error,
-            IdentifierPathTarget::Error,
-        );
-        true
-    }
-
-    fn leave_revert_statement(&mut self, node: &ir::RevertStatement) {
-        let type_id = self.type_of_identifier_path(&node.error, None);
-        self.binder.set_node_type(node.id(), type_id);
-    }
-
-    fn enter_emit_statement(&mut self, node: &ir::EmitStatement) -> bool {
-        self.resolve_and_check_identifier_path_as_expression(
-            &node.event,
-            IdentifierPathTarget::Event,
-        );
-        true
-    }
-
-    fn leave_emit_statement(&mut self, node: &ir::EmitStatement) {
-        let type_id = self.type_of_identifier_path(&node.event, None);
-        self.binder.set_node_type(node.id(), type_id);
-    }
-
     fn leave_new_expression(&mut self, node: &ir::NewExpression) {
         let type_id = self.resolve_type_name(&node.type_name, Some(DataLocation::Memory));
         let typing = type_id.map_or(Typing::Unresolved, Typing::NewExpression);

crates/solidity-v2/outputs/cargo/semantic/src/passes/p5_resolve_references/disambiguation.rs

@@ -213,13 +213,8 @@ impl Pass<'_> {
         argument_type: TypeId,
         external_call: bool,
     ) -> bool {
-        if external_call {
-            self.types
-                .implicitly_convertible_to_for_external_call(argument_type, parameter_type)
-        } else {
-            self.types
-                .implicitly_convertible_to(argument_type, parameter_type)
-        }
+        self.types
+            .implicitly_convertible_to_for_call(argument_type, parameter_type, external_call)
     }
 
     pub(super) fn lookup_function_matching_named_arguments(