Box trivial cyclic refs #41

jmpesp · 2022-03-14T16:14:01Z

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

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

ahl

awesome! ship it

ahl · 2022-03-14T19:07:12Z

typify-impl/src/convert.rs

+        // Rust can emit "anyOf":[{"$ref":"#/definitions/C"},{"type":"null"} for
+        // Option, so match against that here.


I might say something like "short-circuit the check for mutual exclusivity below to handle the common case of an Option"

agreed, done in 3841159

ahl · 2022-03-14T19:30:45Z

typify-impl/src/lib.rs

@@ -221,6 +227,39 @@ impl TypeSpace {

        // 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).


Suggested change

// 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>).

Actually, I think this comment just needs a rewhack based on what's going on below now...

rewhacked as part of 3459a0c

ahl · 2022-03-14T19:31:25Z

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


move this up to live between Option and Array?

done in 8b70340

ahl · 2022-03-14T19:32:18Z

typify-impl/src/type_entry.rs

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


maybe move this next to Option because it's almost the same code?

done in 8b70340

note, not nice code

ahl · 2022-03-14T22:46:46Z

typify-impl/src/lib.rs

+                                    }
+
+                                    // TODO unconditional
+                                    let _ = self


I don't think we need to explicitly ignore the return, but it certainly doesn't hurt.

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

ahl

ship it!

ahl · 2022-03-15T22:46:58Z

typify-impl/src/lib.rs

+    ///
+    fn break_trivial_cyclic_refs(
+        &mut self,
+        type_id: &TypeId,


might you want to rename this parent_type_id to make the comments below more explicit? same for parent_type_entry

agreed, done in 961dbc1

ahl · 2022-03-15T22:48:50Z

typify-impl/src/lib.rs

+                    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);
+                    }


just noting that I think this logic is repeated 3 times

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

ahl · 2022-03-15T22:55:25Z

typify-impl/src/lib.rs

+            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();
+                }
+            }


are we exercising this code path with a test? I'm not sure I can imagine how we would get here

This one is mainly hit when recursing, aka

struct A { a: Option<Box<A>> }

ahl · 2022-03-15T22:57:59Z

typify-impl/src/lib.rs

+                        }
+                    }
+                }
+            }


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...

done in 63481f1

ahl · 2022-03-15T23:00:02Z

typify-impl/src/test_util.rs

-        .convert_schema_object(Name::Required(name), &schema.schema)
-        .unwrap();
+
+    // If we have converted the type already, use that, otherwise convert schema object


Suggested change

// 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

done in bc4a146

typify-impl/src/test_util.rs

jmpesp · 2022-03-16T14:55:21Z

There's currently a problem compiling omicron with this branch, please do not merge :)

jmpesp · 2022-03-16T14:59:49Z

6b34cc6 fixes it

Box struct properties with trivial cyclic refs

422eb69

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

jmpesp requested a review from ahl March 14, 2022 16:14

fmt

251b013

ahl reviewed Mar 14, 2022

View reviewed changes

make it actually work!

ba05861

note, not nice code

ahl reviewed Mar 14, 2022

View reviewed changes

jmpesp added 5 commits March 15, 2022 10:53

make it a little nicer

3459a0c

expand comment for maybe_option in convert_any_of

3841159

move Box around

8b70340

do not unconditionally create box type, use self.id_to_box

425f906

handle looking for optional self-ref with recursion

fmt

9644b13

jmpesp changed the title ~~Box struct properties with trivial cyclic refs~~ Box trivial cyclic refs Mar 15, 2022

jmpesp mentioned this pull request Mar 15, 2022

supply real volume data to propolis oxidecomputer/omicron#771

Merged

ahl reviewed Mar 15, 2022

View reviewed changes

jmpesp added 4 commits March 16, 2022 09:04

rename argument to parent_type_id

961dbc1

refactor duplicated logic into fn check_for_cyclic_ref

3d9210c

check for cyclic ref in newtype

63481f1

commit suggested comment

bc4a146

do not recurse, simply stop looking at Option

6b34cc6

jmpesp merged commit 6617df9 into oxidecomputer:main Mar 16, 2022

jmpesp deleted the box_cyclic_refs branch March 16, 2022 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Box trivial cyclic refs #41

Box trivial cyclic refs #41

jmpesp commented Mar 14, 2022

ahl left a comment

ahl Mar 14, 2022

jmpesp Mar 15, 2022

ahl Mar 14, 2022

ahl Mar 14, 2022

jmpesp Mar 15, 2022

ahl Mar 14, 2022

jmpesp Mar 15, 2022

ahl Mar 14, 2022

jmpesp Mar 15, 2022

ahl Mar 14, 2022

jmpesp Mar 15, 2022

ahl left a comment

ahl Mar 15, 2022

jmpesp Mar 16, 2022

ahl Mar 15, 2022

jmpesp Mar 16, 2022

ahl Mar 15, 2022

jmpesp Mar 16, 2022

ahl Mar 15, 2022

jmpesp Mar 16, 2022

ahl Mar 15, 2022

jmpesp Mar 16, 2022

jmpesp commented Mar 16, 2022

jmpesp commented Mar 16, 2022

		// Rust can emit "anyOf":[{"$ref":"#/definitions/C"},{"type":"null"} for
		// Option, so match against that here.

	// 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>).

	// 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

Box trivial cyclic refs #41

Box trivial cyclic refs #41

Conversation

jmpesp commented Mar 14, 2022

ahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmpesp commented Mar 16, 2022

jmpesp commented Mar 16, 2022