From e496bdcda74a6f9ce0718033e99bf308064476ac Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 01:49:31 +0000 Subject: [PATCH] fix(types): memoise the two zod lazy getters that can be, pin why the other eight cannot (objectui#7918) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ten `z.lazy` exports of the zod node face rebuild their schema on every getter call, so `S._zod.def.getter() !== S._zod.def.getter()`. The card did not claim that was wrong — it asked whether the spelling was dodging a temporal dead zone, and that check is what this commit carries. Each of the ten was rewritten in place to `const inner = ; z.lazy(() => inner)`, the package rebuilt, and the built barrel imported in a fresh process. Eight refuse to load: seven name the very const being declared, and `SchemaNodeSchema` names `BaseSchemaCore`, which `base.zod.ts` declares below it. Their `z.lazy` is load-bearing and they keep the spelling they have. The two that loaded clean are memoised here, using the shape the face's other `z.lazy` sites already use — a getter returning a module-level constant: FilterBuilderConditionSchema not recursive at all NavigationItemSchema self-reference already deferred by the inner `z.lazy(() => NavigationItemSchema)` on `children` Two corrections to the finding came out of the check, both measured: - The recursion point was already identity-comparable through the right handle. zod 4.4.3 caches a lazy's inner type on `def._cachedInner` to preserve "identity for cycle detection on recursive schemas", and `_zod.innerType` reads that cache — stable for all ten, including the eight. What is unstable is the public `.unwrap()`, which `ZodLazy` defines as `() => _zod.def.getter()`, going around the cache. Memoising is still worth doing where it is free, because it makes `.unwrap()` honest. - The "rebuilt on every parse" cost does not exist. The getter runs once per lazy for the life of the process: one call during the first parse of a 13-node document, zero during the second. Wall clock agrees — 0.94x with overlapping ranges over nine trials of 200 parses. No accept/reject behaviour moves; a memoised getter changes schema identity, not what is declared or admitted. The measurement, the eight ReferenceError messages, the identity matrix and executable reproductions of both the TDZ mechanism and the once-per-process getter are pinned in `zod-lazy-getter-identity-7918.test.ts`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0114Ytxr5sM1vdW19Y9WAx6E --- .changeset/7918-zod-lazy-getter-identity.md | 54 ++++ .../zod-lazy-getter-identity-7918.test.ts | 279 ++++++++++++++++++ packages/types/src/zod/app.zod.ts | 23 +- packages/types/src/zod/complex.zod.ts | 29 +- 4 files changed, 376 insertions(+), 9 deletions(-) create mode 100644 .changeset/7918-zod-lazy-getter-identity.md create mode 100644 packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts diff --git a/.changeset/7918-zod-lazy-getter-identity.md b/.changeset/7918-zod-lazy-getter-identity.md new file mode 100644 index 0000000000..aaba556a64 --- /dev/null +++ b/.changeset/7918-zod-lazy-getter-identity.md @@ -0,0 +1,54 @@ +--- +'@object-ui/types': patch +--- + +Two of the ten `z.lazy` exports in the zod node face now memoise their getter, so their +public `.unwrap()` compares by identity (objectui#7918). The other eight are **deliberately +unchanged** — measured, memoising them is a module-load `ReferenceError`. + +The card that found this did not claim the ten were wrong. It asked whether the spelling was +buying a temporal-dead-zone dodge, and that check is what shipped. Each of the ten was +rewritten in place to `const inner = ; z.lazy(() => inner)`, the package rebuilt, and +the built barrel imported in a fresh process. **Eight refuse to load.** Seven name the very +const being declared (`children: z.array(TreeNodeSchema)` sits inside `TreeNodeSchema`'s own +initialiser); `SchemaNodeSchema` names `BaseSchemaCore`, which `base.zod.ts` declares below +it. For those eight the `z.lazy` is load-bearing, so `ActionSchema`, `AppMenuItemSchema`, +`FilterGroupSchema`, `MenuItemSchema`, `NavLinkSchema`, `NavigationMenuItemSchema`, +`SchemaNodeSchema` and `TreeNodeSchema` keep the spelling they have. The two that loaded +clean are memoised: `FilterBuilderConditionSchema` is not recursive at all, and +`NavigationItemSchema` already defers its self-reference through an inner +`z.lazy(() => NavigationItemSchema)` on `children`. + +⚠️ Two corrections to the finding, both measured, both worth more than the edit: + +**The recursion point was already identity-comparable, through the right handle.** +`zod@4.4.3` caches a lazy's resolved inner type on `def._cachedInner` — its own comment says +this preserves "identity for cycle detection on recursive schemas" — and `S._zod.innerType` +reads that cache. It is stable for all ten, including the eight, and survives `.describe()` +clones. What is *not* stable is `S.unwrap()`, because `ZodLazy` defines it as +`() => inst._zod.def.getter()`, going around the cache (`ZodPromise` spells its own as a +stored field). So a schema walker can recognise the recursion point today by reading +`_zod.innerType`; the objectui#7581 false negative — `ActionSchema` reported "not exported by +name" when it plainly is — was the wrong handle, not an unrecognisable schema. Memoising is +still worth doing where it is free, because it makes the public `.unwrap()` honest. + +**The "rebuilt on every parse" cost does not exist.** The finding recorded, explicitly +unmeasured, that a document with N nodes reconstructs the recursive sub-schema N times. It +does not: the getter runs **once per lazy for the life of the process**, via the same +`_cachedInner` — measured at one call during the first parse of a 13-node document and zero +during the second. Wall clock agrees. `NavigationItemSchema` over a 73-node document, +memoised versus not, medians of nine trials of 200 parses: 149,973 ns versus 140,293 ns per +parse — ratio 0.94x, with the ranges overlapping. Those are shared-box seconds, so the +absolutes are not idle-machine figures; the ratio is the reading, and the reading is "no +difference". There is no parse-time win here, and anyone pricing the strict face +(objectui#7935 / objectstack#5250) should strike this from the input list. + +No accept/reject behaviour moves — a memoised getter changes schema *identity*, not what is +declared or admitted. The measurement, the eight `ReferenceError` messages, the identity +matrix and an executable reproduction of both the TDZ mechanism and the once-per-process +getter are pinned in `packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts`. + +⚠️ Also settled while locating the ten: `AppMenuItemSchema` has no declaration of its own — +it is the barrel alias of `app.zod.ts`'s `MenuItemSchema`, while the barrel's own +`MenuItemSchema` is `overlay.zod.ts`'s. Two different schemas, so the list really is ten +entries and not nine. diff --git a/packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts b/packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts new file mode 100644 index 0000000000..e2a9e3b396 --- /dev/null +++ b/packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts @@ -0,0 +1,279 @@ +/** + * objectui#7918 — the `z.lazy` getter-identity ledger for the zod node face. + * + * Ten exports of `@object-ui/types/zod` are `z.lazy(() => …)` whose getter builds + * a NEW schema on every call, so `S._zod.def.getter() !== S._zod.def.getter()`. + * The card did NOT claim that was wrong — it asked whether the spelling was + * dodging a temporal-dead-zone error, and made that check the deliverable. This + * file is the answer, kept executable so it cannot rot. Three things were + * settled, and two of them contradict the card. + * + * ## 1. The TDZ question: yes for eight of the ten + * + * Each of the ten was rewritten in place to the obvious memoisation + * — `const inner = ; z.lazy(() => inner)` — the package rebuilt, and the + * built barrel imported in a fresh process. EIGHT refuse to load: + * + * ActionSchema ReferenceError: Cannot access 'ActionSchema' before initialization + * AppMenuItemSchema ReferenceError: Cannot access 'MenuItemSchema' before initialization + * FilterGroupSchema ReferenceError: Cannot access 'FilterGroupSchema' before initialization + * MenuItemSchema ReferenceError: Cannot access 'MenuItemSchema' before initialization + * NavLinkSchema ReferenceError: Cannot access 'NavLinkSchema' before initialization + * NavigationMenuItemSchema ReferenceError: Cannot access 'NavigationMenuItemSchema' before initialization + * SchemaNodeSchema ReferenceError: Cannot access 'BaseSchemaCore' before initialization + * TreeNodeSchema ReferenceError: Cannot access 'TreeNodeSchema' before initialization + * + * Seven name the very const being declared (`children: z.array(TreeNodeSchema)` + * sits inside `TreeNodeSchema`'s own initialiser); `SchemaNodeSchema` names + * `BaseSchemaCore`, which `base.zod.ts` declares BELOW it. For those eight the + * `z.lazy` is LOAD-BEARING — it is buying a TDZ dodge, not a style — and they + * keep the spelling they have. `mechanism` below reproduces the failure. + * + * The two that loaded clean were memoised: `FilterBuilderConditionSchema` is not + * recursive at all, and `NavigationItemSchema` already defers its self-reference + * through the inner `z.lazy(() => NavigationItemSchema)` on `children`. + * + * ## 2. The card's consequence ① is real, but only for the PUBLIC handle + * + * `zod@4.4.3` caches a lazy's resolved inner type on the shared def + * (`$ZodLazy` -> `util.defineLazy(inst._zod, 'innerType', …)`, which fills + * `def._cachedInner` once). Its own comment says the cache exists to preserve + * "identity for cycle detection on recursive schemas". So: + * + * S._zod.innerType STABLE for all ten, even the eight (internal) + * S.unwrap() fresh object per call for the eight (PUBLIC) + * S._zod.def.getter() fresh object per call for the eight (internal) + * + * `.unwrap()` is unstable because `ZodLazy` defines it as + * `() => inst._zod.def.getter()` — it goes around the cache. (`ZodPromise` + * spells its own as `() => inst._zod.def.innerType`, i.e. a stored field.) + * + * ⇒ A walker that wants to recognise the recursion point by reference CAN do it + * today, for all ten, by reading `_zod.innerType`. The objectui#7581 script's + * false negative — `ActionSchema` reported "not exported by name" when it plainly + * is — was the wrong handle, not an unrecognisable schema. Memoising is still + * worth doing where it is free, because it makes the PUBLIC `.unwrap()` honest. + * + * ## 3. The card's consequence ② does not reproduce + * + * The card recorded, explicitly unmeasured, that "a document with N nodes + * reconstructs the whole recursive sub-schema N times". Measured, it does not: + * the getter runs ONCE per lazy for the lifetime of the process, because of the + * same `_cachedInner`. `consequence2` below pins it — one call during the first + * parse of a 13-node document, zero during the second. + * + * Wall clock agrees. `NavigationItemSchema` parsing a 73-node document, memoised + * vs. not, same tree, same process count, medians of nine trials of 200 parses: + * 149,973 ns vs 140,293 ns per parse — ratio 0.94x, with the two ranges + * overlapping (143,913–153,846 vs 137,266–149,399). Shared-box seconds, so the + * absolutes are not idle-machine figures; the ratio is the reading, and the + * reading is "no difference". ⛔ There is no parse-time win here to claim. + * + * ## If you are here to memoise the other eight + * + * The naive shape cannot work — that is measured above. A shape that COULD is to + * hoist each body to a module const and push the self-reference behind an inner + * `z.lazy(() => X)`, the way `NavigationItemSchema.children` already does. That + * is deliberately NOT taken here: it trades one identity for another, since + * `children` is currently `z.array(TreeNodeSchema)` — whose element IS the + * exported schema — and the rewrite replaces that element with a fresh anonymous + * wrapper. With consequence ② disproved and `_zod.innerType` already stable, + * the remaining prize is only public `.unwrap()` identity. Whoever prices the + * strict face (objectui#7935 / objectstack#5250) should make that trade + * deliberately. Update this ledger in the same change. + */ +import { describe, it, expect } from 'vitest'; +import { z } from 'zod'; +import { + ActionSchema, + AppMenuItemSchema, + FilterBuilderConditionSchema, + FilterGroupSchema, + MenuItemSchema, + NavLinkSchema, + NavigationItemSchema, + NavigationMenuItemSchema, + SchemaNodeSchema, + TreeNodeSchema, +} from '../zod/index.zod.js'; +import { MenuItemSchema as AppMenuItemSource } from '../zod/app.zod.js'; +import { MenuItemSchema as OverlayMenuItemSource } from '../zod/overlay.zod.js'; + +type LazyInternals = { _zod: { def: { getter: () => unknown }; innerType: unknown } }; +type Unwrappable = { unwrap: () => unknown }; + +/** `S._zod.def.getter() === S._zod.def.getter()` — the card's one-line probe. */ +const getterStable = (S: unknown): boolean => { + const { getter } = (S as LazyInternals)._zod.def; + return getter() === getter(); +}; +/** The PUBLIC accessor. `ZodLazy` spells it `() => inst._zod.def.getter()`. */ +const unwrapStable = (S: unknown): boolean => (S as Unwrappable).unwrap() === (S as Unwrappable).unwrap(); +/** zod's own cached handle, filled once into `def._cachedInner`. */ +const innerTypeStable = (S: unknown): boolean => (S as LazyInternals)._zod.innerType === (S as LazyInternals)._zod.innerType; + +const MEMOISED: ReadonlyArray = [ + ['FilterBuilderConditionSchema', FilterBuilderConditionSchema], + ['NavigationItemSchema', NavigationItemSchema], +]; +/** ⛔ Do not "fix" these — each one's `z.lazy` dodges a real ReferenceError. */ +const TDZ_BOUND: ReadonlyArray = [ + ['ActionSchema', ActionSchema], + ['AppMenuItemSchema', AppMenuItemSchema], + ['FilterGroupSchema', FilterGroupSchema], + ['MenuItemSchema', MenuItemSchema], + ['NavLinkSchema', NavLinkSchema], + ['NavigationMenuItemSchema', NavigationMenuItemSchema], + ['SchemaNodeSchema', SchemaNodeSchema], + ['TreeNodeSchema', TreeNodeSchema], +]; + +describe('objectui#7918 · z.lazy getter identity', () => { + // ── Controls ─────────────────────────────────────────────────────────── + // A `false` below is only a reading if a positive control that MUST hit does + // hit. Without this pair, "everything answers false" is equally consistent + // with the probe being broken. + describe('controls', () => { + it('POSITIVE: a z.lazy whose getter returns a module-level constant is stable', () => { + const moduleLevelConst = z.object({ a: z.string() }); + expect(getterStable(z.lazy(() => moduleLevelConst))).toBe(true); + }); + + it('NEGATIVE: a z.lazy whose getter builds fresh is not stable', () => { + expect(getterStable(z.lazy(() => z.object({ a: z.string() })))).toBe(false); + }); + }); + + // ── The mechanism the eight are dodging ──────────────────────────────── + describe('mechanism', () => { + it('the naive memoisation throws when the body names the const being declared', () => { + const naiveMemoisation = (): z.ZodType => { + // `const inner = ; z.lazy(() => inner)` with a self-referencing + // body — the exact shape all ten would take. The body is built EAGERLY, + // so it reads `Recursive` while `Recursive` is still in its temporal + // dead zone. + const inner: z.ZodType = z.object({ + // @ts-expect-error TS2448 — used before declaration, which is the point. + children: z.array(Recursive).optional(), + }); + const Recursive: z.ZodType = z.lazy(() => inner); + return Recursive; + }; + expect(naiveMemoisation).toThrow(ReferenceError); + }); + }); + + // ── The ledger ───────────────────────────────────────────────────────── + describe('ledger', () => { + it.each(MEMOISED)('%s is memoised — public unwrap() compares by identity', (_name, S) => { + expect(getterStable(S)).toBe(true); + expect(unwrapStable(S)).toBe(true); + }); + + it.each(TDZ_BOUND)('%s stays lazy-per-call — memoising it is a module-load ReferenceError', (_name, S) => { + expect(getterStable(S)).toBe(false); + expect(unwrapStable(S)).toBe(false); + }); + }); + + // ── Consequence ①, corrected ─────────────────────────────────────────── + describe('the recursion point IS identity-comparable today, via _zod.innerType', () => { + it.each([...MEMOISED, ...TDZ_BOUND])( + '%s has a stable _zod.innerType even when its getter is not stable', + (_name, S) => { expect(innerTypeStable(S)).toBe(true); }, + ); + + it('the cached identity survives a .describe() clone, which is what zod caches it for', () => { + const clone = TreeNodeSchema.describe('a clone'); + expect((clone as unknown as LazyInternals)._zod.innerType) + .toBe((TreeNodeSchema as unknown as LazyInternals)._zod.innerType); + }); + }); + + // ── Consequence ②, disproved ─────────────────────────────────────────── + describe('the schema is NOT rebuilt per parse', () => { + it('the getter runs once per lazy for the life of the process, not once per node', () => { + let calls = 0; + const Recursive: z.ZodType = z.lazy(() => { + calls += 1; + return z.object({ id: z.string(), children: z.array(Recursive).optional() }); + }); + // 13 nodes: 1 root + 3 children + 9 grandchildren. + const doc = { + id: 'r', + children: Array.from({ length: 3 }, (_, i) => ({ + id: `c${i}`, + children: Array.from({ length: 3 }, (_, j) => ({ id: `g${i}${j}` })), + })), + }; + expect(calls).toBe(0); + expect(Recursive.safeParse(doc).success).toBe(true); + expect(calls).toBe(1); // not 13, and not 4 + expect(Recursive.safeParse(doc).success).toBe(true); + expect(calls).toBe(1); // still 1 — `def._cachedInner` holds it + }); + }); + + // ── What `AppMenuItemSchema` is ──────────────────────────────────────── + // The card lists both `AppMenuItemSchema` and `MenuItemSchema`. Triage could + // not find a declaration named `AppMenuItemSchema` and refused to conclude it + // does not exist. It exists: it is the BARREL ALIAS of `app.zod.ts`'s + // `MenuItemSchema` (`index.zod.ts`: `MenuItemSchema as AppMenuItemSchema`), + // while the barrel's own `MenuItemSchema` comes from `overlay.zod.ts`. Two + // different schemas, so the card's list really is ten entries, not nine. + describe('AppMenuItemSchema is a barrel alias, not a missing declaration', () => { + it('resolves to app.zod.ts MenuItemSchema', () => { + expect(AppMenuItemSchema).toBe(AppMenuItemSource); + }); + + it('is a different schema from the barrel MenuItemSchema, which is overlay.zod.ts', () => { + expect(MenuItemSchema).toBe(OverlayMenuItemSource); + expect(AppMenuItemSchema).not.toBe(MenuItemSchema); + }); + }); + + // ── The two changed schemas still parse what they parsed ─────────────── + describe('memoisation moved no accept/reject behaviour', () => { + it('NavigationItemSchema still accepts a nested navigation tree', () => { + const doc = { + id: 'root', type: 'group', label: 'Root', + children: [ + { id: 'crm', type: 'group', label: 'CRM', + children: [{ id: 'acct', type: 'object', label: 'Accounts', objectName: 'account' }] }, + { type: 'separator' }, + ], + }; + expect(NavigationItemSchema.safeParse(doc).success).toBe(true); + }); + + it('NavigationItemSchema still runs its superRefine on nested children', () => { + // `label` missing on a non-separator child — the refinement, not the + // field declarations, is what refuses this. + const bad = { + id: 'root', type: 'group', label: 'Root', + children: [{ id: 'acct', type: 'object' }], + }; + expect(NavigationItemSchema.safeParse(bad).success).toBe(false); + }); + + it('FilterBuilderConditionSchema still accepts a condition and refuses a bad operator', () => { + expect(FilterBuilderConditionSchema.safeParse( + { field: 'amount', operator: 'greater_than', value: 100 }, + ).success).toBe(true); + expect(FilterBuilderConditionSchema.safeParse( + { field: 'amount', operator: 'not_a_real_operator' }, + ).success).toBe(false); + }); + + it('FilterGroupSchema still nests conditions and sub-groups through the memoised arm', () => { + const group = { + id: 'g1', logic: 'and', + conditions: [ + { field: 'amount', operator: 'greater_than', value: 100 }, + { id: 'g2', logic: 'or', conditions: [{ field: 'stage', operator: 'equals', value: 'won' }] }, + ], + }; + expect(FilterGroupSchema.safeParse(group).success).toBe(true); + }); + }); +}); diff --git a/packages/types/src/zod/app.zod.ts b/packages/types/src/zod/app.zod.ts index c38c63bb4a..f5fe10b7a8 100644 --- a/packages/types/src/zod/app.zod.ts +++ b/packages/types/src/zod/app.zod.ts @@ -38,8 +38,25 @@ export const NavigationItemTypeSchema = z.enum([ /** * Navigation Item Schema — unified model aligned with @objectstack/spec. + * + * MEMOISED (objectui#7918): the getter returns a module-level constant, so the + * PUBLIC `NavigationItemSchema.unwrap()` is reference-stable. (`ZodLazy` spells + * `unwrap` as `() => _zod.def.getter()`, going around the cache zod keeps on + * `def._cachedInner`, so an un-memoised lazy hands out a fresh schema per call.) + * Safe here for a reason specific to THIS schema — its self-reference is already + * deferred by the inner `z.lazy(() => NavigationItemSchema)` on `children` below, + * so the body itself names nothing that is still uninitialised when it is built. + * + * ⚠️ Seven of the other nine `z.lazy` exports of this face CANNOT take this + * shape — `MenuItemSchema` further down this very file among them. Those bodies + * name the const being declared DIRECTLY (`children: z.array(MenuItemSchema)`, + * no inner `z.lazy`), so building the body eagerly throws `ReferenceError: + * Cannot access '' before initialization` at module load. Their `z.lazy` + * is buying a TDZ dodge, not a style. Measured one schema at a time in + * `../__tests__/zod-lazy-getter-identity-7918.test.ts` — read that before + * "fixing" any of them to match this one. */ -export const NavigationItemSchema: z.ZodType = z.lazy(() => z.object({ +const NavigationItemObject = z.object({ // Declared optional so a bare `{ type: 'separator' }` — which the spec // accepts, and which carries no identity or text by definition — validates // here too (objectstack#4115). Every OTHER type still requires both; that is @@ -112,7 +129,9 @@ export const NavigationItemSchema: z.ZodType = z.lazy(() => z.object({ }); } } -})); +}); + +export const NavigationItemSchema: z.ZodType = z.lazy(() => NavigationItemObject); /** * Navigation Area Schema — business-domain partition of navigation, DERIVED diff --git a/packages/types/src/zod/complex.zod.ts b/packages/types/src/zod/complex.zod.ts index 6741fcd958..ee9cf17f5d 100644 --- a/packages/types/src/zod/complex.zod.ts +++ b/packages/types/src/zod/complex.zod.ts @@ -264,14 +264,29 @@ export const FilterOperatorSchema = z.enum([ /** * Filter Condition Schema + * + * MEMOISED (objectui#7918): the getter returns a module-level constant, so the + * PUBLIC `FilterBuilderConditionSchema.unwrap()` is reference-stable. (`ZodLazy` + * spells `unwrap` as `() => _zod.def.getter()`, going around the cache zod keeps + * on `def._cachedInner`, so an un-memoised lazy hands out a fresh schema per + * call.) Safe here because this body is NOT recursive — it names only + * `FilterOperatorSchema`, declared above. + * + * ⚠️ `FilterGroupSchema` below CANNOT take this shape, and neither can six other + * `z.lazy` exports of this face: their bodies name the very const being declared + * (or, for `SchemaNodeSchema`, one declared below it), so evaluating the body + * eagerly throws `ReferenceError: Cannot access '' before initialization` + * at module load. The `z.lazy` there is buying a TDZ dodge, not a style. Measured + * one schema at a time in `../__tests__/zod-lazy-getter-identity-7918.test.ts` + * — read that before "fixing" any of them to match this one. */ -export const FilterBuilderConditionSchema: z.ZodType = z.lazy(() => - z.object({ - field: z.string().describe('Field name'), - operator: FilterOperatorSchema.describe('Filter operator'), - value: z.any().optional().describe('Filter value'), - }) -); +const FilterBuilderConditionObject = z.object({ + field: z.string().describe('Field name'), + operator: FilterOperatorSchema.describe('Filter operator'), + value: z.any().optional().describe('Filter value'), +}); + +export const FilterBuilderConditionSchema: z.ZodType = z.lazy(() => FilterBuilderConditionObject); /** * Filter Group Schema — the shape `FilterBuilder` actually reads