[NLL] Returns are interesting for free regions #53175

matthewjasper · 2018-08-07T21:25:12Z

Based on #53088 - creating now to get feedback.

Closes #51175

Make assigning to the return type interesting.
Use "returning this value" instead of "return" in error messages.
Prefer one of the explanations that we have a name for to a generic interesting cause in some cases.
Treat causes that involve the destination of a call like assignments.

rust-highfive · 2018-08-07T21:25:15Z

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

matthewjasper · 2018-08-07T21:47:22Z

r? @nikomatsakis

nikomatsakis

I'm happy with this, apart from wanting a comment. The results seem great.

nikomatsakis · 2018-08-08T18:39:33Z

src/test/ui/error-codes/E0621-does-not-trigger-for-closures.nll.stderr

-   |                |   |     requires that `'1` must outlive `'2`
-   |                |   has type `&'1 i32`
-   |                lifetime `'2` appears in return type
+   |                    --                       ^ returning this value requires that `'1` must outlive `'2`


nikomatsakis · 2018-08-08T18:41:06Z

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

   |
 LL |     pub fn new(lexer: &'a mut Lexer) -> Parser<'a> {
   |                ----- consider changing the type of `lexer` to `&'a mut Lexer<'a>`
 LL |         Parser { lexer: lexer }
-   |                         ^^^^^ lifetime `'a` required
+   |         ^^^^^^^^^^^^^^^^^^^^^^^ lifetime `'a` required


These seem less good but still "tolerable" — I think with a bit more thought we should be able to narrow back down to the lexer expression. The problem is that we still have trouble distinguishing "intermediate, passthrough" regions (like the one on Parser) from the "source regions" (the ones on lexer). That's ok for now though I think.

nikomatsakis · 2018-08-08T18:41:20Z

src/test/ui/issue-40510-1.nll.stderr

+   |     |return type of closure is &'2 mut std::boxed::Box<()>
+   |     lifetime `'1` represents this closure's body
+LL |         &mut x
+   |         ^^^^^^ returning this value requires that `'1` must outlive `'2`


Clear win. Nice!

nikomatsakis · 2018-08-08T18:42:02Z

src/test/ui/lifetime-errors/ex1-return-one-existing-name-if-else-3.nll.stderr

-   |            consider changing type to `(&'a i32, &'a i32)`
+   |            ------ consider changing type to `(&'a i32, &'a i32)`
+LL |     if x > y { x } else { y } //~ ERROR explicit lifetime
+   |                           ^ lifetime `'a` required


nikomatsakis · 2018-08-08T18:43:28Z

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

@@ -42,7 +42,7 @@ impl fmt::Display for ConstraintCategory {
        // Must end with a space. Allows for empty names to be provided.
        match self {
            ConstraintCategory::Assignment => write!(f, "assignment "),
-            ConstraintCategory::Return => write!(f, "return "),
+            ConstraintCategory::Return => write!(f, "returning this value "),


I like how this reads, though it's less "concise", which I think some people value. Still, seems ok to me.

nikomatsakis · 2018-08-08T18:46:36Z

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

@@ -254,7 +258,22 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                debug!("classify_constraint: terminator.kind={:?}", terminator.kind);
                match terminator.kind {
                    TerminatorKind::DropAndReplace { .. } => ConstraintCategory::Assignment,
-                    TerminatorKind::Call { .. } => ConstraintCategory::CallArgument,
+                    TerminatorKind::Call { destination: Some((ref place, _)), .. } => {
+                        if tcx.any_free_region_meets(


I'd like a comment here... something lke:

Classify calls differently depending on whether or not the sub region appears in the return type. If the return type contains the sub-region, then this is either an assignment or a return, depending on whether we are writing to the RETURN_PLACE or not. The idea here is that the region is being propagated from an input into the output place, so it's a kind of assignment. Otherwise, if the sub-region only appears in the argument types, then use the CallArgument classification.

nikomatsakis · 2018-08-08T18:47:10Z

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

+            infcx.extract_type_name(&return_ty)
+        });
+
+                let mir_node_id = tcx.hir.as_local_node_id(mir_def_id).expect("non-local mir");


Nit: wild indentation

matthewjasper · 2018-08-08T22:01:29Z

Rebased, some tests were changed in the mean time. Added another commit where we had lifetimes in some messages the wrong way around.

nikomatsakis · 2018-08-09T20:29:55Z

@bors r+

bors · 2018-08-09T20:29:56Z

📌 Commit 49cf4e1e49e8ef5527148fa663d47ace4a30f89c has been approved by nikomatsakis

matthewjasper · 2018-08-14T20:28:03Z

Rebased and tests updated

nikomatsakis · 2018-08-16T17:19:39Z

@bors r+

bors · 2018-08-16T17:19:39Z

📌 Commit a19db49 has been approved by nikomatsakis

bors · 2018-08-17T02:44:22Z

⌛ Testing commit a19db49 with merge 5cbed2c8cc23b11aee0543296770e77137070580...

bors · 2018-08-17T02:50:10Z

💔 Test failed - status-travis

bors · 2018-08-17T07:20:17Z

💔 Test failed - status-travis

rust-highfive · 2018-08-17T07:20:19Z

The job dist-arm-linux 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.

travis_time:end:0552a9fe:start=1534490371244013800,finish=1534490371253802880,duration=9789080
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:04409332
$ 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 -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:1287bcaa
travis_time:start:1287bcaa
$ 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:03b37b1e
$ 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)

kennytm · 2018-08-17T07:42:18Z

@bors retry travis-ci/travis-ci#9696

bors · 2018-08-17T09:41:18Z

⌛ Testing commit a19db49 with merge 3087520eac999cbb0b528b0985381a0b5b33e80c...

bors · 2018-08-17T10:40:20Z

💔 Test failed - status-travis

rust-highfive · 2018-08-17T10:40:22Z

The job x86_64-gnu-aux 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:52:37] [RUSTC-TIMING] tempfile test:false 1.138
[00:52:38] [RUSTC-TIMING] pulldown_cmark test:false 8.429
[00:52:38]    Compiling rustdoc v0.0.0 (file:///checkout/src/librustdoc)

Broadcast message from root@travis-job-8b0b114a-6c82-4d24-91e8-2e146428c76d
 (unknown) at 10:38 ...
The system is going down for power off NOW!
[00:53:55] 
[00:53:55] Session terminated, terminating shell... ...terminated.
[00:53:55] Makefile:28: recipe for target 'all' failed
[00:53:55] make: *** [all] Terminated

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 143.
travis_time:start:0469b5e3
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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)

kennytm · 2018-08-17T12:08:40Z

@bors retry travis-ci/travis-ci#4924

bors · 2018-08-17T13:35:44Z

⌛ Testing commit a19db49 with merge 5c194343d069ce4839b85c1208b434e48afe4146...

bors · 2018-08-17T14:16:48Z

💔 Test failed - status-travis

rust-highfive · 2018-08-17T14:16:51Z

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:37:57] [TIMING] Rustdoc { host: "x86_64-unknown-linux-gnu" } -- 86.604
[00:37:57] Build completed successfully in 0:32:38
[00:37:57]     Finished dev [unoptimized] target(s) in 0.25s
[00:37:58] thread 'main' panicked at 'fs::read_dir(builder.src.join("src/doc/book/redirects")) failed with No such file or directory (os error 2)', bootstrap/doc.rs:294:21
[00:37:58] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap doc
[00:37:58] Build completed unsuccessfully in 0:00:01
[00:37:58] Makefile:28: recipe for target 'all' failed
[00:37:58] Makefile:28: recipe for target 'all' failed
[00:37:58] make: *** [all] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0982019a
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:217c58a0:start=1534515331016287719,finish=1534515331023178740,duration=6891021
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:03596451
$ 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 -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:063a9c78
travis_time:start:063a9c78
$ 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:05bd3d6c
$ 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)

kennytm · 2018-08-18T10:27:12Z

@bors retry

[00:01:23] Submodule path 'src/tools/rustfmt': checked out '5c9a2b6c13d3b6f8d3f9c02b130bb4b54fd489fb'
[00:02:00] 
[00:02:00] gzip: stdin: not in gzip format
[00:02:00] tar: Child returned status 1
[00:02:00] tar: Error is not recoverable: exiting now

bors · 2018-08-18T11:57:53Z

⌛ Testing commit a19db49 with merge 7de3dea...

[NLL] Returns are interesting for free regions Based on #53088 - creating now to get feedback. Closes #51175 * Make assigning to the return type interesting. * Use "returning this value" instead of "return" in error messages. * Prefer one of the explanations that we have a name for to a generic interesting cause in some cases. * Treat causes that involve the destination of a call like assignments.

bors · 2018-08-18T14:35:35Z

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

rust-highfive assigned cramertj Aug 7, 2018

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

rust-highfive assigned nikomatsakis and unassigned cramertj Aug 7, 2018

nikomatsakis requested changes Aug 8, 2018

View reviewed changes

matthewjasper force-pushed the more-return-stuff branch from 9586af6 to 0cecd38 Compare August 8, 2018 21:58

This comment has been minimized.

Sign in to view

matthewjasper force-pushed the more-return-stuff branch from 0cecd38 to 49cf4e1 Compare August 8, 2018 22:21

nikomatsakis approved these changes Aug 9, 2018

View reviewed changes

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 Aug 9, 2018

matthewjasper added 7 commits August 14, 2018 20:35

Make assigning to the return type interesting

092f03a

Treat assigning the destination of a call as an assignment

74ce76a

Change "return" to "returning this value" when explaining free regions

7710d95

Tweek best_blame_constraint to slightly prefer specific explanations

372e4ae

Update tests

371c23f

Name the right expected and actual return lifetimes

3c8eb4e

Update former compile-fail tests

a19db49

matthewjasper force-pushed the more-return-stuff branch from 49cf4e1 to a19db49 Compare August 14, 2018 20:27

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

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 Aug 17, 2018

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

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 Aug 17, 2018

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

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 Aug 18, 2018

bors merged commit a19db49 into rust-lang:master Aug 18, 2018

matthewjasper deleted the more-return-stuff branch August 18, 2018 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NLL] Returns are interesting for free regions #53175

