Fix crash on recursive allOf/anyOf schemas (OPDS 2.0, FHIR)

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,

packages/quicktype-core/src/Type/TypeGraph.ts

@@ -387,6 +387,7 @@ export class TypeGraph {
         map: ReadonlyMap<Type, Type>,
         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);

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<Type> {
+    const result = new Set<Type>();
+    const index = new Map<Type, number>();
+    const lowlink = new Map<Type, number>();
+    const onStack = new Set<Type>();
+    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<Type>]> = [
+            [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 | <enum>` 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<string, Type[]>();
+    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<Type, Type>();
+    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,

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,

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<Type>,
         builder: GraphRewriteBuilder<Type>,
         forwardingRef: TypeRef,
@@ -58,6 +73,38 @@ export function flattenUnions(
         );
     }
 
+    function replaceWithUnification(
+        types: ReadonlySet<Type>,
+        builder: GraphRewriteBuilder<Type>,
+        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,