diff --git a/.noir-sync-commit b/.noir-sync-commit index 3e5ac973b0c..256571eae0c 100644 --- a/.noir-sync-commit +++ b/.noir-sync-commit @@ -1 +1 @@ -1b26440889379f491315cd9d088537b1898d57c5 +c3cb38a7c4de6fc321b367eda3fca6d06e76b77a diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs index b4ba80a307c..bd78febc1d0 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1385,7 +1385,7 @@ impl<'context> Elaborator<'context> { } if let Some(name) = name { - self.interner.register_global(global_id, name, self.module_id()); + self.interner.register_global(global_id, name, global.visibility, self.module_id()); } self.local_module = old_module; diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs index 132d1988b78..bb8d041eab4 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/noir/noir-repo/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/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs index 35cd5145579..ef06cfdaad8 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs +++ b/noir/noir-repo/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/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index b981dcb348f..1690295ffda 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/noir/noir-repo/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/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 9805e04a0b1..16fd43ba2a2 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -111,6 +111,7 @@ pub struct UnresolvedGlobal { pub module_id: LocalModuleId, pub global_id: GlobalId, pub stmt_def: LetStatement, + pub visibility: ItemVisibility, } pub struct ModuleAttribute { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index df2ada94ce0..ba1075dfb30 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -357,7 +357,12 @@ impl<'a> ModCollector<'a> { if context.def_interner.is_in_lsp_mode() { let parent_module_id = ModuleId { krate, local_id: self.module_id }; let name = name.to_string(); - context.def_interner.register_type_alias(type_alias_id, name, parent_module_id); + context.def_interner.register_type_alias( + type_alias_id, + name, + visibility, + parent_module_id, + ); } } errors @@ -591,7 +596,12 @@ impl<'a> ModCollector<'a> { if context.def_interner.is_in_lsp_mode() { let parent_module_id = ModuleId { krate, local_id: self.module_id }; - context.def_interner.register_trait(trait_id, name.to_string(), parent_module_id); + context.def_interner.register_trait( + trait_id, + name.to_string(), + visibility, + parent_module_id, + ); } self.def_collector.items.traits.insert(trait_id, unresolved); @@ -879,7 +889,7 @@ fn push_child_module( ); if interner.is_in_lsp_mode() { - interner.register_module(mod_id, mod_name.0.contents.clone()); + interner.register_module(mod_id, visibility, mod_name.0.contents.clone()); } } @@ -1221,7 +1231,7 @@ pub(crate) fn collect_global( interner.set_doc_comments(ReferenceId::Global(global_id), doc_comments); - let global = UnresolvedGlobal { file_id, module_id, global_id, stmt_def: global }; + let global = UnresolvedGlobal { file_id, module_id, global_id, stmt_def: global, visibility }; (global, error) } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir_def/expr.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir_def/expr.rs index 063b960863c..71e0256a7e8 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir_def/expr.rs +++ b/noir/noir-repo/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/noir/noir-repo/compiler/noirc_frontend/src/locations.rs b/noir/noir-repo/compiler/noirc_frontend/src/locations.rs index 65adf4ca9c4..f3439316538 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/locations.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/locations.rs @@ -278,8 +278,12 @@ impl NodeInterner { .next() } - pub(crate) fn register_module(&mut self, id: ModuleId, name: String) { - let visibility = ItemVisibility::Public; + pub(crate) fn register_module( + &mut self, + id: ModuleId, + visibility: ItemVisibility, + name: String, + ) { self.register_name_for_auto_import(name, ModuleDefId::ModuleId(id), visibility, None); } @@ -287,11 +291,10 @@ impl NodeInterner { &mut self, id: GlobalId, name: String, + visibility: ItemVisibility, parent_module_id: ModuleId, ) { self.add_definition_location(ReferenceId::Global(id), Some(parent_module_id)); - - let visibility = ItemVisibility::Public; self.register_name_for_auto_import(name, ModuleDefId::GlobalId(id), visibility, None); } @@ -306,10 +309,14 @@ impl NodeInterner { self.register_name_for_auto_import(name, ModuleDefId::TypeId(id), visibility, None); } - pub(crate) fn register_trait(&mut self, id: TraitId, name: String, parent_module_id: ModuleId) { + pub(crate) fn register_trait( + &mut self, + id: TraitId, + name: String, + visibility: ItemVisibility, + parent_module_id: ModuleId, + ) { self.add_definition_location(ReferenceId::Trait(id), Some(parent_module_id)); - - let visibility = ItemVisibility::Public; self.register_name_for_auto_import(name, ModuleDefId::TraitId(id), visibility, None); } @@ -317,11 +324,10 @@ impl NodeInterner { &mut self, id: TypeAliasId, name: String, + visibility: ItemVisibility, parent_module_id: ModuleId, ) { self.add_definition_location(ReferenceId::Alias(id), Some(parent_module_id)); - - let visibility = ItemVisibility::Public; self.register_name_for_auto_import(name, ModuleDefId::TypeAliasId(id), visibility, None); } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/monomorphization/mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/monomorphization/mod.rs index 63ef807d898..295aa9056b6 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/noir/noir-repo/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/noir/noir-repo/compiler/noirc_frontend/src/tests/imports.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests/imports.rs index 5ebc5b3bdbd..1da7f5ce0ce 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests/imports.rs +++ b/noir/noir-repo/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/noir/noir-repo/compiler/noirc_frontend/src/tests/visibility.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests/visibility.rs index 6054ddad47d..30a01818879 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests/visibility.rs +++ b/noir/noir-repo/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), diff --git a/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs b/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs index 769f801a6f1..bf1fe906be3 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs @@ -186,7 +186,7 @@ fn foo(x: SomeTypeInBar) {}"#; let src = r#" mod foo { mod bar { - mod some_module_in_bar {} + pub mod some_module_in_bar {} } } @@ -198,7 +198,7 @@ fn foo(x: SomeTypeInBar) {}"#; let expected = r#" mod foo { mod bar { - mod some_module_in_bar {} + pub mod some_module_in_bar {} } } @@ -216,7 +216,7 @@ fn foo(x: SomeTypeInBar) {}"#; let src = r#"mod foo { mod bar { - mod some_module_in_bar {} + pub(crate) mod some_module_in_bar {} } } @@ -228,7 +228,7 @@ fn main() { mod foo { mod bar { - mod some_module_in_bar {} + pub(crate) mod some_module_in_bar {} } } diff --git a/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs b/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs index 147fb5be4f3..e4beea48064 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs @@ -1575,7 +1575,7 @@ mod completion_tests { async fn test_auto_import_suggests_modules_too() { let src = r#" mod foo { - mod barbaz { + pub mod barbaz { fn hello_world() {} } } @@ -2271,4 +2271,56 @@ mod completion_tests { ) .await; } + + #[test] + async fn test_does_not_auto_import_private_global() { + let src = r#"mod moo { + global foobar = 1; + } + + fn main() { + fooba>|< + }"#; + + assert_completion(src, Vec::new()).await; + } + + #[test] + async fn test_does_not_auto_import_private_type_alias() { + let src = r#"mod moo { + type foobar = i32; + } + + fn main() { + fooba>|< + }"#; + + assert_completion(src, Vec::new()).await; + } + + #[test] + async fn test_does_not_auto_import_private_trait() { + let src = r#"mod moo { + trait Foobar {} + } + + fn main() { + Fooba>|< + }"#; + + assert_completion(src, Vec::new()).await; + } + + #[test] + async fn test_does_not_auto_import_private_module() { + let src = r#"mod moo { + mod foobar {} + } + + fn main() { + fooba>|< + }"#; + + assert_completion(src, Vec::new()).await; + } }