NLL: unexpected "free region `` does not outlive" error

[nikomatsakis]

In the test borrowck/borrowck-describe-lvalue.rs, when using NLL by default for MIR borrowck, I started getting the odd error "Free region does not outlive" error below:

    // Field from upvar nested
    {
        let mut x = 0;
           || {
               || { //[mir]~ ERROR free region `` does not outlive
                   let y = &mut x;
                   &mut x; //[ast]~ ERROR cannot borrow `**x` as mutable more than once at a time
                   //[mir]~^ ERROR cannot borrow `x` as mutable more than once at a time
                   *y = 1;
                   drop(y);
                }
           };
    }

Seems like a bug.

[nikomatsakis]

Context:

Closures desugar to a struct. Sometimes this struct will have requirements that are placed on its creator. It's the closure's job to infer those requirements. The creator then tries to check it they are met.

Example:

fn foo(mut x: Vec<&'a u32>, y: &'b u32) {
  let c = || {
    x.push(y);
  };
  c();
}

The closure here desugars to something like

let c = ClosureStruct { x: &mut x, y: &y };
c();

where

struct ClosureStruct<'x, 'y, 'a, 'b> {
  x: &'x mut Vec<&'a u32>,
  y: &'y &'b u32
}

impl FnOnce for ClosureStruct { .. }

In the course of type-checking that impl, we will check that x.push(y) statement. We will find that this type checks, but only if 'b: 'a. These regions are declared outside the closure, so we will propagate them to our caller, who will check (and -- in the case of this example -- generate an error).

This is what the CloureReturnRequirements are all about:

rust/src/librustc/mir/mod.rs

Lines 2032 to 2094 in 6c53749

	/// After we borrow check a closure, we are left with various
	/// requirements that we have inferred between the free regions that
	/// appear in the closure's signature or on its field types. These
	/// requirements are then verified and proved by the closure's
	/// creating function. This struct encodes those requirements.
	///
	/// The requirements are listed as being between various
	/// `RegionVid`. The 0th region refers to `'static`; subsequent region
	/// vids refer to the free regions that appear in the closure (or
	/// generator's) type, in order of appearance. (This numbering is
	/// actually defined by the `UniversalRegions` struct in the NLL
	/// region checker. See for example
	/// `UniversalRegions::closure_mapping`.) Note that we treat the free
	/// regions in the closure's type "as if" they were erased, so their
	/// precise identity is not important, only their position.
	///
	/// Example: If type check produces a closure with the closure substs:
	///
	/// ```text
	/// ClosureSubsts = [
	/// i8, // the "closure kind"
	/// for<'x> fn(&'a &'x u32) -> &'x u32, // the "closure signature"
	/// &'a String, // some upvar
	/// ]
	/// ```
	///
	/// here, there is one unique free region (`'a`) but it appears
	/// twice. We would "renumber" each occurrence to a unique vid, as follows:
	///
	/// ```text
	/// ClosureSubsts = [
	/// i8, // the "closure kind"
	/// for<'x> fn(&'1 &'x u32) -> &'x u32, // the "closure signature"
	/// &'2 String, // some upvar
	/// ]
	/// ```
	///
	/// Now the code might impose a requirement like `'1: '2`. When an
	/// instance of the closure is created, the corresponding free regions
	/// can be extracted from its type and constrained to have the given
	/// outlives relationship.
	///
	/// In some cases, we have to record outlives requirements between
	/// types and regions as well. In that case, if those types include
	/// any regions, those regions are recorded as `ReClosureBound`
	/// instances assigned one of these same indices. Those regions will
	/// be substituted away by the creator. We use `ReClosureBound` in
	/// that case because the regions must be allocated in the global
	/// TyCtxt, and hence we cannot use `ReVar` (which is what we use
	/// internally within the rest of the NLL code).
	#[derive(Clone, Debug, RustcEncodable, RustcDecodable)]
	pub struct ClosureRegionRequirements<'gcx> {
	/// The number of external regions defined on the closure. In our
	/// example above, it would be 3 -- one for `'static`, then `'1`
	/// and `'2`. This is just used for a sanity check later on, to
	/// make sure that the number of regions we see at the callsite
	/// matches.
	pub num_external_vids: usize,
	/// Requirements between the various free regions defined in
	/// indices.
	pub outlives_requirements: Vec<ClosureOutlivesRequirement<'gcx>>,
	}

