fix: detect cycles in globals (#6859)

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);
         }
     }
 

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

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)
+            }
         }
     }
 }

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

@@ -20,6 +20,7 @@ use crate::monomorphization::{
     perform_impl_bindings, perform_instantiation_bindings, resolve_trait_method,
     undo_instantiation_bindings,
 };
+use crate::node_interner::GlobalValue;
 use crate::token::{FmtStrFragment, Tokens};
 use crate::TypeVariable;
 use crate::{
@@ -568,26 +569,39 @@ 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 {
-                    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(
-                            || {
+                let global_id = *global_id;
+                let global_info = self.elaborator.interner.get_global(global_id);
+                let global_crate_id = global_info.crate_id;
+                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 })
+                    }
+                    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() || crate_of_global != self.crate_id {
-                        self.evaluate_let(let_.clone())?;
-                    }
+                        self.elaborator.interner.get_global_mut(global_id).value =
+                            GlobalValue::Resolving;
+
+                        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 = Some(value.clone());
-                    Ok(value)
+                        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) => {

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)?

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<comptime::Value>,
+    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

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 { .. })
+    )));
+}