Box trivial cyclic refs#41
Conversation
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
| // Rust can emit "anyOf":[{"$ref":"#/definitions/C"},{"type":"null"} for | ||
| // Option, so match against that here. |
There was a problem hiding this comment.
I might say something like "short-circuit the check for mutual exclusivity below to handle the common case of an Option"
| // TODO Look for containment cycles that we need to break with a Box<T> | ||
|
|
||
| // 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). |
There was a problem hiding this comment.
| // cycles, it's not that bad to look at trivial cycles (A->A). | |
| // cycles, it's not that bad to look at trivial cycles (A->A) (including A->Option<A>). |
There was a problem hiding this comment.
Actually, I think this comment just needs a rewhack based on what's going on below now...
| /// types named as reference targets. | ||
| Reference(TypeId), | ||
|
|
||
| Box(TypeId), |
There was a problem hiding this comment.
move this up to live between Option and Array?
| 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> } | ||
| } | ||
|
|
There was a problem hiding this comment.
maybe move this next to Option because it's almost the same code?
note, not nice code
| } | ||
|
|
||
| // TODO unconditional | ||
| let _ = self |
There was a problem hiding this comment.
I don't think we need to explicitly ignore the return, but it certainly doesn't hurt.
There was a problem hiding this comment.
This has moved to the end of add_ref_types, and I removed the comment. The intention is clear from the code that we're modifying existing entries in id_to_entry unconditionally.
handle looking for optional self-ref with recursion
| /// | ||
| fn break_trivial_cyclic_refs( | ||
| &mut self, | ||
| type_id: &TypeId, |
There was a problem hiding this comment.
might you want to rename this parent_type_id to make the comments below more explicit? same for parent_type_entry
| 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(); | ||
| } 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); | ||
| } |
There was a problem hiding this comment.
just noting that I think this logic is repeated 3 times
There was a problem hiding this comment.
Thanks for the prompt - I originally assumed I could not pull that into a method because it wouldn't make the borrow checker happy, but turns out it's possible. Done in 3d9210c
| 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(); | ||
| } | ||
| } |
There was a problem hiding this comment.
are we exercising this code path with a test? I'm not sure I can imagine how we would get here
There was a problem hiding this comment.
This one is mainly hit when recursing, aka
struct A {
a: Option<Box<A>>
}
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
maybe we want to handle the TypeDetails::Newtype case? That's the only other one I can imagine hitting trivially. It would look like struct A(Box<A>)... (and talk about a useless type!!) Certainly we'll need to consider it when if/when we deal with the general case...
| .convert_schema_object(Name::Required(name), &schema.schema) | ||
| .unwrap(); | ||
|
|
||
| // If we have converted the type already, use that, otherwise convert schema object |
There was a problem hiding this comment.
| // 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 |
|
There's currently a problem compiling omicron with this branch, please do not merge :) |
|
6b34cc6 fixes it |
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