From dc1b05ec05c5af1ac186b4c214f56ad952bfba25 Mon Sep 17 00:00:00 2001 From: Alex Kirszenberg Date: Wed, 16 Aug 2023 10:04:00 +0200 Subject: [PATCH] Misc comments in turbo_tasks (vercel/turbo#5723) ### Description Adds some comments to explain some of the implementation details of the `Vc` PR. Closes WEB-1380 --- .../src/task/concrete_task_input.rs | 6 ++++ crates/turbo-tasks/src/task/function.rs | 32 +++++++++++++++++-- crates/turbo-tasks/src/task/task_input.rs | 5 +++ crates/turbo-tasks/src/task/task_output.rs | 2 ++ crates/turbo-tasks/src/unit.rs | 12 ++++++- crates/turbo-tasks/src/vc/default.rs | 11 +++++++ crates/turbo-tasks/src/vc/mod.rs | 5 +-- 7 files changed, 68 insertions(+), 5 deletions(-) diff --git a/crates/turbo-tasks/src/task/concrete_task_input.rs b/crates/turbo-tasks/src/task/concrete_task_input.rs index 839e8e54b750d..24a10268f0c06 100644 --- a/crates/turbo-tasks/src/task/concrete_task_input.rs +++ b/crates/turbo-tasks/src/task/concrete_task_input.rs @@ -318,6 +318,12 @@ impl<'de> Deserialize<'de> for SharedValue { } } +/// Intermediate representation of task inputs. +/// +/// When a task is called, all its arguments will be converted and stored as +/// [`ConcreteTaskInput`]s. When the task is actually run, these inputs will be +/// converted back into the argument types. This is handled by the [`TaskInput`] +/// trait. #[allow(clippy::derived_hash_with_manual_eq)] #[derive(Debug, Hash, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] pub enum ConcreteTaskInput { diff --git a/crates/turbo-tasks/src/task/function.rs b/crates/turbo-tasks/src/task/function.rs index b25fa6f59c06b..422e56f1618b9 100644 --- a/crates/turbo-tasks/src/task/function.rs +++ b/crates/turbo-tasks/src/task/function.rs @@ -1,3 +1,26 @@ +//! # Function tasks +//! +//! This module contains the trait definitions and implementations that are +//! necessary for accepting functions as tasks when using the +//! `turbo_tasks::function` macro. +//! +//! This system is inspired by Bevy's Systems and Axum's Handlers. +//! +//! The original principle is somewhat simple: a function is accepted if all +//! of its arguments implement `TaskInput` and its return type implements +//! `TaskOutput`. There are a few hoops one needs to jump through to make this +//! work, but they are described in this blog post: +//! https://blog.logrocket.com/rust-bevy-entity-component-system/ +//! +//! However, there are is an additional complication in our case: async methods +//! that accept a reference to the receiver as their first argument. +//! +//! This complication handled through our own version of the `async_trait` +//! crate, which allows us to target `async fn` as trait bounds. The naive +//! approach runs into many issues with lifetimes, hence the need for an +//! intermediate trait. However, this implementation doesn't support all async +//! methods (see commented out tests). + use std::{future::Future, marker::PhantomData, pin::Pin}; use anyhow::{bail, Context, Result}; @@ -56,10 +79,15 @@ trait TaskFnInputFunction: Send + Sync + C fn functor(&self, name: &'static str, inputs: &[ConcreteTaskInput]) -> Result; } -pub trait TaskFnMode: Send + Sync + 'static {} - pub trait TaskInputs: Send + Sync + 'static {} +/// Modes to allow multiple `TaskFnInputFunction` blanket implementations on +/// `Fn`s. Even though the implementations are non-conflicting in practice, they +/// could be in theory (at least from with the compiler's current limitations). +/// Despite this, the compiler is still able to infer the correct mode from a +/// function. +pub trait TaskFnMode: Send + Sync + 'static {} + pub struct FunctionMode; impl TaskFnMode for FunctionMode {} diff --git a/crates/turbo-tasks/src/task/task_input.rs b/crates/turbo-tasks/src/task/task_input.rs index 8dce9bb4abf40..1b3f044d47b8d 100644 --- a/crates/turbo-tasks/src/task/task_input.rs +++ b/crates/turbo-tasks/src/task/task_input.rs @@ -12,6 +12,10 @@ use crate::{ TypedForInput, Value, Vc, VcValueType, }; +/// Trait to implement in order for a type to be accepted as a +/// `turbo_tasks::function` argument. +/// +/// See also [`ConcreteTaskInput`]. pub trait TaskInput: Send + Sync + Clone { fn try_from_concrete(input: &ConcreteTaskInput) -> Result; fn into_concrete(self) -> ConcreteTaskInput; @@ -306,6 +310,7 @@ macro_rules! tuple_impls { }; } +// Implement `TaskInput` for all tuples of 1 to 12 elements. tuple_impls! { A } tuple_impls! { A B } tuple_impls! { A B C } diff --git a/crates/turbo-tasks/src/task/task_output.rs b/crates/turbo-tasks/src/task/task_output.rs index ce9b9173a90bd..3b64c8a6359bf 100644 --- a/crates/turbo-tasks/src/task/task_output.rs +++ b/crates/turbo-tasks/src/task/task_output.rs @@ -4,6 +4,8 @@ use anyhow::Result; use crate::{unit, RawVc, Vc}; +/// Trait to implement in order for a type to be accepted as a +/// `turbo_tasks::function` return type. pub trait TaskOutput { type Return; diff --git a/crates/turbo-tasks/src/unit.rs b/crates/turbo-tasks/src/unit.rs index 6b1c81042f9ea..d3af9af7b84a9 100644 --- a/crates/turbo-tasks/src/unit.rs +++ b/crates/turbo-tasks/src/unit.rs @@ -1,5 +1,15 @@ -use crate::Vc; +use crate::{ValueDefault, Vc}; +// TODO(alexkirsz) Should this be `#[turbo_tasks::function]` or is it okay to +// always return a new `Vc`? pub fn unit() -> Vc<()> { Vc::cell(()) } + +impl ValueDefault for () { + // TODO(alexkirsz) Should this be `#[turbo_tasks::function]` or is it + // preferrable to always return a new `Vc`? + fn value_default() -> Vc { + Vc::cell(()) + } +} diff --git a/crates/turbo-tasks/src/vc/default.rs b/crates/turbo-tasks/src/vc/default.rs index 1e94efeccfbcb..d5a885dae81f7 100644 --- a/crates/turbo-tasks/src/vc/default.rs +++ b/crates/turbo-tasks/src/vc/default.rs @@ -2,6 +2,17 @@ use turbo_tasks::Vc; use crate::{self as turbo_tasks}; +/// `Vc` analog to the `Default` trait. +/// +/// Implementing this trait on `T` will make `Vc::default()` produce +/// `T::value_default()`. +/// +/// There are two ways to implement this trait: +/// 1. Annotating with `#[turbo_tasks::value_impl]`: this will make +/// `Vc::default()` always return the same underlying value (i.e. a +/// singleton). +/// 2. No annotations: this will make `Vc::default()` always return a different +/// value. #[turbo_tasks::value_trait] pub trait ValueDefault { fn value_default() -> Vc; diff --git a/crates/turbo-tasks/src/vc/mod.rs b/crates/turbo-tasks/src/vc/mod.rs index 85ea1af9b0125..a4421cc2b40f6 100644 --- a/crates/turbo-tasks/src/vc/mod.rs +++ b/crates/turbo-tasks/src/vc/mod.rs @@ -46,6 +46,9 @@ where pub(crate) _t: PhantomData, } +/// This only exists to satisfy the Rust type system. However, this struct can +/// never actually be instantiated, as dereferencing a `Vc` will result in a +/// linker error. See the implementation of `Deref` for `Vc`. pub struct VcDeref where T: ?Sized, @@ -53,8 +56,6 @@ where _t: PhantomData, } -trait Impossible {} - macro_rules! do_not_use_or_you_will_be_fired { ($($name:ident)*) => { impl VcDeref