From 1d67c12e4ccb7a549dadb563eeb5feb2fc22ad2c Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 18 Dec 2024 12:32:59 -0300 Subject: [PATCH 1/4] Small refactor to avoid doing `self.elaborator.interner.get_global` twice --- compiler/noirc_frontend/src/hir/comptime/interpreter.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index b6ae462976..b0faaf9f2c 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -568,11 +568,11 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { DefinitionKind::Local(_) => self.lookup(&ident), DefinitionKind::Global(global_id) => { // Avoid resetting the value if it is already known - if let Some(value) = &self.elaborator.interner.get_global(*global_id).value { + let global_id = *global_id; + let global_info = self.elaborator.interner.get_global(global_id); + if let Some(value) = &global_info.value { Ok(value.clone()) } else { - let global_id = *global_id; - let crate_of_global = self.elaborator.interner.get_global(global_id).crate_id; let let_ = self.elaborator.interner.get_global_let_statement(global_id).ok_or_else( || { @@ -581,7 +581,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { }, )?; - if let_.runs_comptime() || crate_of_global != self.crate_id { + if let_.runs_comptime() || global_info.crate_id != self.crate_id { self.evaluate_let(let_.clone())?; } From 4053db3c3ef6d7f00661d67f06d5d74e5905134d Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 18 Dec 2024 13:10:17 -0300 Subject: [PATCH 2/4] fix: detect cycles in globals --- .../noirc_frontend/src/hir/comptime/errors.rs | 11 +++++++++- .../src/hir/comptime/interpreter.rs | 15 +++++++++++++- compiler/noirc_frontend/src/tests.rs | 20 +++++++++++++++++++ 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/comptime/errors.rs b/compiler/noirc_frontend/src/hir/comptime/errors.rs index 3df20b3920..6ff918328a 100644 --- a/compiler/noirc_frontend/src/hir/comptime/errors.rs +++ b/compiler/noirc_frontend/src/hir/comptime/errors.rs @@ -243,6 +243,9 @@ pub enum InterpreterError { CannotInterpretFormatStringWithErrors { location: Location, }, + GlobalsDependencyCycle { + location: Location, + }, // These cases are not errors, they are just used to prevent us from running more code // until the loop can be resumed properly. These cases will never be displayed to users. @@ -319,7 +322,8 @@ impl InterpreterError { | InterpreterError::CannotResolveExpression { location, .. } | InterpreterError::CannotSetFunctionBody { location, .. } | InterpreterError::UnknownArrayLength { location, .. } - | InterpreterError::CannotInterpretFormatStringWithErrors { location } => *location, + | InterpreterError::CannotInterpretFormatStringWithErrors { location } + | InterpreterError::GlobalsDependencyCycle { location } => *location, InterpreterError::FailedToParseMacro { error, file, .. } => { Location::new(error.span(), *file) @@ -674,6 +678,11 @@ impl<'a> From<&'a InterpreterError> for CustomDiagnostic { "Some of the variables to interpolate could not be evaluated".to_string(); CustomDiagnostic::simple_error(msg, secondary, location.span) } + InterpreterError::GlobalsDependencyCycle { location } => { + let msg = "This global recursively depends on itself".to_string(); + let secondary = String::new(); + CustomDiagnostic::simple_error(msg, secondary, location.span) + } } } } diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index b0faaf9f2c..7b52d1aae3 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -1,4 +1,4 @@ -use std::collections::VecDeque; +use std::collections::{HashSet, VecDeque}; use std::{collections::hash_map::Entry, rc::Rc}; use acvm::blackbox_solver::BigIntSolverWithId; @@ -20,6 +20,7 @@ use crate::monomorphization::{ perform_impl_bindings, perform_instantiation_bindings, resolve_trait_method, undo_instantiation_bindings, }; +use crate::node_interner::GlobalId; use crate::token::{FmtStrFragment, Tokens}; use crate::TypeVariable; use crate::{ @@ -66,6 +67,12 @@ pub struct Interpreter<'local, 'interner> { /// Stateful bigint calculator. bigint_solver: BigIntSolverWithId, + + /// Globals currently being interpreted, to detect recursive cycles. + /// Note that recursive cycles are also detected by NodeInterner, + /// it's just that the error message there happens after we evaluate globals: + /// if we don't detect cycles here too the program will stack overflow. + globals_being_interpreted: HashSet, } #[allow(unused)] @@ -82,6 +89,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { bound_generics: Vec::new(), in_loop: false, bigint_solver: BigIntSolverWithId::default(), + globals_being_interpreted: HashSet::new(), } } @@ -572,6 +580,9 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { let global_info = self.elaborator.interner.get_global(global_id); if let Some(value) = &global_info.value { Ok(value.clone()) + } else if self.globals_being_interpreted.contains(&global_id) { + let location = self.elaborator.interner.expr_location(&id); + Err(InterpreterError::GlobalsDependencyCycle { location }) } else { let let_ = self.elaborator.interner.get_global_let_statement(global_id).ok_or_else( @@ -582,7 +593,9 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { )?; if let_.runs_comptime() || global_info.crate_id != self.crate_id { + self.globals_being_interpreted.insert(global_id); self.evaluate_let(let_.clone())?; + self.globals_being_interpreted.remove(&global_id); } let value = self.lookup(&ident)?; diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 1aed3d62d3..2d7cf8acca 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3855,3 +3855,23 @@ fn disallows_underscore_on_right_hand_side() { assert_eq!(name, "_"); } + +#[test] +fn errors_on_cyclic_globals() { + let src = r#" + pub comptime global A: u32 = B; + pub comptime global B: u32 = A; + + fn main() { } + "#; + let errors = get_program_errors(src); + + assert!(errors.iter().any(|(error, _)| matches!( + error, + CompilationError::InterpreterError(InterpreterError::GlobalsDependencyCycle { .. }) + ))); + assert!(errors.iter().any(|(error, _)| matches!( + error, + CompilationError::ResolverError(ResolverError::DependencyCycle { .. }) + ))); +} From 2d6c5a03c43bf36910845bccaa4016967dcdc5db Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 18 Dec 2024 14:34:12 -0300 Subject: [PATCH 3/4] Track global value resolution in GlobalInfo --- compiler/noirc_frontend/src/elaborator/mod.rs | 6 +- .../noirc_frontend/src/elaborator/types.rs | 7 ++- .../src/hir/comptime/interpreter.rs | 55 +++++++++---------- .../src/monomorphization/mod.rs | 4 +- compiler/noirc_frontend/src/node_interner.rs | 11 +++- 5 files changed, 45 insertions(+), 38 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 4a5947f15e..55144f8944 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -4,8 +4,8 @@ use std::{ }; use crate::{ - ast::ItemVisibility, hir_def::traits::ResolvedTraitBound, usage_tracker::UsageTracker, - StructField, StructType, TypeBindings, + ast::ItemVisibility, hir_def::traits::ResolvedTraitBound, node_interner::GlobalValue, + usage_tracker::UsageTracker, StructField, StructType, TypeBindings, }; use crate::{ ast::{ @@ -1689,7 +1689,7 @@ impl<'context> Elaborator<'context> { self.debug_comptime(location, |interner| value.display(interner).to_string()); - self.interner.get_global_mut(global_id).value = Some(value); + self.interner.get_global_mut(global_id).value = GlobalValue::Resolved(value); } } diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 3551edf93f..9aafd690bb 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -28,8 +28,8 @@ use crate::{ traits::{NamedType, ResolvedTraitBound, Trait, TraitConstraint}, }, node_interner::{ - DependencyId, ExprId, ImplSearchErrorKind, NodeInterner, TraitId, TraitImplKind, - TraitMethodId, + DependencyId, ExprId, GlobalValue, ImplSearchErrorKind, NodeInterner, TraitId, + TraitImplKind, TraitMethodId, }, token::SecondaryAttribute, Generics, Kind, ResolvedGeneric, Type, TypeBinding, TypeBindings, UnificationError, @@ -426,7 +426,8 @@ impl<'context> Elaborator<'context> { let rhs = stmt.expression; let span = self.interner.expr_span(&rhs); - let Some(global_value) = &self.interner.get_global(id).value else { + let GlobalValue::Resolved(global_value) = &self.interner.get_global(id).value + else { self.push_err(ResolverError::UnevaluatedGlobalType { span }); return None; }; diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index 7b52d1aae3..fecf7fda02 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -1,4 +1,4 @@ -use std::collections::{HashSet, VecDeque}; +use std::collections::VecDeque; use std::{collections::hash_map::Entry, rc::Rc}; use acvm::blackbox_solver::BigIntSolverWithId; @@ -20,7 +20,7 @@ use crate::monomorphization::{ perform_impl_bindings, perform_instantiation_bindings, resolve_trait_method, undo_instantiation_bindings, }; -use crate::node_interner::GlobalId; +use crate::node_interner::GlobalValue; use crate::token::{FmtStrFragment, Tokens}; use crate::TypeVariable; use crate::{ @@ -67,12 +67,6 @@ pub struct Interpreter<'local, 'interner> { /// Stateful bigint calculator. bigint_solver: BigIntSolverWithId, - - /// Globals currently being interpreted, to detect recursive cycles. - /// Note that recursive cycles are also detected by NodeInterner, - /// it's just that the error message there happens after we evaluate globals: - /// if we don't detect cycles here too the program will stack overflow. - globals_being_interpreted: HashSet, } #[allow(unused)] @@ -89,7 +83,6 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { bound_generics: Vec::new(), in_loop: false, bigint_solver: BigIntSolverWithId::default(), - globals_being_interpreted: HashSet::new(), } } @@ -578,29 +571,35 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { // Avoid resetting the value if it is already known let global_id = *global_id; let global_info = self.elaborator.interner.get_global(global_id); - if let Some(value) = &global_info.value { - Ok(value.clone()) - } else if self.globals_being_interpreted.contains(&global_id) { - let location = self.elaborator.interner.expr_location(&id); - Err(InterpreterError::GlobalsDependencyCycle { location }) - } else { - let let_ = - self.elaborator.interner.get_global_let_statement(global_id).ok_or_else( - || { + let global_crate_id = global_info.crate_id; + match &global_info.value { + GlobalValue::Resolved(value) => Ok(value.clone()), + GlobalValue::Resolving => { + let location = self.elaborator.interner.expr_location(&id); + Err(InterpreterError::GlobalsDependencyCycle { location }) + } + GlobalValue::Unresolved => { + let let_ = self + .elaborator + .interner + .get_global_let_statement(global_id) + .ok_or_else(|| { let location = self.elaborator.interner.expr_location(&id); InterpreterError::VariableNotInScope { location } - }, - )?; + })?; - if let_.runs_comptime() || global_info.crate_id != self.crate_id { - self.globals_being_interpreted.insert(global_id); - self.evaluate_let(let_.clone())?; - self.globals_being_interpreted.remove(&global_id); - } + self.elaborator.interner.get_global_mut(global_id).value = + GlobalValue::Resolving; - let value = self.lookup(&ident)?; - self.elaborator.interner.get_global_mut(global_id).value = Some(value.clone()); - Ok(value) + if let_.runs_comptime() || global_crate_id != self.crate_id { + self.evaluate_let(let_.clone())?; + } + + let value = self.lookup(&ident)?; + self.elaborator.interner.get_global_mut(global_id).value = + GlobalValue::Resolved(value.clone()); + Ok(value) + } } } DefinitionKind::NumericGeneric(type_variable, numeric_typ) => { diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 5297a89cc9..8c07d71de2 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -11,7 +11,7 @@ use crate::ast::{FunctionKind, IntegerBitSize, Signedness, UnaryOp, Visibility}; use crate::hir::comptime::InterpreterError; use crate::hir::type_check::{NoMatchingImplFoundError, TypeCheckError}; -use crate::node_interner::{ExprId, ImplSearchErrorKind}; +use crate::node_interner::{ExprId, GlobalValue, ImplSearchErrorKind}; use crate::token::FmtStrFragment; use crate::{ debug::DebugInstrumenter, @@ -895,7 +895,7 @@ impl<'interner> Monomorphizer<'interner> { DefinitionKind::Global(global_id) => { let global = self.interner.get_global(*global_id); - let expr = if let Some(value) = global.value.clone() { + let expr = if let GlobalValue::Resolved(value) = global.value.clone() { value .into_hir_expression(self.interner, global.location) .map_err(MonomorphizationError::InterpreterError)? diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index ae30b68c09..bee703a4de 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -609,7 +609,14 @@ pub struct GlobalInfo { pub crate_id: CrateId, pub location: Location, pub let_statement: StmtId, - pub value: Option, + pub value: GlobalValue, +} + +#[derive(Debug, Clone)] +pub enum GlobalValue { + Unresolved, + Resolving, + Resolved(comptime::Value), } #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] @@ -861,7 +868,7 @@ impl NodeInterner { crate_id, let_statement, location, - value: None, + value: GlobalValue::Unresolved, }); self.global_attributes.insert(id, attributes); id From 989cb6aa1443f711136f2c9d464ff902a5d25e86 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 18 Dec 2024 14:36:18 -0300 Subject: [PATCH 4/4] Explain why the error we produce isn't very informative --- compiler/noirc_frontend/src/hir/comptime/interpreter.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index fecf7fda02..e9e37243e0 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -575,6 +575,8 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { match &global_info.value { GlobalValue::Resolved(value) => Ok(value.clone()), GlobalValue::Resolving => { + // Note that the error we issue here isn't very informative (it doesn't include the actual cycle) + // but the general dependency cycle detector will give a better error later on during compilation. let location = self.elaborator.interner.expr_location(&id); Err(InterpreterError::GlobalsDependencyCycle { location }) }