fix: Track graphs of item dependencies to find dependency cycles

compiler/noirc_frontend/Cargo.toml

@@ -23,6 +23,7 @@ rustc-hash = "1.1.0"
 small-ord-set = "0.1.3"
 regex = "1.9.1"
 tracing.workspace = true
+petgraph = "0.6"
 
 [dev-dependencies]
 strum = "0.24"

compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs

@@ -320,6 +320,7 @@ impl DefCollector {
         // We must wait to resolve non-integer globals until after we resolve structs since structs
         // globals will need to reference the struct type they're initialized to to ensure they are valid.
         resolved_globals.extend(resolve_globals(context, other_globals, crate_id));
+        errors.extend(resolved_globals.errors);
 
         // Bind trait impls to their trait. Collect trait functions, that have a
         // default implementation, which hasn't been overridden.
@@ -338,31 +339,31 @@ impl DefCollector {
         // over trait methods if there are name conflicts.
         errors.extend(collect_impls(context, crate_id, &def_collector.collected_impls));
 
-        // Lower each function in the crate. This is now possible since imports have been resolved
-        let file_func_ids = resolve_free_functions(
+        // Resolve each function in the crate. This is now possible since imports have been resolved
+        let mut functions = Vec::new();
+        functions.extend(resolve_free_functions(
             &mut context.def_interner,
             crate_id,
             &context.def_maps,
             def_collector.collected_functions,
             None,
             &mut errors,
-        );
+        ));
 
-        let file_method_ids = resolve_impls(
+        functions.extend(resolve_impls(
             &mut context.def_interner,
             crate_id,
             &context.def_maps,
             def_collector.collected_impls,
             &mut errors,
-        );
-        let file_trait_impls_ids = resolve_trait_impls(
+        ));
+
+        functions.extend(resolve_trait_impls(
             context,
             def_collector.collected_traits_impls,
             crate_id,
             &mut errors,
-        );
-
-        errors.extend(resolved_globals.errors);
+        ));
 
         for macro_processor in macro_processors {
             macro_processor.process_typed_ast(&crate_id, context).unwrap_or_else(
@@ -371,12 +372,11 @@ impl DefCollector {
                 },
             );
         }
-        errors.extend(type_check_globals(&mut context.def_interner, resolved_globals.globals));
 
-        // Type check all of the functions in the crate
-        errors.extend(type_check_functions(&mut context.def_interner, file_func_ids));
-        errors.extend(type_check_functions(&mut context.def_interner, file_method_ids));
-        errors.extend(type_check_functions(&mut context.def_interner, file_trait_impls_ids));
+        errors.extend(context.def_interner.check_for_dependency_cycles());
+
+        errors.extend(type_check_globals(&mut context.def_interner, resolved_globals.globals));
+        errors.extend(type_check_functions(&mut context.def_interner, functions));
         errors
     }
 }

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

@@ -86,6 +86,8 @@ pub enum ResolverError {
     NestedSlices { span: Span },
     #[error("Usage of the `#[foreign]` or `#[builtin]` function attributes are not allowed outside of the Noir standard library")]
     LowLevelFunctionOutsideOfStdlib { ident: Ident },
+    #[error("Dependency cycle found, '{item}' recursively depends on itself: {cycle} ")]
+    DependencyCycle { span: Span, item: String, cycle: String },
 }
 
 impl ResolverError {
@@ -318,6 +320,13 @@ impl From<ResolverError> for Diagnostic {
                 "Usage of the `#[foreign]` or `#[builtin]` function attributes are not allowed outside of the Noir standard library".into(),
                 ident.span(),
             ),
+            ResolverError::DependencyCycle { span, item, cycle } => {
+                Diagnostic::simple_error(
+                    "Dependency cycle found".into(),
+                    format!("'{item}' recursively depends on itself: {cycle}"),
+                    span,
+                )
+            },
         }
     }
 }

compiler/noirc_frontend/src/hir/resolution/resolver.rs

@@ -93,6 +93,9 @@ pub struct Resolver<'a> {
     /// to the corresponding trait impl ID.
     current_trait_impl: Option<TraitImplId>,
 
