fix(tools): detect render-phase cycles and fix class-name JSDoc placement
Address ds-review-bot v5/v6 review round 6: - renderType tracks the active ancestor schemas by object identity (the frame stack is the DFS path). A stateful getter can mutate the graph after validation so a child returns an ancestor at render time; without this the walk pushed frames forever instead of degrading. A repeated ancestor now degrades to Any, honoring the never-throw contract; distinct nodes in a legitimately deep chain are different objects, so it stays O(1) per push and O(depth) memory. - The multiline allocateClassName JSDoc was still attached to the MAX_CLASS_NAME_BASE constant (a self-referential @link, and the function had no doc). Move the doc onto the function and give the constant its own one-liner. - Tests cover the post-validation cycle and a non-object render-time child; py-types.ts stays at 100% per-file coverage.
This commit is contained in:
@@ -122,6 +122,9 @@ function camelCase(raw: string): string {
|
|||||||
return /^[A-Za-z]/.test(joined) ? joined : `Tool${joined}`
|
return /^[A-Za-z]/.test(joined) ? joined : `Tool${joined}`
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Class-name base cap keeping each emitted name — and total text — linear in schema depth. */
|
||||||
|
const MAX_CLASS_NAME_BASE = 120
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Reserve a unique class name from a base, suffixing `2`, `3`, … on collision.
|
* Reserve a unique class name from a base, suffixing `2`, `3`, … on collision.
|
||||||
* The base is capped at {@link MAX_CLASS_NAME_BASE} first: child class names
|
* The base is capped at {@link MAX_CLASS_NAME_BASE} first: child class names
|
||||||
@@ -133,7 +136,6 @@ function camelCase(raw: string): string {
|
|||||||
* `2`, so a deep chain sharing one capped base stays O(1) per allocation
|
* `2`, so a deep chain sharing one capped base stays O(1) per allocation
|
||||||
* (amortized) instead of Θ(depth²) in time.
|
* (amortized) instead of Θ(depth²) in time.
|
||||||
*/
|
*/
|
||||||
const MAX_CLASS_NAME_BASE = 120
|
|
||||||
function allocateClassName(base: string, state: RenderState): string {
|
function allocateClassName(base: string, state: RenderState): string {
|
||||||
const capped = base.length > MAX_CLASS_NAME_BASE ? base.slice(0, MAX_CLASS_NAME_BASE) : base
|
const capped = base.length > MAX_CLASS_NAME_BASE ? base.slice(0, MAX_CLASS_NAME_BASE) : base
|
||||||
let name = capped
|
let name = capped
|
||||||
@@ -220,6 +222,15 @@ function renderType(schema: unknown, className: string, state: RenderState): str
|
|||||||
const newFrame = (schema: unknown, className: string, validated: boolean): Frame =>
|
const newFrame = (schema: unknown, className: string, validated: boolean): Frame =>
|
||||||
({ schema, className, phase: 'start', children: [], childIndex: 0, childTypes: [], entries: [], validated })
|
({ schema, className, phase: 'start', children: [], childIndex: 0, childTypes: [], entries: [], validated })
|
||||||
const frames: Frame[] = [newFrame(schema, className, false)]
|
const frames: Frame[] = [newFrame(schema, className, false)]
|
||||||
|
// Ancestor schemas by object identity — the frame stack IS the DFS path, so
|
||||||
|
// this set holds exactly the current node's ancestors. A stateful getter can
|
||||||
|
// mutate the graph after validation (an `items`/property that validated as a
|
||||||
|
// scalar but returns an ancestor at render time); without this, the walk
|
||||||
|
// would push frames forever. A repeated ancestor degrades to `Any` per the
|
||||||
|
// never-throw contract. Distinct nodes in a legitimately deep chain are all
|
||||||
|
// different objects, so this stays O(1) per push and O(depth) memory.
|
||||||
|
const activeSchemas = new Set<object>()
|
||||||
|
if (typeof schema === 'object' && schema !== null) activeSchemas.add(schema)
|
||||||
let result: string | undefined
|
let result: string | undefined
|
||||||
// The no-throw contract must hold across the WHOLE walk, not just the root
|
// The no-throw contract must hold across the WHOLE walk, not just the root
|
||||||
// validation: a hostile stateful getter (a `type` that returns a scalar on
|
// validation: a hostile stateful getter (a `type` that returns a scalar on
|
||||||
@@ -231,7 +242,10 @@ function renderType(schema: unknown, className: string, state: RenderState): str
|
|||||||
/* jscpd:ignore-start -- the explicit-stack walk skeleton deliberately parallels
|
/* jscpd:ignore-start -- the explicit-stack walk skeleton deliberately parallels
|
||||||
ts-types.ts's renderSupportedSchema; the two sibling renderers keep symmetric shapes. */
|
ts-types.ts's renderSupportedSchema; the two sibling renderers keep symmetric shapes. */
|
||||||
const finish = (type: string): void => {
|
const finish = (type: string): void => {
|
||||||
frames.pop()
|
const popped = frames.pop()
|
||||||
|
if (popped !== undefined && typeof popped.schema === 'object' && popped.schema !== null) {
|
||||||
|
activeSchemas.delete(popped.schema)
|
||||||
|
}
|
||||||
const parent = frames.at(-1)
|
const parent = frames.at(-1)
|
||||||
if (parent === undefined) result = type
|
if (parent === undefined) result = type
|
||||||
else parent.childTypes.push(type)
|
else parent.childTypes.push(type)
|
||||||
@@ -249,6 +263,18 @@ function renderType(schema: unknown, className: string, state: RenderState): str
|
|||||||
/* v8 ignore next -- childIndex is bounded by children.length. */
|
/* v8 ignore next -- childIndex is bounded by children.length. */
|
||||||
if (child === undefined) throw new Error('missing python render child')
|
if (child === undefined) throw new Error('missing python render child')
|
||||||
frame.childIndex++
|
frame.childIndex++
|
||||||
|
// A child schema already on the active path is a cycle a post-
|
||||||
|
// validation mutation introduced; degrade it to `Any` rather than
|
||||||
|
// recurse forever. A fresh object joins the path (finish removes it);
|
||||||
|
// a non-object child carries no identity to track.
|
||||||
|
if (typeof child.schema === 'object' && child.schema !== null) {
|
||||||
|
if (activeSchemas.has(child.schema)) {
|
||||||
|
state.typing.add('Any')
|
||||||
|
frame.childTypes.push('Any')
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
activeSchemas.add(child.schema)
|
||||||
|
}
|
||||||
frames.push(newFrame(child.schema, child.className, true))
|
frames.push(newFrame(child.schema, child.className, true))
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -144,6 +144,43 @@ describe('jsonSchemaToPy', () => {
|
|||||||
expect(text).toContain('class FooArgsPhase3(TypedDict):')
|
expect(text).toContain('class FooArgsPhase3(TypedDict):')
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('degrades to Any instead of looping when a stateful getter introduces a cycle after validation', () => {
|
||||||
|
// `items` validates as a scalar, then returns the root schema at render
|
||||||
|
// time — a cycle a post-validation mutation introduced. The walk must
|
||||||
|
// degrade to Any rather than push frames forever.
|
||||||
|
let itemReads = 0
|
||||||
|
const root: Record<string, unknown> = { type: 'array' }
|
||||||
|
Object.defineProperty(root, 'items', {
|
||||||
|
enumerable: true,
|
||||||
|
get() {
|
||||||
|
itemReads += 1
|
||||||
|
return itemReads <= 1 ? { type: 'string' } : root
|
||||||
|
},
|
||||||
|
})
|
||||||
|
let out: string | undefined
|
||||||
|
expect(() => { out = jsonSchemaToPy(root) }).not.toThrow()
|
||||||
|
// list[...] of a self-cycle: the inner cycle degrades to Any.
|
||||||
|
expect(out).toBe('list[Any]')
|
||||||
|
})
|
||||||
|
|
||||||
|
it('degrades to Any when a stateful getter returns a non-object child at render time', () => {
|
||||||
|
// `items` validates as a scalar node, then returns a bare string (a
|
||||||
|
// non-object) at render. The walk must handle a non-object child without
|
||||||
|
// tracking identity and degrade it, not throw.
|
||||||
|
let itemReads = 0
|
||||||
|
const root: Record<string, unknown> = { type: 'array' }
|
||||||
|
Object.defineProperty(root, 'items', {
|
||||||
|
enumerable: true,
|
||||||
|
get() {
|
||||||
|
itemReads += 1
|
||||||
|
return itemReads <= 1 ? { type: 'string' } : 'not-a-schema-object'
|
||||||
|
},
|
||||||
|
})
|
||||||
|
let out: string | undefined
|
||||||
|
expect(() => { out = jsonSchemaToPy(root) }).not.toThrow()
|
||||||
|
expect(out).toBe('list[Any]')
|
||||||
|
})
|
||||||
|
|
||||||
it('emits exact digits for a beyond-safe-range integer literal', () => {
|
it('emits exact digits for a beyond-safe-range integer literal', () => {
|
||||||
// Python integers are arbitrary-precision, so the emitted digits ARE the
|
// Python integers are arbitrary-precision, so the emitted digits ARE the
|
||||||
// value the model programs against. `String(2 ** 60)` prints the rounded
|
// value the model programs against. `String(2 ** 60)` prints the rounded
|
||||||
|
|||||||
Reference in New Issue
Block a user