Skip to content

Commit

Permalink
fix(elaborator): Fix regression introduced by lazy-global changes (#5223
Browse files Browse the repository at this point in the history
)

# 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.
  • Loading branch information
jfecher authored Jun 12, 2024
1 parent 87a1d8e commit fde432a
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 4 deletions.
15 changes: 12 additions & 3 deletions compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
}
}
Expand All @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/hir/resolution/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/hir/resolution/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand Down
7 changes: 7 additions & 0 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down

0 comments on commit fde432a

Please sign in to comment.