NLL diagnostics replaced nice closure errors w/ indecipherable free region errors

[pnkfelix]

The following tests all have a common weakness around how they report errors under NLL. Under AST borrowck they would say something somewhat understandable about closures. But under NLL they don't anymore.

List:

• borrowck/issue-45983.rs
• borrowck/issue-7573.rs
• borrowck/regions-escape-bound-fn-2.rs
• borrowck/regions-escape-bound-fn.rs
• borrowck/regions-escape-unboxed-closure.rs
• closure-expected-type/expect-region-supply-region.rs
• error-codes/E0621-does-not-trigger-for-closures.rs

(This list of tests is drawn from an informal paper document that I have been using to keep notes for myself as I work on this...)

Example (from ui/borrowck/issue-45983.rs):

AST borrowck says:

error: borrowed data cannot be stored outside of its closure
  --> $DIR/issue-45983.rs:17:27
   |
LL |     let x = None;
   |         - borrowed data cannot be stored into here...
LL |     give_any(|y| x = Some(y));
   |              ---          ^ cannot be stored outside of its closure
   |              |
| ...because it cannot outlive this closure

NLL says:

error: free region `` does not outlive free region `'_#2r`
  --> $DIR/issue-45983.rs:17:27
   |
LL |     give_any(|y| x = Some(y));
   |                           ^

[pnkfelix]

there is also the broader issue #49397; this one is more targeted and thus separate.

[KiChjang]

As @nikomatsakis has mentioned in the NLL triage docs, I was indeed looking at these errors, so I'm assigning this to myself.

[nikomatsakis]

I'm unassigning @KiChjang as I don't think they made any progress (ping me on Zulip @KiChjang if you still want to hack on this). I think that this is highly related to what I wrote on #51168 (that is, in this comment), and that the solution I described there would be good here too. So I'm going to mark this as a sub-issue and hence deferred.

[nikomatsakis]

Actualy, I'll make this the primary issue, since it has an amusing title. Subissues:

• NLL: missing note that "borrowed value needs to live until here" (sometimes?) #51168

[nikomatsakis]

The reason for this is indeed the closure, I think. We detect the error when analyzing the closure body, so it is hard for us to report a span that is outside the closure. I think this will be pretty tricky to fix and is maybe not that important. What we could probably do is take a different tactic: we could explain that x is owned by the closure and yet we are returning a reference to it (just as we would a top-level function, perhaps).

I'm imagining:

error[E0597]: `x` is owned by the closure, so you cannot return a reference to it
  --> $DIR/issue-11925.rs:18:35
   |
LL |         let f = to_fn_once(move|| &x); //~ ERROR does not live long enough
   |                                   ^
   |                                   |
   |                                   |
| cannot return reference to data owned by the closure

for bonus points, we can highlight the move keyword and/or the use of x that caused it to a move in the first place (we have that data available somewhere).

[nikomatsakis]

OK, I've been digging a bit into the code around this issue and I have some (incomplete) thoughts. I'd like to cc @davidtwco as the work on #51188 and this are definitely entangled.

I'm looking specifically at borrowck/issue-45983.rs, which currently (under AST borrow check) gives this error:

error: borrowed data cannot be stored outside of its closure
  --> issue-45983.rs:17:27
   |
16 |     let x = None;
   |         - borrowed data cannot be stored into here...
17 |     give_any(|y| x = Some(y));
   |              ---          ^ cannot be stored outside of its closure
   |              |
   |              ...because it cannot outlive this closure

Under MIR borrowck we get an unfortunate error like this:

error: free region `` does not outlive free region `'_#2r`
  --> issue-45983.rs:17:27
   |
17 |     give_any(|y| x = Some(y));
   |                           ^

We'd like to issue some kind of error more tailored to the fact that we are in a closure. But we'd also like it to fit into a broader spectrum of "region reporting errors".

Some background

The first thing to talk about are NLL "universal regions". A universal region (short for "universally quantified" region) is basically a lifetime parameter (we use the term region and lifetime interchangeably). So 'static is a universal region, as is 'a in this example:

fn foo<'a>(x: &'a u32) { /* 'a here is a universal region */ }

Universal regions fall into a few categories, described here:

rust/src/librustc_mir/borrow_check/nll/universal_regions.rs

