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.

Sign up for GitHub

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

Remove RcStruct #1299

Merged
merged 13 commits into from
Nov 15, 2021
Merged

Remove RcStruct #1299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
08ad0cd
Get rid of lifetime on SerdeSeDataStruct
Manishearth Nov 15, 2021
b5d3fc3
Use owned data structs in SerdeSeDataStruct
Manishearth Nov 15, 2021
94b8473
Move HelloWorld over to from_owned
Manishearth Nov 15, 2021
d18b5ae
Remove partial owned test
Manishearth Nov 15, 2021
3964702
Remove lifetime on ErasedCart
Manishearth Nov 15, 2021
1033f55
Move ErasedCart to Yoke
Manishearth Nov 15, 2021
602fc4f
Use from_owned in DataMarker test
Manishearth Nov 15, 2021
eee3ee0
Remove use of from_partial_owned from DataWarehouse test
Manishearth Nov 15, 2021
37dc4de
Remove DataPayload::from_partial_owned()
Manishearth Nov 15, 2021
9e818fa
Remove RcStruct
Manishearth Nov 15, 2021
bb0bb93
Use custom derive for yoke
Manishearth Nov 15, 2021
28ea424
appease clippy
Manishearth Nov 15, 2021
3a0b59a
Fix test
Manishearth Nov 15, 2021
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
82 changes: 1 addition & 81 deletions provider/core/src/data_provider.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,26 +95,10 @@ pub struct DataResponseMetadata {
pub data_langid: Option<LanguageIdentifier>,
}

/// Dummy trait that lets us `dyn Drop`
///
/// `dyn Drop` isn't legal (and doesn't make sense since `Drop` is not
/// implement on all destructible types). However, all trait objects come with
/// a destructor, so we can just use an empty trait to get a destructor object.
///
/// This should eventually be moved into the yoke crate once it's cleaner
/// https://github.com/unicode-org/icu4x/issues/1284
pub(crate) trait ErasedDestructor<'data>: IsCovariant<'data> {}
impl<'data, T: IsCovariant<'data>> ErasedDestructor<'data> for T {}

/// All that Yoke needs from the Cart type is the destructor. We can
/// use a trait object to handle that.
pub(crate) type ErasedCart<'data> = Rc<dyn ErasedDestructor<'data> + 'data>;