[NLL] Returns are interesting for free regions #53175

matthewjasper commented Aug 7, 2018 •

edited

Loading

rust-highfive commented Aug 7, 2018

matthewjasper commented Aug 7, 2018

nikomatsakis left a comment

nikomatsakis Aug 8, 2018

nikomatsakis Aug 8, 2018

nikomatsakis Aug 8, 2018

nikomatsakis Aug 8, 2018

nikomatsakis Aug 8, 2018

nikomatsakis Aug 8, 2018

nikomatsakis Aug 8, 2018

matthewjasper commented Aug 8, 2018 •

edited

Loading

This comment has been minimized.

nikomatsakis commented Aug 9, 2018

bors commented Aug 9, 2018

matthewjasper commented Aug 14, 2018

nikomatsakis commented Aug 16, 2018

bors commented Aug 16, 2018

bors commented Aug 17, 2018

bors commented Aug 17, 2018

bors commented Aug 17, 2018

rust-highfive commented Aug 17, 2018

kennytm commented Aug 17, 2018

bors commented Aug 17, 2018

bors commented Aug 17, 2018

rust-highfive commented Aug 17, 2018

kennytm commented Aug 17, 2018

bors commented Aug 17, 2018

bors commented Aug 17, 2018

rust-highfive commented Aug 17, 2018