Lines 159 to 199 in b58b721

	#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
	pub enum RegionClassification {
	/// A **global** region is one that can be named from
	/// anywhere. There is only one, `'static`.
	Global,
	/// An **external** region is only relevant for closures. In that
	/// case, it refers to regions that are free in the closure type
	/// -- basically, something bound in the surrounding context.
	///
	/// Consider this example:
	///
	/// ```
	/// fn foo<'a, 'b>(a: &'a u32, b: &'b u32, c: &'static u32) {
	/// let closure = for<'x> |x: &'x u32| { .. };
	/// ^^^^^^^ pretend this were legal syntax
	/// for declaring a late-bound region in
	/// a closure signature
	/// }
	/// ```
	///
	/// Here, the lifetimes `'a` and `'b` would be **external** to the
	/// closure.
	///
	/// If we are not analyzing a closure, there are no external
	/// lifetimes.
	External,
	/// A **local** lifetime is one about which we know the full set
	/// of relevant constraints (that is, relationships to other named
	/// regions). For a closure, this includes any region bound in
	/// the closure's signature. For a fn item, this includes all
	/// regions other than global ones.
	///
	/// Continuing with the example from `External`, if we were
	/// analyzing the closure, then `'x` would be local (and `'a` and
	/// `'b` are external). If we are analyzing the function item
	/// `foo`, then `'a` and `'b` are local (and `'x` is not in
	/// scope).
	Local,
	}

All region errors in NLL fall into the category of two universal regions that the code requires to have an outlives relationship, but which do not. So for example:

