From 6c702dcd592209a7aab38a4ef0e29e2622d09a37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 04:27:02 +0200 Subject: [PATCH 1/2] fix(hir): a local named like a proxy local is not a proxy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `proxy_locals` is a BARE-NAME `HashSet`, collected by a module-wide pre-scan with no scope discrimination, and `expr_member` lowers `.prop` to `Expr::ProxyGet` for ANY function that uses that name. `js_proxy_get` on a non-proxy answers `undefined`, so function readLoop() { const a = build(10); … a.length … } // undefined function neverCalled() { const a = new Proxy(raw, {}) as any; … } made `readLoop`'s `for (i = 0; i < a.length; i++)` run ZERO iterations. The proxy function never had to be called, the proxy did not have to be over the same array, and it did not have to be an array at all — only the NAME had to collide. Renaming the proxy local is what proves it. TWO DEFECTS, both in the `poison`-set remedy this file already documents for exactly this hazard class: 1. `poison` fired only for a colliding `new ()`. A name bound to a CALL — `const a = build(10)`, the overwhelmingly common shape — never poisoned anything. Call/await initializers now do. 2. The subtraction removed poisoned names from `weakmap_locals` and `weakset_locals` only: two of the five sets it feeds. So even the covered case left `proxy_locals` wrong — a colliding `new Other()` made that object's `a.v` read `undefined`, which the new test pins. THE COMMENT ARGUING AGAINST (2) WAS WRONG, AND MEASURABLY SO. It weighed a lost codegen fast path against "no upside", reasoning about METHOD dispatch (`.deref()`, `.register()`), where a poisoned name costs speed. A PROPERTY read is the other half: there the fallback is correct and what a poisoned name buys back is a wrong answer. Settled by fixture rather than argument — a genuine, actually-used proxy with a poisoned name still returns 42 from its `get` trap, and its `has` trap still works. The new test asserts BOTH directions, because a fix that broke proxies instead would otherwise "pass": the victim reads, and a real proxy under a colliding name, a uniquely-named proxy with get+set traps, and a `has` trap. Fixes #7775 --- crates/perry-hir/src/lower/pre_scan.rs | 26 ++++ ...est_gap_proxy_local_name_collision_7775.ts | 126 ++++++++++++++++++ 2 files changed, 152 insertions(+) create mode 100644 test-files/test_gap_proxy_local_name_collision_7775.ts diff --git a/crates/perry-hir/src/lower/pre_scan.rs b/crates/perry-hir/src/lower/pre_scan.rs index 66d1ae75ed..8680a4309c 100644 --- a/crates/perry-hir/src/lower/pre_scan.rs +++ b/crates/perry-hir/src/lower/pre_scan.rs @@ -95,6 +95,21 @@ pub(crate) fn pre_scan_weakref_locals(ast_module: &ast::Module, ctx: &mut Loweri } _ => {} } + } else if matches!(init_unwrapped, ast::Expr::Call(_) | ast::Expr::Await(_)) { + // #7775: a name bound to a CALL result is just as ambiguous as + // one bound to `new ()`, and it was not poisoned at + // all. `const a = build(10)` in one function and + // `const a = new Proxy(raw, {})` in another made the FIRST + // function's `a.length` lower to `js_proxy_get` on a plain + // array — `undefined`, so the read loop after it ran zero + // iterations. The proxy function never even had to be called. + // + // Deliberately narrow: only call/await initializers, which are + // the opaque ones. A literal, a member read or an identifier + // copy stays unpoisoned, so the common `const p = new Proxy(…)` + // in a module that also does `const p = { … }` elsewhere is + // untouched by this arm. + poison.insert(ident.id.sym.to_string()); } else if let ast::Expr::Member(member) = init_unwrapped { // #1750: `const w = path.win32` / `const p = path.posix`. // Record the alias so `w.normalize(...)` later dispatches like @@ -344,6 +359,17 @@ pub(crate) fn pre_scan_weakref_locals(ast_module: &ast::Module, ctx: &mut Loweri for name in &poison { ctx.weakmap_locals.remove(name); ctx.weakset_locals.remove(name); + // #7775: `proxy_locals` was left in — the note above weighed a lost + // codegen fast path against "no upside", because it only considered + // METHOD dispatch (`.deref()`, `.register()`), where a poisoned name + // costs speed. A PROPERTY read is the other half and the objection does + // not hold there: an ambiguous name routed a NON-proxy receiver's + // `a.length` to `js_proxy_get`, which answers `undefined`. That is a + // wrong answer, not a slow one, and it is what a poisoned name buys + // back. A genuine proxy keeps working through the ordinary dynamic + // property path (asserted in + // `test-files/test_gap_proxy_local_name_collision_7775.ts`). + ctx.proxy_locals.remove(name); } } diff --git a/test-files/test_gap_proxy_local_name_collision_7775.ts b/test-files/test_gap_proxy_local_name_collision_7775.ts new file mode 100644 index 0000000000..bfc76b559c --- /dev/null +++ b/test-files/test_gap_proxy_local_name_collision_7775.ts @@ -0,0 +1,126 @@ +// `proxy_locals` is a BARE-NAME set with no scope discrimination, collected by +// a module-wide pre-scan. A name bound to `new Proxy(...)` anywhere made EVERY +// function's `.prop` lower to `js_proxy_get` — including functions that +// bind the same name to something else entirely, and including the case where +// the proxy's own function is never called. +// +// `js_proxy_get` on a plain array answers `undefined`, so `a.length` came back +// undefined and the `for (i = 0; i < a.length; i++)` after it ran ZERO +// iterations. Wrong answer, no diagnostic. +// +// The pre-scan already had the remedy — a `poison` set for ambiguous names — +// but it only fired for a colliding `new ()`, and its result was +// subtracted from `weakmap_locals`/`weakset_locals` only. Both halves are why +// this survived. + +class P { + x: number; + y: number; + constructor(x: number, y: number) { + this.x = x; + this.y = y; + } +} + +function build(n: number): P[] { + const a: P[] = []; + for (let i = 0; i < n; i++) a.push(new P(i, i + 1)); + return a; +} + +// The victim: `a` here is a plain array, bound to a CALL result — the case the +// poison set did not cover. +function readLoop(): number { + const a = build(10); + let s = 0; + for (let i = 0; i < a.length; i++) { + const r = a[i]; + s += r.x + r.y; + } + return s; +} + +// Never called. Its mere presence was enough. +function neverCalled(): number { + const raw = build(10); + const a: P[] = new Proxy(raw, {}) as any; + return a.length; +} + +console.log("read loop:", readLoop()); +console.log("length:", build(4).length); + +// The proxy need not be over the victim's array, or over any array of the same +// type — only the NAME had to collide. +function unrelatedProxyName(): number { + const nums: number[] = [1, 2, 3]; + const a: any = new Proxy(nums, {}); + return a.length; +} +function alsoVictim(): number { + const a = build(3); + return a.length; +} +console.log("also victim:", alsoVictim()); + +// THE OTHER DIRECTION. A genuine, actually-used proxy whose name collides must +// still behave like a proxy — poisoning the name costs it a codegen fast path, +// and this asserts that the ordinary dynamic property path is still correct +// rather than merely slower. Without this the fix could "pass" by breaking +// proxies instead. +class Other { + v = 1; +} +function realProxyWithCollidingName(): string { + const a: any = new Proxy( + { q: 7 }, + { + get(target: any, key: string) { + return key === "q" ? 42 : target[key]; + }, + }, + ); + return `${a.q} ${a.missing}`; +} +function collidingNonProxy(): number { + const a = new Other(); + return a.v; +} +console.log("real proxy:", realProxyWithCollidingName()); +console.log("colliding non-proxy:", collidingNonProxy()); + +// A proxy with a UNIQUE name keeps every trap working — the common case, and +// the one an over-broad poison would silently degrade. +function uniquelyNamedProxy(): string { + const guarded: any = new Proxy( + { hit: 0 }, + { + get(target: any, key: string) { + if (key === "doubled") return target.hit * 2; + return target[key]; + }, + set(target: any, key: string, value: any) { + target[key] = value; + return true; + }, + }, + ); + guarded.hit = 21; + return `${guarded.hit} ${guarded.doubled}`; +} +console.log("unique proxy:", uniquelyNamedProxy()); + +// A `has` trap through a colliding name, so the fallback is exercised for more +// than a plain get. +function collidingHasTrap(): string { + const a: any = new Proxy( + {}, + { + has(_target: any, key: string) { + return key === "yes"; + }, + }, + ); + return `${"yes" in a} ${"no" in a}`; +} +console.log("colliding has trap:", collidingHasTrap()); From ec8fa6f2cc9df0b18212b765ad607aad7b5b6819 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 04:27:49 +0200 Subject: [PATCH 2/2] docs(changelog): add fragment for the #7775 proxy name-collision fix --- .../7828-proxy-local-name-collision.md | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 changelog.d/7828-proxy-local-name-collision.md diff --git a/changelog.d/7828-proxy-local-name-collision.md b/changelog.d/7828-proxy-local-name-collision.md new file mode 100644 index 0000000000..525071b63b --- /dev/null +++ b/changelog.d/7828-proxy-local-name-collision.md @@ -0,0 +1,22 @@ +**A local that merely shares a name with a proxy local is no longer treated as a proxy.** +`proxy_locals` is a bare-name set collected by a module-wide pre-scan with no +scope discrimination, and `expr_member` lowered `.prop` to +`Expr::ProxyGet` for any function using that name. `js_proxy_get` on a +non-proxy answers `undefined`, so a plain array's `a.length` came back +undefined and the `for (i = 0; i < a.length; i++)` after it ran **zero +iterations** — with no diagnostic, and without the proxy's own function ever +being called, without the proxy being over the same array, and without it being +an array at all. Only the name had to collide. Two defects in the `poison`-set +remedy the pre-scan already documents for this hazard: it fired only for a +colliding `new ()`, so a name bound to a *call* +(`const a = build(10)`) poisoned nothing; and its result was subtracted from +`weakmap_locals` and `weakset_locals` only — two of the five sets it feeds — so +even the covered case left `proxy_locals` wrong, making a colliding +`new Other()` instance read `a.v` as `undefined`. The comment restricting the +subtraction weighed a lost codegen fast path against "no upside", reasoning +about *method* dispatch; for a *property* read the fallback is correct and what +a poisoned name buys back is a wrong answer. Verified by fixture rather than +argument: a genuine, actually-used proxy under a poisoned name still returns 42 +from its `get` trap and its `has` trap still works. +`test-files/test_gap_proxy_local_name_collision_7775.ts` asserts both +directions, since a fix that broke proxies instead would otherwise pass. (#7775)