From ab0b1a85cc91f8ed748ee393ece54f5c3b43d7ef Mon Sep 17 00:00:00 2001 From: jfecher Date: Wed, 5 Jun 2024 17:37:14 +0100 Subject: [PATCH] fix(experimental elaborator): Fix globals which use function calls (#5172) # Description ## Problem\* Resolves an issue where we would give a type error if a global ever used a function call in its definition. ## Summary\* This one was more difficult to fix than it seemed since fixing the main bug revealed progressively more bugs. ## Additional Context https://github.com/noir-lang/noir/pull/5170 is included here as well to help me see that all tests are now passing. ## 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. --------- Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- Cargo.lock | 1 - compiler/noirc_driver/Cargo.toml | 1 - compiler/noirc_driver/src/lib.rs | 20 ++- compiler/noirc_frontend/src/elaborator/mod.rs | 124 +++++++++++------- .../noirc_frontend/src/elaborator/patterns.rs | 65 +++++---- .../src/elaborator/statements.rs | 54 ++++---- .../noirc_frontend/src/elaborator/traits.rs | 5 +- .../noirc_frontend/src/elaborator/types.rs | 32 ++--- .../src/hir/comptime/interpreter.rs | 7 +- .../src/hir/resolution/resolver.rs | 4 +- .../noirc_frontend/src/hir/type_check/mod.rs | 2 + .../noirc_frontend/src/hir_def/function.rs | 25 ++++ .../src/monomorphization/errors.rs | 23 ++-- .../src/monomorphization/mod.rs | 17 ++- compiler/noirc_frontend/src/node_interner.rs | 7 + tooling/nargo_cli/build.rs | 88 +++++++++++++ 16 files changed, 323 insertions(+), 152 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4b140a513f2..61bfdd08c0e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2775,7 +2775,6 @@ dependencies = [ "noirc_frontend", "rust-embed", "serde", - "thiserror", "tracing", ] diff --git a/compiler/noirc_driver/Cargo.toml b/compiler/noirc_driver/Cargo.toml index 495410b7b06..a9949f5093a 100644 --- a/compiler/noirc_driver/Cargo.toml +++ b/compiler/noirc_driver/Cargo.toml @@ -24,6 +24,5 @@ serde.workspace = true fxhash.workspace = true rust-embed.workspace = true tracing.workspace = true -thiserror.workspace = true aztec_macros = { path = "../../aztec_macros" } diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index d7368f299b8..848d9987ca3 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -24,7 +24,6 @@ use noirc_frontend::monomorphization::{ use noirc_frontend::node_interner::FuncId; use noirc_frontend::token::SecondaryAttribute; use std::path::Path; -use thiserror::Error; use tracing::info; mod abi_gen; @@ -117,13 +116,22 @@ fn parse_expression_width(input: &str) -> Result for CompileError { + fn from(error: MonomorphizationError) -> Self { + Self::MonomorphizationError(error) + } +} + +impl From for CompileError { + fn from(error: RuntimeError) -> Self { + Self::RuntimeError(error) + } } impl From for FileDiagnostic { diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 649a93b0bd6..9403e12496f 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -18,12 +18,17 @@ use crate::{ scope::ScopeForest as GenericScopeForest, type_check::{check_trait_impl_method_matches_declaration, TypeCheckError}, }, - hir_def::{expr::HirIdent, function::Parameters, traits::TraitConstraint}, + hir_def::{ + expr::HirIdent, + function::{FunctionBody, Parameters}, + traits::TraitConstraint, + }, macros_api::{ - Ident, NodeInterner, NoirFunction, NoirStruct, Pattern, SecondaryAttribute, StructId, + BlockExpression, Ident, NodeInterner, NoirFunction, NoirStruct, Pattern, + SecondaryAttribute, StructId, }, node_interner::{ - DefinitionId, DefinitionKind, DependencyId, ExprId, FuncId, TraitId, TypeAliasId, + DefinitionId, DefinitionKind, DependencyId, ExprId, FuncId, GlobalId, TraitId, TypeAliasId, }, Shared, Type, TypeVariable, }; @@ -134,7 +139,8 @@ pub struct Elaborator<'context> { /// ``` resolving_ids: BTreeSet, - trait_bounds: Vec, + /// Each constraint in the `where` clause of the function currently being resolved. + trait_bounds: Vec, current_function: Option, @@ -271,17 +277,29 @@ impl<'context> Elaborator<'context> { self.trait_id = functions.trait_id; // TODO: Resolve? self.self_type = functions.self_type; - for (local_module, id, func) in functions.functions { + for (local_module, id, _) in functions.functions { self.local_module = local_module; - self.recover_generics(|this| this.elaborate_function(func, id)); + self.recover_generics(|this| this.elaborate_function(id)); } self.self_type = None; self.trait_id = None; } - fn elaborate_function(&mut self, function: NoirFunction, id: FuncId) { - self.current_function = Some(id); + fn elaborate_function(&mut self, id: FuncId) { + let func_meta = self.interner.func_meta.get_mut(&id); + let func_meta = + func_meta.expect("FuncMetas should be declared before a function is elaborated"); + + let (kind, body, body_span) = match func_meta.take_body() { + FunctionBody::Unresolved(kind, body, span) => (kind, body, span), + FunctionBody::Resolved => return, + // Do not error for the still-resolving case. If there is a dependency cycle, + // the dependency cycle check will find it later on. + FunctionBody::Resolving => return, + }; + + let old_function = std::mem::replace(&mut self.current_function, Some(id)); // Without this, impl methods can accidentally be placed in contracts. See #3254 if self.self_type.is_some() { @@ -289,20 +307,17 @@ impl<'context> Elaborator<'context> { } self.scopes.start_function(); - self.current_item = Some(DependencyId::Function(id)); + let old_item = std::mem::replace(&mut self.current_item, Some(DependencyId::Function(id))); + + let func_meta = func_meta.clone(); - self.trait_bounds = function.def.where_clause.clone(); + self.trait_bounds = func_meta.trait_constraints.clone(); - if function.def.is_unconstrained { + if self.interner.function_modifiers(&id).is_unconstrained { self.in_unconstrained_fn = true; } - let func_meta = self.interner.func_meta.get(&id); - let func_meta = func_meta - .expect("FuncMetas should be declared before a function is elaborated") - .clone(); - - // The `DefinitionId`s for each parameter were already created in `define_function_meta` + // The DefinitionIds for each parameter were already created in define_function_meta // so we need to reintroduce the same IDs into scope here. for parameter in &func_meta.parameter_idents { let name = self.interner.definition_name(parameter.id).to_owned(); @@ -313,14 +328,13 @@ impl<'context> Elaborator<'context> { self.declare_numeric_generics(&func_meta.parameters, func_meta.return_type()); self.add_trait_constraints_to_scope(&func_meta); - let (hir_func, body_type) = match function.kind { + let (hir_func, body_type) = match kind { FunctionKind::Builtin | FunctionKind::LowLevel | FunctionKind::Oracle => { (HirFunction::empty(), Type::Error) } FunctionKind::Normal | FunctionKind::Recursive => { - let block_span = function.def.span; - let (block, body_type) = self.elaborate_block(function.def.body); - let expr_id = self.intern_expr(block, block_span); + let (block, body_type) = self.elaborate_block(body); + let expr_id = self.intern_expr(block, body_span); self.interner.push_expr_type(expr_id, body_type.clone()); (HirFunction::unchecked_from_expr(expr_id), body_type) } @@ -372,10 +386,19 @@ impl<'context> Elaborator<'context> { self.check_for_unused_variables_in_scope_tree(func_scope_tree); } - self.trait_bounds.clear(); + let meta = self + .interner + .func_meta + .get_mut(&id) + .expect("FuncMetas should be declared before a function is elaborated"); + + meta.function_body = FunctionBody::Resolved; + self.trait_bounds.clear(); + self.in_unconstrained_fn = false; self.interner.update_fn(id, hir_func); - self.current_function = None; + self.current_function = old_function; + self.current_item = old_item; } /// This turns function parameters of the form: @@ -442,15 +465,6 @@ impl<'context> Elaborator<'context> { } } - fn resolve_where_clause(&mut self, clause: &mut [UnresolvedTraitConstraint]) { - for bound in clause { - if let Some(trait_id) = self.resolve_trait_by_path(bound.trait_bound.trait_path.clone()) - { - bound.trait_bound.trait_id = Some(trait_id); - } - } - } - fn resolve_trait_by_path(&mut self, path: Path) -> Option { let path_resolver = StandardPathResolver::new(self.module_id()); @@ -522,9 +536,6 @@ impl<'context> Elaborator<'context> { is_trait_function: bool, ) { self.current_function = Some(func_id); - self.resolve_where_clause(&mut func.def.where_clause); - // `func` is no longer mutated. - let func = &*func; // Without this, impl methods can accidentally be placed in contracts. See #3254 if self.self_type.is_some() { @@ -594,6 +605,7 @@ impl<'context> Elaborator<'context> { typ.clone(), DefinitionKind::Local(None), &mut parameter_idents, + None, ); parameters.push((pattern, typ.clone(), visibility)); @@ -616,6 +628,9 @@ impl<'context> Elaborator<'context> { .map(|(name, typevar, _span)| (name.clone(), typevar.clone())) .collect(); + let statements = std::mem::take(&mut func.def.body.statements); + let body = BlockExpression { statements }; + let meta = FuncMeta { name: name_ident, kind: func.kind, @@ -633,6 +648,7 @@ impl<'context> Elaborator<'context> { is_entry_point, is_trait_function, has_inline_attribute, + function_body: FunctionBody::Unresolved(func.kind, body, func.def.span), }; self.interner.push_fn_meta(meta, func_id); @@ -1109,19 +1125,12 @@ impl<'context> Elaborator<'context> { let comptime = let_stmt.comptime; - self.elaborate_global_let(let_stmt, global_id); + let (let_statement, _typ) = self.elaborate_let(let_stmt, Some(global_id)); + let statement_id = self.interner.get_global(global_id).let_statement; + self.interner.replace_statement(statement_id, let_statement); if comptime { - let let_statement = self - .interner - .get_global_let_statement(global_id) - .expect("Let statement of global should be set by elaborate_global_let"); - - let mut interpreter = Interpreter::new(self.interner, &mut self.comptime_scopes); - - if let Err(error) = interpreter.evaluate_let(let_statement) { - self.errors.push(error.into_compilation_error_pair()); - } + self.elaborate_comptime_global(global_id); } // Avoid defaulting the types of globals here since they may be used in any function. @@ -1130,6 +1139,29 @@ impl<'context> Elaborator<'context> { self.type_variables.clear(); } + fn elaborate_comptime_global(&mut self, global_id: GlobalId) { + let let_statement = self + .interner + .get_global_let_statement(global_id) + .expect("Let statement of global should be set by elaborate_global_let"); + + let global = self.interner.get_global(global_id); + let definition_id = global.definition_id; + let location = global.location; + + let mut interpreter = Interpreter::new(self.interner, &mut self.comptime_scopes); + + if let Err(error) = interpreter.evaluate_let(let_statement) { + self.errors.push(error.into_compilation_error_pair()); + } else { + let value = interpreter + .lookup_id(definition_id, location) + .expect("The global should be defined since evaluate_let did not error"); + + self.interner.get_global_mut(global_id).value = Some(value); + } + } + fn define_function_metas( &mut self, functions: &mut [UnresolvedFunctions], diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 12b0b7b5e64..411caeba7b8 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -11,10 +11,11 @@ use crate::{ }, hir_def::{ expr::{HirIdent, ImplKind}, + function::FunctionBody, stmt::HirPattern, }, macros_api::{HirExpression, Ident, Path, Pattern}, - node_interner::{DefinitionId, DefinitionKind, ExprId, TraitImplKind}, + node_interner::{DefinitionId, DefinitionKind, DependencyId, ExprId, GlobalId, TraitImplKind}, Shared, StructType, Type, TypeBindings, }; @@ -27,7 +28,14 @@ impl<'context> Elaborator<'context> { expected_type: Type, definition_kind: DefinitionKind, ) -> HirPattern { - self.elaborate_pattern_mut(pattern, expected_type, definition_kind, None, &mut Vec::new()) + self.elaborate_pattern_mut( + pattern, + expected_type, + definition_kind, + None, + &mut Vec::new(), + None, + ) } /// Equivalent to `elaborate_pattern`, this version just also @@ -38,8 +46,16 @@ impl<'context> Elaborator<'context> { expected_type: Type, definition_kind: DefinitionKind, created_ids: &mut Vec, + global_id: Option, ) -> HirPattern { - self.elaborate_pattern_mut(pattern, expected_type, definition_kind, None, created_ids) + self.elaborate_pattern_mut( + pattern, + expected_type, + definition_kind, + None, + created_ids, + global_id, + ) } fn elaborate_pattern_mut( @@ -49,6 +65,7 @@ impl<'context> Elaborator<'context> { definition: DefinitionKind, mutable: Option, new_definitions: &mut Vec, + global_id: Option, ) -> HirPattern { match pattern { Pattern::Identifier(name) => { @@ -58,7 +75,14 @@ impl<'context> Elaborator<'context> { (Some(_), DefinitionKind::Local(_)) => DefinitionKind::Local(None), (_, other) => other, }; - let ident = self.add_variable_decl(name, mutable.is_some(), true, definition); + let ident = if let Some(global_id) = global_id { + // Globals don't need to be added to scope, they're already in the def_maps + let id = self.interner.get_global(global_id).definition_id; + let location = Location::new(name.span(), self.file); + HirIdent::non_trait_method(id, location) + } else { + self.add_variable_decl(name, mutable.is_some(), true, definition) + }; self.interner.push_definition_type(ident.id, expected_type); new_definitions.push(ident.clone()); HirPattern::Identifier(ident) @@ -74,6 +98,7 @@ impl<'context> Elaborator<'context> { definition, Some(span), new_definitions, + global_id, ); let location = Location::new(span, self.file); HirPattern::Mutable(Box::new(pattern), location) @@ -104,6 +129,7 @@ impl<'context> Elaborator<'context> { definition.clone(), mutable, new_definitions, + global_id, ) }); let location = Location::new(span, self.file); @@ -200,6 +226,7 @@ impl<'context> Elaborator<'context> { definition.clone(), mutable, new_definitions, + None, ); if unseen_fields.contains(&field) { @@ -300,7 +327,7 @@ impl<'context> Elaborator<'context> { let mut global_id = None; let global = self.interner.get_all_globals(); for global_info in global { - if global_info.ident == name && global_info.local_id == self.local_module { + if global_info.local_id == self.local_module && global_info.ident == name { global_id = Some(global_info.id); } } @@ -332,22 +359,6 @@ impl<'context> Elaborator<'context> { ident } - // Checks for a variable having been declared before. - // (Variable declaration and definition cannot be separate in Noir.) - // Once the variable has been found, intern and link `name` to this definition, - // returning (the ident, the IdentId of `name`) - // - // If a variable is not found, then an error is logged and a dummy id - // is returned, for better error reporting UX - pub(super) fn find_variable_or_default(&mut self, name: &Ident) -> (HirIdent, usize) { - self.use_variable(name).unwrap_or_else(|error| { - self.push_err(error); - let id = DefinitionId::dummy_id(); - let location = Location::new(name.span(), self.file); - (HirIdent::non_trait_method(id, location), 0) - }) - } - /// Lookup and use the specified variable. /// This will increment its use counter by one and return the variable if found. /// If the variable is not found, an error is returned. @@ -404,6 +415,16 @@ impl<'context> Elaborator<'context> { match self.interner.definition(hir_ident.id).kind { DefinitionKind::Function(id) => { if let Some(current_item) = self.current_item { + // Lazily evaluate functions found within globals if necessary. + // Otherwise if we later attempt to evaluate the global it will + // see an empty function body. + if matches!(current_item, DependencyId::Global(_)) { + let meta = self.interner.function_meta(&id); + + if matches!(&meta.function_body, FunctionBody::Unresolved(..)) { + self.elaborate_function(id); + } + } self.interner.add_function_dependency(current_item, id); } } @@ -555,7 +576,7 @@ impl<'context> Elaborator<'context> { } } - fn get_ident_from_path(&mut self, path: Path) -> (HirIdent, usize) { + pub fn get_ident_from_path(&mut self, path: Path) -> (HirIdent, usize) { let location = Location::new(path.span(), self.file); let error = match path.as_ident().map(|ident| self.use_variable(ident)) { diff --git a/compiler/noirc_frontend/src/elaborator/statements.rs b/compiler/noirc_frontend/src/elaborator/statements.rs index 5bcd43da6e5..dd3e2778726 100644 --- a/compiler/noirc_frontend/src/elaborator/statements.rs +++ b/compiler/noirc_frontend/src/elaborator/statements.rs @@ -14,7 +14,7 @@ use crate::{ }, }, macros_api::{ - ForLoopStatement, ForRange, HirStatement, LetStatement, Statement, StatementKind, + ForLoopStatement, ForRange, HirStatement, LetStatement, Path, Statement, StatementKind, }, node_interner::{DefinitionId, DefinitionKind, GlobalId, StmtId}, Type, @@ -53,39 +53,27 @@ impl<'context> Elaborator<'context> { } pub(super) fn elaborate_local_let(&mut self, let_stmt: LetStatement) -> (HirStatement, Type) { - let (statement, typ, _) = self.elaborate_let(let_stmt); - (statement, typ) + self.elaborate_let(let_stmt, None) } - /// Elaborates a global let statement. Compared to the local version, this - /// version fixes up the result to use the given DefinitionId rather than - /// the fresh one defined by the let pattern. - pub(super) fn elaborate_global_let(&mut self, let_stmt: LetStatement, global_id: GlobalId) { - let (let_statement, _typ, new_ids) = self.elaborate_let(let_stmt); - let statement_id = self.interner.get_global(global_id).let_statement; - - // To apply the changes from the fresh id created in elaborate_let to this global - // we need to change the definition kind and update the type. - assert_eq!(new_ids.len(), 1, "Globals should only define 1 value"); - let new_id = new_ids[0].id; - - self.interner.definition_mut(new_id).kind = DefinitionKind::Global(global_id); - - let definition_id = self.interner.get_global(global_id).definition_id; - let definition_type = self.interner.definition_type(new_id); - self.interner.push_definition_type(definition_id, definition_type); - - self.interner.replace_statement(statement_id, let_statement); - } - - /// Elaborate a local or global let statement. In addition to the HirLetStatement and unit - /// type, this also returns each HirIdent defined by this let statement. - fn elaborate_let(&mut self, let_stmt: LetStatement) -> (HirStatement, Type, Vec) { + /// Elaborate a local or global let statement. + /// If this is a global let, the DefinitionId of the global is specified so that + /// elaborate_pattern can create a Global definition kind with the correct ID + /// instead of a local one with a fresh ID. + pub(super) fn elaborate_let( + &mut self, + let_stmt: LetStatement, + global_id: Option, + ) -> (HirStatement, Type) { let expr_span = let_stmt.expression.span; let (expression, expr_type) = self.elaborate_expression(let_stmt.expression); - let definition = DefinitionKind::Local(Some(expression)); let annotated_type = self.resolve_type(let_stmt.r#type); + let definition = match global_id { + None => DefinitionKind::Local(Some(expression)), + Some(id) => DefinitionKind::Global(id), + }; + // First check if the LHS is unspecified // If so, then we give it the same type as the expression let r#type = if annotated_type != Type::Error { @@ -109,18 +97,18 @@ impl<'context> Elaborator<'context> { expr_type }; - let mut created_ids = Vec::new(); let pattern = self.elaborate_pattern_and_store_ids( let_stmt.pattern, r#type.clone(), definition, - &mut created_ids, + &mut Vec::new(), + global_id, ); let attributes = let_stmt.attributes; let comptime = let_stmt.comptime; let let_ = HirLetStatement { pattern, r#type, expression, attributes, comptime }; - (HirStatement::Let(let_), Type::Unit, created_ids) + (HirStatement::Let(let_), Type::Unit) } pub(super) fn elaborate_constrain(&mut self, stmt: ConstrainStatement) -> (HirStatement, Type) { @@ -250,7 +238,9 @@ impl<'context> Elaborator<'context> { match lvalue { LValue::Ident(ident) => { let mut mutable = true; - let (ident, scope_index) = self.find_variable_or_default(&ident); + let span = ident.span(); + let path = Path::from_single(ident.0.contents, span); + let (ident, scope_index) = self.get_ident_from_path(path); self.resolve_local_variable(ident.clone(), scope_index); let typ = if ident.id == DefinitionId::dummy_id() { diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index 30a6f50ad5a..3e04dbc784a 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -149,8 +149,6 @@ impl<'context> Elaborator<'context> { let old_generic_count = self.generics.len(); self.scopes.start_function(); - self.trait_bounds = where_clause.to_vec(); - let kind = FunctionKind::Normal; let def = FunctionDefinition { name: name.clone(), @@ -174,10 +172,9 @@ impl<'context> Elaborator<'context> { let mut function = NoirFunction { kind, def }; self.define_function_meta(&mut function, func_id, true); - self.elaborate_function(function, func_id); + self.elaborate_function(func_id); let _ = self.scopes.end_function(); // Don't check the scope tree for unused variables, they can't be used in a declaration anyway. - self.trait_bounds.clear(); self.generics.truncate(old_generic_count); } } diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 5e3ace423e5..159ad1c7493 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -5,10 +5,7 @@ use iter_extended::vecmap; use noirc_errors::{Location, Span}; use crate::{ - ast::{ - BinaryOpKind, IntegerBitSize, UnresolvedGenerics, UnresolvedTraitConstraint, - UnresolvedTypeExpression, - }, + ast::{BinaryOpKind, IntegerBitSize, UnresolvedGenerics, UnresolvedTypeExpression}, hir::{ comptime::{Interpreter, Value}, def_map::ModuleDefId, @@ -371,31 +368,18 @@ impl<'context> Elaborator<'context> { return None; } - for UnresolvedTraitConstraint { typ, trait_bound } in self.trait_bounds.clone() { - if let UnresolvedTypeData::Named(constraint_path, _, _) = &typ.typ { + for constraint in self.trait_bounds.clone() { + if let Type::NamedGeneric(_, name) = &constraint.typ { // if `path` is `T::method_name`, we're looking for constraint of the form `T: SomeTrait` - if constraint_path.segments.len() == 1 - && path.segments[0] != constraint_path.last_segment() - { + if path.segments[0].0.contents != name.as_str() { continue; } - if let Ok(ModuleDefId::TraitId(trait_id)) = - self.resolve_path(trait_bound.trait_path.clone()) + let the_trait = self.interner.get_trait(constraint.trait_id); + if let Some(method) = + the_trait.find_method(path.segments.last().unwrap().0.contents.as_str()) { - let the_trait = self.interner.get_trait(trait_id); - if let Some(method) = - the_trait.find_method(path.segments.last().unwrap().0.contents.as_str()) - { - let constraint = TraitConstraint { - trait_id, - typ: self.resolve_type(typ.clone()), - trait_generics: vecmap(trait_bound.trait_generics, |typ| { - self.resolve_type(typ) - }), - }; - return Some((method, constraint, true)); - } + return Some((method, constraint, true)); } } } diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index 8809e63d00a..e1e19ad653c 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -239,6 +239,11 @@ impl<'a> Interpreter<'a> { /// Mutate an existing variable, potentially from a prior scope fn mutate(&mut self, id: DefinitionId, argument: Value, location: Location) -> IResult<()> { + // If the id is a dummy, assume the error was already issued elsewhere + if id == DefinitionId::dummy_id() { + return Ok(()); + } + for scope in self.scopes.iter_mut().rev() { if let Entry::Occupied(mut entry) = scope.entry(id) { entry.insert(argument); @@ -253,7 +258,7 @@ impl<'a> Interpreter<'a> { self.lookup_id(ident.id, ident.location) } - fn lookup_id(&self, id: DefinitionId, location: Location) -> IResult { + pub fn lookup_id(&self, id: DefinitionId, location: Location) -> IResult { for scope in self.scopes.iter().rev() { if let Some(value) = scope.get(&id) { return Ok(value.clone()); diff --git a/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/compiler/noirc_frontend/src/hir/resolution/resolver.rs index 18e1a5a22ca..4988628db83 100644 --- a/compiler/noirc_frontend/src/hir/resolution/resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/resolver.rs @@ -20,6 +20,7 @@ use crate::hir_def::expr::{ HirMethodCallExpression, HirPrefixExpression, ImplKind, }; +use crate::hir_def::function::FunctionBody; use crate::hir_def::traits::{Trait, TraitConstraint}; use crate::macros_api::SecondaryAttribute; use crate::token::{Attributes, FunctionAttribute}; @@ -1078,10 +1079,11 @@ impl<'a> Resolver<'a> { is_entry_point: self.is_entry_point_function(func), has_inline_attribute, - // This is only used by the elaborator + // These fields are only used by the elaborator all_generics: Vec::new(), is_trait_function: false, parameter_idents: Vec::new(), + function_body: FunctionBody::Resolved, } } diff --git a/compiler/noirc_frontend/src/hir/type_check/mod.rs b/compiler/noirc_frontend/src/hir/type_check/mod.rs index 65a3186b004..98e1cd9c72a 100644 --- a/compiler/noirc_frontend/src/hir/type_check/mod.rs +++ b/compiler/noirc_frontend/src/hir/type_check/mod.rs @@ -452,6 +452,7 @@ pub mod test { PathResolution, PathResolutionError, PathResolutionResult, }; use crate::hir_def::expr::HirIdent; + use crate::hir_def::function::FunctionBody; use crate::hir_def::stmt::HirLetStatement; use crate::hir_def::stmt::HirPattern::Identifier; use crate::hir_def::types::Type; @@ -559,6 +560,7 @@ pub mod test { has_inline_attribute: false, all_generics: Vec::new(), parameter_idents: Vec::new(), + function_body: FunctionBody::Resolved, }; interner.push_fn_meta(func_meta, func_id); diff --git a/compiler/noirc_frontend/src/hir_def/function.rs b/compiler/noirc_frontend/src/hir_def/function.rs index 3bd85e94dca..53eabe21081 100644 --- a/compiler/noirc_frontend/src/hir_def/function.rs +++ b/compiler/noirc_frontend/src/hir_def/function.rs @@ -7,6 +7,7 @@ use super::expr::{HirBlockExpression, HirExpression, HirIdent}; use super::stmt::HirPattern; use super::traits::TraitConstraint; use crate::ast::{FunctionKind, FunctionReturnType, Visibility}; +use crate::macros_api::BlockExpression; use crate::node_interner::{ExprId, NodeInterner, TraitImplId}; use crate::{Type, TypeVariable}; @@ -142,6 +143,15 @@ pub struct FuncMeta { /// that indicates it should be inlined differently than the default (inline everything). /// For example, such as `fold` (never inlined) or `no_predicates` (inlined after flattening) pub has_inline_attribute: bool, + + pub function_body: FunctionBody, +} + +#[derive(Debug, Clone)] +pub enum FunctionBody { + Unresolved(FunctionKind, BlockExpression, Span), + Resolving, + Resolved, } impl FuncMeta { @@ -173,4 +183,19 @@ impl FuncMeta { _ => unreachable!(), } } + + /// Take this function body, returning an owned version while avoiding + /// cloning any large Expressions inside by replacing a Unresolved with a Resolving variant. + pub fn take_body(&mut self) -> FunctionBody { + match &mut self.function_body { + FunctionBody::Unresolved(kind, block, span) => { + let statements = std::mem::take(&mut block.statements); + let (kind, span) = (*kind, *span); + self.function_body = FunctionBody::Resolving; + FunctionBody::Unresolved(kind, BlockExpression { statements }, span) + } + FunctionBody::Resolving => FunctionBody::Resolving, + FunctionBody::Resolved => FunctionBody::Resolved, + } + } } diff --git a/compiler/noirc_frontend/src/monomorphization/errors.rs b/compiler/noirc_frontend/src/monomorphization/errors.rs index 3011c26cffe..2db570540d6 100644 --- a/compiler/noirc_frontend/src/monomorphization/errors.rs +++ b/compiler/noirc_frontend/src/monomorphization/errors.rs @@ -1,14 +1,12 @@ -use thiserror::Error; - use noirc_errors::{CustomDiagnostic, FileDiagnostic, Location}; -#[derive(Debug, Error)] +use crate::hir::comptime::InterpreterError; + +#[derive(Debug)] pub enum MonomorphizationError { - #[error("Length of generic array could not be determined.")] UnknownArrayLength { location: Location }, - - #[error("Type annotations needed")] TypeAnnotationsNeeded { location: Location }, + InterpreterError(InterpreterError), } impl MonomorphizationError { @@ -16,6 +14,7 @@ impl MonomorphizationError { match self { MonomorphizationError::UnknownArrayLength { location } | MonomorphizationError::TypeAnnotationsNeeded { location } => *location, + MonomorphizationError::InterpreterError(error) => error.get_location(), } } } @@ -31,9 +30,15 @@ impl From for FileDiagnostic { impl MonomorphizationError { fn into_diagnostic(self) -> CustomDiagnostic { - let message = self.to_string(); - let location = self.location(); + let message = match self { + MonomorphizationError::UnknownArrayLength { .. } => { + "Length of generic array could not be determined." + } + MonomorphizationError::TypeAnnotationsNeeded { .. } => "Type annotations needed", + MonomorphizationError::InterpreterError(error) => return (&error).into(), + }; - CustomDiagnostic::simple_error(message, String::new(), location.span) + let location = self.location(); + CustomDiagnostic::simple_error(message.into(), String::new(), location.span) } } diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index b4567bf9af7..a25d6488c83 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -877,12 +877,19 @@ impl<'interner> Monomorphizer<'interner> { } } DefinitionKind::Global(global_id) => { - let Some(let_) = self.interner.get_global_let_statement(*global_id) else { - unreachable!( - "Globals should have a corresponding let statement by monomorphization" - ) + let global = self.interner.get_global(*global_id); + + let expr = if let Some(value) = global.value.clone() { + value + .into_hir_expression(self.interner, global.location) + .map_err(MonomorphizationError::InterpreterError)? + } else { + let let_ = self.interner.get_global_let_statement(*global_id).expect( + "Globals should have a corresponding let statement by monomorphization", + ); + let_.expression }; - self.expr(let_.expression)? + self.expr(expr)? } DefinitionKind::Local(_) => match self.lookup_captured_expr(ident.id) { Some(expr) => expr, diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index c223017d752..40ca9ce04e0 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -13,6 +13,7 @@ use petgraph::prelude::NodeIndex as PetGraphIndex; use crate::ast::Ident; use crate::graph::CrateId; +use crate::hir::comptime; use crate::hir::def_collector::dc_crate::CompilationError; use crate::hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait, UnresolvedTypeAlias}; use crate::hir::def_map::{LocalModuleId, ModuleId}; @@ -466,6 +467,7 @@ pub struct GlobalInfo { pub local_id: LocalModuleId, pub location: Location, pub let_statement: StmtId, + pub value: Option, } impl Default for NodeInterner { @@ -681,6 +683,7 @@ impl NodeInterner { local_id, let_statement, location, + value: None, }); self.global_attributes.insert(id, attributes); id @@ -1003,6 +1006,10 @@ impl NodeInterner { &self.globals[global_id.0] } + pub fn get_global_mut(&mut self, global_id: GlobalId) -> &mut GlobalInfo { + &mut self.globals[global_id.0] + } + pub fn get_global_definition(&self, global_id: GlobalId) -> &DefinitionInfo { let global = self.get_global(global_id); self.definition(global.definition_id) diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index 53c3966cb4c..25a69489788 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -92,6 +92,17 @@ fn execution_success_{test_name}() {{ cmd.assert().success(); }} +#[test] +fn execution_success_elaborator_{test_name}() {{ + let test_program_dir = PathBuf::from("{test_dir}"); + + let mut cmd = Command::cargo_bin("nargo").unwrap(); + cmd.arg("--program-dir").arg(test_program_dir); + cmd.arg("execute").arg("--force").arg("--use-elaborator"); + + cmd.assert().success(); +}} + #[test]{brillig_ignored} fn execution_success_{test_name}_brillig() {{ let test_program_dir = PathBuf::from("{test_dir}"); @@ -137,6 +148,17 @@ fn execution_failure_{test_name}() {{ cmd.arg("--program-dir").arg(test_program_dir); cmd.arg("execute").arg("--force"); + cmd.assert().failure().stderr(predicate::str::contains("The application panicked (crashed).").not()); +}} + +#[test] +fn execution_failure_elaborator_{test_name}() {{ + let test_program_dir = PathBuf::from("{test_dir}"); + + let mut cmd = Command::cargo_bin("nargo").unwrap(); + cmd.arg("--program-dir").arg(test_program_dir); + cmd.arg("execute").arg("--force").arg("--use-elaborator"); + cmd.assert().failure().stderr(predicate::str::contains("The application panicked (crashed).").not()); }} "#, @@ -174,6 +196,17 @@ fn noir_test_success_{test_name}() {{ cmd.arg("--program-dir").arg(test_program_dir); cmd.arg("test"); + cmd.assert().success(); +}} + +#[test] +fn noir_test_success_elaborator_{test_name}() {{ + let test_program_dir = PathBuf::from("{test_dir}"); + + let mut cmd = Command::cargo_bin("nargo").unwrap(); + cmd.arg("--program-dir").arg(test_program_dir); + cmd.arg("test").arg("--use-elaborator"); + cmd.assert().success(); }} "#, @@ -211,6 +244,17 @@ fn noir_test_failure_{test_name}() {{ cmd.arg("--program-dir").arg(test_program_dir); cmd.arg("test"); + cmd.assert().failure(); +}} + +#[test] +fn noir_test_failure_elaborator_{test_name}() {{ + let test_program_dir = PathBuf::from("{test_dir}"); + + let mut cmd = Command::cargo_bin("nargo").unwrap(); + cmd.arg("--program-dir").arg(test_program_dir); + cmd.arg("test").arg("--use-elaborator"); + cmd.assert().failure(); }} "#, @@ -259,6 +303,30 @@ fn compile_success_empty_{test_name}() {{ panic!("`nargo info` failed with: {{}}", String::from_utf8(output.stderr).unwrap_or_default()); }} + // `compile_success_empty` tests should be able to compile down to an empty circuit. + let json: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap_or_else(|e| {{ + panic!("JSON was not well-formatted {{:?}}\n\n{{:?}}", e, std::str::from_utf8(&output.stdout)) + }}); + let num_opcodes = &json["programs"][0]["functions"][0]["acir_opcodes"]; + assert_eq!(num_opcodes.as_u64().expect("number of opcodes should fit in a u64"), 0); +}} + +#[test] +fn compile_success_empty_elaborator_{test_name}() {{ + let test_program_dir = PathBuf::from("{test_dir}"); + let mut cmd = Command::cargo_bin("nargo").unwrap(); + cmd.arg("--program-dir").arg(test_program_dir); + cmd.arg("info"); + cmd.arg("--json"); + cmd.arg("--force"); + cmd.arg("--use-elaborator"); + + let output = cmd.output().expect("Failed to execute command"); + + if !output.status.success() {{ + panic!("`nargo info` failed with: {{}}", String::from_utf8(output.stderr).unwrap_or_default()); + }} + // `compile_success_empty` tests should be able to compile down to an empty circuit. let json: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap_or_else(|e| {{ panic!("JSON was not well-formatted {{:?}}\n\n{{:?}}", e, std::str::from_utf8(&output.stdout)) @@ -301,6 +369,16 @@ fn compile_success_contract_{test_name}() {{ cmd.arg("--program-dir").arg(test_program_dir); cmd.arg("compile").arg("--force"); + cmd.assert().success(); +}} +#[test] +fn compile_success_contract_elaborator_{test_name}() {{ + let test_program_dir = PathBuf::from("{test_dir}"); + + let mut cmd = Command::cargo_bin("nargo").unwrap(); + cmd.arg("--program-dir").arg(test_program_dir); + cmd.arg("compile").arg("--force").arg("--use-elaborator"); + cmd.assert().success(); }} "#, @@ -338,6 +416,16 @@ fn compile_failure_{test_name}() {{ cmd.arg("--program-dir").arg(test_program_dir); cmd.arg("compile").arg("--force"); + cmd.assert().failure().stderr(predicate::str::contains("The application panicked (crashed).").not()); +}} +#[test] +fn compile_failure_elaborator_{test_name}() {{ + let test_program_dir = PathBuf::from("{test_dir}"); + + let mut cmd = Command::cargo_bin("nargo").unwrap(); + cmd.arg("--program-dir").arg(test_program_dir); + cmd.arg("compile").arg("--force").arg("--use-elaborator"); + cmd.assert().failure().stderr(predicate::str::contains("The application panicked (crashed).").not()); }} "#,