Skip to content

Commit 92a00e0

Browse files
ndrenondreno
authored
Merge pull request #7 from barbacane-dev/fix/upstream-pr1001-feedback
fix: address upstream review feedback on multi-type union fix
2 parents f947665 + 571fc84 commit 92a00e0

4 files changed

Lines changed: 14 additions & 87 deletions

File tree

‎CHANGELOG.adoc‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@
1313

1414
== Unreleased changes (barbacane-dev fork)
1515

16+
=== Tests / docs
17+
* tighten the multi-type `type` + subschemas regression fixture and code comment, per upstream review feedback on https://github.com/oxidecomputer/typify/pull/1001[oxidecomputer/typify#1001]: drop the `SingleTypeOneOfArrayBranch` case (whose contradictory schema caused codegen to emit an unreachable enum variant), trim verbose `$comment` rationales, and shorten the match-arm doc comment in `convert.rs`
18+
1619
https://github.com/barbacane-dev/typify/compare/v1.0.0\...HEAD[Full list of commits]
1720

1821
== 1.0.0 (released 2026-05-13)

‎typify-impl/src/convert.rs‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -464,12 +464,9 @@ impl TypeSpace {
464464
extensions: _,
465465
} => self.convert_unknown_enum(type_name, original_schema, metadata, enum_values),
466466

