Allow shortening lifetime in CoerceUnsized for &mut

[theemathas]

This modifies the &mut -> &mut CoerceUnsized impl so that, it can shorten the lifetime.

Note that there are already two impls that allow shortening the lifetime like this (the &mut T -> &U and the &T -> &U impls). So this change makes the impls consistent with each other.

I initially tried to also do the same to the CoerceUnsized impl for core::cell::{Ref, RefMut}. However, this can't be done because Ref and RefMut "store" the lifetime and the data in different fields, and CoerceUnsized can only coerce one field.

I don't know if there is a visible effect in stable code or not.

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[theemathas]

I changed the PR to just edit the impl for &mut, and leave the impls for Ref and RefMut alone. I don't know if this is the right way to go though...

[scottmcm]

TBH, I feel quite unqualified to review the soundness of this. Maybe someone from types feels more confident?

[scottmcm]

r? types

[jackh726]

So, there should be some test that we can add to observe this behavior change (stable or unstable). Without it, we definitely shouldn't be making it.

I can think more about this and try to come up with such a test later this week, but @theemathas perhaps you can try before I get to it, since you're proposing the change there must be some reason.

[jackh726]

One thing that you could try is to remove the lifetime shortening on the other impl(s) and see if you can find tests that require that - and use that as a starting point for a similar test.

[theemathas]

I just realized: This change actually does have a visible effect in stable rust.

Consider the following code:

use std::cell::Cell;

struct Thing;
trait Trait {}
impl Trait for Thing {}

fn works<'a: 'b, 'b>(x: Cell<&'a Thing>) -> Cell<&'b dyn Trait> {
    x
}

fn fails<'a: 'b, 'b>(x: Cell<&'a mut Thing>) -> Cell<&'b mut dyn Trait> {
    x
}

Currently, the works function compiles, and the fails function does not compile.

error: lifetime may not live long enough
  --> src/lib.rs:12:5
   |
11 | fn fails<'a: 'b, 'b>(x: Cell<&'a mut Thing>) -> Cell<&'b mut dyn Trait> {
   |          --      -- lifetime `'b` defined here
   |          |
   |          lifetime `'a` defined here
12 |     x
   |     ^ function was supposed to return data with lifetime `'a` but it is returning data with lifetime `'b`
   |
   = help: consider adding the following bound: `'b: 'a`
   = note: requirement occurs because of the type `Cell<&mut dyn Trait>`, which makes the generic argument `&mut dyn Trait` invariant
   = note: the struct `Cell<T>` is invariant over the parameter `T`
   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance

This PR makes it so that both functions compile instead.

I am now unsure on what the best way forward is.

[theemathas]

The inconsistent lifetimes in the impls were there since ancient times 843db01#diff-3da87c1e923c79167e692c9c84af74599a13c74a83e797b131aff23470a47411R1223-R1233

[theemathas]

Even "no-op" unsize-coercions have strange behavior:

use std::cell::Cell;
struct Thing;
trait Trait {}

// currently errors
fn with_thing<'a: 'b, 'b>(x: Cell<&'a Thing>) -> Cell<&'b Thing> {
    x
}

// currently compiles
fn with_trait<'a: 'b, 'b>(x: Cell<&'a dyn Trait>) -> Cell<&'b dyn Trait> {
    x
}

// currently errors, will compile with this PR
fn with_trait_mut<'a: 'b, 'b>(x: Cell<&'a mut dyn Trait>) -> Cell<&'b mut dyn Trait> {
    x
}

[jackh726]

I just realized: This change actually does have a visible effect in stable rust.

Please add this as a test here.

[jackh726]

Okay, discussion on Zulip hasn't raised any issues here. Going to fcp merge for types here. I'm going to ping @rust-lang/libs-api, but I think this is probably "just okay" to have as a types team PR. This isn't really a new "API surface" as much as a change to what coercions we accept.

---

This impl change allows (as an example) the following to compile:

// currently compiles
fn with_trait<'a: 'b, 'b>(x: Cell<&'a dyn Trait>) -> Cell<&'b dyn Trait> {
    x
}

// currently errors, will compile with this PR
fn with_trait_mut<'a: 'b, 'b>(x: Cell<&'a mut dyn Trait>) -> Cell<&'b mut dyn Trait> {
    x
}

@rfcbot merge types

[rust-rfcbot]

Team member @jackh726 has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @jackh726
• @lcnr
• @oli-obk
• @spastorino

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[theemathas]

I'm not sure whether this will cause problems with user-defined CoerceUnsized impls in the future (once we stabilize that). For example:

#![feature(coerce_unsized)]

use std::ops::CoerceUnsized;
use std::marker::PhantomData;

pub struct Inv<P>(PhantomData<fn(P) -> P>, P);

impl<T: CoerceUnsized<U>, U> CoerceUnsized<Inv<U>> for Inv<T> {}

pub trait Trait {}

// compiles
pub fn works<'a>(x: Inv<&'static dyn Trait>) -> Inv<&'a dyn Trait> {
    x
}

// currently errors, will compile after this PR
pub fn doesnt_work_yet<'a>(x: Inv<&'static mut dyn Trait>) -> Inv<&'a mut dyn Trait> {
    x
}

// errors
pub fn fails<'a>(x: Inv<&'static i32>) -> Inv<&'a i32> {
    x
}

In other words, allowing this shortening of lifetimes may potentially change the safety requirements / reasoning needed for one to implement the CoerceUnsized trait in the future. This is because allowing more coercions means that implementors of CoerceUnsized can assume fewer things as invariants.

I am unable to figure out a nice way to state what invariants a CoerceUnsized impl may break. (In other words, I'm unsure what safety invariants a type could impose on its private fields if the type implements CoerceUnsized.)

As a result, I am unable to reason whether this PR is a good idea or a bad idea. (I don't have concrete concerns. I just find it hard if not impossible to give a proper justification on whether this PR is doing the right thing.)

An alternative to this PR is to go the other way: disallow lifetime-shortening coercions on shared references, to make them consistent with exclusive references.

Does the interaction with future implementors of CoerceUnsized make this PR require a decision from T-libs-api?