Don't hoist a server function if a compile error was emitted

This allows us to skip unnecessary work, and also avoids generating invalid code, e.g. not allowed `super` calls in hoisted methods.
vercel · Nov 25, 2024 · 241806a · 241806a
1 parent 3ddd390
commit 241806a
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 123 deletions.
diff --git a/crates/next-custom-transforms/src/transforms/server_actions.rs b/crates/next-custom-transforms/src/transforms/server_actions.rs
@@ -131,6 +131,7 @@ pub fn server_actions<C: Comments>(file_name: &FileName, config: Config, comment
         has_action: false,
         has_cache: false,
         this_status: ThisStatus::Allowed,
+        did_emit_error: false,
 
         reference_index: 0,
         in_module_level: true,
@@ -182,6 +183,7 @@ struct ServerActions<C: Comments> {
     has_action: bool,
     has_cache: bool,
     this_status: ThisStatus,
+    did_emit_error: bool,
 
     reference_index: u32,
     in_module_level: bool,
@@ -213,6 +215,11 @@ struct ServerActions<C: Comments> {
 }
 
 impl<C: Comments> ServerActions<C> {
+    fn emit_error(&mut self, error_kind: ServerActionsErrorKind) {
+        emit_error(error_kind);
+        self.did_emit_error = true;
+    }
+
     fn generate_server_reference_id(
         &self,
         export_name: &str,
@@ -345,13 +352,12 @@ impl<C: Comments> ServerActions<C> {
         // Even if it's a file-level action or cache module, the function body
         // might still have directives that override the module-level annotations.
         if let Some(body) = maybe_body {
-            let directive_visitor = &mut DirectiveVisitor {
-                config: &self.config,
-                directive: None,
-                has_file_directive: self.file_directive.is_some(),
-                is_allowed_position: true,
-                location: DirectiveLocation::FunctionBody,
-            };
+            let directive_visitor = &mut DirectiveVisitor::new(
+                &self.config,
+                DirectiveLocation::FunctionBody,
+                None,
+                self.file_directive.is_some(),
+            );
 
             body.stmts.retain(|stmt| {
                 let has_directive = directive_visitor.visit_stmt(stmt);
@@ -360,6 +366,7 @@ impl<C: Comments> ServerActions<C> {
             });
 
             directive = directive_visitor.directive.clone();
+            self.did_emit_error |= directive_visitor.did_emit_error;
         }
 
         // All exported functions inherit the file directive if they don't have their own directive.
@@ -371,13 +378,8 @@ impl<C: Comments> ServerActions<C> {
     }
 
     fn get_directive_for_module(&mut self, stmts: &mut Vec<ModuleItem>) -> Option<Directive> {
-        let directive_visitor = &mut DirectiveVisitor {
-            config: &self.config,
-            directive: None,
-            has_file_directive: false,
-            is_allowed_position: true,
-            location: DirectiveLocation::Module,
-        };
+        let directive_visitor =
+            &mut DirectiveVisitor::new(&self.config, DirectiveLocation::Module, None, false);
 
         stmts.retain(|item| {
             if let ModuleItem::Stmt(stmt) = item {
@@ -941,6 +943,7 @@ impl<C: Comments> VisitMut for ServerActions<C> {
     }
 
     fn visit_mut_function(&mut self, f: &mut Function) {
+        let old_did_emit_error = replace(&mut self.did_emit_error, false);
         let directive = self.get_directive_for_function(f.body.as_mut());
         let declared_idents_until = self.declared_idents.len();
         let current_names = take(&mut self.names);
@@ -967,26 +970,27 @@ impl<C: Comments> VisitMut for ServerActions<C> {
             self.fn_decl_ident = old_fn_decl_ident;
         }
 
-        if !self.config.is_react_server_layer {
-            return;
-        }
-
-        let mut child_names = if self.should_track_names {
-            let names = take(&mut self.names);
-            self.names = current_names;
-            self.names.extend(names.iter().cloned());
-            names
-        } else {
-            take(&mut self.names)
-        };
-
         if let Some(directive) = directive {
+            let mut child_names = if self.should_track_names {
+                let names = take(&mut self.names);
+                self.names = current_names;
+                self.names.extend(names.iter().cloned());
+                names
+            } else {
+                take(&mut self.names)
+            };
+
             if !f.is_async {
-                emit_error(ServerActionsErrorKind::InlineSyncFunction {
+                self.emit_error(ServerActionsErrorKind::InlineSyncFunction {
                     span: f.span,
-                    directive,
+                    directive: directive.clone(),
                 });
+            }
 
+            let error_emitted_for_fn = replace(&mut self.did_emit_error, old_did_emit_error);
+
+            // Don't hoist a function if 1) an error was emitted, or 2) we're in the client layer.
+            if error_emitted_for_fn || !self.config.is_react_server_layer {
                 return;
             }
 
@@ -1152,7 +1156,7 @@ impl<C: Comments> VisitMut for ServerActions<C> {
 
         if let Some(directive) = directive {
             if !a.is_async {
-                emit_error(ServerActionsErrorKind::InlineSyncFunction {
+                self.emit_error(ServerActionsErrorKind::InlineSyncFunction {
                     span: a.span,
                     directive,
                 });
@@ -1310,11 +1314,11 @@ impl<C: Comments> VisitMut for ServerActions<C> {
             let (is_action_fn, is_cache_fn) = has_body_directive(&n.function.body);
 
             if is_action_fn {
-                emit_error(
+                self.emit_error(
                     ServerActionsErrorKind::InlineUseServerInClassInstanceMethod { span: n.span },
                 );
             } else if is_cache_fn {
-                emit_error(
+                self.emit_error(
                     ServerActionsErrorKind::InlineUseCacheInClassInstanceMethod { span: n.span },
                 );
             } else {
@@ -1726,7 +1730,7 @@ impl<C: Comments> VisitMut for ServerActions<C> {
                 }
 
                 if disallowed_export_span != DUMMY_SP {
-                    emit_error(ServerActionsErrorKind::ExportedSyncFunction {
+                    self.emit_error(ServerActionsErrorKind::ExportedSyncFunction {
                         span: disallowed_export_span,
                         in_action_file,
                     });
@@ -2132,7 +2136,7 @@ impl<C: Comments> VisitMut for ServerActions<C> {
 
     fn visit_mut_this_expr(&mut self, n: &mut ThisExpr) {
         if let ThisStatus::Forbidden { directive } = &self.this_status {
-            emit_error(ServerActionsErrorKind::ForbiddenExpression {
+            self.emit_error(ServerActionsErrorKind::ForbiddenExpression {
                 span: n.span,
                 expr: "this".into(),
                 directive: directive.clone(),
@@ -2143,7 +2147,7 @@ impl<C: Comments> VisitMut for ServerActions<C> {
     fn visit_mut_ident(&mut self, n: &mut Ident) {
         if n.sym == *"arguments" {
             if let ThisStatus::Forbidden { directive } = &self.this_status {
-                emit_error(ServerActionsErrorKind::ForbiddenExpression {
+                self.emit_error(ServerActionsErrorKind::ForbiddenExpression {
                     span: n.span,
                     expr: "arguments".into(),
                     directive: directive.clone(),
@@ -2578,9 +2582,31 @@ struct DirectiveVisitor<'a> {
     directive: Option<Directive>,
     has_file_directive: bool,
     is_allowed_position: bool,
+    did_emit_error: bool,
 }
 
 impl DirectiveVisitor<'_> {
+    fn new(
+        config: &Config,
+        location: DirectiveLocation,
+        directive: Option<Directive>,
+        has_file_directive: bool,
+    ) -> DirectiveVisitor {
+        DirectiveVisitor {
+            config,
+            location,
+            directive,
+            has_file_directive,
+            is_allowed_position: true,
+            did_emit_error: false,
+        }
+    }
+
+    fn emit_error(&mut self, error_kind: ServerActionsErrorKind) {
+        emit_error(error_kind);
+        self.did_emit_error = true;
+    }
+
     /**
      * Returns `true` if the statement contains a server directive.
      * The found directive is assigned to `DirectiveVisitor::directive`.
@@ -2596,11 +2622,11 @@ impl DirectiveVisitor<'_> {
             }) => {
                 if value == "use server" {
                     if in_fn_body && !allow_inline {
-                        emit_error(ServerActionsErrorKind::InlineUseServerInClientComponent {
+                        self.emit_error(ServerActionsErrorKind::InlineUseServerInClientComponent {
                             span: *span,
                         })
                     } else if let Some(Directive::UseCache { .. }) = self.directive {
-                        emit_error(ServerActionsErrorKind::MultipleDirectives {
+                        self.emit_error(ServerActionsErrorKind::MultipleDirectives {
                             span: *span,
                             location: self.location.clone(),
                         });
@@ -2609,15 +2635,15 @@ impl DirectiveVisitor<'_> {
 
                         return true;
                     } else {
-                        emit_error(ServerActionsErrorKind::MisplacedDirective {
+                        self.emit_error(ServerActionsErrorKind::MisplacedDirective {
                             span: *span,
                             directive: value.to_string(),
                             location: self.location.clone(),
                         });
                     }
                 } else if detect_similar_strings(value, "use server") {
                     // Detect typo of "use server"
-                    emit_error(ServerActionsErrorKind::MisspelledDirective {
+                    self.emit_error(ServerActionsErrorKind::MisspelledDirective {
                         span: *span,
                         directive: value.to_string(),
                         expected_directive: "use server".to_string(),
@@ -2626,17 +2652,17 @@ impl DirectiveVisitor<'_> {
                 // `use cache` or `use cache: foo`
                 if value == "use cache" || value.starts_with("use cache: ") {
                     if in_fn_body && !allow_inline {
-                        emit_error(ServerActionsErrorKind::InlineUseCacheInClientComponent {
+                        self.emit_error(ServerActionsErrorKind::InlineUseCacheInClientComponent {
                             span: *span,
                         })
                     } else if let Some(Directive::UseServer) = self.directive {
-                        emit_error(ServerActionsErrorKind::MultipleDirectives {
+                        self.emit_error(ServerActionsErrorKind::MultipleDirectives {
                             span: *span,
                             location: self.location.clone(),
                         });
                     } else if self.is_allowed_position {
                         if !self.config.dynamic_io_enabled {
-                            emit_error(ServerActionsErrorKind::UseCacheWithoutDynamicIO {
+                            self.emit_error(ServerActionsErrorKind::UseCacheWithoutDynamicIO {
                                 span: *span,
                                 directive: value.to_string(),
                             });
@@ -2651,7 +2677,7 @@ impl DirectiveVisitor<'_> {
                             let cache_kind = RcStr::from(value.split_at("use cache: ".len()).1);
 
                             if !self.config.cache_kinds.contains(&cache_kind) {
-                                emit_error(ServerActionsErrorKind::UnknownCacheKind {
+                                self.emit_error(ServerActionsErrorKind::UnknownCacheKind {
                                     span: *span,
                                     cache_kind: cache_kind.clone(),
                                 });
@@ -2662,7 +2688,7 @@ impl DirectiveVisitor<'_> {
 
                         return true;
                     } else {
-                        emit_error(ServerActionsErrorKind::MisplacedDirective {
+                        self.emit_error(ServerActionsErrorKind::MisplacedDirective {
                             span: *span,
                             directive: value.to_string(),
                             location: self.location.clone(),
@@ -2671,7 +2697,7 @@ impl DirectiveVisitor<'_> {
                 } else {
                     // Detect typo of "use cache"
                     if detect_similar_strings(value, "use cache") {
-                        emit_error(ServerActionsErrorKind::MisspelledDirective {
+                        self.emit_error(ServerActionsErrorKind::MisspelledDirective {
                             span: *span,
                             directive: value.to_string(),
                             expected_directive: "use cache".to_string(),
@@ -2691,25 +2717,25 @@ impl DirectiveVisitor<'_> {
                 // Match `("use server")`.
                 if value == "use server" || detect_similar_strings(value, "use server") {
                     if self.is_allowed_position {
-                        emit_error(ServerActionsErrorKind::WrappedDirective {
+                        self.emit_error(ServerActionsErrorKind::WrappedDirective {
                             span: *span,
                             directive: "use server".to_string(),
                         });
                     } else {
-                        emit_error(ServerActionsErrorKind::MisplacedWrappedDirective {
+                        self.emit_error(ServerActionsErrorKind::MisplacedWrappedDirective {
                             span: *span,
                             directive: "use server".to_string(),
                             location: self.location.clone(),
                         });
                     }
                 } else if value == "use cache" || detect_similar_strings(value, "use cache") {
                     if self.is_allowed_position {
-                        emit_error(ServerActionsErrorKind::WrappedDirective {
+                        self.emit_error(ServerActionsErrorKind::WrappedDirective {
                             span: *span,
                             directive: "use cache".to_string(),
                         });
                     } else {
-                        emit_error(ServerActionsErrorKind::MisplacedWrappedDirective {
+                        self.emit_error(ServerActionsErrorKind::MisplacedWrappedDirective {
                             span: *span,
                             directive: "use cache".to_string(),
                             location: self.location.clone(),

diff --git a/crates/next-custom-transforms/tests/errors/server-actions/server-graph/17/output.js b/crates/next-custom-transforms/tests/errors/server-actions/server-graph/17/output.js
@@ -1,11 +1,3 @@
-/* __next_internal_action_entry_do_not_use__ {"803128060c414d59f8552e4788b846c0d2b7f74743":"$$RSC_SERVER_CACHE_0"} */ import { registerServerReference } from "private-next-rsc-server-reference";
-import { encryptActionBoundArgs, decryptActionBoundArgs } from "private-next-rsc-action-encryption";
-import { cache as $$cache__ } from "private-next-rsc-cache-wrapper";
-export var $$RSC_SERVER_CACHE_0 = $$cache__("x", "803128060c414d59f8552e4788b846c0d2b7f74743", 0, /*#__TURBOPACK_DISABLE_EXPORT_MERGING__*/ async function foo() {
+export async function foo() {
     return 'data';
-});
-Object.defineProperty($$RSC_SERVER_CACHE_0, "name", {
-    "value": "foo",
-    "writable": false
-});
-export var foo = registerServerReference($$RSC_SERVER_CACHE_0, "803128060c414d59f8552e4788b846c0d2b7f74743", null);
+}
diff --git a/crates/next-custom-transforms/tests/errors/server-actions/server-graph/20/output.js b/crates/next-custom-transforms/tests/errors/server-actions/server-graph/20/output.js
@@ -1,11 +1,3 @@
-/* __next_internal_action_entry_do_not_use__ {"803128060c414d59f8552e4788b846c0d2b7f74743":"$$RSC_SERVER_CACHE_0"} */ import { registerServerReference } from "private-next-rsc-server-reference";
-import { encryptActionBoundArgs, decryptActionBoundArgs } from "private-next-rsc-action-encryption";
-import { cache as $$cache__ } from "private-next-rsc-cache-wrapper";
-export var $$RSC_SERVER_CACHE_0 = $$cache__("default", "803128060c414d59f8552e4788b846c0d2b7f74743", 0, /*#__TURBOPACK_DISABLE_EXPORT_MERGING__*/ async function fn() {
+export async function fn() {
     'use server';
-});
-Object.defineProperty($$RSC_SERVER_CACHE_0, "name", {
-    "value": "fn",
-    "writable": false
-});
-export var fn = registerServerReference($$RSC_SERVER_CACHE_0, "803128060c414d59f8552e4788b846c0d2b7f74743", null);
+}
diff --git a/crates/next-custom-transforms/tests/errors/server-actions/server-graph/22/output.js b/crates/next-custom-transforms/tests/errors/server-actions/server-graph/22/output.js
@@ -1,13 +1,12 @@
-/*#__TURBOPACK_DISABLE_EXPORT_MERGING__*/ /* __next_internal_action_entry_do_not_use__ {"006a88810ecce4a4e8b59d53b8327d7e98bbf251d7":"$$RSC_SERVER_ACTION_0","8069348c79fce073bae2f70f139565a2fda1c74c74":"$$RSC_SERVER_CACHE_2","80951c375b4a6a6e89d67b743ec5808127cfde405d":"$$RSC_SERVER_CACHE_1"} */ import { registerServerReference } from "private-next-rsc-server-reference";
+/* __next_internal_action_entry_do_not_use__ {"006a88810ecce4a4e8b59d53b8327d7e98bbf251d7":"$$RSC_SERVER_ACTION_0"} */ import { registerServerReference } from "private-next-rsc-server-reference";
 import { encryptActionBoundArgs, decryptActionBoundArgs } from "private-next-rsc-action-encryption";
-import { cache as $$cache__ } from "private-next-rsc-cache-wrapper";
 export const /*#__TURBOPACK_DISABLE_EXPORT_MERGING__*/ $$RSC_SERVER_ACTION_0 = async function e() {
     // this is not allowed here
     this.foo();
     // arguments is not allowed here
     console.log(arguments);
 };
-export var $$RSC_SERVER_CACHE_1 = $$cache__("default", "80951c375b4a6a6e89d67b743ec5808127cfde405d", 0, async function a() {
+async function a() {
     // this is not allowed here
     this.foo();
     // arguments is not allowed here
@@ -31,25 +30,15 @@ export var $$RSC_SERVER_CACHE_1 = $$cache__("default", "80951c375b4a6a6e89d67b74
         };
         const e = registerServerReference($$RSC_SERVER_ACTION_0, "006a88810ecce4a4e8b59d53b8327d7e98bbf251d7", null);
     }
-});
-Object.defineProperty($$RSC_SERVER_CACHE_1, "name", {
-    "value": "a",
-    "writable": false
-});
-var a = registerServerReference($$RSC_SERVER_CACHE_1, "80951c375b4a6a6e89d67b743ec5808127cfde405d", null);
-export var $$RSC_SERVER_CACHE_2 = $$cache__("default", "8069348c79fce073bae2f70f139565a2fda1c74c74", 0, /*#__TURBOPACK_DISABLE_EXPORT_MERGING__*/ async function fetch1() {
-    // this is not allowed here
-    this.result = await fetch('https://example.com').then((res)=>res.json());
-    // arguments is not allowed here
-    console.log(arguments);
-});
-Object.defineProperty($$RSC_SERVER_CACHE_2, "name", {
-    "value": "fetch",
-    "writable": false
-});
+}
 export const api = {
     result: null,
     product: {
-        fetch: registerServerReference($$RSC_SERVER_CACHE_2, "8069348c79fce073bae2f70f139565a2fda1c74c74", null)
+        async fetch () {
+            // this is not allowed here
+            this.result = await fetch('https://example.com').then((res)=>res.json());
+            // arguments is not allowed here
+            console.log(arguments);
+        }
     }
 };