fix: don't warn twice when referring to private item (#6216)

# Description

## Problem

We were producing "this item is private" errors twice.

## Summary

The problem was that the code looked up a path twice, and errored each
time it found an error in that lookup.

Now the code still looks up the path twice, but only errors once (if the
first lookup succeeds, we report the optional error of that lookup,
otherwise we do the same with the second one).

## Additional Context

Also applied some refactors (made a tuple return value its own struct,
and made an enum variant use a struct instead of a tuple too).

This fixes the issue but we are still doing two Path lookups here, which
means there's an optimization opportunity to only do the lookup once.
But this can be done in a separate PR.

## Documentation

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

compiler/noirc_frontend/src/elaborator/patterns.rs

@@ -12,7 +12,7 @@ use crate::{
         type_check::{Source, TypeCheckError},
     },
     hir_def::{
-        expr::{HirExpression, HirIdent, HirMethodReference, ImplKind},
+        expr::{HirExpression, HirIdent, HirMethodReference, ImplKind, TraitMethod},
         stmt::HirPattern,
     },
     node_interner::{DefinitionId, DefinitionKind, ExprId, FuncId, GlobalId, TraitImplKind},
@@ -525,11 +525,15 @@ impl<'context> Elaborator<'context> {
     }
 
     fn resolve_variable(&mut self, path: Path) -> HirIdent {
-        if let Some((method, constraint, assumed)) = self.resolve_trait_generic_path(&path) {
+        if let Some(trait_path_resolution) = self.resolve_trait_generic_path(&path) {
+            if let Some(error) = trait_path_resolution.error {
+                self.push_err(error);
+            }
+
             HirIdent {
                 location: Location::new(path.span, self.file),
-                id: self.interner.trait_method_id(method),
-                impl_kind: ImplKind::TraitMethod(method, constraint, assumed),
+                id: self.interner.trait_method_id(trait_path_resolution.method.method_id),
+                impl_kind: ImplKind::TraitMethod(trait_path_resolution.method),
             }
         } else {
             // If the Path is being used as an Expression, then it is referring to a global from a separate module
@@ -595,8 +599,12 @@ impl<'context> Elaborator<'context> {
         // We need to do this first since otherwise instantiating the type below
         // will replace each trait generic with a fresh type variable, rather than
         // the type used in the trait constraint (if it exists). See #4088.
-        if let ImplKind::TraitMethod(_, constraint, assumed) = &ident.impl_kind {
-            self.bind_generics_from_trait_constraint(constraint, *assumed, &mut bindings);
+        if let ImplKind::TraitMethod(method) = &ident.impl_kind {
+            self.bind_generics_from_trait_constraint(
+                &method.constraint,
+                method.assumed,
+                &mut bindings,
+            );
         }
 
         // An identifiers type may be forall-quantified in the case of generic functions.
@@ -634,18 +642,18 @@ impl<'context> Elaborator<'context> {
             }
         }
 
-        if let ImplKind::TraitMethod(_, mut constraint, assumed) = ident.impl_kind {
-            constraint.apply_bindings(&bindings);
-            if assumed {
-                let trait_generics = constraint.trait_generics.clone();
-                let object_type = constraint.typ;
+        if let ImplKind::TraitMethod(mut method) = ident.impl_kind {
+            method.constraint.apply_bindings(&bindings);
+            if method.assumed {
+                let trait_generics = method.constraint.trait_generics.clone();
+                let object_type = method.constraint.typ;
                 let trait_impl = TraitImplKind::Assumed { object_type, trait_generics };
                 self.interner.select_impl_for_expression(expr_id, trait_impl);
             } else {
                 // Currently only one impl can be selected per expr_id, so this
                 // constraint needs to be pushed after any other constraints so
                 // that monomorphization can resolve this trait method to the correct impl.
-                self.push_trait_constraint(constraint, expr_id);
+                self.push_trait_constraint(method.constraint, expr_id);
             }
         }
 
@@ -731,7 +739,7 @@ impl<'context> Elaborator<'context> {
                 let mut constraint =
                     self.interner.get_trait(method_id.trait_id).as_constraint(span);
                 constraint.trait_generics = generics;
-                ImplKind::TraitMethod(method_id, constraint, false)
+                ImplKind::TraitMethod(TraitMethod { method_id, constraint, assumed: false })
             }
         };
 

compiler/noirc_frontend/src/elaborator/types.rs

@@ -15,7 +15,7 @@ use crate::{
         comptime::{Interpreter, Value},
         def_collector::dc_crate::CompilationError,
         def_map::ModuleDefId,
-        resolution::errors::ResolverError,
+        resolution::{errors::ResolverError, import::PathResolutionError},
         type_check::{
             generics::{Generic, TraitGenerics},
             NoMatchingImplFoundError, Source, TypeCheckError,
@@ -24,15 +24,15 @@ use crate::{
     hir_def::{
         expr::{
             HirBinaryOp, HirCallExpression, HirExpression, HirLiteral, HirMemberAccess,
-            HirMethodReference, HirPrefixExpression,
+            HirMethodReference, HirPrefixExpression, TraitMethod,
         },
         function::{FuncMeta, Parameters},
         stmt::HirStatement,
         traits::{NamedType, TraitConstraint},
     },
     node_interner::{
-        DefinitionKind, DependencyId, ExprId, FuncId, GlobalId, ImplSearchErrorKind, NodeInterner,
-        TraitId, TraitImplKind, TraitMethodId,
+        DefinitionKind, DependencyId, ExprId, GlobalId, ImplSearchErrorKind, NodeInterner, TraitId,
+        TraitImplKind, TraitMethodId,
     },
     token::SecondaryAttribute,
     Generics, Kind, ResolvedGeneric, Type, TypeBinding, TypeBindings, TypeVariable,
@@ -44,6 +44,11 @@ use super::{lints, Elaborator};
 pub const SELF_TYPE_NAME: &str = "Self";
 pub const WILDCARD_TYPE: &str = "_";
 
+pub(super) struct TraitPathResolution {
+    pub(super) method: TraitMethod,
+    pub(super) error: Option<PathResolutionError>,
+}
+
 impl<'context> Elaborator<'context> {
     /// Translates an UnresolvedType to a Type with a `TypeKind::Normal`
     pub(crate) fn resolve_type(&mut self, typ: UnresolvedType) -> Type {
@@ -522,10 +527,7 @@ impl<'context> Elaborator<'context> {
     //
     // Returns the trait method, trait constraint, and whether the impl is assumed to exist by a where clause or not
     // E.g. `t.method()` with `where T: Foo<Bar>` in scope will return `(Foo::method, T, vec![Bar])`
-    fn resolve_trait_static_method_by_self(
-        &mut self,
-        path: &Path,
-    ) -> Option<(TraitMethodId, TraitConstraint, bool)> {
+    fn resolve_trait_static_method_by_self(&mut self, path: &Path) -> Option<TraitPathResolution> {
         let trait_impl = self.current_trait_impl?;
         let trait_id = self.interner.try_get_trait_implementation(trait_impl)?.borrow().trait_id;
 
@@ -537,7 +539,10 @@ impl<'context> Elaborator<'context> {
                 let the_trait = self.interner.get_trait(trait_id);
                 let method = the_trait.find_method(method.0.contents.as_str())?;
                 let constraint = the_trait.as_constraint(path.span);
-                return Some((method, constraint, true));
+                return Some(TraitPathResolution {
+                    method: TraitMethod { method_id: method, constraint, assumed: true },
+                    error: None,
+                });
             }
         }
         None
@@ -547,16 +552,18 @@ impl<'context> Elaborator<'context> {
     //
     // Returns the trait method, trait constraint, and whether the impl is assumed to exist by a where clause or not
     // E.g. `t.method()` with `where T: Foo<Bar>` in scope will return `(Foo::method, T, vec![Bar])`
-    fn resolve_trait_static_method(
-        &mut self,
-        path: &Path,
-    ) -> Option<(TraitMethodId, TraitConstraint, bool)> {
-        let func_id: FuncId = self.lookup(path.clone()).ok()?;
+    fn resolve_trait_static_method(&mut self, path: &Path) -> Option<TraitPathResolution> {
+        let path_resolution = self.resolve_path(path.clone()).ok()?;
+        let ModuleDefId::FunctionId(func_id) = path_resolution.module_def_id else { return None };
+
         let meta = self.interner.function_meta(&func_id);
         let the_trait = self.interner.get_trait(meta.trait_id?);
         let method = the_trait.find_method(path.last_name())?;
         let constraint = the_trait.as_constraint(path.span);
-        Some((method, constraint, false))
+        Some(TraitPathResolution {
+            method: TraitMethod { method_id: method, constraint, assumed: false },
+            error: path_resolution.error,
+        })
     }
 
     // This resolves a static trait method T::trait_method by iterating over the where clause
@@ -567,7 +574,7 @@ impl<'context> Elaborator<'context> {
     fn resolve_trait_method_by_named_generic(
         &mut self,
         path: &Path,
-    ) -> Option<(TraitMethodId, TraitConstraint, bool)> {
+    ) -> Option<TraitPathResolution> {
         if path.segments.len() != 2 {
             return None;
         }
@@ -581,7 +588,10 @@ impl<'context> Elaborator<'context> {
 
                 let the_trait = self.interner.get_trait(constraint.trait_id);
                 if let Some(method) = the_trait.find_method(path.last_name()) {
-                    return Some((method, constraint, true));
+                    return Some(TraitPathResolution {
+                        method: TraitMethod { method_id: method, constraint, assumed: true },
+                        error: None,
+                    });
                 }
             }
         }
@@ -595,7 +605,7 @@ impl<'context> Elaborator<'context> {
     pub(super) fn resolve_trait_generic_path(
         &mut self,
         path: &Path,
-    ) -> Option<(TraitMethodId, TraitConstraint, bool)> {
+    ) -> Option<TraitPathResolution> {
         self.resolve_trait_static_method_by_self(path)
             .or_else(|| self.resolve_trait_static_method(path))
             .or_else(|| self.resolve_trait_method_by_named_generic(path))

compiler/noirc_frontend/src/hir/comptime/interpreter.rs

@@ -544,8 +544,8 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
             InterpreterError::VariableNotInScope { location }
         })?;
 
-        if let ImplKind::TraitMethod(method, _, _) = ident.impl_kind {
-            let method_id = resolve_trait_method(self.elaborator.interner, method, id)?;
+        if let ImplKind::TraitMethod(method) = ident.impl_kind {
+            let method_id = resolve_trait_method(self.elaborator.interner, method.method_id, id)?;
             let typ = self.elaborator.interner.id_type(id).follow_bindings();
             let bindings = self.elaborator.interner.get_instantiation_bindings(id).clone();
             return Ok(Value::Function(method_id, typ, Rc::new(bindings)));

compiler/noirc_frontend/src/hir_def/expr.rs

@@ -70,7 +70,14 @@ pub enum ImplKind {
     /// and eventually linked to this id. The boolean indicates whether the impl
     /// is already assumed to exist - e.g. when resolving a path such as `T::default`
     /// when there is a corresponding `T: Default` constraint in scope.
-    TraitMethod(TraitMethodId, TraitConstraint, bool),
+    TraitMethod(TraitMethod),
+}
+
+#[derive(Debug, Clone)]
+pub struct TraitMethod {
+    pub method_id: TraitMethodId,
+    pub constraint: TraitConstraint,
+    pub assumed: bool,
 }
 
 impl Eq for HirIdent {}
@@ -247,7 +254,7 @@ impl HirMethodCallExpression {
                     trait_generics,
                     span: location.span,
                 };
-                (id, ImplKind::TraitMethod(method_id, constraint, false))
+                (id, ImplKind::TraitMethod(TraitMethod { method_id, constraint, assumed: false }))
             }
         };
         let func_var = HirIdent { location, id, impl_kind };

compiler/noirc_frontend/src/monomorphization/mod.rs

@@ -850,8 +850,8 @@ impl<'interner> Monomorphizer<'interner> {
     ) -> Result<ast::Expression, MonomorphizationError> {
         let typ = self.interner.id_type(expr_id);
 
-        if let ImplKind::TraitMethod(method, _, _) = ident.impl_kind {
-            return self.resolve_trait_method_expr(expr_id, typ, method);
+        if let ImplKind::TraitMethod(method) = ident.impl_kind {
+            return self.resolve_trait_method_expr(expr_id, typ, method.method_id);
         }
 
         // Ensure all instantiation bindings are bound.

compiler/noirc_frontend/src/tests/imports.rs

@@ -73,7 +73,7 @@ fn warns_on_use_of_private_exported_item() {
     "#;
 
     let errors = get_program_errors(src);
-    assert_eq!(errors.len(), 2); // An existing bug causes this error to be duplicated
+    assert_eq!(errors.len(), 1);
 
     assert!(matches!(
         &errors[0].0,

compiler/noirc_frontend/src/tests/visibility.rs

@@ -94,7 +94,7 @@ fn errors_if_trying_to_access_public_function_inside_private_module() {
     "#;
 
     let errors = get_program_errors(src);
-    assert_eq!(errors.len(), 2); // There's a bug that duplicates this error
+    assert_eq!(errors.len(), 1);
 
     let CompilationError::ResolverError(ResolverError::PathResolutionError(
         PathResolutionError::Private(ident),