fix(codegen): stop new(X) picking a builtin over a same-named imported function ctor - #10608
Closed
proggeramlug wants to merge 3 commits into
Closed
proggeramlug wants to merge 3 commits into
proggeramlug wants to merge 3 commits into
Conversation
added 2 commits
September 18, 2026 08:15
…d function ctor lower_new_impl_inner called lower_builtin_new for any class_name absent from ctx.classes before checking import_function_prefixes. Imported classes already land in ctx.classes and skip the builtin block; an imported plain function constructor (Headers, EventEmitter-shaped, ...) never does, so any unconditional builtin arm (not gated by required_sources) fired regardless of whether the callee was a bare identifier or wrapped in (X as any) -- peel_new_callee strips that cast before lower_new branches on callee shape.
…(X as any)() Gap test: Headers/EventEmitter/Stream x function/class declaration x named/default import x plain-new/cast-new, plus a local-alias control that already worked. Property-based discriminators, not instanceof -- #10477 (imported non-class instanceof) is not yet fixed on this base and would conflate the two bugs. Unit tests: direct Expr::New harness asserting js_new_function_construct fires (not the builtin arm) once a name resolves to an imported function constructor, the builtin still fires when unshadowed, a V8-fallback import of the same name still falls to the builtin, and an imported CLASS of the same name already shadowed the builtin before this fix (regression guard).
proggeramlug
added
the
package-audit
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (23)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe fix prevents imported function constructors from being replaced by same-named builtins during ChangesBuiltin constructor shadowing
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The constructor-shadowing change is covered by compiler tests and parity checks for the affected import forms, with no actionable risk remaining. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 11.76% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 22 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
proggeramlug
pushed a commit
that referenced
this pull request
Sep 18, 2026
Contributor
Author
|
Landed via merge train #10652 (v0.5.1596). All source commits preserve authorship; merged main matches the validated train exactly. |
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
1 participant
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
new (X as any)()(and, as this PR's testing found, plainnew X()too) on animported binding whose name collides with a Perry builtin constructor
(
Headers, confirmed; any other name matching an unconditionallower_builtin_newarm is the same class of bug) constructed the builtininstead of the user's own function/class of the same name.
Root cause
lower_new_impl_inner(crates/perry-codegen/src/lower_call/new.rs:279)called into the unconditional builtin-constructor dispatch table
(
lower_builtin_new) for anyclass_nameabsent fromctx.classes, beforeever checking
ctx.import_function_prefixes— the table where an importedplain function constructor is tracked. Imported classes are already
registered in
ctx.classesfor the importing module (via theopts.imported_classesstub-building pass incodegen/mod.rs), so theyalways skipped the builtin block. An imported plain function never lands in
ctx.classes, so any builtin arm not gated by the existingrequired_sourcesprovenance check (used for the genuinely ambiguous
Client/Pool/Database/Redis/MongoClient/Decimal/RateLimiterMemory/CronJobnames) firedunconditionally.
The
as anycast is not the trigger. HIR'speel_new_callee(
crates/perry-hir/src/lower/expr_new/helpers.rs) stripsParen/TsAs/TsTypeAssertion/TsNonNull/TsConstAssertionfrom the callee beforelower_newever branches on its shape, sonew X()andnew (X as any)()lower to the byte-identical
Expr::New { class_name: "X", .. }. I verifiedthis empirically: on this base (
9df5075fbe3), bothnew Headers(1)andnew (Headers as any)(1)threw the real fetch-APIHeaders's "init is notiterable" error identically. The issue's "alias works, cast doesn't" framing
describes a real, reproducible symptom, but the actual discriminator is
local-variable alias vs. bare imported identifier, not cast vs. no-cast —
a local alias takes a completely different codegen path (
NewDynamicoverLocalGet, resolved as a runtime value viajs_new_function_construct,never touching the by-name builtin table at all).
I also found the bug's actual scope is narrower than the issue's three
named examples: on this base, only
Headers(declared as a plain function)reproduces.
EventEmitter(declared as aclassin the repro) alreadyworked, because imported classes skip the builtin block as described above.
Streamalready worked too, becauselower_builtin_newhas no"Stream"arm at all (nothing to collide with). The gap test below still exercises all
three names, in both function- and class-declaration form, because the
underlying mechanism (an unconditional builtin arm + a same-named imported
plain function) applies to any of Perry's ~50 unconditional builtin
constructor names, not just
Headers.Fix
Added a
user_owns_constructionguard — mirroring the existingrequired_sourcesgate pattern — to the same condition that already letsimported classes skip the builtin block:
When skipped, control falls through to the pre-existing
import_function_prefixesarm below
ctx.classes.get(class_name)(added for #4698), which constructs thevalue via
js_new_function_construct— the same runtime helper every otherimported-function-constructor call already uses. No new runtime code.
Tests
Gap test:
test-files/test_gap_10589_new_cast_builtin_shadow.ts+test-files/_helpers/new_cast_builtin_shadow_10589/(18 helper files). CoversHeaders/EventEmitter/Stream× function-declaration/class-declaration ×named-import/default-import, each probing
new X()(plain),new (X as any)()(cast), and
new alias()for a local-variable alias (the control that alreadyworked — a regression there is caught by the same assertion). Each of the 12
scenarios lives in its own tiny helper module because a single module can only
bind one top-level identifier per reserved name. Discriminators are
own-property checks (
__mark === "user"), notinstanceof— #10477(imported non-class constructor
instanceof) is not fixed on this base yet,and using
instanceofhere would conflate the two bugs.Proof the test fails on baseline, passes on the fix (both via
PERRY_SKIP_BUILD=1 ./run_parity_tests.sh --filter test_gap_10589):crates/perry-codegen/src/lower_call/new.rsat9df5075fbe3):PARITY_FAIL—headers fn named: plain=THROW:Headers constructor: init is not iterable (received 0x3ff0000000000000) cast=THROW:... alias=truevs. Node'splain=true cast=true alias=true.PASS, 100% parity, all 12 scenarios match Node 26.5.1 exactly.Unit tests:
crates/perry-codegen/src/lower_call/new_builtin_shadow_tests.rs(4 tests, direct
Expr::New+CompileOptionsharness, asserting on emittedLLVM IR):
call ... @js_new_function_constructpresent,call ... @js_headers_newabsent);Validation
cargo test --profile perry-dev -p perry-codegen --tests: all pass(36
test result: okblocks across the crate, 0 failed, including the 4 newtests and everything pre-existing).
Lint (
SKIP_COMPILE_GATES=1 ./scripts/run_lint_gates.sh, compile tier notrun per this campaign's known-red-on-Linux note): 76/77 script gates
passed; the one failure,
Public benchmark evidence freshness, isdocumented pre-existing-red on
main(this campaign's FIX_BRIEF and theproject's own testing docs both call it out).
Gap suite, targeted (
PERRY_SKIP_BUILD=1 PERRY_NO_AUTO_OPTIMIZE=1 ./run_parity_tests.sh --filter <name>): the new test plus 18 adjacentshadowing/native-base-construction/EventEmitter/Stream/Headers gap tests
(
new_local_shadows_class,module_const_local_shadow,new_globalthis_shadowed_10359(fix(hir,codegen):new globalThis.X()constructs the global when a binding shadows X (#10359) #10375's fix),9466_shadowed_class_identity,proto_write_local_shadows_class,5437_class_ref_shadows_captured_local,buffer_own_prop_shadow_intrinsic_6405,6316/6326/6336/6343_native_base_*,class_expr_dynamic_parent_ctor,events_import_4995,fetch_response,node_events_console_3072_3081_3080,process_emitter_3047_3046_3050,10430_stream_module_constructor) — all pass. (An early pass showed8 spurious
COMPILE_FAILs from a stale auto-optimize runtime-archive cacheleft over from my own iterative rebuilds on this box — commit-stamp
mismatch between compiler and a cached auto-built runtime archive, not a
code issue; rebuilding + clearing
target/perry-auto-*resolved it and all18 passed cleanly on rerun.)
Performance:
lower_new_impl_innerruns at Perry's own compile time(codegen), not in the compiled program's runtime, so this fix cannot regress
the runtime instruction count of any
newcall whose emitted IR isunchanged — which is everything except the specific collision case. Verified
directly:
--trace llvmIR for a representative unaffectednewcall(imported plain-function constructor, non-colliding name) is byte-identical
between baseline and fixed compilers (0-line diff, 3567 lines each). Backed
that with
perf stat -e instructions,task-clock, release builds(
CARGO_PROFILE_RELEASE_CODEGEN_UNITS=16, 3 runs each):new Widget(i))benchmarks/bench_histogram_numarray.ts(new Array(BUCKETS), unconditional builtin arm, unaffected)Both well within the ±1% noise floor; no regression. Node wall time on the
loop workload for context: 0.076s (Perry's ~2.7-3.3s wall / ~35.8B
instructions for 3M allocating iterations is a pre-existing characteristic
of
js_new_function_construct-routed construction, identical on bothbinaries — out of scope for this PR, not introduced or affected by it).
Package check: the issue does not name a specific npm package (found during
general package-audit validation of
x instanceof Fis always false whenFis an imported non-class constructor (ES5 function, factory-made function, CJSmodule.exports = F); namespace access and local aliases work #10477), so this step is not applicable.Not verified
default (targeted filters only; CI's gap-suite shards are the full gate).
cargo test --workspacewas not run; only-p perry-codegen(the onlycrate touched).
Related
Database,Decimal,WebSocket,Redis,LRUCache,Command,CronJob,Pool,Socketare rewritten to native handles #10439 (native bindings chosen by name rather than by what theidentifier resolves to) but a distinct mechanism — that issue's repros are
bare class/type references; this one requires a same-named imported plain
function specifically, since imported classes already have a
ctx.classesescape hatch.
x instanceof Fis always false whenFis an imported non-class constructor (ES5 function, factory-made function, CJSmodule.exports = F); namespace access and local aliases work #10477 / PR fix(hir): give an imported constructor's instanceof its value #10596 (x instanceof Ffor an imported non-classconstructor) — same general family (imported-function-constructor support
gaps) but a different bug in a different subsystem (HIR
instanceofRHSvalue attachment vs. codegen
newbuiltin-vs-user dispatch ordering); notthe same root cause, contrary to what the issue's discovery narrative might
suggest.
new globalThis.X()constructs the global when a binding shadows X (#10359) #10375 (shadowednew globalThis.X()), whichis an HIR-level global-property-qualified-callee fix; checked as prior art,
not directly reusable here since this bug is purely in codegen's
bare-identifier dispatch ordering.
Fixes #10589
Summary by CodeRabbit
Bug Fixes
as anycasts, and aliases now consistently instantiate the imported user-defined constructor instead of the built-in.Tests