diff --git a/Cargo.lock b/Cargo.lock index 05f0ab0176..812cdebde4 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" @@ -2984,6 +2990,7 @@ dependencies = [ "iter-extended", "noirc_errors", "noirc_printable_type", + "petgraph", "regex", "rustc-hash", "serde", @@ -3195,6 +3202,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 80d767f7f2..a3a8d46057 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 f7441750fc..69743935dc 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 7bd4de77e8..d15196b10c 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/resolver.rs b/compiler/noirc_frontend/src/hir/resolution/resolver.rs index 7b87624460..b899a5a325 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 cf3e3436c8..ed7aa86e71 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 = @@ -74,7 +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); + .resolve_struct_fields(unresolved.struct_def, type_id); (generics, fields, errors) } diff --git a/compiler/noirc_frontend/src/hir/type_check/stmt.rs b/compiler/noirc_frontend/src/hir/type_check/stmt.rs index 76bd064bf8..1bd6c16277 100644 --- a/compiler/noirc_frontend/src/hir/type_check/stmt.rs +++ b/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, }); diff --git a/compiler/noirc_frontend/src/hir_def/function.rs b/compiler/noirc_frontend/src/hir_def/function.rs index 9fff301f5f..0ea352a3d0 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 4d94669015..b910be1fdd 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 b856b54f6c..53583bb0f1 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,14 @@ 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 +166,24 @@ 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), + Global(StmtId), + Function(FuncId), + Alias(TypeAliasId), +} + /// 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 +472,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 +1453,143 @@ 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) { + 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(&mut self, id: DependencyId) -> PetGraphIndex { + 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 strongly_connected_components = tarjan_scc(&self.dependency_graph); + let mut errors = Vec::new(); + + 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(); + push_error(struct_type.name.to_string(), &scc, i, struct_type.location); + 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(_) => (), + } + } + } + } + + 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::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(); + + for i in 0..scc.len() { + cycle += " -> "; + cycle += &index_to_string(scc[(start_index + i + 1) % scc.len()]); + } + + 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 82f66fe892..5aae579ecb 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); + } }