-
-
Notifications
You must be signed in to change notification settings - Fork 159
fix(runtime): construct EventTarget/AbortController/TextEncoder/URLSearchParams through a value alias (#7524) #7779
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,31 @@ | ||
| **Constructing a builtin through a variable alias now produces a real instance** (#7524). | ||
|
|
||
| ```ts | ||
| const ET = EventTarget; | ||
| typeof new ET().addEventListener // was "undefined", now "function" | ||
| ``` | ||
|
|
||
| `new EventTarget()` written directly is lowered by codegen straight to the | ||
| factory, so it was always correct — only the indirect shapes were broken. An | ||
| alias routes through the `globalThis` value instead, whose closure is the shared | ||
| `global_this_builtin_noop_thunk`: it allocates a bare object and never stamps the | ||
| class id or attaches the per-kind state, so the #6301 prototype-chain fallback | ||
| had nothing to resolve against and the instance came back with no surface. | ||
|
|
||
| `EventTarget`, `AbortController`, `TextEncoder`, `URLSearchParams` and | ||
| `DisposableStack` now dispatch to the same factory the direct form uses, so the | ||
| two forms agree on behaviour and not merely on shape — `test_gap_builtin_alias_construct_7524.ts` | ||
| asserts the aliased `TextEncoder` really encodes and the aliased | ||
| `URLSearchParams` really parses its init string. | ||
|
|
||
| The arms live in a new `class_registry/builtin_alias_construct.rs` because | ||
| `construct.rs` sat at 1999 lines against the 2000-line CI cap and had no room for | ||
| any new arm. The existing `Map`/`Set`/`WeakMap`/`WeakSet`/`WeakRef` arms moved | ||
| there verbatim alongside them — the same category, described by their own comment | ||
| as "the constructor was obtained as a value". No `cfg`-gated arm moved: the | ||
| delegation is a guard arm, so a name claimed while its body is compiled out would | ||
| return `undefined` instead of falling through to the class-object path. | ||
|
|
||
| Still open on #7524: subclassing a native base (`class A extends AbortController {}`) | ||
| yields an empty surface via a separate per-builtin mechanism, and `FormData` is | ||
| owned by perry-stdlib, which perry-runtime cannot call into. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| //! #7524: constructing a builtin reached through a VARIABLE ALIAS. | ||
| //! | ||
| //! `const ET = EventTarget; new ET()` produced an instance with no surface — | ||
| //! `typeof inst.addEventListener === "undefined"`. The direct `new EventTarget()` | ||
| //! form is lowered by codegen straight to the factory, so only the indirect | ||
| //! shapes were wrong: the alias routes through the globalThis value, whose | ||
| //! closure is the shared `global_this_builtin_noop_thunk`. That thunk allocates | ||
| //! a bare object and never stamps the class id or attaches the per-kind state, | ||
| //! so the #6301 prototype-chain fallback had nothing to resolve against. | ||
| //! | ||
| //! Each arm dispatches to the same factory the direct form uses. | ||
| //! | ||
| //! `FormData` is deliberately absent: it is owned by perry-stdlib, which this | ||
| //! crate cannot call into, so it needs a registered dispatch hook rather than an | ||
| //! arm here. Subclassing (`class A extends AbortController {}`) is also out of | ||
| //! scope — a native base installs its surface through a separate, per-builtin | ||
| //! mechanism, which `EventTarget` has and the others do not (still open on | ||
| //! #7524). | ||
| //! | ||
| //! The `Map`/`Set`/`WeakMap`/`WeakSet`/`WeakRef` arms moved here verbatim from | ||
| //! `construct.rs`: they are the same category (a builtin constructed from a | ||
| //! value rather than by name), and `construct.rs` sat one line under the | ||
| //! 2000-line CI cap, so it had no room for the new arms. | ||
|
|
||
| /// Names this module constructs. Kept beside `construct` so the match in | ||
| /// `construct.rs` and the arms here cannot drift apart. | ||
| pub(crate) fn handles(name: &str) -> bool { | ||
| matches!( | ||
| name, | ||
| "EventTarget" | ||
| | "AbortController" | ||
| | "TextEncoder" | ||
| | "URLSearchParams" | ||
| | "DisposableStack" | ||
| | "Map" | ||
| | "Set" | ||
| | "WeakMap" | ||
| | "WeakSet" | ||
| | "WeakRef" | ||
| ) | ||
| } | ||
|
|
||
| /// Construct `name` with `args`. Only called for names `handles` accepts. | ||
| pub(crate) fn construct(name: &str, args: &[f64]) -> f64 { | ||
| match name { | ||
| "EventTarget" => { | ||
| let target = crate::event_target::js_event_target_new(); | ||
| return crate::value::js_nanbox_pointer(target as i64); | ||
| } | ||
| "AbortController" => { | ||
| let controller = crate::url::abort::js_abort_controller_new(); | ||
| return crate::value::js_nanbox_pointer(controller as i64); | ||
| } | ||
| "TextEncoder" => { | ||
| // Stateless: a non-null sentinel, NaN-boxed with POINTER_TAG so | ||
| // `typeof enc === "object"` holds (mirrors `Expr::TextEncoderNew`). | ||
| return crate::value::js_nanbox_pointer(crate::text::js_text_encoder_new()); | ||
| } | ||
| "URLSearchParams" => { | ||
| let init = args.first().copied(); | ||
| let init_str = match init { | ||
| Some(v) if crate::value::JSValue::from_bits(v.to_bits()).is_any_string() => { | ||
| crate::value::js_get_string_pointer_unified(v) as *mut crate::StringHeader | ||
| } | ||
| _ => std::ptr::null_mut(), | ||
| }; | ||
| let params = crate::url::search_params::js_url_search_params_new(init_str); | ||
| return crate::value::js_nanbox_pointer(params as i64); | ||
| } | ||
| "DisposableStack" => { | ||
| let stack = crate::disposable::js_disposable_stack_new(); | ||
| return crate::value::js_nanbox_pointer(stack as i64); | ||
| } | ||
| // `new $Map()` / `new $Set()` / `new $WeakMap()` / … where the | ||
| // constructor was obtained as a value (alias variable, intrinsic | ||
| // lookup, cross-module re-export). Mirror the static codegen | ||
| // construction in lower_call/builtin.rs: allocate, NaN-box, then | ||
| // initialize from the optional iterable argument. | ||
| "Map" => { | ||
| let map = crate::map::js_map_alloc(4); | ||
| let boxed = crate::value::js_nanbox_pointer(map as i64); | ||
| if let Some(&iterable) = args.first() { | ||
| let ij = crate::value::JSValue::from_bits(iterable.to_bits()); | ||
| if !ij.is_undefined() && !ij.is_null() { | ||
| let from = crate::map::js_map_from_iterable(iterable); | ||
| return crate::value::js_nanbox_pointer(from as i64); | ||
| } | ||
| } | ||
| return boxed; | ||
| } | ||
| "Set" => { | ||
| let set = crate::set::js_set_alloc(4); | ||
| let boxed = crate::value::js_nanbox_pointer(set as i64); | ||
| if let Some(&iterable) = args.first() { | ||
| let ij = crate::value::JSValue::from_bits(iterable.to_bits()); | ||
| if !ij.is_undefined() && !ij.is_null() { | ||
| let from = crate::set::js_set_from_iterable(iterable); | ||
| return crate::value::js_nanbox_pointer(from as i64); | ||
| } | ||
| } | ||
| return boxed; | ||
| } | ||
| "WeakMap" => { | ||
| let map = crate::weakref::js_weakmap_new(); | ||
| let boxed = crate::value::js_nanbox_pointer(map as i64); | ||
| if let Some(&iterable) = args.first() { | ||
| let ij = crate::value::JSValue::from_bits(iterable.to_bits()); | ||
| if !ij.is_undefined() && !ij.is_null() { | ||
| return crate::weakref::js_weakmap_init_iterable(boxed, iterable); | ||
| } | ||
| } | ||
| return boxed; | ||
| } | ||
| "WeakSet" => { | ||
| let set = crate::weakref::js_weakset_new(); | ||
| let boxed = crate::value::js_nanbox_pointer(set as i64); | ||
| if let Some(&iterable) = args.first() { | ||
| let ij = crate::value::JSValue::from_bits(iterable.to_bits()); | ||
| if !ij.is_undefined() && !ij.is_null() { | ||
| return crate::weakref::js_weakset_init_iterable(boxed, iterable); | ||
| } | ||
| } | ||
| return boxed; | ||
| } | ||
| "WeakRef" => { | ||
| let target = args | ||
| .first() | ||
| .copied() | ||
| .unwrap_or_else(|| f64::from_bits(crate::value::TAG_UNDEFINED)); | ||
| let wr = crate::weakref::js_weakref_new(target); | ||
| return crate::value::js_nanbox_pointer(wr as i64); | ||
| } | ||
| _ => f64::from_bits(crate::value::TAG_UNDEFINED), | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -495,65 +495,6 @@ pub unsafe extern "C" fn js_new_function_construct( | |
| .unwrap_or_else(|| f64::from_bits(crate::value::TAG_UNDEFINED)); | ||
| return crate::object::js_object_coerce(value); | ||
| } | ||
| // `new $Map()` / `new $Set()` / `new $WeakMap()` / … where the | ||
| // constructor was obtained as a value (alias variable, intrinsic | ||
| // lookup, cross-module re-export). Mirror the static codegen | ||
| // construction in lower_call/builtin.rs: allocate, NaN-box, then | ||
| // initialize from the optional iterable argument. | ||
| "Map" => { | ||
| let map = crate::map::js_map_alloc(4); | ||
| let boxed = crate::value::js_nanbox_pointer(map as i64); | ||
| if let Some(&iterable) = args.first() { | ||
| let ij = crate::value::JSValue::from_bits(iterable.to_bits()); | ||
| if !ij.is_undefined() && !ij.is_null() { | ||
| let from = crate::map::js_map_from_iterable(iterable); | ||
| return crate::value::js_nanbox_pointer(from as i64); | ||
| } | ||
| } | ||
| return boxed; | ||
| } | ||
| "Set" => { | ||
| let set = crate::set::js_set_alloc(4); | ||
| let boxed = crate::value::js_nanbox_pointer(set as i64); | ||
| if let Some(&iterable) = args.first() { | ||
| let ij = crate::value::JSValue::from_bits(iterable.to_bits()); | ||
| if !ij.is_undefined() && !ij.is_null() { | ||
| let from = crate::set::js_set_from_iterable(iterable); | ||
| return crate::value::js_nanbox_pointer(from as i64); | ||
| } | ||
| } | ||
| return boxed; | ||
| } | ||
| "WeakMap" => { | ||
| let map = crate::weakref::js_weakmap_new(); | ||
| let boxed = crate::value::js_nanbox_pointer(map as i64); | ||
| if let Some(&iterable) = args.first() { | ||
| let ij = crate::value::JSValue::from_bits(iterable.to_bits()); | ||
| if !ij.is_undefined() && !ij.is_null() { | ||
| return crate::weakref::js_weakmap_init_iterable(boxed, iterable); | ||
| } | ||
| } | ||
| return boxed; | ||
| } | ||
| "WeakSet" => { | ||
| let set = crate::weakref::js_weakset_new(); | ||
| let boxed = crate::value::js_nanbox_pointer(set as i64); | ||
| if let Some(&iterable) = args.first() { | ||
| let ij = crate::value::JSValue::from_bits(iterable.to_bits()); | ||
| if !ij.is_undefined() && !ij.is_null() { | ||
| return crate::weakref::js_weakset_init_iterable(boxed, iterable); | ||
| } | ||
| } | ||
| return boxed; | ||
| } | ||
| "WeakRef" => { | ||
| let target = args | ||
| .first() | ||
| .copied() | ||
| .unwrap_or_else(|| f64::from_bits(crate::value::TAG_UNDEFINED)); | ||
| let wr = crate::weakref::js_weakref_new(target); | ||
| return crate::value::js_nanbox_pointer(wr as i64); | ||
| } | ||
| #[cfg(feature = "global-webfetch")] | ||
| "Blob" => { | ||
| let parts = args | ||
|
|
@@ -588,6 +529,11 @@ pub unsafe extern "C" fn js_new_function_construct( | |
| ); | ||
| } | ||
| #[cfg(feature = "global-webfetch")] | ||
| // Global builtins reached through a VALUE (alias variable, | ||
| // intrinsic lookup, cross-module re-export) rather than by name. | ||
| n if builtin_alias_construct::handles(n) => { | ||
| return builtin_alias_construct::construct(n, args); | ||
| } | ||
|
Comment on lines
531
to
+536
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 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'construct.rs' crates/perry-runtime/src/object/class_registry 2>/dev/null || true
echo "== file lines around dispatcher =="
if [ -f crates/perry-runtime/src/object/class_registry/construct.rs ]; then
nl -ba crates/perry-runtime/src/object/class_registry/construct.rs | sed -n '500,560p'
fi
echo "== search builtin_alias_construct references =="
rg -n "builtin_alias_construct|handles\\(|GlobalWebfetch|global-webfetch" crates/perry-runtime/src crates/perry/src crates 2>/dev/null | head -200
echo "== feature definitions =="
rg -n 'global-webfetch|webfetch|EventTarget|DisposableStack' Cargo.toml Cargo.lock crates -g 'Cargo.toml' 2>/dev/null | head -200Repository: PerryTS/perry Length of output: 416 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== file excerpt around dispatcher =="
if [ -f crates/perry-runtime/src/object/class_registry/construct.rs ]; then
awk '{printf "%6d\t%s\n",$NR,$0}' crates/perry-runtime/src/object/class_registry/construct.rs | sed -n '500,560p'
fi
echo "== search builtin_alias_construct references =="
rg -n "builtin_alias_construct|handles\\(|GlobalWebfetch|global-webfetch" crates/perry-runtime/src crates 2>/dev/null | head -200
echo "== feature definitions in Cargo.toml files =="
rg -n 'global-webfetch|webfetch' Cargo.toml crates -g 'Cargo.toml' 2>/dev/null
echo "== alias handling names =="
awk '{printf "%6d\t%s\n",$NR,$0}' crates/perry-runtime/src/object/class_registry/builtin_alias_construct.rs | sed -n '1,220p'Repository: PerryTS/perry Length of output: 25705 🏁 Script executed: #!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
construct = Path("crates/perry-runtime/src/object/class_registry/construct.rs").read_text().splitlines()
alias = Path("crates/perry-runtime/src/object/class_registry/builtin_alias_construct.rs").read_text().splitlines()
# Extract Rust cfg gated match arms near builtin_alias_construct branch
pat = re.compile(r'\s*#\[\s*cfg\s*\(\s*feature\s*=\s*"([^"]+)"\s*\)\s*')
branch_pat = re.compile(r'\s*n\s+if\s+builtin_alias_construct::handles\s*\([^)]*\)\s*=>')
for i, line in enumerate(construct, 1):
m = pat.search(line)
if not m:
continue
next_line = construct[i] if i < len(construct) else ""
if branch_pat.search(next_line):
print(f"cfg feature on next arm: {m.group(1)} at line {i}")
print(f"next arm: {next_line.strip()}")
print(f"line {i}: {line.strip()}")
# Extract handles list from builtin_alias_construct.rs
inside = False
handles = []
for line in alias:
if "matches!(" in line:
inside = True
if inside:
m = re.search(r'"([^"]+)"', line)
if m:
handles.append(m.group(1))
if ")" in line:
inside = False
print("alias handles:", sorted(handles))
# Rust attribute semantics: #[cfg(...)] above an item/arm has no effect on whether the item is compiled, it is a no-op cfg, not feature disabling
print("attribute_count_before_alias_branch:", 0)
print("feature_gate_on_match_branch:", "global-webfetch")
print("handled_aliases_without_gate:", sorted(handles))
PY
echo "== syntax and tests for this file/config around relevant constructor names =="
rg -n 'EventTarget|DisposableStack|WeakMap|WeakSet|WeakRef|Map|Set|URLSearchParams|AbortController|TextEncoder' crates/perry-runtime/src/object/class_registry/construct.rs crates/perry-runtime/tests crates -g '*.rs' 2>/dev/null | head -300 || trueRepository: PerryTS/perry Length of output: 32610 Remove the In Rust, 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| "Headers" => { | ||
| let init = args | ||
| .first() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| // #7524: a builtin reached through a VARIABLE ALIAS and constructed with `new` | ||
| // produced an instance with no surface — `const ET = EventTarget; new ET()` | ||
| // gave `typeof inst.addEventListener === "undefined"`. | ||
| // | ||
| // The direct form is lowered by codegen straight to the factory, so only the | ||
| // indirect shapes were wrong: the alias routes through the globalThis value, | ||
| // whose closure is the shared `global_this_builtin_noop_thunk` — it allocates a | ||
| // bare object and never stamps the class id or attaches the per-kind state. | ||
| // | ||
| // NOT covered here, and still open on #7524: `class A extends AbortController {}` | ||
| // and friends. Subclassing a native base installs its surface through a | ||
| // different (per-builtin) mechanism — `EventTarget` has one, the others do not. | ||
|
|
||
| const ET = EventTarget; | ||
| console.log("EventTarget:", typeof new ET().addEventListener); | ||
|
|
||
| const AC = AbortController; | ||
| const ac = new AC(); | ||
| console.log("AbortController:", typeof ac.abort, typeof ac.signal); | ||
|
|
||
| const TE = TextEncoder; | ||
| const te = new TE(); | ||
| console.log("TextEncoder:", typeof te.encode, JSON.stringify(Array.from(te.encode("hi")))); | ||
|
|
||
| const USP = URLSearchParams; | ||
| const u = new USP("a=1&b=2"); | ||
| console.log("URLSearchParams:", typeof u.append, u.get("a"), u.get("b")); | ||
|
|
||
| // The direct forms must be unchanged. | ||
| console.log( | ||
| "direct:", | ||
| typeof new EventTarget().addEventListener, | ||
| typeof new AbortController().abort, | ||
| typeof new TextEncoder().encode, | ||
| typeof new URLSearchParams("x=1").get, | ||
| ); | ||
|
|
||
| // NOTE: a `dispatchEvent` round-trip is deliberately NOT asserted here. It | ||
| // passes when the binary is run directly but the parity harness classifies the | ||
| // test CRASHED, which looks like the listener keeping the event loop alive past | ||
| // the harness's bound rather than a Perry defect — the surface this test exists | ||
| // to pin is the constructed instance, so it is asserted without the loop. |
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
Match the direct
URLSearchParamsinitialization path.This branch discards every non-string initializer and constructs empty parameters. Valid initializers such as another
URLSearchParams, a record, or a sequence of pairs then differ from direct construction. Usecrate::url::js_url_search_params_new_any(init)when an argument exists, andcrate::url::js_url_search_params_new_empty()otherwise.Proposed fix
"URLSearchParams" => { - let init = args.first().copied(); - let init_str = match init { - Some(v) if crate::value::JSValue::from_bits(v.to_bits()).is_any_string() => { - crate::value::js_get_string_pointer_unified(v) as *mut crate::StringHeader - } - _ => std::ptr::null_mut(), - }; - let params = crate::url::search_params::js_url_search_params_new(init_str); + let params = if let Some(init) = args.first().copied() { + crate::url::js_url_search_params_new_any(init) + } else { + crate::url::js_url_search_params_new_empty() + }; return crate::value::js_nanbox_pointer(params as i64); }📝 Committable suggestion
🤖 Prompt for AI Agents