From 571fc84dc29479e8159a10b5606e07aec5ddab29 Mon Sep 17 00:00:00 2001 From: Nicolas Dreno Date: Wed, 13 May 2026 13:45:54 +0200 Subject: [PATCH] fix: address upstream review feedback on multi-type union fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upstream PR oxidecomputer/typify#1001 (Nicolas's re-submission of #1000 against upstream main, the same fix already shipped in v1.0.0 as commit 91580aa) received CHANGES_REQUESTED feedback from @ahl. Applying the same edits to the fork copy so the two stay aligned and the fixture stops asserting on bad codegen. Three changes: 1. Drop the `SingleTypeOneOfArrayBranch` test case. The schema `{ type: "object", oneOf: [{...object}, {...array}] }` is contradictory (outer `type: object` makes the array branch unsatisfiable), yet codegen was emitting the unreachable `Array([String; 2])` variant. The case was originally added as a "regression guard" for the pre-fix tolerant-but-wrong handling; keeping it would assert that the wrong output stays wrong. The underlying buggy behaviour (singleton outer type + conflicting branch type not pruned) is left as a follow-up — see TODO at the convert.rs match arm. 2. Trim `$comment` rationales in the JSON fixture. The originals read like LLM-generated test justifications; the surviving comments are one line each and only retained where the case isn't self-explanatory. 3. Replace the 6-line match-arm comment in convert.rs with @ahl's suggested 3-line version. Same intent, less narration. The match arm itself (`None | Some(SingleOrVec::Single(_))`) is unchanged — the actual fix from 91580aa is preserved. --- CHANGELOG.adoc | 3 + typify-impl/src/convert.rs | 7 +-- .../schemas/type-array-with-subschemas.json | 31 ++-------- .../schemas/type-array-with-subschemas.rs | 60 ++----------------- 4 files changed, 14 insertions(+), 87 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 443bf7b1..4cf7a831 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -13,6 +13,9 @@ == Unreleased changes (barbacane-dev fork) +=== Tests / docs +* 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` + https://github.com/barbacane-dev/typify/compare/v1.0.0\...HEAD[Full list of commits] == 1.0.0 (released 2026-05-13) diff --git a/typify-impl/src/convert.rs b/typify-impl/src/convert.rs index ebd2ecc2..50a9a82f 100644 --- a/typify-impl/src/convert.rs +++ b/typify-impl/src/convert.rs @@ -464,12 +464,9 @@ impl TypeSpace { extensions: _, } => self.convert_unknown_enum(type_name, original_schema, metadata, enum_values), - // Subschemas with no body validation and either no type or a + // Subschemas with no additional validation and either no type or a // single type. A multi-type (`Vec`) instance_type is deliberately - // excluded so that it flows through the merge arm below, which - // folds the type union into each subschema branch. Preserving the - // earlier behaviour for `None` / `Single` keeps existing tolerant - // handling of schemas whose outer type may conflict with a branch. + // excluded so that it flows through the merge arm below. SchemaObject { metadata, instance_type: None | Some(SingleOrVec::Single(_)), diff --git a/typify/tests/schemas/type-array-with-subschemas.json b/typify/tests/schemas/type-array-with-subschemas.json index 18f26a45..686c4876 100644 --- a/typify/tests/schemas/type-array-with-subschemas.json +++ b/typify/tests/schemas/type-array-with-subschemas.json @@ -1,9 +1,8 @@ { "$schema": "http://json-schema.org/draft-07/schema#", - "$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.", + "$comment": "Coverage for issue #954: multi-type `type: [...]` alongside oneOf/anyOf/allOf/not.", "definitions": { "TypeArrayOneOfItems": { - "$comment": "Canonical issue #954 shape. Each oneOf branch only constrains `items`, so the type union must be folded into every branch rather than dropped.", "type": [ "string", "number", "boolean", "array" ], "oneOf": [ { "items": { "type": "string" } }, @@ -12,7 +11,6 @@ ] }, "TypeArrayAnyOfItems": { - "$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.", "type": [ "string", "number", "array" ], "anyOf": [ { "items": { "type": "string" } }, @@ -20,7 +18,6 @@ ] }, "TypeArrayAllOfRefinement": { - "$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.", "type": [ "string", "array" ], "allOf": [ { "items": { "type": "string" } }, @@ -28,29 +25,11 @@ ] }, "TypeArrayNotExclusion": { - "$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.", "type": [ "string", "number", "array" ], "not": { "type": "object" } }, - "SingleTypeOneOfArrayBranch": { - "$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.", - "type": "object", - "oneOf": [ - { - "type": "object", - "properties": { "kind": { "type": "string" } }, - "required": [ "kind" ] - }, - { - "type": "array", - "items": { "type": "string" }, - "minItems": 2, - "maxItems": 2 - } - ] - }, "TypeArrayOneOfExplicitArrayBranches": { - "$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.", + "$comment": "Each oneOf branch pins `type: array`; the non-array variants from the outer union should be pruned.", "type": [ "string", "array" ], "oneOf": [ { "type": "array", "items": { "type": "string" } }, @@ -58,7 +37,7 @@ ] }, "TypeArrayPartiallyUnsatisfiableOneOf": { - "$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.", + "$comment": "Two oneOf branches conflict with the outer type union and should be dropped during merge.", "type": [ "string", "array" ], "oneOf": [ { "type": "object", "properties": { "name": { "type": "string" } } }, @@ -67,7 +46,7 @@ ] }, "TypeArrayFullyUnsatisfiableOneOf": { - "$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.", + "$comment": "Every branch conflicts with the outer type union; must resolve cleanly rather than panic.", "type": [ "string", "number" ], "oneOf": [ { "type": "array", "items": { "type": "string" } }, @@ -75,7 +54,7 @@ ] }, "TypeArrayOneOfAndAllOf": { - "$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.", + "$comment": "oneOf and allOf on the same object, plus a multi-type `type` array.", "type": [ "string", "array" ], "allOf": [ { "minLength": 1 } diff --git a/typify/tests/schemas/type-array-with-subschemas.rs b/typify/tests/schemas/type-array-with-subschemas.rs index abd17f6b..14725ea4 100644 --- a/typify/tests/schemas/type-array-with-subschemas.rs +++ b/typify/tests/schemas/type-array-with-subschemas.rs @@ -25,56 +25,12 @@ pub mod error { } } } -#[doc = "`SingleTypeOneOfArrayBranch`"] -#[doc = r""] -#[doc = r"
JSON schema"] -#[doc = r""] -#[doc = r" ```json"] -#[doc = "{"] -#[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.\","] -#[doc = " \"oneOf\": ["] -#[doc = " {"] -#[doc = " \"properties\": {"] -#[doc = " \"kind\": {"] -#[doc = " \"type\": \"string\""] -#[doc = " }"] -#[doc = " },"] -#[doc = " \"required\": ["] -#[doc = " \"kind\""] -#[doc = " ],"] -#[doc = " \"type\": \"object\""] -#[doc = " },"] -#[doc = " {"] -#[doc = " \"items\": {"] -#[doc = " \"type\": \"string\""] -#[doc = " },"] -#[doc = " \"maxItems\": 2,"] -#[doc = " \"minItems\": 2,"] -#[doc = " \"type\": \"array\""] -#[doc = " }"] -#[doc = " ],"] -#[doc = " \"type\": \"object\""] -#[doc = "}"] -#[doc = r" ```"] -#[doc = r"
"] -#[derive(:: serde :: Deserialize, :: serde :: Serialize, Clone, Debug)] -#[serde(untagged)] -pub enum SingleTypeOneOfArrayBranch { - Object { kind: ::std::string::String }, - Array([::std::string::String; 2usize]), -} -impl ::std::convert::From<[::std::string::String; 2usize]> for SingleTypeOneOfArrayBranch { - fn from(value: [::std::string::String; 2usize]) -> Self { - Self::Array(value) - } -} #[doc = "`TypeArrayAllOfRefinement`"] #[doc = r""] #[doc = r"
JSON schema"] #[doc = r""] #[doc = r" ```json"] #[doc = "{"] -#[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.\","] #[doc = " \"allOf\": ["] #[doc = " {"] #[doc = " \"items\": {"] @@ -109,7 +65,6 @@ impl ::std::convert::From<::std::vec::Vec<::std::string::String>> for TypeArrayA #[doc = r""] #[doc = r" ```json"] #[doc = "{"] -#[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.\","] #[doc = " \"anyOf\": ["] #[doc = " {"] #[doc = " \"items\": {"] @@ -154,7 +109,6 @@ impl ::std::convert::From for TypeArrayAnyOfItems { #[doc = "{"] #[doc = " \"allOf\": ["] #[doc = " {"] -#[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.\","] #[doc = " \"type\": ["] #[doc = " \"string\","] #[doc = " \"number\","] @@ -202,7 +156,6 @@ impl ::std::convert::From<::std::vec::Vec<::std::string::String>> for TypeArrayA #[doc = "{"] #[doc = " \"allOf\": ["] #[doc = " {"] -#[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.\","] #[doc = " \"type\": ["] #[doc = " \"string\","] #[doc = " \"number\","] @@ -248,7 +201,7 @@ impl ::std::convert::From<::std::vec::Vec> for TypeArrayAnyOfItemsVariant1 #[doc = r""] #[doc = r" ```json"] #[doc = "{"] -#[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.\","] +#[doc = " \"$comment\": \"Every branch conflicts with the outer type union; must resolve cleanly rather than panic.\","] #[doc = " \"oneOf\": ["] #[doc = " {"] #[doc = " \"items\": {"] @@ -292,7 +245,6 @@ pub enum TypeArrayFullyUnsatisfiableOneOf {} #[doc = r""] #[doc = r" ```json"] #[doc = "{"] -#[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.\","] #[doc = " \"not\": {"] #[doc = " \"type\": \"object\""] #[doc = " },"] @@ -327,7 +279,7 @@ impl ::std::convert::From<::std::vec::Vec<::serde_json::Value>> for TypeArrayNot #[doc = r""] #[doc = r" ```json"] #[doc = "{"] -#[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.\","] +#[doc = " \"$comment\": \"oneOf and allOf on the same object, plus a multi-type `type` array.\","] #[doc = " \"allOf\": ["] #[doc = " {"] #[doc = " \"minLength\": 1"] @@ -604,7 +556,7 @@ impl<'de> ::serde::Deserialize<'de> for TypeArrayOneOfAndAllOfVariant1String { #[doc = r""] #[doc = r" ```json"] #[doc = "{"] -#[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.\","] +#[doc = " \"$comment\": \"Each oneOf branch pins `type: array`; the non-array variants from the outer union should be pruned.\","] #[doc = " \"oneOf\": ["] #[doc = " {"] #[doc = " \"items\": {"] @@ -650,7 +602,6 @@ impl ::std::convert::From<::std::vec::Vec> for TypeArrayOneOfExplicitArrayB #[doc = r""] #[doc = r" ```json"] #[doc = "{"] -#[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.\","] #[doc = " \"oneOf\": ["] #[doc = " {"] #[doc = " \"items\": {"] @@ -707,7 +658,6 @@ impl ::std::convert::From for TypeArrayOneOfItems { #[doc = "{"] #[doc = " \"allOf\": ["] #[doc = " {"] -#[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.\","] #[doc = " \"type\": ["] #[doc = " \"string\","] #[doc = " \"number\","] @@ -769,7 +719,6 @@ impl ::std::convert::From<::std::vec::Vec<::std::string::String>> for TypeArrayO #[doc = "{"] #[doc = " \"allOf\": ["] #[doc = " {"] -#[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.\","] #[doc = " \"type\": ["] #[doc = " \"string\","] #[doc = " \"number\","] @@ -831,7 +780,6 @@ impl ::std::convert::From<::std::vec::Vec> for TypeArrayOneOfItemsVariant1 #[doc = "{"] #[doc = " \"allOf\": ["] #[doc = " {"] -#[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.\","] #[doc = " \"type\": ["] #[doc = " \"string\","] #[doc = " \"number\","] @@ -891,7 +839,7 @@ impl ::std::convert::From<::std::vec::Vec> for TypeArrayOneOfItemsVariant2 #[doc = r""] #[doc = r" ```json"] #[doc = "{"] -#[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.\","] +#[doc = " \"$comment\": \"Two oneOf branches conflict with the outer type union and should be dropped during merge.\","] #[doc = " \"oneOf\": ["] #[doc = " {"] #[doc = " \"properties\": {"]