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

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...

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

r? types

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.

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.

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.

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

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
}

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

Please add this as a test here.

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

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.

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?

I personally feel fine with the issue described in the above comment. I cannot imagine a user writing

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

while relying on some "does not shorten lifetimes" of that CoerceUnsized impl. Being able to shorten lifetimes this way is generally desirable. One unsound thing would be a type whose drop impl writes its only field to some side-table. That way you can own a value which still has to be treated as shared.

We should add a comment to CoerceUnsized explaining how there are impls which shorten lifetimes, so if you implement CoerceUnsized for a type which has shared ownership of T, it would be unsound.

This ties into there being a difference MutableAccess and MutableSharedAccess wrt how variance should be computed, which I've also alluded to in https://rust-lang.zulipchat.com/#narrow/channel/326866-t-types.2Fnominated/topic/.23149219.3A.20Allow.20shortening.20lifetime.20in.20CoerceUnsized.20for.20.26mut/near/582743558

I've not looked very closely at this but I'm happy to just trust y'all here so gonna tick my box

🔔 This is now entering its final comment period, as per the review above. 🔔

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

I've added a test.