Borrow checker on beta rejects code accepted on stable.

[khuey]

See the repo at https://github.com/khuey/rust-nightly-borrow-checker-failure

This repository builds on 1.34.2. It fails on 1.35.0-beta.8 with

error[E0597]: `global_lock` does not live long enough
  --> src/lib.rs:24:26
   |
24 |             let locked = global_lock.as_ref().lock().unwrap();
   |                          ^^^^^^^^^^^ borrowed value does not live long enough
25 |             ptr::write(&mut self.global_lock, Some(locked));
   |                        --------------------- cast requires that `global_lock` is borrowed for `'static`
26 |         }
27 |     }
   |     - `global_lock` dropped here while still borrowed

error: aborting due to previous error

[Manishearth]

cc @nikomatsakis

[Manishearth]

We're cutting off a new stable next week, we should prioritize investigating and fixing this.

[Centril]

Maximally reduced (playpen):

pub fn bar<'a>(this: &'a mut ((), &'static ())) {
    let beta = from(&this.0);
    let locked = as_ref(&beta);
    write(&mut this.1, locked);
}

fn write<T>(_: *mut T, _: T) {}
fn from<'a>(_: &'a ()) -> &'static () { panic!() }
fn as_ref<'a>(_: &'a &'static ()) -> &'a () { panic!() }

[matthewjasper]

Probably #58673, which fixed pointer casts being able to launder lifetimes with NLL.

[Manishearth]

@matthewjasper any idea on how we'd be able to fix this in time for the release? Either by backporting or by fixing the actual bug?

[matthewjasper]

The example provided should be using let locked = (*global_lock.as_ptr()).lock().unwrap();.

[khuey]

Yeah that works.

[pnkfelix]

Hmm. Was I wrong about PR #58673 only affecting code opting in via #![feature(nll)]? I had thought when I reviewed that PR that it did not affect NLL migrate mode, and therefore could not be breaking stable/beta ...

Update: Ah! I figured out why NLL migrate mode does not "save" us here: The NLL error being signaled would only be downgraded to a warning if the AST-borrowck accepted the code. But the AST-borrowck rejects this code (or at least, it rejects the minimized example provided by @Centril , as you can see on this playpen). And therefore, we have a case where NLL previously accepted some code (but AST-borrowck rejected it), and now with PR #58673 in place, NLL rejects it with a hard error (because the migrate mode, upon inspecting the behavior of AST-borrowck, does not do the downgrade-to-warning).

[pietroalbini]

Me and @pnkfelix discussed this on Discord. The beta to stable promotion is happening today so there is no time for the compiler team to decide what's the best fix for this issue, and we don't want to rush things.

I'm going to back out PR #58673 from the 1.35.0 release, but it will still be present on beta 1.36.0 and nightly, giving the teams more breathing room to reach a decision.

[pietroalbini]

An update: backing out the change doesn't seem to be trivial so I'm going to do the initial stable PR without it (aka with the regression).

When I tried doing the revert I got a bunch of ICEs both in the test suite and while compiling stage1 rustc, so the backout seems risky enough not to do it right before 1.35.0 goes out.

cc @rust-lang/release

[Manishearth]

Can we mention this in the release blog post so people are warned?

[Centril]

Assigning myself as a reminder re. blog post (but not for anything else).

[pnkfelix]

triage: I don't think we're going to try to "fix" the regression here. From the investigation documented above, it seems like acceptable fallout from a soundness fix; a warning-cycle would have been nice, but its not a necessity, and its not going to block the release.

Removing I-nominated tag, and not going to add a priority label. Leaving assigned to @Centril regarding messaging.

[Centril]

Decided not to include in blogpost or relnotes. Closing therefore.