fix(hir): a local named like a proxy local is not a proxy

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 `<name>.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 <OtherClass>()`, 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)

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 <OtherClass>()`, 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);
     }
 }
 

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 `<name>.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 <OtherClass>()`, 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());