+    /// If we're currently resolving fields in a struct type, this is set to that type.
+    current_struct_type: Option<StructId>,
+
     /// True if the current module is a contract.
     /// This is usually determined by self.path_resolver.module_id(), but it can
     /// be overridden for impls. Impls are an odd case since the methods within resolve
@@ -148,6 +151,7 @@ impl<'a> Resolver<'a> {
             errors: Vec::new(),
             lambda_stack: Vec::new(),
             current_trait_impl: None,
+            current_struct_type: None,
             file,
             in_contract,
         }
@@ -599,6 +603,11 @@ impl<'a> Resolver<'a> {
                     struct_type.borrow().to_string()
                 });
 
+                if let Some(current_struct) = self.current_struct_type {
+                    let dependency_id = struct_type.borrow().id;
+                    self.interner.add_type_dependency(current_struct, dependency_id);
+                }
+
                 Type::Struct(struct_type, args)
             }
             None => Type::Error,
@@ -651,6 +660,9 @@ impl<'a> Resolver<'a> {
         // If we cannot find a local generic of the same name, try to look up a global
         match self.path_resolver.resolve(self.def_maps, path.clone()) {
             Ok(ModuleDefId::GlobalId(id)) => {
+                if let Some(current_struct) = self.current_struct_type {
+                    self.interner.add_type_global_dependency(current_struct, id);
+                }
                 Some(Type::Constant(self.eval_global_as_array_length(id)))
             }
             _ => None,
@@ -830,12 +842,14 @@ impl<'a> Resolver<'a> {
     pub fn resolve_struct_fields(
         mut self,
         unresolved: NoirStruct,
+        struct_id: StructId,
     ) -> (Generics, Vec<(Ident, Type)>, Vec<ResolverError>) {
         let generics = self.add_generics(&unresolved.generics);
 
         // Check whether the struct definition has globals in the local module and add them to the scope
         self.resolve_local_globals();
 
+        self.current_struct_type = Some(struct_id);
         let fields = vecmap(unresolved.fields, |(ident, typ)| (ident, self.resolve_type(typ)));
 
         (generics, fields, self.errors)
@@ -1577,13 +1591,15 @@ impl<'a> Resolver<'a> {
                 }
 
                 let pattern = self.resolve_pattern_mutable(*pattern, Some(span), definition);
-                HirPattern::Mutable(Box::new(pattern), span)
+                let location = Location::new(span, self.file);
+                HirPattern::Mutable(Box::new(pattern), location)
             }
             Pattern::Tuple(fields, span) => {
                 let fields = vecmap(fields, |field| {
                     self.resolve_pattern_mutable(field, mutable, definition.clone())
                 });
-                HirPattern::Tuple(fields, span)
+                let location = Location::new(span, self.file);
+                HirPattern::Tuple(fields, location)
             }
             Pattern::Struct(name, fields, span) => {
                 let error_identifier = |this: &mut Self| {
@@ -1612,7 +1628,8 @@ impl<'a> Resolver<'a> {
                 let fields = self.resolve_constructor_fields(typ, fields, span, resolve_field);
 
                 let typ = Type::Struct(struct_type, generics);
-                HirPattern::Struct(typ, fields, span)
+                let location = Location::new(span, self.file);
+                HirPattern::Struct(typ, fields, location)
             }
         }
     }

compiler/noirc_frontend/src/hir/resolution/structs.rs

@@ -32,7 +32,8 @@ pub(crate) fn resolve_structs(
     // Each struct should already be present in the NodeInterner after def collection.
     for (type_id, typ) in structs {
         let file_id = typ.file_id;
-        let (generics, fields, resolver_errors) = resolve_struct_fields(context, crate_id, typ);
+        let (generics, fields, resolver_errors) =
+            resolve_struct_fields(context, crate_id, type_id, typ);
         errors.extend(vecmap(resolver_errors, |err| (err.into(), file_id)));
         context.def_interner.update_struct(type_id, |struct_def| {
             struct_def.set_fields(fields);
@@ -67,14 +68,15 @@ pub(crate) fn resolve_structs(
 fn resolve_struct_fields(
     context: &mut Context,
     krate: CrateId,
+    type_id: StructId,
     unresolved: UnresolvedStruct,
 ) -> (Generics, Vec<(Ident, Type)>, Vec<ResolverError>) {
     let path_resolver =
         StandardPathResolver::new(ModuleId { local_id: unresolved.module_id, krate });
     let file_id = unresolved.file_id;
     let (generics, fields, errors) =
         Resolver::new(&mut context.def_interner, &path_resolver, &context.def_maps, file_id)
-            .resolve_struct_fields(unresolved.struct_def);
+            .resolve_struct_fields(unresolved.struct_def, type_id);
 
     (generics, fields, errors)
 }

compiler/noirc_frontend/src/hir/type_check/stmt.rs

@@ -93,7 +93,7 @@ impl<'interner> TypeChecker<'interner> {
         match pattern {
             HirPattern::Identifier(ident) => self.interner.push_definition_type(ident.id, typ),
             HirPattern::Mutable(pattern, _) => self.bind_pattern(pattern, typ),
-            HirPattern::Tuple(fields, span) => match typ {
+            HirPattern::Tuple(fields, location) => match typ {
                 Type::Tuple(field_types) if field_types.len() == fields.len() => {
                     for (field, field_type) in fields.iter().zip(field_types) {
                         self.bind_pattern(field, field_type);
@@ -107,16 +107,16 @@ impl<'interner> TypeChecker<'interner> {
                     self.errors.push(TypeCheckError::TypeMismatchWithSource {
                         expected,
                         actual: other,
-                        span: *span,
+                        span: location.span,
                         source: Source::Assignment,
                     });
                 }
             },
-            HirPattern::Struct(struct_type, fields, span) => {
+            HirPattern::Struct(struct_type, fields, location) => {
                 self.unify(struct_type, &typ, || TypeCheckError::TypeMismatchWithSource {
                     expected: struct_type.clone(),
                     actual: typ.clone(),
-                    span: *span,
+                    span: location.span,
                     source: Source::Assignment,
                 });
 

compiler/noirc_frontend/src/hir_def/function.rs

@@ -43,12 +43,7 @@ pub struct Parameters(pub Vec<Param>);
 impl Parameters {
     pub fn span(&self) -> Span {
         assert!(!self.is_empty());
-        let mut spans = vecmap(&self.0, |param| match &param.0 {
-            HirPattern::Identifier(ident) => ident.location.span,
-            HirPattern::Mutable(_, span) => *span,
-            HirPattern::Tuple(_, span) => *span,
-            HirPattern::Struct(_, _, span) => *span,
-        });
+        let mut spans = vecmap(&self.0, |param| param.0.span());
 
         let merged_span = spans.pop().unwrap();
         for span in spans {

compiler/noirc_frontend/src/hir_def/stmt.rs

@@ -2,7 +2,7 @@ use super::expr::HirIdent;
 use crate::node_interner::ExprId;
 use crate::{Ident, Type};
 use fm::FileId;
-use noirc_errors::Span;
+use noirc_errors::{Location, Span};
 
 /// A HirStatement is the result of performing name resolution on
 /// the Statement AST node. Unlike the AST node, any nested nodes
@@ -60,9 +60,9 @@ pub struct HirConstrainStatement(pub ExprId, pub FileId, pub Option<ExprId>);
 #[derive(Debug, Clone, Hash)]
 pub enum HirPattern {
     Identifier(HirIdent),
-    Mutable(Box<HirPattern>, Span),
-    Tuple(Vec<HirPattern>, Span),
-    Struct(Type, Vec<(Ident, HirPattern)>, Span),
+    Mutable(Box<HirPattern>, Location),
+    Tuple(Vec<HirPattern>, Location),
+    Struct(Type, Vec<(Ident, HirPattern)>, Location),
 }
 
 impl HirPattern {
@@ -92,9 +92,9 @@ impl HirPattern {
     pub fn span(&self) -> Span {
         match self {
             HirPattern::Identifier(ident) => ident.location.span,
-            HirPattern::Mutable(_, span)
-            | HirPattern::Tuple(_, span)
-            | HirPattern::Struct(_, _, span) => *span,
+            HirPattern::Mutable(_, location)
+            | HirPattern::Tuple(_, location)
+            | HirPattern::Struct(_, _, location) => location.span,
         }
     }
 }