rework how we handle outlives relationships #54453

nikomatsakis · 2018-09-21T23:32:36Z

When we encounter an outlives relationship involving a projection, we use to over-constrain in some cases with region constraints. We also used to evaluate whether the where-clauses in the environment might apply before running inference.

We now avoid doing both of those things:

If there are where-clauses in the environment that might be useful, we add no constraints.
After inference is done, we check if we wound up inferring values compatible with the where-clause, and make use of them if so.

I realize now that this PR includes some meandering commits and refactorings from when I expected to go in a different direction. If desired, I could try to remove some of those.

Fixes #53121
Fixes #53789

r? @pnkfelix

rust-highfive · 2018-09-21T23:48:41Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:04:38] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:38] tidy error: /checkout/src/test/ui/nll/ty-outlives/projection-where-clause-env-wrong-lifetime.rs:14: line longer than 100 chars
[00:04:38] tidy error: /checkout/src/librustc_typeck/check/dropck.rs:131: line longer than 100 chars
[00:04:39] some tidy checks failed
[00:04:39] 
[00:04:39] 
[00:04:39] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:39] 
[00:04:39] 
[00:04:39] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:39] Build completed unsuccessfully in 0:00:49
[00:04:39] Build completed unsuccessfully in 0:00:49
[00:04:39] Makefile:79: recipe for target 'tidy' failed
[00:04:39] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:16f3e506
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:040f4ac6:start=1537573239136581532,finish=1537573239141609599,duration=5028067
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:128bfcf4
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:03eecf8e
travis_time:start:03eecf8e
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:03444b55
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

pnkfelix · 2018-09-22T04:49:12Z

Aww tidy failure ...

bors · 2018-09-23T04:10:18Z

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

pnkfelix · 2018-09-24T14:18:47Z

src/librustc/infer/mod.rs

+/// A flag that is given when running region resolution: if true, it
+/// indicates that we should not report the region errors to the user
+/// if NLL is enabled, since NLL will also detect them (and do a
+/// better job of it).


Should this comment include the special role of borrowck=migrate?

Instead of checking for the borrowck=mode after seeing UnlessNll(true), maybe we should revise how UnlessNll itself is initialized based on inspecting that BorrowckMode?

Its up to you; I just know that I initially found the name UnlessNll confusing, given that we will emit the errors under the migration mode... But of course that was inherited from the previous code...

I feel like if we do the checking before initializing UnlessNll, I would just call it Ignore or something like that, since at that point it is encoding whether to ignore the error and nothing else, right?

