From bb862ae28738b07d0883572233b0dc4494fd6a3b Mon Sep 17 00:00:00 2001 From: Pierre-Antoine Champin Date: Wed, 15 Dec 2021 16:48:41 +0100 Subject: [PATCH 1/2] detect recursive definitions --- crates/core/src/codegen/mod.rs | 44 +++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/crates/core/src/codegen/mod.rs b/crates/core/src/codegen/mod.rs index 1b7cb662..730258f6 100644 --- a/crates/core/src/codegen/mod.rs +++ b/crates/core/src/codegen/mod.rs @@ -9,7 +9,7 @@ use crate::target::{ use ast::{Ast, SchemaAst}; use jtd::Schema; use namespace::Namespace; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use std::fs::File; use std::io::Write; use std::path::Path; @@ -36,6 +36,7 @@ struct CodeGenerator<'a, T> { out_dir: &'a Path, strategy: Strategy, definition_names: BTreeMap, + recursive_definitions: BTreeSet, } struct FileData { @@ -50,6 +51,7 @@ impl<'a, T: Target> CodeGenerator<'a, T> { out_dir, strategy: target.strategy(), definition_names: BTreeMap::new(), + recursive_definitions: BTreeSet::new(), } } @@ -68,6 +70,18 @@ impl<'a, T: Target> CodeGenerator<'a, T> { self.definition_names.insert(name.clone(), ast_name); } + // Detect recursive definitions, as some target language need to handle + // them specifically (e.g. Rust). + // Note that a type is *not* considered to be recursive it it contains + // instances of itself only through "elements" or "values" + // (the underlying container is considered to break the recursion). + for (name, ast) in &schema_ast.definitions { + let mut visited = vec![]; + if find_recursion(name, ast, &schema_ast.definitions, &mut visited) { + self.recursive_definitions.insert(name.clone()); + } + } + // If the target is using FilePerType partitioning, then this state // won't actually be used at all. If it's using SingleFile partitioning, // then this is the only file state that will be used. @@ -479,3 +493,31 @@ impl<'a, T: Target> CodeGenerator<'a, T> { Ok(()) } } + +fn find_recursion(name: &str, ast: &Ast, definitions: &BTreeMap, visited: &mut Vec) -> bool { + match ast { + Ast::Ref { definition, .. } => { + if definition == name { + true + } else if visited.iter().any(|i| i == &name) { + false + } else { + visited.push(definition.clone()); + find_recursion(name, &definitions[definition], definitions, visited) + } + } + Ast::NullableOf { type_, .. } => { + find_recursion(name, type_, definitions, visited) + } + Ast::Struct { fields, .. } => { + fields.iter() + .any(|f| find_recursion(name, &f.type_, definitions, visited)) + } + Ast::Discriminator { variants, .. } => { + variants.iter() + .flat_map(|v| v.fields.iter()) + .any(|f| find_recursion(name, &f.type_, definitions, visited)) + } + _ => false, + } +} \ No newline at end of file From a68190a1761156be95a7fb41c4968f1a0d1e4a77 Mon Sep 17 00:00:00 2001 From: Pierre-Antoine Champin Date: Wed, 15 Dec 2021 17:38:20 +0100 Subject: [PATCH 2/2] only recursive types are now wrapped in Box --- crates/core/src/codegen/mod.rs | 19 +++++++++++++++---- crates/core/src/target/mod.rs | 1 + crates/target_csharp_system_text/src/lib.rs | 1 + crates/target_go/src/lib.rs | 1 + crates/target_java_jackson/src/lib.rs | 1 + crates/target_python/src/lib.rs | 1 + crates/target_ruby/src/lib.rs | 1 + crates/target_ruby_sig/src/lib.rs | 1 + crates/target_rust/src/lib.rs | 16 +++++++++------- crates/target_typescript/src/lib.rs | 1 + 10 files changed, 32 insertions(+), 11 deletions(-) diff --git a/crates/core/src/codegen/mod.rs b/crates/core/src/codegen/mod.rs index 730258f6..1f6d1b09 100644 --- a/crates/core/src/codegen/mod.rs +++ b/crates/core/src/codegen/mod.rs @@ -134,8 +134,17 @@ impl<'a, T: Target> CodeGenerator<'a, T> { // Ref nodes are a special sort of "expr-like" node, where we // already know what the name of the expression is; it's the name of // the definition. - Ast::Ref { definition, .. } => self.definition_names[&definition].clone(), - + // Note however that recursive definition may need some special + // treatment by the target. + Ast::Ref { metadata, definition } => { + let sub_expr = self.definition_names[&definition].clone(); + if self.recursive_definitions.iter().any(|i| i == &definition) { + self.target + .expr(&mut file_data.state, metadata, Expr::RecursiveRef(sub_expr)) + } else { + sub_expr + } + } // The remaining "expr-like" node types just build up strings and // possibly alter the per-file state (usually in order to add // "imports" to the file). @@ -501,9 +510,11 @@ fn find_recursion(name: &str, ast: &Ast, definitions: &BTreeMap, vi true } else if visited.iter().any(|i| i == &name) { false - } else { + } else if let Some(ast2) = definitions.get(definition) { visited.push(definition.clone()); - find_recursion(name, &definitions[definition], definitions, visited) + find_recursion(name, &ast2, definitions, visited) + } else { + false } } Ast::NullableOf { type_, .. } => { diff --git a/crates/core/src/target/mod.rs b/crates/core/src/target/mod.rs index cfabf9c6..14158671 100644 --- a/crates/core/src/target/mod.rs +++ b/crates/core/src/target/mod.rs @@ -87,6 +87,7 @@ pub enum Expr { ArrayOf(String), DictOf(String), NullableOf(String), + RecursiveRef(String), } #[derive(Debug)] diff --git a/crates/target_csharp_system_text/src/lib.rs b/crates/target_csharp_system_text/src/lib.rs index f1f6e371..6125e46a 100644 --- a/crates/target_csharp_system_text/src/lib.rs +++ b/crates/target_csharp_system_text/src/lib.rs @@ -125,6 +125,7 @@ impl jtd_codegen::target::Target for Target { format!("IDictionary", sub_expr) } target::Expr::NullableOf(sub_expr) => format!("{}?", sub_expr), + target::Expr::RecursiveRef(sub_expr) => sub_expr, } } diff --git a/crates/target_go/src/lib.rs b/crates/target_go/src/lib.rs index 3c934b86..b8b92cbe 100644 --- a/crates/target_go/src/lib.rs +++ b/crates/target_go/src/lib.rs @@ -114,6 +114,7 @@ impl jtd_codegen::target::Target for Target { target::Expr::ArrayOf(sub_expr) => format!("[]{}", sub_expr), target::Expr::DictOf(sub_expr) => format!("map[string]{}", sub_expr), target::Expr::NullableOf(sub_expr) => format!("*{}", sub_expr), + target::Expr::RecursiveRef(sub_expr) => sub_expr, } } diff --git a/crates/target_java_jackson/src/lib.rs b/crates/target_java_jackson/src/lib.rs index d8696c37..96123287 100644 --- a/crates/target_java_jackson/src/lib.rs +++ b/crates/target_java_jackson/src/lib.rs @@ -125,6 +125,7 @@ impl jtd_codegen::target::Target for Target { format!("Map", sub_expr) } target::Expr::NullableOf(sub_expr) => sub_expr, // everything is already nullable + target::Expr::RecursiveRef(sub_expr) => sub_expr, } } diff --git a/crates/target_python/src/lib.rs b/crates/target_python/src/lib.rs index 1e91cb59..c0eb5dec 100644 --- a/crates/target_python/src/lib.rs +++ b/crates/target_python/src/lib.rs @@ -135,6 +135,7 @@ impl jtd_codegen::target::Target for Target { format!("Optional[{}]", sub_expr) } + target::Expr::RecursiveRef(sub_expr) => sub_expr, } } diff --git a/crates/target_ruby/src/lib.rs b/crates/target_ruby/src/lib.rs index db7e3ee7..5c64dc96 100644 --- a/crates/target_ruby/src/lib.rs +++ b/crates/target_ruby/src/lib.rs @@ -117,6 +117,7 @@ impl jtd_codegen::target::Target for Target { format!("Hash[String, {}]", sub_expr) } target::Expr::NullableOf(sub_expr) => sub_expr, + target::Expr::RecursiveRef(sub_expr) => sub_expr, } } diff --git a/crates/target_ruby_sig/src/lib.rs b/crates/target_ruby_sig/src/lib.rs index 271bab75..4f96c411 100644 --- a/crates/target_ruby_sig/src/lib.rs +++ b/crates/target_ruby_sig/src/lib.rs @@ -116,6 +116,7 @@ impl jtd_codegen::target::Target for Target { format!("Hash[String, {}]", sub_expr) } target::Expr::NullableOf(sub_expr) => format!("{}?", sub_expr), + target::Expr::RecursiveRef(sub_expr) => sub_expr, } } diff --git a/crates/target_rust/src/lib.rs b/crates/target_rust/src/lib.rs index 5c7ae462..f69c8f65 100644 --- a/crates/target_rust/src/lib.rs +++ b/crates/target_rust/src/lib.rs @@ -121,14 +121,16 @@ impl jtd_codegen::target::Target for Target { format!("HashMap", sub_expr) } - // TODO: A Box here is necessary because otherwise a recursive data - // structure may fail to compile, such as in the geojson test case. + target::Expr::NullableOf(sub_expr) => format!("Option<{}>", sub_expr), + // A Box here is usually necessary for recursive data structures, + // such as in the geojson test case. // - // A more "proper" fix to this problem would be to have a cyclical - // reference detector, and insert Box only if it's necessary to - // break a cyclic dependency. It's unclear how much of a problem - // this is in the real world. - target::Expr::NullableOf(sub_expr) => format!("Option>", sub_expr), + // Note that this strategy is slighyly over-defensive; + // in a cycle of mutually recursive types, + // only one of the types needs to be boxed to break the cycle. + // In such cases, the user may want to optimize the code, + // overriding some of the boxings with metadata.rustType. + target::Expr::RecursiveRef(sub_expr) => format!("Box<{}>", sub_expr), } } diff --git a/crates/target_typescript/src/lib.rs b/crates/target_typescript/src/lib.rs index a6465579..185dce8e 100644 --- a/crates/target_typescript/src/lib.rs +++ b/crates/target_typescript/src/lib.rs @@ -92,6 +92,7 @@ impl jtd_codegen::target::Target for Target { target::Expr::ArrayOf(sub_expr) => format!("{}[]", sub_expr), target::Expr::DictOf(sub_expr) => format!("{{ [key: string]: {} }}", sub_expr), target::Expr::NullableOf(sub_expr) => format!("({} | null)", sub_expr), + target::Expr::RecursiveRef(sub_expr) => sub_expr, } }