[nll] better error message when returning refs to upvars

[pnkfelix]

In an example like:

rust/src/test/ui/issue-40510-1.rs

Lines 14 to 20 in 59fa6bd

	fn f() {
	let mut x: Box<()> = Box::new(());
	|| {
	&mut x
	};
	}

AST-borrowck accepts such code, but NLL will start rejecting it.

@nikomatsakis concluded in a comment back in April (#49824 (comment)) that similar code (or at least it looks similar to me) is in fact incorrect, and decided that NLL was correct in rejecting such cases.

This issue is just marking that this change in semantics has occurred, so that I can link to it from places like #52663

Known instances:

• NLL: unexpected "free region `` does not outlive" error #49824 (comment)
• https://github.com/rust-lang/rust/blob/59fa6bd6c14eebd213884da08a33639b4c848bb3/src/test/ui/issue-40510-1.rs
• https://github.com/rust-lang/rust/blob/59fa6bd6c14eebd213884da08a33639b4c848bb3/src/test/ui/issue-40510-3.rs

[pnkfelix]

tagging this with T-lang because it is one of the first cases on #52663 that I think they may want to dive into with @nikomatsakis

[cramertj]

It seems correct to reject this code as an FnMut, but it seems like it would be fine as an FnOnce that hands out a reference with a fixed lifetime.

I hit a similar issue with the current implementation of async closures in that they don't ever allow mutation of upvars because the returned closure-as-a-future can't be tied to the lifetime of the call to the closure. It would seem like mutation would be fine so long as the outer closure is FnOnce, but the compiler doesn't accept it.

[pnkfelix]

Its also important to note that @nikomatsakis 's comment (#49824 (comment)) was for a slightly different scenario, namely this (#49824 (comment))

fn main() {
    let mut x = 0;
    || {
        || {
            let _y = &mut x;
        }
    };
}

I'd need to review the list of instances here more carefully to determine whether every case that we hit has the same sort of issue (where if the user adds move to force it to be FnOnce, then things work).

At the very least we could try to tell them to do that as a hint.

[matthewjasper]

Note that AST borrowck rejects this if you try to call the closure.

[pnkfelix]

I'm not going to tag this with a particular NLL- triage label at the moment; I suspect @nikomatsakis will want to do so at the next WG-compiler-nll meeting.

I suspect the tag should be one of:

• NLL-diagnostics, in that we could improve the feedback to the user to advise them to add a move to their lambda expression, or
• NLL-complete, as in code we used to accept and should continue to accept via some means (be it "smarter" inference of FnOnce, or ... something else ... )

[nikomatsakis]

OK, so, I've been seeing a number of quasi-duplicates of this (e.g., #51354). It took me a bit to understand what was going on so I'm going to copy my comment from that issue here for reference:

I think NLL is correct to reject, but the reason is subtle -- in short, we inferred this closure to be a FnMut closure, but as such, it only has "temporary" mutable access to its captured variables (in this case, v).

That is, it allowed to mutate them, but only during the fn body -- but here we are returning a mutable reference, so we are basically enabling mutation even after the function has returned.

That is not permitted by the signature of FnMut: it would mean that you couldn't call the fn again until you had finished with the return value.

If you use move here, things will build, because then we are FnOnce. You might wonder why the compiler can't infer it -- the answer is that we don't know the lifetime relationships we would need to know until later.

It seems to me then that this is indeed an NLL-diagnostics issue (and, to some extent, NLL-fixed-by-NLL). NLL is correctly rejecting these programs. This is a case where our existing closure inference (which does not take region relationships into account) is not "equipped" to handle that these closures are FnOnce.

I am interested in potentially finding ways to improve the inference but that feels like an orthogonal job from NLL and one that we should do via a distinct RFC.

[nikomatsakis]

Moving to Edition RC 2.

[nikomatsakis]

So this example (adapted from #51354):

#![feature(nll)]

fn main() {
    let mut v: Vec<()> = Vec::new();
    || &mut v;
}

Currently gives this error:

error: unsatisfied lifetime constraints
 --> src/main.rs:5:8
  |
5 |     || &mut v;
  |     -- ^^^^^^ returning this value requires that `'1` must outlive `'2`
  |     ||
  |     |return type of closure is &'2 mut std::vec::Vec<()>
  |     lifetime `'1` represents this closure's body
  |
  = note: closure implements `FnMut`, so references to captured variables can't escape the closure

This is "ok" but not great. The note is in the right direction: the key problem here is that this is a FnMut closure, so a reference to a captured variable (v) cannot escape. But I think this should be promoted beyond note, and maybe we can also offer some tips to help the user? Unclear.

[nikomatsakis]

I think I would like an error like:

error: captured variable cannot escape `FnMut` closure body
 --> src/main.rs:5:8
  |
5 |     || &mut v;
  |     -- ^^^^^^ creates a reference to the captured variable `v` which escapes the closure body
  |     |
  |     inferred to be a `FnMut` closure
  |
  = note: `FnMut` closures only have access to their captured variables while they are executing
  = note: therefore, they cannot return allow a reference to a captured variable to escape

Maybe moving those last two notes to an "extended error" description?

I'm not sure if/when to offer move tips, as changing to a move closure doesn't always help (it won't help here, for example).

[pnkfelix]

Indeed the captured &mut Vec<()> example is an important counter-example to the thoughts I was having about move.

That leads me to wonder whether we should be suggesting some kind of explicit type ascription in order to force it to be an FnOnce, like in this example (play):

fn id<'a>(f: impl FnOnce() -> &'a mut Vec<()>) -> &'a mut Vec<()> {
    f()
}

fn foo() {
    let mut v: Vec<()> = Vec::new();
    id(|| &mut v).push(());
    assert_eq!(v.len(), 1);
}