Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(turbo-tasks): Simplify most type bounds on Vc<T> and related types #72823

Merged
merged 3 commits into from
Nov 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 {}

Comment on lines -433 to -435
Copy link
Member Author

@bgw bgw Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out these unsafe impls weren't needed at all. The future is already Send + Sync without them.

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 {}
Comment on lines -204 to -206
Copy link
Member Author

@bgw bgw Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These unsafe impls were okay (there's no way to construct a Vc<T> that isn't Send + Sync), but they weren't providing hardly any value because Send + Sync is auto-implemented already when T: Send + Sync.

The only effect of removing these was a couple added bounds on ValueDebugFormat for Vc and ResolvedVc.

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,
Comment on lines -449 to -450
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeating the same left-hand-side for a where clause is valid, but unconventional. Converting this to use + instead to make it easier to read.

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
Loading