closure handling for NLL #46509

nikomatsakis · 2017-12-05T01:14:06Z

This branch overhauls how we handle closures and universally quantified regions in the NLL code. The goal is to lay the groundwork for "region erasure" by the HIR-based type checker, as well as to avoid "crazy hacks" when it comes to closures. This means that when we type-check a closure, we cannot make use of any of the precise values of free regions in its signature, since those are inferred by the HIR type-checker. Therefore, the new code handles closures by:

Creating fresh regions for every free region that appears in the closure's type, which now includes both its signature and the types of all upvars.
- This means that the closure is type-checked without knowing about the connections.
When we encounter some region relationship that we can't locally verify, we propagate it to the closure's creator.
During MIR typeck, the closure creators then validates those relationships.

For a good example and explanation, see e.g. the test src/test/nll/closure-requirements/propagate-despite-same-free-region.rs.

Upcoming changes in the next NLL PR (not included in this PR in order to keep it manageable):

Improvements to the MIR type checker so that it handles a lot of stuff presently overlooked
Refactor how we store region values to make it more efficient, better encapsulated
Propagate "type-outlives" relationships like T: 'a in a similar fashion to the one covered in this PR (still in the works, but close)
Improvements to error reporting (still in the works)

r? @arielb1 or @pnkfelix

pnkfelix · 2017-12-06T13:16:38Z

src/librustc_mir/util/pretty.rs

+}
+
+struct ExtraComments<'cx, 'gcx: 'tcx, 'tcx: 'cx> {
+    _tcx: TyCtxt<'cx, 'gcx, 'tcx>, // don't need it now, but bet we will soon


A potential alternative: Instead of having ExtraComments carry a tcx and get passed into a callback closure, you might consider having the closure itself be responsible for carrying the tcx?

At least, that's what I ended up deciding to do as part of a cleanup of the drop_flag_effects code, see here, where I found it more pleasant to put one or two type params in the signature than to pass around TyCtxt and its three required lifetimes...

In this case, it's a mir visitor, not a closure.

But also, I don't personally mind passing around the three lifetimes. Especially once the in-band lifetimes work lands makes its way into beta (it's in nightly already...), that will be relatively lightweight.

pnkfelix · 2017-12-06T13:22:42Z

src/librustc_mir/util/pretty.rs

+        self.super_constant(constant, location);
+        let Constant { span, ty, literal } = constant;
+        self.push(&format!("mir::Constant"));
+        self.push(&format!("└ span: {:?}", span));


box characters! 😒

but seriously, just in case someone eventually wants to write tests of such output by hand, why not stick to ascii?

And this person doesn't have an editor that supports copy-and-paste? As it happens, you actually cannot test any of these comments our mir-opt infrastructure, since that infrastructure strips out comments right now. So in that event further changes would be needed anyhow.

I mean I don't care too much, I can change to some ASCII thing (maybe \?) if you prefer. But it seems like we should be able to use box characters in debug output gosh darn it. =)

pnkfelix · 2017-12-06T13:30:17Z

src/librustc/ty/sty.rs

+    /// contain any bound regions that would be bound by the
+    /// binder. This is commonly used to 'inject' a value T into a
+    /// different binding level.
+    pub fn new_not_binding<'tcx>(value: T) -> Binder<T>


(Personally i'd call this dummy_binding or new_dummy_binding (since in my head the binding exists, it just is useless), but I can cope with this function name.)

I could get behind Behinder::dummy or something.

arielb1 · 2017-12-06T18:33:38Z

src/librustc_mir/util/pretty.rs

@@ -11,7 +11,8 @@
 use rustc::hir;


arielb1 · 2017-12-06T18:57:23Z

src/librustc_typeck/collect.rs

-                ref t => bug!("closure with non-closure type: {:?}", t),
-            }
+        NodeExpr(&hir::Expr { node: hir::ExprClosure(..), .. }) => {
+            // In order to property accommodate regions during NLL


The reason a closure sig can't return fn signatures is because closure signatures are not fn signatures - e.g. they miss the self-argument, and in any case they are right in the closure's type.

arielb1 · 2017-12-06T19:51:58Z

src/test/ui/nll/closure-requirements/escape-argument-callee.rs

+// - takes an argument `y`
+// - stores `y` into another, longer-lived spot
+//
+// *but* the signature of the closure doesn't indicate that `y` lives


but in the closure's signature, y and the slot are different unrelated higher-ranked regions. Because closure inference is not allowed to add where-clauses between higher-ranked regions, there is no set of free lifetimes that allows the closure to run, so the closure reports the error (and hence we see it before the closure's "external requirements" report).

