Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(elaborator): Fix regression introduced by lazy-global changes #5223

Merged
merged 1 commit into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -311,6 +311,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;
vezenovm marked this conversation as resolved.
Show resolved Hide resolved
if self.self_type.is_some() {
self.in_contract = false;
}
Expand Down Expand Up @@ -406,6 +407,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 @@ -547,6 +549,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 @@ -662,6 +665,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 @@ -1080,6 +1084,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 @@ -1114,8 +1119,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);
jfecher marked this conversation as resolved.
Show resolved Hide resolved

let global_id = global.global_id;
self.current_item = Some(DependencyId::Global(global_id));
Expand Down Expand Up @@ -1147,6 +1152,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 @@ -1186,11 +1193,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 @@ -1205,10 +1214,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 @@ -333,7 +333,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(
jfecher marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -7,9 +7,9 @@
use iter_extended::vecmap;
use noirc_arena::{Arena, Index};
use noirc_errors::{Location, Span, Spanned};
use petgraph::algo::tarjan_scc;

Check warning on line 10 in compiler/noirc_frontend/src/node_interner.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (petgraph)

Check warning on line 10 in compiler/noirc_frontend/src/node_interner.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (tarjan)
use petgraph::prelude::DiGraph;

Check warning on line 11 in compiler/noirc_frontend/src/node_interner.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (petgraph)
use petgraph::prelude::NodeIndex as PetGraphIndex;

Check warning on line 12 in compiler/noirc_frontend/src/node_interner.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (petgraph)

use crate::ast::Ident;
use crate::graph::CrateId;
Expand Down Expand Up @@ -63,7 +63,7 @@
function_modules: HashMap<FuncId, ModuleId>,

/// This graph tracks dependencies between different global definitions.
/// This is used to ensure the absense of dependency cycles for globals and types.

Check warning on line 66 in compiler/noirc_frontend/src/node_interner.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (absense)
dependency_graph: DiGraph<DependencyId, ()>,

/// To keep track of where each DependencyId is in `dependency_graph`, we need
Expand Down Expand Up @@ -480,7 +480,7 @@
function_modifiers: HashMap::new(),
function_modules: HashMap::new(),
func_id_to_trait: HashMap::new(),
dependency_graph: petgraph::graph::DiGraph::new(),

Check warning on line 483 in compiler/noirc_frontend/src/node_interner.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (petgraph)
dependency_graph_indices: HashMap::new(),
id_to_location: HashMap::new(),
definitions: vec![],
Expand Down Expand Up @@ -1411,6 +1411,13 @@
) -> 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(());
}
jfecher marked this conversation as resolved.
Show resolved Hide resolved

// Replace each generic with a fresh type variable
let substitutions = impl_generics
.into_iter()
Expand Down Expand Up @@ -1656,7 +1663,7 @@
}

pub(crate) fn check_for_dependency_cycles(&self) -> Vec<(CompilationError, FileId)> {
let strongly_connected_components = tarjan_scc(&self.dependency_graph);

Check warning on line 1666 in compiler/noirc_frontend/src/node_interner.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (tarjan)
let mut errors = Vec::new();

let mut push_error = |item: String, scc: &[_], i, location: Location| {
Expand Down
Loading