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

#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 <[email protected]>

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" }

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<ExpressionWidth, std::io::Error
     }
 }
 
-#[derive(Debug, Error)]
+#[derive(Debug)]
 pub enum CompileError {
-    #[error(transparent)]
-    MonomorphizationError(#[from] MonomorphizationError),
+    MonomorphizationError(MonomorphizationError),
+    RuntimeError(RuntimeError),
+}
 
-    #[error(transparent)]
-    RuntimeError(#[from] RuntimeError),
+impl From<MonomorphizationError> for CompileError {
+    fn from(error: MonomorphizationError) -> Self {
+        Self::MonomorphizationError(error)
+    }
+}
+
+impl From<RuntimeError> for CompileError {
+    fn from(error: RuntimeError) -> Self {
+        Self::RuntimeError(error)
+    }
 }
 
 impl From<CompileError> for FileDiagnostic {

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<StructId>,
 
-    trait_bounds: Vec<UnresolvedTraitConstraint>,
+    /// Each constraint in the `where` clause of the function currently being resolved.
+    trait_bounds: Vec<TraitConstraint>,
 
     current_function: Option<FuncId>,
 
@@ -271,38 +277,47 @@ 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() {
             self.in_contract = false;
         }
 
         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<TraitId> {
         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],