fix: use server closure variable collision with local declarations#527
fix: use server closure variable collision with local declarations#527james-elicx merged 10 commits intomainfrom
use server closure variable collision with local declarations#527Conversation
|
/bigbonk review |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good fix for a real problem — the periscopic/plugin-rsc interaction with block-scoped declarations inside "use server" functions is a genuine bug, and catching it with a pre-transform plugin is the right approach. The test structure is solid: reproducing the bug without the fix, then verifying the fix works, plus a regression guard for the non-colliding case.
However, there are two bugs in the renamingWalk implementation that would cause incorrect transforms in production code, and collectOuterNames misses function parameters which means it won't detect an important class of collisions.
Bug 1: renamingWalk incorrectly renames property keys and member expression properties
renamingWalk renames every Identifier node matching the name, but not all Identifier nodes are variable references. It needs to skip:
MemberExpression.propertywhencomputed === false—obj.cookiesshould stayobj.cookies, not becomeobj.__local_cookiesProperty.keywhencomputed === falseandshorthand === false—{ cookies: value }should keep its key- Shorthand properties (
{ cookies }) need special handling — the shorthand must be expanded to{ cookies: __local_cookies }, not turned into{ __local_cookies }(which changes the object shape)
I verified this locally:
const obj = { cookies: "test" }; → { __local_cookies: "test" } ← WRONG, changes object shape
const x = formData.cookies; → formData.__local_cookies ← WRONG, accesses wrong property
Bug 2: collectOuterNames only collects VariableDeclaration, not function parameters
If the colliding name comes from a function parameter rather than a const/let/var:
function buildAction(cookies) { // ← parameter, not VariableDeclaration
async function submitAction(formData) {
"use server";
const cookies = formData.get("value");
return cookies;
}
return submitAction;
}collectOuterNames won't include cookies because it only looks at VariableDeclaration nodes. But periscopic still sees the parameter binding in the outer scope and still mis-classifies the inner const cookies as a closure variable — same SyntaxError. The fix needs to also collect names from FunctionDeclaration.params, FunctionExpression.params, ArrowFunctionExpression.params, and CatchClause.param.
Minor: traversal gap when localDecls.size > 0 but collisions.size === 0
When isServerFn is true, children are only visited if localDecls.size === 0 (line 954). If a server function has local declarations but none collide with outer scope, the code falls through to if (!isServerFn) on line 991 — which is false — so children of that node are never visited. Any nested server functions (admittedly rare) would be missed. The child-traversal logic should run for all three cases: no local decls, no collisions, and after handling collisions.
| if (node.type === "Identifier" && node.name === from) { | ||
| s.update(node.start, node.end, to); | ||
| return; | ||
| } |
There was a problem hiding this comment.
renamingWalk renames every Identifier node that matches the name, but not all identifiers are variable references. This needs to distinguish between:
- Variable references (should rename) —
return cookies,cookies + "x", assignment LHS - Non-computed member expression properties (should NOT rename) —
obj.cookieswhere thecookiesIdentifier isMemberExpression.propertywithcomputed: false - Non-computed, non-shorthand object property keys (should NOT rename) —
{ cookies: value }wherecookiesisProperty.keywithcomputed: false - Shorthand object properties (needs expansion) —
{ cookies }must become{ cookies: __local_cookies }, not{ __local_cookies }
Verified locally:
const obj = { cookies: "test" }; → { __local_cookies: "test" } // changes object shape
const x = formData.cookies; → formData.__local_cookies // accesses wrong property
The fix: before recursing into children, check the parent node type and skip the identifier when it is a non-computed property key or member expression property. For shorthand properties, you need to expand them (set shorthand: false equivalent) and only rename the value, not the key. Since you are using MagicString text replacement, the shorthand expansion would need to rewrite cookies as cookies: __local_cookies at the Property node level rather than at the Identifier level.
| for (const decl of n.declarations) { | ||
| collectPatternNames(decl.id, names); | ||
| } | ||
| } |
There was a problem hiding this comment.
collectOuterNames only collects names from VariableDeclaration nodes, but function parameters also create bindings that periscopic sees in the outer scope. This means:
function buildAction(cookies) { // parameter — not a VariableDeclaration
async function submitAction(formData) {
"use server";
const cookies = formData.get("value"); // collision NOT detected
return cookies;
}
return submitAction;
}This needs to also collect from:
FunctionDeclaration.params/FunctionExpression.params/ArrowFunctionExpression.paramsCatchClause.param
Something like:
if (n.type === "FunctionDeclaration" || n.type === "FunctionExpression" || n.type === "ArrowFunctionExpression") {
for (const p of n.params ?? []) collectPatternNames(p, names);
}
if (n.type === "CatchClause" && n.param) {
collectPatternNames(n.param, names);
}
packages/vinext/src/index.ts
Outdated
| } | ||
|
|
||
| // Recurse into children (if not a server function we already handled above) | ||
| if (!isServerFn) { |
There was a problem hiding this comment.
When isServerFn === true and localDecls.size > 0 but collisions.size === 0, the code reaches this point and skips child traversal because isServerFn is true. Any nested "use server" functions inside this function body would be missed entirely.
The child-visit logic should run unconditionally when isServerFn is true — whether there are local decls and collisions or not. The early-return on line 954 handles the localDecls.size === 0 case but the other two paths (collisions found, no collisions) both fall through here and get blocked.
Consider restructuring so that child traversal always happens for server functions after collision handling, or move the !isServerFn guard to only skip when collisions.size > 0 (since renamingWalk already handles the subtree).
|
Review posted successfully. Here's a summary of what I found and reported: Review: REQUEST_CHANGES on PR #527 The fix addresses a real periscopic/plugin-rsc bug where block-scoped I identified two bugs and one minor issue, all verified with local test runs:
|
|
/bigbonk review |
There was a problem hiding this comment.
All three issues from the previous review have been addressed in the current code. Verified by reading the implementation and running the test suite (8/8 pass).
Bug 1 (renamingWalk renaming non-variable identifiers): Fixed — renamingWalk now correctly:
- Skips non-computed
MemberExpression.propertynodes (lines 1037-1043) - Skips non-shorthand
Property.keynodes (lines 1046-1057) - Expands shorthand properties
{ cookies }→{ cookies: __local_cookies }(line 1052) - Skips the value node of shorthand properties to avoid double-rewriting (lines 1060-1063)
Bug 2 (collectOuterNames missing function parameters): Fixed — collectOuterNames now collects from FunctionDeclaration/FunctionExpression/ArrowFunctionExpression params (lines 916-922) and CatchClause.param (lines 924-926).
Minor traversal gap: Fixed — after collision handling, child traversal always runs for server functions (lines 1001-1010), with an explicit return on line 1010 preventing fallthrough to the non-server-function branch.
Test coverage for all three fixes is present (lines 203-334).
|
All three issues from the previous review have already been fixed in the current code. I verified this by:
I posted an approval review on PR #527 confirming the fixes are correct and well-tested. No code changes were needed. |
|
/bigbonk review for bugs |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for bugs |
|
/bigbonk review this pr for bugs |
There was a problem hiding this comment.
Thorough review of the vinext:fix-use-server-closure-collision plugin. The approach is sound — a pre-transform plugin that renames shadowing declarations before plugin-rsc's periscopic analysis is the right solution for this class of bugs. The implementation handles the non-trivial cases well (shorthand properties, member expressions, nested server functions, hoisted vars, catch bindings, function params). Tests are comprehensive with 19 cases covering the original bug, the fix, and numerous regression scenarios.
All 19 tests pass. The code is well-commented and the scoping logic correctly distinguishes block-scoped vs function-scoped names in the ancestor set construction.
A few edge cases that aren't handled, roughly ordered by likelihood of hitting them in real server actions:
1. for...of / for...in declarations are missed by the local-decl scan (false negative)
The local-decl scan at line 988 only checks direct bodyStmts children of type VariableDeclaration. But for (const cookies of arr) { ... } places the VariableDeclaration inside ForOfStatement.left, not as a direct bodyStmt. If cookies also exists in an outer scope, the collision is missed.
This is a false negative (the original SyntaxError remains for this pattern), not a miscompile. Unusual in server actions but possible.
2. class and import declarations not collected as ancestor names (false negative)
collectFunctionScopedNames and the immediate-stmt scan at line 953 only check VariableDeclaration, FunctionDeclaration, and function params. A class cookies {} at module scope (or import { cookies } from ...) creates a binding that periscopic sees, but this plugin won't detect the collision. Again a false negative — the original bug persists for these specific patterns.
3. Label identifiers incorrectly renamed (false positive)
renamingWalk renames every Identifier matching the name unless guarded by the MemberExpression/Property checks. But LabeledStatement.label is also an Identifier that isn't a variable reference. cookies: for (...) would have its label renamed to __local_cookies: for (...). Extremely rare in server actions.
4. No collision check on the __local_ rename target
If a server function already has const __local_cookies = ... and the plugin renames cookies → __local_cookies, it creates a new name collision. Extremely unlikely in practice, but a uniquifying suffix (or counter) would make it bulletproof.
None of these are blocking for the purpose of this PR — the patterns are uncommon in real "use server" functions, and the fix correctly handles the common case that triggered it (Payload CMS). They'd be worth addressing as follow-ups.
|
Review posted as APPROVE on PR #527. Here's a summary: The fix is correct and well-tested for the common case (Payload CMS's I identified 4 edge cases that aren't handled, none of which are blocking:
All are uncommon patterns in real server actions and appropriate as follow-ups rather than blockers. |
- Extend local-decl scan to use collectAllDeclaredNames recursively, fixing false negatives for for...of / for...in loop variables - Collect ClassDeclaration and ImportDeclaration names as ancestor bindings so collisions with class/import names are detected - Guard LabeledStatement.label, BreakStatement.label, ContinueStatement.label in renamingWalk — these are Identifiers but not variable references - Use a suffixed rename target (__local_0_name, etc.) when __local_name is already declared in the function body, preventing secondary collisions - Clean up stale test comments that described the plugin as commented-out - Add 7 new tests covering all of the above cases (26 total, all passing)
|
/bigbonk review this pr for bugs |
There was a problem hiding this comment.
Review: vinext:fix-use-server-closure-collision
Thorough review of the plugin implementation and test suite. All 26 tests pass.
Summary
The approach is correct: a "pre"-enforced Vite plugin that renames shadowing declarations inside "use server" function bodies before plugin-rsc's periscopic analysis misclassifies them as closure variables. The implementation handles the tricky AST cases well.
What's done right
Scope analysis is solid. The two-tier collection strategy — collectFunctionScopedNames for hoisted var/FunctionDeclaration names that cross block boundaries, plus immediate-statement scanning for block-scoped let/const/class/import — correctly models JavaScript's scoping rules. The ancestorNames set is accumulated incrementally as the tree is descended, avoiding the false-positive problem of picking up names from sibling scopes.
renamingWalk correctness guards are comprehensive. Non-computed MemberExpression.property, non-computed Property.key, shorthand property expansion ({ cookies } → { cookies: __local_cookies }), LabeledStatement.label, BreakStatement.label, ContinueStatement.label — all handled correctly. The renamedRanges Set prevents double s.update() on the same span (which MagicString throws on), and the shorthand value guard at line 1127 correctly handles the case where key and value are the same AST node with shared source positions.
Nested server function handling is correct. After renaming collisions in an outer server function, the namesForChildren set is updated to remove the original name and add the renamed version (lines 1037-1045). This prevents false-positive collision detection in nested server functions that independently declare the same name.
Rename target uniqueness (lines 1013-1018) avoids secondary collisions when __local_<name> is already declared, falling back to __local_0_<name>, etc.
Test coverage is excellent — 26 tests covering the original bug reproduction, the fix, and regression cases for every known edge case (sibling scope false positives, double s.update, directive prologue strictness, hoisted var in nested functions, shorthand properties, module-level declarations, catch bindings, for...of/for...in, class/import declarations, label identifiers, rename target uniqueness).
Minor observations (non-blocking)
-
collectAllDeclaredNamesdoesn't collectClassDeclarationnames as local declarations. If a"use server"function body containsclass cookies {}, it won't be added tolocalDecls, so the collision with an outercookiesbinding would be missed. In contrast, the ancestor-name collection at line 961 correctly handlesClassDeclaration. This is a false-negative edge case that's extremely unlikely in real server actions. -
collectAllDeclaredNamesdoesn't have an early return forClassDeclaration. UnlikecollectFunctionScopedNameswhich stops at class bodies (line 1216-1218),collectAllDeclaredNamesrecurses into them. This is benign (class bodies containMethodDefinition/PropertyDefinition, notVariableDeclaration) but does unnecessary work. Consistency would be nice but not important. -
Generic
Object.keystraversal visits non-AST properties. The traversal at lines 972-980, 1073-1081, and 1176-1184 iterates all object keys and recurses into anything with a.typeproperty. This works because Rollup's AST nodes don't have extra objects with.typethat aren't AST nodes, but it's slightly more fragile than using a visitor with known child-key lists. Fine for a pre-transform plugin that only needs to be "good enough" before plugin-rsc.
None of these affect correctness for the patterns this fix targets.
|
Review posted on PR #527 with approval. Here's a summary: Approved. The fix is correct and well-tested (26/26 tests pass). The plugin correctly addresses a real periscopic/plugin-rsc bug where block-scoped declarations inside
I noted 2 minor non-blocking observations (missing |
collectAllDeclaredNames was missing a ClassDeclaration case, so a
`class cookies {}` declared inside a 'use server' function body would
not be added to localDecls. The collision with an outer `cookies`
binding would be silently missed — the original periscopic SyntaxError
would persist for this pattern.
Fix by adding a ClassDeclaration branch that records the class name and
returns without recursing into the class body (same pattern already used
for FunctionDeclaration and consistent with collectFunctionScopedNames).
Adds a regression test for the class-in-body collision case (27 tests total).
|
/bigbonk review this pr for bugs |
There was a problem hiding this comment.
Review: APPROVE
All 27 tests pass. The implementation is correct and well-tested.
What this fixes
@vitejs/plugin-rsc uses periscopic for scope analysis before hoisting "use server" inline functions. Due to how periscopic models BlockStatement scopes, a const/let declaration inside a server function body gets classified as a closure variable from the outer scope rather than a local binding. plugin-rsc then injects const [name] = await decryptActionBoundArgs(...) at the top of the hoisted function, colliding with the existing declaration and producing a SyntaxError.
Why the approach is sound
A pre-transform rename plugin (enforce: "pre") is the right solution. It runs before plugin-rsc's periscopic analysis, so by the time periscopic sees the code, the shadowing name is gone. The renamed __local_<name> is never referenced in the outer scope, so periscopic correctly ignores it. This avoids any need to patch periscopic or plugin-rsc internals.
Implementation quality
The scope analysis is well-modeled:
- Two-tier name collection (
collectFunctionScopedNamesfor hoistedvar/FunctionDeclarationvs immediate-statement scanning for block-scopedlet/const/class/import) correctly reflects JavaScript's scoping rules ancestorNamesis accumulated incrementally during descent rather than being a whole-AST pass, which avoids false positives from sibling scopesnamesForChildrenis correctly adjusted post-rename (removing the original name, adding the renamed one) to prevent cascading false positives in nested server functions
renamingWalk handles all the non-trivial AST identity cases:
- Non-computed
MemberExpression.property— skip - Non-computed non-shorthand
Property.key— skip - Shorthand
Property{ x }— expand to{ x: __local_x }preserving object shape LabeledStatement.label,BreakStatement.label,ContinueStatement.label— skiprenamedRangesSet prevents doubles.update()on same span- Stops at nested functions that re-declare the name (including hoisted
var) - Stops at nested
"use server"functions (handled independently byvisitNode)
The rename target uniqueness logic (lines 1013-1018) handles the edge case where __local_<name> is already declared by falling back to __local_0_<name>, etc.
Test coverage
26 focused test cases plus 1 plugin-presence check, covering:
- Core bug reproduction and fix verification
- False positive prevention (sibling scopes, block-scoped siblings, directive prologue strictness)
- AST edge cases (shorthand properties, member expressions, labels, nested server functions, hoisted vars)
- All binding types as ancestor names (var/let/const, function params, catch params, function declarations, class declarations, imports)
- All local-decl sites (direct statements, for...of/for...in loop variables, class declarations in body)
- Rename target collision avoidance
Clean implementation, good comments, well-tested.
|
Reviewed and approved PR #527. Here's my assessment: All 27 tests pass. The implementation is correct. The fix addresses a real periscopic/plugin-rsc bug where block-scoped declarations inside Key implementation strengths:
|
|
1k lines approved first pass 😮 Do you do the same loop locally before bonk? |
With these kinds of pieces of work, I'm very involved and treat it more like pair programming where I frequently guide what the agent is doing, and will often interrupt the agent if it veers off-course from where I think it should be going. So I get very heavily involved in getting these bigger pieces of work in the direction I want them to go. Then I review it myself and also have the agent review the diff as well, while continuously testing it with my reproduction. Depending on what it is, I will often use the agent panel in Zed where I'm able to edit the code at the same time as the agent. It's usually small things or addressing comments that I give the agent a bit more freedom on. I generally avoid going full hands-off for things that aren't small. |
2 participants
This was observed in the wild with Payload CMS.
Fix
Add a
"pre"-enforced Vite plugin (vinext:fix-use-server-closure-collision) that runs beforeplugin-rscsees the file. For any"use server"function whose body declares a variable that shadows an outer-scope name, it renames the inner declaration and all its usages within that function body to__local_<name>. The outer binding is untouched.After the rename,
periscopicno longer seescookiesreferenced inside the action, so it is not added tobindVarsat all. Only genuinely-closed-over variables (likeconfig, which is not redeclared inside the action) remain.Tests
tests/use-server-closure-collision.test.ts— unit tests that exercise the plugin transform directly:transformHoistInlineDirectiveon raw source and confirming the duplicate declaration