NLL migrate mode erroneously downgrades error with nested closures, yielding use-after-free

[RalfJung]

The following code compiles with the 2018 edition on stable, beta and nightly, and produces this error:

Execution operation failed: Output was not valid UTF-8: invalid utf-8 sequence of 1 bytes from index 1813

Code:

use std::marker::PhantomData;

struct Scope<'env> {
    invariant: PhantomData<fn(&'env ()) -> &'env ()>
}

fn scope<'env, F, T>(f: F) -> T
where
    F:  FnOnce(&Scope<'env>) -> T
{
    f(&Scope { invariant: PhantomData })
}

impl<'env> Scope<'env> {
    fn spawn<'scope, F, T>(&'scope self, f: F) -> T
    where
        F: FnOnce() -> T + Send + 'env,
        T: Send + 'env
    {
        f()
    }
}

fn main() {
    let mut greeting = "Hello world!".to_string();
    let res = scope(|s| s.spawn(|| &greeting));

    greeting = "DEALLOCATED".to_string();
    drop(greeting);

    println!("thread result: {:?}", res);
}

And indeed there's a use-after-free here.

Interestingly, in the 2015 edition this is correctly detected as incorrect, though the error is not quite what I would have expected:

   Compiling playground v0.0.1 (/playground)
error[E0597]: `*greeting` does not live long enough
  --> src/main.rs:26:37
   |
26 |     let res = scope(|s| s.spawn(|| &greeting));
   |                                 --  ^^^^^^^^ - borrowed value only lives until here
   |                                 |   |
   |                                 |   borrowed value does not live long enough
   |                                 capture occurs here
...
32 | }
   | - borrowed value needs to live until here

I would have expected a complaint at the re-assignment of greeting.

(I found this while modifying an example by @stjepang and failing to reproduce some code that gets a migration warning on 2018...)

[tesuji]

In stable (clean build):

Standard Error

   Compiling playground v0.0.1 (/playground)
warning[E0506]: cannot assign to `greeting` because it is borrowed
  --> src/main.rs:28:5
   |
26 |     let res = scope(|s| s.spawn(|| &greeting));
   |                     ---             -------- borrow occurs due to use in closure
   |                     |
   |                     borrow of `greeting` occurs here
27 | 
28 |     greeting = "DEALLOCATED".to_string();
   |     ^^^^^^^^ assignment to borrowed `greeting` occurs here
...
31 |     println!("thread result: {:?}", res);
   |                                     --- borrow later used here
   |
   = warning: This error has been downgraded to a warning for backwards compatibility with previous releases.
           It represents potential unsoundness in your code.
           This warning will become a hard error in the future.

warning[E0505]: cannot move out of `greeting` because it is borrowed
  --> src/main.rs:29:10
   |
26 |     let res = scope(|s| s.spawn(|| &greeting));
   |                     ---             -------- borrow occurs due to use in closure
   |                     |
   |                     borrow of `greeting` occurs here
...
29 |     drop(greeting);
   |          ^^^^^^^^ move out of `greeting` occurs here
30 | 
31 |     println!("thread result: {:?}", res);
   |                                     --- borrow later used here
   |
   = warning: This error has been downgraded to a warning for backwards compatibility with previous releases.
           It represents potential unsoundness in your code.
           This warning will become a hard error in the future.

    Finished release [optimized] target(s) in 1.13s
     Running `target/release/playground`

Standard Output

thread result: "h\u{10}0\u{5}5V\u{0}\u{0}\u{0}\u{0}\u{0}"

[matthewjasper]

So the playground doesn't show this if you run the code, but we are emitting warnings. Looks like the issue is with the migrate mode.

[hellow554]

warning: This error has been downgraded to a warning for backwards compatibility with previous releases.
    It represents potential unsoundness in your code.
    This warning will become a hard error in the future.

So the solution would be to make it an hard error now? ;)

[pnkfelix]

cc #55492

[pnkfelix]

triage: P-high. Leaving assigned to @matthewjasper until they complain. :)

[RalfJung]

As @lqd pointed out on Zulip, the code actually gets rejected properly when adding #![feature(nll)].

[lqd]

Yeah, this is why Felix changed the title. This problem is not in MIR borrowck but in the "migrate mode" itself: the piece of code responsible for checking if an NLL error would have been missed by AST borrowck, and downgrading it to a warning (to make the AST borrowck bug fixes not break existing code during a migration period).

Here, the 2 borrowck's errors are different enough that migrate mode misses that they are related and shouldn't downgrade them.