Better handling of optional properties in Rust (#2)

Author: ryanatallah

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<String, String>,
+    recursive_definitions: BTreeSet<String>,
 }
 
 struct FileData<T> {
@@ -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.
@@ -120,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).
@@ -479,3 +502,33 @@ impl<'a, T: Target> CodeGenerator<'a, T> {
         Ok(())
     }
 }
+
+fn find_recursion(name: &str, ast: &Ast, definitions: &BTreeMap<String, Ast>, visited: &mut Vec<String>) -> bool {
+    match ast {
+        Ast::Ref { definition, .. } => {
+            if definition == name {
+                true
+            } else if visited.iter().any(|i| i == &name) {
+                false
+            } else if let Some(ast2) = definitions.get(definition) {
+                visited.push(definition.clone());
+                find_recursion(name, &ast2, definitions, visited)
+            } else {
+                false
+            }
+        }
+        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,
+    }
+}

crates/core/src/target/mod.rs

@@ -87,6 +87,7 @@ pub enum Expr {
     ArrayOf(String),
     DictOf(String),
     NullableOf(String),
+    RecursiveRef(String),
 }
 
 #[derive(Debug)]

crates/target_csharp_system_text/src/lib.rs

@@ -125,6 +125,7 @@ impl jtd_codegen::target::Target for Target {
                 format!("IDictionary<string, {}>", sub_expr)
             }
             target::Expr::NullableOf(sub_expr) => format!("{}?", sub_expr),
+            target::Expr::RecursiveRef(sub_expr) => sub_expr,
         }
     }
 

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

crates/target_java_jackson/src/lib.rs

@@ -125,6 +125,7 @@ impl jtd_codegen::target::Target for Target {
                 format!("Map<String, {}>", sub_expr)
             }
             target::Expr::NullableOf(sub_expr) => sub_expr, // everything is already nullable
+            target::Expr::RecursiveRef(sub_expr) => sub_expr,
         }
     }
 

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

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

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

crates/target_rust/src/lib.rs

@@ -121,14 +121,16 @@ impl jtd_codegen::target::Target for Target {
                 format!("HashMap<String, {}>", 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<T> 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<Box<{}>>", 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),
         }
     }
 

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