Allow shortening lifetime in CoerceUnsized for &mut

[theemathas]

View all comments

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.

This change has an effect on stable code, since it allows shortening lifetimes inside invariant types via a &mut -> &mut unsize coercion.

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

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[theemathas]

@rustbot ready

[theemathas]

@jackh726 This PR is still waiting for approval.

[theemathas]

@rustbot reroll

I've rewritten the test.

[camelid]

r? @jackh726

Re-assigning Jack since he has context for this and I do not.

[jackh726]

@bors r+ rollup

[rust-bors]

📋 This PR cannot be approved because it currently has the following label: needs-fcp.

[theemathas]

@bors r=jackh726 rollup

[rust-bors]

📌 Commit 94c4323 has been approved by jackh726

It is now in the queue for this repository.