fix: Track graphs of item dependencies to find dependency cycles (#4266)

# Description

## Problem\*

Resolves #1122

## Summary\*

Adds a dependency graph to catch cases where structs will cyclically
depend on one other.

For example, the program:

```rs
struct Foo {
    bar: Bar,
}

struct Bar {
    foo: Foo,
}
```
Will now give the following error:
```
error: Dependency cycle found
  ┌─ /home/user/Code/Noir/noir/short/src/main.nr:6:1
  │  
6 │ ╭ struct Bar {
7 │ │     foo: Foo,
8 │ │ }
  │ ╰─' 'Bar' recursively depends on itself: Bar -> Foo -> Bar
  │  
```
The error is still issued if these are split across separate modules.

## Additional Context

Note that self-referential structs like `struct Foo { foo: Foo }` are
not currently caught - we may need a separate check for this.

This error is also only currently possible for cyclic struct
dependencies, but once this is merged I'm going to expand this to more
PRs to allow for globals and type aliases as well. This would implement
#1734 as well as a similar
feature to allow type aliases to reference other type aliases which is
currently disallowed.

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** 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: kevaundray <[email protected]>

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