[NLL] Record more infomation on free region constraints in typeck #54262

matthewjasper · 2018-09-15T17:46:51Z

Changes:

Makes the span of the MIR return place point to the return type
Don't try to use a path to a type alias as a path to the adt it aliases (fixes an ICE)
Don't claim that self is declared outside of the function. see this test
Remove boring/interesting distinction and instead add a ConstraintCategory to the constraint.
Add categories for implicit Sized and Copy requirements, for closure bounds, for user type annotations and impl Trait.
Don't use the span of the first statement for Locations::All bounds (even if it happens to work on the tests we have)

Future work:

Fine tuning the heuristic used to choose the place the report the error.
Reporting multiple places (behind a flag)
Better closure bounds reporting. This probably requires some discussion.

bors · 2018-09-18T09:16:41Z

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

nikomatsakis · 2018-09-18T15:48:04Z

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

+        a: Ty<'tcx>,
+        b: Ty<'tcx>,
+        locations: Locations,
+        category: ConstraintCategory,


So, I did not want to do it this way — but I'm trying to remember why and coming up empty. Seems ok.

I'm wondering if it makes sense to keep both locations and category as distinct things. Perhaps so: in particular, locations (once we transition to polonius) will be relevant to whether or not this is an error, whereas category is not.

nikomatsakis · 2018-09-18T15:51:56Z

src/test/ui/associated-types/cache/project-fn-ret-contravariant.krisskross.nll.stderr

-   |            ^^^^^^^^^^^ assignment requires that `'b` must outlive `'a`
+...
+LL |    (a, b) //[krisskross]~ ERROR 55:5: 55:6: lifetime mismatch [E0623]
+   |    ^^^^^^ function was supposed to return data with lifetime `'b` but it is returning data with lifetime `'a`


Interesting example. I feel like this is a good case where I'd prefer to see more than one highlight.

Or, even better, if we could highlight a and not the entire tuple. I think this would be possible: we could probably detect if we are returning an aggregate and see if we can trace back to one of the inputs into the aggregate. This is particularly easy in tuples.

That said, another route that might be better (or just complementary) is to highlight more than one location. In this case, I think it'd be nice to highlight the bar(foo, y) point as well, basically saying "data gets the lifetime 'a from here". Anyway, I'm noting this down for further iteration.

nikomatsakis · 2018-09-18T15:52:28Z

src/test/ui/impl-trait/must_outlive_least_region_or_bound.nll.stderr

   |
-LL |     x
-   |     ^
+LL | fn ty_param_wont_outlive_static<T:Debug>(x: T) -> impl Debug + 'static {


this feels like a regression, no?

I guess actually it's another case of "it'd be nice to highlight more spots"

The highlighted span is the span of the first statement, it just happens to be the right span in this case.

nikomatsakis · 2018-09-18T15:56:50Z

src/test/ui/impl-trait/type_parameters_captured.nll.stderr

   |
-LL |     x
-   |     ^
+LL | fn foo<T>(x: T) -> impl Any + 'static {


another case where highlight the return type feels not quite right

the core problem here is that, with impl Trait in particular, it's useful to highlight the return type.

I wonder if we could add a note like this:

