fix(compiler): extract "use server" function declarations nested in a function

.changeset/extract-nested-server-function-declarations.md

@@ -0,0 +1,9 @@
+---
+"@solidjs/compiler": patch
+---
+
+A `"use server"` function declaration nested inside another function is now extracted like any other server function.
+
+- Previously the directive on a nested declaration was silently ignored: the body shipped to the client and ran there, while captures from the enclosing function were still rejected at compile time.
+- The declaration is hoisted to a `const` at the top of its block, so calling it before its source position still works.
+- Its id follows the binding path, such as `outer.inner`.

packages/compiler/README.md

@@ -153,7 +153,7 @@ result.functions; // [{ id, name, exports }] for manifest building
 
 A function-level `"use server"` function is extracted out of its lexical position, so it may only reference its own parameters and locals, module top-level bindings, and globals. Capturing anything else is a compile error. That includes `this` and `arguments` in a marked arrow, which an arrow takes from the function it was written in. Use a `function` if the server function needs its own `this` or `arguments`.
 
-A function-level directive only works where the pass can extract the function: a function declaration, a function expression, or an arrow with a block body. Methods, getters, and setters are never extracted, so a directive on one is a compile error rather than a directive that silently does nothing. Assign a function to a property instead.
+A function-level directive only works where the pass can extract the function: a function declaration (at any nesting depth), a function expression, or an arrow with a block body. Methods, getters, and setters are never extracted, so a directive on one is a compile error rather than a directive that silently does nothing. Assign a function to a property instead.
 
 A module-level `"use server"` module can only export server functions. Its client build is rebuilt from those exports alone, so anything else would be missing from the browser bundle. Re-exports, `export *`, class and enum exports, destructured exports, and exports declared without an initializer are compile errors naming the export and its position. Type-only and `declare` exports are erased and are fine.
 

packages/compiler/__tests__/directives-id-scheme.test.js

