fix: address upstream review feedback on multi-type union fix#7
Merged
Conversation
Upstream PR oxidecomputer#1001 (Nicolas's re-submission of oxidecomputer#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Upstream PR oxidecomputer/typify#1001 (Nicolas's re-submission of oxidecomputer#1000 against upstream `main`, carrying the same fix already shipped in v1.0.0 as commit `91580aa`) received `CHANGES_REQUESTED` from @ahl on 2026-04-24. We merged our equivalent commit before noticing the review, so this PR applies the same edits to the fork copy.
The match arm itself (`None | Some(SingleOrVec::Single(_))`) is unchanged — the actual oxidecomputer#954 fix is preserved.
Changes
Drop the `SingleTypeOneOfArrayBranch` test case. The schema `{ type: "object", oneOf: [{...object}, {...array}] }` is contradictory: outer `type: object` makes the array branch unsatisfiable, yet our codegen still emits the unreachable `Array([String; 2])` variant. ahl flagged this as the test case generating the wrong type. The case was added as a "regression guard" for the pre-fix tolerant-but-wrong handling — keeping it would assert that the wrong output stays wrong.
Trim `$comment` rationales in `type-array-with-subschemas.json`. The originals read like LLM-generated justifications (ahl: "are these LLM-written?"). Surviving comments are one line each and only retained where the case isn't self-explanatory.
Shorten the match-arm comment in `convert.rs` per ahl's inline suggestion. Same intent, less narration.
Follow-up not in this PR
The underlying buggy behaviour — singleton outer `type` + conflicting subschema branch type isn't pruned — remains. Fixing it (filter out unsatisfiable branches before generating variants) is a real codegen change with broader fixture churn. Tracked as a follow-up; would land in 1.1.0.
Test plan