Skip to content

Commit

Permalink
Fix ReadRef<T>::cell when T != T::Read::Repr (#8845)
Browse files Browse the repository at this point in the history
## Description

Fixes the test case introduced in #8843.

The theory is that Vcs are supposed to be stored as `<<T as
VcValueType>::Read as VcRead<T>>::Repr`. However, it looks like due to
an oversight, `ReadRef::cell` is trying to store the value as `T`.

This introduces a way to perform 

## Testing Instructions

```
cargo nextest r -p turbo-tasks -p turbo-tasks-memory
```
  • Loading branch information
bgw authored Jul 29, 2024
1 parent 12f7f73 commit 0275dee
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 13 deletions.
1 change: 0 additions & 1 deletion crates/turbo-tasks-memory/tests/generics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ async fn test_changing_generic() {

// Test that we can convert a `Vc` to a `ReadRef`, and then back to a `Vc`.
#[tokio::test]
#[ignore = "TODO: This panics! There's a casting bug in the generics implementation."]
async fn test_read_ref_round_trip() {
run(&REGISTRATION, async move {
let c: Vc<Option<Vc<u8>>> = Vc::cell(Some(Vc::cell(1)));
Expand Down
10 changes: 8 additions & 2 deletions crates/turbo-tasks/src/read_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use crate::{
debug::{ValueDebugFormat, ValueDebugFormatString},
macro_helpers::find_cell_by_type,
trace::{TraceRawVcs, TraceRawVcsContext},
triomphe_utils::unchecked_sidecast_triomphe_arc,
SharedReference, Vc, VcRead, VcValueType,
};

Expand Down Expand Up @@ -242,8 +243,13 @@ where
/// Returns a new cell that points to the same value as the given
/// reference.
pub fn cell(read_ref: ReadRef<T>) -> Vc<T> {
let local_cell = find_cell_by_type(T::get_value_type_id());
local_cell.update_shared_reference(SharedReference::new(read_ref.0));
let type_id = T::get_value_type_id();
let local_cell = find_cell_by_type(type_id);
// SAFETY: `T` and `T::Read::Repr` must have equivalent memory representations,
// guaranteed by the unsafe implementation of `VcValueType`.
local_cell.update_shared_reference(SharedReference::new(unsafe {
unchecked_sidecast_triomphe_arc::<T, <T::Read as VcRead<T>>::Repr>(read_ref.0)
}));
Vc {
node: local_cell.into(),
_t: PhantomData,
Expand Down
39 changes: 29 additions & 10 deletions crates/turbo-tasks/src/triomphe_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,45 @@ use unsize::Coercion;
/// Attempt to downcast a [`triomphe::Arc<dyn Any + Send +
/// Sync>`][`triomphe::Arc`] to a concrete type.
///
/// Checks that the downcast is safe using [`Any::is`].
///
/// Ported from [`std::sync::Arc::downcast`] to [`triomphe::Arc`].
pub fn downcast_triomphe_arc<T: Any + Send + Sync>(
this: triomphe::Arc<dyn Any + Send + Sync>,
) -> Result<triomphe::Arc<T>, triomphe::Arc<dyn Any + Send + Sync>> {
if (*this).is::<T>() {
unsafe {
// Get the pointer to the offset (*const T) inside of the ArcInner.
let ptr = triomphe::Arc::into_raw(this);
// SAFETY: The negative offset from the data (ptr) in an Arc to the start of the
// data structure is fixed regardless of type `T`.
//
// SAFETY: Casting from a fat pointer to a thin pointer is safe, as long as the
// types are compatible (they are).
Ok(triomphe::Arc::from_raw(ptr.cast()))
}
unsafe { Ok(unchecked_sidecast_triomphe_arc(this)) }
} else {
Err(this)
}
}

/// Transmutes the contents of `Arc<T>` to `Arc<U>`. Updates the `Arc`'s fat
/// pointer metadata.
///
/// Unlike [`downcast_triomphe_arc`] this make no checks the transmute is safe.
///
/// # Safety
///
/// It must be [safe to transmute][transmutes] from `T` to `U`.
///
/// [transmutes]: https://doc.rust-lang.org/nomicon/transmutes.html
pub unsafe fn unchecked_sidecast_triomphe_arc<T, U>(this: triomphe::Arc<T>) -> triomphe::Arc<U>
where
T: ?Sized,
{
unsafe {
// Get the pointer to the offset (*const T) inside of the ArcInner.
let ptr = triomphe::Arc::into_raw(this);
// SAFETY: The negative offset from the data (ptr) in an Arc to the start of the
// data structure is fixed regardless of type `T`.
//
// SAFETY: Casting from a fat pointer to a thin pointer is safe, as long as the
// types are compatible (they are).
triomphe::Arc::from_raw(ptr.cast())
}
}

/// [`Coercion::to_any`] except that it coerces to `dyn Any + Send + Sync` as
/// opposed to `dyn Any`.
pub fn coerce_to_any_send_sync<T: Any + Send + Sync>() -> Coercion<T, dyn Any + Send + Sync> {
Expand Down

0 comments on commit 0275dee

Please sign in to comment.