From d240d4500e4d5a074bed6dfcb856a5386f691e9e Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Thu, 19 Dec 2024 18:08:54 -0800 Subject: [PATCH] refactor(turbo-tasks): Remove public OperationVc constructor, introduce a macro annotation for creating OperationVc types (#72871) Introduces a new `operation` argument to the `#[turbo_tasks::function]` macro for creating functions that return `OperationVc`: ``` // this function's external signature returns an `OperationVc` #[turbo_tasks::function(operation)] async fn multiply(value: OperationVc, coefficient: i32) -> Result> { Ok(Vc::cell(*value.connect().await? * coefficient)) } ``` This is important to solve a few major footguns with manually-constructed `OperationVc`s: - It can be hard to know if a `Vc` is an operation or not, and the only warning you'd get if you got it wrong was a runtime error. - Local-task-based resolution (#69126) will implicitly create local `Vc`s if an argument isn't resolved. - ~We want to enforce that all arguments to `OperationVc`-returning functions are also `OperationVc`s or non-`Vc` values. Otherwise you could end up with a stale/wrong `OperationVc`.~ Scrapped, this was too hard/impractical, see below. This removes the public constructor. Methods are not currently supported because: 1. Operations are uncommon, and it's not worth the effort, IMO. 2. There's no way to make it work for dynamic-dispatched method calls, as we cannot resolve the type of `self`. It could be made to work for inherent impls and non-object trait types. --- This is basically implementing @sokra's TODO comment in the old `OperationVc::new` constructor: ``` // TODO to avoid this runtime check, we should mark functions with `(operation)` and return // a OperationVc directly ``` But with assertions for the function arguments, based on some discussion in DMs: We allow *either* `ResolvedVc` or `OperationVc` as arguments because: - Only accepting `OperationVc` arguments was impossible to refactor to, as this made it "viral" and there are too many places where we need to use operations that have too many dependencies. - While it makes sense for `State` to require `OperationVc` as arguments to any operation (keeps the whole dependency/call tree alive), for collectibles, sometimes you want the arguments to be `ResolvedVc`. It's just a matter of if you want to include collectibles generated when creating the arguments or not. --- .../turbo-tasks-backend/tests/operation_vc.rs | 1 + .../fail_attribute_invalid_args.stderr | 2 +- ...ttribute_invalid_args_inherent_impl.stderr | 2 +- .../fail_operation_method_self_ref.rs | 18 +++ .../fail_operation_method_self_ref.stderr | 5 + .../fail_operation_method_self_type.rs | 18 +++ .../fail_operation_method_self_type.stderr | 14 +++ ...fail_operation_method_self_type_base_vc.rs | 18 +++ ..._operation_method_self_type_base_vc.stderr | 27 +++++ .../tests/function/fail_operation_vc_arg.rs | 13 +++ .../function/fail_operation_vc_arg.stderr | 21 ++++ .../tests/function/pass_operation.rs | 20 ++++ .../pass_operation_accepts_resolved.rs | 21 ++++ .../turbo-tasks-macros/src/assert_fields.rs | 1 + .../crates/turbo-tasks-macros/src/func.rs | 110 ++++++++++++++++-- .../turbo-tasks-memory/tests/operation_vc.rs | 1 + .../turbo-tasks-testing/tests/operation_vc.rs | 36 ++++++ .../crates/turbo-tasks/src/macro_helpers.rs | 2 + .../crates/turbo-tasks/src/vc/operation.rs | 19 ++- 19 files changed, 325 insertions(+), 24 deletions(-) create mode 120000 turbopack/crates/turbo-tasks-backend/tests/operation_vc.rs create mode 100644 turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_ref.rs create mode 100644 turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_ref.stderr create mode 100644 turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type.rs create mode 100644 turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type.stderr create mode 100644 turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type_base_vc.rs create mode 100644 turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type_base_vc.stderr create mode 100644 turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_vc_arg.rs create mode 100644 turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_vc_arg.stderr create mode 100644 turbopack/crates/turbo-tasks-macros-tests/tests/function/pass_operation.rs create mode 100644 turbopack/crates/turbo-tasks-macros-tests/tests/function/pass_operation_accepts_resolved.rs create mode 120000 turbopack/crates/turbo-tasks-memory/tests/operation_vc.rs create mode 100644 turbopack/crates/turbo-tasks-testing/tests/operation_vc.rs diff --git a/turbopack/crates/turbo-tasks-backend/tests/operation_vc.rs b/turbopack/crates/turbo-tasks-backend/tests/operation_vc.rs new file mode 120000 index 0000000000000..0fe3b98a6abc6 --- /dev/null +++ b/turbopack/crates/turbo-tasks-backend/tests/operation_vc.rs @@ -0,0 +1 @@ +../../turbo-tasks-testing/tests/operation_vc.rs \ No newline at end of file diff --git a/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_attribute_invalid_args.stderr b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_attribute_invalid_args.stderr index 6b044dc6c8c9f..611cb7bafe374 100644 --- a/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_attribute_invalid_args.stderr +++ b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_attribute_invalid_args.stderr @@ -1,4 +1,4 @@ -error: unexpected token, expected one of: "fs", "network", "non_local_return", "local_cells" +error: unexpected token, expected one of: "fs", "network", "non_local_return", "operation", or "local_cells" --> tests/function/fail_attribute_invalid_args.rs:9:25 | 9 | #[turbo_tasks::function(invalid_argument)] diff --git a/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_attribute_invalid_args_inherent_impl.stderr b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_attribute_invalid_args_inherent_impl.stderr index 781f1e8c86d08..30500cc07d4da 100644 --- a/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_attribute_invalid_args_inherent_impl.stderr +++ b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_attribute_invalid_args_inherent_impl.stderr @@ -1,4 +1,4 @@ -error: unexpected token, expected one of: "fs", "network", "non_local_return", "local_cells" +error: unexpected token, expected one of: "fs", "network", "non_local_return", "operation", or "local_cells" --> tests/function/fail_attribute_invalid_args_inherent_impl.rs:14:29 | 14 | #[turbo_tasks::function(invalid_argument)] diff --git a/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_ref.rs b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_ref.rs new file mode 100644 index 0000000000000..0468a282a641a --- /dev/null +++ b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_ref.rs @@ -0,0 +1,18 @@ +#![allow(dead_code)] +#![feature(arbitrary_self_types)] +#![feature(arbitrary_self_types_pointers)] + +use turbo_tasks::Vc; + +#[turbo_tasks::value] +struct Foobar; + +#[turbo_tasks::value_impl] +impl Foobar { + #[turbo_tasks::function(operation)] + fn self_ref(&self) -> Vc<()> { + Vc::cell(()) + } +} + +fn main() {} diff --git a/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_ref.stderr b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_ref.stderr new file mode 100644 index 0000000000000..9fb7a9cf20145 --- /dev/null +++ b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_ref.stderr @@ -0,0 +1,5 @@ +error: methods taking `self` are not supported with `operation` + --> tests/function/fail_operation_method_self_ref.rs:13:17 + | +13 | fn self_ref(&self) -> Vc<()> { + | ^^^^^ diff --git a/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type.rs b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type.rs new file mode 100644 index 0000000000000..b1ca57b83d0f4 --- /dev/null +++ b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type.rs @@ -0,0 +1,18 @@ +#![allow(dead_code)] +#![feature(arbitrary_self_types)] +#![feature(arbitrary_self_types_pointers)] + +use turbo_tasks::{OperationVc, Vc}; + +#[turbo_tasks::value] +struct Foobar; + +#[turbo_tasks::value_impl] +impl Foobar { + #[turbo_tasks::function(operation)] + fn arbitrary_self_type(self: OperationVc) -> Vc<()> { + Vc::cell(()) + } +} + +fn main() {} diff --git a/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type.stderr b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type.stderr new file mode 100644 index 0000000000000..8fbffa3a48f54 --- /dev/null +++ b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type.stderr @@ -0,0 +1,14 @@ +error: methods taking `self` are not supported with `operation` + --> tests/function/fail_operation_method_self_type.rs:13:28 + | +13 | fn arbitrary_self_type(self: OperationVc) -> Vc<()> { + | ^^^^^^^^^^^^^^^^^^^^^^^ + +error[E0307]: invalid `self` parameter type: `OperationVc` + --> tests/function/fail_operation_method_self_type.rs:13:34 + | +13 | fn arbitrary_self_type(self: OperationVc) -> Vc<()> { + | ^^^^^^^^^^^^^^^^^ + | + = note: type of `self` must be `Self` or a type that dereferences to it + = help: consider changing to `self`, `&self`, `&mut self`, `self: Box`, `self: Rc`, `self: Arc`, or `self: Pin

` (where P is one of the previous types except `Self`) diff --git a/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type_base_vc.rs b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type_base_vc.rs new file mode 100644 index 0000000000000..aff8c2ec0522c --- /dev/null +++ b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type_base_vc.rs @@ -0,0 +1,18 @@ +#![allow(dead_code)] +#![feature(arbitrary_self_types)] +#![feature(arbitrary_self_types_pointers)] + +use turbo_tasks::Vc; + +#[turbo_tasks::value] +struct Foobar; + +#[turbo_tasks::value_impl] +impl Foobar { + #[turbo_tasks::function(operation)] + fn arbitrary_self_type_base_vc(self: Vc) -> Vc<()> { + Vc::cell(()) + } +} + +fn main() {} diff --git a/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type_base_vc.stderr b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type_base_vc.stderr new file mode 100644 index 0000000000000..ff17cba4aaca0 --- /dev/null +++ b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_method_self_type_base_vc.stderr @@ -0,0 +1,27 @@ +error: methods taking `self` are not supported with `operation` + --> tests/function/fail_operation_method_self_type_base_vc.rs:13:36 + | +13 | fn arbitrary_self_type_base_vc(self: Vc) -> Vc<()> { + | ^^^^^^^^^^^^^^ + +error[E0277]: the trait bound `Vc: NonLocalValue` is not satisfied + --> tests/function/fail_operation_method_self_type_base_vc.rs:13:42 + | +13 | fn arbitrary_self_type_base_vc(self: Vc) -> Vc<()> { + | ^^^^^^^^ the trait `NonLocalValue` is not implemented for `Vc` + | + = help: the following other types implement trait `NonLocalValue`: + &T + &mut T + () + (A, Z, Y, X, W, V, U, T) + (B, A, Z, Y, X, W, V, U, T) + (C, B, A, Z, Y, X, W, V, U, T) + (D, C, B, A, Z, Y, X, W, V, U, T) + (E, D, C, B, A, Z, Y, X, W, V, U, T) + and $N others +note: required by a bound in `assert_argument_is_non_local_value` + --> $WORKSPACE/turbopack/crates/turbo-tasks/src/macro_helpers.rs + | + | pub fn assert_argument_is_non_local_value() {} + | ^^^^^^^^^^^^^ required by this bound in `assert_argument_is_non_local_value` diff --git a/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_vc_arg.rs b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_vc_arg.rs new file mode 100644 index 0000000000000..e33d8a9aa3405 --- /dev/null +++ b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_vc_arg.rs @@ -0,0 +1,13 @@ +#![allow(dead_code)] + +use anyhow::Result; +use turbo_tasks::{ResolvedVc, Vc}; + +#[turbo_tasks::function(operation)] +async fn multiply(value: Vc, coefficient: ResolvedVc) -> Result> { + let value = *value.await?; + let coefficient = *coefficient.await?; + Ok(Vc::cell(value * coefficient)) +} + +fn main() {} diff --git a/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_vc_arg.stderr b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_vc_arg.stderr new file mode 100644 index 0000000000000..401362a874dd8 --- /dev/null +++ b/turbopack/crates/turbo-tasks-macros-tests/tests/function/fail_operation_vc_arg.stderr @@ -0,0 +1,21 @@ +error[E0277]: the trait bound `Vc: NonLocalValue` is not satisfied + --> tests/function/fail_operation_vc_arg.rs:7:26 + | +7 | async fn multiply(value: Vc, coefficient: ResolvedVc) -> Result> { + | ^^^^^^^ the trait `NonLocalValue` is not implemented for `Vc` + | + = help: the following other types implement trait `NonLocalValue`: + &T + &mut T + () + (A, Z, Y, X, W, V, U, T) + (B, A, Z, Y, X, W, V, U, T) + (C, B, A, Z, Y, X, W, V, U, T) + (D, C, B, A, Z, Y, X, W, V, U, T) + (E, D, C, B, A, Z, Y, X, W, V, U, T) + and $N others +note: required by a bound in `assert_argument_is_non_local_value` + --> $WORKSPACE/turbopack/crates/turbo-tasks/src/macro_helpers.rs + | + | pub fn assert_argument_is_non_local_value() {} + | ^^^^^^^^^^^^^ required by this bound in `assert_argument_is_non_local_value` diff --git a/turbopack/crates/turbo-tasks-macros-tests/tests/function/pass_operation.rs b/turbopack/crates/turbo-tasks-macros-tests/tests/function/pass_operation.rs new file mode 100644 index 0000000000000..b16018d515c8d --- /dev/null +++ b/turbopack/crates/turbo-tasks-macros-tests/tests/function/pass_operation.rs @@ -0,0 +1,20 @@ +use anyhow::Result; +use turbo_tasks::{OperationVc, Vc}; + +#[turbo_tasks::function(operation)] +fn bare_op_fn() -> Vc { + Vc::cell(21) +} + +#[turbo_tasks::function(operation)] +async fn multiply(value: OperationVc, coefficient: i32) -> Result> { + Ok(Vc::cell(*value.connect().await? * coefficient)) +} + +#[allow(dead_code)] +fn use_operations() { + let twenty_one: OperationVc = bare_op_fn(); + let _fourty_two: OperationVc = multiply(twenty_one, 2); +} + +fn main() {} diff --git a/turbopack/crates/turbo-tasks-macros-tests/tests/function/pass_operation_accepts_resolved.rs b/turbopack/crates/turbo-tasks-macros-tests/tests/function/pass_operation_accepts_resolved.rs new file mode 100644 index 0000000000000..9d3a1ee298b9f --- /dev/null +++ b/turbopack/crates/turbo-tasks-macros-tests/tests/function/pass_operation_accepts_resolved.rs @@ -0,0 +1,21 @@ +use anyhow::Result; +use turbo_tasks::{OperationVc, ResolvedVc, Vc}; + +#[turbo_tasks::function(operation)] +fn bare_op_fn() -> Vc { + Vc::cell(21) +} + +// operations can take `ResolvedVc`s too (anything that's a `NonLocalValue`). +#[turbo_tasks::function(operation)] +async fn multiply(value: OperationVc, coefficient: ResolvedVc) -> Result> { + Ok(Vc::cell((*value.connect().await?) * (*coefficient.await?))) +} + +#[allow(dead_code)] +fn use_operations() { + let twenty_one: OperationVc = bare_op_fn(); + let _fourty_two: OperationVc = multiply(twenty_one, ResolvedVc::cell(2)); +} + +fn main() {} diff --git a/turbopack/crates/turbo-tasks-macros/src/assert_fields.rs b/turbopack/crates/turbo-tasks-macros/src/assert_fields.rs index 34a0c69bab29e..756861e83ca98 100644 --- a/turbopack/crates/turbo-tasks-macros/src/assert_fields.rs +++ b/turbopack/crates/turbo-tasks-macros/src/assert_fields.rs @@ -47,6 +47,7 @@ pub fn assert_fields_impl_trait( // we reproduce the field types here to ensure any generics get used struct #assertion_struct_ident #impl_generics (#(#field_types),*) #where_clause; impl #impl_generics #assertion_struct_ident #ty_generics #where_clause { + #[allow(non_snake_case)] fn #assertion_fn_ident< Expected: #trait_path + ?Sized >() {} diff --git a/turbopack/crates/turbo-tasks-macros/src/func.rs b/turbopack/crates/turbo-tasks-macros/src/func.rs index 0d1ee842a11c0..c9d684c11aa67 100644 --- a/turbopack/crates/turbo-tasks-macros/src/func.rs +++ b/turbopack/crates/turbo-tasks-macros/src/func.rs @@ -30,6 +30,8 @@ pub struct TurboFn<'a> { inputs: Vec, /// Should we check that the return type contains a `NonLocalValue`? non_local: Option, + /// Should we return `OperationVc` and require that all arguments are `NonLocalValue`s? + operation: bool, /// Should this function use `TaskPersistence::LocalCells`? local_cells: bool, } @@ -274,6 +276,7 @@ impl TurboFn<'_> { this, inputs, non_local: args.non_local_return, + operation: args.operation.is_some(), local_cells: args.local_cells.is_some(), inline_ident, }) @@ -298,14 +301,24 @@ impl TurboFn<'_> { subpat: None, })), colon_token: Default::default(), - ty: Box::new(expand_task_input_type(&input.ty).into_owned()), + ty: if self.operation { + // operations shouldn't have their arguments rewritten, they require all + // arguments are explicitly `NonLocalValue`s + Box::new(input.ty.clone()) + } else { + Box::new(expand_task_input_type(&input.ty).into_owned()) + }, }) }) .collect(); let ident = &self.ident; let orig_output = &self.output; - let new_output = expand_vc_return_type(orig_output); + let new_output = expand_vc_return_type( + orig_output, + self.operation + .then(|| parse_quote!(turbo_tasks::OperationVc)), + ); parse_quote! { fn #ident(#exposed_inputs) -> #new_output @@ -342,6 +355,11 @@ impl TurboFn<'_> { .map(|(idx, arg)| match arg { FnArg::Receiver(_) => (arg.clone(), None), FnArg::Typed(pat_type) => { + if self.operation { + // operations shouldn't have their arguments rewritten, they require all + // arguments are explicitly `NonLocalValue`s + return (arg.clone(), None); + } let Cow::Owned(expanded_ty) = expand_task_input_type(&pat_type.ty) else { // common-case: skip if no type conversion is needed return (arg.clone(), None); @@ -500,10 +518,35 @@ impl TurboFn<'_> { let return_type = &self.output; quote_spanned! { span => - { - turbo_tasks::macro_helpers::assert_returns_non_local_value::<#return_type, _>() + turbo_tasks::macro_helpers::assert_returns_non_local_value::<#return_type, _>(); + } + } else if self.operation { + let mut assertions = Vec::new(); + // theoretically we could support methods by rewriting the exposed self argument, but + // it's not worth it, given the rarity of operations. + const SELF_ERROR: &str = "methods taking `self` are not supported with `operation`"; + for arg in &self.orig_signature.inputs { + match arg { + FnArg::Receiver(receiver) => { + receiver.span().unwrap().error(SELF_ERROR).emit(); + } + FnArg::Typed(pat_type) => { + if let Pat::Ident(ident) = &*pat_type.pat { + // needed for syn 1.x where arbitrary self types use FnArg::Typed, this + // is fixed in syn 2.x, where `self` is always `FnArg::Receiver`. + if ident.ident == "self" { + pat_type.span().unwrap().error(SELF_ERROR).emit(); + } + } + let ty = &pat_type.ty; + assertions.push(quote_spanned! { + ty.span() => + turbo_tasks::macro_helpers::assert_argument_is_non_local_value::<#ty>(); + }); + } } } + quote! { #(#assertions)* } } else { quote! {} } @@ -550,7 +593,7 @@ impl TurboFn<'_> { let output = &self.output; let inputs = self.input_idents(); let assertions = self.get_assertions(); - if let Some(converted_this) = self.converted_this() { + let mut block = if let Some(converted_this) = self.converted_this() { let persistence = self.persistence_with_this(); parse_quote! { { @@ -584,7 +627,21 @@ impl TurboFn<'_> { ) } } + }; + if self.operation { + block = parse_quote! { + { + let vc_output = #block; + // Assumption: The turbo-tasks manager will not create a local task for a + // function where all task inputs are "resolved" (where "resolved" in this case + // includes `OperationVc`). This is checked with a debug_assert, but not in + // release mode. + #[allow(deprecated)] + turbo_tasks::OperationVc::cell_private(vc_output) + } + }; } + block } pub(crate) fn is_method(&self) -> bool { @@ -655,6 +712,12 @@ pub struct FunctionArguments { /// /// If [`Self::local_cells`] is set, this will also be set to the same span. non_local_return: Option, + /// Should the function return an `OperationVc` instead of a `Vc`? Also ensures that all + /// arguments are `OperationValue`s. Mutually exclusive with the `non_local_return` and + /// `local_cells` flags. + /// + /// If there is an error due to this option being set, it should be reported to this span. + operation: Option, /// Changes the behavior of `Vc::cell` to create local cells that are not cached across task /// executions. Cells can be converted to their non-local versions by calling `Vc::resolve`. /// @@ -686,6 +749,9 @@ impl Parse for FunctionArguments { ("non_local_return", Meta::Path(_)) => { parsed_args.non_local_return = Some(meta.span()); } + ("operation", Meta::Path(_)) => { + parsed_args.operation = Some(meta.span()); + } ("local_cells", Meta::Path(_)) => { let span = Some(meta.span()); parsed_args.local_cells = span; @@ -695,11 +761,18 @@ impl Parse for FunctionArguments { return Err(syn::Error::new_spanned( meta, "unexpected token, expected one of: \"fs\", \"network\", \ - \"non_local_return\", \"local_cells\"", + \"non_local_return\", \"operation\", or \"local_cells\"", )) } } } + if let (Some(_), Some(span)) = (parsed_args.non_local_return, parsed_args.operation) { + return Err(syn::Error::new( + span, + "\"operation\" is mutually exclusive with \"non_local_return\" and \ + \"local_cells\" options", + )); + } Ok(parsed_args) } } @@ -817,11 +890,18 @@ fn expand_task_input_type(orig_input: &Type) -> Cow<'_, Type> { } } -fn expand_vc_return_type(orig_output: &Type) -> Type { - // HACK: Approximate the expansion that we'd otherwise get from - // `::Return`, so that the return type shown in the rustdocs - // is as simple as possible. Break out as soon as we see something we don't - // recognize. +/// Performs [external signature rewriting][mdbook]. +/// +/// The expanded return type is normally a `turbo_tasks::Vc`, but the `turbo_tasks::Vc` type can be +/// replaced with a custom type using `replace_vc`. Type parameters are preserved during the +/// replacement. This is used for operation functions. +/// +/// This is a hack! It approximates the expansion that we'd otherwise get from +/// `::Return`, so that the return type shown in the rustdocs is as simple as +/// possible. Break out as soon as we see something we don't recognize. +/// +/// [mdbook]: https://turbopack-rust-docs.vercel.sh/turbo-engine/tasks.html#external-signature-rewriting +fn expand_vc_return_type(orig_output: &Type, replace_vc: Option) -> Type { let mut new_output = orig_output.clone(); let mut found_vc = false; loop { @@ -906,6 +986,14 @@ fn expand_vc_return_type(orig_output: &Type) -> Type { Unable to process type.", ) .emit(); + } else if let Some(replace_vc) = replace_vc { + let Type::Path(mut vc_path) = new_output else { + unreachable!("Since found_vc is true, the outermost type must be a path to `Vc`") + }; + let mut new_path = replace_vc; + new_path.path.segments.last_mut().unwrap().arguments = + vc_path.path.segments.pop().unwrap().into_value().arguments; + new_output = Type::Path(new_path) } new_output diff --git a/turbopack/crates/turbo-tasks-memory/tests/operation_vc.rs b/turbopack/crates/turbo-tasks-memory/tests/operation_vc.rs new file mode 120000 index 0000000000000..0fe3b98a6abc6 --- /dev/null +++ b/turbopack/crates/turbo-tasks-memory/tests/operation_vc.rs @@ -0,0 +1 @@ +../../turbo-tasks-testing/tests/operation_vc.rs \ No newline at end of file diff --git a/turbopack/crates/turbo-tasks-testing/tests/operation_vc.rs b/turbopack/crates/turbo-tasks-testing/tests/operation_vc.rs new file mode 100644 index 0000000000000..d4cefdc009e4d --- /dev/null +++ b/turbopack/crates/turbo-tasks-testing/tests/operation_vc.rs @@ -0,0 +1,36 @@ +#![feature(arbitrary_self_types)] +#![feature(arbitrary_self_types_pointers)] +#![allow(clippy::needless_return)] // tokio macro-generated code doesn't respect this + +use anyhow::Result; +use turbo_tasks::{OperationVc, ResolvedVc, Vc}; +use turbo_tasks_testing::{register, run, Registration}; + +static REGISTRATION: Registration = register!(); + +#[turbo_tasks::function(operation)] +fn bare_op_fn() -> Vc { + Vc::cell(21) +} + +// operations can take `ResolvedVc`s too (anything that's a `NonLocalValue`). +#[turbo_tasks::function(operation)] +async fn multiply(value: OperationVc, coefficient: ResolvedVc) -> Result> { + Ok(Vc::cell((*value.connect().await?) * (*coefficient.await?))) +} + +#[turbo_tasks::function] +fn use_operations() -> Vc { + let twenty_one: OperationVc = bare_op_fn(); + let fourty_two: OperationVc = multiply(twenty_one, ResolvedVc::cell(2)); + fourty_two.connect() +} + +#[tokio::test] +async fn test_use_operations() -> Result<()> { + run(®ISTRATION, || async { + assert_eq!(*use_operations().await?, 42); + Ok(()) + }) + .await +} diff --git a/turbopack/crates/turbo-tasks/src/macro_helpers.rs b/turbopack/crates/turbo-tasks/src/macro_helpers.rs index dfaa0a00160e9..1ccccdd660919 100644 --- a/turbopack/crates/turbo-tasks/src/macro_helpers.rs +++ b/turbopack/crates/turbo-tasks/src/macro_helpers.rs @@ -52,6 +52,8 @@ where { } +pub fn assert_argument_is_non_local_value() {} + #[macro_export] macro_rules! stringify_path { ($path:path) => { diff --git a/turbopack/crates/turbo-tasks/src/vc/operation.rs b/turbopack/crates/turbo-tasks/src/vc/operation.rs index ccfb011acfeab..805a83e771a13 100644 --- a/turbopack/crates/turbo-tasks/src/vc/operation.rs +++ b/turbopack/crates/turbo-tasks/src/vc/operation.rs @@ -54,19 +54,16 @@ where } impl OperationVc { - /// Creates a new `OperationVc` from a `Vc`. + /// Called by the `#[turbo_tasks::function]` macro. /// - /// The caller must ensure that the `Vc` is not a local task and it points to a a single - /// operation. - /// - /// **This API is a placeholder and will likely be removed soon** in favor of a future API that - /// uses macros and static (compile-time) assertions in place of runtime assertions. - pub fn new(node: Vc) -> Self { - // TODO to avoid this runtime check, we should mark functions with `(operation)` and return - // a OperationVc directly - assert!( + /// The macro ensures that the `Vc` is not a local task and it points to a single operation. + #[doc(hidden)] + #[deprecated = "This is an internal function. Use #[turbo_tasks::function(operation)] instead."] + pub fn cell_private(node: Vc) -> Self { + debug_assert!( matches!(node.node, RawVc::TaskOutput(..)), - "OperationVc::new must be called on the immediate return value of a task function" + "OperationVc::cell_private must be called on the immediate return value of a task \ + function" ); Self { node } }