arielb1 · 2017-12-06T19:53:39Z

src/test/ui/nll/closure-requirements/escape-argument.rs

+// - stores `y` into another, longer-lived spot
+//
+// but is invoked with a spot that doesn't live long
+// enough to store `y`.


Here the regions are higher-ranked in the closure signature but explicitly linked, so (I hope) this only checks that the closure signature is obeyed.

arielb1 · 2017-12-06T19:55:56Z

src/test/ui/nll/closure-requirements/escape-upvar-nested.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// As in via-upvar, test closure that:


As in escape-upvar-ref?

arielb1 · 2017-12-06T19:56:43Z

src/test/ui/nll/closure-requirements/via-upvar-nested.stderr

@@ -0,0 +1,87 @@
+note: External requirements


Where is via-upvar-nested.rs?

oops, deleted

arielb1 · 2017-12-06T19:57:45Z

src/test/ui/nll/closure-requirements/escape-upvar-ref.rs

+// Test closure that:
+// - captures a variable `y`
+// - stores reference to `y` into another, longer-lived spot
+


Here both the spot and y are upvars and therefore closure-external ("free") regions

arielb1 · 2017-12-06T20:00:38Z

src/test/ui/nll/closure-requirements/propagate-approximated-ref.rs

+// Note: the use of `Cell` here is to introduce invariance. One less
+// variable.
+//
+// FIXME: The `supply` function *ought* to generate an error, but it


Could you try to make this an issue on the milestone?

It was #45827 -- this is currently marked as closed however since the work has landed on nll-master (and the test is fixed there).

arielb1 · 2017-12-06T20:10:28Z

src/test/ui/nll/closure-requirements/propagate-approximated-to-empty.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// Test a case where we fail to approximate one of the regions and


Similarly to escape-argument-callee, a test case where the closure requires a relationship between 2 unrelated higher-ranked regions, with no helpful relations between the HBRs and free regions.

Maybe also add to this test cases where:

'c: 'x, 'b: 'y \- CAN'T HELP, because we can't prevent 'x and 'y from being assigned "different" short lifetimes 'y: 'c \- can't help, because we can't prevent 'x from being short.

arielb1 · 2017-12-06T20:11:41Z

src/test/ui/nll/closure-requirements/propagate-approximated-to-static.rs

+
+// Callee knows that:
+//
+// 'x: 'a


Actually, digging into this revealed something weird. Why is this not getting an error reported? It's propagating the right relationships, but they're not yielding any errors...

Oh, it's another case of "is fixed later on in nll-master". In particular, it's #45827 I think but maybe one of the other issues. I can add a fixme, but I just tested and I do get the expected errors on nll-master.

arielb1 · 2017-12-06T20:36:36Z

src/test/ui/nll/closure-requirements/region-ebr-does-not-outlive-static.rs

@@ -0,0 +1,26 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT


can't you find some compile-fail tests and run them with NLL?

Conceivably. =) Yeah, I'll search.

arielb1 · 2017-12-06T20:37:22Z

src/test/ui/nll/closure-requirements/region-lbr-named-does-not-outlive-static.rs

+// a variety of errors from the older, AST-based machinery (notably
+// borrowck), and then we get the NLL error at the end.
+
+// compile-flags:-Znll -Zborrowck=mir


Why do you have different tests with -Z nll and with -Z nll -Z borrowck=mir?

Mistake. It used to be that -Znll implies -Zborrowck-mir so maybe it dates from then. They should really all have -Znll -Zborrowck=mir -Zverbose.

arielb1 · 2017-12-06T20:56:56Z

src/librustc_mir/borrow_check/nll/mod.rs

