diff --git a/packages/plugin-rsc/src/transforms/hoist.test.ts b/packages/plugin-rsc/src/transforms/hoist.test.ts index 22742a470..68a9dded1 100644 --- a/packages/plugin-rsc/src/transforms/hoist.test.ts +++ b/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, "", "$$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, "", "$$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, "", "$$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, "", "$$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, "", "$$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, "", "$$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, "", "$$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['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['scope'] | undefined + let innerBlockScope: ReturnType['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) + }) +}) diff --git a/packages/plugin-rsc/src/transforms/hoist.ts b/packages/plugin-rsc/src/transforms/hoist.ts index eb5bf15ba..d2f6aa78d 100644 --- a/packages/plugin-rsc/src/transforms/hoist.ts +++ b/packages/plugin-rsc/src/transforms/hoist.ts @@ -2,7 +2,7 @@ import { tinyassert } from '@hiogawa/utils' import type { Program, Literal } from 'estree' import { walk } from 'estree-walker' import MagicString from 'magic-string' -import { analyze } from 'periscopic' +import { analyze, extract_names } from 'periscopic' export function transformHoistInlineDirective( input: string, @@ -71,7 +71,7 @@ export function transformHoistInlineDirective( ) } - const scope = analyzed.map.get(node) + const scope = analyzed.map.get(node.body) tinyassert(scope) const declName = node.type === 'FunctionDeclaration' && node.id.name const originalName = @@ -82,11 +82,13 @@ export function transformHoistInlineDirective( 'anonymous_server_function' // bind variables which are neither global nor in own scope + const ownParams = new Set(node.params.flatMap((p) => extract_names(p))) const bindVars = [...scope.references].filter((ref) => { - // declared function itself is included as reference - if (ref === declName) { + // own parameters are available in a hoisted function + if (ownParams.has(ref)) { return false } + const owner = scope.find_owner(ref) return owner && owner !== scope && owner !== analyzed.scope })