[nll] better error message when returning refs to upvars #54802

davidtwco · 2018-10-03T23:52:29Z

davidtwco

This PR achieves (approximately) the intended error from the issue for that test case but I'm not convinced it improves all of the other tests that it is affecting.

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

src/test/ui/borrowck/borrowck-describe-lvalue.rs

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

nikomatsakis · 2018-10-04T19:03:09Z

src/test/ui/issues/issue-40510-3.nll.stderr

-   |       ||
-   |       |return type of closure is [closure@$DIR/issue-40510-3.rs:18:9: 20:10 x:&'2 mut std::vec::Vec<()>]
-   |       lifetime `'1` represents this closure's body
+   |        - inferred to be a `FnMut` closure
 LL | /         || {
 LL | |             x.push(())
 LL | |         }


Here I suspect this will be a bit confusing because two closures are in play. We actually can probably tell that — if the statement is an assignment with a closure rvalue, then there is an "inner" and an "outer" closure, and maybe we can adjust the message to say something like:

creates a closure that captures x by reference; this inner closure is then returned from the outer closure, allowing x to escape the closure body

that wording doesn't feel 💯, though, and I don't know if it's easy to find the name x (we don't seem to be using it in other messages)

Maybe something like this?

creates a closure that contains a reference to a captured variable; this inner closure then escapes the closure body

Pushed a change with this.

Note that @nikomatsakis was suggesting in particular the phrases "inner closure" and "outer closure", in that there are two closures in play and he wanted some way that the diagnostic would distinguish between the two when discussing them.

I don't see any use of the word "inner" in the diff, so I don't think you quite encoded exactly what @nikomatsakis was asking for.

(But I also do think that what you've done is a vast improvement over what we had before...)

pnkfelix · 2018-10-08T15:01:01Z

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

+    /// Adds a suggestion to errors where a `impl Trait` is returned.
+    ///
+    /// ```text
+    /// help: to allow this impl Trait to capture borrowed data with lifetime `'1`, add `'_` as


note to self: are we literally saying '_ when we mean it as a placeholder for a concrete lifetime like 'a? Or is it just supposed to be a replacement for the default of 'static ?

note to self and to @davidtwco : Wait, so now I don't get a "Submit review" prompt? what the hey...?

This comment annotates a function that was added in a previous PR - I just added it in this PR because I noticed it was missing.

IIRC from that PR, we suggest a named lifetime if we have one and '_ otherwise.

pnkfelix · 2018-10-08T21:20:35Z

@bors r+

bors · 2018-10-08T21:20:36Z

📌 Commit b5ba1a0acb74c235cbe06a70ef6c5c1c2e2c0dda has been approved by pnkfelix

This commit improves the errors for `FnMut` closures where a reference to a captured variable is escaping.

Now when a `FnMut` closure is returning a closure that contains a reference to a captured variable, we provide an error that makes it more clear what is happening.

davidtwco · 2018-10-09T11:23:21Z

@bors r=pnkfelix

bors · 2018-10-09T11:23:21Z

📌 Commit 98633b4 has been approved by pnkfelix

bors · 2018-10-10T04:46:01Z

⌛ Testing commit 98633b4 with merge 9930347c09c1daa97593944478864af1ebca033d...

bors · 2018-10-10T06:36:07Z

💔 Test failed - status-travis

rust-highfive · 2018-10-10T06:36:08Z

The job dist-x86_64-linux-alt 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:15019c02:start=1539153340572864547,finish=1539153340581263607,duration=8399060
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:004b4524
$ 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:3210cfea
travis_time:start:3210cfea
$ 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:11d4f9d4
$ 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)

davidtwco · 2018-10-10T07:03:24Z

@bors retry

bors · 2018-10-10T14:51:07Z

⌛ Testing commit 98633b4 with merge e1041c6...

@nikomatsakis

