From 11c7c9df806303b45c50398793693ba682b25e18 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 1 Feb 2024 15:46:22 -0600 Subject: [PATCH 1/4] Add basic dependency graph; catch struct cycles --- Cargo.lock | 17 +++ compiler/noirc_frontend/Cargo.toml | 1 + .../src/hir/def_collector/dc_crate.rs | 28 ++--- .../src/hir/resolution/errors.rs | 9 ++ .../src/hir/resolution/structs.rs | 63 +++++++++- compiler/noirc_frontend/src/node_interner.rs | 113 ++++++++++++++++++ 6 files changed, 216 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1585d81d6e1..d80bb82059d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1662,6 +1662,12 @@ dependencies = [ "winapi", ] +[[package]] +name = "fixedbitset" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0ce7134b9999ecaf8bcd65542e436736ef32ddca1b3e06094cb6ec5755203b80" + [[package]] name = "flate2" version = "1.0.28" @@ -2981,6 +2987,7 @@ dependencies = [ "iter-extended", "noirc_errors", "noirc_printable_type", + "petgraph", "regex", "rustc-hash", "serde", @@ -3192,6 +3199,16 @@ version = "2.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9b2a4787296e9989611394c33f193f676704af1686e70b8f8033ab5ba9a35a94" +[[package]] +name = "petgraph" +version = "0.6.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e1d3afd2628e69da2be385eb6f2fd57c8ac7977ceeff6dc166ff1657b0e386a9" +dependencies = [ + "fixedbitset", + "indexmap 2.0.0", +] + [[package]] name = "phf" version = "0.10.1" diff --git a/compiler/noirc_frontend/Cargo.toml b/compiler/noirc_frontend/Cargo.toml index 80d767f7f2c..a3a8d460572 100644 --- a/compiler/noirc_frontend/Cargo.toml +++ b/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" diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index f7441750fc8..69743935dc3 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/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 } } diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index 7bd4de77e84..d15196b10c0 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/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 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, + ) + }, } } } diff --git a/compiler/noirc_frontend/src/hir/resolution/structs.rs b/compiler/noirc_frontend/src/hir/resolution/structs.rs index cf3e3436c88..9534cfa14de 100644 --- a/compiler/noirc_frontend/src/hir/resolution/structs.rs +++ b/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,6 +68,7 @@ pub(crate) fn resolve_structs( fn resolve_struct_fields( context: &mut Context, krate: CrateId, + type_id: StructId, unresolved: UnresolvedStruct, ) -> (Generics, Vec<(Ident, Type)>, Vec) { let path_resolver = @@ -76,5 +78,64 @@ fn resolve_struct_fields( Resolver::new(&mut context.def_interner, &path_resolver, &context.def_maps, file_id) .resolve_struct_fields(unresolved.struct_def); + // Register any other types used by this struct as a dependency in the dependency graph + // TODO: Need to move this to while we're resolving an UnresolvedType, not after. + // Otherwise we can't see global variable dependencies. + for (_, field) in &fields { + add_type_dependency(context, type_id, field); + } + (generics, fields, errors) } + +fn add_type_dependency(context: &mut Context, struct_id: StructId, typ: &Type) { + match typ { + Type::FieldElement + | Type::Integer(_, _) + | Type::Bool + | Type::Unit + | Type::NotConstant + | Type::Constant(_) + | Type::Error => (), + + // We don't count traits as dependencies since the type represented + // by an impl trait is opaque and not needed until type checking. + Type::TraitAsType(_, _, _) => (), + + Type::Array(length, elem) | Type::FmtString(length, elem) => { + add_type_dependency(context, struct_id, length); + add_type_dependency(context, struct_id, elem); + } + + Type::String(length) => { + add_type_dependency(context, struct_id, length); + } + Type::Struct(other_struct, _) => { + let dependency_id = other_struct.borrow().id; + context.def_interner.add_type_dependency(struct_id, dependency_id); + } + Type::Tuple(fields) => { + for field in fields { + add_type_dependency(context, struct_id, field); + } + } + Type::TypeVariable(variable, _) | Type::NamedGeneric(variable, _) => { + if let crate::TypeBinding::Bound(typ) = &*variable.borrow() { + add_type_dependency(context, struct_id, typ); + } + } + Type::Function(args, result, environment) => { + for arg in args { + add_type_dependency(context, struct_id, arg); + } + add_type_dependency(context, struct_id, result); + add_type_dependency(context, struct_id, environment); + } + Type::MutableReference(element) => { + add_type_dependency(context, struct_id, element); + } + Type::Forall(_, typ) => { + add_type_dependency(context, struct_id, typ); + } + } +} diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index b856b54f6ca..de16f804d19 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -1,15 +1,21 @@ +use std::borrow::Cow; use std::collections::HashMap; use arena::{Arena, Index}; use fm::FileId; use iter_extended::vecmap; use noirc_errors::{Location, Span, Spanned}; +use petgraph::algo::tarjan_scc; +use petgraph::prelude::DiGraph; +use petgraph::prelude::NodeIndex as PetGraphIndex; use crate::ast::Ident; use crate::graph::CrateId; +use crate::hir::def_collector::dc_crate::CompilationError; use crate::hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait, UnresolvedTypeAlias}; use crate::hir::def_map::{LocalModuleId, ModuleId}; +use crate::hir::resolution::errors::ResolverError; use crate::hir_def::stmt::HirLetStatement; use crate::hir_def::traits::TraitImpl; use crate::hir_def::traits::{Trait, TraitConstraint}; @@ -41,6 +47,7 @@ type StructAttributes = Vec; pub struct NodeInterner { pub(crate) nodes: Arena, pub(crate) func_meta: HashMap, + function_definition_ids: HashMap, // For a given function ID, this gives the function's modifiers which includes @@ -51,6 +58,15 @@ pub struct NodeInterner { // Contains the source module each function was defined in function_modules: HashMap, + /// This graph tracks dependencies between different global definitions. + /// This is used to ensure the absense of dependency cycles for globals + /// and types. + dependency_graph: DiGraph, + + /// To keep track of where each DependencyId is in `dependency_graph`, we need + /// this separate graph to map between the ids and indices. + dependency_graph_indices: HashMap, + // Map each `Index` to it's own location pub(crate) id_to_location: HashMap, @@ -151,6 +167,22 @@ pub struct NodeInterner { pub(crate) type_ref_locations: Vec<(Type, Location)>, } +/// A dependency in the dependency graph may be a type or a definition. +/// Types can depend on definitions too. E.g. `Foo` depends on `COUNT` in: +/// +/// ```struct +/// global COUNT = 3; +/// +/// struct Foo { +/// array: [Field; COUNT], +/// } +/// ``` +#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] +pub enum DependencyId { + Struct(StructId), + Definition(DefinitionId), +} + /// A trait implementation is either a normal implementation that is present in the source /// program via an `impl` block, or it is assumed to exist from a `where` clause or similar. #[derive(Debug, Clone)] @@ -439,6 +471,8 @@ impl Default for NodeInterner { function_modifiers: HashMap::new(), function_modules: HashMap::new(), func_id_to_trait: HashMap::new(), + dependency_graph: petgraph::graph::DiGraph::new(), + dependency_graph_indices: HashMap::new(), id_to_location: HashMap::new(), definitions: vec![], id_to_type: HashMap::new(), @@ -1418,6 +1452,85 @@ impl NodeInterner { pub(crate) fn ordering_type(&self) -> Type { self.ordering_type.clone().expect("Expected ordering_type to be set in the NodeInterner") } + + /// Register that `dependent` depends on `dependency`. + /// This is usually because `dependent` refers to `dependency` in one of its struct fields. + pub fn add_type_dependency(&mut self, dependent: StructId, dependency: StructId) { + let dependent_index = self.get_or_insert_dependency_type(dependent); + let dependency_index = self.get_or_insert_dependency_type(dependency); + + self.dependency_graph.update_edge(dependent_index, dependency_index, ()); + } + + fn get_or_insert_dependency_type(&mut self, id: StructId) -> PetGraphIndex { + let id = DependencyId::Struct(id); + if let Some(index) = self.dependency_graph_indices.get(&id) { + return *index; + } + + let index = self.dependency_graph.add_node(id); + self.dependency_graph_indices.insert(id, index); + index + } + + pub(crate) fn check_for_dependency_cycles(&self) -> Vec<(CompilationError, FileId)> { + let components = tarjan_scc(&self.dependency_graph); + let mut errors = Vec::new(); + + for strongly_connected_component in components { + if strongly_connected_component.len() > 1 { + // If a SCC contains a type or global, it must be the only element in the SCC + for (i, index) in strongly_connected_component.iter().enumerate() { + match self.dependency_graph[*index] { + DependencyId::Struct(struct_id) => { + let struct_type = self.get_struct(struct_id); + let struct_type = struct_type.borrow(); + let item = struct_type.name.to_string(); + let cycle = + self.get_cycle_error_string(&strongly_connected_component, i); + let span = struct_type.location.span; + let error = ResolverError::DependencyCycle { item, cycle, span }; + errors.push((error.into(), struct_type.location.file)); + break; + } + DependencyId::Definition(definition_id) => { + let definition = self.definition(definition_id); + if let DefinitionKind::Global(_) = &definition.kind { + let item = definition.name.to_string(); + let cycle = + self.get_cycle_error_string(&strongly_connected_component, i); + let span = definition.location.span; + let error = ResolverError::DependencyCycle { item, cycle, span }; + errors.push((error.into(), definition.location.file)); + break; + } + } + } + } + } + } + + errors + } + + /// Build up a string starting from the given item containing each item in the dependency + /// cycle. The final result will resemble `foo -> bar -> baz -> foo`, always going back to the + /// element at the given start index. + fn get_cycle_error_string(&self, scc: &[PetGraphIndex], start_index: usize) -> String { + let index_to_string = |index: PetGraphIndex| match self.dependency_graph[index] { + DependencyId::Struct(id) => Cow::Owned(self.get_struct(id).borrow().name.to_string()), + DependencyId::Definition(id) => Cow::Borrowed(self.definition_name(id)), + }; + + let mut cycle = index_to_string(scc[start_index]).to_string(); + + for i in 0..scc.len() { + cycle += " -> "; + cycle += &index_to_string(scc[(start_index + i + 1) % scc.len()]); + } + + cycle + } } impl Methods { From 26f27118ad8a0c1d12a46b488c18e5a61d13b70c Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 5 Feb 2024 12:59:23 -0600 Subject: [PATCH 2/4] Cleanup --- .../src/hir/resolution/resolver.rs | 23 +++- .../src/hir/resolution/structs.rs | 61 +-------- .../noirc_frontend/src/hir/type_check/stmt.rs | 8 +- .../noirc_frontend/src/hir_def/function.rs | 7 +- compiler/noirc_frontend/src/hir_def/stmt.rs | 14 +-- compiler/noirc_frontend/src/node_interner.rs | 116 +++++++++++++----- compiler/noirc_frontend/src/tests.rs | 10 ++ 7 files changed, 131 insertions(+), 108 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/compiler/noirc_frontend/src/hir/resolution/resolver.rs index 7b876244601..b899a5a325a 100644 --- a/compiler/noirc_frontend/src/hir/resolution/resolver.rs +++ b/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, + /// If we're currently resolving fields in a struct type, this is set to that type. + current_struct_type: Option, + /// 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) { 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) } } } diff --git a/compiler/noirc_frontend/src/hir/resolution/structs.rs b/compiler/noirc_frontend/src/hir/resolution/structs.rs index 9534cfa14de..ed7aa86e718 100644 --- a/compiler/noirc_frontend/src/hir/resolution/structs.rs +++ b/compiler/noirc_frontend/src/hir/resolution/structs.rs @@ -76,66 +76,7 @@ fn resolve_struct_fields( 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); - - // Register any other types used by this struct as a dependency in the dependency graph - // TODO: Need to move this to while we're resolving an UnresolvedType, not after. - // Otherwise we can't see global variable dependencies. - for (_, field) in &fields { - add_type_dependency(context, type_id, field); - } + .resolve_struct_fields(unresolved.struct_def, type_id); (generics, fields, errors) } - -fn add_type_dependency(context: &mut Context, struct_id: StructId, typ: &Type) { - match typ { - Type::FieldElement - | Type::Integer(_, _) - | Type::Bool - | Type::Unit - | Type::NotConstant - | Type::Constant(_) - | Type::Error => (), - - // We don't count traits as dependencies since the type represented - // by an impl trait is opaque and not needed until type checking. - Type::TraitAsType(_, _, _) => (), - - Type::Array(length, elem) | Type::FmtString(length, elem) => { - add_type_dependency(context, struct_id, length); - add_type_dependency(context, struct_id, elem); - } - - Type::String(length) => { - add_type_dependency(context, struct_id, length); - } - Type::Struct(other_struct, _) => { - let dependency_id = other_struct.borrow().id; - context.def_interner.add_type_dependency(struct_id, dependency_id); - } - Type::Tuple(fields) => { - for field in fields { - add_type_dependency(context, struct_id, field); - } - } - Type::TypeVariable(variable, _) | Type::NamedGeneric(variable, _) => { - if let crate::TypeBinding::Bound(typ) = &*variable.borrow() { - add_type_dependency(context, struct_id, typ); - } - } - Type::Function(args, result, environment) => { - for arg in args { - add_type_dependency(context, struct_id, arg); - } - add_type_dependency(context, struct_id, result); - add_type_dependency(context, struct_id, environment); - } - Type::MutableReference(element) => { - add_type_dependency(context, struct_id, element); - } - Type::Forall(_, typ) => { - add_type_dependency(context, struct_id, typ); - } - } -} diff --git a/compiler/noirc_frontend/src/hir/type_check/stmt.rs b/compiler/noirc_frontend/src/hir/type_check/stmt.rs index a38a16a6580..c2c6d83548c 100644 --- a/compiler/noirc_frontend/src/hir/type_check/stmt.rs +++ b/compiler/noirc_frontend/src/hir/type_check/stmt.rs @@ -92,7 +92,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); @@ -106,16 +106,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, }); diff --git a/compiler/noirc_frontend/src/hir_def/function.rs b/compiler/noirc_frontend/src/hir_def/function.rs index 9fff301f5f7..0ea352a3d04 100644 --- a/compiler/noirc_frontend/src/hir_def/function.rs +++ b/compiler/noirc_frontend/src/hir_def/function.rs @@ -43,12 +43,7 @@ pub struct Parameters(pub Vec); impl Parameters { pub fn span(&self) -> Span { assert!(!self.is_empty()); - let mut spans = vecmap(&self.0, |param| match ¶m.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 { diff --git a/compiler/noirc_frontend/src/hir_def/stmt.rs b/compiler/noirc_frontend/src/hir_def/stmt.rs index 4d946690151..b910be1fdda 100644 --- a/compiler/noirc_frontend/src/hir_def/stmt.rs +++ b/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); #[derive(Debug, Clone, Hash)] pub enum HirPattern { Identifier(HirIdent), - Mutable(Box, Span), - Tuple(Vec, Span), - Struct(Type, Vec<(Ident, HirPattern)>, Span), + Mutable(Box, Location), + Tuple(Vec, 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, } } } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index de16f804d19..36ad1de12fe 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -180,7 +180,9 @@ pub struct NodeInterner { #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub enum DependencyId { Struct(StructId), - Definition(DefinitionId), + Global(StmtId), + Function(FuncId), + Alias(TypeAliasId), } /// A trait implementation is either a normal implementation that is present in the source @@ -1456,14 +1458,20 @@ impl NodeInterner { /// Register that `dependent` depends on `dependency`. /// This is usually because `dependent` refers to `dependency` in one of its struct fields. pub fn add_type_dependency(&mut self, dependent: StructId, dependency: StructId) { - let dependent_index = self.get_or_insert_dependency_type(dependent); - let dependency_index = self.get_or_insert_dependency_type(dependency); + self.add_dependency(DependencyId::Struct(dependent), DependencyId::Struct(dependency)); + } + + pub fn add_type_global_dependency(&mut self, dependent: StructId, dependency: StmtId) { + self.add_dependency(DependencyId::Struct(dependent), DependencyId::Global(dependency)); + } + fn add_dependency(&mut self, dependent: DependencyId, dependency: DependencyId) { + let dependent_index = self.get_or_insert_dependency(dependent); + let dependency_index = self.get_or_insert_dependency(dependency); self.dependency_graph.update_edge(dependent_index, dependency_index, ()); } - fn get_or_insert_dependency_type(&mut self, id: StructId) -> PetGraphIndex { - let id = DependencyId::Struct(id); + fn get_or_insert_dependency(&mut self, id: DependencyId) -> PetGraphIndex { if let Some(index) = self.dependency_graph_indices.get(&id) { return *index; } @@ -1474,37 +1482,45 @@ impl NodeInterner { } pub(crate) fn check_for_dependency_cycles(&self) -> Vec<(CompilationError, FileId)> { - let components = tarjan_scc(&self.dependency_graph); + let strongly_connected_components = tarjan_scc(&self.dependency_graph); let mut errors = Vec::new(); - for strongly_connected_component in components { - if strongly_connected_component.len() > 1 { - // If a SCC contains a type or global, it must be the only element in the SCC - for (i, index) in strongly_connected_component.iter().enumerate() { + let mut push_error = |item: String, scc: &[_], i, location: Location| { + let cycle = self.get_cycle_error_string(scc, i); + let span = location.span; + let error = ResolverError::DependencyCycle { item, cycle, span }; + errors.push((error.into(), location.file)); + }; + + for scc in strongly_connected_components { + if scc.len() > 1 { + // If a SCC contains a type, type alias, or global, it must be the only element in the SCC + for (i, index) in scc.iter().enumerate() { match self.dependency_graph[*index] { DependencyId::Struct(struct_id) => { let struct_type = self.get_struct(struct_id); let struct_type = struct_type.borrow(); - let item = struct_type.name.to_string(); - let cycle = - self.get_cycle_error_string(&strongly_connected_component, i); - let span = struct_type.location.span; - let error = ResolverError::DependencyCycle { item, cycle, span }; - errors.push((error.into(), struct_type.location.file)); + push_error(struct_type.name.to_string(), &scc, i, struct_type.location); break; } - DependencyId::Definition(definition_id) => { - let definition = self.definition(definition_id); - if let DefinitionKind::Global(_) = &definition.kind { - let item = definition.name.to_string(); - let cycle = - self.get_cycle_error_string(&strongly_connected_component, i); - let span = definition.location.span; - let error = ResolverError::DependencyCycle { item, cycle, span }; - errors.push((error.into(), definition.location.file)); - break; - } + DependencyId::Global(global_id) => { + let let_stmt = match self.statement(&global_id) { + HirStatement::Let(let_stmt) => let_stmt, + other => unreachable!( + "Expected global to be a `let` statement, found {other:?}" + ), + }; + + let (name, location) = + self.pattern_name_and_location(&let_stmt.pattern); + push_error(name, &scc, i, location); } + DependencyId::Alias(alias_id) => { + let alias = self.get_type_alias(alias_id); + push_error(alias.name.to_string(), &scc, i, alias.location); + } + // Mutually recursive functions are allowed + DependencyId::Function(_) => (), } } } @@ -1519,7 +1535,19 @@ impl NodeInterner { fn get_cycle_error_string(&self, scc: &[PetGraphIndex], start_index: usize) -> String { let index_to_string = |index: PetGraphIndex| match self.dependency_graph[index] { DependencyId::Struct(id) => Cow::Owned(self.get_struct(id).borrow().name.to_string()), - DependencyId::Definition(id) => Cow::Borrowed(self.definition_name(id)), + DependencyId::Function(id) => Cow::Borrowed(self.function_name(&id)), + DependencyId::Alias(id) => { + Cow::Borrowed(self.get_type_alias(id).name.0.contents.as_ref()) + } + DependencyId::Global(id) => { + let let_stmt = match self.statement(&id) { + HirStatement::Let(let_stmt) => let_stmt, + other => { + unreachable!("Expected global to be a `let` statement, found {other:?}") + } + }; + Cow::Owned(self.pattern_name_and_location(&let_stmt.pattern).0) + } }; let mut cycle = index_to_string(scc[start_index]).to_string(); @@ -1531,6 +1559,38 @@ impl NodeInterner { cycle } + + /// Returns the name and location of this pattern. + /// If this pattern is a tuple or struct the 'name' will be a string representation of the + /// entire pattern. Similarly, the location will be the merged location of all fields. + fn pattern_name_and_location( + &self, + pattern: &crate::hir_def::stmt::HirPattern, + ) -> (String, Location) { + match pattern { + crate::hir_def::stmt::HirPattern::Identifier(ident) => { + let definition = self.definition(ident.id); + (definition.name.clone(), definition.location) + } + crate::hir_def::stmt::HirPattern::Mutable(pattern, _) => { + self.pattern_name_and_location(pattern) + } + crate::hir_def::stmt::HirPattern::Tuple(fields, location) => { + let fields = vecmap(fields, |field| self.pattern_name_and_location(field).0); + + let fields = fields.join(", "); + (format!("({fields})"), *location) + } + crate::hir_def::stmt::HirPattern::Struct(typ, fields, location) => { + let fields = vecmap(fields, |(field_name, field)| { + let field = self.pattern_name_and_location(field).0; + format!("{field_name}: {field}") + }); + let fields = fields.join(", "); + (format!("{typ} {{ {fields} }}"), *location) + } + } + } } impl Methods { diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index a4246a9fe7d..9bd017300ef 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1164,4 +1164,14 @@ fn lambda$f1(mut env$l1: (Field)) -> Field { "#; check_rewrite(src, expected_rewrite); } + + #[test] + fn deny_cyclic_structs() { + let src = r#" + struct Foo { bar: Bar } + struct Bar { foo: Foo } + fn main() {} + "#; + assert_eq!(get_program_errors(src).len(), 1); + } } From ba05b475800f0b7101df954443b9fc5041b6ae56 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Mon, 5 Feb 2024 20:53:46 +0000 Subject: [PATCH 3/4] Update compiler/noirc_frontend/src/node_interner.rs --- compiler/noirc_frontend/src/node_interner.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 36ad1de12fe..9e87f0accd6 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -59,7 +59,7 @@ pub struct NodeInterner { function_modules: HashMap, /// This graph tracks dependencies between different global definitions. - /// This is used to ensure the absense of dependency cycles for globals + /// This is used to ensure the absence of dependency cycles for globals /// and types. dependency_graph: DiGraph, From 6e41880930e8ac1e075395547ff8cf414fd190bb Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 5 Feb 2024 14:59:56 -0600 Subject: [PATCH 4/4] Fix typo --- compiler/noirc_frontend/src/node_interner.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 9e87f0accd6..53583bb0f17 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -59,8 +59,7 @@ pub struct NodeInterner { function_modules: HashMap, /// This graph tracks dependencies between different global definitions. - /// This is used to ensure the absence of dependency cycles for globals - /// and types. + /// This is used to ensure the absense of dependency cycles for globals and types. dependency_graph: DiGraph, /// To keep track of where each DependencyId is in `dependency_graph`, we need