From 619c5451b152d62e01d3c4c1da7e13ff6502f915 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 3 Oct 2024 13:04:44 -0300 Subject: [PATCH] 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. --- .../noirc_frontend/src/elaborator/patterns.rs | 34 ++++++++------ .../noirc_frontend/src/elaborator/types.rs | 46 +++++++++++-------- .../src/hir/comptime/interpreter.rs | 4 +- compiler/noirc_frontend/src/hir_def/expr.rs | 11 ++++- .../src/monomorphization/mod.rs | 4 +- compiler/noirc_frontend/src/tests/imports.rs | 2 +- .../noirc_frontend/src/tests/visibility.rs | 2 +- 7 files changed, 64 insertions(+), 39 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 132d1988b78..bb8d041eab4 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/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 }) } }; diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 35cd5145579..ef06cfdaad8 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/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, +} + 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` 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 { 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` 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 { + 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 { 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 { 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)) diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index b981dcb348f..1690295ffda 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/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))); diff --git a/compiler/noirc_frontend/src/hir_def/expr.rs b/compiler/noirc_frontend/src/hir_def/expr.rs index 063b960863c..71e0256a7e8 100644 --- a/compiler/noirc_frontend/src/hir_def/expr.rs +++ b/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 }; diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 63ef807d898..295aa9056b6 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -850,8 +850,8 @@ impl<'interner> Monomorphizer<'interner> { ) -> Result { 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. diff --git a/compiler/noirc_frontend/src/tests/imports.rs b/compiler/noirc_frontend/src/tests/imports.rs index 5ebc5b3bdbd..1da7f5ce0ce 100644 --- a/compiler/noirc_frontend/src/tests/imports.rs +++ b/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, diff --git a/compiler/noirc_frontend/src/tests/visibility.rs b/compiler/noirc_frontend/src/tests/visibility.rs index 6054ddad47d..30a01818879 100644 --- a/compiler/noirc_frontend/src/tests/visibility.rs +++ b/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),