Skip to content

Commit

Permalink
Remove Coerce for newtypes
Browse files Browse the repository at this point in the history
Summary:
`#[derive(Coerce)]` implements coerce:
- between `Foo<A>` and `Foo<B>` if `A` coerces to `B`
- between `Foo(A)` and `A`

This is confusing.

Remove the latter.

Generally I think we should probably remove `Coerce`: while I like the idea, it is hard to use from both Rust typechecking and safety point of views.

Reviewed By: JakobDegen

Differential Revision: D56123012

fbshipit-source-id: 2109ae1bb1d88bcc8d36e9918706f800d0ea1924
  • Loading branch information
stepancheg authored and facebook-github-bot committed Apr 15, 2024
1 parent e2dd0d6 commit 83245b3
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 67 deletions.
46 changes: 2 additions & 44 deletions starlark/src/coerce.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,37 +39,6 @@ use starlark_map::small_map::SmallMap;
/// and it must be safe for the `From` to be treated as `To`, namely same (or less restrictive) alignment,
/// no additional invariants, value can be dropped as `To`.
///
/// One use of `Coerce` is around newtype wrappers:
///
/// ```
/// use starlark::coerce::coerce;
/// use starlark::coerce::Coerce;
/// #[repr(transparent)]
/// #[derive(Debug, Coerce)]
/// struct Wrapper(String);
///
/// let value = vec![Wrapper("hello".to_owned()), Wrapper("world".to_owned())];
/// assert_eq!(coerce::<_, &Vec<String>>(&value).join(" "), "hello world");
/// let mut value = coerce::<_, Vec<String>>(value);
/// assert_eq!(value.pop(), Some("world".to_owned()));
/// ```
///
/// Another involves containers:
///
/// ```
/// use starlark::coerce::coerce;
/// use starlark::coerce::Coerce;
/// # #[derive(Coerce)]
/// # #[repr(transparent)]
/// # struct Wrapper(String);
/// #[derive(Coerce)]
/// #[repr(C)]
/// struct Container<T>(i32, T);
///
/// let value = Container(20, Wrapper("twenty".to_owned()));
/// assert_eq!(coerce::<_, &Container<String>>(&value).1, "twenty");
/// ```
///
/// If you only need [`coerce`] on newtype references,
/// then the [`ref-cast` crate](https://crates.io/crates/ref-cast)
/// provides that, along with automatic derivations (no `unsafe` required).
Expand Down Expand Up @@ -170,22 +139,10 @@ mod tests {
assert_eq!(f(("test",)), (x.as_str(),))
}

#[test]
fn test_coerce_lifetime() {
#[derive(Coerce)]
#[repr(transparent)]
struct NewtypeWithLifetime<'v>(&'v [usize]);

let newtype = NewtypeWithLifetime(&[1, 2]);
assert_eq!(&[1, 2], coerce(newtype))
}

#[test]
fn test_coerce_type_and_lifetime_params() {
#[derive(Coerce)]
#[repr(C)]
struct Aaa<'a>(&'a u32);
#[derive(Coerce)]
#[repr(C)]
struct Bbb<'a>(&'a u32);

Expand All @@ -212,10 +169,11 @@ mod tests {
fn test_coerce_is_unsound() {
// TODO(nga): fix it.

#[derive(Coerce)]
#[repr(transparent)]
struct Newtype(u8);

unsafe impl Coerce<u8> for Newtype {}

#[derive(Coerce)]
#[repr(transparent)]
struct Struct<T: Trait>(T::Assoc);
Expand Down
27 changes: 4 additions & 23 deletions starlark_derive/src/coerce.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ use syn::Type;

// This macro does two related derivations depending on whether there are any generic parameters.
//
// struct A(B) ==> coerce both ways between A and B
// struct A<T>(...) => coerce A<T1> to A<T2> if coerce T1 to T2 and all the fields support it
pub fn derive_coerce(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
let input = parse_macro_input!(input as DeriveInput);
Expand All @@ -52,33 +51,15 @@ fn derive_coerce_impl(input: DeriveInput) -> syn::Result<proc_macro2::TokenStrea
check_repr(&input)?;

if input.generics.type_params().count() == 0 {
derive_coerce_inner(input)
Err(syn::Error::new_spanned(
&input,
"`Coerce` can only be derived for types with type parameters",
))
} else {
derive_coerce_params(input)
}
}

fn derive_coerce_inner(input: DeriveInput) -> syn::Result<proc_macro2::TokenStream> {
let lifetimes = input.generics.lifetimes().collect::<Vec<_>>();

let field = match &input.data {
Data::Struct(x) if x.fields.len() == 1 => x.fields.iter().next().unwrap(),
_ => {
return Err(syn::Error::new_spanned(
input,
"Type-parameter free types must be a single field struct",
));
}
};

let type1 = input.ident;
let type2 = &field.ty;
Ok(quote! {
unsafe impl < #(#lifetimes),* > starlark::coerce::Coerce<#type1< #(#lifetimes),* >> for #type2 {}
unsafe impl < #(#lifetimes),* > starlark::coerce::Coerce<#type2> for #type1< #(#lifetimes),* > {}
})
}

#[derive(Copy, Clone)]
enum ParamNameMapping {
From,
Expand Down

0 comments on commit 83245b3

Please sign in to comment.