error[E0713] unexpectedly disappeared between 1.52 and 1.53

[Tamschi]

Code

I tried this code:

#[must_use]
pub fn make_mut(this: &mut Pin<Self>) -> Pin<&mut T>
where
	T: Sized + Clone,
{
	match this.tip_toe().acquire() {
		AcquireOutcome::Exclusive => (),
		AcquireOutcome::Shared => *this = (&**this).clone().pipe(Self::pin),
	}
	unsafe {
		Pin::new_unchecked(
			//FAULTY!
			mem::transmute_copy::<Pin<Self>, Self>(this)
				.pointer
				.as_mut(),
		)
	}
}

(https://github.com/Tamschi/tiptoe/blob/11e8b4fc7d8e012273df15eb6429999ef98e9682/src/sync.rs#L336-L352)

I expected to see this happen:

The code above is wrong, as it implicitly decrements the Arc's counter. (Self is a custom Arc.)

On Rust 1.52 and earlier, I get the following very helpful error here:

tiptoe on  HEAD (11e8b4f) is 📦 v0.0.1 via 🦀 v1.55.0 
❯ cargo +1.52 check --all-features
    Checking tiptoe v0.0.1 (C:\…\tiptoe)
error[E0713]: borrow may still be in use when destructor runs
   --> src\sync.rs:347:5
    |
337 |        pub fn make_mut(this: &mut Pin<Self>) -> Pin<&mut T>
    |                              - let's call the lifetime of this reference `'1`
...
346 |  /             Pin::new_unchecked(
347 |  |                 mem::transmute_copy::<Pin<Self>, Self>(this)
    |  |_________________^
348 | ||                     .pointer
    | ||____________________________^
349 |  |                     .as_mut(),
350 |  |             )
    |  |_____________- returning this value requires that borrow lasts for `'1`
351 |            }
352 |        }
    |        - here is drop of temporary value; whose type `sync::Arc<T>` implements the `Drop` trait

Instead, this happened:

Starting with Rust 1.53, I get no error:

❯ cargo +1.53 check --all-features
    Checking tiptoe v0.0.1 (C:\…\tiptoe)
    Finished dev [unoptimized + debuginfo] target(s) in 0.85s

Version it worked on

It most recently worked on:

rustc +1.52 --version --verbose

rustc 1.52.1 (9bc8c42bb 2021-05-09)
binary: rustc
commit-hash: 9bc8c42bb2f19e745a63f3445f1ac248fb015e53
commit-date: 2021-05-09
host: x86_64-pc-windows-msvc
release: 1.52.1
LLVM version: 12.0.0

Version with regression

rustc +1.53 --version --verbose:

rustc 1.53.0 (53cb7b09b 2021-06-17)
binary: rustc
commit-hash: 53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b
commit-date: 2021-06-17
host: x86_64-pc-windows-msvc
release: 1.53.0
LLVM version: 12.0.1

Backtrace

Not applicable.

I couldn't find anything related in the release notes, so I assume this is right:
@rustbot modify labels: +regression-from-stable-to-stable -regression-untriaged

[nbdd0121]

@rustbot label +E-needs-mcve

[Tamschi]

So nobody does duplicate work: I should have the example in a few minutes, down to about 15 lines right now.

(Also in my defence, I wrote my buggy code at something like 3AM, I should be more careful there 😴)

[Tamschi]

use std::ptr::NonNull;

#[derive(Clone)]
pub struct A(NonNull<()>);

impl Drop for A {
    fn drop(&mut self) {
        unimplemented!()
    }
}

impl A {
    pub fn bug(&self) -> &() {
        unsafe { self.clone().0.as_ref() }
    }
}

This (in standard lib.rs) shows the aforementioned behaviour.

It does (correctly) fail to compile in newer versions if the field is a plain () and I just take the reference with & instead of calling .as_ref() on it, with the same error number.

[nbdd0121]

Bisection gives #80771, so it seems as an intentional change. No sure why it's not in the release note though.

@rustbot label -E-needs-mcve -regression-from-stable-to-stable

[Tamschi]

I see, seems like I hit the odd corner case where it would have been a helpful heuristic,
but I can see the argument for permitting it in terms of no pointer handholding in general.

(I personally would prefer a small speed bump here, but I'm unsure what that would look like without getting in the way elsewhere.)

[apiraino]

Looks like then this should be added to a changelog or somewhere to let people know.
Zulip thread of the Prioritization Working Group.

@rust-lang/release perhaps ?

@rustbot label -I-prioritize +T-release

[Mark-Simulacrum]

Cc @thomcc @rust-lang/libs-api since at least my skin of the PR which introduced this suggests we didn't expect changes like this (which could be viewed as code that used to be "sound" since it didn't compile and now does). #80771

But I expect we probably don't want to do anything here - this feels a little similar to the known gap in lints around CString::as_ptr (lack of) lifetime. So just a heads up in case anyone has particular thoughts.

I'm not sure that anything in the release notes would be all that meaningful here, but if there's a PR posted amending those we can probably merge it.