error[E0310]: the parameter type `T` may not live long enough --> $DIR/type_parameters_captured.rs:17:20 | LL | fn foo<T>(x: T) -> impl Any + 'static { | ^^^^^^^^^^^^^^^^^^ LL | x | - the `impl Trait` is inferred to have the type `T` LL | } | = help: consider adding an explicit lifetime bound `T: 'static`...

The idea would be to find the span(s) of where the return slot is assigned and use those, and just show the underlying type.

nikomatsakis · 2018-09-18T15:57:22Z

src/test/ui/issues/issue-10291.nll.stderr

+   |  _________________________________________________________________^
+LL | |         x //~ ERROR E0312
+LL | |     }));
+   | |_____^ closure body requires that `'x` must outlive `'static`


I think this case would be addressed by @davidtwco's PR -- anyway, feels like an improvement in this PR, if not optimal.

nikomatsakis · 2018-09-18T15:57:57Z

src/test/ui/lifetimes/lifetime-errors/ex3-both-anon-regions-one-is-struct.nll.stderr

@@ -6,7 +6,7 @@ LL | fn foo(mut x: Ref, y: &u32) {
   |        |
   |        has type `Ref<'_, '2>`
 LL |     x.b = y; //~ ERROR lifetime mismatch
-   |     ^^^^^^^ requires that `'1` must outlive `'2`
+   |     ^^^^^^^ assignment requires that `'1` must outlive `'2`


Question: is this an improvement? It seems like it's sort of obvious that it's an assignment, not sure if it helps for us to say so?

Maybe if we said something better, like "data with lifetime '1 is stored into a location with lifetime '2 here"? that doesn't quite sound right either.

nikomatsakis · 2018-09-18T16:01:23Z

src/test/ui/nll/issue-50716.stderr

 ...
 LL |     let _x = *s; //~ ERROR
-   |              ^^ `s` escapes the function body here
+   |              ^^ proving this value is `Sized` requires that `'a` must outlive `'static`


nikomatsakis · 2018-09-18T16:12:13Z

src/test/ui/nll/ty-outlives/projection-one-region-closure.stderr

-   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^
-   |
-   = help: consider adding an explicit lifetime bound `T: ReFree(DefId(0/0:8 ~ projection_one_region_closure[317d]::no_relationships_late[0]), BrNamed(crate0:DefIndex(1:16), 'a))`...
+   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^ closure body requires that `'b` must outlive `'a`


So, pulling out the constraint category into typeck might mean that we can package it up with the ClosureRegionRequirements, as well. Have to think a bit about that. This might enable us to give finer-grained reports here.

nikomatsakis · 2018-09-18T16:13:33Z

src/test/ui/regions/regions-close-object-into-object-4.nll.stderr

+LL | fn i<'a, T, U>(v: Box<A<U>+'a>) -> Box<X+'static> {
+   |      -- lifetime `'a` defined here
+LL |     box B(&*v) as Box<X> //~ ERROR cannot infer
+   |     ^^^^^^^^^^^^^^^^^^^^ returning this value requires that `'a` must outlive `'static`


in these cases, it'd be really nice if we could tie this a bit harder to the return type. good follow-up.

nikomatsakis · 2018-09-18T16:14:01Z

r=me post rebase

function body

The MIR/NLL type checker is in a much better position to classify constraints and already has to classify into boring and interesting. Adds spans to Locations::All for error reporting Adds more constraint categories

nikomatsakis · 2018-09-20T18:57:21Z

@bors r+

bors · 2018-09-20T18:57:22Z

📌 Commit bd0895d has been approved by nikomatsakis

bors · 2018-09-23T01:40:01Z

⌛ Testing commit bd0895d with merge 576b640...

@nikomatsakis

[NLL] Record more infomation on free region constraints in typeck Changes: * Makes the span of the MIR return place point to the return type * Don't try to use a path to a type alias as a path to the adt it aliases (fixes an ICE) * Don't claim that `self` is declared outside of the function. [see this test](f2995d5#diff-0c9e6b1b204f42129b481df9ce459d44) * Remove boring/interesting distinction and instead add a `ConstraintCategory` to the constraint. * Add categories for implicit `Sized` and `Copy` requirements, for closure bounds, for user type annotations and `impl Trait`. * Don't use the span of the first statement for Locations::All bounds (even if it happens to work on the tests we have) Future work: * Fine tuning the heuristic used to choose the place the report the error. * Reporting multiple places (behind a flag) * Better closure bounds reporting. This probably requires some discussion. r? @nikomatsakis

bors · 2018-09-23T04:06:07Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 576b640 to master...

rust-highfive assigned nikomatsakis Sep 15, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 15, 2018

matthewjasper added the A-NLL Area: Non-lexical lifetimes (NLL) label Sep 15, 2018

nikomatsakis reviewed Sep 18, 2018

View reviewed changes

nikomatsakis approved these changes Sep 18, 2018

View reviewed changes

matthewjasper added 5 commits September 19, 2018 19:52

Make the span of the MIR return place point to the return type

b210b31

Don't try to use a path to a type alias as a path to the adt it aliases

fcd0cd0

Don't claim that locals in a function are declared outside of the

6e42521

function body

Classify outlives constraints when type checking

994dc4b

The MIR/NLL type checker is in a much better position to classify constraints and already has to classify into boring and interesting. Adds spans to Locations::All for error reporting Adds more constraint categories

Update ui tests

bd0895d

matthewjasper force-pushed the explain-in-typeck branch from 33c9188 to bd0895d Compare September 19, 2018 19:51

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 20, 2018

bors merged commit bd0895d into rust-lang:master Sep 23, 2018

This was referenced Sep 23, 2018

rework how we handle outlives relationships #54453

Merged

[NLL] Get Polonius borrow check to work in simple cases #54468

Merged

impl trait in bindings (feature: impl-trait-existential-types) #53542

Merged

matthewjasper deleted the explain-in-typeck branch November 30, 2018 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NLL] Record more infomation on free region constraints in typeck #54262

[NLL] Record more infomation on free region constraints in typeck #54262

matthewjasper commented Sep 15, 2018

bors commented Sep 18, 2018

nikomatsakis Sep 18, 2018

nikomatsakis Sep 18, 2018

nikomatsakis Sep 18, 2018

nikomatsakis Sep 18, 2018

matthewjasper Sep 18, 2018

nikomatsakis Sep 19, 2018

nikomatsakis Sep 18, 2018

nikomatsakis Sep 18, 2018

nikomatsakis Sep 18, 2018

nikomatsakis Sep 18, 2018

nikomatsakis Sep 18, 2018

nikomatsakis Sep 18, 2018

nikomatsakis Sep 18, 2018

nikomatsakis commented Sep 18, 2018

nikomatsakis commented Sep 20, 2018

bors commented Sep 20, 2018

bors commented Sep 23, 2018

bors commented Sep 23, 2018

[NLL] Record more infomation on free region constraints in typeck #54262

[NLL] Record more infomation on free region constraints in typeck #54262

Conversation

matthewjasper commented Sep 15, 2018

bors commented Sep 18, 2018

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

nikomatsakis commented Sep 18, 2018

nikomatsakis commented Sep 20, 2018

bors commented Sep 20, 2018

bors commented Sep 23, 2018

bors commented Sep 23, 2018