There are lots of tests in src/test/ui/nll on this topic. Each makes use of a unit-testing feature that dumps out the closure return requirements for functions annotated with #[rustc_regions]:

rust/src/test/ui/nll/closure-requirements/return-wrong-bound-region.rs

Lines 19 to 24 in 6c53749

	#[rustc_regions]
	fn test() {
	expect_sig(|a, b| b); // ought to return `a`
	//~^ WARN not reporting region error due to -Znll
	//~| ERROR does not outlive free region
	}

The closure in that function, for example, apparently defines no external requirements:

rust/src/test/ui/nll/closure-requirements/return-wrong-bound-region.stderr

Lines 13 to 22 in 6c53749

	note: No external requirements
	--> $DIR/return-wrong-bound-region.rs:21:16
	|
	LL | expect_sig(|a, b| b); // ought to return `a`
	| ^^^^^^^^
	|
	= note: defining type: DefId(0/1:9 ~ return_wrong_bound_region[317d]::test[0]::{{closure}}[0]) with closure substs [
	i16,
	for<'r, 's> extern "rust-call" fn((&ReLateBound(DebruijnIndex { depth: 1 }, BrNamed(crate0:DefIndex(0:0), 'r)) i32, &ReLateBound(DebruijnIndex { depth: 1 }, BrNamed(crate0:DefIndex(0:0), 's)) i32)) -> &ReLateBound(DebruijnIndex { depth: 1 }, BrNamed(crate0:DefIndex(0:0), 'r)) i32
	]

Anyway, the way to start here is to (a) create a minimized test and (b) dump out the region requirements and then perhaps (c) start trying to see where they are coming from.

[spastorino]

My reduced test case is ...

#![feature(rustc_attrs)]
#![feature(nll)]

#[rustc_regions]
fn main() {
    let mut x = 0;
    || {
        || {
            let _y = &mut x;
        }
    };
}

If I borrow x immutable the issue does not trigger.

Dumping region requirements I get ...

[santiago@archlinux rust1 (master)]$ rustc +stage1 src/test/run-pass/issue-49824.rs
note: No external requirements
  --> src/test/run-pass/issue-49824.rs:18:9
   |
18 | /         || {
19 | |             let _y = &mut x;
20 | |         }
   | |_________^
   |
   = note: defining type: DefId(0/1:10 ~ issue_49824[317d]::main[0]::{{closure}}[0]::{{closure}}[0]) with closure substs [
               i16,
               extern "rust-call" fn(()),
               &mut i32
           ]

error: free region `` does not outlive free region `'_#1r`
  --> src/test/run-pass/issue-49824.rs:18:9
   |
18 | /         || {
19 | |             let _y = &mut x;
20 | |         }
   | |_________^

note: External requirements
  --> src/test/run-pass/issue-49824.rs:17:5
   |
17 | /     || {
18 | |         || {
19 | |             let _y = &mut x;
20 | |         }
21 | |     };
   | |_____^
   |
   = note: defining type: DefId(0/1:9 ~ issue_49824[317d]::main[0]::{{closure}}[0]) with closure substs [
               i16,
               extern "rust-call" fn(()) -> [closure@src/test/run-pass/issue-49824.rs:18:9: 20:10 x:&mut i32],
               &mut i32
           ]
   = note: number of external vids: 3
   = note: where '_#2r: '_#1r

note: No external requirements
  --> src/test/run-pass/issue-49824.rs:15:1
   |
15 | / fn main() {
16 | |     let mut x = 0;
17 | |     || {
18 | |         || {
...  |
21 | |     };
22 | | }
   | |_^
   |
   = note: defining type: DefId(0/0:3 ~ issue_49824[317d]::main[0]) with substs []

error: aborting due to previous error

[pnkfelix]

@spastorino by the way, in case you are not already aware of this: If you pass the -Z verbose flag to rustc, the error messages will use the fmt::Debug formatters to render regions, which means instead of getting an empty string, we get:

error: free region `ReFree(DefId(0/1:9 ~ issue_49824_reduced[317d]::main[0]::{{closure}}[0]), BrEnv)` does not outlive free region `'_#1r`

(That is just general info that can sometimes be useful when staring at compiler output, not necessarily something that will solve this particular problem...)

[pnkfelix]

In particular, by adding -Z verbose when compiling your reduced test case (along with -Z borrowck=mir to enable NLL mode), I get this marginally more informative output (in details section below).

• Oh, in hindsight I think this is just substituting in the single line I quoted in my previous comment. So, "marginally more informative" indeed...

% ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc ../../issue-49824-reduced.rs  -Z borrowck=mir -Z verbose
note: No external requirements
  --> ../../issue-49824-reduced.rs:8:9
   |
8  | /         || {
9  | |             let _y = &mut x;
10 | |         }
   | |_________^
   |
   = note: defining type: DefId(0/1:10 ~ issue_49824_reduced[317d]::main[0]::{{closure}}[0]::{{closure}}[0]) with closure substs [
               i16,
               extern "rust-call" fn(()),
               &'_#1r mut i32
           ]

error: free region `ReFree(DefId(0/1:9 ~ issue_49824_reduced[317d]::main[0]::{{closure}}[0]), BrEnv)` does not outlive free region `'_#1r`
  --> ../../issue-49824-reduced.rs:8:9
   |
8  | /         || {
9  | |             let _y = &mut x;
10 | |         }
   | |_________^

note: External requirements
  --> ../../issue-49824-reduced.rs:7:5
   |
7  | /     || {
8  | |         || {
9  | |             let _y = &mut x;
10 | |         }
11 | |     };
   | |_____^
   |
   = note: defining type: DefId(0/1:9 ~ issue_49824_reduced[317d]::main[0]::{{closure}}[0]) with closure substs [
               i16,
               extern "rust-call" fn(()) -> [closure@../../issue-49824-reduced.rs:8:9: 10:10 x:&'_#1r mut i32],
               &'_#2r mut i32
           ]
   = note: number of external vids: 3
   = note: where '_#2r: '_#1r

note: No external requirements
  --> ../../issue-49824-reduced.rs:5:1
   |
5  | / fn main() {
6  | |     let mut x = 0;
7  | |     || {
8  | |         || {
...  |
11 | |     };
12 | | }
   | |_^
   |
   = note: defining type: DefId(0/0:3 ~ issue_49824_reduced[317d]::main[0]) with substs []

error: aborting due to previous error

[nikomatsakis]

@spastorino and I discussed on gitter

[nikomatsakis]

Hmm, this might be a case where the check is actually working as intended, though the error message is not great. In particular, the outer closure is returning the inner closure -- the way the inference works, the inner closure is inferred to a FnMut that borrows x from its creator. The outer closure in turn borrows x from the top fn. But then the outer closer returns the inner one -- effectively giving away access to x after it has returned. Since it is an FnMut closure, however, it should not do that: it accesses x via an &mut self that is limited to the scope of the fn.

Either of these two variants works. If you use move on the inner closure, that forces both closures to be inferred to FnOnce:

#![feature(rustc_attrs)]
#![feature(nll)]

#[rustc_regions]
fn main() {
    let mut x = 0;
    || {
        move || {
            let _y = &mut x;
        }
    };
}

Or, if we do not return the closure:

#![feature(rustc_attrs)]
#![feature(nll)]

#[rustc_regions]
fn main() {
    let mut x = 0;
    || {
        || {
            let _y = &mut x;
        };
    };
}

I'm not sure which is more in the spirit of the original test. Probably the latter.

[nikomatsakis]

I'm not sure what's left to do here. I guess maybe alter the test case? It seems like NLL is generating the correct error.

[nikomatsakis]

Marking as a Preview 2 blocker just because it's so ridiculously simple. Let's get this done. I'll assign to myself.