-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(hir,codegen): new globalThis.X() constructs the global when a binding shadows X (#10359)
#10375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| - **`new globalThis.X(...)` constructs the global even when a module binding shadows `X` (#10359).** `globalThis.X` names the global object's property, never a module binding, but every arm that lowered the qualified construct *by name* resolved it against the module's bindings. With `import { Event } from "./ev"` in scope, `new globalThis.Event("ping")` built the imported class (`e instanceof Event === true`, `e.type === undefined`), while the aliased `const E = globalThis.Event; new E("ping")` was correct. That defeats `globalThis.X`'s only purpose, escaping a local shadow; OpenCode's graph exports `Event`, `File`, `Request`, `Error`, `WebSocket`, `FormData` and `Storage`. Four by-name paths now back off when an import, class (at any depth), function, local or class alias shares the name, and construct the global property's value instead (`NewDynamic` over `globalThis.X`): | ||
| - the #6726 re-dispatch through the bare-identifier arm (`crates/perry-hir/src/lower/expr_new.rs`) ignored the shadow only for the dedicated intrinsic nodes (`SetNew`, `ErrorNew`, `UrlNew`, …). Names with none (`Event`, `Request`, `Headers`, `MessageChannel`, the three-argument typed-array form) reached the by-name tail (`New { class_name }` / `FuncRef` / `LocalGet`, plus the proxy-local and dynamic-function-subclass arms); | ||
| - `lower_new_member_native`'s `globalThis` fetch-constructor and `MessageChannel`/`BroadcastChannel` arms (`expr_new/member.rs`); | ||
| - `lower_new_non_ident`'s global-object fetch arm, reached through a `globalThis` alias (`const g = globalThis; new g.Headers()`); | ||
| - codegen's `try_static_class_name` (`crates/perry-codegen/src/expr/v8_interop.rs`) folded a `globalThis.X` callee onto a same-named module class, class alias or import. `class Widget {}` plus `globalThis.Widget = class {…}` built the module class, and with no such global `new globalThis.Gadget()` quietly built `class Gadget` instead of throwing a `TypeError`. Now `NewDynamic` (`expr/new_dynamic.rs`) builds the declined callee through the builtin table (`lower_global_intrinsic_new` → `lower_builtin_new`, skipping module classes), the construct the unshadowed form reaches. Only a name no builtin arm owns reads the property at runtime. Without that step, streams came back method-less and `WebSocket` had no `readyState`. | ||
|
|
||
| Unshadowed names keep their by-name intrinsic construct, and a shadowed name with a dedicated intrinsic node (`new globalThis.Map()` under `import { Map }`) keeps that node. Tests: `lower::tests::global_this_new_shadowed` (the shadowed test fails against the pre-fix lowering) and `test-files/test_gap_new_globalthis_shadowed_10359.ts` (byte-identical to Node 26.5.1. Before the fix it diverged from line 1 and crashed at `mc.port1.close()`). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,7 +65,11 @@ pub(crate) fn lower_new_member_native( | |
| // that never import `node:worker_threads`. The runtime global | ||
| // delegates to the full worker_threads factory whenever the | ||
| // stdlib has registered it, so no behavior is lost. | ||
| if is_worker_messaging_constructor_name(class_name) { | ||
| // #10359: `Expr::New` resolves by name, so a same-named user | ||
| // binding would capture it — fall through to the re-dispatch. | ||
| if is_worker_messaging_constructor_name(class_name) | ||
| && !(obj_name == "globalThis" && global_name_has_user_binding(ctx, class_name)) | ||
| { | ||
|
Comment on lines
+68
to
+72
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '45,105p' crates/perry-hir/src/lower/expr_new/member.rs
rg -n 'is_worker_messaging_constructor_name|shadows_unqualified_global\("globalThis"\)|MessageChannel|BroadcastChannel' crates/perry-hir/src/lower test-filesRepository: PerryTS/perry Length of output: 9682 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- member lowering context ---'
sed -n '1,125p' crates/perry-hir/src/lower/expr_new/member.rs
printf '%s\n' '--- expr_new intrinsic context ---'
sed -n '190,245p' crates/perry-hir/src/lower/expr_new.rs
printf '%s\n' '--- helper definitions ---'
rg -n -A35 -B10 'fn global_name_has_user_binding|global_name_has_user_binding|shadows_unqualified_global' crates/perry-hir/src
printf '%s\n' '--- shadowing tests ---'
cat -n crates/perry-hir/src/lower/tests/global_this_new_shadowed.rs
printf '%s\n' '--- related test fixture ---'
cat -n test-files/test_gap_new_globalthis_shadowed_10359.ts
printf '%s\n' '--- Expr::New lowering and resolution references ---'
rg -n -A28 -B12 'Expr::New|class_name.*New|resolve.*class_name|lookup.*class_name' crates/perry-hir/src/lower crates/perry-codegen crates/perry-runtime 2>/dev/null | head -n 260Repository: PerryTS/perry Length of output: 50370 🏁 Script executed: sed -n '1,125p' crates/perry-hir/src/lower/expr_new/member.rs; sed -n '190,245p' crates/perry-hir/src/lower/expr_new.rs; rg -n -A35 -B10 'fn global_name_has_user_binding|global_name_has_user_binding|shadows_unqualified_global' crates/perry-hir/src; cat -n crates/perry-hir/src/lower/tests/global_this_new_shadowed.rs; cat -n test-files/test_gap_new_globalthis_shadowed_10359.tsRepository: PerryTS/perry Length of output: 50370 🏁 Script executed: printf '%s\n' '--- definitions ---'
rg -n -A45 -B12 'global_member_constructor_name|global_name_has_user_binding|shadows_unqualified_global' crates/perry-hir/src/lower
printf '%s\n' '--- tests ---'
cat -n crates/perry-hir/src/lower/tests/global_this_new_shadowed.rs
cat -n test-files/test_gap_new_globalthis_shadowed_10359.tsRepository: PerryTS/perry Length of output: 50370 🏁 Script executed: rg -n -A40 -B8 'pub\(crate\) fn global_member_constructor_name|fn global_member_constructor_name|pub\(crate\) fn global_name_has_user_binding|fn global_name_has_user_binding' crates/perry-hir/src/lower/expr_new_builtins.rs crates/perry-hir/src/lower/expr_new
printf '%s\n' '--- relevant shadowing test ---'
cat -n crates/perry-hir/src/lower/tests/global_this_new_shadowed.rsRepository: PerryTS/perry Length of output: 11529 Guard the worker-messaging fast path on
🤖 Prompt for AI Agents |
||
| return Ok(Some(Expr::New { | ||
| class_name: class_name.to_string(), | ||
| args: lower_optional_args(ctx, new_expr.args.as_deref())?, | ||
|
|
@@ -83,6 +87,7 @@ pub(crate) fn lower_new_member_native( | |
| if obj_name == "globalThis" | ||
| && ctx.lookup_local("globalThis").is_none() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '35,115p' crates/perry-hir/src/lower/expr_new/member.rs
rg -n 'lookup_local\("globalThis"\)|shadows_unqualified_global\("globalThis"\)|fn shadows_unqualified_global|fn lookup_local' crates/perry-hir/src/lower
rg -n 'lower_new_member_native|global_intrinsic_new_once' crates/perry-hir/src/lower/expr_new.rs crates/perry-hir/src/lower/expr_newRepository: PerryTS/perry Length of output: 6704 🏁 Script executed: sed -n '1,125p' crates/perry-hir/src/lower/expr_new/member.rs
sed -n '165,250p' crates/perry-hir/src/lower/expr_new.rs
sed -n '850,1040p' crates/perry-hir/src/lower/context.rs
rg -n 'shadows_unqualified_global|lookup_local_index|add.*binding|declare.*binding|LocalId|Import|Function|Class' crates/perry-hir/src/lower crates/perry-hir/src -g '*.rs' | head -n 220Repository: PerryTS/perry Length of output: 43561 🏁 Script executed: sed -n '1,180p' crates/perry-hir/src/lower/expr_new_builtins.rs
rg -n -A18 -B8 'fn global_name_has_user_binding|fn is_fetch_constructor_name|fn is_reified_global_builtin_constructor|lookup_imported_func|register_import|register_class|register_func' crates/perry-hir/src/lowerRepository: PerryTS/perry Length of output: 50370 🏁 Script executed: rg -n -A14 -B4 'global_name_has_user_binding|is_fetch_constructor_name' crates/perry-hir/src/lower/expr_new.rs crates/perry-hir/src/lower/expr_newRepository: PerryTS/perry Length of output: 15304 Check all bindings that can shadow
🤖 Prompt for AI Agents |
||
| && is_fetch_constructor_name(prop_ident.sym.as_ref()) | ||
| && !global_name_has_user_binding(ctx, prop_ident.sym.as_ref()) | ||
| { | ||
| ctx.uses_fetch = true; | ||
| return Ok(Some(Expr::New { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 14422
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 26953
🏁 Script executed:
rg -n -C 12 'js_get_global_this_builtin_value|populate_global_this_builtins|globalThis.*Event|Event.*globalThis|replace.*global|global.*override' crates/perry-runtime crates/perry-codegen crates/perry/tests test-filesRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 4054
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 28260
Preserve runtime lookup for shadowed global constructors.
When a module binding shadows
Event, this branch callslower_global_intrinsic_newusing only the property name. It bypasses theglobalThis.Eventproperty read required by the HIR lowering contract and test. If code replacesglobalThis.Event,new globalThis.Event()constructs the builtin instead of the installed constructor. Preserve runtime property lookup for qualified global properties, or dispatch intrinsically only when the property is still the builtin.🤖 Prompt for AI Agents