From 422eb69efdbb030e667f0441686c8205960f8aa6 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Fri, 11 Mar 2022 17:40:01 -0500 Subject: [PATCH 01/13] Box struct properties with trivial cyclic refs If a struct A has a property that refers to type A, then insert a Box for this. Note that this commit only checks for these trivial cyclic refs. Saw "anyOf" for Option, so match against that case --- typify-impl/src/convert.rs | 84 ++++++++++++++++++++++++++++++++++- typify-impl/src/enums.rs | 6 +-- typify-impl/src/lib.rs | 46 ++++++++++++++++++- typify-impl/src/type_entry.rs | 16 +++++++ 4 files changed, 145 insertions(+), 7 deletions(-) diff --git a/typify-impl/src/convert.rs b/typify-impl/src/convert.rs index 9bd016a3..f0cabf17 100644 --- a/typify-impl/src/convert.rs +++ b/typify-impl/src/convert.rs @@ -712,6 +712,12 @@ impl TypeSpace { return Ok((ty, metadata)); } + // Rust can emit "anyOf":[{"$ref":"#/definitions/C"},{"type":"null"} for + // Option, so match against that here. + 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 +801,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 +936,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 +1019,79 @@ mod tests { assert_eq!(type_space.iter_types().count(), 4); } + #[test] + fn test_trivial_cycle() { + // Currently schemars will actually include the type A twice if this is + // made a RootSchema; it should really have a reference that it uses as + // the RootSchema. To workaround this, we refer to A from B below. + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct A { + a: Box, + } + + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct B { + a: A, + } + + validate_output::(); + } + + #[test] + fn test_optional_trivial_cycle() { + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct A { + a: Option>, + } + + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct B { + a: A, + } + + validate_output::(); + } + + #[test] + fn test_basic_option() { + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct C {} + + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct A { + a: Option, + } + + validate_output::(); + } + + #[test] + fn test_basic_option_one_deep() { + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct C {} + + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct A { + a: Option, + } + + #[derive(JsonSchema, Schema)] + #[allow(dead_code)] + struct B { + a: A, + } + + 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..f28baa5a 100644 --- a/typify-impl/src/lib.rs +++ b/typify-impl/src/lib.rs @@ -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,8 @@ 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)?; @@ -221,6 +224,39 @@ impl TypeSpace { // TODO Look for containment cycles that we need to break with a Box + // While we aren't yet handling the general case of type containment + // cycles, it's not that bad to look at trivial cycles (A->A). + for index in 0..def_len { + let type_id = TypeId(base_id + index); + let type_entry = self.id_to_entry.get(&type_id).unwrap(); + + let mut trivial_cycle = false; + if let TypeEntryDetails::Struct(s) = &type_entry.details { + for prop in &s.properties { + if prop.type_id == type_id { + trivial_cycle = true; + break; + } + } + } + + if trivial_cycle { + let box_id = self.id_to_box(&type_id); + let type_entry = self.id_to_entry.get_mut(&type_id).unwrap(); + if let TypeEntryDetails::Struct(ref mut s) = &mut type_entry.details { + for prop in &mut s.properties { + if prop.type_id == type_id { + prop.type_id = box_id.clone(); + } + } + } + } + } + + for value in self.id_to_entry.values() { + info!("{:?}", value); + } + Ok(()) } @@ -358,6 +394,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 +480,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/type_entry.rs b/typify-impl/src/type_entry.rs index 182d3d15..572d5d96 100644 --- a/typify-impl/src/type_entry.rs +++ b/typify-impl/src/type_entry.rs @@ -72,6 +72,8 @@ pub(crate) enum TypeEntryDetails { /// reference types in particular to represent simple type aliases between /// types named as reference targets. Reference(TypeId), + + Box(TypeId), } #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] @@ -388,6 +390,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 @@ -481,6 +484,17 @@ impl TypeEntry { quote! { ( #(#type_streams),* ) } } + 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::Unit => quote! { () }, TypeEntryDetails::String => quote! { String }, TypeEntryDetails::BuiltIn(name) @@ -525,6 +539,7 @@ impl TypeEntry { | TypeEntryDetails::Array(_) | TypeEntryDetails::Map(_, _) | TypeEntryDetails::Set(_) + | TypeEntryDetails::Box(_) | TypeEntryDetails::BuiltIn(_) => { let ident = self.type_ident(type_space, true); quote! { @@ -580,6 +595,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 ({})", From 251b013aa2f847e83b9560ca7130ea64637657f5 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Mon, 14 Mar 2022 12:17:59 -0400 Subject: [PATCH 02/13] fmt --- typify-impl/src/convert.rs | 3 ++- typify-impl/src/lib.rs | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/typify-impl/src/convert.rs b/typify-impl/src/convert.rs index f0cabf17..54caca12 100644 --- a/typify-impl/src/convert.rs +++ b/typify-impl/src/convert.rs @@ -714,7 +714,8 @@ impl TypeSpace { // Rust can emit "anyOf":[{"$ref":"#/definitions/C"},{"type":"null"} for // Option, so match against that here. - if let Some(option_type_entry) = self.maybe_option(type_name.clone(), metadata, subschemas) { + if let Some(option_type_entry) = self.maybe_option(type_name.clone(), metadata, subschemas) + { return Ok((option_type_entry, metadata)); } diff --git a/typify-impl/src/lib.rs b/typify-impl/src/lib.rs index f28baa5a..467663ff 100644 --- a/typify-impl/src/lib.rs +++ b/typify-impl/src/lib.rs @@ -180,8 +180,11 @@ impl TypeSpace { None => &ref_name, }; - info!("converting type: {} with schema {}", type_name, - serde_json::to_string(&schema).unwrap()); + 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)?; From ba05861a897c9ce0686d862a03bd6daa3fbbbc01 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Mon, 14 Mar 2022 17:29:36 -0400 Subject: [PATCH 03/13] make it actually work! note, not nice code --- typify-impl/src/convert.rs | 54 +++++++++++---------- typify-impl/src/lib.rs | 93 ++++++++++++++++++++++++++++-------- typify-impl/src/test_util.rs | 21 ++++++-- 3 files changed, 121 insertions(+), 47 deletions(-) diff --git a/typify-impl/src/convert.rs b/typify-impl/src/convert.rs index 54caca12..daa8bcc8 100644 --- a/typify-impl/src/convert.rs +++ b/typify-impl/src/convert.rs @@ -1022,22 +1022,13 @@ mod tests { #[test] fn test_trivial_cycle() { - // Currently schemars will actually include the type A twice if this is - // made a RootSchema; it should really have a reference that it uses as - // the RootSchema. To workaround this, we refer to A from B below. #[derive(JsonSchema, Schema)] #[allow(dead_code)] struct A { a: Box, } - #[derive(JsonSchema, Schema)] - #[allow(dead_code)] - struct B { - a: A, - } - - validate_output::(); + validate_output::(); } #[test] @@ -1048,17 +1039,36 @@ mod tests { a: Option>, } + validate_output::(); + } + + #[test] + fn test_enum_trivial_cycle() { #[derive(JsonSchema, Schema)] + #[serde(tag = "type")] #[allow(dead_code)] - struct B { - a: A, + enum A { + Varant1 { + a: u64, + b: Vec, + rop: Option>, + }, + Variant2 { + a: u64, + b: String, + }, + Variant3 { + a: u64, + b: u64, + c: u64, + }, } - validate_output::(); + validate_output::(); } #[test] - fn test_basic_option() { + fn test_basic_option_flat() { #[derive(JsonSchema, Schema)] #[allow(dead_code)] struct C {} @@ -1073,24 +1083,18 @@ mod tests { } #[test] - fn test_basic_option_one_deep() { - #[derive(JsonSchema, Schema)] - #[allow(dead_code)] - struct C {} - + fn test_unit_option() { #[derive(JsonSchema, Schema)] #[allow(dead_code)] - struct A { - a: Option, - } + struct Foo; #[derive(JsonSchema, Schema)] #[allow(dead_code)] - struct B { - a: A, + struct Bar { + a: Option, } - validate_output::(); + validate_output::(); } // TODO we can turn this on once we generate proper sets. diff --git a/typify-impl/src/lib.rs b/typify-impl/src/lib.rs index 467663ff..af48f6ed 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; @@ -231,33 +231,88 @@ impl TypeSpace { // cycles, it's not that bad to look at trivial cycles (A->A). for index in 0..def_len { let type_id = TypeId(base_id + index); - let type_entry = self.id_to_entry.get(&type_id).unwrap(); - let mut trivial_cycle = false; - if let TypeEntryDetails::Struct(s) = &type_entry.details { - for prop in &s.properties { + let mut type_entry: TypeEntry = self.id_to_entry.get(&type_id).unwrap().clone(); + let mut box_id = None; + + if let TypeEntryDetails::Struct(s) = &mut type_entry.details { + for prop in &mut s.properties { if prop.type_id == type_id { - trivial_cycle = true; - break; + info!("boxing property"); + let box_id = box_id.get_or_insert_with(|| self.id_to_box(&type_id)); + prop.type_id = box_id.clone(); + } else { + // Chase only options for now. TODO make this better + // comment + let mut prop_type_entry = + self.id_to_entry.get_mut(&prop.type_id).unwrap().clone(); + if let TypeEntryDetails::Option(option_type_id) = + &mut prop_type_entry.details + { + if *option_type_id == type_id { + info!("boxing option"); + let box_id = box_id.get_or_insert_with(|| self.id_to_box(&type_id)); + *option_type_id = box_id.clone(); + } + } + // TODO this is unconditional + let _ = self + .id_to_entry + .insert(prop.type_id.clone(), prop_type_entry); } } - } - - if trivial_cycle { - let box_id = self.id_to_box(&type_id); - let type_entry = self.id_to_entry.get_mut(&type_id).unwrap(); - if let TypeEntryDetails::Struct(ref mut s) = &mut type_entry.details { - for prop in &mut s.properties { - if prop.type_id == type_id { - prop.type_id = box_id.clone(); + } else if let TypeEntryDetails::Enum(type_entry_enum) = &mut type_entry.details { + for variant in &mut type_entry_enum.variants { + match &mut variant.details { + VariantDetails::Simple => {} + VariantDetails::Tuple(vec_type_id) => { + for tuple_type_id in vec_type_id { + if *tuple_type_id == type_id { + info!("boxing tuple type"); + let box_id = + box_id.get_or_insert_with(|| self.id_to_box(&type_id)); + *tuple_type_id = box_id.clone(); + } + } + } + VariantDetails::Struct(vec_struct_property) => { + for struct_property in vec_struct_property { + let vec_type_id = &mut struct_property.type_id; + if *vec_type_id == type_id { + info!("boxing variant struct type"); + let box_id = + box_id.get_or_insert_with(|| self.id_to_box(&type_id)); + *vec_type_id = box_id.clone(); + } else { + let mut prop_type_entry = + self.id_to_entry.get_mut(vec_type_id).unwrap().clone(); + + if let TypeEntryDetails::Option(option_type_id) = + &mut prop_type_entry.details + { + if *option_type_id == type_id { + info!("boxing option in struct property"); + let box_id = box_id + .get_or_insert_with(|| self.id_to_box(&type_id)); + *option_type_id = box_id.clone(); + } + } + + // TODO unconditional + let _ = self + .id_to_entry + .insert(vec_type_id.clone(), prop_type_entry); + } + } } } } } - } - for value in self.id_to_entry.values() { - info!("{:?}", value); + if box_id.is_some() { + // we modified it + let _ = self.id_to_entry.insert(type_id, type_entry); + } } Ok(()) diff --git a/typify-impl/src/test_util.rs b/typify-impl/src/test_util.rs index 31f024ac..f2dbf815 100644 --- a/typify-impl/src/test_util.rs +++ b/typify-impl/src/test_util.rs @@ -34,12 +34,27 @@ 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(); + + // 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) { + info!("{} is already ref id {:?}", name, already_type_id); + type_space + .id_to_entry + .get(&already_type_id) + .unwrap() + .clone() + } else { + info!("{} needs to convert from schema.schema", name); + let (ty, _) = type_space + .convert_schema_object(Name::Required(name), &schema.schema) + .unwrap(); + ty + }; let output = ty.output(&type_space); From 3459a0c827ae97b68eb19352d4b39c8edaa56488 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Tue, 15 Mar 2022 10:53:47 -0400 Subject: [PATCH 04/13] make it a little nicer --- typify-impl/src/convert.rs | 14 ++--- typify-impl/src/lib.rs | 119 +++++++++++++++++++++-------------- typify-impl/src/test_util.rs | 2 - 3 files changed, 78 insertions(+), 57 deletions(-) diff --git a/typify-impl/src/convert.rs b/typify-impl/src/convert.rs index daa8bcc8..6e06b480 100644 --- a/typify-impl/src/convert.rs +++ b/typify-impl/src/convert.rs @@ -1043,25 +1043,21 @@ mod tests { } #[test] - fn test_enum_trivial_cycle() { + fn test_enum_trivial_cycles() { #[derive(JsonSchema, Schema)] - #[serde(tag = "type")] #[allow(dead_code)] enum A { + Variant0(u64), Varant1 { a: u64, b: Vec, rop: Option>, }, Variant2 { - a: u64, - b: String, - }, - Variant3 { - a: u64, - b: u64, - c: u64, + a: Box, }, + Variant3(u64, Box), + Variant4(Option>, String), } validate_output::(); diff --git a/typify-impl/src/lib.rs b/typify-impl/src/lib.rs index af48f6ed..5ebe22d4 100644 --- a/typify-impl/src/lib.rs +++ b/typify-impl/src/lib.rs @@ -225,97 +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 - - // While we aren't yet handling the general case of type containment - // cycles, it's not that bad to look at trivial cycles (A->A). + // 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 type_entry = self.id_to_entry.get_mut(&type_id).unwrap().clone(); + self.break_trivial_cyclic_refs(&type_id, &mut type_entry); + let _ = self.id_to_entry.insert(type_id, type_entry); + } - let mut type_entry: TypeEntry = self.id_to_entry.get(&type_id).unwrap().clone(); - let mut box_id = None; + Ok(()) + } - if let TypeEntryDetails::Struct(s) = &mut type_entry.details { + /// 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 break_trivial_cyclic_refs(&mut self, type_id: &TypeId, type_entry: &mut TypeEntry) { + // TODO note that this function unconditionally calls id_to_box and + // does not search for an existing Box(type_id) - should it? That + // would add an additional n to runtime complexity. + let box_id = self.id_to_box(&type_id); + + match &mut type_entry.details { + // Look for the case where a struct property refers to itself + TypeEntryDetails::Struct(s) => { for prop in &mut s.properties { - if prop.type_id == type_id { - info!("boxing property"); - let box_id = box_id.get_or_insert_with(|| self.id_to_box(&type_id)); + if prop.type_id == *type_id { + // A struct property directly refers to itself prop.type_id = box_id.clone(); } else { - // Chase only options for now. TODO make this better - // comment - let mut prop_type_entry = - self.id_to_entry.get_mut(&prop.type_id).unwrap().clone(); + // A struct property optionally refers to itself + let prop_type_entry: &mut TypeEntry = + self.id_to_entry.get_mut(&prop.type_id).unwrap(); + if let TypeEntryDetails::Option(option_type_id) = &mut prop_type_entry.details { - if *option_type_id == type_id { - info!("boxing option"); - let box_id = box_id.get_or_insert_with(|| self.id_to_box(&type_id)); + if *option_type_id == *type_id { *option_type_id = box_id.clone(); } } - // TODO this is unconditional - let _ = self - .id_to_entry - .insert(prop.type_id.clone(), prop_type_entry); } } - } else if let TypeEntryDetails::Enum(type_entry_enum) = &mut type_entry.details { + } + // Look for the cases where an enum variant refers to itself + 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 enum VariantDetails::Tuple(vec_type_id) => { for tuple_type_id in vec_type_id { - if *tuple_type_id == type_id { - info!("boxing tuple type"); - let box_id = - box_id.get_or_insert_with(|| self.id_to_box(&type_id)); + // A tuple entry directly refers to the enum + if *tuple_type_id == *type_id { *tuple_type_id = box_id.clone(); + } else { + // A tuple entry optionally refers to the + // enum + let tuple_type_entry: &mut TypeEntry = + self.id_to_entry.get_mut(&tuple_type_id).unwrap(); + + if let TypeEntryDetails::Option(option_type_id) = + &mut tuple_type_entry.details + { + if *option_type_id == *type_id { + *option_type_id = box_id.clone(); + } + } } } } + // Look for a struct property that refers to the enum VariantDetails::Struct(vec_struct_property) => { for struct_property in vec_struct_property { let vec_type_id = &mut struct_property.type_id; - if *vec_type_id == type_id { - info!("boxing variant struct type"); - let box_id = - box_id.get_or_insert_with(|| self.id_to_box(&type_id)); + // A struct property refers to the enum + if *vec_type_id == *type_id { *vec_type_id = box_id.clone(); } else { + // A struct property optionally refers to + // the enum let mut prop_type_entry = self.id_to_entry.get_mut(vec_type_id).unwrap().clone(); if let TypeEntryDetails::Option(option_type_id) = &mut prop_type_entry.details { - if *option_type_id == type_id { - info!("boxing option in struct property"); - let box_id = box_id - .get_or_insert_with(|| self.id_to_box(&type_id)); + if *option_type_id == *type_id { *option_type_id = box_id.clone(); } } - - // TODO unconditional - let _ = self - .id_to_entry - .insert(vec_type_id.clone(), prop_type_entry); } } } } } } - - if box_id.is_some() { - // we modified it - let _ = self.id_to_entry.insert(type_id, type_entry); - } + // Containers that can be size 0 are *not* cyclic references for that type + TypeEntryDetails::Array(_) => {} + TypeEntryDetails::Set(_) => {} + TypeEntryDetails::Map(_, _) => {} + // Everything else can be ignored + _ => {} } - - Ok(()) } /// Add a new type and return a type identifier that may be used in diff --git a/typify-impl/src/test_util.rs b/typify-impl/src/test_util.rs index f2dbf815..c2562ebb 100644 --- a/typify-impl/src/test_util.rs +++ b/typify-impl/src/test_util.rs @@ -42,14 +42,12 @@ fn validate_output_impl(ignore_variant_names: bool) { // 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) { - info!("{} is already ref id {:?}", name, already_type_id); type_space .id_to_entry .get(&already_type_id) .unwrap() .clone() } else { - info!("{} needs to convert from schema.schema", name); let (ty, _) = type_space .convert_schema_object(Name::Required(name), &schema.schema) .unwrap(); From 38411598e2c757f3128d22dc0195e33504fa84ff Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Tue, 15 Mar 2022 10:57:25 -0400 Subject: [PATCH 05/13] expand comment for maybe_option in convert_any_of --- typify-impl/src/convert.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/typify-impl/src/convert.rs b/typify-impl/src/convert.rs index 6e06b480..ffa77462 100644 --- a/typify-impl/src/convert.rs +++ b/typify-impl/src/convert.rs @@ -713,7 +713,8 @@ impl TypeSpace { } // Rust can emit "anyOf":[{"$ref":"#/definitions/C"},{"type":"null"} for - // Option, so match against that here. + // 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)); From 8b70340fc85c44b51b45a0583a09c218671a29bf Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Tue, 15 Mar 2022 10:58:48 -0400 Subject: [PATCH 06/13] move Box around --- typify-impl/src/type_entry.rs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/typify-impl/src/type_entry.rs b/typify-impl/src/type_entry.rs index 572d5d96..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), @@ -72,8 +73,6 @@ pub(crate) enum TypeEntryDetails { /// reference types in particular to represent simple type aliases between /// types named as reference targets. Reference(TypeId), - - Box(TypeId), } #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] @@ -437,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 @@ -484,17 +494,6 @@ impl TypeEntry { quote! { ( #(#type_streams),* ) } } - 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::Unit => quote! { () }, TypeEntryDetails::String => quote! { String }, TypeEntryDetails::BuiltIn(name) From 425f906bba23de401baac2c68cea629c960fad79 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Tue, 15 Mar 2022 11:20:42 -0400 Subject: [PATCH 07/13] do not unconditionally create box type, use self.id_to_box handle looking for optional self-ref with recursion --- typify-impl/src/lib.rs | 86 +++++++++++++++++++----------------------- 1 file changed, 38 insertions(+), 48 deletions(-) diff --git a/typify-impl/src/lib.rs b/typify-impl/src/lib.rs index 5ebe22d4..2fa26880 100644 --- a/typify-impl/src/lib.rs +++ b/typify-impl/src/lib.rs @@ -229,8 +229,11 @@ impl TypeSpace { // 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); + self.break_trivial_cyclic_refs(&type_id, &mut type_entry, &mut box_id); let _ = self.id_to_entry.insert(type_id, type_entry); } @@ -254,82 +257,69 @@ impl TypeSpace { /// certain cycles introduce a question of *where* to Box to break the /// cycle, and there's no one answer to this. /// - fn break_trivial_cyclic_refs(&mut self, type_id: &TypeId, type_entry: &mut TypeEntry) { - // TODO note that this function unconditionally calls id_to_box and - // does not search for an existing Box(type_id) - should it? That - // would add an additional n to runtime complexity. - let box_id = self.id_to_box(&type_id); + fn break_trivial_cyclic_refs(&mut self, 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 itself + // Look for the case where an option refers to the parent type + TypeEntryDetails::Option(option_type_id) => { + if *option_type_id == *type_id { + *option_type_id = box_id.get_or_insert_with(|| self.id_to_box(type_id)).clone(); + } + } + + // Look for the case where a struct property refers to the parent + // type TypeEntryDetails::Struct(s) => { for prop in &mut s.properties { if prop.type_id == *type_id { - // A struct property directly refers to itself - prop.type_id = box_id.clone(); + // A struct property directly refers to the parent type + prop.type_id = box_id.get_or_insert_with(|| self.id_to_box(type_id)).clone(); } else { - // A struct property optionally refers to itself - let prop_type_entry: &mut TypeEntry = - self.id_to_entry.get_mut(&prop.type_id).unwrap(); - - if let TypeEntryDetails::Option(option_type_id) = - &mut prop_type_entry.details - { - if *option_type_id == *type_id { - *option_type_id = box_id.clone(); - } - } + // A struct property optionally refers to the parent type + let mut prop_type_entry = + self.id_to_entry.get_mut(&prop.type_id).unwrap().clone(); + self.break_trivial_cyclic_refs(&type_id, &mut prop_type_entry, box_id); + let _ = self.id_to_entry.insert(prop.type_id.clone(), prop_type_entry); } } } - // Look for the cases where an enum variant refers to itself + + // 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 enum + // Look for a tuple entry that refers to the parent type VariantDetails::Tuple(vec_type_id) => { for tuple_type_id in vec_type_id { - // A tuple entry directly refers to the enum + // A tuple entry directly refers to the parent type if *tuple_type_id == *type_id { - *tuple_type_id = box_id.clone(); + *tuple_type_id = box_id.get_or_insert_with(|| self.id_to_box(type_id)).clone(); } else { - // A tuple entry optionally refers to the - // enum - let tuple_type_entry: &mut TypeEntry = - self.id_to_entry.get_mut(&tuple_type_id).unwrap(); - - if let TypeEntryDetails::Option(option_type_id) = - &mut tuple_type_entry.details - { - if *option_type_id == *type_id { - *option_type_id = box_id.clone(); - } - } + // A tuple entry optionally refers to the parent type + let mut tuple_type_entry = + self.id_to_entry.get_mut(&tuple_type_id).unwrap().clone(); + self.break_trivial_cyclic_refs(&type_id, &mut tuple_type_entry, box_id); + let _ = self.id_to_entry.insert(tuple_type_id.clone(), tuple_type_entry); } } } - // Look for a struct property that refers to the enum + // 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; - // A struct property refers to the enum + // A struct property refers to the parent type if *vec_type_id == *type_id { - *vec_type_id = box_id.clone(); + *vec_type_id = box_id.get_or_insert_with(|| self.id_to_box(type_id)).clone(); } else { // A struct property optionally refers to - // the enum + // the parent type let mut prop_type_entry = self.id_to_entry.get_mut(vec_type_id).unwrap().clone(); - - if let TypeEntryDetails::Option(option_type_id) = - &mut prop_type_entry.details - { - if *option_type_id == *type_id { - *option_type_id = box_id.clone(); - } - } + self.break_trivial_cyclic_refs(&type_id, &mut prop_type_entry, box_id); + let _ = self.id_to_entry.insert(vec_type_id.clone(), prop_type_entry); } } } From 9644b1344791253661c02448aa3d3fc19c4efbd2 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Tue, 15 Mar 2022 11:21:58 -0400 Subject: [PATCH 08/13] fmt --- typify-impl/src/lib.rs | 48 ++++++++++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/typify-impl/src/lib.rs b/typify-impl/src/lib.rs index 2fa26880..5e4c5661 100644 --- a/typify-impl/src/lib.rs +++ b/typify-impl/src/lib.rs @@ -257,13 +257,19 @@ impl TypeSpace { /// certain cycles introduce a question of *where* to Box to break the /// cycle, and there's no one answer to this. /// - fn break_trivial_cyclic_refs(&mut self, type_id: &TypeId, type_entry: &mut TypeEntry, box_id: &mut Option) { - + fn break_trivial_cyclic_refs( + &mut self, + type_id: &TypeId, + type_entry: &mut TypeEntry, + box_id: &mut Option, + ) { match &mut type_entry.details { // Look for the case where an option refers to the parent type TypeEntryDetails::Option(option_type_id) => { if *option_type_id == *type_id { - *option_type_id = box_id.get_or_insert_with(|| self.id_to_box(type_id)).clone(); + *option_type_id = box_id + .get_or_insert_with(|| self.id_to_box(type_id)) + .clone(); } } @@ -273,13 +279,17 @@ impl TypeSpace { for prop in &mut s.properties { if prop.type_id == *type_id { // A struct property directly refers to the parent type - prop.type_id = box_id.get_or_insert_with(|| self.id_to_box(type_id)).clone(); + prop.type_id = box_id + .get_or_insert_with(|| self.id_to_box(type_id)) + .clone(); } else { // A struct property optionally refers to the parent type let mut prop_type_entry = self.id_to_entry.get_mut(&prop.type_id).unwrap().clone(); self.break_trivial_cyclic_refs(&type_id, &mut prop_type_entry, box_id); - let _ = self.id_to_entry.insert(prop.type_id.clone(), prop_type_entry); + let _ = self + .id_to_entry + .insert(prop.type_id.clone(), prop_type_entry); } } } @@ -296,13 +306,21 @@ impl TypeSpace { for tuple_type_id in vec_type_id { // A tuple entry directly refers to the parent type if *tuple_type_id == *type_id { - *tuple_type_id = box_id.get_or_insert_with(|| self.id_to_box(type_id)).clone(); + *tuple_type_id = box_id + .get_or_insert_with(|| self.id_to_box(type_id)) + .clone(); } else { // A tuple entry optionally refers to the parent type let mut tuple_type_entry = self.id_to_entry.get_mut(&tuple_type_id).unwrap().clone(); - self.break_trivial_cyclic_refs(&type_id, &mut tuple_type_entry, box_id); - let _ = self.id_to_entry.insert(tuple_type_id.clone(), tuple_type_entry); + self.break_trivial_cyclic_refs( + &type_id, + &mut tuple_type_entry, + box_id, + ); + let _ = self + .id_to_entry + .insert(tuple_type_id.clone(), tuple_type_entry); } } } @@ -312,14 +330,22 @@ impl TypeSpace { let vec_type_id = &mut struct_property.type_id; // A struct property refers to the parent type if *vec_type_id == *type_id { - *vec_type_id = box_id.get_or_insert_with(|| self.id_to_box(type_id)).clone(); + *vec_type_id = box_id + .get_or_insert_with(|| self.id_to_box(type_id)) + .clone(); } else { // A struct property optionally refers to // the parent type let mut prop_type_entry = self.id_to_entry.get_mut(vec_type_id).unwrap().clone(); - self.break_trivial_cyclic_refs(&type_id, &mut prop_type_entry, box_id); - let _ = self.id_to_entry.insert(vec_type_id.clone(), prop_type_entry); + self.break_trivial_cyclic_refs( + &type_id, + &mut prop_type_entry, + box_id, + ); + let _ = self + .id_to_entry + .insert(vec_type_id.clone(), prop_type_entry); } } } From 961dbc1950d52a7bbeb1cf7c8a0db6f26b28ff66 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Wed, 16 Mar 2022 09:04:28 -0400 Subject: [PATCH 09/13] rename argument to parent_type_id --- typify-impl/src/lib.rs | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/typify-impl/src/lib.rs b/typify-impl/src/lib.rs index 5e4c5661..cff8d6d6 100644 --- a/typify-impl/src/lib.rs +++ b/typify-impl/src/lib.rs @@ -259,16 +259,16 @@ impl TypeSpace { /// fn break_trivial_cyclic_refs( &mut self, - type_id: &TypeId, + parent_type_id: &TypeId, type_entry: &mut TypeEntry, box_id: &mut Option, ) { match &mut type_entry.details { // Look for the case where an option refers to the parent type TypeEntryDetails::Option(option_type_id) => { - if *option_type_id == *type_id { + if *option_type_id == *parent_type_id { *option_type_id = box_id - .get_or_insert_with(|| self.id_to_box(type_id)) + .get_or_insert_with(|| self.id_to_box(parent_type_id)) .clone(); } } @@ -277,16 +277,20 @@ impl TypeSpace { // type TypeEntryDetails::Struct(s) => { for prop in &mut s.properties { - if prop.type_id == *type_id { + if prop.type_id == *parent_type_id { // A struct property directly refers to the parent type prop.type_id = box_id - .get_or_insert_with(|| self.id_to_box(type_id)) + .get_or_insert_with(|| self.id_to_box(parent_type_id)) .clone(); } else { // A struct property optionally refers to the parent type let mut prop_type_entry = self.id_to_entry.get_mut(&prop.type_id).unwrap().clone(); - self.break_trivial_cyclic_refs(&type_id, &mut prop_type_entry, box_id); + self.break_trivial_cyclic_refs( + &parent_type_id, + &mut prop_type_entry, + box_id, + ); let _ = self .id_to_entry .insert(prop.type_id.clone(), prop_type_entry); @@ -305,16 +309,16 @@ impl TypeSpace { VariantDetails::Tuple(vec_type_id) => { for tuple_type_id in vec_type_id { // A tuple entry directly refers to the parent type - if *tuple_type_id == *type_id { + if *tuple_type_id == *parent_type_id { *tuple_type_id = box_id - .get_or_insert_with(|| self.id_to_box(type_id)) + .get_or_insert_with(|| self.id_to_box(parent_type_id)) .clone(); } else { // A tuple entry optionally refers to the parent type let mut tuple_type_entry = self.id_to_entry.get_mut(&tuple_type_id).unwrap().clone(); self.break_trivial_cyclic_refs( - &type_id, + &parent_type_id, &mut tuple_type_entry, box_id, ); @@ -329,9 +333,9 @@ impl TypeSpace { for struct_property in vec_struct_property { let vec_type_id = &mut struct_property.type_id; // A struct property refers to the parent type - if *vec_type_id == *type_id { + if *vec_type_id == *parent_type_id { *vec_type_id = box_id - .get_or_insert_with(|| self.id_to_box(type_id)) + .get_or_insert_with(|| self.id_to_box(parent_type_id)) .clone(); } else { // A struct property optionally refers to @@ -339,7 +343,7 @@ impl TypeSpace { let mut prop_type_entry = self.id_to_entry.get_mut(vec_type_id).unwrap().clone(); self.break_trivial_cyclic_refs( - &type_id, + &parent_type_id, &mut prop_type_entry, box_id, ); From 3d9210c400e07daadffab386e253d4c00e0e226e Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Wed, 16 Mar 2022 09:12:23 -0400 Subject: [PATCH 10/13] refactor duplicated logic into fn check_for_cyclic_ref --- typify-impl/src/lib.rs | 83 ++++++++++++------------------------------ 1 file changed, 23 insertions(+), 60 deletions(-) diff --git a/typify-impl/src/lib.rs b/typify-impl/src/lib.rs index cff8d6d6..cf996e93 100644 --- a/typify-impl/src/lib.rs +++ b/typify-impl/src/lib.rs @@ -257,6 +257,25 @@ impl TypeSpace { /// 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(); + self.break_trivial_cyclic_refs(&parent_type_id, &mut child_type_entry, box_id); + let _ = self + .id_to_entry + .insert(child_type_id.clone(), child_type_entry); + } + } + fn break_trivial_cyclic_refs( &mut self, parent_type_id: &TypeId, @@ -266,35 +285,14 @@ impl TypeSpace { match &mut 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(); - } + self.check_for_cyclic_ref(parent_type_id, option_type_id, box_id); } // Look for the case where a struct property refers to the parent // type TypeEntryDetails::Struct(s) => { for prop in &mut s.properties { - if prop.type_id == *parent_type_id { - // A struct property directly refers to the parent type - prop.type_id = box_id - .get_or_insert_with(|| self.id_to_box(parent_type_id)) - .clone(); - } else { - // A struct property optionally refers to the parent type - let mut prop_type_entry = - self.id_to_entry.get_mut(&prop.type_id).unwrap().clone(); - self.break_trivial_cyclic_refs( - &parent_type_id, - &mut prop_type_entry, - box_id, - ); - let _ = self - .id_to_entry - .insert(prop.type_id.clone(), prop_type_entry); - } + self.check_for_cyclic_ref(parent_type_id, &mut prop.type_id, box_id); } } @@ -308,49 +306,14 @@ impl TypeSpace { // Look for a tuple entry that refers to the parent type VariantDetails::Tuple(vec_type_id) => { for tuple_type_id in vec_type_id { - // A tuple entry directly refers to the parent type - if *tuple_type_id == *parent_type_id { - *tuple_type_id = box_id - .get_or_insert_with(|| self.id_to_box(parent_type_id)) - .clone(); - } else { - // A tuple entry optionally refers to the parent type - let mut tuple_type_entry = - self.id_to_entry.get_mut(&tuple_type_id).unwrap().clone(); - self.break_trivial_cyclic_refs( - &parent_type_id, - &mut tuple_type_entry, - box_id, - ); - let _ = self - .id_to_entry - .insert(tuple_type_id.clone(), tuple_type_entry); - } + 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; - // A struct property refers to the parent type - if *vec_type_id == *parent_type_id { - *vec_type_id = box_id - .get_or_insert_with(|| self.id_to_box(parent_type_id)) - .clone(); - } else { - // A struct property optionally refers to - // the parent type - let mut prop_type_entry = - self.id_to_entry.get_mut(vec_type_id).unwrap().clone(); - self.break_trivial_cyclic_refs( - &parent_type_id, - &mut prop_type_entry, - box_id, - ); - let _ = self - .id_to_entry - .insert(vec_type_id.clone(), prop_type_entry); - } + self.check_for_cyclic_ref(parent_type_id, vec_type_id, box_id); } } } From 63481f1589f577bafbafe1be937cccb2ceca4c5a Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Wed, 16 Mar 2022 09:16:19 -0400 Subject: [PATCH 11/13] check for cyclic ref in newtype --- typify-impl/src/convert.rs | 9 +++++++++ typify-impl/src/lib.rs | 7 +++++++ 2 files changed, 16 insertions(+) diff --git a/typify-impl/src/convert.rs b/typify-impl/src/convert.rs index ffa77462..9d06ad34 100644 --- a/typify-impl/src/convert.rs +++ b/typify-impl/src/convert.rs @@ -1064,6 +1064,15 @@ mod tests { 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)] diff --git a/typify-impl/src/lib.rs b/typify-impl/src/lib.rs index cf996e93..cae4c8e7 100644 --- a/typify-impl/src/lib.rs +++ b/typify-impl/src/lib.rs @@ -319,10 +319,17 @@ impl TypeSpace { } } } + + // 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 _ => {} } From bc4a1460ec92ccbc0b8f489803618b2925fc6ffd Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Wed, 16 Mar 2022 09:18:13 -0400 Subject: [PATCH 12/13] commit suggested comment --- typify-impl/src/test_util.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/typify-impl/src/test_util.rs b/typify-impl/src/test_util.rs index c2562ebb..206bc8a5 100644 --- a/typify-impl/src/test_util.rs +++ b/typify-impl/src/test_util.rs @@ -40,7 +40,13 @@ fn validate_output_impl(ignore_variant_names: bool) { .add_ref_types(schema.definitions.clone()) .unwrap(); - // If we have converted the type already, use that, otherwise convert schema object + // 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 From 6b34cc68b9ce8dc0c9794af78c107f869387c703 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Wed, 16 Mar 2022 10:59:38 -0400 Subject: [PATCH 13/13] do not recurse, simply stop looking at Option --- typify-impl/src/lib.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/typify-impl/src/lib.rs b/typify-impl/src/lib.rs index cae4c8e7..baac6db5 100644 --- a/typify-impl/src/lib.rs +++ b/typify-impl/src/lib.rs @@ -269,7 +269,20 @@ impl TypeSpace { .clone(); } else { let mut child_type_entry = self.id_to_entry.get_mut(&child_type_id).unwrap().clone(); - self.break_trivial_cyclic_refs(&parent_type_id, &mut child_type_entry, box_id); + + 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); @@ -283,11 +296,6 @@ impl TypeSpace { box_id: &mut Option, ) { match &mut type_entry.details { - // Look for the case where an option refers to the parent type - TypeEntryDetails::Option(option_type_id) => { - self.check_for_cyclic_ref(parent_type_id, option_type_id, box_id); - } - // Look for the case where a struct property refers to the parent // type TypeEntryDetails::Struct(s) => {