diff --git a/packages/quicktype-core/src/Run.ts b/packages/quicktype-core/src/Run.ts index 85a48c848f..4b41aa02a0 100644 --- a/packages/quicktype-core/src/Run.ts +++ b/packages/quicktype-core/src/Run.ts @@ -35,6 +35,7 @@ import { TypeBuilder } from "./Type/TypeBuilder"; import type { StringTypeMapping } from "./Type/TypeBuilderUtils"; import { TypeGraph } from "./Type/TypeGraph"; import { + combineIdenticalTypes, noneToAny, optionalToNullable, removeIndirectionIntersections, @@ -331,6 +332,22 @@ class Run implements RunContext { }); } + // Merge structurally-identical types (including recursive ones) + // that the flattening fixpoint leaves behind, so recursive + // schemas don't produce piles of duplicate types (issue #2187). + this.time("combine identical types", () => { + const combined = combineIdenticalTypes( + graph, + stringTypeMapping, + debugPrintReconstitution, + ); + if (combined !== graph) { + graph = combined; + // Merging may expose further flattening opportunities. + unionsDone = false; + } + }); + if (graph === graphBeforeRewrites) { assert( intersectionsDone && unionsDone, diff --git a/packages/quicktype-core/src/Type/TypeGraph.ts b/packages/quicktype-core/src/Type/TypeGraph.ts index f20f3a209f..1fde27f6a5 100644 --- a/packages/quicktype-core/src/Type/TypeGraph.ts +++ b/packages/quicktype-core/src/Type/TypeGraph.ts @@ -387,6 +387,7 @@ export class TypeGraph { map: ReadonlyMap, debugPrintRemapping: boolean, force = false, + lostTypeAttributes = false, ): TypeGraph { this.printRewrite(title); @@ -407,6 +408,13 @@ export class TypeGraph { this.serial + 1, this._haveProvenanceAttributes, ); + // Merging distinct-but-structurally-identical types orphans their + // sub-types, dropping those attributes; callers that do so set this so + // the provenance invariant check treats the loss as intentional. + if (lostTypeAttributes) { + builder.setLostTypeAttributes(); + } + const newGraph = builder.finish(); this.checkLostTypeAttributes(builder, newGraph); diff --git a/packages/quicktype-core/src/Type/TypeGraphUtils.ts b/packages/quicktype-core/src/Type/TypeGraphUtils.ts index cecc04fb9b..69bb2ef5f6 100644 --- a/packages/quicktype-core/src/Type/TypeGraphUtils.ts +++ b/packages/quicktype-core/src/Type/TypeGraphUtils.ts @@ -10,7 +10,13 @@ import { TypeNames, namesTypeAttributeKind } from "../attributes/TypeNames"; import type { GraphRewriteBuilder } from "../GraphRewriting"; import { assert, defined, panic } from "../support/Support"; -import { ClassType, IntersectionType, type Type, UnionType } from "./Type"; +import { + ClassType, + IntersectionType, + ObjectType, + type Type, + UnionType, +} from "./Type"; import type { StringTypeMapping } from "./TypeBuilderUtils"; import type { TypeGraph } from "./TypeGraph"; import type { TypeRef } from "./TypeRef"; @@ -127,6 +133,199 @@ export function optionalToNullable( ); } +// The set of types that lie on a cycle, i.e. are reachable from themselves +// through their (non-attribute) children. Computed with an iterative Tarjan +// SCC pass — a type is recursive iff its strongly-connected component has more +// than one member or it has an edge to itself. Iterative (not recursive) so it +// can't itself stack-overflow on the deep graphs this module handles. +function recursiveTypes(graph: TypeGraph): Set { + const result = new Set(); + const index = new Map(); + const lowlink = new Map(); + const onStack = new Set(); + const stack: Type[] = []; + let counter = 0; + + for (const root of graph.allTypesUnordered()) { + if (index.has(root)) continue; + // Explicit work stack of (type, iterator over its children). + const work: Array<[Type, Iterator]> = [ + [root, root.getNonAttributeChildren()[Symbol.iterator]()], + ]; + index.set(root, counter); + lowlink.set(root, counter); + counter += 1; + stack.push(root); + onStack.add(root); + + while (work.length > 0) { + const [t, it] = work[work.length - 1]; + const next = it.next(); + if (!next.done) { + const child = next.value; + if (t === child) result.add(t); // self-edge + if (!index.has(child)) { + index.set(child, counter); + lowlink.set(child, counter); + counter += 1; + stack.push(child); + onStack.add(child); + work.push([ + child, + child.getNonAttributeChildren()[Symbol.iterator](), + ]); + } else if (onStack.has(child)) { + lowlink.set( + t, + Math.min( + defined(lowlink.get(t)), + defined(index.get(child)), + ), + ); + } + + continue; + } + + // Done with t's children: pop it and propagate its lowlink up. + work.pop(); + if (work.length > 0) { + const parent = work[work.length - 1][0]; + lowlink.set( + parent, + Math.min( + defined(lowlink.get(parent)), + defined(lowlink.get(t)), + ), + ); + } + + // t is an SCC root: pop its component. Any component with >1 member + // is a cycle, so all its members are recursive. + if (defined(lowlink.get(t)) === defined(index.get(t))) { + const component: Type[] = []; + let popped: Type; + do { + popped = defined(stack.pop()); + onStack.delete(popped); + component.push(popped); + } while (popped !== t); + if (component.length > 1) { + for (const c of component) result.add(c); + } + } + } + } + + return result; +} + +// Collapse types that are structurally identical to each other into a single +// type. Unlike the identity-map deduplication in `TypeBuilder`, which can only +// deduplicate a type at the moment it is created (and so misses *recursive* +// types, whose structure isn't known until after they've been given a type +// ref), this compares whole types — cycles and all — via +// `structurallyCompatible`. It only ever merges types that are exactly +// bisimilar, so it never over-generalizes; it just removes the duplicate +// recursive types the flattening fixpoint leaves behind on schemas with cyclic +// composition (issues #2187 and #1376). +export function combineIdenticalTypes( + graph: TypeGraph, + stringTypeMapping: StringTypeMapping, + debugPrintRemapping: boolean, +): TypeGraph { + // Only structural (composite) types can be duplicated in a way the identity + // map misses; primitives/enums/strings already deduplicate on creation. + const dedupableKinds = new Set([ + "class", + "object", + "map", + "array", + "union", + "intersection", + ]); + + // Bucket by a cheap structural signature so we only run the (relatively + // expensive) structural-equality check between plausible candidates. + function signature(t: Type): string { + if (t instanceof ClassType || t instanceof ObjectType) { + const props = Array.from(t.getProperties().keys()).sort().join(","); + const additional = t.getAdditionalProperties() !== undefined; + return `${t.kind}|${props}|${additional}`; + } + + if (t instanceof UnionType || t instanceof IntersectionType) { + const memberKinds = Array.from(t.members) + .map((m) => m.kind) + .sort() + .join(","); + return `${t.kind}|${t.members.size}|${memberKinds}`; + } + + return t.kind; + } + + // Only merge types that participate in a cycle. `structurallyCompatible` + // ignores type attributes, so two structurally-identical types can still + // render differently — e.g. a plain `string` and an enum-bearing `string` + // both have kind `string`, so a `null | string` union is "compatible" with + // a `null | ` union and merging the two would wrongly constrain the + // free-form field to the enum's cases (us-senators.json). The identity map + // in `TypeBuilder` already deduplicates attribute-equal *non-recursive* + // duplicates safely; the only duplicates it misses — and the only ones this + // pass exists to remove — are recursive ones, whose structure isn't known at + // creation time. Restricting to cycle members avoids the attribute-blind + // over-merge while still collapsing the recursive duplicates (#2187, #1376). + const recursive = recursiveTypes(graph); + const buckets = new Map(); + for (const t of graph.allTypesUnordered()) { + if (!dedupableKinds.has(t.kind)) continue; + if (!recursive.has(t)) continue; + const sig = signature(t); + let bucket = buckets.get(sig); + if (bucket === undefined) { + bucket = []; + buckets.set(sig, bucket); + } + + bucket.push(t); + } + + const map = new Map(); + for (const bucket of buckets.values()) { + if (bucket.length < 2) continue; + // Deterministic representative order: lowest index first. + bucket.sort((a, b) => a.index - b.index); + const representatives: Type[] = []; + for (const t of bucket) { + const rep = representatives.find((r) => + r.structurallyCompatible(t, false), + ); + if (rep === undefined) { + representatives.push(t); + } else { + map.set(t, rep); + } + } + } + + // Merging two structurally-identical types orphans the (structurally + // identical but distinct) sub-types of the mapped-away type, so their + // attributes — including the provenance the invariant check tracks — are + // intentionally dropped. This is inherent to deduplicating recursive types, + // whose duplicate copies are entirely separate subtrees. Signal the loss so + // the provenance check doesn't flag it, as other knowingly-lossy rewrites do. + return graph.remap( + "combine identical types", + stringTypeMapping, + false, + map, + debugPrintRemapping, + false, + true, + ); +} + export function removeIndirectionIntersections( graph: TypeGraph, stringTypeMapping: StringTypeMapping, diff --git a/packages/quicktype-core/src/UnifyClasses.ts b/packages/quicktype-core/src/UnifyClasses.ts index 63deb15a62..f619888ad9 100644 --- a/packages/quicktype-core/src/UnifyClasses.ts +++ b/packages/quicktype-core/src/UnifyClasses.ts @@ -213,10 +213,13 @@ export class UnifyUnionBuilder extends UnionBuilder< forwardingRef, ); } else { - assert( - additionalProperties === undefined, - "We have additional properties but want to make a class", - ); + // A class can't represent additional properties. A *non-any* + // additionalProperties would have taken the map branch above, + // so anything reaching here is a permissive `any` + // additionalProperties (e.g. `additionalProperties: true` + // unified with a class). Drop it and make a class — consistent + // with how quicktype otherwise ignores permissive `any` + // additionals for languages without a full object type. return this.typeBuilder.getUniqueClassType( typeAttributes, this._makeClassesFixed, diff --git a/packages/quicktype-core/src/rewrites/FlattenUnions.ts b/packages/quicktype-core/src/rewrites/FlattenUnions.ts index 73e0df28ad..64c7750c68 100644 --- a/packages/quicktype-core/src/rewrites/FlattenUnions.ts +++ b/packages/quicktype-core/src/rewrites/FlattenUnions.ts @@ -9,7 +9,11 @@ import type { StringTypeMapping } from "../Type/TypeBuilderUtils"; import type { TypeGraph } from "../Type/TypeGraph"; import { type TypeRef, derefTypeRef } from "../Type/TypeRef"; import { makeGroupsToFlatten } from "../Type/TypeUtils"; -import { UnifyUnionBuilder, unifyTypes } from "../UnifyClasses"; +import { + UnifyUnionBuilder, + unifyTypes, + unionBuilderForUnification, +} from "../UnifyClasses"; export function flattenUnions( graph: TypeGraph, @@ -20,7 +24,18 @@ export function flattenUnions( ): [TypeGraph, boolean] { let needsRepeat = false; - function replace( + // While intersections are still being resolved (the intersection/union + // resolution passes alternate), the graph can contain `intersection` types + // that the unification accumulator can't handle. In that phase we use the + // simple callback below. Once the graph is intersection-free we can unify + // recursively (see `replaceWithUnification`), which is what makes recursive + // schemas converge. + const graphHasIntersections = iterableSome( + graph.allTypesUnordered(), + (t) => t instanceof IntersectionType, + ); + + function replaceSimple( types: ReadonlySet, builder: GraphRewriteBuilder, forwardingRef: TypeRef, @@ -58,6 +73,38 @@ export function flattenUnions( ); } + function replaceWithUnification( + types: ReadonlySet, + builder: GraphRewriteBuilder, + forwardingRef: TypeRef, + ): TypeRef { + // Unify recursively through `unifyTypes` (via `unionBuilderForUnification`) + // rather than building result unions with a raw `getUnionType` callback. + // The recursive path memoizes in-progress unifications (`registerUnion`), + // so a recursive reference back into the type currently being built ties + // the knot instead of being re-unified into a fresh copy. Without this the + // recursion unrolls one level per pass and the fixpoint never converges on + // schemas with cyclic composition (issue #2187, same class as #1376). + const unionBuilder = unionBuilderForUnification( + builder, + makeObjectTypes, + true, + conflateNumbers, + ); + return unifyTypes( + types, + emptyTypeAttributes, + builder, + unionBuilder, + conflateNumbers, + forwardingRef, + ); + } + + const replace = graphHasIntersections + ? replaceSimple + : replaceWithUnification; + const allUnions = setFilter( graph.allTypesUnordered(), (t) => t instanceof UnionType, diff --git a/test/check-recursive-schema.ts b/test/check-recursive-schema.ts new file mode 100644 index 0000000000..32eff78e00 --- /dev/null +++ b/test/check-recursive-schema.ts @@ -0,0 +1,57 @@ +// Guard: recursively-composed JSON Schemas must not blow the stack. +// +// Schemas with cyclic composition — a type that refers back to itself through +// `allOf`/`anyOf`/`oneOf` — used to make the union-flattening fixpoint diverge, +// crashing with "Maximum call stack size exceeded". Real-world schemas that hit +// this include the FHIR schema (issue #1376) and OPDS 2.0 (issue #2187). +// +// The fixture harness can't guard this: its schema fixtures must also compile +// and round-trip in every target language, and a schema big/gnarly enough to +// trigger the non-convergence produces output that not every language compiles. +// This check only asserts that quicktype *generates* — it never has to compile +// the result — so it can use a schema that reproduces the crash directly. + +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; + +import { main as quicktype } from "../src"; + +const schemaPath = path.join( + __dirname, + "inputs", + "regressions", + "recursive-composition.schema", +); + +export async function checkRecursiveSchemaConverges(): Promise { + const outDir = fs.mkdtempSync(path.join(os.tmpdir(), "quicktype-recursive-")); + const outPath = path.join(outDir, "out.ts"); + try { + await quicktype({ + srcLang: "schema", + src: [schemaPath], + lang: "typescript", + out: outPath, + quiet: true, + telemetry: "disable", + }); + } catch (e) { + console.error( + `error: quicktype failed on a recursively-composed schema (regression of #1376 / #2187):\n${ + e instanceof Error ? e.stack ?? e.message : String(e) + }`, + ); + process.exit(1); + } finally { + fs.rmSync(outDir, { recursive: true, force: true }); + } +} + +// Allow running the check standalone: +// NODE_PATH=`pwd`/node_modules npx ts-node --project test/tsconfig.json test/check-recursive-schema.ts +if (require.main === module) { + checkRecursiveSchemaConverges().then(() => { + console.error("* recursively-composed schema converged"); + }); +} diff --git a/test/inputs/regressions/recursive-composition.schema b/test/inputs/regressions/recursive-composition.schema new file mode 100644 index 0000000000..7e6b7fb0cb --- /dev/null +++ b/test/inputs/regressions/recursive-composition.schema @@ -0,0 +1,2051 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "$ref": "#/definitions/d0_feed", + "definitions": { + "d0_feed": { + "title": "OPDS Feed", + "type": "object", + "properties": { + "metadata": { + "description": "Contains feed-level metadata such as title or number of items", + "$ref": "#/definitions/d1_feed_metadata" + }, + "links": { + "description": "Feed-level links such as search or pagination", + "type": "array", + "items": { + "$ref": "#/definitions/d2_link" + }, + "uniqueItems": true, + "minItems": 1, + "contains": { + "properties": { + "rel": { + "anyOf": [ + { + "type": "string", + "const": "self" + }, + { + "type": "array", + "contains": { + "const": "self" + } + } + ] + } + }, + "required": [ + "rel" + ] + } + }, + "publications": { + "description": "A list of publications that can be acquired", + "type": "array", + "items": { + "$ref": "#/definitions/d3_publication" + }, + "uniqueItems": true, + "minItems": 1 + }, + "navigation": { + "description": "Navigation for the catalog using links", + "type": "array", + "items": { + "$ref": "#/definitions/d2_link" + }, + "uniqueItems": true, + "minItems": 1, + "allOf": [ + { + "description": "Each Link Object in a navigation collection must contain a title", + "items": { + "required": [ + "title" + ] + } + } + ] + }, + "facets": { + "description": "Facets are meant to re-order or obtain a subset for the current list of publications", + "type": "array", + "items": { + "type": "object", + "properties": { + "metadata": { + "$ref": "#/definitions/d1_feed_metadata" + }, + "links": { + "type": "array", + "items": { + "$ref": "#/definitions/d2_link" + }, + "uniqueItems": true, + "minItems": 1 + } + } + }, + "uniqueItems": true, + "minItems": 1 + }, + "groups": { + "description": "Groups provide a curated experience, grouping publications or navigation links together", + "type": "array", + "items": { + "type": "object", + "properties": { + "metadata": { + "$ref": "#/definitions/d1_feed_metadata" + }, + "links": { + "type": "array", + "items": { + "$ref": "#/definitions/d2_link" + }, + "uniqueItems": true, + "minItems": 1 + }, + "publications": { + "type": "array", + "items": { + "$ref": "#/definitions/d3_publication" + }, + "uniqueItems": true, + "minItems": 1 + }, + "navigation": { + "type": "array", + "items": { + "$ref": "#/definitions/d2_link" + }, + "uniqueItems": true, + "minItems": 1 + } + }, + "required": [ + "metadata" + ] + } + } + }, + "required": [ + "metadata", + "links" + ], + "additionalProperties": { + "$ref": "#/definitions/d4_subcollection" + }, + "anyOf": [ + { + "required": [ + "publications" + ] + }, + { + "required": [ + "navigation" + ] + }, + { + "required": [ + "groups" + ] + } + ] + }, + "d4_subcollection": { + "title": "Core Collection Model", + "anyOf": [ + { + "type": "object", + "properties": { + "metadata": { + "type": "object" + }, + "links": { + "type": "array", + "items": { + "$ref": "#/definitions/d2_link" + } + }, + "additionalProperties": { + "$ref": "#/definitions/d4_subcollection" + } + }, + "required": [ + "metadata", + "links" + ] + }, + { + "type": "array", + "items": { + "anyOf": [ + { + "$ref": "#/definitions/d2_link" + }, + { + "type": "object", + "properties": { + "metadata": { + "type": "object" + }, + "links": { + "type": "array", + "items": { + "$ref": "#/definitions/d2_link" + } + } + }, + "additionalProperties": { + "$ref": "#/definitions/d4_subcollection" + } + } + ] + } + } + ] + }, + "d2_link": { + "title": "Link Object for the Readium Web Publication Manifest", + "type": "object", + "properties": { + "href": { + "description": "URI or URI template of the linked resource", + "type": "string" + }, + "type": { + "description": "MIME type of the linked resource", + "type": "string" + }, + "templated": { + "description": "Indicates that a URI template is used in href", + "type": "boolean" + }, + "title": { + "description": "Title of the linked resource", + "type": "string" + }, + "rel": { + "description": "Relation between the linked resource and its containing collection", + "type": [ + "string", + "array" + ], + "items": { + "type": "string" + } + }, + "properties": { + "description": "Properties associated to the linked resource", + "type": "object", + "properties": { + "page": { + "description": "Indicates how the linked resource should be displayed in a reading environment that displays synthetic spreads.", + "type": "string", + "enum": [ + "left", + "right", + "center" + ] + } + }, + "allOf": [ + { + "$ref": "#/definitions/d5_properties" + }, + { + "$ref": "#/definitions/d6_properties" + }, + { + "$ref": "#/definitions/d7_properties" + } + ] + }, + "height": { + "description": "Height of the linked resource in pixels", + "type": "integer", + "exclusiveMinimum": 0 + }, + "width": { + "description": "Width of the linked resource in pixels", + "type": "integer", + "exclusiveMinimum": 0 + }, + "size": { + "description": "Original size of the resource in bytes, prior to any use of encryption or compression in an archive", + "type": "integer", + "exclusiveMinimum": 0 + }, + "bitrate": { + "description": "Bitrate of the linked resource in kbps", + "type": "number", + "exclusiveMinimum": 0 + }, + "duration": { + "description": "Length of the linked resource in seconds", + "type": "number", + "exclusiveMinimum": 0 + }, + "language": { + "description": "Expected language of the linked resource", + "type": [ + "string", + "array" + ], + "items": { + "type": "string", + "pattern": "^((?(en-GB-oed|i-ami|i-bnn|i-default|i-enochian|i-hak|i-klingon|i-lux|i-mingo|i-navajo|i-pwn|i-tao|i-tay|i-tsu|sgn-BE-FR|sgn-BE-NL|sgn-CH-DE)|(art-lojban|cel-gaulish|no-bok|no-nyn|zh-guoyu|zh-hakka|zh-min|zh-min-nan|zh-xiang))|((?([A-Za-z]{2,3}(-(?[A-Za-z]{3}(-[A-Za-z]{3}){0,2}))?)|[A-Za-z]{4}|[A-Za-z]{5,8})(-(?