Skip to content

Commit

Permalink
refactor(turbo-tasks): Simplify most type bounds on Vc<T> and related…
Browse files Browse the repository at this point in the history
… types (#72823)

`Vc<T>` had a type bound that `T: Send + ?Sized`. The general recommendation is to remove all type bounds from the struct that aren't necessary:

- https://rust-lang.github.io/api-guidelines/future-proofing.html#c-struct-bounds
- rust-lang/api-guidelines#6
- rust-lang/rust-clippy#1689

The reasoning is that type bounds on structs are "infectious". Any generic function, trait, or struct referring to `Vc<T>` was required to add `T: Send` bounds.

*Sidenote: The [`implied_bounds` feature](https://rust-lang.github.io/rfcs/2089-implied-bounds.html) might mitigate some of this if ever stabilized.*

Removing the `T: Send` type bound from `Vc<T>` means that there's less places where we need to specify it.

This pattern can be seen in many parts of the stdlib. For example, `Arc<T>` doesn't require that `T: Send + Sync`, though in reality to do anything useful with it, you need `T: Send + Sync`.

## Is this safe?

Yes, bounds are checked during cell construction, and the lower-level APIs used for creating cells require `Send + Sync`, so this would be hard to mess up without explicitly unsafe code.

Also, `Vc<T>` also technically requires that `T: Sync`, but that wasn't enforced in the struct definition (only during cell construction). We did just fine without that bound on the struct.

## Why are you leaving `?Sized`?

There's an implicit `Sized` bound on type parameters in struct definitions unless explicitly specified otherwise with `?Sized`.

Right now this bound isn't used. We use a box, e.g. `Vc<Box<dyn Foo>>`, but in the future we might be able to drop that intermediate box from the type signature. I'm leaving the `?Sized` bound in place (and adding it in a few places where it was missing) in hopes of that.
  • Loading branch information
bgw authored and wyattjoh committed Nov 28, 2024
1 parent 48bf449 commit ac24457
Show file tree
Hide file tree
Showing 14 changed files with 78 additions and 84 deletions.
4 changes: 2 additions & 2 deletions turbopack/crates/turbo-tasks/src/collectibles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ use auto_hash_map::AutoSet;
use crate::{Vc, VcValueTrait};

pub trait CollectiblesSource {
fn take_collectibles<T: VcValueTrait + Send>(self) -> AutoSet<Vc<T>>;
fn peek_collectibles<T: VcValueTrait + Send>(self) -> AutoSet<Vc<T>>;
fn take_collectibles<T: VcValueTrait>(self) -> AutoSet<Vc<T>>;
fn peek_collectibles<T: VcValueTrait>(self) -> AutoSet<Vc<T>>;
}
6 changes: 3 additions & 3 deletions turbopack/crates/turbo-tasks/src/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,7 @@ impl<B: Backend + 'static> TurboTasks<B> {
/// Creates a new root task
pub fn spawn_root_task<T, F, Fut>(&self, functor: F) -> TaskId
where
T: Send,
T: ?Sized,
F: Fn() -> Fut + Send + Sync + Clone + 'static,
Fut: Future<Output = Result<Vc<T>>> + Send,
{
Expand All @@ -529,7 +529,7 @@ impl<B: Backend + 'static> TurboTasks<B> {
#[track_caller]
pub fn spawn_once_task<T, Fut>(&self, future: Fut) -> TaskId
where
T: Send,
T: ?Sized,
Fut: Future<Output = Result<Vc<T>>> + Send + 'static,
{
let id = self.backend.create_transient_task(
Expand Down Expand Up @@ -1741,7 +1741,7 @@ pub fn notify_scheduled_tasks() {
with_turbo_tasks(|tt| tt.notify_scheduled_tasks())
}

pub fn emit<T: VcValueTrait + Send>(collectible: Vc<T>) {
pub fn emit<T: VcValueTrait + ?Sized>(collectible: Vc<T>) {
with_turbo_tasks(|tt| tt.emit_collectible(T::get_trait_type_id(), collectible.node))
}

Expand Down
7 changes: 2 additions & 5 deletions turbopack/crates/turbo-tasks/src/raw_vc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ impl RawVc {
}

impl CollectiblesSource for RawVc {
fn peek_collectibles<T: VcValueTrait + Send>(self) -> AutoSet<Vc<T>> {
fn peek_collectibles<T: VcValueTrait + ?Sized>(self) -> AutoSet<Vc<T>> {
let tt = turbo_tasks();
tt.notify_scheduled_tasks();
let map = tt.read_task_collectibles(self.get_task_id(), T::get_trait_type_id());
Expand All @@ -261,7 +261,7 @@ impl CollectiblesSource for RawVc {
.collect()
}

fn take_collectibles<T: VcValueTrait + Send>(self) -> AutoSet<Vc<T>> {
fn take_collectibles<T: VcValueTrait + ?Sized>(self) -> AutoSet<Vc<T>> {
let tt = turbo_tasks();
tt.notify_scheduled_tasks();
let map = tt.read_task_collectibles(self.get_task_id(), T::get_trait_type_id());
Expand Down Expand Up @@ -430,7 +430,4 @@ impl Future for ReadRawVcFuture {
}
}

unsafe impl Send for ReadRawVcFuture {}
unsafe impl Sync for ReadRawVcFuture {}

impl Unpin for ReadRawVcFuture {}
4 changes: 2 additions & 2 deletions turbopack/crates/turbo-tasks/src/task/from_task_input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@ mod private {
/// Implements the sealed trait pattern:
/// <https://rust-lang.github.io/api-guidelines/future-proofing.html>
pub trait Sealed {}
impl<T> Sealed for ResolvedVc<T> where T: Send {}
impl<T> Sealed for ResolvedVc<T> where T: ?Sized {}
impl<T> Sealed for Vec<T> where T: FromTaskInput {}
impl<T> Sealed for Option<T> where T: FromTaskInput {}
}

impl<T> FromTaskInput for ResolvedVc<T>
where
T: Send,
T: Send + Sync + ?Sized,
{
type TaskInput = Vc<T>;
fn from_task_input(from: Vc<T>) -> ResolvedVc<T> {
Expand Down
4 changes: 2 additions & 2 deletions turbopack/crates/turbo-tasks/src/task/task_input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ where

impl<T> TaskInput for Vc<T>
where
T: Send,
T: Send + Sync + ?Sized,
{
fn is_resolved(&self) -> bool {
Vc::is_resolved(*self)
Expand All @@ -114,7 +114,7 @@ where
// `Vc`, but it is useful for structs that contain `ResolvedVc` and want to derive `TaskInput`.
impl<T> TaskInput for ResolvedVc<T>
where
T: Send,
T: Send + Sync + ?Sized,
{
fn is_resolved(&self) -> bool {
true
Expand Down
2 changes: 1 addition & 1 deletion turbopack/crates/turbo-tasks/src/task/task_output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub trait TaskOutput {

impl<T> TaskOutput for Vc<T>
where
T: ?Sized + Send,
T: ?Sized,
{
type Return = Vc<T>;

Expand Down
4 changes: 2 additions & 2 deletions turbopack/crates/turbo-tasks/src/trait_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ where

impl<T> TraitRef<T>
where
T: VcValueTrait + ?Sized + Send,
T: VcValueTrait + ?Sized,
{
/// Returns a new cell that points to a value that implements the value
/// trait `T`.
Expand Down Expand Up @@ -130,7 +130,7 @@ pub trait IntoTraitRef {

impl<T> IntoTraitRef for Vc<T>
where
T: VcValueTrait + ?Sized + Send,
T: VcValueTrait + ?Sized,
{
type ValueTrait = T;

Expand Down
65 changes: 29 additions & 36 deletions turbopack/crates/turbo-tasks/src/vc/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ mod traits;

use std::{
any::Any,
fmt::Debug,
future::{Future, IntoFuture},
hash::{Hash, Hasher},
marker::PhantomData,
Expand Down Expand Up @@ -50,7 +51,7 @@ use crate::{
#[serde(transparent, bound = "")]
pub struct Vc<T>
where
T: ?Sized + Send,
T: ?Sized,
{
pub(crate) node: RawVc,
#[doc(hidden)]
Expand Down Expand Up @@ -186,7 +187,7 @@ where
#[doc(hidden)]
impl<T> Deref for Vc<T>
where
T: ?Sized + Send,
T: ?Sized,
{
type Target = VcDeref<T>;

Expand All @@ -200,14 +201,11 @@ where
}
}

impl<T> Copy for Vc<T> where T: ?Sized + Send {}

unsafe impl<T> Send for Vc<T> where T: ?Sized + Send {}
unsafe impl<T> Sync for Vc<T> where T: ?Sized + Send {}
impl<T> Copy for Vc<T> where T: ?Sized {}

impl<T> Clone for Vc<T>
where
T: ?Sized + Send,
T: ?Sized,
{
fn clone(&self) -> Self {
*self
Expand All @@ -216,7 +214,7 @@ where

impl<T> Hash for Vc<T>
where
T: ?Sized + Send,
T: ?Sized,
{
fn hash<H: Hasher>(&self, state: &mut H) {
self.node.hash(state);
Expand All @@ -225,20 +223,23 @@ where

impl<T> PartialEq<Vc<T>> for Vc<T>
where
T: ?Sized + Send,
T: ?Sized,
{
fn eq(&self, other: &Self) -> bool {
self.node == other.node
}
}

impl<T> Eq for Vc<T> where T: ?Sized + Send {}
impl<T> Eq for Vc<T> where T: ?Sized {}

// TODO(alexkirsz) This should not be implemented for Vc. Instead, users should
// use the `ValueDebug` implementation to get a `D: Debug`.
impl<T> std::fmt::Debug for Vc<T>
/// Generates an opaque debug representation of the [`Vc`] itself, but not the data inside of it.
///
/// This is implemented to allow types containing [`Vc`] to implement the synchronous [`Debug`]
/// trait, but in most cases users should use the [`ValueDebug`] implementation to get a string
/// representation of the contents of the cell.
impl<T> Debug for Vc<T>
where
T: Send,
T: ?Sized,
{
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("Vc").field("node", &self.node).finish()
Expand Down Expand Up @@ -309,7 +310,7 @@ where

impl<T> Vc<T>
where
T: ?Sized + Send,
T: ?Sized,
{
/// Connects the operation pointed to by this `Vc` to the current task.
pub fn connect(vc: Self) {
Expand Down Expand Up @@ -340,19 +341,14 @@ where
pub fn upcast<K>(vc: Self) -> Vc<K>
where
T: Upcast<K>,
K: VcValueTrait + ?Sized + Send,
K: VcValueTrait + ?Sized,
{
Vc {
node: vc.node,
_t: PhantomData,
}
}
}

impl<T> Vc<T>
where
T: ?Sized + Send,
{
/// Resolve the reference until it points to a cell directly.
///
/// Resolving will wait for task execution to be finished, so that the
Expand Down Expand Up @@ -415,7 +411,7 @@ where

impl<T> Vc<T>
where
T: VcValueTrait + ?Sized + Send,
T: VcValueTrait + ?Sized,
{
/// Attempts to sidecast the given `Vc<Box<dyn T>>` to a `Vc<Box<dyn K>>`.
/// This operation also resolves the `Vc`.
Expand All @@ -427,7 +423,7 @@ where
/// removing the need for a `Result` return type.
pub async fn try_resolve_sidecast<K>(vc: Self) -> Result<Option<Vc<K>>, ResolveTypeError>
where
K: VcValueTrait + ?Sized + Send,
K: VcValueTrait + ?Sized,
{
let raw_vc: RawVc = vc.node;
let raw_vc = raw_vc
Expand All @@ -446,8 +442,7 @@ where
/// Returns `None` if the underlying value type is not a `K`.
pub async fn try_resolve_downcast<K>(vc: Self) -> Result<Option<Vc<K>>, ResolveTypeError>
where
K: Upcast<T>,
K: VcValueTrait + ?Sized + Send,
K: Upcast<T> + VcValueTrait + ?Sized,
{
let raw_vc: RawVc = vc.node;
let raw_vc = raw_vc
Expand All @@ -466,8 +461,7 @@ where
/// Returns `None` if the underlying value type is not a `K`.
pub async fn try_resolve_downcast_type<K>(vc: Self) -> Result<Option<Vc<K>>, ResolveTypeError>
where
K: Upcast<T>,
K: VcValueType,
K: Upcast<T> + VcValueType,
{
let raw_vc: RawVc = vc.node;
let raw_vc = raw_vc
Expand All @@ -482,20 +476,20 @@ where

impl<T> CollectiblesSource for Vc<T>
where
T: ?Sized + Send,
T: ?Sized,
{
fn take_collectibles<Vt: VcValueTrait + Send>(self) -> AutoSet<Vc<Vt>> {
fn take_collectibles<Vt: VcValueTrait>(self) -> AutoSet<Vc<Vt>> {
self.node.take_collectibles()
}

fn peek_collectibles<Vt: VcValueTrait + Send>(self) -> AutoSet<Vc<Vt>> {
fn peek_collectibles<Vt: VcValueTrait>(self) -> AutoSet<Vc<Vt>> {
self.node.peek_collectibles()
}
}

impl<T> From<RawVc> for Vc<T>
where
T: ?Sized + Send,
T: ?Sized,
{
fn from(node: RawVc) -> Self {
Self {
Expand All @@ -507,7 +501,7 @@ where

impl<T> TraceRawVcs for Vc<T>
where
T: ?Sized + Send,
T: ?Sized,
{
fn trace_raw_vcs(&self, trace_context: &mut TraceRawVcsContext) {
TraceRawVcs::trace_raw_vcs(&self.node, trace_context);
Expand All @@ -516,8 +510,7 @@ where

impl<T> ValueDebugFormat for Vc<T>
where
T: ?Sized + Send,
T: Upcast<Box<dyn ValueDebug>>,
T: Upcast<Box<dyn ValueDebug>> + Send + Sync + ?Sized,
{
fn value_debug_format(&self, depth: usize) -> ValueDebugFormatString {
ValueDebugFormatString::Async(Box::pin(async move {
Expand Down Expand Up @@ -560,11 +553,11 @@ where
}
}

impl<T> Unpin for Vc<T> where T: ?Sized + Send {}
impl<T> Unpin for Vc<T> where T: ?Sized {}

impl<T> Default for Vc<T>
where
T: ValueDefault + Send,
T: ValueDefault,
{
fn default() -> Self {
T::value_default()
Expand Down
Loading

0 comments on commit ac24457

Please sign in to comment.