[nll] better error message when returning refs to upvars Fixes #53040. r? @nikomatsakis

bors · 2018-10-10T17:30:41Z

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

rust-highfive assigned nikomatsakis Oct 3, 2018

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

davidtwco commented Oct 3, 2018

View reviewed changes

src/test/ui/issues/issue-49824.nll.stderr Show resolved Hide resolved

pnkfelix reviewed Oct 4, 2018

View reviewed changes

src/test/ui/borrowck/borrowck-describe-lvalue.rs Show resolved Hide resolved

pnkfelix mentioned this pull request Oct 4, 2018

NLL: need a (new) round of review of deltas between .stderr and .nll.stderr output #52663

Closed

nikomatsakis reviewed Oct 4, 2018

View reviewed changes

davidtwco changed the title ~~WIP: [nll] better error message when returning refs to upvars~~ [nll] better error message when returning refs to upvars Oct 5, 2018

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

pnkfelix reviewed Oct 8, 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 Oct 8, 2018

This comment has been minimized.

Sign in to view

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

davidtwco added 2 commits October 9, 2018 12:31

Improve errors for FnMut closures.

c65e119

This commit improves the errors for `FnMut` closures where a reference to a captured variable is escaping.

Improve message for closure returning a closure.

98633b4

Now when a `FnMut` closure is returning a closure that contains a reference to a captured variable, we provide an error that makes it more clear what is happening.

davidtwco force-pushed the issue-53040 branch from b5ba1a0 to 98633b4 Compare October 9, 2018 11:22

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 Oct 9, 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 Oct 10, 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 Oct 10, 2018

bors added a commit that referenced this pull request Oct 10, 2018

Auto merge of #54802 - davidtwco:issue-53040, r=pnkfelix

e1041c6

[nll] better error message when returning refs to upvars Fixes #53040. r? @nikomatsakis

bors merged commit 98633b4 into rust-lang:master Oct 10, 2018

davidtwco deleted the issue-53040 branch October 10, 2018 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[nll] better error message when returning refs to upvars #54802

[nll] better error message when returning refs to upvars #54802

davidtwco commented Oct 3, 2018

davidtwco left a comment

nikomatsakis Oct 4, 2018

davidtwco Oct 4, 2018

pnkfelix Oct 8, 2018

pnkfelix Oct 8, 2018

pnkfelix Oct 8, 2018

davidtwco Oct 8, 2018 •

edited

Loading

pnkfelix commented Oct 8, 2018

bors commented Oct 8, 2018

This comment has been minimized.

davidtwco commented Oct 9, 2018

bors commented Oct 9, 2018

bors commented Oct 10, 2018

bors commented Oct 10, 2018

rust-highfive commented Oct 10, 2018

davidtwco commented Oct 10, 2018

bors commented Oct 10, 2018

bors commented Oct 10, 2018

[nll] better error message when returning refs to upvars #54802

[nll] better error message when returning refs to upvars #54802

Conversation

davidtwco commented Oct 3, 2018

davidtwco left a comment

Choose a reason for hiding this comment

nikomatsakis Oct 4, 2018

Choose a reason for hiding this comment

davidtwco Oct 4, 2018

Choose a reason for hiding this comment

pnkfelix Oct 8, 2018

Choose a reason for hiding this comment

pnkfelix Oct 8, 2018

Choose a reason for hiding this comment

pnkfelix Oct 8, 2018

Choose a reason for hiding this comment

davidtwco Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

pnkfelix commented Oct 8, 2018

bors commented Oct 8, 2018

This comment has been minimized.

davidtwco commented Oct 9, 2018

bors commented Oct 9, 2018

bors commented Oct 10, 2018

bors commented Oct 10, 2018

rust-highfive commented Oct 10, 2018

davidtwco commented Oct 10, 2018

bors commented Oct 10, 2018

bors commented Oct 10, 2018

davidtwco Oct 8, 2018 •

edited

Loading