Skip to content

Better handling of optional properties in Rust #45

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 56 additions & 3 deletions crates/core/src/codegen/mod.rs
Original file line number Diff line number Diff line change
@@ -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,
}
}
1 change: 1 addition & 0 deletions crates/core/src/target/mod.rs
Original file line number Diff line number Diff line change
@@ -87,6 +87,7 @@ pub enum Expr {
ArrayOf(String),
DictOf(String),
NullableOf(String),
RecursiveRef(String),
}

#[derive(Debug)]
1 change: 1 addition & 0 deletions crates/target_csharp_system_text/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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,
}
}

1 change: 1 addition & 0 deletions crates/target_go/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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,
}
}

1 change: 1 addition & 0 deletions crates/target_java_jackson/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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,
}
}

1 change: 1 addition & 0 deletions crates/target_python/src/lib.rs
Original file line number Diff line number Diff line change
@@ -135,6 +135,7 @@ impl jtd_codegen::target::Target for Target {

format!("Optional[{}]", sub_expr)
}
target::Expr::RecursiveRef(sub_expr) => sub_expr,
}
}

1 change: 1 addition & 0 deletions crates/target_ruby/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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,
}
}

1 change: 1 addition & 0 deletions crates/target_ruby_sig/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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,
}
}

16 changes: 9 additions & 7 deletions crates/target_rust/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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),
}
}

1 change: 1 addition & 0 deletions crates/target_typescript/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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,
}
}