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) 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());