From fde432aacc436b6c57f0d937d7c86836bac0b465 Mon Sep 17 00:00:00 2001 From: jfecher Date: Wed, 12 Jun 2024 10:45:58 -0500 Subject: [PATCH] fix(elaborator): Fix regression introduced by lazy-global changes (#5223) # Description ## Problem\* Fixes a regression introduced by #5191 ## Summary\* When a global is lazily evaluated, the local module id and file id of the elaborator would change without being reset to the previous value. This lead to scoping issues after the global was evaluated. ## Additional Context I was unable to create a repro for this aside from running `nargo t --use-elaborator` on `aztec-nr` unfortunately. ## 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/mod.rs | 15 ++++++++++++--- .../noirc_frontend/src/hir/resolution/errors.rs | 2 +- .../noirc_frontend/src/hir/resolution/traits.rs | 1 + compiler/noirc_frontend/src/node_interner.rs | 7 +++++++ 4 files changed, 21 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index aee3b5c5807..3c88a419b99 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -308,6 +308,7 @@ impl<'context> Elaborator<'context> { let old_function = std::mem::replace(&mut self.current_function, Some(id)); // Without this, impl methods can accidentally be placed in contracts. See #3254 + let was_in_contract = self.in_contract; if self.self_type.is_some() { self.in_contract = false; } @@ -403,6 +404,7 @@ impl<'context> Elaborator<'context> { self.trait_bounds.clear(); self.in_unconstrained_fn = false; self.interner.update_fn(id, hir_func); + self.in_contract = was_in_contract; self.current_function = old_function; self.current_item = old_item; } @@ -544,6 +546,7 @@ impl<'context> Elaborator<'context> { self.current_function = Some(func_id); // Without this, impl methods can accidentally be placed in contracts. See #3254 + let was_in_contract = self.in_contract; if self.self_type.is_some() { self.in_contract = false; } @@ -659,6 +662,7 @@ impl<'context> Elaborator<'context> { }; self.interner.push_fn_meta(meta, func_id); + self.in_contract = was_in_contract; self.current_function = None; self.scopes.end_function(); self.current_item = None; @@ -1095,6 +1099,7 @@ impl<'context> Elaborator<'context> { // make sure every struct's fields is accurately set. for id in struct_ids { let struct_type = self.interner.get_struct(id); + // Only handle structs without generics as any generics args will be checked // after monomorphization when performing SSA codegen if struct_type.borrow().generics.is_empty() { @@ -1129,8 +1134,8 @@ impl<'context> Elaborator<'context> { } fn elaborate_global(&mut self, global: UnresolvedGlobal) { - self.local_module = global.module_id; - self.file = global.file_id; + let old_module = std::mem::replace(&mut self.local_module, global.module_id); + let old_file = std::mem::replace(&mut self.file, global.file_id); let global_id = global.global_id; self.current_item = Some(DependencyId::Global(global_id)); @@ -1162,6 +1167,8 @@ impl<'context> Elaborator<'context> { // Otherwise we may prematurely default to a Field inside the next function if this // global was unused there, even if it is consistently used as a u8 everywhere else. self.type_variables.clear(); + self.local_module = old_module; + self.file = old_file; } fn elaborate_comptime_global(&mut self, global_id: GlobalId) { @@ -1201,11 +1208,13 @@ impl<'context> Elaborator<'context> { self.local_module = *local_module; for (generics, _, function_set) in function_sets { + self.file = function_set.file_id; self.add_generics(generics); let self_type = self.resolve_type(self_type.clone()); function_set.self_type = Some(self_type.clone()); self.self_type = Some(self_type); self.define_function_metas_for_functions(function_set); + self.self_type = None; self.generics.clear(); } } @@ -1220,10 +1229,10 @@ impl<'context> Elaborator<'context> { let trait_generics = vecmap(&trait_impl.trait_generics, |generic| self.resolve_type(generic.clone())); + trait_impl.resolved_trait_generics = trait_generics; let self_type = self.resolve_type(unresolved_type.clone()); - self.self_type = Some(self_type.clone()); trait_impl.methods.self_type = Some(self_type); diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index 06f6dda7142..07fbde4db8e 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -319,7 +319,7 @@ impl<'a> From<&'a ResolverError> for Diagnostic { "Usage of the `#[foreign]` or `#[builtin]` function attributes are not allowed outside of the Noir standard library".into(), ident.span(), ), - ResolverError::OracleMarkedAsConstrained { ident } => Diagnostic::simple_error( + ResolverError::OracleMarkedAsConstrained { ident } => Diagnostic::simple_warning( error.to_string(), "Oracle functions must have the `unconstrained` keyword applied".into(), ident.span(), diff --git a/compiler/noirc_frontend/src/hir/resolution/traits.rs b/compiler/noirc_frontend/src/hir/resolution/traits.rs index 3d355fd4447..4c360731711 100644 --- a/compiler/noirc_frontend/src/hir/resolution/traits.rs +++ b/compiler/noirc_frontend/src/hir/resolution/traits.rs @@ -300,6 +300,7 @@ fn collect_trait_impl( let file = def_maps[&crate_id].file_id(trait_impl.module_id); let mut resolver = Resolver::new(interner, &path_resolver, def_maps, file); resolver.add_generics(&trait_impl.generics); + let typ = resolver.resolve_type(unresolved_type); errors.extend(take_errors(trait_impl.file_id, resolver)); diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 6c5f9b6bbcb..e29c3183993 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -1411,6 +1411,13 @@ impl NodeInterner { ) -> Result<(), (Span, FileId)> { self.trait_implementations.insert(impl_id, trait_impl.clone()); + // Avoid adding error types to impls since they'll conflict with every other type. + // We don't need to return an error since we expect an error to already be issued when + // the error type is created. + if object_type == Type::Error { + return Ok(()); + } + // Replace each generic with a fresh type variable let substitutions = impl_generics .into_iter()