Skip to content

Fix nested intersections in union flattening#2914

Closed
ankrgyl wants to merge 4 commits into
glideapps:masterfrom
braintrustdata:intersection-fix
Closed

Fix nested intersections in union flattening#2914
ankrgyl wants to merge 4 commits into
glideapps:masterfrom
braintrustdata:intersection-fix

Conversation

@ankrgyl

@ankrgyl ankrgyl commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Description

Fix union flattening when an intersection is nested inside another union.

FlattenUnions previously checked only whether the types being unified were themselves IntersectionType instances. An intersection nested inside a UnionType was therefore passed to unifyTypes, eventually causing Internal error: Unknown type intersection.

The fix recursively checks union members for intersections and uses the existing reconstitution path when one is found.

Related Issue

Fixes #2913

Previous Behaviour / Output

Returned an error

New Behaviour / Output

Generates the types

How Has This Been Tested?

Added a unit test with the repro from #2913 and ran it on the full OpenAI OpenAPI spec

@ankrgyl ankrgyl mentioned this pull request Jul 12, 2026
Open
@schani

schani commented Jul 12, 2026

Copy link
Copy Markdown
Member

Thanks for the fix! We reviewed it in depth and it looks correct: we reproduced the Internal error: Unknown type intersection crash on master with the #2913 schema shape, confirmed the branch resolves the intersection properly (merged properties, sane output), checked the recursive containsIntersection walk is cycle-safe via its seen set, and verified 10 intersection/union-heavy schema fixtures (including vega-lite) generate byte-identically to master in Rust and TypeScript — no regressions. Happy to share the full review notes.

One request before merging: we'd like the test to be a standard JSON Schema fixture rather than a Vitest unit test, so the fix is exercised across all target languages with a real round-trip plus a negative example. We've prepared that conversion on the branch fixture-test-2914 (single commit fd61626 on top of this PR's head):

Since this PR's head is on an org-owned fork, GitHub doesn't allow maintainer pushes, so to pick it up you can merge it into intersection-fix yourself:

git pull https://github.com/glideapps/quicktype.git fixture-test-2914

or cherry-pick fd616267. Verified locally: full build, unit suite green after the removal, all 22 CI target languages generate from the new schema, and end-to-end harness passes for schema-golang and schema-typescript.

🤖 Generated with Claude Code

@schani schani mentioned this pull request Jul 12, 2026
Merged
@schani

schani commented Jul 12, 2026

Copy link
Copy Markdown
Member

One-click option for the fixture conversion mentioned above: I've opened braintrustdata#1 against this PR's intersection-fix branch — merging it there updates this PR automatically. The manual git pull/cherry-pick instructions from the earlier comment remain a fine alternative.

@ankrgyl

ankrgyl commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

One-click option for the fixture conversion mentioned above: I've opened braintrustdata#1 against this PR's intersection-fix branch — merging it there updates this PR automatically. The manual git pull/cherry-pick instructions from the earlier comment remain a fine alternative.

That is very kind of you! Let me get this updated now

@schani @claude
Convert nested-intersection-union unit test to a schema fixture (#1)
a8f3bae 
Replace the Vitest unit test (which checked Rust output strings for one
language) with a standard JSON Schema fixture exercising the same shape:
an allOf nested inside a oneOf inside another oneOf, taken from the
issue glideapps#2913 repro. The fixture runs against every language in CI,
round-trips a positive instance, and expects failure on a malformed
union member for languages with the "union" feature.

Verified end-to-end locally with schema-golang and schema-typescript;
all 22 CI target languages generate successfully from the schema.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@schani schani mentioned this pull request Jul 13, 2026
Merged
@schani

schani commented Jul 13, 2026

Copy link
Copy Markdown
Member

Diagnosed the CI failure: only cjson,schema-cjson genuinely failed (the other red jobs are fail-fast cancellations). cjson's generated parser doesn't check invalid types in unions/maps/arrays — a documented limitation for which the three precedent .fail.union.json schemas are already skip-listed — so it silently accepted the new fixture's expected-failure instance. One-line fix: braintrustdata#2, same merge-to-update flow as before. Note several languages never ran the new fixture due to fail-fast, so the next full CI run is the real arbiter.

@schani @claude
cjson: skip nested-intersection-union.schema — invalid-type inputs ar…
4c54752 
…e not checked (known limitation) (#2)

cjson's generated parser does not validate invalid types in unions, maps,
and arrays (documented above this skip list), so it silently coerces the
fixture's expected-failure instance (array where an object is required)
into an empty object instead of rejecting it. Skip the schema for cjson,
exactly like the existing class-with-additional, go-schema-pattern-properties,
and multi-type-enum skips.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@schani

schani commented Jul 13, 2026

Copy link
Copy Markdown
Member

The next CI failure is the same known invalid-type limitation in the C++ renderer: it accepts the new non-object expected-failure input after the nested union is flattened to an all-optional class. Direct maintainer pushes are disabled on this org-owned fork, so I opened braintrustdata#3 with the minimal C++ skipSchema fix. Merging it updates this PR.

@ankrgyl

ankrgyl commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

The next CI failure is the same known invalid-type limitation in the C++ renderer: it accepts the new non-object expected-failure input after the nested union is flattened to an all-optional class. Direct maintainer pushes are disabled on this org-owned fork, so I opened braintrustdata#3 with the minimal C++ skipSchema fix. Merging it updates this PR.

awesome just merged that in. if i can somehow make it easier for you/us to iterate on this, let me know, happy to. eg i can try to run all the tests locally?

@schani

schani commented Jul 14, 2026

Copy link
Copy Markdown
Member

awesome just merged that in. if i can somehow make it easier for you/us to iterate on this, let me know, happy to. eg i can try to run all the tests locally?

Sorry about all this noise. quicktype's CI stops at the first failure instead of finishing all the tests, so there might still be others failing. Would you mind enabling allowing pushes from us on this PR?

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

@schani schani mentioned this pull request Jul 14, 2026
Merged
@schani

schani commented Jul 14, 2026

Copy link
Copy Markdown
Member

Fixed via #2944

@schani schani closed this Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

FlattenUnions fails when unifying an array element union that indirectly contains an intersection.

2 participants

@ankrgyl @schani