diff --git a/typify-impl/src/convert.rs b/typify-impl/src/convert.rs index 9bd016a3..9d06ad34 100644 --- a/typify-impl/src/convert.rs +++ b/typify-impl/src/convert.rs @@ -712,6 +712,14 @@ impl TypeSpace { return Ok((ty, metadata)); } + // Rust can emit "anyOf":[{"$ref":"#/definitions/C"},{"type":"null"} for + // Option, so match against that here to short circuit the check for + // mutual exclusivity below. + if let Some(option_type_entry) = self.maybe_option(type_name.clone(), metadata, subschemas) + { + return Ok((option_type_entry, metadata)); + } + // Check if this could be more precisely handled as a "one-of". This // occurs if each subschema is mutually exclusive i.e. so that exactly // one of them can match. @@ -795,7 +803,7 @@ impl TypeSpace { return Ok((ty, metadata)); } let ty = self - .maybe_option_as_enum(type_name.clone(), metadata, subschemas) + .maybe_option(type_name.clone(), metadata, subschemas) .or_else(|| self.maybe_externally_tagged_enum(type_name.clone(), metadata, subschemas)) .or_else(|| self.maybe_adjacently_tagged_enum(type_name.clone(), metadata, subschemas)) .or_else(|| self.maybe_internally_tagged_enum(type_name.clone(), metadata, subschemas)) @@ -930,9 +938,10 @@ impl TypeSpace { mod tests { use std::num::{NonZeroU16, NonZeroU32, NonZeroU64, NonZeroU8}; + use schema::Schema; use schemars::{schema_for, JsonSchema}; - use crate::{validate_builtin, Name, TypeSpace}; + use crate::{test_util::validate_output, validate_builtin, Name, TypeSpace}; use paste::paste; fn int_helper() { @@ -1012,6 +1021,88 @@ mod tests { assert_eq!(type_space.iter_types().count(), 4); } + #[test] + fn test_trivial_cycle() { + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct A { + a: Box, + } + + validate_output::(); + } + + #[test] + fn test_optional_trivial_cycle() { + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct A { + a: Option>, + } + + validate_output::(); + } + + #[test] + fn test_enum_trivial_cycles() { + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + enum A { + Variant0(u64), + Varant1 { + a: u64, + b: Vec, + rop: Option>, + }, + Variant2 { + a: Box, + }, + Variant3(u64, Box), + Variant4(Option>, String), + } + + validate_output::(); + } + + #[test] + fn test_newtype_trivial_cycle() { + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct A(Box); + + validate_output::(); + } + + #[test] + fn test_basic_option_flat() { + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct C {} + + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct A { + a: Option, + } + + validate_output::(); + } + + #[test] + fn test_unit_option() { + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct Foo; + + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct Bar { + a: Option, + } + + validate_output::(); + } + // TODO we can turn this on once we generate proper sets. #[ignore] #[test] diff --git a/typify-impl/src/enums.rs b/typify-impl/src/enums.rs index c827cc6c..bba4b93e 100644 --- a/typify-impl/src/enums.rs +++ b/typify-impl/src/enums.rs @@ -17,7 +17,7 @@ use crate::{ }; impl TypeSpace { - pub(crate) fn maybe_option_as_enum( + pub(crate) fn maybe_option( &mut self, type_name: Name, metadata: &Option>, @@ -1245,7 +1245,7 @@ mod tests { } #[test] - fn test_maybe_option_as_enum() { + fn test_maybe_option() { let subschemas = vec![ SchemaObject { instance_type: Some(SingleOrVec::Single(Box::new(InstanceType::String))), @@ -1261,7 +1261,7 @@ mod tests { let mut type_space = TypeSpace::default(); let type_entry = type_space - .maybe_option_as_enum(Name::Unknown, &None, &subschemas) + .maybe_option(Name::Unknown, &None, &subschemas) .unwrap(); assert_eq!(type_entry.details, TypeEntryDetails::Option(TypeId(1))) diff --git a/typify-impl/src/lib.rs b/typify-impl/src/lib.rs index fb0a5b40..baac6db5 100644 --- a/typify-impl/src/lib.rs +++ b/typify-impl/src/lib.rs @@ -8,7 +8,7 @@ use quote::quote; use rustfmt_wrapper::rustfmt; use schemars::schema::{Metadata, Schema}; use thiserror::Error; -use type_entry::{TypeEntry, TypeEntryDetails, TypeEntryNewtype}; +use type_entry::{TypeEntry, TypeEntryDetails, TypeEntryNewtype, VariantDetails}; #[cfg(test)] mod test_util; @@ -48,6 +48,7 @@ pub enum TypeDetails<'a> { Array(TypeId), Map(TypeId, TypeId), Set(TypeId), + Box(TypeId), Tuple(Box + 'a>), Unit, Builtin(&'a str), @@ -163,7 +164,8 @@ impl TypeSpace { // Assign IDs to reference types before actually converting them. We'll // need these in the case of forward (or circular) references. let base_id = self.next_id; - self.next_id += definitions.len() as u64; + let def_len = definitions.len() as u64; + self.next_id += def_len; for (index, (ref_name, _)) in definitions.iter().enumerate() { self.ref_to_id @@ -178,7 +180,11 @@ impl TypeSpace { None => &ref_name, }; - info!("converting type: {}", type_name); + info!( + "converting type: {} with schema {}", + type_name, + serde_json::to_string(&schema).unwrap() + ); let (type_entry, metadata) = self.convert_schema(Name::Required(type_name.to_string()), &schema)?; @@ -219,11 +225,124 @@ impl TypeSpace { // of derives than we might otherwise. This notably prevents us from // using a HashSet or BTreeSet type where we might like to. - // TODO Look for containment cycles that we need to break with a Box + // Once all ref types are in, look for containment cycles that we need + // to break with a Box. + for index in 0..def_len { + let type_id = TypeId(base_id + index); + + let mut box_id = None; + + let mut type_entry = self.id_to_entry.get_mut(&type_id).unwrap().clone(); + self.break_trivial_cyclic_refs(&type_id, &mut type_entry, &mut box_id); + let _ = self.id_to_entry.insert(type_id, type_entry); + } Ok(()) } + /// If a type refers to itself, this creates a cycle that will eventually be + /// emit as a Rust struct that cannot be constructed. Break those cycles + /// here. + /// + /// While we aren't yet handling the general case of type containment + /// cycles, it's not that bad to look at trivial cycles such as: + /// + /// 1) A type refering to itself: A -> A + /// 2) A type optionally referring to itself: A -> Option + /// 3) An enum variant referring to itself, either optionally or directly. + /// + /// TODO currently only trivial cycles are broken. A more generic solution + /// may be required, but it may also a point to ask oneself why such a + /// complicated type is required :) A generic solution is difficult because + /// certain cycles introduce a question of *where* to Box to break the + /// cycle, and there's no one answer to this. + /// + fn check_for_cyclic_ref( + &mut self, + parent_type_id: &TypeId, + child_type_id: &mut TypeId, + box_id: &mut Option, + ) { + if *child_type_id == *parent_type_id { + *child_type_id = box_id + .get_or_insert_with(|| self.id_to_box(parent_type_id)) + .clone(); + } else { + let mut child_type_entry = self.id_to_entry.get_mut(&child_type_id).unwrap().clone(); + + match &mut child_type_entry.details { + // Look for the case where an option refers to the parent type + TypeEntryDetails::Option(option_type_id) => { + if *option_type_id == *parent_type_id { + *option_type_id = box_id + .get_or_insert_with(|| self.id_to_box(parent_type_id)) + .clone(); + } + } + + _ => {} + } + + let _ = self + .id_to_entry + .insert(child_type_id.clone(), child_type_entry); + } + } + + fn break_trivial_cyclic_refs( + &mut self, + parent_type_id: &TypeId, + type_entry: &mut TypeEntry, + box_id: &mut Option, + ) { + match &mut type_entry.details { + // Look for the case where a struct property refers to the parent + // type + TypeEntryDetails::Struct(s) => { + for prop in &mut s.properties { + self.check_for_cyclic_ref(parent_type_id, &mut prop.type_id, box_id); + } + } + + // Look for the cases where an enum variant refers to the parent + // type + TypeEntryDetails::Enum(type_entry_enum) => { + for variant in &mut type_entry_enum.variants { + match &mut variant.details { + // Simple variants will not refer to anything + VariantDetails::Simple => {} + // Look for a tuple entry that refers to the parent type + VariantDetails::Tuple(vec_type_id) => { + for tuple_type_id in vec_type_id { + self.check_for_cyclic_ref(parent_type_id, tuple_type_id, box_id); + } + } + // Look for a struct property that refers to the parent type + VariantDetails::Struct(vec_struct_property) => { + for struct_property in vec_struct_property { + let vec_type_id = &mut struct_property.type_id; + self.check_for_cyclic_ref(parent_type_id, vec_type_id, box_id); + } + } + } + } + } + + // Look for cases where a newtype refers to a parent type + TypeEntryDetails::Newtype(new_type_entry) => { + self.check_for_cyclic_ref(parent_type_id, &mut new_type_entry.type_id, box_id); + } + + // Containers that can be size 0 are *not* cyclic references for that type + TypeEntryDetails::Array(_) => {} + TypeEntryDetails::Set(_) => {} + TypeEntryDetails::Map(_, _) => {} + + // Everything else can be ignored + _ => {} + } + } + /// Add a new type and return a type identifier that may be used in /// function signatures or embedded within other types. pub fn add_type(&mut self, schema: &Schema) -> Result { @@ -358,6 +477,11 @@ impl TypeSpace { fn type_to_option(&mut self, ty: TypeEntry) -> TypeEntry { TypeEntryDetails::Option(self.assign_type(ty)).into() } + + /// Create a Box from a pre-assigned TypeId and assign it an ID. + fn id_to_box(&mut self, id: &TypeId) -> TypeId { + self.assign_type(TypeEntryDetails::Box(id.clone()).into()) + } } impl ToString for TypeSpace { @@ -439,6 +563,7 @@ impl<'a> Type<'a> { TypeDetails::Map(key_id.clone(), value_id.clone()) } TypeEntryDetails::Set(type_id) => TypeDetails::Set(type_id.clone()), + TypeEntryDetails::Box(type_id) => TypeDetails::Box(type_id.clone()), TypeEntryDetails::Tuple(types) => TypeDetails::Tuple(Box::new(types.iter().cloned())), // Builtin types diff --git a/typify-impl/src/test_util.rs b/typify-impl/src/test_util.rs index 31f024ac..206bc8a5 100644 --- a/typify-impl/src/test_util.rs +++ b/typify-impl/src/test_util.rs @@ -34,12 +34,31 @@ fn validate_output_impl(ignore_variant_names: bool) { let name = type_name::().rsplit_once("::").unwrap().1.to_string(); let mut type_space = TypeSpace::default(); + + // Convert all references type_space .add_ref_types(schema.definitions.clone()) .unwrap(); - let (ty, _) = type_space - .convert_schema_object(Name::Required(name), &schema.schema) - .unwrap(); + + // In some situations, `schema_for!(T)` may actually give us two copies of + // the type: one in the definitions and one in the schema. This will occur + // in particular for cyclic types i.e. those for which the type itself is a + // reference. + // + // If we have converted the type already, use that, otherwise convert schema + // object + let ty = if let Some(already_type_id) = type_space.ref_to_id.get(&name) { + type_space + .id_to_entry + .get(&already_type_id) + .unwrap() + .clone() + } else { + let (ty, _) = type_space + .convert_schema_object(Name::Required(name), &schema.schema) + .unwrap(); + ty + }; let output = ty.output(&type_space); diff --git a/typify-impl/src/type_entry.rs b/typify-impl/src/type_entry.rs index 182d3d15..23adf3dc 100644 --- a/typify-impl/src/type_entry.rs +++ b/typify-impl/src/type_entry.rs @@ -54,6 +54,7 @@ pub(crate) enum TypeEntryDetails { Newtype(TypeEntryNewtype), Option(TypeId), + Box(TypeId), Array(TypeId), Map(TypeId, TypeId), Set(TypeId), @@ -388,6 +389,7 @@ impl TypeEntry { | TypeEntryDetails::Map(_, _) | TypeEntryDetails::Set(_) | TypeEntryDetails::Unit + | TypeEntryDetails::Box(_) | TypeEntryDetails::Tuple(_) => quote! {}, // We should never get here as reference types should only be used @@ -434,6 +436,17 @@ impl TypeEntry { } } + TypeEntryDetails::Box(id) => { + let inner_ty = type_space + .id_to_entry + .get(id) + .expect("unresolved type id for box"); + + let item = inner_ty.type_ident(type_space, external); + + quote! { Box<#item> } + } + TypeEntryDetails::Array(id) => { let inner_ty = type_space .id_to_entry @@ -525,6 +538,7 @@ impl TypeEntry { | TypeEntryDetails::Array(_) | TypeEntryDetails::Map(_, _) | TypeEntryDetails::Set(_) + | TypeEntryDetails::Box(_) | TypeEntryDetails::BuiltIn(_) => { let ident = self.type_ident(type_space, true); quote! { @@ -580,6 +594,7 @@ impl TypeEntry { TypeEntryDetails::Array(type_id) => format!("array {}", type_id.0), TypeEntryDetails::Map(key_id, value_id) => format!("map {} {}", key_id.0, value_id.0), TypeEntryDetails::Set(type_id) => format!("set {}", type_id.0), + TypeEntryDetails::Box(type_id) => format!("box {}", type_id.0), TypeEntryDetails::Tuple(type_ids) => { format!( "tuple ({})",