From 9308eeea33d5e36857ec30c6ae1ed3aeae0e3a16 Mon Sep 17 00:00:00 2001 From: Ralph Date: Fri, 3 Jul 2026 09:59:52 -0700 Subject: [PATCH 1/2] =?UTF-8?q?fix(hir):=20#5912=20=E2=80=94=20new=20URL()?= =?UTF-8?q?/TextEncoder()/etc.=20ignore=20lexical=20shadowing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `new URL(...)` (and URLSearchParams/URLPattern/TextEncoder/TextDecoder) were dispatched by bare identifier name with no check for whether the name actually resolves to the global constructor or is shadowed by a local function/class — unlike sibling well-known constructors (Function, Object) in the same match, which already guard on lookup_local/lookup_func/lookup_class. Real packages ship their own tolerant polyfills under these names (found via @mixmark-io/domino's lib/URL.js, which defines `function URL(url) { ... }` and calls `new URL()` with zero args against its own constructor) and hit perry's native WHATWG URL constructor instead, which requires at least one argument. The same unguarded name match also existed in a second, independent spot: `static_receiver_class` (used to decide whether `.toString()` / `.toJSON()` / `JSON.stringify()` on a receiver should route through the native Date/URL fast paths) classified any `new URL(...)` — or a local typed as `URL` — as the native type by name alone, so even after fixing the construction site, printing/serializing a shadowed instance still silently substituted native URL output. Fixes both sites by checking `lookup_local`/`lookup_func`/ `lookup_class` before applying the native lowering, matching the existing shadowing-guard pattern used elsewhere in expr_new.rs. --- .../src/lower/expr_call/static_receiver.rs | 22 ++++++++++++ crates/perry-hir/src/lower/expr_new.rs | 34 ++++++++++++++++--- .../test_issue_5912_url_new_local_shadow.ts | 26 ++++++++++++++ 3 files changed, 77 insertions(+), 5 deletions(-) create mode 100644 test-files/test_issue_5912_url_new_local_shadow.ts diff --git a/crates/perry-hir/src/lower/expr_call/static_receiver.rs b/crates/perry-hir/src/lower/expr_call/static_receiver.rs index cbcd310d2b..f720507d21 100644 --- a/crates/perry-hir/src/lower/expr_call/static_receiver.rs +++ b/crates/perry-hir/src/lower/expr_call/static_receiver.rs @@ -64,6 +64,20 @@ pub(super) fn static_receiver_class( _ => None, }; if let Some(class_name) = class_name { + // Issue #5912: a local function/const/class shadowing one of the + // well-known names below (e.g. a vendored `function URL(url) { + // ... }` polyfill) is the user's own value, never perry's native + // built-in — classify as a generic "Object" (skips the ambiguous + // Date/URL method arms, same treatment as an object-literal + // receiver below) instead of misrouting `.toString()`/`.toJSON()` + // through the native fast paths (`UrlInstanceToJSON` etc.) on a + // value that was never actually constructed via the native path. + if ctx.lookup_local(class_name).is_some() + || ctx.lookup_func(class_name).is_some() + || ctx.lookup_class(class_name).is_some() + { + return Some("Object"); + } let resolved_class = ctx .resolve_class_alias(class_name) .unwrap_or_else(|| class_name.to_string()); @@ -139,6 +153,14 @@ pub(super) fn static_receiver_class( _ => None, }; if let Some(n) = named { + // Issue #5912: the local's inferred type name can legitimately + // be "URL" because it holds an instance of a REAL user class + // named `URL` (shadowing the global) — not perry's native + // WHATWG URL. Route those through generic dispatch too, same + // as the `New`-expression branch above. + if ctx.lookup_class(n).is_some() { + return Some("Object"); + } return match n { "Date" => Some("Date"), "URL" => Some("URL"), diff --git a/crates/perry-hir/src/lower/expr_new.rs b/crates/perry-hir/src/lower/expr_new.rs index b30fb587b6..ad675d5ce7 100644 --- a/crates/perry-hir/src/lower/expr_new.rs +++ b/crates/perry-hir/src/lower/expr_new.rs @@ -941,15 +941,31 @@ pub(super) fn lower_new(ctx: &mut LoweringContext, new_expr: &ast::NewExpr) -> R } } - // Handle URL class - if class_name == "URL" { + // Handle URL class. #5912: gated on `callee_local_at_entry` / + // `lookup_func` so a local function/const shadowing the global + // name (e.g. a vendored `function URL(url?) {...}` polyfill) + // routes through the generic local-dispatch fallback below + // instead of always binding to perry's native WHATWG URL + // constructor — matches the `lookup_local`/`lookup_func`/ + // `lookup_class` shadowing guard used for `Function`/`Object` + // above (a named function declaration is tracked via + // `lookup_func`, not `lookup_local`/`callee_local_at_entry`). + if class_name == "URL" + && callee_local_at_entry.is_none() + && ctx.lookup_func(&class_name).is_none() + && ctx.lookup_class(&class_name).is_none() + { return Ok( lower_url_encoding_constructor(ctx, "URL", new_expr.args.as_deref())?.unwrap(), ); } // Handle URLSearchParams / URLPattern classes - if matches!(class_name.as_str(), "URLSearchParams" | "URLPattern") { + if matches!(class_name.as_str(), "URLSearchParams" | "URLPattern") + && callee_local_at_entry.is_none() + && ctx.lookup_func(&class_name).is_none() + && ctx.lookup_class(&class_name).is_none() + { return Ok(lower_url_encoding_constructor( ctx, &class_name, @@ -993,7 +1009,11 @@ pub(super) fn lower_new(ctx: &mut LoweringContext, new_expr: &ast::NewExpr) -> R return Ok(Expr::FinalizationRegistryNew(Box::new(cb))); } // Handle TextEncoder constructor - if class_name == "TextEncoder" { + if class_name == "TextEncoder" + && callee_local_at_entry.is_none() + && ctx.lookup_func(&class_name).is_none() + && ctx.lookup_class(&class_name).is_none() + { return Ok(lower_url_encoding_constructor( ctx, "TextEncoder", @@ -1002,7 +1022,11 @@ pub(super) fn lower_new(ctx: &mut LoweringContext, new_expr: &ast::NewExpr) -> R .unwrap()); } // Handle TextDecoder constructor: new TextDecoder(label?, opts?) - if class_name == "TextDecoder" { + if class_name == "TextDecoder" + && callee_local_at_entry.is_none() + && ctx.lookup_func(&class_name).is_none() + && ctx.lookup_class(&class_name).is_none() + { return Ok(lower_url_encoding_constructor( ctx, "TextDecoder", diff --git a/test-files/test_issue_5912_url_new_local_shadow.ts b/test-files/test_issue_5912_url_new_local_shadow.ts new file mode 100644 index 0000000000..5b4c8da88c --- /dev/null +++ b/test-files/test_issue_5912_url_new_local_shadow.ts @@ -0,0 +1,26 @@ +// Issue #5912 — `new URL(...)` (and URLSearchParams/URLPattern/TextEncoder/ +// TextDecoder) were dispatched by bare identifier name with no check for +// whether the name is actually the global constructor or shadowed by a +// local function/class. Real packages ship their own tolerant `URL` +// polyfill (e.g. @mixmark-io/domino's lib/URL.js calls `new URL()` with +// zero args against ITS OWN constructor) and hit perry's native URL +// constructor instead, which requires at least one argument. +// +// This exercises the exact shadowing shape: a local function named `URL` +// that tolerates a missing argument, matching Node's output. + +function URL(url?: string) { + return { url: url ?? "default", kind: "local" }; +} + +console.log(JSON.stringify(new URL())); +console.log(JSON.stringify(new URL("explicit"))); + +function withTextEncoder() { + function TextEncoder(label?: string) { + return { label: label ?? "utf-8", kind: "local" }; + } + return new TextEncoder(); +} + +console.log(JSON.stringify(withTextEncoder())); From 58765b21f4ca100a83c1ac3a3cb108505c954be8 Mon Sep 17 00:00:00 2001 From: Ralph Date: Fri, 3 Jul 2026 13:39:53 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix(hir):=20#5912=20CodeRabbit=20follow-up?= =?UTF-8?q?=20=E2=80=94=20globalThis=20escape=20hatch,=20alias,=20imported?= =?UTF-8?q?=20bindings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three issues from CodeRabbit's review of the initial fix: 1. Real regression: the static_receiver_class shadowing guard collapsed `new globalThis.URL(...)` and bare `new URL(...)` into the same class_name capture, so a locally-shadowed URL made even the explicit globalThis-qualified form misclassify as "Object" and lose the native fast path. Track which callee shape matched and only apply the shadow check to the bare-identifier form. 2. Coverage gap: `const MyURL = URL; new MyURL()` bypassed the shadowing guard entirely via the resolve_class_alias branch, which runs before the later callee_local_at_entry/lookup_func/lookup_class checks. Added the same shadowing check there, against the ALIAS-RESOLVED name (aliases are name-keyed, not scope-aware). 3. Coverage gap: an imported binding shadowing one of these names (`import { URL } from "./polyfill"`) wasn't covered by the lookup_local/lookup_func/lookup_class triplet. Added lookup_imported_func alongside it in expr_new.rs (module-level registry, safe to query fresh — unlike lookup_local, this one isn't subject to the scope-stack-disturbance issue callee_local_at_entry exists to avoid, so no special snapshotting needed). static_receiver.rs's own guard now calls the existing `shadows_unqualified_global` helper (already covers all four cases) instead of reimplementing three of them by hand. Extended the regression test with both the globalThis-escape-hatch and alias cases; both match node byte-for-byte. --- .../src/lower/expr_call/static_receiver.rs | 43 +++++++++++-------- crates/perry-hir/src/lower/expr_new.rs | 38 ++++++++++++---- .../test_issue_5912_url_new_local_shadow.ts | 11 +++++ 3 files changed, 66 insertions(+), 26 deletions(-) diff --git a/crates/perry-hir/src/lower/expr_call/static_receiver.rs b/crates/perry-hir/src/lower/expr_call/static_receiver.rs index f720507d21..76abdffcfb 100644 --- a/crates/perry-hir/src/lower/expr_call/static_receiver.rs +++ b/crates/perry-hir/src/lower/expr_call/static_receiver.rs @@ -50,32 +50,41 @@ pub(super) fn static_receiver_class( } } if let ast::Expr::New(new_expr) = obj { - let class_name = match new_expr.callee.as_ref() { - ast::Expr::Ident(ident) => Some(ident.sym.as_ref()), + // Issue #5912 (CodeRabbit follow-up): `new globalThis.URL(...)` + // always reaches the REAL global regardless of any local `URL` + // shadowing — that's the entire point of the explicit `globalThis.` + // qualifier. Track which callee shape matched instead of collapsing + // both into one `class_name` capture; the original version fed both + // forms into the same shadow check below, incorrectly downgrading + // `new globalThis.URL()` to "Object" whenever a local `URL` shadowed + // the bare name. + let (class_name, is_global_qualified) = match new_expr.callee.as_ref() { + ast::Expr::Ident(ident) => (Some(ident.sym.as_ref()), false), ast::Expr::Member(member) if matches!(member.obj.as_ref(), ast::Expr::Ident(obj) if obj.sym.as_ref() == "globalThis") && ctx.lookup_local("globalThis").is_none() => { match &member.prop { - ast::MemberProp::Ident(prop) => Some(prop.sym.as_ref()), - _ => None, + ast::MemberProp::Ident(prop) => (Some(prop.sym.as_ref()), true), + _ => (None, false), } } - _ => None, + _ => (None, false), }; if let Some(class_name) = class_name { - // Issue #5912: a local function/const/class shadowing one of the - // well-known names below (e.g. a vendored `function URL(url) { - // ... }` polyfill) is the user's own value, never perry's native - // built-in — classify as a generic "Object" (skips the ambiguous - // Date/URL method arms, same treatment as an object-literal - // receiver below) instead of misrouting `.toString()`/`.toJSON()` - // through the native fast paths (`UrlInstanceToJSON` etc.) on a - // value that was never actually constructed via the native path. - if ctx.lookup_local(class_name).is_some() - || ctx.lookup_func(class_name).is_some() - || ctx.lookup_class(class_name).is_some() - { + // Issue #5912: a local function/const/class/imported-binding + // (`shadows_unqualified_global` covers all four — see #5912 + // review follow-up) shadowing one of the well-known names below + // (e.g. a vendored `function URL(url) { ... }` polyfill) is the + // user's own value, never perry's native built-in — classify as + // a generic "Object" (skips the ambiguous Date/URL method arms, + // same treatment as an object-literal receiver below) instead of + // misrouting `.toString()`/`.toJSON()` through the native fast + // paths (`UrlInstanceToJSON` etc.) on a value that was never + // actually constructed via the native path. Doesn't apply to the + // `globalThis.X` form above — see the comment on + // `is_global_qualified`. + if !is_global_qualified && ctx.shadows_unqualified_global(class_name) { return Some("Object"); } let resolved_class = ctx diff --git a/crates/perry-hir/src/lower/expr_new.rs b/crates/perry-hir/src/lower/expr_new.rs index ad675d5ce7..0a98ea01cb 100644 --- a/crates/perry-hir/src/lower/expr_new.rs +++ b/crates/perry-hir/src/lower/expr_new.rs @@ -416,8 +416,16 @@ pub(super) fn lower_new(ctx: &mut LoweringContext, new_expr: &ast::NewExpr) -> R } } + // Issue #5912 (CodeRabbit follow-up): an alias like + // `const MyURL = URL; new MyURL()` must not bind to the native + // constructor either when the ALIASED name is itself shadowed + // (`function URL(url) {...}` in scope) — `resolve_class_alias` + // is name-keyed and not scope-aware, so it happily maps + // `MyURL` -> `"URL"` without knowing `URL` was ever shadowed. if let Some(resolved) = ctx.resolve_class_alias(&class_name) { - if is_url_encoding_constructor_name(&resolved) { + if is_url_encoding_constructor_name(&resolved) + && !ctx.shadows_unqualified_global(&resolved) + { if let Some(expr) = lower_url_encoding_constructor(ctx, &resolved, new_expr.args.as_deref())? { @@ -942,17 +950,26 @@ pub(super) fn lower_new(ctx: &mut LoweringContext, new_expr: &ast::NewExpr) -> R } // Handle URL class. #5912: gated on `callee_local_at_entry` / - // `lookup_func` so a local function/const shadowing the global - // name (e.g. a vendored `function URL(url?) {...}` polyfill) - // routes through the generic local-dispatch fallback below - // instead of always binding to perry's native WHATWG URL - // constructor — matches the `lookup_local`/`lookup_func`/ - // `lookup_class` shadowing guard used for `Function`/`Object` - // above (a named function declaration is tracked via - // `lookup_func`, not `lookup_local`/`callee_local_at_entry`). + // `lookup_func` / `lookup_imported_func` so a local function/ + // const/imported-binding shadowing the global name (e.g. a + // vendored `function URL(url?) {...}` polyfill, or `import { + // URL } from "./my-url-polyfill"`) routes through the generic + // local-dispatch fallback below instead of always binding to + // perry's native WHATWG URL constructor — matches the + // `lookup_local`/`lookup_func`/`lookup_class` shadowing guard + // used for `Function`/`Object` above (a named function + // declaration is tracked via `lookup_func`, not + // `lookup_local`/`callee_local_at_entry`). Deliberately doesn't + // use the `shadows_unqualified_global` one-liner here: that + // helper's `lookup_local` call is a FRESH scope lookup, but + // `callee_local_at_entry` must stay a pre-captured snapshot (see + // the comment above its definition) — the Error-type branch + // just above already lowers `new_expr.args`, which can disturb + // the locals scope stack before we get here. if class_name == "URL" && callee_local_at_entry.is_none() && ctx.lookup_func(&class_name).is_none() + && ctx.lookup_imported_func(&class_name).is_none() && ctx.lookup_class(&class_name).is_none() { return Ok( @@ -964,6 +981,7 @@ pub(super) fn lower_new(ctx: &mut LoweringContext, new_expr: &ast::NewExpr) -> R if matches!(class_name.as_str(), "URLSearchParams" | "URLPattern") && callee_local_at_entry.is_none() && ctx.lookup_func(&class_name).is_none() + && ctx.lookup_imported_func(&class_name).is_none() && ctx.lookup_class(&class_name).is_none() { return Ok(lower_url_encoding_constructor( @@ -1012,6 +1030,7 @@ pub(super) fn lower_new(ctx: &mut LoweringContext, new_expr: &ast::NewExpr) -> R if class_name == "TextEncoder" && callee_local_at_entry.is_none() && ctx.lookup_func(&class_name).is_none() + && ctx.lookup_imported_func(&class_name).is_none() && ctx.lookup_class(&class_name).is_none() { return Ok(lower_url_encoding_constructor( @@ -1025,6 +1044,7 @@ pub(super) fn lower_new(ctx: &mut LoweringContext, new_expr: &ast::NewExpr) -> R if class_name == "TextDecoder" && callee_local_at_entry.is_none() && ctx.lookup_func(&class_name).is_none() + && ctx.lookup_imported_func(&class_name).is_none() && ctx.lookup_class(&class_name).is_none() { return Ok(lower_url_encoding_constructor( diff --git a/test-files/test_issue_5912_url_new_local_shadow.ts b/test-files/test_issue_5912_url_new_local_shadow.ts index 5b4c8da88c..171774628c 100644 --- a/test-files/test_issue_5912_url_new_local_shadow.ts +++ b/test-files/test_issue_5912_url_new_local_shadow.ts @@ -24,3 +24,14 @@ function withTextEncoder() { } console.log(JSON.stringify(withTextEncoder())); + +// CodeRabbit follow-up on the #5913 PR — an explicit `globalThis.` qualifier +// is an escape hatch to the REAL global and must keep working even while the +// bare `URL` identifier is shadowed above. +console.log(new (globalThis as any).URL("https://example.com/path").hostname); + +// CodeRabbit follow-up — a local alias of the shadowed name must not resolve +// back to the native constructor either (`resolve_class_alias` is name-keyed, +// not scope-aware, so this needs its own explicit check). +const MyURL = URL; +console.log(JSON.stringify(new MyURL("via-alias")));