kennytm commented Aug 18, 2018

bors commented Aug 18, 2018

bors commented Aug 18, 2018

[NLL] Returns are interesting for free regions #53175

[NLL] Returns are interesting for free regions #53175

Conversation

matthewjasper commented Aug 7, 2018 • edited Loading

rust-highfive commented Aug 7, 2018

matthewjasper commented Aug 7, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Aug 8, 2018

Choose a reason for hiding this comment

nikomatsakis Aug 8, 2018

Choose a reason for hiding this comment

nikomatsakis Aug 8, 2018

Choose a reason for hiding this comment

nikomatsakis Aug 8, 2018

Choose a reason for hiding this comment

nikomatsakis Aug 8, 2018

Choose a reason for hiding this comment

nikomatsakis Aug 8, 2018

Choose a reason for hiding this comment

nikomatsakis Aug 8, 2018

Choose a reason for hiding this comment

matthewjasper commented Aug 8, 2018 • edited Loading

This comment has been minimized.

nikomatsakis commented Aug 9, 2018

bors commented Aug 9, 2018

matthewjasper commented Aug 14, 2018

nikomatsakis commented Aug 16, 2018

bors commented Aug 16, 2018

bors commented Aug 17, 2018

bors commented Aug 17, 2018

bors commented Aug 17, 2018

rust-highfive commented Aug 17, 2018

kennytm commented Aug 17, 2018

bors commented Aug 17, 2018

bors commented Aug 17, 2018

rust-highfive commented Aug 17, 2018

kennytm commented Aug 17, 2018

bors commented Aug 17, 2018

bors commented Aug 17, 2018

rust-highfive commented Aug 17, 2018

kennytm commented Aug 18, 2018

bors commented Aug 18, 2018

bors commented Aug 18, 2018

matthewjasper commented Aug 7, 2018 •

edited

Loading

matthewjasper commented Aug 8, 2018 •

edited

Loading