(Right now it's saying "is this the sort of error we should ignore if NLL is enabled" — maybe I can improve the comment this way?)

I'm not inclined to move the logic earlier because it's created many places -- well, I guess I can just have a constructor like RegionErrors::suppress_if_nll() or something?

ok, did that, and it seems nice. will push a commit.

pnkfelix · 2018-09-24T14:43:12Z

src/librustc/infer/outlives/verify.rs

+    /// user has `<T as Trait<'a>>::Item: 'b` in the environment, then
+    /// the clause from the environment only applies if `'0 = 'a`,
+    /// which we don't know yet. But we would still include `'b` in
+    /// this list.


I'm going through the PR commit-by-commit, so this question may get answered later on, but I don't want to forget to ask it: What is the intended use of this check, where it sounds like it is incomplete on two axes...?

My current hypothesis is that this check is used to provide diagnostics in scenarios where we are comfortable giving feedback suggesting the user might need to satisfy some constraint, when the reality is that they might be able to sidestep the constraint... That probably didn't make sense. let me try again

Here's a different version of my question: You have given a concrete example where the list returned by this function would include 'b, even though that bound may not apply, since it depends on the whether it is true that '0 = 'a. So: Do you have an example of a diagnostic where as a user one can see the effect of 'b being on this list?

My question might be answered by cebf83502fd7c57dd2a7d1481b00d93ec03bab0e

(Though commit cebf83502fd7c57dd2a7d1481b00d93ec03bab0e only has tests being updated that are observing internal region representations... I think some of tests still show side-effects that end-users can observe without using -Z flags or internal features, but I hope that by the time I get to the end of the commit series, there will be a concrete test that demonstrates the effect without -Z flags or internal features.)

pnkfelix · 2018-09-24T15:13:57Z

src/test/ui/nll/ty-outlives/projection-where-clause-none.rs

@@ -0,0 +1,26 @@
+#![feature(nll)]
+
+// Test that we are NOT able to establish that `<T as


why is there no projection-where-clause-none.stderr in this PR?

(it seems like a bunch of .stderr files have been left out of this PR?)

hmm, curious, I have no such files locally...? I'll fix, anyway

maybe I misunderstand how ui tests work...

I had assumed that any test without // run-pass or // compile-pass was implicitly interpreted as a // compile-fail test; and all such tests should have stderr output. So that would mean they need .stderr files, right?

Does compiletest ... not complain if the .stderr files are entirely missing for compile-fail tests? It certainly seemed to complain about them missing for // run-pass tests...

pnkfelix · 2018-09-24T15:15:59Z

Okay this all seems fine.

r=me once you've placated travis, which presumably will require tidying and adding some missing .stderr files.

nikomatsakis · 2018-09-24T17:16:26Z

@pnkfelix

What is the intended use of this check, where it sounds like it is incomplete on two axes...?

I'm pulling the answer to this out because I will rebase and your comments will likely be lost. The idea of the "approximate" check is that we find out: "Are there any where clauses that might potentially be useful once region inference is done, presuming we inferred the various region values to the right values?"

It's not really meant to be conservative in "two dimensions". That is, projection_approx_declared_bounds_from_env ought to return anything that might potentially be useful. I suspect this is not presently true, though, around bound regions and a few other edge cases (e.g., I think we by and large ignore things like for<'a> T::Proj<'a>: 'a, which is really a bug, although a pre-existing one; we also are too conservative I think around equality of types like for<'a> fn(&'a u32) vs for<'b> fn(&'b u32). Also a pre-existing bug.)

I might be able to fix those cases where we are "overly" conservative though via a more thorough erasure. I think erasing regions today preserves the lifetime bounds, but we would ideally look for an "equality" check that just completely ignores region equality (even around binders). I was working towards that but never finished it.

rust-highfive · 2018-09-25T14:19:12Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:04:35] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:35] tidy error: /checkout/src/test/ui/nll/ty-outlives/projection-where-clause-env-wrong-lifetime.rs:14: line longer than 100 chars
[00:04:35] tidy error: /checkout/src/librustc/infer/error_reporting/mod.rs:302: line longer than 100 chars
[00:04:35] tidy error: /checkout/src/librustc_typeck/check/dropck.rs:131: line longer than 100 chars
[00:04:36] some tidy checks failed
[00:04:36] 
[00:04:36] 
[00:04:36] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:36] 
[00:04:36] 
[00:04:36] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:36] Build completed unsuccessfully in 0:00:49
[00:04:36] Build completed unsuccessfully in 0:00:49
[00:04:36] Makefile:79: recipe for target 'tidy' failed
[00:04:36] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:02331a24
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:06acdfe4:start=1537884636963782349,finish=1537884636968797348,duration=5014999
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:03df68ae
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:17a3be8c
travis_time:start:17a3be8c
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:042558e4
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

