fix(rsc): handle shadowed local declarations in "use server" closure binding

packages/plugin-rsc/src/transforms/hoist.test.ts

@@ -1,3 +1,5 @@
+import { walk } from 'estree-walker'
+import { analyze } from 'periscopic'
 import { parseAstAsync } from 'vite'
 import { describe, expect, it } from 'vitest'
 import { transformHoistInlineDirective } from './hoist'
@@ -448,6 +450,240 @@ export async function kv() {
     `)
   })
 
+  // periscopic misclassifies block-scoped declarations inside a "use server"
+  // function body as outer-scope closure variables when the same name exists in
+  // an enclosing scope.  The hoist transform then injects a duplicate `const`
+  // declaration (from decryptActionBoundArgs) which causes a SyntaxError at
+  // runtime.
+  describe('local declaration shadows outer binding', () => {
+    it('const shadows outer variable', async () => {
+      // `cookies` is declared in the outer scope AND re-declared with const
+      // inside the server action. periscopic sees it as a closure ref, but it
+      // is NOT — the server action owns its own `cookies`.
+      const input = `
+function buildAction(config) {
+  const cookies = getCookies();
+
+  async function submitAction(formData) {
+    "use server";
+    const cookies = formData.get("value");
+    return doSomething(config, cookies);
+  }
+
+  return submitAction;
+}
+`
+      expect(await testTransform(input)).toMatchInlineSnapshot(`
+        "
+        function buildAction(config) {
+          const cookies = getCookies();
+
+          const submitAction = /* #__PURE__ */ $$register($$hoist_0_submitAction, "<id>", "$$hoist_0_submitAction").bind(null, config);
+
+          return submitAction;
+        }
+
+        ;export async function $$hoist_0_submitAction(config, formData) {
+            "use server";
+            const cookies = formData.get("value");
+            return doSomething(config, cookies);
+          };
+        /* #__PURE__ */ Object.defineProperty($$hoist_0_submitAction, "name", { value: "submitAction" });
+        "
+      `)
+      expect(await testTransform(input, { encode: true }))
+        .toMatchInlineSnapshot(`
+        "
+        function buildAction(config) {
+          const cookies = getCookies();
+
+          const submitAction = /* #__PURE__ */ $$register($$hoist_0_submitAction, "<id>", "$$hoist_0_submitAction").bind(null, __enc([config]));
+
+          return submitAction;
+        }
+
+        ;export async function $$hoist_0_submitAction($$hoist_encoded, formData) {
+            const [config] = __dec($$hoist_encoded);
+        "use server";
+            const cookies = formData.get("value");
+            return doSomething(config, cookies);
+          };
+        /* #__PURE__ */ Object.defineProperty($$hoist_0_submitAction, "name", { value: "submitAction" });
+        "
+      `)
+    })
+
+    it('const shadows function parameter', async () => {
+      // the outer `cookies` binding comes from a function parameter, not a
+      // variable declaration — collectOuterNames must handle params too.
+      const input = `
+function buildAction(cookies) {
+  async function submitAction(formData) {
+    "use server";
+    const cookies = formData.get("value");
+    return cookies;
+  }
+
+  return submitAction;
+}
+`
+      expect(await testTransform(input)).toMatchInlineSnapshot(`
+        "
+        function buildAction(cookies) {
+          const submitAction = /* #__PURE__ */ $$register($$hoist_0_submitAction, "<id>", "$$hoist_0_submitAction");
+
+          return submitAction;
+        }
+
+        ;export async function $$hoist_0_submitAction(formData) {
+            "use server";
+            const cookies = formData.get("value");
+            return cookies;
+          };
+        /* #__PURE__ */ Object.defineProperty($$hoist_0_submitAction, "name", { value: "submitAction" });
+        "
+      `)
+    })
+
+    it('destructured local declaration not included in bound args', async () => {
+      // `const { cookies } = ...` — the name comes from a destructuring pattern,
+      // not a plain Identifier declarator.  Must still be excluded from bindVars.
+      const input = `
+function buildAction(config) {
+  const cookies = getCookies();
+
+  async function submitAction(formData) {
+    "use server";
+    const { cookies } = parseForm(formData);
+    return doSomething(config, cookies);
+  }
+
+  return submitAction;
+}
+`
+      expect(await testTransform(input)).toMatchInlineSnapshot(`
+        "
+        function buildAction(config) {
+          const cookies = getCookies();
+
+          const submitAction = /* #__PURE__ */ $$register($$hoist_0_submitAction, "<id>", "$$hoist_0_submitAction").bind(null, config);
+
+          return submitAction;
+        }
+
+        ;export async function $$hoist_0_submitAction(config, formData) {
+            "use server";
+            const { cookies } = parseForm(formData);
+            return doSomething(config, cookies);
+          };
+        /* #__PURE__ */ Object.defineProperty($$hoist_0_submitAction, "name", { value: "submitAction" });
+        "
+      `)
+    })
+
+    it('inner accessing both outer and own names', async () => {
+      const input = `
+function outer() {
+  const cookies = 0;
+  async function action() {
+    "use server";
+    if (condition) {
+      const cookies = 1;  // block-scoped to the if
+      process(cookies);
+    }
+    return cookies;  // refers to OUTER cookies — needs binding
+  }
+}
+`
+      expect(await testTransform(input)).toMatchInlineSnapshot(`
+        "
+        function outer() {
+          const cookies = 0;
+          const action = /* #__PURE__ */ $$register($$hoist_0_action, "<id>", "$$hoist_0_action").bind(null, cookies);
+        }
+
+        ;export async function $$hoist_0_action(cookies) {
+            "use server";
+            if (condition) {
+              const cookies = 1;  // block-scoped to the if
+              process(cookies);
+            }
+            return cookies;  // refers to OUTER cookies — needs binding
+          };
+        /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" });
+        "
+      `)
+    })
+
+    // TODO: not working
+    it('inner has own block then shadows', async () => {
+      const input = `
+function outer() {
+  const cookie = 0;
+  async function action() {
+    "use server";
+    if (cond) {
+      const cookie = 1;
+      return cookie;  // refers to if-block's cookie
+    }
+  }
+}
+`
+      expect(await testTransform(input)).toMatchInlineSnapshot(`
+        "
+        function outer() {
+          const cookie = 0;
+          const action = /* #__PURE__ */ $$register($$hoist_0_action, "<id>", "$$hoist_0_action").bind(null, cookie);
+        }
+
+        ;export async function $$hoist_0_action(cookie) {
+            "use server";
+            if (cond) {
+              const cookie = 1;
+              return cookie;  // refers to if-block's cookie
+            }
+          };
+        /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" });
+        "
+      `)
+    })
+  })
+
+  // TODO: is this supposed to work? probably yes
+  it('self-referencing function', async () => {
+    const input = `
+function Parent() {
+  const count = 0;
+
+  async function recurse(n) {
+    "use server";
+    if (n > 0) return recurse(n - 1);
+    return count;
+  }
+
+  return recurse;
+}
+`
+    expect(await testTransform(input)).toMatchInlineSnapshot(`
+      "
+      function Parent() {
+        const count = 0;
+
+        const recurse = /* #__PURE__ */ $$register($$hoist_0_recurse, "<id>", "$$hoist_0_recurse").bind(null, count, recurse);
+
+        return recurse;
+      }
+
+      ;export async function $$hoist_0_recurse(count, recurse, n) {
+          "use server";
+          if (n > 0) return recurse(n - 1);
+          return count;
+        };
+      /* #__PURE__ */ Object.defineProperty($$hoist_0_recurse, "name", { value: "recurse" });
+      "
+    `)
+  })
+
   it('no ending new line', async () => {
     const input = `\
 export async function test() {
@@ -464,3 +700,122 @@ export async function test() {
     `)
   })
 })
+
+// TODO: report upstream
+// https://github.com/Rich-Harris/periscopic/
+describe('periscopic behavior', () => {
+  it('re-export confuses scopes', async () => {
+    // periscopic bug: `export { x } from "y"` creates a block scope with `x`
+    // as a declaration, which shadows the real module-level import.
+    // `find_owner` then returns that intermediate scope instead of
+    // `analyzed.scope`, causing the hoist algorithm to misidentify `redirect`
+    // as a closure variable.  The workaround in hoist.ts strips re-exports
+    // before calling analyze.
+    const ast = await parseAstAsync(`
+export { redirect } from "react-router/rsc";
+import { redirect } from "react-router/rsc";
+
+export default () => {
+  const f = async () => {
+    "use server";
+    throw redirect();
+  };
+}
+`)
+    const { map, scope: root } = analyze(ast)
+    // find the arrow with "use server"
+    let serverScope: ReturnType<typeof analyze>['scope'] | undefined
+    walk(ast, {
+      enter(node) {
+        if (
+          node.type === 'ArrowFunctionExpression' &&
+          node.body.type === 'BlockStatement' &&
+          node.body.body.some(
+            (s: any) =>
+              s.type === 'ExpressionStatement' &&
+              s.expression.type === 'Literal' &&
+              s.expression.value === 'use server',
+          )
+        ) {
+          serverScope = map.get(node)
+        }
+      },
+    })
+    expect(serverScope).toBeDefined()
+    expect(serverScope!.references.has('redirect')).toBe(true)
+    // find_owner should return the root scope (where the import lives), but
+    // instead returns the synthetic block scope periscopic creates for the
+    // re-export — this is a periscopic bug.
+    const owner = serverScope!.find_owner('redirect')
+    expect(owner).not.toBe(root)
+    expect(owner).not.toBe(serverScope)
+  })
+
+  it('shadowed variable: find_owner misses child block scope', async () => {
+    // When a `const` inside a function body shadows an outer name, periscopic
+    // puts the declaration in the BlockStatement's scope (a child of the
+    // function scope).  `find_owner` walks *up* from the function scope, so it
+    // finds the outer declaration instead of the inner one.
+    //
+    // This is not a periscopic bug — it is correct scope modeling.  The hoist
+    // algorithm was using find_owner from the function scope, which cannot see
+    // declarations in child (block) scopes.
+    const ast = await parseAstAsync(`
+function outer() {
+  const cookies = getCookies();
+  async function inner(formData) {
+    "use server";
+    const cookies = formData.get("value");
+    return cookies;
+  }
+}
+`)
+    const { map, scope: root } = analyze(ast)
+    // find the inner function and its body block scope
+    let innerFnScope: ReturnType<typeof analyze>['scope'] | undefined
+    let innerBlockScope: ReturnType<typeof analyze>['scope'] | undefined
+    walk(ast, {
+      enter(node) {
+        if (
+          node.type === 'FunctionDeclaration' &&
+          (node as any).id?.name === 'inner'
+        ) {
+          innerFnScope = map.get(node)
+        }
+        if (
+          node.type === 'BlockStatement' &&
+          node.body.some(
+            (s: any) =>
+              s.type === 'ExpressionStatement' &&
+              s.expression.type === 'Literal' &&
+              s.expression.value === 'use server',
+          )
+        ) {
+          innerBlockScope = map.get(node)
+        }
+      },
+    })
+    expect(innerFnScope).toBeDefined()
+    expect(innerBlockScope).toBeDefined()
+
+    // periscopic correctly declares `cookies` in the block scope (child of function scope)
+    expect(innerBlockScope!.declarations.has('cookies')).toBe(true)
+    // the function scope does NOT have `cookies` — only `formData` (param)
+    expect(innerFnScope!.declarations.has('cookies')).toBe(false)
+    expect(innerFnScope!.declarations.has('formData')).toBe(true)
+
+    // `cookies` propagates up as a reference through all ancestor scopes
+    expect(innerFnScope!.references.has('cookies')).toBe(true)
+
+    // find_owner from function scope walks UP and finds the OUTER `cookies`,
+    // not the inner one (which is in a child block scope, unreachable by walking up)
+    const ownerFromFnScope = innerFnScope!.find_owner('cookies')
+    expect(ownerFromFnScope).not.toBe(innerFnScope)
+    expect(ownerFromFnScope).not.toBe(innerBlockScope)
+    expect(ownerFromFnScope).not.toBe(root)
+
+    // find_owner from block scope correctly finds the INNER `cookies`
+    const ownerFromBlockScope = innerBlockScope!.find_owner('cookies')
+    expect(ownerFromBlockScope).toBe(innerBlockScope)
+  })
+})