Remove Yokeable::Output from ZeroCopyFrom trait #1499

sffc · 2022-01-12T02:39:30Z

Fixes #1255
Replaces #1257

Using this trait:

pub trait ZeroCopyFromV2<'a, C: ?Sized>: 'a {
    fn zero_copy_from_v2(cart: &'a C) -> Self;
}

And this function signature:

impl<Y, C> Yoke<Y, C>
where
    Y: for<'a> Yokeable<'a>,
    for<'a> YokeTraitHack<<Y as Yokeable<'a>>::Output>: ZeroCopyFromV2<'a, <C as Deref>::Target>,
    C: StableDeref + Deref,
{
    pub fn attach_to_zero_copy_v2_cart(cart: C) -> Self {
        Yoke::<Y, C>::attach_to_cart(cart, |c| {
            YokeTraitHack::<<Y as Yokeable>::Output>::zero_copy_from_v2(c).0
        })
    }
}

I managed to get this to compile:

    let yoke = Yoke::<
        Cow<'static, str>,
        String
    >::attach_to_zero_copy_v2_cart("demo".to_string());

sffc · 2022-01-12T02:41:15Z

Without the YokeTraitHack, I was getting errors like

error[E0277]: the trait bound `for<'a> <Cow<'static, str> as Yokeable<'a>>::Output: ZeroCopyFromV2<'a, str>` is not satisfied
   --> utils/yoke/src/zero_copy_from.rs:313:16
    |
313 |       let yoke = Yoke::<
    |  ________________^
314 | |         Cow<'static, str>,
315 | |         String
316 | |     >::attach_to_zero_copy_v2_cart("demo".to_string());
    | |__________________________________^ the trait `for<'a> ZeroCopyFromV2<'a, str>` is not implemented for `<Cow<'static, str> as Yokeable<'a>>::Output`
    |
note: required by `zero_copy_from::<impl Yoke<Y, C>>::attach_to_zero_copy_v2_cart`
   --> utils/yoke/src/zero_copy_from.rs:151:5
    |
151 |     pub fn attach_to_zero_copy_v2_cart(cart: C) -> Self {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: consider further restricting the associated type
    |
312 | fn test_v2() where for<'a> <Cow<'static, str> as Yokeable<'a>>::Output: ZeroCopyFromV2<'a, str> {
    |              ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

Manishearth

lgtm, minor issue

Manishearth · 2022-01-12T17:37:49Z

experimental/segmenter/src/provider.rs

@@ -61,8 +61,8 @@ impl Deref for LineBreakPropertyTable<'_> {
    }
 }

-impl<'a> ZeroCopyFrom<LineBreakPropertyTable<'a>> for LineBreakPropertyTable<'static> {
-    fn zero_copy_from<'b>(cart: &'b LineBreakPropertyTable<'a>) -> <Self as Yokeable<'b>>::Output {
+impl<'zcf> ZeroCopyFrom<'zcf, LineBreakPropertyTable<'_>> for LineBreakPropertyTable<'zcf> {


praise: good call on 'zcf

Manishearth · 2022-01-12T17:38:35Z

utils/yoke/derive/examples/derive.rs

@@ -51,12 +51,12 @@ pub struct HasTuples<'data> {
 }

 pub fn assert_zcf_tuples<'b, 'data>(x: &'b HasTuples<'data>) -> HasTuples<'b> {
-    HasTuples::<'static>::zero_copy_from(x)
+    HasTuples::<'b>::zero_copy_from(x)


question: is this explicit lt still necessary? one of the benefits of the simplification is that a bunch of these won't be needed anymore

Manishearth · 2022-01-12T17:46:38Z

utils/zerovec/src/yoke_impls.rs

-        for<'b> Yokeable<'b, Output = <K as ZeroMapKV<'b>>::Container>,
-    <V as ZeroMapKV<'static>>::Container:
-        for<'b> Yokeable<'b, Output = <V as ZeroMapKV<'b>>::Container>,
+    <K as ZeroMapKV<'a>>::Container: ZeroCopyFrom<'a, <K as ZeroMapKV<'s>>::Container>,


thought: ah, these bounds get much simpler, i was hoping this would be the case

Manishearth · 2022-01-12T17:47:27Z

utils/yoke/src/zero_copy_from.rs

@@ -61,31 +62,42 @@ use stable_deref_trait::StableDeref;
 /// }
 ///
 /// // Reference from a borrowed version of self
-/// impl<'data> ZeroCopyFrom<MyStruct<'data>> for MyStruct<'static> {
-///     fn zero_copy_from<'b>(cart: &'b MyStruct<'data>) -> MyStruct<'b> {
+/// impl<'zcf> ZeroCopyFrom<'zcf, MyStruct<'_>> for MyStruct<'zcf> {


nit: the yokeable impl is no longer necessary in the docs above

Manishearth · 2022-01-12T19:15:41Z

utils/zerovec/src/yoke_impls.rs

-impl<'a, T: 'static + AsULE + ?Sized> ZeroCopyFrom<'a, ZeroVec<'_, T>> for ZeroVec<'a, T> {
+impl<'zcf, T> ZeroCopyFrom<'zcf, ZeroVec<'_, T>> for ZeroVec<'zcf, T>
+where
+    T: 'zcf + AsULE + ?Sized,


issue: not sure if T: 'zcf makes sense here, but aiui it doesn't cause harm to have

We need either T: 'zcf or T: 'static to make the compiler happy when compiling ZeroVec<'zcf, T>, so I figured we should use the less restrictive lifetime.

Yeah, I think it should be 'static for now, because a non-'static AsULE doesn't quite make sense (.get() can't work), and it seems misleading to have a 'zcf bound that isn't actually useful.

Manishearth · 2022-01-12T19:16:32Z

utils/zerovec/src/yoke_impls.rs

-    V: 'static + for<'b> ZeroMapKV<'b> + ?Sized,
-    <K as ZeroMapKV<'a>>::Container: ZeroCopyFrom<'a, <K as ZeroMapKV<'s>>::Container>,
-    <V as ZeroMapKV<'a>>::Container: ZeroCopyFrom<'a, <V as ZeroMapKV<'s>>::Container>,
+    K: 'zcf + for<'b> ZeroMapKV<'b> + ?Sized,


ditto: does the 'zcf bound actually get us anything?

Initial compiling ZeroCopyFromV2

0bb32aa

sffc requested a review from Manishearth January 12, 2022 02:39

sffc added 5 commits January 12, 2022 00:08

Update more uses of ZCF

3d19612

Fix all compiling in yoke crate

bb8454a

Fix zerovec

eeb564a

Fix remaining build issues

6a485c6

Use 'zcf for the lifetime parameter

96d5f75

sffc changed the title ~~Initial compiling ZeroCopyFromV2~~ Remove Yokeable::Output from ZeroCopyFrom trait Jan 12, 2022

Another 'zcf

c7e5d9e

sffc marked this pull request as ready for review January 12, 2022 09:15

sffc requested review from aethanyc and makotokato as code owners January 12, 2022 09:15

sffc removed request for aethanyc and makotokato January 12, 2022 09:15

Manishearth previously approved these changes Jan 12, 2022

View reviewed changes

Simplification and review feedback

2655d63

sffc dismissed Manishearth’s stale review via 2655d63 January 12, 2022 19:06

sffc requested a review from Manishearth January 12, 2022 19:06

Manishearth reviewed Jan 12, 2022

View reviewed changes

sffc requested a review from Manishearth January 12, 2022 21:03

Use 'static instead of 'zcf in bounds

f14b3fe

Manishearth approved these changes Jan 12, 2022

View reviewed changes

sffc merged commit 57c44dd into unicode-org:main Jan 12, 2022

sffc deleted the zcf_v2 branch January 12, 2022 22:42

Manishearth mentioned this pull request Jan 12, 2022

ZeroCopyFrom simplification attempt #1257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Yokeable::Output from ZeroCopyFrom trait #1499

Remove Yokeable::Output from ZeroCopyFrom trait #1499

sffc commented Jan 12, 2022 •

edited

Loading

sffc commented Jan 12, 2022

Manishearth left a comment

Manishearth Jan 12, 2022

Manishearth Jan 12, 2022

Manishearth Jan 12, 2022

Manishearth Jan 12, 2022

Manishearth Jan 12, 2022

sffc Jan 12, 2022

Manishearth Jan 12, 2022

sffc Jan 12, 2022

Manishearth Jan 12, 2022

Remove Yokeable::Output from ZeroCopyFrom trait #1499

Remove Yokeable::Output from ZeroCopyFrom trait #1499

Conversation

sffc commented Jan 12, 2022 • edited Loading

sffc commented Jan 12, 2022

Manishearth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Jan 12, 2022 •

edited

Loading