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

[james-elicx]

Description

This PR fixes a bug where periscopic misclassifies block-scoped declarations inside a "use server" function body as closure variables when the same name exists in a scope, causing a syntax error in the hoisted output (ran into this when testing Payload CMS with Vinext).

It essentially collects the names of local declarations from the function's scope and then stop them from being bound.

There's also a few regression tests that were generated by AI.

This is an attempt to upstream a real fix for cloudflare/vinext#527.

[hi-ogawa]

Great find and thanks for the fix!

[hi-ogawa]

I think this wasn't periscopic bug, but it's our misusage or algorithmic issue. I'm trying to fix the root cause and might need to take a time a bit more. I have added a test case which works with your approach but breaks my approach, and another one which breaks yours and works with mine.

[hi-ogawa]

This is the new edge case which your collectLocallyDeclaredNames wasn't able to cover and didn't bind cookies.

[james-elicx]

Thanks for taking the time to look into this further. Sorry, I'm not that familiar with how the plugin works under the hood, so I thought I might have missed something 😓

[james-elicx]

i was doing some experimenting with this one and managed to get it working by walking through the function's body and checking if there is a reference to the variable that isn't shadows in the current scope.

This is what my bindVars looked like with the changes needed for this to work. Unsure whether this is the kind of approach you're after though, so I haven't pushed a commit for it.

const bindVars = [...scope.references].filter((ref) => {
  // own parameters are available in a hoisted function
  if (ownParams.has(ref)) {
    return false
  }

  const owner = scope.find_owner(ref)
  if (!(owner && owner !== scope && owner !== analyzed.scope)) {
    return false
  }

  let shouldBeBound = false
  let shadowDepth = 0

  walk(node.body, {
    enter(node) {
      const innerScope = analyzed.map.get(node)
      if (
        innerScope?.declarations.has(ref) &&
        'body' in node &&
        node !== node.body
      ) {
        // if the node declares a variable with the same name and it's not the function
        // body itself, it shadows the reference we're looking for
        shadowDepth++
      }

      if (
        shadowDepth === 0 &&
        node.type === 'Identifier' &&
        node.name === ref
      ) {
        shouldBeBound = true
        this.skip()
      }
    },
    leave(node) {
      const innerScope = analyzed.map.get(node)
      if (
        innerScope?.declarations.has(ref) &&
        'body' in node &&
        node !== node.body
      ) {
        shadowDepth--
      }
    },
  })

  return shouldBeBound
})

[hi-ogawa]

I was thinking maybe we can re-run periscopic analyze against BlockStatement with "use server". The global variables found there (modulo actual global variables of original program) might give the right bind variables.
(EDIT: hmm, no, my approach doesn't work either since such "global variable" cannot tell if it's actual global or the variable outside)

[james-elicx]

I think this one probably needs to filter the declaration name out of the bind variables, and then inject a line inside the hoisted function the just re-declares the original function name with the value being the hoisted function.

Possibly something like const recuse = (...) => $$hoist_0_recurse(...) after the 'use server' directive.

---

ended up going and playing around with it after i started writing this.

in the bindVars filter callback I added a return false for declName && ref === declName to filter out the function name from the bindings.

then further down below where the function declaration is appended, i did the following:

if (declName && scope.references.has(declName)) {
  const boundArgs =
    bindVars.length > 0 ? `${bindVars.join(', ')}, ` : ''
  const directiveNode = node.body.body[0]!
  output.appendLeft(
    directiveNode.end,
    `\nconst ${declName} = (...$$args) => ${newName}(${boundArgs}...$$args);`,
  )
}

basically just creates a new function declaration that calls the hoisted function with the bound arguments and optionally any other arguments the user calls the function with.

i would caveat that with that i don't think it would work like that if the server action was encrypted though - would probably need doing differently in that case. also didn't try it out with extra levels of nested scopes or shadowing.

[hi-ogawa]

Also I'm wondering maybe we should ditch periscopic and design estree scope management utility from scratch. I think that might be actually easier than working around what periscopic doesn't provide. Such full rewrite might be easier for human to review.

[james-elicx]

Considering the periscopic library is actually rather small, that might be a decent path to take - at least, it gives more control over what's going on.

I'm happy to spend some time trying to come up with something if you would like me to?

[hi-ogawa]

Yes, their library is small, so it looks better to get inspired but write from ground up. I think that's going to be important to tackle more edge cases like in #1155. Please feel free to explore. Thanks for the help!