From 241806a2a1c5f0b10623b1e48aeddff576182439 Mon Sep 17 00:00:00 2001 From: Hendrik Liebau Date: Mon, 25 Nov 2024 21:23:04 +0100 Subject: [PATCH] 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. --- .../src/transforms/server_actions.rs | 124 +++++++++++------- .../server-actions/server-graph/17/output.js | 12 +- .../server-actions/server-graph/20/output.js | 12 +- .../server-actions/server-graph/22/output.js | 29 ++-- .../server-actions/server-graph/24/output.js | 14 +- .../server-actions/server-graph/26/output.js | 20 +-- .../errors/use-cache-not-allowed/2/output.js | 12 +- 7 files changed, 100 insertions(+), 123 deletions(-) diff --git a/crates/next-custom-transforms/src/transforms/server_actions.rs b/crates/next-custom-transforms/src/transforms/server_actions.rs index 634650c093ed19..7dc86779c44b45 100644 --- 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(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 { 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 { } impl ServerActions { + 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 ServerActions { // 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 ServerActions { }); 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 ServerActions { } fn get_directive_for_module(&mut self, stmts: &mut Vec) -> Option { - 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 VisitMut for ServerActions { } 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 VisitMut for ServerActions { 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 VisitMut for ServerActions { 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 VisitMut for ServerActions { 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 VisitMut for ServerActions { } 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 VisitMut for ServerActions { 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 VisitMut for ServerActions { 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, has_file_directive: bool, is_allowed_position: bool, + did_emit_error: bool, } impl DirectiveVisitor<'_> { + fn new( + config: &Config, + location: DirectiveLocation, + directive: Option, + 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,7 +2635,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(), @@ -2617,7 +2643,7 @@ impl DirectiveVisitor<'_> { } } 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,12 +2717,12 @@ 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(), @@ -2704,12 +2730,12 @@ impl DirectiveVisitor<'_> { } } 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 index 643356d06296ae..59eeb6f9f14d56 100644 --- 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 index ae58732828e817..4a35b2cae0a8ba 100644 --- 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 index 881e15a7e6333f..d8140d9d7f7270 100644 --- 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); + } } }; diff --git a/crates/next-custom-transforms/tests/errors/server-actions/server-graph/24/output.js b/crates/next-custom-transforms/tests/errors/server-actions/server-graph/24/output.js index b98e7e1aef798a..64e28ef1bc81a1 100644 --- a/crates/next-custom-transforms/tests/errors/server-actions/server-graph/24/output.js +++ b/crates/next-custom-transforms/tests/errors/server-actions/server-graph/24/output.js @@ -1,13 +1,13 @@ -/* __next_internal_action_entry_do_not_use__ {"006a88810ecce4a4e8b59d53b8327d7e98bbf251d7":"$$RSC_SERVER_ACTION_0","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, /*#__TURBOPACK_DISABLE_EXPORT_MERGING__*/ async function a() { +// exported! +export async function a() { // this is not allowed here this.foo(); // arguments is not allowed here @@ -31,10 +31,4 @@ 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 -}); -// exported! -export var a = registerServerReference($$RSC_SERVER_CACHE_1, "80951c375b4a6a6e89d67b743ec5808127cfde405d", null); +} diff --git a/crates/next-custom-transforms/tests/errors/server-actions/server-graph/26/output.js b/crates/next-custom-transforms/tests/errors/server-actions/server-graph/26/output.js index bc52378465ff18..6d2337e0228bf9 100644 --- a/crates/next-custom-transforms/tests/errors/server-actions/server-graph/26/output.js +++ b/crates/next-custom-transforms/tests/errors/server-actions/server-graph/26/output.js @@ -1,19 +1,11 @@ -/* __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 bar() { - // arguments is not allowed here - console.log(arguments); - // this is not allowed here - return this.foo(); -}); -Object.defineProperty($$RSC_SERVER_CACHE_0, "name", { - "value": "bar", - "writable": false -}); export class MyClass { static async foo() { return fetch('https://example.com').then((res)=>res.json()); } - static bar = registerServerReference($$RSC_SERVER_CACHE_0, "803128060c414d59f8552e4788b846c0d2b7f74743", null); + static async bar() { + // arguments is not allowed here + console.log(arguments); + // this is not allowed here + return this.foo(); + } } diff --git a/crates/next-custom-transforms/tests/errors/use-cache-not-allowed/2/output.js b/crates/next-custom-transforms/tests/errors/use-cache-not-allowed/2/output.js index 182a84a215ce4c..4e0131967b37ed 100644 --- a/crates/next-custom-transforms/tests/errors/use-cache-not-allowed/2/output.js +++ b/crates/next-custom-transforms/tests/errors/use-cache-not-allowed/2/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 Page() { +export default async function Page() { return

hello world

; -}); -Object.defineProperty($$RSC_SERVER_CACHE_0, "name", { - "value": "Page", - "writable": false -}); -export default registerServerReference($$RSC_SERVER_CACHE_0, "803128060c414d59f8552e4788b846c0d2b7f74743", null); +}