Conversation
7d0207c to
848a0e9
Compare
|
Oh, didn't make any comments. this fix is for the fixed circular object validation It's AI slop, so I hope it works. :) |
There was a problem hiding this comment.
Not recommended for merge in current form. This PR addresses a legitimate issue (infinite recursion when parsing objects with circular references), and the fundamental approach is correct. However, the implementation has significant code quality issues that should be addressed before merging into a library as widely used as Zod.
What works well:
- The tests are comprehensive and cover important edge cases
- The
(input, schema) → outputmapping correctly handles the case where the same object is parsed by different schemas - Lazy initialization of the
visitedmap avoids overhead for simple cases
Blocking concerns:
- Massive code duplication — ~35 lines of nearly identical circular tracking logic are copy-pasted 4 times across
$ZodArray,$ZodObject,$ZodObjectJIT, and$ZodTuple. This violates DRY and will make maintenance difficult. - Incomplete coverage —
$ZodRecord,$ZodMap, and$ZodSetalso iterate over collections and could theoretically have circular values, but don't have tracking added. The record tests pass only because they delegate to$ZodObjectwhich has tracking. - Unrelated change — The
.gitignoreaddition for.claude/is unrelated and shouldn't be part of this PR.
I'd recommend either:
- Extracting the circular tracking logic into helper functions (e.g.,
checkCircularRefandregisterCircularRef) - Or discussing with maintainers whether this feature belongs in core Zod at all, vs. being a userland utility
Thank you for the contribution and for tackling a real pain point!
packages/zod/src/v4/core/schemas.ts
Outdated
| // Check if we need circular reference tracking | ||
| // We need it if there are any object/array elements that could create cycles | ||
| let visited = ctx?.visited; | ||
| const needsTracking = input.some((item) => typeof item === "object" && item !== null); | ||
|
|
||
| if (needsTracking) { | ||
| // Lazily initialize visited if not already set | ||
| if (!visited) { | ||
| visited = new WeakMap(); | ||
| (ctx as any).visited = visited; | ||
| } | ||
|
|
||
| // Check if we've already seen this (array, schema) pair | ||
| const schemaMap = visited.get(input); | ||
| if (schemaMap) { | ||
| const cached = schemaMap.get(inst); | ||
| if (cached !== undefined) { | ||
| // Already validated (or being validated) by THIS schema - return cached output | ||
| payload.value = cached; | ||
| return payload; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| payload.value = Array(input.length); | ||
|
|
||
| // Store the mapping before recursing (so circular refs can find it) | ||
| if (visited && needsTracking) { | ||
| let schemaMap = visited.get(input); | ||
| if (!schemaMap) { | ||
| schemaMap = new WeakMap(); | ||
| visited.set(input, schemaMap); | ||
| } | ||
| schemaMap.set(inst, payload.value); | ||
| } |
There was a problem hiding this comment.
This ~35-line block is duplicated almost identically in $ZodObject (lines 1934-1977), $ZodObjectJIT (lines 2110-2141), and $ZodTuple (lines 2708-2742). Consider extracting this into helper functions, e.g.:
function checkCircularRef(
input: object,
inst: object,
ctx: ParseContext | undefined
): object | undefined {
const visited = ctx?.visited;
if (!visited) return undefined;
return visited.get(input)?.get(inst);
}
function registerCircularRef(
input: object,
inst: object,
output: object,
ctx: ParseContext | undefined
): void {
// ... initialization and registration logic
}This would reduce ~140 lines of duplicated code to ~40 lines of shared logic.
packages/zod/src/v4/core/schemas.ts
Outdated
| // Check if we need circular reference tracking | ||
| // We need it if there are any nested object/array values that could create cycles | ||
| let visited = ctx?.visited; | ||
| let needsTracking = !!visited; | ||
| if (!needsTracking) { | ||
| for (const key in input) { | ||
| const v = input[key]; | ||
| if (typeof v === "object" && v !== null) { | ||
| needsTracking = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (needsTracking) { | ||
| // Lazily initialize visited if not already set | ||
| if (!visited) { | ||
| visited = new WeakMap(); | ||
| (ctx as any).visited = visited; | ||
| } | ||
|
|
||
| // Check if we've already seen this (object, schema) pair | ||
| const schemaMap = visited.get(input); | ||
| if (schemaMap) { | ||
| const cached = schemaMap.get(inst); | ||
| if (cached !== undefined) { | ||
| // Already validated (or being validated) by THIS schema - return cached output | ||
| payload.value = cached; | ||
| return payload; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This circular tracking code duplicates logic from $ZodObject, but there's a subtle inefficiency: after checking for a cached result (and not finding one), this code calls superParse which performs the exact same check again. Either:
- Remove this check entirely and let
superParsehandle all circular tracking, OR - If you keep the check, also do the registration here and avoid calling
superParsefor the circular case
Current flow: JIT checks cache (miss) → calls superParse → superParse checks cache again (miss) → stores → parses
.gitignore
Outdated
| tsconfig.vitest-temp.json | ||
| *.tgz | ||
|
|
||
| .claude/ |
There was a problem hiding this comment.
This change is unrelated to circular object validation. If you want to add .claude/ to gitignore, it should be a separate PR or at minimum mentioned in the PR description.
Review CompleteI've reviewed PR #5664 and left detailed feedback. The PR addresses a legitimate issue (infinite recursion when parsing objects with circular references), but has significant code quality concerns: Summary:
Recommendation: Not ready for merge in current form. See inline comments for specific suggestions. |
848a0e9 to
7dcc108
Compare
lawless-m
left a comment
There was a problem hiding this comment.
responded to pullfrogs criticism
Add circular reference tracking to prevent infinite recursion when validating objects with circular references via z.lazy(). Uses a WeakMap to track (input object, schema) pairs during parsing. Changes: - Add helper functions (needsCircularTracking, checkCircularRef, registerCircularRef) - Add circular detection to $ZodArray, $ZodObject, $ZodObjectJIT, $ZodTuple - Add circular detection to $ZodRecord, $ZodMap, $ZodSet - Disable JIT fast path when circular tracking is active - Add comprehensive test suite for circular validation scenarios Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
7dcc108 to
22965b8
Compare
|
pullfrog is a great tool |
No description provided.