From e08bb307aa88409bf5b9ad002ba4c22615d94cf8 Mon Sep 17 00:00:00 2001 From: Toby Hutton Date: Tue, 31 May 2022 00:52:52 +0000 Subject: [PATCH] Prevent recursive enums and structs from getting converted from parse nodes to AST nodes. (#1768) --- sway-core/src/convert_parse_tree.rs | 45 +++++--- sway-core/src/error.rs | 10 ++ .../semantic_analysis/node_dependencies.rs | 107 ++++++++++++++---- test/src/e2e_vm_tests/mod.rs | 3 + .../should_fail/recursive_enum/Forc.lock | 3 + .../should_fail/recursive_enum/Forc.toml | 6 + .../should_fail/recursive_enum/src/main.sw | 11 ++ .../should_fail/recursive_struct/Forc.lock | 3 + .../should_fail/recursive_struct/Forc.toml | 6 + .../should_fail/recursive_struct/src/main.sw | 11 ++ .../recursive_type_chain/Forc.lock | 3 + .../recursive_type_chain/Forc.toml | 6 + .../recursive_type_chain/src/main.sw | 45 ++++++++ 13 files changed, 218 insertions(+), 41 deletions(-) create mode 100644 test/src/e2e_vm_tests/test_programs/should_fail/recursive_enum/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_fail/recursive_enum/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_fail/recursive_enum/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_fail/recursive_struct/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_fail/recursive_struct/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_fail/recursive_struct/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_fail/recursive_type_chain/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_fail/recursive_type_chain/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_fail/recursive_type_chain/src/main.sw diff --git a/sway-core/src/convert_parse_tree.rs b/sway-core/src/convert_parse_tree.rs index 9a4c58ba4a1..4361998930f 100644 --- a/sway-core/src/convert_parse_tree.rs +++ b/sway-core/src/convert_parse_tree.rs @@ -178,6 +178,8 @@ pub enum ConvertParseTreeError { ConstrainedNonExistentType { ty_name: Ident, span: Span }, #[error("__generate_uid does not take arguments")] GenerateUidTooManyArgs { span: Span }, + #[error("recursive types are not supported")] + RecursiveType { span: Span }, } impl ConvertParseTreeError { @@ -228,6 +230,7 @@ impl ConvertParseTreeError { ConvertParseTreeError::InvalidAttributeArgument { span, .. } => span.clone(), ConvertParseTreeError::ConstrainedNonExistentType { span, .. } => span.clone(), ConvertParseTreeError::GenerateUidTooManyArgs { span, .. } => span.clone(), + ConvertParseTreeError::RecursiveType { span } => span.clone(), } } } @@ -479,16 +482,20 @@ fn item_struct_to_struct_declaration( item_struct: ItemStruct, ) -> Result { let span = item_struct.span(); + let fields = item_struct + .fields + .into_inner() + .into_iter() + .map(|type_field| type_field_to_struct_field(ec, type_field)) + .collect::, _>>()?; + if fields.iter().any( + |field| matches!(&field.r#type, TypeInfo::Custom { name, ..} if name == &item_struct.name), + ) { + return Err(ec.error(ConvertParseTreeError::RecursiveType { span })); + } let struct_declaration = StructDeclaration { name: item_struct.name, - fields: { - item_struct - .fields - .into_inner() - .into_iter() - .map(|type_field| type_field_to_struct_field(ec, type_field)) - .collect::>()? - }, + fields, type_parameters: generic_params_opt_to_type_parameters( ec, item_struct.generics, @@ -505,6 +512,18 @@ fn item_enum_to_enum_declaration( item_enum: ItemEnum, ) -> Result { let span = item_enum.span(); + let variants = item_enum + .fields + .into_inner() + .into_iter() + .enumerate() + .map(|(tag, type_field)| type_field_to_enum_variant(ec, type_field, tag)) + .collect::, _>>()?; + if variants.iter().any(|variant| { + matches!(&variant.r#type, TypeInfo::Custom { name, ..} if name == &item_enum.name) + }) { + return Err(ec.error(ConvertParseTreeError::RecursiveType { span })); + } let enum_declaration = EnumDeclaration { name: item_enum.name, type_parameters: generic_params_opt_to_type_parameters( @@ -512,15 +531,7 @@ fn item_enum_to_enum_declaration( item_enum.generics, item_enum.where_clause_opt, )?, - variants: { - item_enum - .fields - .into_inner() - .into_iter() - .enumerate() - .map(|(tag, type_field)| type_field_to_enum_variant(ec, type_field, tag)) - .collect::>()? - }, + variants, span, visibility: pub_token_opt_to_visibility(item_enum.visibility), }; diff --git a/sway-core/src/error.rs b/sway-core/src/error.rs index d1b9da4a44d..6728083fb0a 100644 --- a/sway-core/src/error.rs +++ b/sway-core/src/error.rs @@ -831,6 +831,14 @@ pub enum CompileError { call_chain: String, // Pretty list of symbols, e.g., "a, b and c". span: Span, }, + #[error("Type {name} is recursive, which is unsupported at this time.")] + RecursiveType { name: Ident, span: Span }, + #[error("Type {name} is recursive via {type_chain}, which is unsupported at this time.")] + RecursiveTypeChain { + name: Ident, + type_chain: String, // Pretty list of symbols, e.g., "a, b and c". + span: Span, + }, #[error( "The size of this type is not known. Try putting it on the heap or changing the type." )] @@ -1113,6 +1121,8 @@ impl CompileError { ArgumentParameterTypeMismatch { span, .. } => span.clone(), RecursiveCall { span, .. } => span.clone(), RecursiveCallChain { span, .. } => span.clone(), + RecursiveType { span, .. } => span.clone(), + RecursiveTypeChain { span, .. } => span.clone(), TypeWithUnknownSize { span, .. } => span.clone(), InfiniteDependencies { span, .. } => span.clone(), GMFromExternalContract { span, .. } => span.clone(), diff --git a/sway-core/src/semantic_analysis/node_dependencies.rs b/sway-core/src/semantic_analysis/node_dependencies.rs index 9dfe0537054..24ef30d2181 100644 --- a/sway-core/src/semantic_analysis/node_dependencies.rs +++ b/sway-core/src/semantic_analysis/node_dependencies.rs @@ -20,7 +20,7 @@ pub(crate) fn order_ast_nodes_by_dependency(nodes: Vec) -> CompileResul DependencyMap::from_iter(nodes.iter().filter_map(Dependencies::gather_from_decl_node)); // Check here for recursive calls now that we have a nice map of the dependencies to help us. - let mut errors = find_recursive_calls(&decl_dependencies); + let mut errors = find_recursive_decls(&decl_dependencies); if !errors.is_empty() { // Because we're pulling these errors out of a HashMap they'll probably be in a funny // order. Here we'll sort them by span start. @@ -45,22 +45,27 @@ pub(crate) fn order_ast_nodes_by_dependency(nodes: Vec) -> CompileResul // ------------------------------------------------------------------------------------------------- // Recursion detection. -fn find_recursive_calls(decl_dependencies: &DependencyMap) -> Vec { +fn find_recursive_decls(decl_dependencies: &DependencyMap) -> Vec { decl_dependencies .iter() - .filter_map(|(dep_sym, _)| find_recursive_call(decl_dependencies, dep_sym)) + .filter_map(|(dep_sym, _)| find_recursive_decl(decl_dependencies, dep_sym)) .collect() } -fn find_recursive_call( +fn find_recursive_decl( decl_dependencies: &DependencyMap, - fn_sym: &DependentSymbol, + dep_sym: &DependentSymbol, ) -> Option { - if let DependentSymbol::Fn(_, Some(fn_span)) = fn_sym { - let mut chain = Vec::new(); - find_recursive_call_chain(decl_dependencies, fn_sym, fn_span, &mut chain) - } else { - None + match dep_sym { + DependentSymbol::Fn(_, Some(fn_span)) => { + let mut chain = Vec::new(); + find_recursive_call_chain(decl_dependencies, dep_sym, fn_span, &mut chain) + } + DependentSymbol::Symbol(_) => { + let mut chain = Vec::new(); + find_recursive_type_chain(decl_dependencies, dep_sym, &mut chain) + } + _otherwise => None, } } @@ -98,6 +103,34 @@ fn find_recursive_call_chain( } } +fn find_recursive_type_chain( + decl_dependencies: &DependencyMap, + dep_sym: &DependentSymbol, + chain: &mut Vec, +) -> Option { + if let DependentSymbol::Symbol(sym_ident) = dep_sym { + if chain.iter().any(|seen_sym| seen_sym == sym_ident) { + // See above about it only being an error if we're referring back to the start. + return if &chain[0] != sym_ident { + None + } else { + Some(build_recursive_type_error(sym_ident.clone(), &chain[1..])) + }; + } + decl_dependencies.get(dep_sym).and_then(|deps_set| { + chain.push(sym_ident.clone()); + let result = deps_set + .deps + .iter() + .find_map(|dep_sym| find_recursive_type_chain(decl_dependencies, dep_sym, chain)); + chain.pop(); + result + }) + } else { + None + } +} + fn build_recursion_error(fn_sym: Ident, span: Span, chain: &[Ident]) -> CompileError { match chain.len() { // An empty chain indicates immediate recursion. @@ -128,6 +161,34 @@ fn build_recursion_error(fn_sym: Ident, span: Span, chain: &[Ident]) -> CompileE } } +fn build_recursive_type_error(name: Ident, chain: &[Ident]) -> CompileError { + let span = name.span().clone(); + match chain.len() { + // An empty chain indicates immediate recursion. + 0 => CompileError::RecursiveType { name, span }, + // Chain entries indicate mutual recursion. + 1 => CompileError::RecursiveTypeChain { + name, + type_chain: chain[0].as_str().to_string(), + span, + }, + n => { + let mut msg = chain[0].as_str().to_string(); + for ident in &chain[1..(n - 1)] { + msg.push_str(", "); + msg.push_str(ident.as_str()); + } + msg.push_str(" and "); + msg.push_str(chain[n - 1].as_str()); + CompileError::RecursiveTypeChain { + name, + type_chain: msg, + span, + } + } + } +} + // ------------------------------------------------------------------------------------------------- // Dependency gathering. @@ -375,9 +436,8 @@ impl Dependencies { fields, .. } => { - self.deps.insert(DependentSymbol::Symbol( - struct_name.suffix.as_str().to_string(), - )); + self.deps + .insert(DependentSymbol::Symbol(struct_name.suffix.clone())); self.gather_from_iter(fields.iter(), |deps, field| { deps.gather_from_expr(&field.value) }) @@ -472,14 +532,13 @@ impl Dependencies { self.deps.insert(if is_fn_app { DependentSymbol::Fn(call_path.suffix.clone(), None) } else { - DependentSymbol::Symbol(call_path.suffix.as_str().to_string()) + DependentSymbol::Symbol(call_path.suffix.clone()) }); } else if use_prefix && call_path.prefixes.len() == 1 { // Here we can use the prefix (e.g., for 'Enum::Variant' -> 'Enum') as long is it's // only a single element. - self.deps.insert(DependentSymbol::Symbol( - call_path.prefixes[0].as_str().to_string(), - )); + self.deps + .insert(DependentSymbol::Symbol(call_path.prefixes[0].clone())); } self } @@ -509,7 +568,7 @@ impl Dependencies { name, type_arguments, } => { - self.deps.insert(DependentSymbol::Symbol(name.to_string())); + self.deps.insert(DependentSymbol::Symbol(name.clone())); self.gather_from_type_arguments(type_arguments) } TypeInfo::Tuple(elems) => self.gather_from_iter(elems.iter(), |deps, elem| { @@ -544,7 +603,7 @@ impl Dependencies { #[derive(Debug, Eq)] enum DependentSymbol { - Symbol(String), + Symbol(Ident), Fn(Ident, Option), Impl(Ident, String, String), // Trait or self, type implementing for, and method names concatenated. } @@ -599,11 +658,11 @@ fn decl_name(decl: &Declaration) -> Option { decl.name.clone(), Some(decl.span.clone()), )), - Declaration::ConstantDeclaration(decl) => dep_sym(decl.name.as_str().to_string()), - Declaration::StructDeclaration(decl) => dep_sym(decl.name.as_str().to_string()), - Declaration::EnumDeclaration(decl) => dep_sym(decl.name.as_str().to_string()), - Declaration::TraitDeclaration(decl) => dep_sym(decl.name.as_str().to_string()), - Declaration::AbiDeclaration(decl) => dep_sym(decl.name.as_str().to_string()), + Declaration::ConstantDeclaration(decl) => dep_sym(decl.name.clone()), + Declaration::StructDeclaration(decl) => dep_sym(decl.name.clone()), + Declaration::EnumDeclaration(decl) => dep_sym(decl.name.clone()), + Declaration::TraitDeclaration(decl) => dep_sym(decl.name.clone()), + Declaration::AbiDeclaration(decl) => dep_sym(decl.name.clone()), // These have the added complexity of converting CallPath and/or TypeInfo into a name. Declaration::ImplSelf(decl) => { diff --git a/test/src/e2e_vm_tests/mod.rs b/test/src/e2e_vm_tests/mod.rs index 091270d9afa..c7cf4171f39 100644 --- a/test/src/e2e_vm_tests/mod.rs +++ b/test/src/e2e_vm_tests/mod.rs @@ -543,6 +543,9 @@ pub fn run(filter_regex: Option) { "should_fail/match_expressions_empty_arms", "should_fail/type_mismatch_error_message", "should_fail/non_literal_const_decl", + "should_fail/recursive_enum", + "should_fail/recursive_struct", + "should_fail/recursive_type_chain", ]; number_of_tests_run += negative_project_names.iter().fold(0, |acc, name| { if filter(name) { diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/recursive_enum/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_enum/Forc.lock new file mode 100644 index 00000000000..6b7e4b735fe --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_enum/Forc.lock @@ -0,0 +1,3 @@ +[[package]] +name = 'recursive_enum' +dependencies = [] diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/recursive_enum/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_enum/Forc.toml new file mode 100644 index 00000000000..5342e612104 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_enum/Forc.toml @@ -0,0 +1,6 @@ +[project] +name = "recursive_enum" +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" +implicit-std = false diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/recursive_enum/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_enum/src/main.sw new file mode 100644 index 00000000000..21da4d7f364 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_enum/src/main.sw @@ -0,0 +1,11 @@ +script; + +enum E { + Eins: bool, + Zwei: u64, + Drei: E, +} + +fn main() { + +} diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/recursive_struct/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_struct/Forc.lock new file mode 100644 index 00000000000..682eb5a613d --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_struct/Forc.lock @@ -0,0 +1,3 @@ +[[package]] +name = 'recursive_struct' +dependencies = [] diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/recursive_struct/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_struct/Forc.toml new file mode 100644 index 00000000000..4f9f4ab89e6 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_struct/Forc.toml @@ -0,0 +1,6 @@ +[project] +name = "recursive_struct" +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" +implicit-std = false diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/recursive_struct/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_struct/src/main.sw new file mode 100644 index 00000000000..64b810ce1c1 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_struct/src/main.sw @@ -0,0 +1,11 @@ +script; + +struct S { + Eins: bool, + Zwei: u64, + Drei: S, +} + +fn main() { + +} diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/recursive_type_chain/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_type_chain/Forc.lock new file mode 100644 index 00000000000..3abe0a81d81 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_type_chain/Forc.lock @@ -0,0 +1,3 @@ +[[package]] +name = 'recursive_type_chain' +dependencies = [] diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/recursive_type_chain/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_type_chain/Forc.toml new file mode 100644 index 00000000000..4f42029940f --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_type_chain/Forc.toml @@ -0,0 +1,6 @@ +[project] +name = "recursive_type_chain" +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" +implicit-std = false diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/recursive_type_chain/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_type_chain/src/main.sw new file mode 100644 index 00000000000..5466d0f01e6 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_fail/recursive_type_chain/src/main.sw @@ -0,0 +1,45 @@ +script; + +enum E { + Eins: F, +} + +enum F { + Zwei: G, +} + +enum G { + Drei: E, +} + +enum H { + Vier: I, +} + +enum I { + Funf: H, +} + +struct S { + one: T, +} + +struct T { + two: S, +} + +struct X { + three: Y, +} + +enum Y { + four: Z, +} + +struct Z { + five: X, +} + +fn main() { + +}