467-
// Subschemas with no body validation and either no type or a
467+
// Subschemas with no additional validation and either no type or a
468468
// single type. A multi-type (`Vec`) instance_type is deliberately
469-
// excluded so that it flows through the merge arm below, which
470-
// folds the type union into each subschema branch. Preserving the
471-
// earlier behaviour for `None` / `Single` keeps existing tolerant
472-
// handling of schemas whose outer type may conflict with a branch.
469+
// excluded so that it flows through the merge arm below.
473470
SchemaObject {
474471
metadata,
475472
instance_type: None | Some(SingleOrVec::Single(_)),

‎typify/tests/schemas/type-array-with-subschemas.json‎

Lines changed: 5 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
11
{
22
"$schema": "http://json-schema.org/draft-07/schema#",
3-
"$comment": "Regression coverage for issue #954: schemas with a multi-type `type: [...]` array on the same object as `oneOf` / `anyOf` / `allOf` / `not` previously discarded the `type` constraint (TODO at convert.rs wildcarded `instance_type`). Single-type + subschema cases must still pass through the earlier arm unchanged.",
3+
"$comment": "Coverage for issue #954: multi-type `type: [...]` alongside oneOf/anyOf/allOf/not.",
44
"definitions": {
55
"TypeArrayOneOfItems": {
6-
"$comment": "Canonical issue #954 shape. Each oneOf branch only constrains `items`, so the type union must be folded into every branch rather than dropped.",
76
"type": [ "string", "number", "boolean", "array" ],
87
"oneOf": [
98
{ "items": { "type": "string" } },
@@ -12,53 +11,33 @@
1211
]
1312
},
1413
"TypeArrayAnyOfItems": {
15-
"$comment": "Same shape as TypeArrayOneOfItems but using anyOf. anyOf travels through try_merge_with_each_subschema on a sibling path from oneOf; it should fold the type union the same way.",
1614
"type": [ "string", "number", "array" ],
1715
"anyOf": [
1816
{ "items": { "type": "string" } },
1917
{ "items": { "type": "number" } }
2018
]
2119
},
2220
"TypeArrayAllOfRefinement": {
23-
"$comment": "allOf is folded pairwise into the parent rather than producing branches. The type union must survive and the array-only constraints should apply when the Array variant is selected.",
2421
"type": [ "string", "array" ],
2522
"allOf": [
2623
{ "items": { "type": "string" } },
2724
{ "minItems": 1 }
2825
]
2926
},
3027
"TypeArrayNotExclusion": {
31-
"$comment": "not: object is redundant when the outer type union excludes object, but merging must not drop the type union when the not branch is applied.",
3228
"type": [ "string", "number", "array" ],
3329
"not": { "type": "object" }
3430
},
35-
"SingleTypeOneOfArrayBranch": {
36-
"$comment": "Regression guard (rust-collisions pattern). Outer singleton type + oneOf where one branch has a conflicting explicit type. This must continue to pass through the earlier arm (no merge), otherwise the array branch becomes unsatisfiable and is silently dropped.",
37-
"type": "object",
38-
"oneOf": [
39-
{
40-
"type": "object",
41-
"properties": { "kind": { "type": "string" } },
42-
"required": [ "kind" ]
43-
},
44-
{
45-
"type": "array",
46-
"items": { "type": "string" },
47-
"minItems": 2,
48-
"maxItems": 2
49-
}
50-
]
51-
},
5231
"TypeArrayOneOfExplicitArrayBranches": {
53-
"$comment": "Case 7: each oneOf branch pins `type: array`, so the intersection with the outer type union must prune the non-array primitives. Only array variants should be emitted.",
32+
"$comment": "Each oneOf branch pins `type: array`; the non-array variants from the outer union should be pruned.",
5433
"type": [ "string", "array" ],
5534
"oneOf": [
5635
{ "type": "array", "items": { "type": "string" } },
5736
{ "type": "array", "items": { "type": "number" } }
5837
]
5938
},
6039
"TypeArrayPartiallyUnsatisfiableOneOf": {
61-
"$comment": "Some oneOf branches conflict with the outer type union and should be dropped during merge; the surviving branch must carry the outer type union. The two eliminated branches use object/number which the outer `[string, array]` disallows.",
40+
"$comment": "Two oneOf branches conflict with the outer type union and should be dropped during merge.",
6241
"type": [ "string", "array" ],
6342
"oneOf": [
6443
{ "type": "object", "properties": { "name": { "type": "string" } } },
@@ -67,15 +46,15 @@
6746
]
6847
},
6948
"TypeArrayFullyUnsatisfiableOneOf": {
70-
"$comment": "Case 9: every branch conflicts with the outer type union, so `try_merge_with_each_subschema` returns empty and the schema resolves to never. Must emit an empty enum cleanly rather than panic.",
49+
"$comment": "Every branch conflicts with the outer type union; must resolve cleanly rather than panic.",
7150
"type": [ "string", "number" ],
7251
"oneOf": [
7352
{ "type": "array", "items": { "type": "string" } },
7453
{ "type": "object", "properties": { "k": { "type": "string" } } }
7554
]
7655
},
7756
"TypeArrayOneOfAndAllOf": {
78-
"$comment": "Case 10: oneOf and allOf on the same object, both alongside a multi-type `type` array. Exercises the full merge path (allOf folded first, then oneOf fanned out) with the Vec instance_type flowing through the merge arm.",
57+
"$comment": "oneOf and allOf on the same object, plus a multi-type `type` array.",
7958
"type": [ "string", "array" ],
8059
"allOf": [
8160
{ "minLength": 1 }

‎typify/tests/schemas/type-array-with-subschemas.rs‎

Lines changed: 4 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -25,56 +25,12 @@ pub mod error {
2525
}
2626
}
2727
}
28-
#[doc = "`SingleTypeOneOfArrayBranch`"]
29-
#[doc = r""]
30-
#[doc = r" <details><summary>JSON schema</summary>"]
31-
#[doc = r""]
32-
#[doc = r" ```json"]
33-
#[doc = "{"]
34-
#[doc = " \"$comment\": \"Regression guard (rust-collisions pattern). Outer singleton type + oneOf where one branch has a conflicting explicit type. This must continue to pass through the earlier arm (no merge), otherwise the array branch becomes unsatisfiable and is silently dropped.\","]
35-
#[doc = " \"oneOf\": ["]
36-
#[doc = " {"]
37-
#[doc = " \"properties\": {"]
38-
#[doc = " \"kind\": {"]
39-
#[doc = " \"type\": \"string\""]
40-
#[doc = " }"]
41-
#[doc = " },"]
42-
#[doc = " \"required\": ["]
43-
#[doc = " \"kind\""]
44-
#[doc = " ],"]
45-
#[doc = " \"type\": \"object\""]
46-
#[doc = " },"]
47-
#[doc = " {"]
48-
#[doc = " \"items\": {"]
49-
#[doc = " \"type\": \"string\""]
50-
#[doc = " },"]
51-
#[doc = " \"maxItems\": 2,"]
52-
#[doc = " \"minItems\": 2,"]
53-
#[doc = " \"type\": \"array\""]
54-
#[doc = " }"]
55-
#[doc = " ],"]
56-
#[doc = " \"type\": \"object\""]
57-
#[doc = "}"]
58-
#[doc = r" ```"]
59-
#[doc = r" </details>"]
60-
#[derive(:: serde :: Deserialize, :: serde :: Serialize, Clone, Debug)]
61-
#[serde(untagged)]
62-
pub enum SingleTypeOneOfArrayBranch {
63-
Object { kind: ::std::string::String },
64-
Array([::std::string::String; 2usize]),
65-
}
66-
impl ::std::convert::From<[::std::string::String; 2usize]> for SingleTypeOneOfArrayBranch {
67-
fn from(value: [::std::string::String; 2usize]) -> Self {
68-
Self::Array(value)
69-
}
70-
}
7128
#[doc = "`TypeArrayAllOfRefinement`"]
7229
#[doc = r""]
7330
#[doc = r" <details><summary>JSON schema</summary>"]
7431
#[doc = r""]
7532
#[doc = r" ```json"]
7633
#[doc = "{"]
77-
#[doc = " \"$comment\": \"allOf is folded pairwise into the parent rather than producing branches. The type union must survive and the array-only constraints should apply when the Array variant is selected.\","]
7834
#[doc = " \"allOf\": ["]
7935
#[doc = " {"]
8036
#[doc = " \"items\": {"]
@@ -109,7 +65,6 @@ impl ::std::convert::From<::std::vec::Vec<::std::string::String>> for TypeArrayA
10965
#[doc = r""]
11066
#[doc = r" ```json"]
11167
#[doc = "{"]
112-
#[doc = " \"$comment\": \"Same shape as TypeArrayOneOfItems but using anyOf. anyOf travels through try_merge_with_each_subschema on a sibling path from oneOf; it should fold the type union the same way.\","]
11368
#[doc = " \"anyOf\": ["]
11469
#[doc = " {"]
11570
#[doc = " \"items\": {"]
@@ -154,7 +109,6 @@ impl ::std::convert::From<TypeArrayAnyOfItemsVariant1> for TypeArrayAnyOfItems {
154109
#[doc = "{"]
155110
#[doc = " \"allOf\": ["]
156111
#[doc = " {"]
157-
#[doc = " \"$comment\": \"Same shape as TypeArrayOneOfItems but using anyOf. anyOf travels through try_merge_with_each_subschema on a sibling path from oneOf; it should fold the type union the same way.\","]
158112
#[doc = " \"type\": ["]
159113
#[doc = " \"string\","]
160114
#[doc = " \"number\","]
@@ -202,7 +156,6 @@ impl ::std::convert::From<::std::vec::Vec<::std::string::String>> for TypeArrayA
202156
#[doc = "{"]
203157
#[doc = " \"allOf\": ["]
204158
#[doc = " {"]
205-
#[doc = " \"$comment\": \"Same shape as TypeArrayOneOfItems but using anyOf. anyOf travels through try_merge_with_each_subschema on a sibling path from oneOf; it should fold the type union the same way.\","]
206159
#[doc = " \"type\": ["]
207160
#[doc = " \"string\","]
208161
#[doc = " \"number\","]
@@ -248,7 +201,7 @@ impl ::std::convert::From<::std::vec::Vec<f64>> for TypeArrayAnyOfItemsVariant1
248201
#[doc = r""]
249202
#[doc = r" ```json"]
250203
#[doc = "{"]
251-
#[doc = " \"$comment\": \"Case 9: every branch conflicts with the outer type union, so `try_merge_with_each_subschema` returns empty and the schema resolves to never. Must emit an empty enum cleanly rather than panic.\","]
204+
#[doc = " \"$comment\": \"Every branch conflicts with the outer type union; must resolve cleanly rather than panic.\","]
252205
#[doc = " \"oneOf\": ["]
253206
#[doc = " {"]
254207
#[doc = " \"items\": {"]
@@ -292,7 +245,6 @@ pub enum TypeArrayFullyUnsatisfiableOneOf {}
292245
#[doc = r""]
293246
#[doc = r" ```json"]
294247
#[doc = "{"]
295-
#[doc = " \"$comment\": \"not: object is redundant when the outer type union excludes object, but merging must not drop the type union when the not branch is applied.\","]
296248
#[doc = " \"not\": {"]
297249
#[doc = " \"type\": \"object\""]
298250
#[doc = " },"]
@@ -327,7 +279,7 @@ impl ::std::convert::From<::std::vec::Vec<::serde_json::Value>> for TypeArrayNot
327279
#[doc = r""]
328280
#[doc = r" ```json"]
329281
#[doc = "{"]
330-
#[doc = " \"$comment\": \"Case 10: oneOf and allOf on the same object, both alongside a multi-type `type` array. Exercises the full merge path (allOf folded first, then oneOf fanned out) with the Vec instance_type flowing through the merge arm.\","]
282+
#[doc = " \"$comment\": \"oneOf and allOf on the same object, plus a multi-type `type` array.\","]
331283
#[doc = " \"allOf\": ["]
332284
#[doc = " {"]
333285
#[doc = " \"minLength\": 1"]
@@ -604,7 +556,7 @@ impl<'de> ::serde::Deserialize<'de> for TypeArrayOneOfAndAllOfVariant1String {
604556
#[doc = r""]
605557
#[doc = r" ```json"]
606558
#[doc = "{"]
607-
#[doc = " \"$comment\": \"Case 7: each oneOf branch pins `type: array`, so the intersection with the outer type union must prune the non-array primitives. Only array variants should be emitted.\","]
559+
#[doc = " \"$comment\": \"Each oneOf branch pins `type: array`; the non-array variants from the outer union should be pruned.\","]
608560
#[doc = " \"oneOf\": ["]
609561
#[doc = " {"]
610562
#[doc = " \"items\": {"]
@@ -650,7 +602,6 @@ impl ::std::convert::From<::std::vec::Vec<f64>> for TypeArrayOneOfExplicitArrayB
650602
#[doc = r""]
651603
#[doc = r" ```json"]
652604
#[doc = "{"]
653-
#[doc = " \"$comment\": \"Canonical issue #954 shape. Each oneOf branch only constrains `items`, so the type union must be folded into every branch rather than dropped.\","]
654605
#[doc = " \"oneOf\": ["]
655606
#[doc = " {"]
656607
#[doc = " \"items\": {"]
@@ -707,7 +658,6 @@ impl ::std::convert::From<TypeArrayOneOfItemsVariant2> for TypeArrayOneOfItems {
707658
#[doc = "{"]
708659
#[doc = " \"allOf\": ["]
709660
#[doc = " {"]
710-
#[doc = " \"$comment\": \"Canonical issue #954 shape. Each oneOf branch only constrains `items`, so the type union must be folded into every branch rather than dropped.\","]
711661
#[doc = " \"type\": ["]
712662
#[doc = " \"string\","]
713663
#[doc = " \"number\","]
@@ -769,7 +719,6 @@ impl ::std::convert::From<::std::vec::Vec<::std::string::String>> for TypeArrayO
769719
#[doc = "{"]
770720
#[doc = " \"allOf\": ["]
771721
#[doc = " {"]
772-
#[doc = " \"$comment\": \"Canonical issue #954 shape. Each oneOf branch only constrains `items`, so the type union must be folded into every branch rather than dropped.\","]
773722
#[doc = " \"type\": ["]
774723
#[doc = " \"string\","]
775724
#[doc = " \"number\","]
@@ -831,7 +780,6 @@ impl ::std::convert::From<::std::vec::Vec<f64>> for TypeArrayOneOfItemsVariant1
831780
#[doc = "{"]
832781
#[doc = " \"allOf\": ["]
833782
#[doc = " {"]
834-
#[doc = " \"$comment\": \"Canonical issue #954 shape. Each oneOf branch only constrains `items`, so the type union must be folded into every branch rather than dropped.\","]
835783
#[doc = " \"type\": ["]
836784
#[doc = " \"string\","]
837785
#[doc = " \"number\","]
@@ -891,7 +839,7 @@ impl ::std::convert::From<::std::vec::Vec<bool>> for TypeArrayOneOfItemsVariant2
891839
#[doc = r""]
892840
#[doc = r" ```json"]
893841
#[doc = "{"]
894-
#[doc = " \"$comment\": \"Some oneOf branches conflict with the outer type union and should be dropped during merge; the surviving branch must carry the outer type union. The two eliminated branches use object/number which the outer `[string, array]` disallows.\","]
842+
#[doc = " \"$comment\": \"Two oneOf branches conflict with the outer type union and should be dropped during merge.\","]
895843
#[doc = " \"oneOf\": ["]
896844
#[doc = " {"]
897845
#[doc = " \"properties\": {"]

0 commit comments

Comments
 (0)