diff --git a/packages/plugin-rsc/package.json b/packages/plugin-rsc/package.json index d290c7188..d68980c25 100644 --- a/packages/plugin-rsc/package.json +++ b/packages/plugin-rsc/package.json @@ -43,7 +43,6 @@ "es-module-lexer": "^2.0.0", "estree-walker": "^3.0.3", "magic-string": "^0.30.21", - "periscopic": "^4.0.2", "srvx": "^0.11.12", "strip-literal": "^3.1.0", "turbo-stream": "^3.2.0", diff --git a/packages/plugin-rsc/src/transforms/analyze-node.ts b/packages/plugin-rsc/src/transforms/analyze-node.ts new file mode 100644 index 000000000..821e7ccee --- /dev/null +++ b/packages/plugin-rsc/src/transforms/analyze-node.ts @@ -0,0 +1,350 @@ +import type { Node, Function as FunctionNode, MemberExpression } from 'estree' +import { walk } from 'estree-walker' +import { isReference } from './is-reference' + +export function extractNames( + node: Node | Node[], + idents: Set = new Set(), +): Set { + if (Array.isArray(node)) { + for (const n of node) extractNames(n, idents) + return idents + } + + switch (node.type) { + case 'Identifier': + return idents.add(node.name) + + case 'MemberExpression': { + let obj: Node = node + // find root of member expr (e.g. `foo` in `foo.bar.baz = ...`) + while (obj.type === 'MemberExpression') obj = obj.object + return extractNames(obj, idents) + } + + case 'ObjectPattern': + for (const prop of node.properties) { + switch (prop.type) { + case 'Property': + extractNames(prop.value, idents) + break + case 'RestElement': + extractNames(prop.argument, idents) + break + } + } + break + + case 'ArrayPattern': + for (const el of node.elements) { + if (el) extractNames(el, idents) + } + break + + case 'RestElement': + return extractNames(node.argument, idents) + + case 'AssignmentPattern': + return extractNames(node.left, idents) + } + + return idents +} + +class Scope { + public declarations: Set = new Set() + + constructor( + public parent: Scope | null, + public isBlock: boolean, + ) {} + + declare(name: string): void { + this.declarations.add(name) + } + + declareInFunctionScope(name: string): void { + if (this.isBlock && this.parent) { + return this.parent.declareInFunctionScope(name) + } + this.declare(name) + } + + findOwner(name: string): Scope | null { + if (this.declarations.has(name)) return this + if (this.parent) return this.parent.findOwner(name) + return null + } +} + +export type NodeAnalysis = { + scope: Scope + map: WeakMap +} + +export function analyze(root: Node): NodeAnalysis { + const map = new WeakMap() + const rootScope = new Scope(null, false) + + let scope = rootScope + + walk(root, { + enter(node) { + switch (node.type) { + case 'ImportDeclaration': + for (const spec of node.specifiers) { + scope.declare(spec.local.name) + } + break + + case 'ExportAllDeclaration': + case 'ExportNamedDeclaration': + // re-exports don't get their own scope. + break + + case 'FunctionDeclaration': + case 'FunctionExpression': + case 'ArrowFunctionExpression': { + if (node.type === 'FunctionDeclaration' && node.id) { + // function ref exists on the parent's scope with declarations + scope.declare(node.id.name) + } + + scope = new Scope(scope, false) + map.set(node, scope) + + if (node.type === 'FunctionExpression' && node.id) { + // function ref is visible in its own scope for recursion + scope.declare(node.id.name) + } + + for (const p of node.params) { + for (const name of extractNames(p)) scope.declare(name) + } + break + } + + case 'BlockStatement': + case 'ForStatement': + case 'ForInStatement': + case 'ForOfStatement': { + scope = new Scope(scope, true) + map.set(node, scope) + break + } + + case 'CatchClause': { + scope = new Scope(scope, true) + map.set(node, scope) + if (node.param) { + for (const name of extractNames(node.param)) scope.declare(name) + } + break + } + + case 'VariableDeclaration': { + for (const decl of node.declarations) { + for (const name of extractNames(decl.id)) { + if (node.kind === 'var') { + scope.declareInFunctionScope(name) + } else { + scope.declare(name) + } + } + } + break + } + + case 'ClassDeclaration': { + if (node.id) scope.declare(node.id.name) + break + } + } + }, + + leave(node: Node) { + if (map.has(node) && scope.parent) { + scope = scope.parent + } + }, + }) + + return { map, scope: rootScope } +} + +export type VariableUsage = { + isUsedBare: boolean + members: Map> +} + +export type FunctionCaptureAnalysis = { + captures: Map + isSelfReferencing: boolean +} + +export function analyzeFunctionCaptures( + fnNode: FunctionNode, + programScope: NodeAnalysis, +): FunctionCaptureAnalysis { + const captures = new Map() + let isSelfReferencing = false + + const fnName = + (fnNode.type === 'FunctionDeclaration' || + fnNode.type === 'FunctionExpression') && + fnNode.id?.name + const fnParams = extractNames(fnNode.params) + + const fnDeclScope = programScope.map.get(fnNode) + const fnBodyScope = programScope.map.get(fnNode.body) + const fnScope = fnDeclScope ?? fnBodyScope ?? null + + let currentScope: Scope = fnBodyScope ?? programScope.scope + + const enter = (node: Node, parent: Node | null) => { + const s = programScope.map.get(node) + if (s) currentScope = s + + const isObjectOfNonComputedMember = + parent?.type === 'MemberExpression' && + parent.object === node && + !parent.computed + const isOutermostMemberExpr = + node.type === 'MemberExpression' && + !node.computed && + !isObjectOfNonComputedMember + + let root: Node = node // e.g. `config` in `config.db.host` + while (root.type === 'MemberExpression') root = root.object + + if (!isReference(root, parent)) return + const name = root.name + + if (fnName && name === fnName) { + isSelfReferencing = true + return + } + + if (fnParams.has(name)) return + + const ownerScope = currentScope.findOwner(name) + if ( + !ownerScope || + ownerScope === programScope.scope || + isInsideFunctionBody(ownerScope, fnScope, programScope.scope) + ) { + // either undeclared, declared inside the function body, or in the root scope + // not considered a capture for hoisting/binding purposes. + return + } + + if (!captures.has(name)) { + captures.set(name, { isUsedBare: false, members: new Map() }) + } + const usage = captures.get(name)! + + if (isOutermostMemberExpr) { + if (usage.isUsedBare) return + + const pathKey = memberExprToPathKey(node) + if (!usage.members.has(pathKey)) { + usage.members.set(pathKey, []) + } + + usage.members + .get(pathKey)! + .push({ start: node.start, end: node.end, suffix: '' }) + } else if (!isObjectOfNonComputedMember) { + usage.isUsedBare = true + // if a variable is used by itself, the entire variable must be bound instead + // of individual member paths, so we stop tracking them. + usage.members.clear() + } + } + + const leave = (node: Node) => { + const s = programScope.map.get(node) + if (s?.parent) currentScope = s.parent + } + + // walk the params to capture variables referenced in default values + currentScope = (fnDeclScope ?? fnBodyScope)?.parent ?? programScope.scope + for (const param of fnNode.params) { + walk(param, { enter, leave }) + } + + currentScope = fnBodyScope ?? programScope.scope + walk(fnNode.body, { + enter(node: Node, parent: Node | null) { + if (node !== fnNode.body) enter(node, parent) + }, + leave(node: Node) { + if (node !== fnNode.body) leave(node) + }, + }) + + // de-duplicate captured member paths by prefix + // + // e.g. if both `config.cookies` and `config.cookies.names` are captured, we only + // bind `config.cookies` and rewrite the `config.cookies.names` occurrence to + // `$$bind_0_config_cookies.names` instead of binding both paths separately. + + for (const usage of captures.values()) { + if (usage.isUsedBare || usage.members.size <= 1) continue + + const pathPrefixes = new Set() + const paths = [...usage.members.keys()].sort((a, b) => a.length - b.length) + + // we go from shortest path to longest, seeing if the current one is already + // covered by a previously preserved shorter one for the de-duping. + for (const path of paths) { + let prefixedBy: string | undefined + for (const prefix of pathPrefixes) { + if (path.startsWith(prefix + '.')) { + prefixedBy = prefix + break + } + } + + if (prefixedBy !== undefined) { + const suffix = path.slice(prefixedBy.length) + const prefixMember = usage.members.get(prefixedBy)! + for (const r of usage.members.get(path)!) { + prefixMember.push({ start: r.start, end: r.end, suffix }) + } + usage.members.delete(path) + } else { + pathPrefixes.add(path) + } + } + } + + return { captures, isSelfReferencing } +} + +function memberExprToPathKey(expr: MemberExpression): string { + const parts: string[] = [] + + let node: Node = expr + while (node.type === 'MemberExpression') { + if ('name' in node.property) parts.unshift(node.property.name) + node = node.object + } + if (node.type === 'Identifier') parts.unshift(node.name) + + return parts.join('.') +} + +function isInsideFunctionBody( + scope: Scope, + bodyScope: Scope | null, + rootScope: Scope, +): boolean { + let s: Scope | null = scope + while (s) { + if (s === bodyScope) return true + if (s === rootScope) return false + s = s.parent + } + return false +} diff --git a/packages/plugin-rsc/src/transforms/cjs.ts b/packages/plugin-rsc/src/transforms/cjs.ts index 766ab13a0..5725bcc1e 100644 --- a/packages/plugin-rsc/src/transforms/cjs.ts +++ b/packages/plugin-rsc/src/transforms/cjs.ts @@ -3,7 +3,7 @@ import { fileURLToPath, pathToFileURL } from 'node:url' import type { Program, Node } from 'estree' import { walk } from 'estree-walker' import MagicString from 'magic-string' -import { analyze } from 'periscopic' +import { analyze } from './analyze-node' // TODO: // replacing require("xxx") into import("xxx") affects Vite's resolution. diff --git a/packages/plugin-rsc/src/transforms/hoist.test.ts b/packages/plugin-rsc/src/transforms/hoist.test.ts index 22742a470..122a4ad6a 100644 --- a/packages/plugin-rsc/src/transforms/hoist.test.ts +++ b/packages/plugin-rsc/src/transforms/hoist.test.ts @@ -260,8 +260,8 @@ function Counter() { } ;export function $$hoist_0_anonymous_server_function($$hoist_encoded, formData) { - const [name] = __dec($$hoist_encoded); - "use server"; + "use server"; + const [name] = __dec($$hoist_encoded); count += Number(formData.get(name)); }; /* #__PURE__ */ Object.defineProperty($$hoist_0_anonymous_server_function, "name", { value: "anonymous_server_function" }); @@ -325,15 +325,15 @@ function validator(action) { } ;export async function $$hoist_0_anonymous_server_function($$hoist_encoded, y) { - const [x] = __dec($$hoist_encoded); - "use server"; + "use server"; + const [x] = __dec($$hoist_encoded); return x + y; }; /* #__PURE__ */ Object.defineProperty($$hoist_0_anonymous_server_function, "name", { value: "anonymous_server_function" }); ;export async function $$hoist_1_anonymous_server_function($$hoist_encoded, arg) { - const [action] = __dec($$hoist_encoded); - "use server"; + "use server"; + const [action] = __dec($$hoist_encoded); return action(arg); }; /* #__PURE__ */ Object.defineProperty($$hoist_1_anonymous_server_function, "name", { value: "anonymous_server_function" }); @@ -448,6 +448,642 @@ export async function kv() { `) }) + 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) { + "use server"; + const [config] = __dec($$hoist_encoded); + 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" }); + " + `) + }) + + it('var at top of body shadows outer variable', async () => { + // `var` is function-scoped, so `var cookies` at the top of the action body + // correctly shadows the outer `cookies` — it must NOT be bound. + const input = ` +function buildAction(config) { + const cookies = getCookies(); + + async function submitAction(formData) { + "use server"; + var 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"; + var cookies = formData.get("value"); + return doSomething(config, cookies); + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_submitAction, "name", { value: "submitAction" }); + " + `) + }) + + it('var inside a nested block shadows outer variable', async () => { + // `var` inside an if-block is still function-scoped in JS, so it shadows + // the outer `cookies` across the entire action body — must NOT be bound. + const input = ` +function buildAction(config) { + const cookies = getCookies(); + + async function submitAction(formData) { + "use server"; + if (condition) { + var 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"; + if (condition) { + var cookies = formData.get("value"); + } + return doSomething(config, cookies); + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_submitAction, "name", { value: "submitAction" }); + " + `) + }) + + 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"); + } + + ;export async function $$hoist_0_action() { + "use server"; + if (cond) { + const cookie = 1; + return cookie; // refers to if-block's cookie + } + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + }) + + describe('binding member expressions', () => { + it('member access only binds the member expression, not the root variable', async () => { + const input = ` +function MyForm({ config }) { + async function submitAction(formData) { + "use server"; + + const prefix = config.cookiePrefix; // ONLY member access, never bare config + console.log(config.cookiePrefix); + return config.cookiePrefix; + } + + return "test"; +} +` + expect(await testTransform(input)).toMatchInlineSnapshot(` + " + function MyForm({ config }) { + const submitAction = /* #__PURE__ */ $$register($$hoist_0_submitAction, "", "$$hoist_0_submitAction").bind(null, config.cookiePrefix); + + return "test"; + } + + ;export async function $$hoist_0_submitAction($$bind_0_config_cookiePrefix, formData) { + "use server"; + + const prefix = $$bind_0_config_cookiePrefix; // ONLY member access, never bare config + console.log($$bind_0_config_cookiePrefix); + return $$bind_0_config_cookiePrefix; + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_submitAction, "name", { value: "submitAction" }); + " + `) + }) + + it('multiple different props from same object are each bound separately', async () => { + const input = ` +function outer(config) { + async function action(formData) { + "use server"; + return config.host + config.port; + } +} +` + expect(await testTransform(input)).toMatchInlineSnapshot(` + " + function outer(config) { + const action = /* #__PURE__ */ $$register($$hoist_0_action, "", "$$hoist_0_action").bind(null, config.host, config.port); + } + + ;export async function $$hoist_0_action($$bind_0_config_host, $$bind_1_config_port, formData) { + "use server"; + return $$bind_0_config_host + $$bind_1_config_port; + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + + it('bare use of var falls back to binding the whole variable', async () => { + const input = ` +function outer(config) { + async function action(formData) { + "use server"; + const prefix = config.cookiePrefix; + return doSomething(config); + } +} +` + expect(await testTransform(input)).toMatchInlineSnapshot(` + " + function outer(config) { + const action = /* #__PURE__ */ $$register($$hoist_0_action, "", "$$hoist_0_action").bind(null, config); + } + + ;export async function $$hoist_0_action(config, formData) { + "use server"; + const prefix = config.cookiePrefix; + return doSomething(config); + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + + it('computed member access falls back to binding the whole variable', async () => { + const input = ` +function outer(config, key) { + async function action(formData) { + "use server"; + return config[key]; + } +} +` + expect(await testTransform(input)).toMatchInlineSnapshot(` + " + function outer(config, key) { + const action = /* #__PURE__ */ $$register($$hoist_0_action, "", "$$hoist_0_action").bind(null, config, key); + } + + ;export async function $$hoist_0_action(config, key, formData) { + "use server"; + return config[key]; + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + + it('mixed vars: one member-only, one bare', async () => { + const input = ` +function outer(config, user) { + async function action(formData) { + "use server"; + return config.cookiePrefix + user; + } +} +` + expect(await testTransform(input)).toMatchInlineSnapshot(` + " + function outer(config, user) { + const action = /* #__PURE__ */ $$register($$hoist_0_action, "", "$$hoist_0_action").bind(null, config.cookiePrefix, user); + } + + ;export async function $$hoist_0_action($$bind_0_config_cookiePrefix, user, formData) { + "use server"; + return $$bind_0_config_cookiePrefix + user; + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + + it('multi-level member access binds the full chain', async () => { + const input = ` +function outer(config) { + async function action(formData) { + "use server"; + return config.db.host; + } +} +` + expect(await testTransform(input)).toMatchInlineSnapshot(` + " + function outer(config) { + const action = /* #__PURE__ */ $$register($$hoist_0_action, "", "$$hoist_0_action").bind(null, config.db.host); + } + + ;export async function $$hoist_0_action($$bind_0_config_db_host, formData) { + "use server"; + return $$bind_0_config_db_host; + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + + it('shorter path subsumes longer path from same root', async () => { + // config.cookies is used bare AND config.cookies.names is used — the + // shorter path subsumes the longer one: only config.cookies is bound, + // and config.cookies.names is rewritten to $$bind_0_config_cookies.names + const input = ` +function outer(config) { + async function action(formData) { + "use server"; + const list = config.cookies.names; + return config.cookies; + } +} +` + expect(await testTransform(input)).toMatchInlineSnapshot(` + " + function outer(config) { + const action = /* #__PURE__ */ $$register($$hoist_0_action, "", "$$hoist_0_action").bind(null, config.cookies); + } + + ;export async function $$hoist_0_action($$bind_0_config_cookies, formData) { + "use server"; + const list = $$bind_0_config_cookies.names; + return $$bind_0_config_cookies; + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + + it('longer paths without an observed shorter prefix are kept separate', async () => { + // config.cookies.names and config.cookies.value share a common ancestor + // (config.cookies) but that ancestor was never directly used in the source, + // so we must NOT synthesise it — each path is bound independently. + const input = ` +function outer(config) { + async function action(formData) { + "use server"; + return config.cookies.names + config.cookies.value; + } +} +` + expect(await testTransform(input)).toMatchInlineSnapshot(` + " + function outer(config) { + const action = /* #__PURE__ */ $$register($$hoist_0_action, "", "$$hoist_0_action").bind(null, config.cookies.names, config.cookies.value); + } + + ;export async function $$hoist_0_action($$bind_0_config_cookies_names, $$bind_1_config_cookies_value, formData) { + "use server"; + return $$bind_0_config_cookies_names + $$bind_1_config_cookies_value; + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + + it('observed shorter prefix subsumes longer paths from same root', async () => { + // config.cookies IS directly used, so it subsumes config.cookies.names + const input = ` +function outer(config) { + async function action(formData) { + "use server"; + return config.cookies.names + config.cookies.value + config.cookies; + } +} +` + expect(await testTransform(input)).toMatchInlineSnapshot(` + " + function outer(config) { + const action = /* #__PURE__ */ $$register($$hoist_0_action, "", "$$hoist_0_action").bind(null, config.cookies); + } + + ;export async function $$hoist_0_action($$bind_0_config_cookies, formData) { + "use server"; + return $$bind_0_config_cookies.names + $$bind_0_config_cookies.value + $$bind_0_config_cookies; + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + + it('independent sibling paths are not deduplicated', async () => { + // config.cookies and config.db share the root but neither is a prefix of + // the other — they must remain as separate bound params + const input = ` +function outer(config) { + async function action(formData) { + "use server"; + return config.cookies + config.db; + } +} +` + expect(await testTransform(input)).toMatchInlineSnapshot(` + " + function outer(config) { + const action = /* #__PURE__ */ $$register($$hoist_0_action, "", "$$hoist_0_action").bind(null, config.cookies, config.db); + } + + ;export async function $$hoist_0_action($$bind_0_config_cookies, $$bind_1_config_db, formData) { + "use server"; + return $$bind_0_config_cookies + $$bind_1_config_db; + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + + it('shadowed object: outer member path is bound, inner shadowed occurrence is left alone', async () => { + // config.db.host on line A refers to the outer `config` — it is bound and + // rewritten to $$bind_0_config_db_host. + // config.db.host on line B is inside a block where `const config` shadows + // the outer one — isInsideFunctionBody correctly skips it during the walk, + // so it is never recorded as a free-var usage and is left unrewritten. + // The two occurrences look identical in source but resolve to different + // bindings, and the transform handles them correctly without falling back + // to binding the whole bare `config`. + const input = ` + function outer(config) { + async function action(formData) { + "use server"; + const oldHost = config.db.host; + if (condition) { + const config = { db: { host: "test" } }; // shadows outer config + return config.db.host; // should refer to inner config, not outer + } + const oldHost2 = config.db.host; + } + } +` + expect(await testTransform(input)).toMatchInlineSnapshot(` + " + function outer(config) { + const action = /* #__PURE__ */ $$register($$hoist_0_action, "", "$$hoist_0_action").bind(null, config.db.host); + } + + ;export async function $$hoist_0_action($$bind_0_config_db_host, formData) { + "use server"; + const oldHost = $$bind_0_config_db_host; + if (condition) { + const config = { db: { host: "test" } }; // shadows outer config + return config.db.host; // should refer to inner config, not outer + } + const oldHost2 = $$bind_0_config_db_host; + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + }) + + 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); + + return recurse; + } + + ;export async function $$hoist_0_recurse(count, n) { + "use server"; + const recurse = (...$$args) => $$hoist_0_recurse(count, ...$$args); + if (n > 0) return recurse(n - 1); + return count; + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_recurse, "name", { value: "recurse" }); + " + `) + + // With encryption the hoisted function receives a single opaque + // `$$hoist_encoded` argument. The self-referencing alias must forward + // that same encoded value directly rather than re-encoding the already- + // decoded locals. + expect(await testTransform(input, { encode: true })).toMatchInlineSnapshot(` + " + function Parent() { + const count = 0; + + const recurse = /* #__PURE__ */ $$register($$hoist_0_recurse, "", "$$hoist_0_recurse").bind(null, __enc([count])); + + return recurse; + } + + ;export async function $$hoist_0_recurse($$hoist_encoded, n) { + "use server"; + const [count] = __dec($$hoist_encoded); + const recurse = (...$$args) => $$hoist_0_recurse($$hoist_encoded, ...$$args); + 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() { @@ -463,4 +1099,252 @@ export async function test() { " `) }) + + describe('fixes for periscopic bugs', () => { + it('re-export of an import is not treated as a closure variable', async () => { + // periscopic bug: `export { redirect } from "y"` created a synthetic block + // scope with `redirect` as a declaration, shadowing the real module-level + // import. `find_owner` returned that intermediate scope instead of the + // root scope, so the hoist algorithm mistakenly treated `redirect` as a + // closure variable and emitted `.bind(null, redirect)`. + // + // Our analyzer skips re-export declarations entirely — no synthetic scope, + // no spurious declaration — so `redirect` is correctly identified as a + // module-level import and emitted with no .bind() args. + expect( + await testTransform(` +export { redirect } from "react-router/rsc"; +import { redirect } from "react-router/rsc"; + +export default () => { + const f = async () => { + "use server"; + throw redirect(); + }; +} +`), + ).toMatchInlineSnapshot(` + " + export { redirect } from "react-router/rsc"; + import { redirect } from "react-router/rsc"; + + export default () => { + const f = /* #__PURE__ */ $$register($$hoist_0_f, "", "$$hoist_0_f"); + } + + ;export async function $$hoist_0_f() { + "use server"; + throw redirect(); + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_f, "name", { value: "f" }); + " + `) + }) + + it('const inside function body that shadows an outer name is not bound', async () => { + // periscopic bug: `const cookies` inside the function body is placed in the + // BlockStatement's scope (a child of the function scope). The hoist + // algorithm called `find_owner` from the function scope, which walks *up* + // the scope chain and cannot see child block scopes — so it found the + // *outer* `cookies` and incorrectly emitted `.bind(null, cookies)`, causing + // a duplicate `const cookies` declaration at runtime (a SyntaxError). + // + // Our analyzer starts the owner lookup from the innermost scope at the + // point of each identifier use, so the inner `const cookies` correctly + // shadows the outer one and is not bound. + expect( + await testTransform(` +function outer() { + const cookies = getCookies(); + async function inner(formData) { + "use server"; + const cookies = formData.get("value"); + return cookies; + } +} +`), + ).toMatchInlineSnapshot(` + " + function outer() { + const cookies = getCookies(); + const inner = /* #__PURE__ */ $$register($$hoist_0_inner, "", "$$hoist_0_inner"); + } + + ;export async function $$hoist_0_inner(formData) { + "use server"; + const cookies = formData.get("value"); + return cookies; + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_inner, "name", { value: "inner" }); + " + `) + }) + }) + + describe('additional scoping edge cases', () => { + it('for-of loop variable is not bound, but the iterable is', async () => { + // `item` is declared by the for-of statement itself (block-scoped to the + // loop) — it must not appear in .bind() args. `outerList` is a free + // variable from the enclosing scope and must be bound. + expect( + await testTransform(` +function outer(outerList) { + async function action() { + "use server"; + for (const item of outerList) { + process(item); + } + } +} +`), + ).toMatchInlineSnapshot(` + " + function outer(outerList) { + const action = /* #__PURE__ */ $$register($$hoist_0_action, "", "$$hoist_0_action").bind(null, outerList); + } + + ;export async function $$hoist_0_action(outerList) { + "use server"; + for (const item of outerList) { + process(item); + } + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + + it('catch binding shadows outer name and is not bound', async () => { + // `err` is declared as a catch binding. Even if an outer `err` exists, + // the catch binding shadows it inside the catch block — it must not be + // included in .bind() args. + expect( + await testTransform(` +function outer(config, err) { + async function action() { + "use server"; + try { + return config.value; + } catch (err) { + return err.message; + } + } +} +`), + ).toMatchInlineSnapshot(` + " + function outer(config, err) { + const action = /* #__PURE__ */ $$register($$hoist_0_action, "", "$$hoist_0_action").bind(null, config.value); + } + + ;export async function $$hoist_0_action($$bind_0_config_value) { + "use server"; + try { + return $$bind_0_config_value; + } catch (err) { + return err.message; + } + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + + it('named function expression self-reference gets an alias', async () => { + // `self` is declared in the named function expression's own scope — + // it is not a free variable and must not appear in .bind() args. + // However, the hoisted function is renamed to $$hoist_0_action, so + // the `self` reference inside the body would break. Just like the + // FunctionDeclaration self-referencing case, an alias is emitted + // inside the hoisted function body so that `self` resolves correctly. + expect( + await testTransform(` +function outer(count) { + const action = async function self(n) { + "use server"; + if (n > 0) return self(n - 1); + return count; + }; +} +`), + ).toMatchInlineSnapshot(` + " + function outer(count) { + const action = /* #__PURE__ */ $$register($$hoist_0_action, "", "$$hoist_0_action").bind(null, count); + } + + ;export async function $$hoist_0_action(count, n) { + "use server"; + const self = (...$$args) => $$hoist_0_action(count, ...$$args); + if (n > 0) return self(n - 1); + return count; + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + + it('class declaration inside body is not bound', async () => { + // `Helper` is declared with `class` inside the action body — it belongs + // to the action itself and must not appear in .bind() args. + expect( + await testTransform(` +function outer(config) { + async function action() { + "use server"; + class Helper { + run() { return config.value; } + } + return new Helper().run(); + } +} +`), + ).toMatchInlineSnapshot(` + " + function outer(config) { + const action = /* #__PURE__ */ $$register($$hoist_0_action, "", "$$hoist_0_action").bind(null, config.value); + } + + ;export async function $$hoist_0_action($$bind_0_config_value) { + "use server"; + class Helper { + run() { return $$bind_0_config_value; } + } + return new Helper().run(); + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + + it('outer variable in destructured param default is bound', async () => { + // `outerDefault` is referenced as a default value inside a destructured + // parameter. Parameter defaults are evaluated in the caller's scope, not + // the function's own scope, so `outerDefault` is a free variable that must + // be bound. Without this fix the hoisted function would reference an + // undefined `outerDefault` at the module level. + expect( + await testTransform(` +function outer(outerDefault) { + async function action({ x = outerDefault } = {}) { + "use server"; + return x; + } +} +`), + ).toMatchInlineSnapshot(` + " + function outer(outerDefault) { + const action = /* #__PURE__ */ $$register($$hoist_0_action, "", "$$hoist_0_action").bind(null, outerDefault); + } + + ;export async function $$hoist_0_action(outerDefault, { x = outerDefault } = {}) { + "use server"; + return x; + }; + /* #__PURE__ */ Object.defineProperty($$hoist_0_action, "name", { value: "action" }); + " + `) + }) + }) }) diff --git a/packages/plugin-rsc/src/transforms/hoist.ts b/packages/plugin-rsc/src/transforms/hoist.ts index eb5bf15ba..102f3f5f8 100644 --- a/packages/plugin-rsc/src/transforms/hoist.ts +++ b/packages/plugin-rsc/src/transforms/hoist.ts @@ -1,8 +1,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, analyzeFunctionCaptures } from './analyze-node' export function transformHoistInlineDirective( input: string, @@ -37,18 +36,6 @@ export function transformHoistInlineDirective( ? exactRegex(options.directive) : options.directive - // re-export somehow confuses periscopic scopes so remove them before analysis - walk(ast, { - enter(node) { - if (node.type === 'ExportAllDeclaration') { - this.remove() - } - if (node.type === 'ExportNamedDeclaration' && !node.declaration) { - this.remove() - } - }, - }) - const analyzed = analyze(ast) const names: string[] = [] @@ -71,9 +58,8 @@ export function transformHoistInlineDirective( ) } - const scope = analyzed.map.get(node) - tinyassert(scope) const declName = node.type === 'FunctionDeclaration' && node.id.name + const exprName = node.type === 'FunctionExpression' && node.id?.name const originalName = declName || (parent?.type === 'VariableDeclarator' && @@ -81,17 +67,28 @@ export function transformHoistInlineDirective( parent.id.name) || 'anonymous_server_function' + const fnCaptures = analyzeFunctionCaptures(node, analyzed) + // bind variables which are neither global nor in own scope - const bindVars = [...scope.references].filter((ref) => { - // declared function itself is included as reference - if (ref === declName) { - return false - } - const owner = scope.find_owner(ref) - return owner && owner !== scope && owner !== analyzed.scope - }) + const bindVars = [...fnCaptures.captures.entries()].flatMap( + ([ref, usage]) => { + if (usage.isUsedBare || usage.members.size === 0) { + return [{ param: ref, arg: ref }] + } + + return [...usage.members.entries()].map( + ([pathKey, ranges], idx) => { + const param = `$$bind_${idx}_${pathKey.replace(/\./g, '_')}` + for (const { start, end, suffix } of ranges) { + output.update(start, end, param + suffix) + } + return { param, arg: pathKey } + }, + ) + }, + ) let newParams = [ - ...bindVars, + ...bindVars.map((b) => b.param), ...node.params.map((n) => input.slice(n.start, n.end)), ].join(', ') if (bindVars.length > 0 && options.decode) { @@ -100,10 +97,10 @@ export function transformHoistInlineDirective( ...node.params.map((n) => input.slice(n.start, n.end)), ].join(', ') output.appendLeft( - node.body.body[0]!.start, - `const [${bindVars.join(',')}] = ${options.decode( + node.body.body[0]!.end, + `\nconst [${bindVars.map((b) => b.param).join(',')}] = ${options.decode( '$$hoist_encoded', - )};\n`, + )};`, ) } @@ -126,14 +123,29 @@ export function transformHoistInlineDirective( ) output.move(node.start, node.end, input.length) + if ((declName || exprName) && fnCaptures.isSelfReferencing) { + const aliasName = declName || exprName! + const boundArgs = + bindVars.length === 0 + ? '' + : options.decode + ? '$$hoist_encoded, ' + : `${bindVars.map((v) => v.arg).join(', ')}, ` + const directiveNode = node.body.body[0]! + output.appendLeft( + directiveNode.end, + `\nconst ${aliasName} = (...$$args) => ${newName}(${boundArgs}...$$args);`, + ) + } + // replace original declartion with action register + bind let newCode = `/* #__PURE__ */ ${runtime(newName, newName, { directiveMatch: match, })}` if (bindVars.length > 0) { const bindArgs = options.encode - ? options.encode('[' + bindVars.join(', ') + ']') - : bindVars.join(', ') + ? options.encode('[' + bindVars.map((b) => b.arg).join(', ') + ']') + : bindVars.map((b) => b.arg).join(', ') newCode = `${newCode}.bind(null, ${bindArgs})` } if (declName) { diff --git a/packages/plugin-rsc/src/transforms/is-reference.ts b/packages/plugin-rsc/src/transforms/is-reference.ts new file mode 100644 index 000000000..cf33b7d90 --- /dev/null +++ b/packages/plugin-rsc/src/transforms/is-reference.ts @@ -0,0 +1,54 @@ +// https://github.com/Rich-Harris/is-reference/blob/master/src/index.js + +import type { Identifier, Node } from 'estree' + +/** + * Check if a node is a reference to an identifier, as opposed to a declaration or a property name. + */ +export function isReference( + node: Node, + parent: Node | null, +): node is Identifier { + if (node.type === 'MemberExpression') { + return !node.computed && isReference(node.object, node) + } + + if (node.type !== 'Identifier') return false + + switch (parent?.type) { + // disregard `bar` in `foo.bar` + case 'MemberExpression': + return parent.computed || node === parent.object + + // disregard the `foo` in `class {foo(){}}` but keep it in `class {[foo](){}}` + case 'MethodDefinition': + return parent.computed + + // disregard the `meta` in `import.meta` + case 'MetaProperty': + return parent.meta === node + + // disregard the `foo` in `class {foo=bar}` but keep it in `class {[foo]=bar}` and `class {bar=foo}` + case 'PropertyDefinition': + return parent.computed || node === parent.value + + // disregard the `bar` in `{ bar: foo }`, but keep it in `{ [bar]: foo }` + case 'Property': + return parent.computed || node === parent.value + + // disregard the `bar` in `export { foo as bar }` or + // the foo in `import { foo as bar }` + case 'ExportSpecifier': + case 'ImportSpecifier': + return node === parent.local + + // disregard the `foo` in `foo: while (...) { ... break foo; ... continue foo;}` + case 'LabeledStatement': + case 'BreakStatement': + case 'ContinueStatement': + return false + + default: + return true + } +} diff --git a/packages/plugin-rsc/src/transforms/proxy-export.ts b/packages/plugin-rsc/src/transforms/proxy-export.ts index f08a05701..2a8acc47b 100644 --- a/packages/plugin-rsc/src/transforms/proxy-export.ts +++ b/packages/plugin-rsc/src/transforms/proxy-export.ts @@ -1,7 +1,7 @@ import { tinyassert } from '@hiogawa/utils' import type { Node, Program } from 'estree' import MagicString from 'magic-string' -import { extract_names } from 'periscopic' +import { extractNames } from './analyze-node' import { hasDirective } from './utils' export type TransformProxyExportOptions = { @@ -111,9 +111,9 @@ export function transformProxyExport( } } } - const names = node.declaration.declarations.flatMap((decl) => - extract_names(decl.id), - ) + const names = node.declaration.declarations.flatMap((decl) => [ + ...extractNames(decl.id), + ]) createExport(node, names) } else { node.declaration satisfies never diff --git a/packages/plugin-rsc/src/transforms/utils.ts b/packages/plugin-rsc/src/transforms/utils.ts index aafcecbfc..2462ad0d6 100644 --- a/packages/plugin-rsc/src/transforms/utils.ts +++ b/packages/plugin-rsc/src/transforms/utils.ts @@ -1,6 +1,6 @@ import { tinyassert } from '@hiogawa/utils' import type { Program } from 'estree' -import { extract_names } from 'periscopic' +import { extractNames } from './analyze-node' export function hasDirective( body: Program['body'], @@ -41,7 +41,7 @@ export function getExportNames( * export const foo = 1, bar = 2 */ for (const decl of node.declaration.declarations) { - exportNames.push(...extract_names(decl.id)) + exportNames.push(...extractNames(decl.id)) } } else { node.declaration satisfies never diff --git a/packages/plugin-rsc/src/transforms/wrap-export.ts b/packages/plugin-rsc/src/transforms/wrap-export.ts index 5c6cdcae4..cd2878b6c 100644 --- a/packages/plugin-rsc/src/transforms/wrap-export.ts +++ b/packages/plugin-rsc/src/transforms/wrap-export.ts @@ -1,7 +1,7 @@ import { tinyassert } from '@hiogawa/utils' import type { Node, Program } from 'estree' import MagicString from 'magic-string' -import { extract_names } from 'periscopic' +import { extractNames } from './analyze-node' type ExportMeta = { declName?: string @@ -130,9 +130,9 @@ export function transformWrapExport( 'let', ) } - const names = node.declaration.declarations.flatMap((decl) => - extract_names(decl.id), - ) + const names = node.declaration.declarations.flatMap((decl) => [ + ...extractNames(decl.id), + ]) // treat only simple single decl as function let isFunction = false if (node.declaration.declarations.length === 1) { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 5934d68f5..54ec57fd8 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -442,9 +442,6 @@ importers: magic-string: specifier: ^0.30.21 version: 0.30.21 - periscopic: - specifier: ^4.0.2 - version: 4.0.2 srvx: specifier: ^0.11.12 version: 0.11.12 @@ -3990,9 +3987,6 @@ packages: periscopic@3.1.0: resolution: {integrity: sha512-vKiQ8RRtkl9P+r/+oefh25C3fhybptkHKCZSPlcXiJux2tJF55GnEj3BVn4A5gKfq9NWWXXrxkHBwVPUfH0opw==} - periscopic@4.0.2: - resolution: {integrity: sha512-sqpQDUy8vgB7ycLkendSKS6HnVz1Rneoc3Rc+ZBUCe2pbqlVuCC5vF52l0NJ1aiMg/r1qfYF9/myz8CZeI2rjA==} - picocolors@1.1.1: resolution: {integrity: sha512-xceH2snhtb5M9liqDsmEw56le376mTZkEX/jEb/RxNFyegNul7eNslCXP9FDj/Lcu0X8KEyMceP2ntpaHrDEVA==} @@ -4812,9 +4806,6 @@ packages: youch@4.1.0-beta.10: resolution: {integrity: sha512-rLfVLB4FgQneDr0dv1oddCVZmKjcJ6yX6mS4pU82Mq/Dt9a3cLZQ62pDBL4AUO+uVrCvtWz3ZFUL2HFAFJ/BXQ==} - zimmerframe@1.1.2: - resolution: {integrity: sha512-rAbqEGa8ovJy4pyBxZM70hg4pE6gDgaQ0Sl9M3enG3I0d6H4XSAM3GeNGLKnsBpuijUow064sf7ww1nutC5/3w==} - zwitch@2.0.4: resolution: {integrity: sha512-bXE4cR/kVZhKZX/RjPEflHaKVhUVl85noU3v6b8apfQEc1x4A+zBxjZ4lN8LqGd6WZ3dl98pY4o717VFmoPp+A==} @@ -7619,12 +7610,6 @@ snapshots: estree-walker: 3.0.3 is-reference: 3.0.2 - periscopic@4.0.2: - dependencies: - '@types/estree': 1.0.8 - is-reference: 3.0.2 - zimmerframe: 1.1.2 - picocolors@1.1.1: {} picomatch@4.0.3: {} @@ -8416,6 +8401,4 @@ snapshots: cookie: 1.0.2 youch-core: 0.3.3 - zimmerframe@1.1.2: {} - zwitch@2.0.4: {}