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 don't quite understand the errors here...

[zachs18]

IIUC, the errors are because when implementing CoerceUnsized for ADTs, the non-coerced fields (i.e. the fields other than the pointer field) must be exactly the same type between the source and destination instantiations in the current implementation.

https://github.com/rust-lang/rust/blob/94b49fd998d6723e0a9240a7cff5f9df37b84dd8/compiler/rustc_hir_analysis/src/coherence/builtin.rs#L485-502 (the error is produced on line 557, lines 485-492 describe the check)

Since BorrowRefMut<'a> is not exactly the same type as BorrowRefMut<'b> (as per Ty::eq), this isn't allowed. The comment starting on line 552 (just before the equality check) is also probably relevant

rust/compiler/rustc_hir_analysis/src/coherence/builtin.rs

Lines 522 to 533 in 94b49fd

	// Ignore fields that aren't changed; it may
	// be that we could get away with subtyping or
	// something more accepting, but we use
	// equality because we want to be able to
	// perform this check without computing
	// variance or constraining opaque types' hidden types.
	// (This is because we may have to evaluate constraint
	// expressions in the course of execution.)
	// See e.g., #41936.
	if a == b {
	return None;
	}

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

[theemathas]

I already tried and couldn't find a case where there's user-observable behavior change, other than directly writing a trait bound on CoerceUnsized or something similar.

I wanted to make this change in order to make the & -> & and &mut -> & and &mut -> &mut impls consistent with each other, as a matter of principle. Although I don't have a particular opinion on whether the lifetime-shortening should be allowed in all three impls or none of the three impls.

If we do stabilize CoerceUnsized at some point, an inconsistency between the three impls might result in weird borrow-check errors that are hard to explain to a user who's trying to be generic over types that implement CoerceUnsized.

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

@jackh726 There seems to be no user-observable effect due to this change in our test suite.

I tried removing the lifetime shortening on the & -> & and &mut -> & impls.

Diff of changes

diff --git a/library/core/src/ops/unsize.rs b/library/core/src/ops/unsize.rs
index f0781ee01fd..e39dded51d4 100644
--- a/library/core/src/ops/unsize.rs
+++ b/library/core/src/ops/unsize.rs
@@ -42,7 +42,7 @@ pub trait CoerceUnsized<T: PointeeSized> {
 impl<'a, T: PointeeSized + Unsize<U>, U: PointeeSized> CoerceUnsized<&'a mut U> for &'a mut T {}
 // &mut T -> &U
 #[unstable(feature = "coerce_unsized", issue = "18598")]
-impl<'a, 'b: 'a, T: PointeeSized + Unsize<U>, U: PointeeSized> CoerceUnsized<&'a U> for &'b mut T {}
+impl<'a, T: PointeeSized + Unsize<U>, U: PointeeSized> CoerceUnsized<&'a U> for &'a mut T {}
 // &mut T -> *mut U
 #[unstable(feature = "coerce_unsized", issue = "18598")]
 impl<'a, T: PointeeSized + Unsize<U>, U: PointeeSized> CoerceUnsized<*mut U> for &'a mut T {}
@@ -52,7 +52,7 @@ impl<'a, T: PointeeSized + Unsize<U>, U: PointeeSized> CoerceUnsized<*const U> f

 // &T -> &U
 #[unstable(feature = "coerce_unsized", issue = "18598")]
-impl<'a, 'b: 'a, T: PointeeSized + Unsize<U>, U: PointeeSized> CoerceUnsized<&'a U> for &'b T {}
+impl<'a, T: PointeeSized + Unsize<U>, U: PointeeSized> CoerceUnsized<&'a U> for &'a T {}
 // &T -> *const U
 #[unstable(feature = "coerce_unsized", issue = "18598")]
 impl<'a, T: PointeeSized + Unsize<U>, U: PointeeSized> CoerceUnsized<*const U> for &'a T {}

The only test failure was in tests/ui/error-codes/E0476.rs. And it's only a diagnostics change due to rustc outputting the contents of the impl in question in the diagnostics.

Diff of change in test output

diff --git a/tests/ui/error-codes/E0476.next.stderr b/tests/ui/error-codes/E0476.next.stderr
index 454dbecc7d0..c8fd40d144c 100644
--- a/tests/ui/error-codes/E0476.next.stderr
+++ b/tests/ui/error-codes/E0476.next.stderr
@@ -5,8 +5,8 @@ LL | impl<'a, 'b, T, S> CoerceUnsized<&'a Wrapper<T>> for &'b Wrapper<S> where S
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: conflicting implementation in crate `core`:
-           - impl<'a, 'b, T, U> CoerceUnsized<&'a U> for &'b T
-             where 'b: 'a, T: Unsize<U>, T: ?Sized, U: ?Sized;
+           - impl<'a, T, U> CoerceUnsized<&'a U> for &'a T
+             where T: Unsize<U>, T: ?Sized, U: ?Sized;

 error[E0476]: lifetime of the source pointer does not outlive lifetime bound of the object type
   --> $DIR/E0476.rs:11:1
diff --git a/tests/ui/error-codes/E0476.old.stderr b/tests/ui/error-codes/E0476.old.stderr
index 454dbecc7d0..c8fd40d144c 100644
--- a/tests/ui/error-codes/E0476.old.stderr
+++ b/tests/ui/error-codes/E0476.old.stderr
@@ -5,8 +5,8 @@ LL | impl<'a, 'b, T, S> CoerceUnsized<&'a Wrapper<T>> for &'b Wrapper<S> where S
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: conflicting implementation in crate `core`:
-           - impl<'a, 'b, T, U> CoerceUnsized<&'a U> for &'b T
-             where 'b: 'a, T: Unsize<U>, T: ?Sized, U: ?Sized;
+           - impl<'a, T, U> CoerceUnsized<&'a U> for &'a T
+             where T: Unsize<U>, T: ?Sized, U: ?Sized;

 error[E0476]: lifetime of the source pointer does not outlive lifetime bound of the object type
   --> $DIR/E0476.rs:11:1

[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