fix(hir): #5912 — new URL()/TextEncoder()/etc. ignore lexical shadowing#5913
Open
proggeramlug wants to merge 1 commit into
Open
fix(hir): #5912 — new URL()/TextEncoder()/etc. ignore lexical shadowing#5913proggeramlug wants to merge 1 commit into
proggeramlug wants to merge 1 commit into
Conversation
…shadowing
`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.
📝 WalkthroughWalkthroughThis PR fixes Issue ChangesShadow-aware dispatch for built-in constructors
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/perry-hir/src/lower/expr_call/static_receiver.rs (1)
52-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSkip the shadow check for
new globalThis.URL()
globalThis.URLstill flows through the same shadowing guard as bareURL, so a localURLbinding forcesnew globalThis.URL(...)to classify as"Object"and miss the native URL fast path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-hir/src/lower/expr_call/static_receiver.rs` around lines 52 - 80, The shadowing guard in static receiver classification is too broad and incorrectly treats new globalThis.URL() like a locally shadowed URL binding. Update the logic in the static receiver lowering path around the class_name extraction and lookup checks so that the shadow check only applies to bare identifiers, not to Member expressions rooted at globalThis. Preserve the native fast path for globalThis.URL while still returning "Object" for truly shadowed local names.crates/perry-hir/src/lower/expr_new.rs (2)
944-1036: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard these constructor fast paths with imported-binding shadowing. Imported names like
URLorTextEncodercan still take the native path here; useshadows_unqualified_global(and the same gate forWeakRef/FinalizationRegistry) instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-hir/src/lower/expr_new.rs` around lines 944 - 1036, The constructor fast paths in the lowering logic still ignore imported bindings, so names like URL and TextEncoder can incorrectly take the native path even when shadowed. Update the checks in the new-expression handling branch to use shadows_unqualified_global alongside the existing callee_local_at_entry/lookup_func/lookup_class guards, and apply the same imported-binding shadowing gate to the WeakRef and FinalizationRegistry special cases in expr_new::lower_expr.
419-427: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlias-based URL/encoding constructors still need the shadowing guard
resolve_class_alias()can remap a local alias likeMyURLback toURL, and this early branch returns before the latercallee_local_at_entry/lookup_func/lookup_classchecks. That letsnew MyURL(...)bind to the native constructor even whenURLis shadowed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-hir/src/lower/expr_new.rs` around lines 419 - 427, The early alias-resolution branch in expr_new’s constructor lowering bypasses the shadowing check, so a local alias like MyURL can still resolve to the native URL/encoding constructor even when URL is shadowed. Update the logic around resolve_class_alias, is_url_encoding_constructor_name, and lower_url_encoding_constructor so it still consults the callee_local_at_entry / lookup_func / lookup_class shadowing guard before returning the lowered expression, or otherwise fold the alias case into the same guarded path.
🧹 Nitpick comments (1)
crates/perry-hir/src/lower/expr_new.rs (1)
953-1036: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated 3-condition guard across four branches.
The same
callee_local_at_entry.is_none() && ctx.lookup_func(...).is_none() && ctx.lookup_class(...).is_none()triplet is duplicated for URL, URLSearchParams/URLPattern, TextEncoder, and TextDecoder. Consider a small helper (e.g.fn is_shadowed(ctx, callee_local_at_entry, name) -> bool) to reduce duplication and keep future edits (e.g. addinglookup_imported_func) in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-hir/src/lower/expr_new.rs` around lines 953 - 1036, The same shadowing check is duplicated in the constructor lowering branches for URL, URLSearchParams/URLPattern, TextEncoder, and TextDecoder. Add a small helper in expr_new.rs, such as one used from lower_new_expr/lower_url_encoding_constructor call sites, to centralize the callee_local_at_entry plus ctx.lookup_func/ctx.lookup_class checks. Replace the repeated guard with that helper so future changes to shadowing rules only need to be made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-hir/src/lower/expr_call/static_receiver.rs`:
- Around line 75-80: The static receiver guard in `StaticReceiver::lower` is
reimplementing shadowing logic manually and misses imported functions, so
replace the combined `lookup_local`/`lookup_func`/`lookup_class` checks with
`ctx.shadows_unqualified_global(class_name)` to match the existing semantics in
`Context`. This keeps the behavior consistent with `context.rs` and ensures
imported bindings are also treated as shadows when resolving the class name.
---
Outside diff comments:
In `@crates/perry-hir/src/lower/expr_call/static_receiver.rs`:
- Around line 52-80: The shadowing guard in static receiver classification is
too broad and incorrectly treats new globalThis.URL() like a locally shadowed
URL binding. Update the logic in the static receiver lowering path around the
class_name extraction and lookup checks so that the shadow check only applies to
bare identifiers, not to Member expressions rooted at globalThis. Preserve the
native fast path for globalThis.URL while still returning "Object" for truly
shadowed local names.
In `@crates/perry-hir/src/lower/expr_new.rs`:
- Around line 944-1036: The constructor fast paths in the lowering logic still
ignore imported bindings, so names like URL and TextEncoder can incorrectly take
the native path even when shadowed. Update the checks in the new-expression
handling branch to use shadows_unqualified_global alongside the existing
callee_local_at_entry/lookup_func/lookup_class guards, and apply the same
imported-binding shadowing gate to the WeakRef and FinalizationRegistry special
cases in expr_new::lower_expr.
- Around line 419-427: The early alias-resolution branch in expr_new’s
constructor lowering bypasses the shadowing check, so a local alias like MyURL
can still resolve to the native URL/encoding constructor even when URL is
shadowed. Update the logic around resolve_class_alias,
is_url_encoding_constructor_name, and lower_url_encoding_constructor so it still
consults the callee_local_at_entry / lookup_func / lookup_class shadowing guard
before returning the lowered expression, or otherwise fold the alias case into
the same guarded path.
---
Nitpick comments:
In `@crates/perry-hir/src/lower/expr_new.rs`:
- Around line 953-1036: The same shadowing check is duplicated in the
constructor lowering branches for URL, URLSearchParams/URLPattern, TextEncoder,
and TextDecoder. Add a small helper in expr_new.rs, such as one used from
lower_new_expr/lower_url_encoding_constructor call sites, to centralize the
callee_local_at_entry plus ctx.lookup_func/ctx.lookup_class checks. Replace the
repeated guard with that helper so future changes to shadowing rules only need
to be made in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: df79d016-2d59-4b62-8123-8544468b2762
📒 Files selected for processing (3)
crates/perry-hir/src/lower/expr_call/static_receiver.rscrates/perry-hir/src/lower/expr_new.rstest-files/test_issue_5912_url_new_local_shadow.ts
Comment on lines
+75
to
+80
| 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"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reuse ctx.shadows_unqualified_global instead of duplicating the check.
context.rs already defines shadows_unqualified_global combining lookup_local/lookup_func/lookup_imported_func/lookup_class. This new guard reimplements three of those four checks manually and omits lookup_imported_func — so a same-named binding imported from another module (import { URL } from './polyfill') would not be caught here, unlike the intended shadowing semantics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-hir/src/lower/expr_call/static_receiver.rs` around lines 75 -
80, The static receiver guard in `StaticReceiver::lower` is reimplementing
shadowing logic manually and misses imported functions, so replace the combined
`lookup_local`/`lookup_func`/`lookup_class` checks with
`ctx.shadows_unqualified_global(class_name)` to match the existing semantics in
`Context`. This keeps the behavior consistent with `context.rs` and ensures
imported bindings are also treated as shadows when resolving the class name.
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.
Fixes #5912.
Problem
new URL(...)(andURLSearchParams/URLPattern/TextEncoder/TextDecoder) were dispatched by bare identifier name inexpr_new.rswith 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 onlookup_local/lookup_func/lookup_class.Real packages ship their own tolerant polyfills under these names — found via a real-world source compile of
@mixmark-io/domino'slib/URL.js, which definesfunction URL(url) { ... }and internally callsnew URL()with zero args against its own constructor. Perry routed that straight into the native WHATWG URL constructor instead, which requires at least one argument:Second site
The same unguarded name match also existed independently in
static_receiver_class(expr_call/static_receiver.rs), which decides whether.toString()/.toJSON()/JSON.stringify(...)on a receiver should route through the native Date/URL fast paths. It classified anynew URL(...)expression — or a local whose inferred type name is"URL"— as the native type purely by name, so even after fixing the construction site, printing/serializing a shadowed instance still silently substituted native URL serialization (UrlInstanceToJSON/UrlInstanceToString) for the user's own object.Fix
Both sites now check
lookup_local/lookup_func/lookup_classbefore applying the native lowering/classification, matching the existing shadowing-guard pattern already used forFunction/Objectin the same file.Testing
test-files/test_issue_5912_url_new_local_shadow.ts(function-shadow + nested-function-shadow cases), output verified byte-for-byte againstnode --experimental-strip-types.class URL { ... }shadow (construction +.toString()+JSON.stringify()) matches Node.new URL(...)usage (hostname/pathname/toString/toJSON/JSON.stringify) is unaffected.test-files/test_gap_url_*/test_compat_url_date_math.ts/shadowing-related gap tests either pass or fail identically with and without this change (those failures are pre-existing linker/runtime gaps unrelated to this fix, reproduced on unmodifiedmaintoo).🤖 Generated with Claude Code
Summary by CodeRabbit
URL,TextEncoder, and related names are treated correctly.newexpressions fall back to generic behavior when a matching name is defined in user code.