+    }
+}
+
+fn for_each_region_constraint(


that's one weird factoring. I would have a Display implementation for OutlivesRequirement

Two reasons I did it this way:

We are not displaying to the "end user" -- this is still debug output, so Display feels inappropriate to me. But I could make some sort of newtype that implements Debug.

In one context, we want to prepend a | and use writeln! for each line, and in the other, we want to invoke err.note() with each line.

The latter is real killer, though I think err.note() can handle internal newlines.

arielb1 · 2017-12-06T21:07:25Z

src/test/ui/nll/named-region-basic.stderr

-   |     ^^^
-
-error: aborting due to 2 previous errors
-


Thinking about the tests, I didn't see a test where a free region was directly compared against a bound region. aka

fn foo<'a, F: for<'x> FnOnce( Cell<&'a u32>, Cell<&'x u32>)>(_cell: Cell<&'a u32>, _f: F) { } fn case1() { let a = 0; let cell = Cell::new(&a); foo(cell, |cell_1, cell_2| { cell_1.set(cell_2.get()); // forces 'x: 'a, error in closure }) } fn case1() { let a = 0; let cell = Cell::new(&a); foo(cell, |cell_1, cell_2| { cell_2.set(cell_1.get()); // forces 'a: 'x, implies 'a = 'static -> borrow error }) }

You could also have a test where we can use a given to convert a free/bound relationship to a free/free relationship, but we already have tests that show we can use givens to convert bound/bound to free/free (propagate-despite-same-free-region) so I think it's less critical to add another test - you can still add one if you want :-).

yeah, I think existing tests cover that ok (free/bound -> free/free).

arielb1 · 2017-12-06T21:09:48Z

src/test/ui/nll/closure-requirements/propagate-despite-same-free-region.rs

+// need to propagate; but in fact we do because identity of free
+// regions is erased.
+
+// compile-flags:-Znll -Zborrowck=mir -Zverbose


doesn't this need a compilation successful marker?

Those are not enforced by the UI framework right now. That reminds me that I was going to open an issue on that. It's kind of the "default".

bors · 2017-12-07T07:19:01Z

☔ The latest upstream changes (presumably #46533) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2017-12-07T10:26:45Z

I pushed changes addressing all existing comments, except that:

I opted to leave for_each_region_constraint as explained here
I think there are a few more duplicate-y tests that I could readily de-duplicate from the compile-fail directory
I left in the box-drawing characters, as explained here

The overall format is now easier to read. Also, There is now graphviz output, as well as a `#[rustc_regions]` annotation that dumps internal state.

It's inefficient, and the substitution there doesn't account for the extra regions used by NLL inference, so it's a bad thing to encourage. As it happens all callers already know if they have a closure or not, from what I can tell.

It's just not useful. It also makes it hard to have tests that probe internal state, since the interning number is very sensitive. Dumping the number in the case of gensym is not ideal but will do for now.

This includes the additional case suggested by arielb1.

as suggested by arielb1

suggested by arielb1

per pnkfelix's suggestion

arielb1 · 2017-12-07T12:57:44Z

src/librustc_mir/borrow_check/nll/region_infer/mod.rs

+
+            // Shrink `fr` until we find a non-local region (if we do).
+            // We'll call that `fr-` -- it's ever so slightly smaller than `fr`.
+            if let Some(fr_minus) = self.universal_regions.non_local_lower_bound(longer_fr) {


The more I read this scheme, the less certain I become it has any chance of working in "real life". But I don't really know of a better way, and I suppose there's only 1 way to see.

arielb1 · 2017-12-07T13:12:06Z

Nice PR. If you want to change the tests somewhat, that's OK by me.

@bors r+ p=1

bors · 2017-12-07T13:12:06Z

📌 Commit 1db58d7 has been approved by arielb1

bors · 2017-12-07T14:59:30Z

⌛ Testing commit 1db58d7 with merge a8437a0...

@arielb1

…ielb1 closure handling for NLL This branch overhauls how we handle closures and universally quantified regions in the NLL code. The goal is to lay the groundwork for "region erasure" by the HIR-based type checker, as well as to avoid "crazy hacks" when it comes to closures. This means that when we type-check a closure, we cannot make use of *any* of the precise values of free regions in its signature, since those are inferred by the HIR type-checker. Therefore, the new code handles closures by: - Creating fresh regions for every free region that appears in the closure's type, which now includes both its signature and the types of all upvars. - This means that the closure is type-checked without knowing about the connections. - When we encounter some region relationship that we can't locally verify, we propagate it to the closure's creator. - During MIR typeck, the closure creators then validates those relationships. For a good example and explanation, see e.g. the test `src/test/nll/closure-requirements/propagate-despite-same-free-region.rs`. Upcoming changes in the next NLL PR (not included in this PR in order to keep it manageable): - Improvements to the MIR type checker so that it handles a lot of stuff presently overlooked - Refactor how we store region values to make it more efficient, better encapsulated - Propagate "type-outlives" relationships like `T: 'a` in a similar fashion to the one covered in this PR (still in the works, but close) - Improvements to error reporting (still in the works) r? @arielb1 or @pnkfelix

bors · 2017-12-07T18:13:05Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing a8437a0 to master...

rust-highfive assigned arielb1 Dec 5, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 5, 2017

pnkfelix reviewed Dec 6, 2017

View reviewed changes

arielb1 reviewed Dec 6, 2017

View reviewed changes

src/librustc_mir/util/pretty.rs

@@ -11,7 +11,8 @@

use rustc::hir;

Copy link

Contributor

arielb1 Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok nic

arielb1 reviewed Dec 6, 2017

View reviewed changes

nikomatsakis added 8 commits December 7, 2017 05:27

renumber types in ty::Const and relate them to mir::Constant

e5dc4ba

extend MIR dump with detailed, extra information

2ec959f

mir-borrowck returns closure requirements, mir-typeck enforces

ab1c1bc

add closure requirement tests, improve debugging output

05441ab

The overall format is now easier to read. Also, There is now graphviz output, as well as a `#[rustc_regions]` annotation that dumps internal state.

replace InferCtxt::fn_sig with closure_sig

d5ef3e2

make fn_sig().subst() ICE when used with a closure

b4d71ea

It's inefficient, and the substitution there doesn't account for the extra regions used by NLL inference, so it's a bad thing to encourage. As it happens all callers already know if they have a closure or not, from what I can tell.

fix filter to support & and |

681f54f

don't print self symbol's internal index unless gensymed

47b8ef5

It's just not useful. It also makes it hard to have tests that probe internal state, since the interning number is very sensitive. Dumping the number in the case of gensym is not ideal but will do for now.

nikomatsakis added 13 commits December 7, 2017 05:28

rewrite comment on how to get closure sig, per arielb1's suggestion

05977fe

escape-argument-callee: improve comment per arielb1's suggestions

dac59ac

escape-argument: improve comment per arielb1's suggestion

782b9f5

escape-upvar-nested: patch "dangling reference" to renamed test

8b657d3

escape-upvar-ref: patch comment per arielb1

6ee31e2

delete stray file

3c42f18

test approximations of the "shorter side" that resort to 'static

ca60826

This includes the additional case suggested by arielb1.

test more failure cases for approximating the "longer" side

2a17b93

as suggested by arielb1

propagate-approximated-ref: include FIXME comment

b8e9eaf

region-ebr-does-not-outlive-static: reuse old test instead

75cf482

use -Znll -Zborrowck=mir -Zverbose consistently

86e7b5c

add test comparing free region to bound region

4703770

suggested by arielb1

rename Binder::new_not_binding to Binder::dummy

1db58d7

per pnkfelix's suggestion

nikomatsakis force-pushed the nll-master-to-rust-master-3 branch from e1b8261 to 1db58d7 Compare December 7, 2017 10:39

arielb1 reviewed Dec 7, 2017

View reviewed changes

bors merged commit 1db58d7 into rust-lang:master Dec 7, 2017

This was referenced Dec 7, 2017

Make GlobalCtxt implement Sync #45912

Closed

Move collector to librustc_mir::monomorphize #45525

Merged

WIP: Parallelize passes using rayon #46564

Closed

		@@ -0,0 +1,26 @@
		// Copyright 2016 The Rust Project Developers. See the COPYRIGHT

closure handling for NLL #46509

closure handling for NLL #46509

Conversation

nikomatsakis commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Dec 7, 2017

nikomatsakis commented Dec 7, 2017

Choose a reason for hiding this comment

arielb1 commented Dec 7, 2017

bors commented Dec 7, 2017

bors commented Dec 7, 2017

bors commented Dec 7, 2017

nikomatsakis Dec 6, 2017 •

edited

Loading

arielb1 Dec 6, 2017 •

edited

Loading

arielb1 Dec 6, 2017 •

edited

Loading

arielb1 Dec 6, 2017 •

edited

Loading