Fix soundness issue in Yoke::attach_to_cart() around implied bounds

[Manishearth]

Fixes #2926

The reasoning can be found in the safety docs added.

There are two potential fixes here. One is to force Yoke to always be invariant over any lifetimes found in the cart. This is unfortunate, since Yoke<_, &[u8]> is a useful cart and there's no actual safety reason for that to need to be invariant. Invariant lifetimes are annoying to work with. However, the safe function attach_to_cart() will support all carts, including ones with wacky borrowing.

The alternative is to enforce C::Target: 'static in attach_to_cart(). This will still allow Yoke<_, &[u8]> to be constructed with the method, but if there are borrows behind the deref, they will have to be constructed unsafely via replace_cart(). That's unfortunate but retains maximum flexibility for the user. I prefer this.
The latter is what is implemented.

[steffahn]

You can define an invariant marker using PhantomData<fn(T) -> T> or PhantomData<fn() -> Cell<T>> which avoids the need for an unsafe implementation of Send or Sync

[Manishearth]

oh beautiful, thanks. Forgot you could do that.

[sffc]

What are example call sites that work in option 1 but not option 2 and vice versa?

[Manishearth]

What are example call sites that work in option 1 but not option 2 and vice versa?

Option 1 (force invariance):

• You can have a Cart of type Box<&[..]> (Yoke<Y, Box<&[u8]>>)
• You cannot treat carts with borrows, _including the cart type &[u8] as covariant: so Yoke<Y, &[u8]> will have a pinned lifetime when it doesn't need to

Option 2:

• You can have a Cart of type Box<&[..]> (Yoke<Y, Box<&[u8]>>), BUT you cannot create it with attach_to_cart(); so you cannot create it safely.
• Yoke<Y, &[u8]> will be quite flexible and you can pass it around easily

[sffc]

I see; I think we definitely want Yoke<T, &[u8]> to have a covariant lifetime.

[sffc]

Is there a difference between Option 1 and Option 2 regarding the ability to borrow local variables, like you can apparently do now?

[Manishearth]

Is there a difference between Option 1 and Option 2 regarding the ability to borrow local variables, like you can apparently do now?

In the attach_to_cart closure? No. As the cart itself? Both allow it, Option 1 forces the Yoke of such carts to be invariant, making it a bit more awkward to use.

At this point I'm heavily leaning towards Option 2 and I think we should just do that, I just need to update this PR.

[CAD97]

(PR description should be updated to state that option 2 is taken by the PR instead of option 1)

[Manishearth]

Hmm, is the IsCovariant stuff even meaningful now? I don't think it enables anything

[Manishearth]

@sffc are you fine with me removing IsCovariant? I can't remember why we added it, I think we wanted it for trait objects but we don't need it anymore

[Manishearth]

This change doesn't break it but it breaks any meaningful tests of IsCovariant since it mostly useful for trait objects with nested borrows

[sffc]

IsCovariant originated in #1041 to fix #760. The line of code using IsCovariant in a bound no longer exists, so it seems harmless for ICU4X to remove the trait, but we should consider what alternatives people with a problem similar to #760 should use now.

Alternatively, should we use C: IsCovariant as a bound instead of C::Target: 'static? I think that was floated as a third option but it was not pursued?

[Manishearth]

IsCovariant originated in #1041 to fix #760. The line of code using IsCovariant in a bound no longer exists, so it seems harmless for ICU4X to remove the trait, but we should consider what alternatives people with a problem similar to #760 should use now.

I still think that carts with complex borrowing are probably a sign of bad design (especially nested yoking like we were doing there); and we should not aim to support them in our main attach_to_cart() function. For these use cases I'd rather expose more unsafe APIs so that people can opt in to this stuff, because trait objects and variance are a particularly tricky zone to reason about even if we do use traits like IsCovariant (I'm very concerned about some niche soundness issues there). Note that IsCovariant also required unsafe code to be useful, and Yoke didn't really know about it.

We still support trait objects as carts, just not trait object carts where the borrowing is hidden behind the trait object, constructed via attach_to_cart().

We may even be able to expose safe APIs for some of the use cases here, but I'd want to separate it from the primary attach_to_cart API either way.

Alternatively, should we use C: IsCovariant as a bound instead of C::Target: 'static? I think that was floated as a third option but it was not pursued?

That requires far more IsCovariant implementations and it means you need to implement that unsafe trait for every potential cart you ever need. We currently do not require custom derives on carts.

[Manishearth]

I removed IsCovariant. If the use case crops up again we can try to design for it more clearly, or just bring IsCovariant back (the rest of this PR does not break IsCovariant, it just makes it harder to write a useful example, and I'm a bit uncomfortable writing one without thinking a lot about soundness)