rust-highfive · 2018-09-25T17:12:24Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:54:42] ....................................................................................................
[00:54:45] .......................................................i............................................
[00:54:48] ....................................................................................................
[00:54:51] ....................................................................................................
[00:54:54] ....iiiiiiiii.......................................................................................
[00:55:00] ....................................................................................................
[00:55:03] ........................................................................................i...........
[00:55:06] ....................................................................................................
[00:55:08] ................................................i.i..ii.............................................
---
[01:21:46]    Compiling rustc_driver v0.0.0 (file:///checkout/src/librustc_driver)
[01:21:48] error[E0061]: this function takes 4 parameters but 3 parameters were supplied
[01:21:48]    --> librustc_driver/test.rs:180:19
[01:21:48]     |
[01:21:48] 180 |             infcx.resolve_regions_and_report_errors(def_id, &region_scope_tree, &outlives_env);
[01:21:48] 
[01:21:48] error: aborting due to previous error
[01:21:48] 
[01:21:48] For more information about this error, try `rustc --explain E0061`.
[01:21:48] For more information about this error, try `rustc --explain E0061`.
[01:21:48] error: Could not compile `rustc_driver`.
[01:21:48] 
[01:21:48] To learn more, run the command again with --verbose.
[01:21:48] 
[01:21:48] 
[01:21:48] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" " jemalloc" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "-p" "rustc_driver" "--" "--quiet"
[01:21:48] 
[01:21:48] 
[01:21:48] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:21:48] Build completed unsuccessfully in 0:35:44
[01:21:48] Build completed unsuccessfully in 0:35:44
[01:21:48] Makefile:58: recipe for target 'check' failed
[01:21:48] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0a35de7e
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:0d641e74:start=1537895535073488418,finish=1537895535078975910,duration=5487492
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:00d8137f
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0f5e8030
travis_time:start:0f5e8030
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0a86e5b0
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

nikomatsakis · 2018-09-25T19:34:21Z

@bors r=pnkfelix

bors · 2018-09-25T19:34:21Z

📌 Commit 781a0bd825684378181f4dedf8864e600723e227 has been approved by pnkfelix

nikomatsakis · 2018-09-25T19:52:38Z

filed #54572 as a follow-up issue

bors · 2018-09-25T22:56:03Z

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

Presently unused.

This lets us remove `for_each_region` and makes things simpler.

It's a bit cleaner to just have `AnyBound` and `AllBound`, after all.

Hopefully this will help clarify the behavior in the various borrowck modes

Pacify the mercilous tidy.

nikomatsakis · 2018-09-26T13:43:02Z

@bors r=pnkfelix

bors · 2018-09-26T13:43:03Z

📌 Commit f23fd4b has been approved by pnkfelix

bors · 2018-09-26T18:17:02Z

⌛ Testing commit f23fd4b with merge c7df1f5...

@pnkfelix

…=pnkfelix rework how we handle outlives relationships When we encounter an outlives relationship involving a projection, we use to over-constrain in some cases with region constraints. We also used to evaluate whether the where-clauses in the environment might apply **before** running inference. We now avoid doing both of those things: - If there are where-clauses in the environment that might be useful, we add no constraints. - After inference is done, we check if we wound up inferring values compatible with the where-clause, and make use of them if so. I realize now that this PR includes some meandering commits and refactorings from when I expected to go in a different direction. If desired, I could try to remove some of those. Fixes #53121 Fixes #53789 r? @pnkfelix

bors · 2018-09-26T20:47:40Z

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

rust-highfive assigned pnkfelix Sep 21, 2018

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

nikomatsakis force-pushed the nll-issue-53121-shred-outlives branch from 2136fd4 to 6796c4d Compare September 21, 2018 23:34

This was referenced Sep 21, 2018

[nll] spurious outlives requirement in shred-0.7.0 #53121

Closed

[NLL] Possible proptest-arbitrary-0.2.2 regression #53789

Closed

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

pnkfelix reviewed Sep 24, 2018

View reviewed changes

nikomatsakis force-pushed the nll-issue-53121-shred-outlives branch from 6796c4d to 1598f77 Compare September 25, 2018 14:04

nikomatsakis force-pushed the nll-issue-53121-shred-outlives branch from 303f8e2 to a7d9b84 Compare September 25, 2018 15:48

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 25, 2018

nikomatsakis mentioned this pull request Sep 25, 2018

"approximate check" for outlives relations can miss matches #54572

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 25, 2018

nikomatsakis added 3 commits September 26, 2018 09:30

regionck: rustfmt

80ed62e

outlives/env: rustfmt

1fb4ea9

build up a map of the region-bound pairs for each body-id

84563da

Presently unused.

nikomatsakis added 16 commits September 26, 2018 09:38

lexical_region_resolve: rustfmt

688aaf3

make normalize work on any type-foldable