@@ -299,9 +299,10 @@ describe("the wire id scheme, differentially", () => {
   });
 
   it("takes a segment from a nested function declaration", () => {
-    // Declarations nested inside another function are not bubbled into a
-    // `const`, so the function itself has to contribute the segment, or two
-    // same-named arrows in sibling declarations collide on `outer.s`.
+    // Two same-named arrows in sibling declarations must not collide on
+    // `outer.s`. Reported in post-bubble order (nested declarations hoist in
+    // reverse source order like top-level ones), which moves the reporting
+    // order without moving an id.
     const source = `
       export function outer() {
         function a() { const s = async () => { "use server"; return 1; }; return s; }
@@ -310,9 +311,23 @@ describe("the wire id scheme, differentially", () => {
       }
     `;
     const hash = hashHex("src/decl.js");
-    expect(ids(source, { filename: "/project/src/decl.js", root: "/project" })).toEqual([
-      `outer.a.s-${hash}`,
-      `outer.b.s-${hash}`
+    expect(ids(source, { filename: "/project/src/decl.js", root: "/project" }).sort()).toEqual(
+      [`outer.a.s-${hash}`, `outer.b.s-${hash}`].sort()
+    );
+  });
+
+  it("names a marked nested function declaration by its path", () => {
+    // A declaration nested in another function is bubbled into a `const`
+    // like a top-level one, so it is extracted and named the same way.
+    const source = `
+      export function outer() {
+        async function inner() { "use server"; return 1; }
+        return inner;
+      }
+    `;
+    const hash = hashHex("src/nested-decl.js");
+    expect(ids(source, { filename: "/project/src/nested-decl.js", root: "/project" })).toEqual([
+      `outer.inner-${hash}`
     ]);
   });
 

packages/compiler/__tests__/directives-nested-declarations.test.js

@@ -0,0 +1,155 @@
+// Function declarations nested inside another function carrying a
+// function-level `"use server"` directive.
+//
+// The bubbling pre-pass turns every function declaration into
+// `const name = function name() {}` at the top of its block so the transform
+// only has to handle expression forms. The Babel original skipped the body of
+// each bubbled declaration, so a declaration nested inside another function
+// was never bubbled and a directive on it was silently ignored: the body
+// shipped to the client and ran there, while the capture validator (which
+// walks every function) still rejected its captures. These tests pin that a
+// nested declaration is extracted like any other marked function, in both
+// modes, and that hoisting still holds after the rewrite.
+
+const path = require("path");
+
+const compilerDir = path.resolve(__dirname, "..");
+const { transformDirectives } = require(compilerDir);
+
+const RUNTIME = "@solidjs/web/server-functions";
+const ROOT = "/project";
+const FILENAME = `${ROOT}/src/module.js`;
+
+function compile(code, overrides = {}) {
+  return transformDirectives(code, {
+    filename: FILENAME,
+    root: ROOT,
+    mode: "server",
+    env: "production",
+    directive: "use server",
+    register: { kind: "named", name: "registerServerReference", source: RUNTIME },
+    create: { kind: "named", name: "createServerReference", source: RUNTIME },
+    ...overrides
+  });
+}
+
+const lines = code => code.split("\n").map(line => line.trim());
+
+describe("nested function declarations", () => {
+  const source = [
+    "export function outer() {",
+    "  async function inner() {",
+    '    "use server";',
+    "    return 1;",
+    "  }",
+    "  return inner;",
+    "}"
+  ].join("\n");
+
+  it("extracts a marked declaration nested in a function (server)", () => {
+    const result = compile(source);
+    expect(result.valid).toBe(true);
+    expect(result.functions.map(f => f.name)).toEqual(["outer.inner"]);
+    const out = lines(result.code);
+    // The body is registered at module top level and the binding inside
+    // `outer` becomes a reference to it, exactly like a top-level function.
+    expect(out).toContain(
+      'const serverFunction_1 = registerServerReference_1("outer.inner-' +
+        hashOf(result) +
+        '", async function inner() {'
+    );
+    expect(out).toContain("const inner = createServerReference_1(serverFunction_1);");
+    expect(result.code).not.toMatch(/"use server"/);
+  });
+
+  it("replaces the declaration with a proxy on the client", () => {
+    const result = compile(source, { mode: "client" });
+    expect(result.valid).toBe(true);
+    const out = lines(result.code);
+    expect(out).toContain(
+      `const inner = createServerReference_1("outer.inner-${hashOf(result)}");`
+    );
+    // The server body is gone from the client build.
+    expect(result.code).not.toMatch(/return 1/);
+    expect(result.code).not.toMatch(/registerServerReference/);
+  });
+
+  it("keeps the declaration callable before its source position", () => {
+    // A function declaration is hoisted to the top of its scope. The bubbled
+    // `const` lands at the top of the block, so a use above the declaration
+    // still resolves after the rewrite.
+    const code = [
+      "export function outer() {",
+      "  const ref = inner;",
+      "  async function inner() {",
+      '    "use server";',
+      "    return 1;",
+      "  }",
+      "  return ref;",
+      "}"
+    ].join("\n");
+    const out = lines(compile(code).code);
+    const declaration = out.indexOf("const inner = createServerReference_1(serverFunction_1);");
+    const use = out.indexOf("const ref = inner;");
+    expect(declaration).toBeGreaterThan(-1);
+    expect(use).toBeGreaterThan(declaration);
+  });
+
+  it("extracts through several levels of nesting", () => {
+    const code = [
+      "export function outer() {",
+      "  function mid() {",
+      '    async function inner() { "use server"; return 1; }',
+      "    return inner;",
+      "  }",
+      "  return mid;",
+      "}"
+    ].join("\n");
+    const result = compile(code);
+    expect(result.functions.map(f => f.name)).toEqual(["outer.mid.inner"]);
+  });
+
+  it("extracts a declaration inside a block inside a function", () => {
+    const code = [
+      "export function outer(flag) {",
+      "  if (flag) {",
+      '    async function inner() { "use server"; return 1; }',
+      "    return inner;",
+      "  }",
+      "}"
+    ].join("\n");
+    const result = compile(code);
+    expect(result.functions.map(f => f.name)).toEqual(["outer.inner"]);
+  });
+
+  it("rejects a nested declaration that captures an enclosing local", () => {
+    // Extraction and validation agree: the same function that is now
+    // extracted is the one whose captures were already being checked.
+    const code = [
+      "export function outer(db) {",
+      '  async function inner() { "use server"; return db.x; }',
+      "  return inner;",
+      "}"
+    ].join("\n");
+    expect(() => compile(code)).toThrow(/`db` is declared in an enclosing function/);
+  });
+
+  it("leaves an unmarked nested declaration's body in place", () => {
+    const code = [
+      "export function outer() {",
+      "  function helper() { return 2; }",
+      '  const go = async () => { "use server"; return 1; };',
+      "  return [helper, go];",
+      "}"
+    ].join("\n");
+    const result = compile(code, { mode: "client" });
+    expect(result.functions.map(f => f.name)).toEqual(["outer.go"]);
+    expect(result.code).toMatch(/return 2/);
+  });
+});
+
+// The file hash is whatever the pass produced; the scheme itself is pinned
+// differentially in directives-id-scheme.test.js.
+function hashOf(result) {
+  return result.functions[0].id.split("-")[1];
+}

packages/compiler/src/directives/transform.rs

@@ -702,13 +702,36 @@ impl<'a> DirectivesTransform<'a> {
 
 /// Babel's function-level pre-pass bubbles *every* function declaration to a
 /// `const` at the top of its enclosing block (so exports keep working and the
-/// directive transform only has to handle expression forms). Declarations
-/// nested inside another bubbled declaration are skipped, matching Babel's
-/// `tmp.skip()`.
+/// directive transform only has to handle expression forms).
+///
+/// Babel skipped the body of each bubbled declaration (`tmp.skip()`), so a
+/// declaration nested inside another function was never bubbled, never became
+/// an expression, and a `"use server"` directive on it was silently ignored,
+/// while the capture validator still treated it as a server function. This
+/// pass descends into bubbled bodies too, so nested declarations are
+/// extracted like every other marked function. Only files that extract at
+/// least one function keep the pass's output, so the extra bubbling never
+/// reaches a file without server functions.
 struct Bubbler<'ctx, 'a> {
     transform: &'ctx mut DirectivesTransform<'a>,
 }
 
+impl<'ctx, 'a> Bubbler<'ctx, 'a> {
+    /// Bubbles the declarations inside `function`'s body, then turns the
+    /// declaration itself into `const name = function name() {}`.
+    fn bubble_declaration(
+        &mut self,
+        mut function: oxc_allocator::Box<'a, oxc_ast::ast::Function<'a>>,
+    ) -> Statement<'a> {
+        walk_mut::walk_function(
+            self,
+            &mut function,
+            oxc_syntax::scope::ScopeFlags::Function,
+        );
+        self.transform.function_declaration_to_const(function)
+    }
+}
+
 impl<'a> VisitMut<'a> for Bubbler<'_, 'a> {
     fn visit_statements(&mut self, statements: &mut ArenaVec<'a, Statement<'a>>) {
         let ast = self.transform.ast();
@@ -718,7 +741,7 @@ impl<'a> VisitMut<'a> for Bubbler<'_, 'a> {
         for statement in old {
             match statement {
                 Statement::FunctionDeclaration(function) if function.id.is_some() => {
-                    hoisted.push(self.transform.function_declaration_to_const(function));
+                    hoisted.push(self.bubble_declaration(function));
                 }
                 Statement::ExportDeclaration(export)
                     if matches!(
@@ -731,7 +754,7 @@ impl<'a> VisitMut<'a> for Bubbler<'_, 'a> {
                         unreachable!("shape checked above");
                     };
                     let name = function.id.as_ref().unwrap().name.to_string();
-                    hoisted.push(self.transform.function_declaration_to_const(function));
+                    hoisted.push(self.bubble_declaration(function));
                     rest.push(
                         self.transform
                             .export_named_specifier_statement(&name, &name),
@@ -753,7 +776,7 @@ impl<'a> VisitMut<'a> for Bubbler<'_, 'a> {
                         unreachable!("shape checked above");
                     };
                     let name = function.id.as_ref().unwrap().name.to_string();
-                    hoisted.push(self.transform.function_declaration_to_const(function));
+                    hoisted.push(self.bubble_declaration(function));
                     export.declaration =
                         ExportDefaultDeclarationKind::from(self.transform.identifier(&name));
                     rest.push(Statement::ExportDefaultDeclaration(export));