fix(python): keep unions of named object types distinct instead of merging them#3061
Open
schani wants to merge 9 commits into
Open
fix(python): keep unions of named object types distinct instead of merging them#3061schani wants to merge 9 commits into
schani wants to merge 9 commits into
Conversation
…rging them (#1646) TypeScript/JSON Schema unions of two-or-more explicitly named, disjoint object types (e.g. `Foo | Bar`) were being collapsed by quicktype-core's union-flattening graph rewrite into a single synthetic class with all fields made optional, discarding the distinction between the original types. This happened for every target language, but is most visible in Python since it has real union types. Adds an opt-in `TargetLanguage.supportsUnionsWithMultipleObjectTypes` capability (default false, enabled for Python), threaded through `flattenUnions`/`UnifyClasses`. A new `explicit-union-member` type attribute, set by `JSONSchemaInput` on direct `$ref` alternatives of a `oneOf`/`anyOf` (TypeScript input lowers `Foo | Bar` to schema `anyOf`), lets `UnifyUnionBuilder.makeObject` keep such members distinct instead of merging them, when they are named, non-empty, closed (no additional properties), and pairwise disjoint in property names. Other languages and other union shapes (anonymous, overlapping properties, maps, etc.) are unaffected and keep the previous merge behavior. Python's renderer already handled unions of classes correctly once the type graph preserves them, so no renderer changes were needed. Test coverage: - test/inputs/schema/named-class-union.schema (+ .1.json/.2.json positive samples and a .1.fail.union.json negative sample) exercises the fix end-to-end via the schema-python fixture. - test/unit/named-class-union.test.ts asserts the merged synthetic class is no longer generated and that non-opted-in languages (TypeScript) are unchanged. Verified locally: `npm run build`, `npm run test:unit` (179/179), full `QUICKTEST=true FIXTURE=schema-python script/test` and `QUICKTEST=true FIXTURE=schema-typescript script/test` (both fully passing, no regressions), and the original TypeScript -> Python repro from the issue now produces `Union[Foo, Bar]` / `Foo | Bar` as expected. C# and other toolchains weren't available locally to spot-check; CI will validate those. Fixes #1646 Co-Authored-By: gpt-5.6-sol via pi <noreply@openai.com>
…unions into all-optional classes (#1646) elm's generated decoder accepts the negative fixture sample because named object unions merge into a class whose properties all decode via Jpipe.optional. cjson, cplusplus, kotlin, kotlin-jackson, and haskell have the same pre-existing merge-into-optional-properties behavior (already documented for other schemas in this file), so they are skipped too. Co-Authored-By: gpt-5.6-sol via pi <noreply@openai.com>
…1646) test/unit/cjson-enum-default.test.ts (added on master by #2357) asserted alphabetical enum-case order, but quicktype preserves JSON Schema declaration order for enum cases in every language (cjson, csharp, java, typescript, python, go, rust all verified), consistent with test/unit/enum-order.test.ts. The test's expectation was simply wrong; fix the expected string to match the correct declaration-order output. No renderer/core code changes. Co-Authored-By: gpt-5.6-sol via pi <noreply@openai.com>
…e-76a-70 # Conflicts: # test/languages.ts
Generated-output differences29 files differ — 1 modified, 28 new, 0 deleted |
Generated-output differences30 files differ — 1 modified, 29 new, 0 deleted |
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
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.
What was broken
TypeScript unions of named interfaces (and the equivalent JSON Schema
oneOf/anyOfof$refs) targeting Python were collapsed into a single synthetic class with all fields made optional, instead of a realUnion[...].Repro from the issue:
quicktype --src-lang typescript --lang pythonproduced:instead of the expected:
Root cause
This isn't Python-specific or TypeScript-input-specific: quicktype-core's
UnionTypemodel only ever allowed one member perkind. Any union with twoclass-kind members was therefore treated as "non-canonical" and unconditionally merged by theflattenUnionsgraph rewrite (viaUnifyUnionBuilder.makeObjectinUnifyClasses.ts), for every target language, before any language-specific renderer saw the type graph. The same merging happens today for JSON SchemaoneOfinput and for every other target language (C#, Rust, Swift, Kotlin, etc.) — Python is just where it's most visible because Python has a native union type.A fully general fix (arbitrary language, arbitrary overlap, arbitrary union arity) would require relaxing that core invariant everywhere and auditing every renderer that assumes at most one object member per union — too large and risky for this issue. This PR takes a minimal, opt-in approach instead.
The fix
TargetLanguage.supportsUnionsWithMultipleObjectTypes(defaultfalse), following the existing precedent of capability flags likesupportsFullObjectType. Enabled it for Python only.Run.tsintoflattenUnions/UnifyClasses.ts.explicit-union-membertype attribute (packages/quicktype-core/src/attributes/ExplicitUnionMember.ts), set byJSONSchemaInput.tson direct$refalternatives of aoneOf/anyOf(TypeScript input lowersFoo | Barto schemaanyOf, so this covers both entry points).UnifyUnionBuilder.makeObjectnow keeps object-kind union members distinct (instead of merging them) when the capability is on and the members are named, non-empty, closed (noadditionalProperties), and pairwise disjoint in property names — i.e. exactly the unambiguous case from the issue. Anything outside that (anonymous shapes, overlapping properties, maps/additional properties, non-opted-in languages) keeps the previous merge behavior.from_union, etc.) once the graph preserves them distinctly, so no renderer changes were needed.Test coverage
test/inputs/schema/named-class-union.schema+.1.json/.2.jsonpositive samples and a.1.fail.union.jsonnegative sample — exercised end-to-end via theschema-pythonfixture suite.test/unit/named-class-union.test.ts— asserts the merged synthetic class is no longer generated for Python (for bothoneOfand theanyOfshape TypeScript input produces), and that non-opted-in languages (TypeScript) are unaffected.Verification
npm run buildpasses.npm run test:unit— 179/179 passing.QUICKTEST=true FIXTURE=schema-python script/test— full suite passing, including the pre-existingrenaming-bug.schemafixture (a regression an earlier attempt at this fix hit there; not an issue here).QUICKTEST=true FIXTURE=schema-typescript script/test— full suite passing, confirming TypeScript output is unaffected.Container.opnow renders asFoo | Bar(orUnion[Foo, Bar]on--python-version 3.7), with correctfrom_union([Foo.from_dict, Bar.from_dict], ...)round-trip code.falsefor every language other than Python, so no other language's output should change.Fixes #1646
🤖 Generated with Claude Code