Fix crash on recursive allOf/anyOf schemas (OPDS 2.0, FHIR)#2890
Fix crash on recursive allOf/anyOf schemas (OPDS 2.0, FHIR)#2890asm0dey wants to merge 3 commits into
Conversation
Schemas with cyclic composition (e.g. OPDS 2.0 glideapps#2187, FHIR glideapps#1376) crashed with "Maximum call stack size exceeded". The union-flattening fixpoint in Run never converged: FlattenUnions built its result unions with a raw getUnionType callback that bypasses unifyTypes' in-progress memo (registerUnion), so a recursive reference back into the type being built was re-unified into a fresh copy each pass instead of tying the knot. The recursion unrolled one level per pass and the graph grew without bound. - FlattenUnions: once the graph is intersection-free, unify recursively via unionBuilderForUnification (the knot-tying path combineClasses already uses) instead of the raw callback. Gated on the absence of intersection types, whose members the unification accumulator can't handle during the intersection-resolution phase. - UnifyClasses.makeObject: unifying a class with a map of `any` (a permissive `additionalProperties: true`) now drops the un-representable additional and makes the class rather than asserting. - TypeGraphUtils.combineIdenticalTypes + Run: after flattening, merge types that are exactly bisimilar (structurallyCompatible) so recursive schemas don't leave behind piles of duplicate types. Only ever merges structurally identical types, so it never over-generalizes. Generated TypeScript output is byte-for-byte identical to before on all 131 schema fixtures and 100 JSON inputs; only the previously-crashing recursive schemas change (they now generate). This change was written with AI assistance (Claude). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JWiCcbQ7Z5uMCqtTpHZbXa
|
@asm0dey Thank you for your contribution! It's failing CI. Also I wonder (not having looked into this too much) whether this is more of a kludge than a proper fix. Can we fix the pass that screws up, instead of introducing a new pass that salvages the screw-up? Also this should have a test that would previously fail. |
|
This is indeed an interesting question. Before this, I tried to research more obvious paths — like "why can't we just cache |
|
Aha, so now it merges too aggressively, I see… |
|
Don't run it yet; I'm trying to experiment here |
…utes Two follow-up fixes to combineIdenticalTypes so the recursive-schema flattening passes CI. Over-merge: candidates were selected with structurallyCompatible, which compares kinds and structure but ignores type attributes, so two types that render differently could look "compatible" — a plain string and an enum-bearing string are both kind `string`, making a `null | string` union compatible with a `null | <enum>` union. Merging those collapsed a free-form field onto the enum (on us-senators, twitterid / youtubeid / leadership_title rendered as the Party enum when generated via a JSON Schema, diverging from the direct-from-JSON output and failing the schema-roundtrip fixture for every target language). The identity map in TypeBuilder already deduplicates attribute-equal non-recursive duplicates at creation time; the only duplicates it misses — and the only ones this pass needs to remove — are recursive ones. Restrict merging to types that lie on a cycle, found via an iterative Tarjan SCC pass. This still collapses the recursive duplicates the fixpoint leaves behind while leaving attribute-distinct non-recursive types alone. Lost attributes: merging a recursive type orphans its (structurally identical but distinct) sub-types, so their attributes — including the provenance the graph's invariant check tracks — are intentionally dropped. TypeGraph.remap takes a lostTypeAttributes flag so combineIdenticalTypes can signal this, the same way other knowingly-lossy rewrites call setLostTypeAttributes; without it the provenance check failed under `--debug provenance`, which all schema/json fixtures run with. Verified against the schema/JSON fixtures with provenance checking, plus the typescript, golang, rust and kotlin fixture suites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JWiCcbQ7Z5uMCqtTpHZbXa
926c088 to
f37c5ce
Compare
|
Pushed a follow-up commit fixing two issues that were failing CI, both in the new 1. Provenance invariant failure (all schema/JSON jobs)All schema/JSON fixtures run with 2. Attribute-blind over-merge (schema-roundtrip, every language)Candidates were selected with - "twitterid": S.NullOr(S.String),
- "youtubeid": S.NullOr(S.String),
+ "twitterid": S.NullOr(Party),
+ "youtubeid": S.NullOr(Party),The identity map in VerificationEmulated the CI matrix locally on every toolchain available on my machine:
🤖 Generated with Claude Code |
|
Now it looks more correct and passes someci-like pipelines on my machine, but still can't remove replaceWithUnification — but claude had to add a proper cycle detection too to not overmerge |
|
@asm0dey Do you have a small repro for this? I'd like to try if I can maybe find a simpler fix. |
|
Coming up with a small repro has proven to be extremely difficult - trying to minimize the example from #1376 rn |
Schemas with cyclic allOf/anyOf composition used to make the union- flattening fixpoint diverge and crash with "Maximum call stack size exceeded" (FHIR glideapps#1376, OPDS 2.0 glideapps#2187). Nothing in the suite guarded it: the per-language schema fixtures must also compile and round-trip, but a schema gnarly enough to trigger the non-convergence generates recursive maps/unions that not every target language compiles. Add a startup check (like the existing check-*.ts regression guards) that only asserts quicktype *generates* from such a schema — it never compiles the output — so it can use a schema that reproduces the crash directly. The input is a self-contained bundle of the OPDS 2.0 feed schema (all external $refs inlined) under test/inputs/regressions/, which is not one of the auto-discovered fixture directories. Verified it crashes without the flattening fix and passes with it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JWiCcbQ7Z5uMCqtTpHZbXa
|
So, I've uploaded a slightly simplified version of the OPDS feed schema — without external dependencies. But I couldn't really minimize it, sadly :( It seems there should be more or less complex cross-dependencies. |
|
I was able to find a much simpler repro: {
"items": {
"anyOf": [
{ "$ref": "#/definitions/a" },
{ "additionalProperties": { "$ref": "#" } }
]
},
"definitions": {
"a": {
"properties": {
"x": { "items": { "$ref": "#/definitions/a" } }
}
}
}
}Not that it makes much sense, but it does crash quicktype. Will try to work with that. |
|
Explainer of what's happening: https://hostr.flingit.run/s/schani/quicktype-unroll-explainer/ |
|
Is it fixed with this fix? |
|
This seems a better fix: #2891 Closing this in favor of that. |
|
Your fix is so much more elegant! |
Description
The union-flattening fixpoint (
Run.ts) never converged on JSON Schemas with cyclic composition (recursiveallOf/anyOf), causingError: Maximum call stack size exceeded.FlattenUnions.replacebuilt its result unions with a rawgetUnionTypecallback that bypassesunifyTypes' in-progress memoization (registerUnion). As a result, a recursive reference back into the type currently being built was re-unified into a fresh copy on every pass instead of tying the knot. The recursion unrolled one level per pass, the type graph grew without bound, and the reconstitution eventually overflowed the stack.Changes (
quicktype-core):rewrites/FlattenUnions.ts— once the graph is intersection-free, unify recursively viaunionBuilderForUnification(the same knot-tying pathcombineClassesalready uses) instead of the raw callback. Gated on the absence ofintersectiontypes, whose members the unification accumulator can't process during the intersection/union resolution phase.UnifyClasses.ts(makeObject) — unifying a class with a map ofany(a permissiveadditionalProperties: true) now drops the un-representable additional and produces the class, rather than assertingWe have additional properties but want to make a class. A non-any additional would already have taken the map branch, so only permissiveanyadditionals reach this point.Type/TypeGraphUtils.ts(combineIdenticalTypes) +Run.ts— after each flatten pass, merge types that are exactly bisimilar (structurallyCompatible, no number conflation) so recursive schemas don't leave behind piles of duplicate types. It only ever merges structurally identical types, so it never over-generalizes.Related Issue
Fixes #2187 (OPDS 2.0 feed schema). Also fixes the same class of failure in #1376 (FHIR schema).
Motivation and Context
quicktype crashed outright — not just wrong output — on real-world, non-pathological schemas that use recursive composition.
app.quicktype.ioand the CLI both failed on the OPDS 2.0 feed schema for every target language, and the FHIR schema in #1376 (open since 2020) fails identically. Both share one root cause: the flatten fixpoint re-derives recursive merged types instead of tying the recursive knot.Previous Behaviour / Output
(Same for
#1376'sfhir.schema.jsonin any language.)New Behaviour / Output
Both schemas now generate finite, sensible types in every target language:
#1376, 3.4 MB): 377 Rust structs.How Has This Been Tested?
master(68ecb28…) and confirmed both now generate with this change.test/inputs/schema/*.jsonfixtures and 100test/inputs/jsoninputs, before vs after — output is byte-for-byte identical. The only behavioural change is that previously-crashing recursive schemas now succeed.mutually-recursive) unchanged.I couldn't run the full
test/test.tscompile-and-round-trip harness locally (needs every language toolchain). Happy to add a fixture (I have a 46 KB self-contained OPDS repro and the FHIR file) if a maintainer can validate the round-trip in CI.