fix(hir/codegen): new <imported-function>() runs the constructor body (zod v4 checks; #4698)#4769
Merged
Merged
Conversation
…zod v4 checks; #4698) `new F(args)` where `F` is a function — or a `const`/`let` holding a closure value — imported from another module silently produced an empty object: the constructor body never ran, so `this.x = …` and `Object.defineProperty(this, …)` writes were lost. This is the zod-v4 `TypeError: Cannot read properties of undefined (reading 'onattach')` crash (#4698): every string/number schema with a check (`.min`/`.max`/`.length`/`.regex`/`.gt`/`.lt`/…) builds its check via `new checks.$ZodCheckMinLength(def)` across the core/checks.ts → core/api.ts module boundary (where `checks` is `import * as`), and the check came back with its non-enumerable `_zod` (and everything else) gone. An imported binding that isn't a registered class fell through to an empty-object placeholder instead of `js_new_function_construct` (binds `this`, runs the body, returns the populated instance). The fix lives entirely in codegen: at HIR-lowering time an imported class and an imported function are indistinguishable (both unknown to `lookup_class`), and the cross-module class-inline machinery in `collect_modules` relies on `new <ImportedClass>()` staying as `Expr::New { class_name }` — so HIR must not reroute it. - perry-codegen lower_call/new.rs: when `lower_new` finds no class for the name but the name resolves to an imported binding (`import_function_prefixes`, excluding V8-fallback specifiers), lower it as an `ExternFuncRef` value and construct via `js_new_function_construct` instead of the empty placeholder. Imported classes are in `ctx.classes` and take the construction path, so they never reach this fallback (guarded by the existing `dependency_is_transformed_before_importer_for_cross_module_inline` test). - perry-codegen expr/v8_interop.rs (`try_static_class_name`) + expr/new_dynamic.rs: `new ns.Foo()` over a namespace import only takes the class path when `Foo` actually names a known class; a closure-valued `const` export (zod's `$ZodCheckMinLength = $constructor(...)`) falls through to the NewDynamic function-construct route (`ExternFuncRef` added to its callee set). Real namespace-imported classes and re-exported builtins are unaffected. `z.string().min(2).parse("hello")` now prints `hello`; `.max`/`.length`/`.regex`/ `.gt`/`.lt` and nested object schemas with checks match Node byte-for-byte. Full `perry` bin test suite (469) green. The `safeParse` error-path / `.email()` error-formatting failures noted in #4698 are a separate, still-open issue. Refs #793.
9a3f28e to
cf736d2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #4698.
new F(args)whereFis a function — or aconst/letholding a closure value — imported from another module silently produced an empty object: the constructor body never ran, sothis.x = …andObject.defineProperty(this, …)writes were lost.This is the zod-v4
TypeError: Cannot read properties of undefined (reading 'onattach')crash. Every string/number schema with a check (.min/.max/.length/.regex/ numeric.gt/.lt/ …) builds its check vianew checks.$ZodCheckMinLength(def)across thecore/checks.ts→core/api.tsmodule boundary (wherechecksis animport * asnamespace), so the check instance came back with its non-enumerable_zodproperty (and everything else) gone — then crashed onch._zod.onattach.Root cause
An imported binding is neither a local, a
func, nor a class in the importing module, so it fell through toExpr::New { class_name }— whose codegen finds no matching class and emits an empty placeholder — instead ofjs_new_function_construct, which bindsthis, runs the body, and returns the populated instance. Three gaps, all on the cross-modulenewpath:perry-hirlower/expr_new.rs— barenew <importedFn>()now routes toNewDynamic { callee: ExternFuncRef, … }when the name resolves vialookup_imported_func, mirroring the existing local /FuncRefbranches.perry-codegenexpr/new_dynamic.rs—ExternFuncRefadded to theroutes_through_function_constructcallee set so it reachesjs_new_function_constructinstead of the best-effort empty-object fallback.perry-codegenexpr/v8_interop.rs::try_static_class_name—new ns.Foo()over a namespace import only takes the class-construction path whenFooactually names a known class; a closure-valuedconstexport (zod's$ZodCheckMinLength = $constructor(...)) now falls through to the function-construct route. Real namespace-imported classes (inctx.classes) and re-exported builtins (intercepted by the helper's global thunks) are unaffected.Verification
The exact repro now matches Node:
.max/.length/.regex/ numeric.gt/.ltand nested object schemas with checks all match Node byte-for-byte. Minimal multi-module + namespace-import repros pass; real namespace-imported classes still construct correctly.perry-hir+perry-codegenunit tests: pass.test_issue_836_zod_class_reexports(cross-module class re-export, directly in the change's blast radius) passes. The pre-existing class-suite failures (iterators, mixins,defineProperty-on-prototype, stream subclass) are single-file programs with zero imports — and all three changes here require a cross-module import to fire, so they are provably inert for those files.Out of scope
The
safeParseerror-path /.email()error-formatting failures noted in #4698 are a separate, still-open issue.Refs #793.