fn foo<'a, 'b>(x: &'a u32) -> &'b u32 { x }

this code would error because it would require that 'a: 'b (which is not true).

Where error reporting takes place

Error reporting takes place in this function:

rust/src/librustc_mir/borrow_check/nll/region_infer/error_reporting.rs

Lines 193 to 201 in b58b721

	/// Report an error because the universal region `fr` was required to outlive
	/// `outlived_fr` but it is not known to do so. For example:
	///
	/// ```
	/// fn foo<'a, 'b>(x: &'a u32) -> &'b u32 { x }
	/// ```
	///
	/// Here we would be invoked with `fr = 'a` and `outlived_fr = `'b`.
	pub(super) fn report_error(

This code makes some effort to examine the constraints and to identify "interesting" ones that are likely to be the source of the error. This system is very much still Under Development. Nonetheless, that is what this chunk of code is doing:

rust/src/librustc_mir/borrow_check/nll/region_infer/error_reporting.rs

Lines 233 to 243 in b58b721

	let constraints = self.find_constraint_paths_from_region(fr.clone());
	let path = constraints.iter().min_by_key(|p| p.len()).unwrap();
	debug!("report_error: shortest_path={:?}", path);
	let mut categorized_path = path.iter().filter_map(|index| {
	self.classify_constraint(index, mir)
	}).collect::<Vec<(ConstraintCategory, Span)>>();
	debug!("report_error: categorized_path={:?}", categorized_path);
	categorized_path.sort_by(|p0, p1| p0.0.cmp(&p1.0));
	debug!("report_error: sorted_path={:?}", categorized_path);

Later, you can see we pick a constraint and try to use it as the "frame" for how we report the error:

rust/src/librustc_mir/borrow_check/nll/region_infer/error_reporting.rs

Line 245 in b58b721

if let Some((category, span)) = &categorized_path.first() {

In this case, the constraint really arises from the assignment that sets x = Some(y); that is the point bb0[2] in the MIR:

        (*(_1.0: &'_#5r mut std::option::Option<&'_#6r ()>)) = std::option::Option<&'_#7r ()>::Some(move _3,);

Looking at the region constraint graph, it seems clear that we should be selecting this as the most interesting point. However, from dumping the current behavior, it seems we're not, and I'm not sure that is, but i'm going to ignore that for now and assume it was the result.

How to make a nice error and what it should say

I am imagining that we look for this scenario:

• The "sub" region is a local universal region
• The "sup" region is an external universal region
• The most interesting constraint was from an assignment
  • The target of the assignment is a "upvar field projection"

then we can

• (a) find the declaration of the upvar (x, in our example)

and finally report an error sorta like this:

error: borrowed data cannot be stored outside of its closure
  --> issue-45983.rs:17:27
   |
16 |     let mut x = None;
   |         ----- borrowed data cannot be stored into here...
17 |     give_any(|y| x = Some(y));
   |              --- ^^^^^^^^^^^ cannot be stored outside of its closure
   |              |
   |              ...because it cannot outlive this closure

I'm preserving the existing wording above, but note that the spans are slightly different (I also think we could improve on the wording, but let's leave that for a second). Here is where the spans would come from:

• "borrowed data cannot be stored into here..." comes from the decl of the upvar being stored to
• "cannot be stored outside of its closure" comes from the span on the MIR statement for the assignment
• "...because it cannot outlive this closure" comes from closure that we are checking

Improving the wording

I think we can do better on the wording, but we'll need a bit more information. By looking at the types of the parameters we ought to be able to figure out that the data comes from the y parameter (we could look at the MIR too, but looking at the types of the parameters is more robust). Then I think we could say something like this

error: borrowed data cannot be stored outside of its closure
  --> issue-45983.rs:17:27
   |
17 |     give_any(|y| x = Some(y));:
   |               -  ^^^^^^^^^^^ stores `y` outside of the closure body
   |               |
   |               `y` is a reference that is only valid in the closure body

we could possibly keep the note on x, but I'm not sure how important it is. Maybe something like this?

error: borrowed data cannot be stored outside of its closure
  --> issue-45983.rs:17:27
   |
16 |     let mut x = None;
   |         ----- `x` is declared here, outside of the closure body
17 |     give_any(|y| x = Some(y));:
   |               -  ^^^^^^^^^^^ stores `y` into `x`
   |               |
   |               `y` is a reference that is only valid in the closure body

[nikomatsakis]

cc @estebank I'd love to see your thoughts on the wording towards the end of the previous comment

[estebank]

I like the last example's clarity, but I'll mull over it for a bit and see if I can come up with any improvement for it.

[estebank]

Maybe...

error: borrowed data cannot be stored outside of its closure
  --> issue-45983.rs:17:27
   |
16 |     let mut x = None;
   |         ----- `x` is declared outside of the closure body
17 |     give_any(|y| x = Some(y));:
   |               -  ^^^^^^^^^^^ can't store `y` into `x` because it's outside of its closure
   |               |
   |               this a borrow only valid for the duration of its closure body

[nikomatsakis]

(See also the examples in #51026)

[nikomatsakis]

Regardless of the specific wording, it seems like we want to do a few things:

• Highlight y and point out that it contains data valid only during the closure body
• Highlight x and point out that it is not inside the closure body
• Highlight where y escapes the closure body

Currently we print this:

error: unsatisfied lifetime constraints
 --> /home/nmatsakis/tmp/issue-51027.rs:7:18
  |
6 |     let x = None;
  |         - lifetime `'2` appears in the type of `x`
7 |     give_any(|y| x = Some(y));
  |               -  ^^^^^^^^^^^ free region requires that `'1` must outlive `'2`
  |               |
  |               lifetime `'1` appears in this argument

You can see we've got notes on all the right places, but the text ain't great. As I wrote before, I think that before printing the error we want to check for a case a: b where:

• The "a" region is a local universal region
• The "b" region is an external universal region

This indicates that data that is "local" to the closure body is trying to flow out of it (I don't think that the 'main constraint' kind is that important).

I would probably like to see the error read roughly like this:

error: borrowed data escapes outside of closure
  --> issue-45983.rs:17:27
   |
16 |     let mut x = None;
   |         ----- `x` is declared here, outside of the closure body
17 |     give_any(|y| x = Some(y));:
   |               -  ^^^^^^^^^^^ `y` escapes the closure body here
   |               |
   |               `y` is a reference that is only valid in the closure body

I am imagining that we would check the sub/sup to see if this is a "escaping data" scenario (as opposed to a "normal" scenario). We would then pass own this "scenario" down to give_region_a_name:

rust/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/region_name.rs

Line 50 in a8247dd

crate fn give_region_a_name(

which would use it to add a suitable label. e.g.:

• instead of "let's call the lifetime of this reference '1"
  • we might print "this reference cannot escape the closure body"
• and instead of "lifetime '1 appears in this argument"
  • we might print "this argument is a reference only valid in the closure body"

and we similarly adjust the "main" label.

Note that in this form I am not giving explicit names (like '1) to the lifetime -- that seems ok, though we may want to incorporate said names later.

[davidtwco]

I've submitted #52572 that addresses this issue.