From e83f0378c822b64ebe88d5d9e434948ac11cbf17 Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Mon, 17 Nov 2025 18:48:12 -0500 Subject: [PATCH] Turbopack: perf: Fix unused argument filtering optimization --- Cargo.lock | 1 + .../tests/trace_transient.rs | 1 + .../crates/turbo-tasks-macros/Cargo.toml | 3 + .../crates/turbo-tasks-macros/src/func.rs | 16 ++- .../turbo-tasks-macros/src/function_macro.rs | 5 +- .../turbo-tasks-macros/src/self_filter.rs | 35 +++++ .../src/value_impl_macro.rs | 123 +++++++++--------- .../src/value_trait_macro.rs | 6 +- 8 files changed, 119 insertions(+), 71 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3dca9d2466246..db1dc39001228 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9383,6 +9383,7 @@ dependencies = [ "proc-macro2", "quote", "regex", + "rstest", "rustc-hash 2.1.1", "syn 2.0.104", ] diff --git a/turbopack/crates/turbo-tasks-backend/tests/trace_transient.rs b/turbopack/crates/turbo-tasks-backend/tests/trace_transient.rs index 7d0e9ff2ca95e..d863dd79e8e18 100644 --- a/turbopack/crates/turbo-tasks-backend/tests/trace_transient.rs +++ b/turbopack/crates/turbo-tasks-backend/tests/trace_transient.rs @@ -50,6 +50,7 @@ impl Adder { #[turbo_tasks::function] async fn add_method(&self, arg1: ResolvedVc, arg2: ResolvedVc) -> Result> { + let _ = self; // Make sure unused argument filtering doesn't remove the arg Ok(Vc::cell(u64::from(*arg1.await?) + u64::from(*arg2.await?))) } } diff --git a/turbopack/crates/turbo-tasks-macros/Cargo.toml b/turbopack/crates/turbo-tasks-macros/Cargo.toml index e49b490f07f02..3911f77ddc960 100644 --- a/turbopack/crates/turbo-tasks-macros/Cargo.toml +++ b/turbopack/crates/turbo-tasks-macros/Cargo.toml @@ -20,3 +20,6 @@ quote = { workspace = true } regex = { workspace = true } rustc-hash = { workspace = true } syn = { workspace = true, features = ["full", "extra-traits", "visit", "visit-mut"] } + +[dev-dependencies] +rstest = { workspace = true } diff --git a/turbopack/crates/turbo-tasks-macros/src/func.rs b/turbopack/crates/turbo-tasks-macros/src/func.rs index 8bff9d3b19277..054f13e133651 100644 --- a/turbopack/crates/turbo-tasks-macros/src/func.rs +++ b/turbopack/crates/turbo-tasks-macros/src/func.rs @@ -27,6 +27,7 @@ pub struct TurboFn<'a> { output: Type, this: Option, + is_self_used: bool, exposed_inputs: Vec, /// Should we return `OperationVc` and require that all arguments are `NonLocalValue`s? operation: bool, @@ -43,6 +44,7 @@ impl TurboFn<'_> { orig_signature: &Signature, definition_context: DefinitionContext, args: FunctionArguments, + is_self_used: bool, ) -> Option> { if !orig_signature.generics.params.is_empty() { orig_signature @@ -202,6 +204,7 @@ impl TurboFn<'_> { ident: orig_ident, output, this, + is_self_used, exposed_inputs, operation: args.operation.is_some(), inline_ident, @@ -271,7 +274,6 @@ impl TurboFn<'_> { pub fn inline_signature_and_block<'a>( &self, orig_block: &'a Block, - is_self_used: bool, ) -> (Signature, Cow<'a, Block>) { let mut shadow_self = None; let (inputs, transform_stmts): (Punctuated<_, _>, Vec>) = self @@ -280,7 +282,7 @@ impl TurboFn<'_> { .iter() .filter(|arg| { let FnArg::Typed(pat_type) = arg else { - return is_self_used; + return self.is_self_used; }; let Pat::Ident(pat_id) = &*pat_type.pat else { return true; @@ -543,8 +545,7 @@ impl TurboFn<'_> { } } - /// The block of the exposed function for a dynamic dispatch call to the - /// given trait. + /// The block of the exposed function for a dynamic dispatch call to the given trait. pub fn dynamic_block(&self, trait_type_ident: &Ident) -> Block { let Some(converted_this) = self.converted_this() else { return parse_quote! { @@ -579,13 +580,14 @@ impl TurboFn<'_> { } } - /// The block of the exposed function for a static dispatch call to the - /// given native function. + /// The block of the exposed function for a static dispatch call to the given native function. pub fn static_block(&self, native_function_ident: &Ident) -> Block { let output = &self.output; let inputs = self.inline_input_idents(); let assertions = self.get_assertions(); - let mut block = if let Some(converted_this) = self.converted_this() { + let mut block = if self.is_self_used + && let Some(converted_this) = self.converted_this() + { let persistence = self.persistence_with_this(); parse_quote! { { diff --git a/turbopack/crates/turbo-tasks-macros/src/function_macro.rs b/turbopack/crates/turbo-tasks-macros/src/function_macro.rs index b01529f020016..7549c05011592 100644 --- a/turbopack/crates/turbo-tasks-macros/src/function_macro.rs +++ b/turbopack/crates/turbo-tasks-macros/src/function_macro.rs @@ -43,7 +43,7 @@ pub fn function(args: TokenStream, input: TokenStream) -> TokenStream { .unwrap_or_default(); let is_self_used = args.operation.is_some() || is_self_used(&block); - let Some(turbo_fn) = TurboFn::new(&sig, DefinitionContext::NakedFn, args) else { + let Some(turbo_fn) = TurboFn::new(&sig, DefinitionContext::NakedFn, args, is_self_used) else { return quote! { // An error occurred while parsing the function signature. } @@ -53,8 +53,7 @@ pub fn function(args: TokenStream, input: TokenStream) -> TokenStream { let ident = &sig.ident; let inline_function_ident = turbo_fn.inline_ident(); - let (inline_signature, inline_block) = - turbo_fn.inline_signature_and_block(&block, is_self_used); + let (inline_signature, inline_block) = turbo_fn.inline_signature_and_block(&block); let inline_attrs = filter_inline_attributes(&attrs[..]); let function_path_string = ident.to_string(); diff --git a/turbopack/crates/turbo-tasks-macros/src/self_filter.rs b/turbopack/crates/turbo-tasks-macros/src/self_filter.rs index 3ff4aa3d477ca..bbfde654e9376 100644 --- a/turbopack/crates/turbo-tasks-macros/src/self_filter.rs +++ b/turbopack/crates/turbo-tasks-macros/src/self_filter.rs @@ -79,3 +79,38 @@ fn contains_self_token(tok: &TokenTree) -> bool { TokenTree::Punct(..) | TokenTree::Literal(..) => false, } } + +#[cfg(test)] +mod tests { + use rstest::rstest; + + use super::*; + + #[rstest] + #[case::no_self_usage(r#"{ let x = 42; println!("hello"); }"#, false)] + #[case::simple_self_usage(r#"{ self.foo(); }"#, true)] + #[case::self_field_access(r#"{ let x = self.field; }"#, true)] + #[case::self_in_nested_block(r#"{ let x = 1; { self.method(); } }"#, true)] + #[case::self_in_impl_block_not_detected( + r#"{ impl Foo { fn bar(&self) { self.baz(); } } }"#, + false + )] + #[case::self_before_impl_block( + r#"{ self.foo(); impl Bar { fn baz(&self) { self.qux(); } } }"#, + true + )] + #[case::self_in_closure(r#"{ let f = || { self.method(); }; }"#, true)] + #[case::self_in_if_condition(r#"{ if self.check() { println!("true"); } }"#, true)] + #[case::self_in_match_arm(r#"{ match x { Some(_) => self.handle(), None => {}, } }"#, true)] + #[case::self_in_macro(r#"{ println!("{:?}", self); }"#, true)] + #[case::self_in_complex_macro(r#"{ format!("value: {}", self.field); }"#, true)] + #[case::no_self_with_similar_idents(r#"{ let myself = 42; let selfish = true; }"#, false)] + #[case::empty_block(r#"{}"#, false)] + #[case::self_in_return_statement(r#"{ return self.value; }"#, true)] + #[case::self_as_function_argument(r#"{ some_function(self); }"#, true)] + fn test_is_self_used(#[case] code: &str, #[case] expected: bool) { + let block: syn::Block = syn::parse_str(code).unwrap(); + let result = is_self_used(&block); + assert_eq!(result, expected); + } +} diff --git a/turbopack/crates/turbo-tasks-macros/src/value_impl_macro.rs b/turbopack/crates/turbo-tasks-macros/src/value_impl_macro.rs index 5308431bcb40b..50046a9a7b64d 100644 --- a/turbopack/crates/turbo-tasks-macros/src/value_impl_macro.rs +++ b/turbopack/crates/turbo-tasks-macros/src/value_impl_macro.rs @@ -73,66 +73,71 @@ pub fn value_impl(args: TokenStream, input: TokenStream) -> TokenStream { let mut errors = Vec::new(); for item in items.iter() { - if let ImplItem::Fn(ImplItemFn { + let ImplItem::Fn(ImplItemFn { attrs, vis, defaultness: _, sig, block, }) = item - { - let ident = &sig.ident; - let (func_args, attrs) = split_function_attributes(attrs); - let func_args = match func_args { - Ok(None) => { - item.span() - .unwrap() - .error("#[turbo_tasks::function] attribute missing") - .emit(); - FunctionArguments::default() - } - Ok(Some(func_args)) => func_args, - Err(error) => { - errors.push(error.to_compile_error()); - FunctionArguments::default() - } - }; - let is_self_used = func_args.operation.is_some() || is_self_used(block); - - let Some(turbo_fn) = - TurboFn::new(sig, DefinitionContext::ValueInherentImpl, func_args) - else { - return quote! { - // An error occurred while parsing the function signature. - }; - }; + else { + continue; + }; + + let ident = &sig.ident; + let (func_args, attrs) = split_function_attributes(attrs); + let func_args = match func_args { + Ok(None) => { + item.span() + .unwrap() + .error("#[turbo_tasks::function] attribute missing") + .emit(); + FunctionArguments::default() + } + Ok(Some(func_args)) => func_args, + Err(error) => { + errors.push(error.to_compile_error()); + FunctionArguments::default() + } + }; + let is_self_used = func_args.operation.is_some() || is_self_used(block); - let inline_function_ident = turbo_fn.inline_ident(); - let (inline_signature, inline_block) = - turbo_fn.inline_signature_and_block(block, is_self_used); - let inline_attrs = filter_inline_attributes(attrs.iter().copied()); - let function_path_string = format!("{ty}::{ident}", ty = ty.to_token_stream()); - let native_fn = NativeFn { - function_global_name: global_name(&function_path_string), - function_path_string, - function_path: parse_quote! { <#ty>::#inline_function_ident }, - is_method: turbo_fn.is_method(), - is_self_used, - filter_trait_call_args: None, // not a trait method + let Some(turbo_fn) = TurboFn::new( + sig, + DefinitionContext::ValueInherentImpl, + func_args, + is_self_used, + ) else { + return quote! { + // An error occurred while parsing the function signature. }; - - let native_function_ident = get_inherent_impl_function_ident(ty_ident, ident); - let native_function_ty = native_fn.ty(); - let native_function_def = native_fn.definition(); - - let turbo_signature = turbo_fn.signature(); - let turbo_block = turbo_fn.static_block(&native_function_ident); - exposed_impl_items.push(quote! { - #(#attrs)* - #vis #turbo_signature #turbo_block - }); - - all_definitions.push(quote! { + }; + + let inline_function_ident = turbo_fn.inline_ident(); + let (inline_signature, inline_block) = turbo_fn.inline_signature_and_block(block); + let inline_attrs = filter_inline_attributes(attrs.iter().copied()); + let function_path_string = format!("{ty}::{ident}", ty = ty.to_token_stream()); + let native_fn = NativeFn { + function_global_name: global_name(&function_path_string), + function_path_string, + function_path: parse_quote! { <#ty>::#inline_function_ident }, + is_method: turbo_fn.is_method(), + is_self_used, + filter_trait_call_args: None, // not a trait method + }; + + let native_function_ident = get_inherent_impl_function_ident(ty_ident, ident); + let native_function_ty = native_fn.ty(); + let native_function_def = native_fn.definition(); + + let turbo_signature = turbo_fn.signature(); + let turbo_block = turbo_fn.static_block(&native_function_ident); + exposed_impl_items.push(quote! { + #(#attrs)* + #vis #turbo_signature #turbo_block + }); + + all_definitions.push(quote! { #[doc(hidden)] impl #ty { // By declaring the native function's body within an `impl` block, we ensure @@ -153,7 +158,6 @@ pub fn value_impl(args: TokenStream, input: TokenStream) -> TokenStream { turbo_tasks::macro_helpers::CollectableFunction(&#native_function_ident) } }) - } } quote! { @@ -207,11 +211,15 @@ pub fn value_impl(args: TokenStream, input: TokenStream) -> TokenStream { continue; } }; + // operations are not currently compatible with methods let is_self_used = func_args.operation.is_some() || is_self_used(block); - let Some(turbo_fn) = - TurboFn::new(sig, DefinitionContext::ValueTraitImpl, func_args) - else { + let Some(turbo_fn) = TurboFn::new( + sig, + DefinitionContext::ValueTraitImpl, + func_args, + is_self_used, + ) else { return quote! { // An error occurred while parsing the function signature. }; @@ -222,8 +230,7 @@ pub fn value_impl(args: TokenStream, input: TokenStream) -> TokenStream { &format!("{ty_ident}_{trait_ident}_{ident}_inline"), ident.span(), ); - let (inline_signature, inline_block) = - turbo_fn.inline_signature_and_block(block, is_self_used); + let (inline_signature, inline_block) = turbo_fn.inline_signature_and_block(block); let inline_attrs = filter_inline_attributes(attrs.iter().copied()); let native_fn = NativeFn { // This global name breaks the pattern. It isn't clear if it is intentional diff --git a/turbopack/crates/turbo-tasks-macros/src/value_trait_macro.rs b/turbopack/crates/turbo-tasks-macros/src/value_trait_macro.rs index c3c64e332962e..5748d6a643752 100644 --- a/turbopack/crates/turbo-tasks-macros/src/value_trait_macro.rs +++ b/turbopack/crates/turbo-tasks-macros/src/value_trait_macro.rs @@ -176,10 +176,12 @@ pub fn value_trait(args: TokenStream, input: TokenStream) -> TokenStream { .emit(); } + let is_self_used = default.as_ref().map(is_self_used).unwrap_or(false); let Some(turbo_fn) = TurboFn::new( sig, DefinitionContext::ValueTrait, FunctionArguments::default(), + is_self_used, ) else { return quote! { // An error occurred while parsing the function signature. @@ -194,12 +196,10 @@ pub fn value_trait(args: TokenStream, input: TokenStream) -> TokenStream { }); let default = if let Some(default) = default { - let is_self_used = is_self_used(default); let inline_function_ident = turbo_fn.inline_ident(); let inline_extension_trait_ident = Ident::new(&format!("{trait_ident}_{ident}_inline"), ident.span()); - let (inline_signature, inline_block) = - turbo_fn.inline_signature_and_block(default, is_self_used); + let (inline_signature, inline_block) = turbo_fn.inline_signature_and_block(default); let inline_attrs = filter_inline_attributes(attrs.iter().copied()); let function_path_string = format!("{trait_ident}::{ident}");