Add return type to `Yoke::with_mut` and `Yokeable::transform_mut`

[jdygert-spok]

I believe this is still sound since R can't involve 'b.

fn with_mut<'a, F, R>(&'a mut self, f: F) -> R
where
    F: 'static + for<'b> FnOnce(&'b mut <Y as Yokeable<'a>>::Output) -> R;

fn transform_mut<F, R>(&'a mut self, f: F) -> R
where
    F: 'static + for<'b> FnOnce(&'b mut Self::Output) -> R;

This would be a breaking change, mainly for anyone manually implementing Yokeable.

Notably, this lets us easily call Iterator::next on an iter in a yoke, or even implement Iterator for Yoke.

[sffc]

My initial reaction is that I think this is a beneficial change. Returning things from closures is a pain, especially closures with restrictive lifetime parameters like the ones here. Does R need to be 'static?

CC @Manishearth

[Manishearth]

My immediate reaction is worry about contravariance and implied bounds sneaking in a 'b anyway: see https://github.com/rust-lang/rust/issues/106431 for the last time this kind of thing happened. It gets pretty subtle and is hard to reason about.

I am overall in favor of this if we can be really sure that it works the way we expect it to.

[Manishearth]

R: 'static would indeed be a way to make this work without any chance of problems. Would be nice to allow self-borrows, but R: 'a is almost certainly going to get the implied bounds messiness.

[sffc]

I think R: 'static solves a lot of use cases; it allows the closure to return primitives and owned data, which it can't currently do since the closure is required to be 'static, so it can't save output variables to its context.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a9d03f0a3d04f8a6e7e212486bd66858

[Manishearth]

I'm in favor of R: 'static + a breaking change

[jdygert-spok]

Hmm, that variance issue is quite tricky. My gut says it wouldn't be a problem here since F is covariant over R, but I don't really know how to verify the soundness.

[robofinch]

The lack of this feature (plus the fact that Yoke::map_project cannot look at the cart) is why I'm swapping away from Yoke (in favor of a manual implementation).

In both cases, my goal is not impossible with Yoke's interface. To return, say, a Copy + 'static type from Yoke::with_mut, it's sufficient to add a dummy field to the yokeable data, write to that field in Yoke::with_mut, and then retrieve it with Yoke::get.

Similarly, if I want to transform one yoked Yokeable into another, but the new target requires slightly more data than what is stored in the original (necessitating access to the cart), then using Yoke::map_project could work if I add references to the cart in each of the Yokeable structs.

I don't know why Yoke::map_project is allowed to indirectly reference the cart, but does not directly provide a reference to the cart. I already have miri tests set up anyway, so Yoke was purely for ease-of-use and lessening the need to think, and "using it requires hacky workarounds with runtime costs" is enough to mean I won't use it.

[Manishearth]

@robofinch does R: 'static still work with your use case?

We're not opposed to this change, I just don't have the time to do it myself. I recommend making a PR.

I'd accept either a PR that adds map_project_with_return with the understanding that we will move over map_project in a future breaking release, or a PR that just moves over all map_project methods and makes a breaking release.

The former is slightly preferred if you are able to restrict the number of with_return APIs that need to be added.

[Manishearth]

Regarding why you can't get a reference to the cart: you can call .backing_cart() on the yoke, no?

Ah I understand now. Yeah that's rather tricky to design safe API for.

Part of it is: there actually are current soundness issues with too much access on the backing cart, see #2095. We're hoping for stuff to settle a bit upstream before trying to fix it (hopefully we get a magic wrapper type), but until then providing direct access to the cart is something we want to limit.

[Manishearth]

Could you file a separate issue for the "reborrow from cart" use case? I think it's solvable, but I don't think map_project is the way. I'd be more comfortable with a more carefully targeted API for that.

[robofinch]

Thanks for responding so quickly!

Sure, I'll open an issue for reborrowing from the cart. Concerns about UB and names of related methods can be discussed there.

I can get why changing with_mut to allow for return types is so hard to reason about. R: 'static would probably be enough for a lot of use cases.... if it's sound. There shouldn't be any way to smuggle out a cart (or yoke) reference via a 'static return type which doesn't involve unsafe, as far as I'm aware, and after reading rust-lang/rust #106431 I don't think I'd trust a shorter lifetime constraint on R.

I'll make a PR when I have the time, assuming we're still open to the breaking change on Yokeable implementors.

[Manishearth]

I'll make a PR when I have the time, assuming we're still open to the breaking change on Yokeable implementors.

@robofinch I'm open to it, but I'd still prefer doing it in a non-breaking way by adding one or two APIs if possible, with a followup issue to do it in a breaking way when we do the next breaking release. If you'd need to duplicate each one of the project APIs, yeah then we should just do the breaking change.

(I'd like to potentially have a chance to reconsider allowing for arbitrary carts, which is one way to fix #2095 without waiting for the Rust team, and that would also be breaking. I'm curious in knowing what your cart types are right now. The main risk with #2095 goes away if the set of cart types is restricted behind an unsafe trait that is implemented on Rc and Arc and some other types.)

[robofinch]

@Manishearth I've opened an issue for the other topic.

I'm using Rc, Arc...... and was using Box, which out of an abundance of caution I'm going to change to the aliasable crate or something . Reading #2095 earlier today is the first time I heard Pin<Box<T>> is currently problematic. I guess I'll need to enable -Zmiri-retag-fields. Feels like a crazy blind spot.

For this topic, I think adding return values to with_mut and transform_mut will cover most uses. (Perhaps a YokeableWithReturn trait providing transform_with_mut. And adding with_mut_return to Yoke shouldn't be breaking.)

You mentioned map_project_with_return, but I don't see a strong utility for that... maybe for fields that are being discarded by map_project, and there's some 'static data that should only be permitted to be returned shortly before the source field (borrowing from the cart) is dropped? And maybe there's utility for 'static, non-Clone data which is in the Yokeable for some reason? I can't imagine many people wouldn't be satisfied with calling get() or some sort of with_mut_return and then map_project and friends afterwards. (Come to think of it, with_mut_return would also allow 'static non-Clone data to be taken out of the Yokeable.)

[robofinch]

I should clarify that my use case has two separate problems, one with with_mut not returning something, and separately with map_project not providing a reference to the cart. Either one could be fixed with or without the other one being fixed.

[Manishearth]

Reading https://github.com/unicode-org/icu4x/issues/2095 earlier today is the first time I heard Pin<Box<T>> is currently problematic. I guess I'll need to enable -Zmiri-retag-fields. Feels like a crazy blind spot.

Note that most likely we'll have wrapper types that opt out of these behaviors.

But yeah caution is good.

[robofinch]

Do we like the names with_mut_return as a (temporary) new method on yoke::Yoke, and TransformMutReturn<'a>: Yokeable<'a> as a trait whose sole method is transform_mut_return?

On the next major version bump, the original with_mut could be replaced with with_mut_return, and Yokeable::transform_mut could be replaced with TransformMutReturn::transform_mut_return (and the temporary methods and trait would be removed).

Also worth mentioning that once the breaking change is made, the new signature of with_mut would not break a normal user. I have no doubt that a sufficiently pathological user of Yoke could find a way to make the change to with_mut break their use, but at least we can have peace of mind that the break will be mostly limited to manual implementors of Yokeable, while people using derive(Yokeable) or using Yoke will probably not need to change anything.

(To make the commit timeline simpler for myself, I intend to wait until the map_with_cart PR is merged before writing the code for this, and wait until the non-breaking changes that close this issue are merged before writing the breaking PR that'd be merged at the next major version bump. I won't handle the derive macro until the breaking PR.)

[Manishearth]

Can we do this without adding a trait and trait method? with_mut could use an Option that gets set within the closure. It should optimize away in most cases.

(Tbh yokeable doesn't need to have all of the methods it does and in the next breaking change we can probably get rid of them in favor of non trait helper functions)

[robofinch]

Ahhh, I had sort of just assumed that everything in Yokeable was needed. Looking closer... yeah, I see what you mean. I think half are wholly unnecessary, and the remaining half says "the body should be { self }, aside from additional assertions if desired". Not a whole lot of freedom for implementors, but it seems reasonable to provide implementors with the opportunity to avoid UB by panicking, so I think those methods should stay.

I don't think setting an Option (local to with_mut) within the closure passed to Yokeable::transform_mut would work, as then the closure wouldn't be 'static. We could, however, just manually do the transmute ourselves in a non-trait helper function as you mentioned, as the safety contract for implementors of Yokeable::transform_mut asserts that it must be equivalent to passing a transmute'd value to the callback. In other words, implementing Yokeable implies it's sound to not even rely on Yokeable::transform_mut to do the transmute. There's no actual need for Yokeable::make, Yokeable::transform_mut, or a hypothetical transform_mut_return. (And no need for an Option.)

I'll open an issue (and later PR) for creating non-trait helper functions, if one doesn't already exist.

After those get merged (with the intent to be stable), the temporary Yoke::with_mut_return method could be implemented.

[Manishearth]

Not a whole lot of freedom for implementors, but it seems reasonable to provide implementors with the opportunity to avoid UB by panicking, so I think those methods should stay.

Correct, though "the body should be self" also works as a way for implementors to self-safety-check, and for the custom derive to doubly ensure that things are covariant.

For that reason I'd say we should still keep .transform() at the very least.

After those get merged (with the intent to be stable), the temporary Yoke::with_mut_return method could be implemented.

You can do it beforehand with the Option trick, or with a manual transmute. It's fine, as long as we have safety comments and a link to this issue.

With the Option trick you don't even need unsafe, it can wrap with_mut.