b5469c5

remove handling of verify from taintset

85d12e2

This lets us remove `for_each_region` and makes things simpler.

change to use impl Trait a bit

58fd6fa

introduce VerifyBound::IfEq (presently unused)

18b86e9

refactor away AnyRegions and AllRegions

7f8c42d

It's a bit cleaner to just have `AnyBound` and `AllBound`, after all.

use IfEq to defer equality comparison around where clauses`

dc9317f

region_infer: rustfmt

2392a09

switch to use VerifyBound instead of RegionTest

0f5dae0

make NLL handle IfEq bounds by using SCC normalization

0b4791e

convert from an UnlessNll flag to a SuppressRegionErrors flag

a13c9f6

Hopefully this will help clarify the behavior in the various borrowck modes

update tests and add stderr files

f87189d

rustfmt error_reporting/mod.rs and dropck.rs

ffe87f6

Pacify the mercilous tidy.

pacify the mercilous tidy.

e2deef3

fix rustc_driver tests

cbaf36a

rustc_driver/test.rs: rustfmt

f23fd4b

nikomatsakis force-pushed the nll-issue-53121-shred-outlives branch from 781a0bd to f23fd4b Compare September 26, 2018 13:42

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 26, 2018

bors merged commit f23fd4b into rust-lang:master Sep 26, 2018

This was referenced Sep 26, 2018

A few cleanups and minor improvements to typeck/check #54533

Merged

extend unused_parens lint to break/return () #54440

Closed

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

Merged

Replace CanonicalVar with DebruijnIndex #52984

Merged

pnkfelix mentioned this pull request Nov 12, 2018

Lifetimes bug on beta 1.31: the associated type may not live long enough #55756

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rework how we handle outlives relationships #54453

rework how we handle outlives relationships #54453

nikomatsakis commented Sep 21, 2018 •

edited

Loading

rust-highfive commented Sep 21, 2018

pnkfelix commented Sep 22, 2018

bors commented Sep 23, 2018

pnkfelix Sep 24, 2018

nikomatsakis Sep 24, 2018

nikomatsakis Sep 24, 2018

nikomatsakis Sep 24, 2018

pnkfelix Sep 24, 2018

pnkfelix Sep 24, 2018

pnkfelix Sep 24, 2018 •

edited

Loading

pnkfelix Sep 24, 2018

pnkfelix Sep 24, 2018

nikomatsakis Sep 24, 2018

pnkfelix Sep 25, 2018

pnkfelix commented Sep 24, 2018

nikomatsakis commented Sep 24, 2018

rust-highfive commented Sep 25, 2018

rust-highfive commented Sep 25, 2018

nikomatsakis commented Sep 25, 2018

bors commented Sep 25, 2018

nikomatsakis commented Sep 25, 2018

bors commented Sep 25, 2018

nikomatsakis commented Sep 26, 2018

bors commented Sep 26, 2018

bors commented Sep 26, 2018

bors commented Sep 26, 2018

		@@ -0,0 +1,26 @@
		#![feature(nll)]

		// Test that we are NOT able to establish that `<T as

rework how we handle outlives relationships #54453

rework how we handle outlives relationships #54453

Conversation

nikomatsakis commented Sep 21, 2018 • edited Loading

rust-highfive commented Sep 21, 2018

pnkfelix commented Sep 22, 2018

bors commented Sep 23, 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

pnkfelix Sep 24, 2018 • 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

pnkfelix commented Sep 24, 2018

nikomatsakis commented Sep 24, 2018

rust-highfive commented Sep 25, 2018

rust-highfive commented Sep 25, 2018

nikomatsakis commented Sep 25, 2018

bors commented Sep 25, 2018

nikomatsakis commented Sep 25, 2018

bors commented Sep 25, 2018

nikomatsakis commented Sep 26, 2018

bors commented Sep 26, 2018

bors commented Sep 26, 2018

bors commented Sep 26, 2018

nikomatsakis commented Sep 21, 2018 •

edited

Loading

pnkfelix Sep 24, 2018 •

edited

Loading