pub(crate) enum DataPayloadInner<M>
where
M: DataMarker,
{
RcStruct(Yoke<M::Yokeable, ErasedCart<'static>>),
Owned(Yoke<M::Yokeable, ()>),
RcBuf(Yoke<M::Yokeable, Rc<[u8]>>),
}
Expand All @@ -126,8 +110,7 @@ where
/// several data stores ("carts"):
///
/// 1. Fully-owned structured data ([`DataPayload::from_owned()`])
/// 2. Partially-owned structured data in an [`Rc`] ([`DataPayload::from_partial_owned()`])
/// 3. A reference-counted byte buffer ([`DataPayload::try_from_rc_buffer()`])
/// 2. A reference-counted byte buffer ([`DataPayload::try_from_rc_buffer()`])
///
/// The type of the data stored in [`DataPayload`], and the type of the structured data store
/// (cart), is determined by the [`DataMarker`] type parameter.
Expand Down Expand Up @@ -204,7 +187,6 @@ where
fn clone(&self) -> Self {
use DataPayloadInner::*;
let new_inner = match &self.inner {
RcStruct(yoke) => RcStruct(yoke.clone()),
Owned(yoke) => Owned(yoke.clone()),
RcBuf(yoke) => RcBuf(yoke.clone()),
};
Expand Down Expand Up @@ -237,48 +219,6 @@ fn test_clone_eq() {
assert_eq!(p1, p2);
}

impl<M> DataPayload<M>
where
M: DataMarker,
{
/// Convert an [`Rc`]`<`[`Cart`]`>` into a [`DataPayload`].
///
/// # Examples
///
/// ```
/// use icu_provider::prelude::*;
/// use icu_provider::hello_world::*;
/// use std::borrow::Cow;
/// use std::rc::Rc;
///
/// let local_data = "example".to_string();
///
/// let rc_struct = Rc::from(HelloWorldV1 {
/// message: Cow::Owned(local_data),
/// });
///
/// let payload = DataPayload::<HelloWorldV1Marker>::from_partial_owned(rc_struct.clone());
///
/// assert_eq!(payload.get(), &*rc_struct);
/// ```
///
/// [`Cart`]: crate::marker::DataMarker::Cart
#[inline]
pub fn from_partial_owned<T>(data: Rc<T>) -> Self
where
M::Yokeable: ZeroCopyFrom<T>,
T: 'static + IsCovariant<'static>,
{
let yoke = unsafe {
// safe because we're not throwing away any actual data, simply type-erasing it
Yoke::attach_to_rc_cart(data).replace_cart(|c| c as ErasedCart<'static>)
};
Self {
inner: DataPayloadInner::RcStruct(yoke),
}
}
}

impl<M> DataPayload<M>
where
M: DataMarker,
Expand Down Expand Up @@ -454,7 +394,6 @@ where
{
use DataPayloadInner::*;
match &mut self.inner {
RcStruct(yoke) => yoke.with_mut(f),
Owned(yoke) => yoke.with_mut(f),
RcBuf(yoke) => yoke.with_mut(f),
}
Expand All @@ -479,7 +418,6 @@ where
pub fn get<'a>(&'a self) -> &'a <M::Yokeable as Yokeable<'a>>::Output {
use DataPayloadInner::*;
match &self.inner {
RcStruct(yoke) => yoke.get(),
Owned(yoke) => yoke.get(),
RcBuf(yoke) => yoke.get(),
}
Expand Down Expand Up @@ -544,9 +482,6 @@ where
{
use DataPayloadInner::*;
match self.inner {
RcStruct(yoke) => DataPayload {
inner: RcStruct(yoke.project(f)),
},
Owned(yoke) => DataPayload {
inner: Owned(yoke.project(f)),
},
Expand Down Expand Up @@ -601,9 +536,6 @@ where
{
use DataPayloadInner::*;
match &self.inner {
RcStruct(yoke) => DataPayload {
inner: RcStruct(yoke.project_cloned(f)),
},
Owned(yoke) => DataPayload {
inner: Owned(yoke.project_cloned(f)),
},
Expand Down Expand Up @@ -691,9 +623,6 @@ where
{
use DataPayloadInner::*;
match self.inner {
RcStruct(yoke) => DataPayload {
inner: RcStruct(yoke.project_with_capture(capture, f)),
},
Owned(yoke) => DataPayload {
inner: Owned(yoke.project_with_capture(capture, f)),
},
Expand Down Expand Up @@ -756,9 +685,6 @@ where
{
use DataPayloadInner::*;
match &self.inner {
RcStruct(yoke) => DataPayload {
inner: RcStruct(yoke.project_cloned_with_capture(capture, f)),
},
Owned(yoke) => DataPayload {
inner: Owned(yoke.project_cloned_with_capture(capture, f)),
},
Expand Down Expand Up @@ -854,9 +780,6 @@ where
{
use DataPayloadInner::*;
Ok(match self.inner {
RcStruct(yoke) => DataPayload {
inner: RcStruct(yoke.try_project_with_capture(capture, f)?),
},
Owned(yoke) => DataPayload {
inner: Owned(yoke.try_project_with_capture(capture, f)?),
},
Expand Down Expand Up @@ -923,9 +846,6 @@ where
{
use DataPayloadInner::*;
Ok(match &self.inner {
RcStruct(yoke) => DataPayload {
inner: RcStruct(yoke.try_project_cloned_with_capture(capture, f)?),
},
Owned(yoke) => DataPayload {
inner: Owned(yoke.try_project_cloned_with_capture(capture, f)?),
},
Expand Down
31 changes: 15 additions & 16 deletions provider/core/src/data_provider/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ).

use alloc::borrow::Cow;
use alloc::rc::Rc;
use alloc::string::String;
use core::fmt::Debug;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -45,8 +44,8 @@ struct HelloCombined<'data> {
/// Supports only HELLO_WORLD_V1. Uses `impl_dyn_provider!()`.
#[derive(Debug)]
struct DataWarehouse {
hello_v1: Rc<HelloWorldV1<'static>>,
hello_alt: Rc<HelloAlt>,
hello_v1: HelloWorldV1<'static>,
hello_alt: HelloAlt,
}

impl DataProvider<HelloWorldV1Marker> for DataWarehouse {
Expand All @@ -57,7 +56,7 @@ impl DataProvider<HelloWorldV1Marker> for DataWarehouse {
req.resource_path.key.match_key(HELLO_WORLD_V1)?;
Ok(DataResponse {
metadata: DataResponseMetadata::default(),
payload: Some(DataPayload::from_partial_owned(self.hello_v1.clone())),
payload: Some(DataPayload::from_owned(self.hello_v1.clone())),
})
}
}
Expand Down Expand Up @@ -86,7 +85,7 @@ impl DataProvider<HelloWorldV1Marker> for DataProvider2 {
req.resource_path.key.match_key(HELLO_WORLD_V1)?;
Ok(DataResponse {
metadata: DataResponseMetadata::default(),
payload: Some(DataPayload::from_partial_owned(self.data.hello_v1.clone())),
payload: Some(DataPayload::from_owned(self.data.hello_v1.clone())),
})
}
}
Expand All @@ -96,7 +95,7 @@ impl DataProvider<HelloAltMarker> for DataProvider2 {
req.resource_path.key.match_key(HELLO_ALT_KEY)?;
Ok(DataResponse {
metadata: DataResponseMetadata::default(),
payload: Some(DataPayload::from_partial_owned(self.data.hello_alt.clone())),
payload: Some(DataPayload::from_owned(self.data.hello_alt.clone())),
})
}
}
Expand All @@ -120,8 +119,8 @@ const DATA: &'static str = r#"{
fn get_warehouse(data: &'static str) -> DataWarehouse {
let data: HelloCombined = serde_json::from_str(data).expect("Well-formed data");
DataWarehouse {
hello_v1: Rc::from(data.hello_v1),
hello_alt: Rc::from(data.hello_alt),
hello_v1: data.hello_v1,
hello_alt: data.hello_alt,
}
}

Expand Down Expand Up @@ -159,7 +158,7 @@ fn get_request_alt() -> DataRequest {
fn test_warehouse_owned() {
let warehouse = get_warehouse(DATA);
let hello_data = get_payload_v1(&warehouse).unwrap();
assert!(matches!(hello_data.inner, DataPayloadInner::RcStruct(_)));
assert!(matches!(hello_data.inner, DataPayloadInner::Owned(_)));
assert!(matches!(
hello_data.get(),
HelloWorldV1 {
Expand All @@ -172,7 +171,7 @@ fn test_warehouse_owned() {
fn test_warehouse_owned_dyn_erased() {
let warehouse = get_warehouse(DATA);
let hello_data = get_payload_v1(&warehouse as &dyn ErasedDataProvider).unwrap();
assert!(matches!(hello_data.inner, DataPayloadInner::RcStruct(_)));
assert!(matches!(hello_data.inner, DataPayloadInner::Owned(_)));
assert!(matches!(
hello_data.get(),
HelloWorldV1 {
Expand All @@ -185,7 +184,7 @@ fn test_warehouse_owned_dyn_erased() {
fn test_warehouse_owned_dyn_generic() {
let warehouse = get_warehouse(DATA);
let hello_data = get_payload_v1(&warehouse as &dyn DataProvider<HelloWorldV1Marker>).unwrap();
assert!(matches!(hello_data.inner, DataPayloadInner::RcStruct(_)));
assert!(matches!(hello_data.inner, DataPayloadInner::Owned(_)));
assert!(matches!(
hello_data.get(),
HelloWorldV1 {
Expand All @@ -209,7 +208,7 @@ fn test_provider2() {
let warehouse = get_warehouse(DATA);
let provider = DataProvider2::from(warehouse);
let hello_data = get_payload_v1(&provider).unwrap();
assert!(matches!(hello_data.inner, DataPayloadInner::RcStruct(_)));
assert!(matches!(hello_data.inner, DataPayloadInner::Owned(_)));
assert!(matches!(
hello_data.get(),
HelloWorldV1 {
Expand All @@ -223,7 +222,7 @@ fn test_provider2_dyn_erased() {
let warehouse = get_warehouse(DATA);
let provider = DataProvider2::from(warehouse);
let hello_data = get_payload_v1(&provider as &dyn ErasedDataProvider).unwrap();
assert!(matches!(hello_data.inner, DataPayloadInner::RcStruct(_)));
assert!(matches!(hello_data.inner, DataPayloadInner::Owned(_)));
assert!(matches!(
hello_data.get(),
HelloWorldV1 {
Expand All @@ -237,7 +236,7 @@ fn test_provider2_dyn_erased_alt() {
let warehouse = get_warehouse(DATA);
let provider = DataProvider2::from(warehouse);
let hello_data = get_payload_alt(&provider as &dyn ErasedDataProvider).unwrap();
assert!(matches!(hello_data.inner, DataPayloadInner::RcStruct(_)));
assert!(matches!(hello_data.inner, DataPayloadInner::Owned(_)));
assert!(matches!(hello_data.get(), HelloAlt { .. }));
}

Expand All @@ -246,7 +245,7 @@ fn test_provider2_dyn_generic() {
let warehouse = get_warehouse(DATA);
let provider = DataProvider2::from(warehouse);
let hello_data = get_payload_v1(&provider as &dyn DataProvider<HelloWorldV1Marker>).unwrap();
assert!(matches!(hello_data.inner, DataPayloadInner::RcStruct(_)));
assert!(matches!(hello_data.inner, DataPayloadInner::Owned(_)));
assert!(matches!(
hello_data.get(),
HelloWorldV1 {
Expand All @@ -260,7 +259,7 @@ fn test_provider2_dyn_generic_alt() {
let warehouse = get_warehouse(DATA);
let provider = DataProvider2::from(warehouse);
let hello_data = get_payload_alt(&provider as &dyn DataProvider<HelloAltMarker>).unwrap();
assert!(matches!(hello_data.inner, DataPayloadInner::RcStruct(_)));
assert!(matches!(hello_data.inner, DataPayloadInner::Owned(_)));
assert!(matches!(hello_data.get(), HelloAlt { .. }));
}

Expand Down
31 changes: 4 additions & 27 deletions provider/core/src/erased.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

//! Collection of traits for providers that support type erasure of data structs.

use crate::data_provider::ErasedCart;
use crate::error::Error;
use crate::prelude::*;
use crate::yoke::*;
Expand Down Expand Up @@ -82,7 +81,6 @@ where
fn upcast(other: DataPayload<M>) -> DataPayload<ErasedDataStructMarker> {
use crate::data_provider::DataPayloadInner::*;
let owned: Box<dyn ErasedDataStruct> = match other.inner {
RcStruct(yoke) => Box::new(yoke),
Owned(yoke) => Box::new(yoke),
RcBuf(yoke) => Box::new(yoke),
};
Expand Down Expand Up @@ -141,18 +139,8 @@ impl DataPayload<ErasedDataStructMarker> {
Owned(yoke) => {
let any_box: Box<dyn Any> = yoke.into_yokeable().0.into_any();
// `any_box` is the Yoke that was converted into the `dyn ErasedDataStruct`. It
// could have been either the RcStruct or the Owned variant of Yoke.
// Check first for Case 2: an RcStruct Yoke.
let y1 = any_box.downcast::<Yoke<M::Yokeable, ErasedCart>>();
let any_box = match y1 {
Ok(yoke) => {
return Ok(DataPayload {
inner: RcStruct(*yoke),
})
}
Err(any_box) => any_box,
};
// Check for Case 3: an Owned Yoke.
// could have been either the RcBuf or the Owned variant of Yoke.
// Check for an Owned Yoke.
let y2 = any_box.downcast::<Yoke<M::Yokeable, ()>>();
let any_box = match y2 {
Ok(yoke) => {
Expand All @@ -162,7 +150,7 @@ impl DataPayload<ErasedDataStructMarker> {
}
Err(any_box) => any_box,
};
// Check for Case 4: an RcBuf Yoke.
// Check for an RcBuf Yoke.
let y2 = any_box.downcast::<Yoke<M::Yokeable, Rc<[u8]>>>();
let any_box = match y2 {
Ok(yoke) => {
Expand All @@ -181,7 +169,7 @@ impl DataPayload<ErasedDataStructMarker> {
// This is unreachable because an ErasedDataStruct payload can only be constructed as fully owned
// (It is impossible for clients to construct an ErasedDataStruct payload manually since ErasedDataStructBox
// has a private field)
RcStruct(_) | RcBuf(_) => unreachable!(),
RcBuf(_) => unreachable!(),
}
}
}
Expand Down Expand Up @@ -254,17 +242,6 @@ mod test {
use crate::marker::CowStrMarker;
use alloc::borrow::Cow;

#[test]
fn test_erased_case_2() {
let data = Rc::new("foo".to_string());
let original = DataPayload::<CowStrMarker>::from_partial_owned(data);
let upcasted = ErasedDataStructMarker::upcast(original);
let downcasted = upcasted
.downcast::<CowStrMarker>()
.expect("Type conversion");
assert_eq!(downcasted.get(), "foo");
}

#[test]
fn test_erased_case_3() {
let data = "foo".to_string();
Expand Down
3 changes: 1 addition & 2 deletions provider/core/src/hello_world.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use crate::prelude::*;
use crate::yoke::{self, *};
use alloc::borrow::Cow;
use alloc::boxed::Box;
use alloc::rc::Rc;
use alloc::string::ToString;
use alloc::vec::Vec;
use core::fmt::Debug;
Expand Down Expand Up @@ -133,7 +132,7 @@ impl DataProvider<HelloWorldV1Marker> for HelloWorldProvider {
metadata: DataResponseMetadata {
data_langid: Some(langid.clone()),
},
payload: Some(DataPayload::from_partial_owned(Rc::from(data))),
payload: Some(DataPayload::from_owned(data)),
})
}
}
Expand Down
Loading