fix: detect cycles in globals

compiler/noirc_frontend/src/hir/comptime/errors.rs

@@ -243,6 +243,9 @@
     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 @@
             | 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)
@@ -631,7 +635,7 @@
             }
             InterpreterError::GenericNameShouldBeAnIdent { name, location } => {
                 let msg =
                     "Generic name needs to be a valid identifer (one word beginning with a letter)"
                         .to_string();
                 let secondary = format!("`{name}` is not a valid identifier");
                 CustomDiagnostic::simple_error(msg, secondary, location.span)
@@ -674,6 +678,11 @@
                     "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)
+            }
         }
     }
 }

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 @@
     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 @@
 
     /// 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<GlobalId>,
 }
 
 #[allow(unused)]
@@ -82,6 +89,7 @@
             bound_generics: Vec::new(),
             in_loop: false,
             bigint_solver: BigIntSolverWithId::default(),
+            globals_being_interpreted: HashSet::new(),
         }
     }
 
@@ -251,7 +259,7 @@
             }
         } else {
             let name = self.elaborator.interner.function_name(&function);
             unreachable!("Non-builtin, lowlevel or oracle builtin fn '{name}'")
         }
     }
 
@@ -568,11 +576,14 @@
             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 if self.globals_being_interpreted.contains(&global_id) {
+                    let location = self.elaborator.interner.expr_location(&id);
+                    Err(InterpreterError::GlobalsDependencyCycle { location })
                 } 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,8 +592,10 @@
                             },
                         )?;
 
-                    if let_.runs_comptime() || crate_of_global != self.crate_id {
+                    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)?;
@@ -1032,7 +1045,7 @@
         }
 
         /// Generate matches for bit shifting, which in Noir only accepts `u8` for RHS.
         macro_rules! match_bitshift {
             (($lhs_value:ident as $lhs:ident $op:literal $rhs_value:ident as $rhs:ident) => $expr:expr) => {
                 match_values! {
                     ($lhs_value as $lhs $op $rhs_value as $rhs) {
@@ -1102,10 +1115,10 @@
             BinaryOpKind::Xor => match_bitwise! {
                 (lhs_value as lhs "^" rhs_value as rhs) => lhs ^ rhs
             },
             BinaryOpKind::ShiftRight => match_bitshift! {
                 (lhs_value as lhs ">>" rhs_value as rhs) => lhs.checked_shr(rhs.into())
             },
             BinaryOpKind::ShiftLeft => match_bitshift! {
                 (lhs_value as lhs "<<" rhs_value as rhs) => lhs.checked_shl(rhs.into())
             },
             BinaryOpKind::Modulo => match_integer! {

compiler/noirc_frontend/src/tests.rs

@@ -3170,7 +3170,7 @@
 }
 
 #[test]
 fn dont_infer_globals_to_u32_from_type_use() {
     let src = r#"
         global ARRAY_LEN = 3;
         global STR_LEN: _ = 2;
@@ -3200,7 +3200,7 @@
 }
 
 #[test]
 fn dont_infer_partial_global_types() {
     let src = r#"
         pub global ARRAY: [Field; _] = [0; 3];
         pub global NESTED_ARRAY: [[Field; _]; 3] = [[]; 3];
@@ -3438,7 +3438,7 @@
 
 #[test]
 fn unconditional_recursion_fail() {
     let srcs = vec![
         r#"
         fn main() {
             main()
@@ -3500,7 +3500,7 @@
         "#,
     ];
 
     for src in srcs {
         let errors = get_program_errors(src);
         assert!(
             !errors.is_empty(),
@@ -3519,7 +3519,7 @@
 
 #[test]
 fn unconditional_recursion_pass() {
     let srcs = vec![
         r#"
         fn main() {
             if false { main(); }
@@ -3855,3 +3855,23 @@
 
     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 { .. })
+    )));
+}