New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Oct 10, 2018

Conversation

Projects
None yet
6 participants
@davidtwco
Member

davidtwco commented Oct 3, 2018

Fixes #53040.

r? @nikomatsakis

@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.

Show resolved Hide resolved src/test/ui/nll/issue-53040.stderr Outdated
| ||
| |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 | | }

This comment has been minimized.

@nikomatsakis

nikomatsakis Oct 4, 2018

Contributor

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

@nikomatsakis

nikomatsakis Oct 4, 2018

Contributor

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

This comment has been minimized.

@davidtwco

davidtwco Oct 4, 2018

Member

Pushed a change with this.

@davidtwco

davidtwco Oct 4, 2018

Member

Pushed a change with this.

This comment has been minimized.

@pnkfelix

pnkfelix Oct 8, 2018

Member

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

pnkfelix Oct 8, 2018

Member

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...)

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

@memoryruins memoryruins added the A-NLL label Oct 6, 2018

/// 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

This comment has been minimized.

@pnkfelix

pnkfelix Oct 8, 2018

Member

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 ?

@pnkfelix

pnkfelix Oct 8, 2018

Member

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 ?

This comment has been minimized.

@pnkfelix

pnkfelix Oct 8, 2018

Member

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

@pnkfelix

pnkfelix Oct 8, 2018

Member

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

This comment has been minimized.

@davidtwco

davidtwco Oct 8, 2018

Member

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.

@davidtwco

davidtwco Oct 8, 2018

Member

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

This comment has been minimized.

Show comment
Hide comment
@pnkfelix
Member

pnkfelix commented Oct 8, 2018

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 8, 2018

Contributor

📌 Commit b5ba1a0 has been approved by pnkfelix

Contributor

bors commented Oct 8, 2018

📌 Commit b5ba1a0 has been approved by pnkfelix

@bors

This comment was marked as resolved.

Show comment
Hide comment
@bors

bors Oct 9, 2018

Contributor

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

Contributor

bors commented Oct 9, 2018

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

davidtwco added some commits Oct 3, 2018

Improve errors for `FnMut` closures.
This commit improves the errors for `FnMut` closures where a reference
to a captured variable is escaping.
Improve message for closure returning a closure.
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

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Oct 9, 2018

Member

@bors r=pnkfelix

Member

davidtwco commented Oct 9, 2018

@bors r=pnkfelix

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 9, 2018

Contributor

📌 Commit 98633b4 has been approved by pnkfelix

Contributor

bors commented Oct 9, 2018

📌 Commit 98633b4 has been approved by pnkfelix

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 10, 2018

Contributor

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

Contributor

bors commented Oct 10, 2018

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

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

Auto merge of #54802 - davidtwco:issue-53040, r=pnkfelix
[nll] better error message when returning refs to upvars

Fixes #53040.

r? @nikomatsakis
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 10, 2018

Contributor

💔 Test failed - status-travis

Contributor

bors commented Oct 10, 2018

💔 Test failed - status-travis

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Oct 10, 2018

Collaborator

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)

Collaborator

rust-highfive commented Oct 10, 2018

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

This comment has been minimized.

Show comment
Hide comment
@davidtwco
Member

davidtwco commented Oct 10, 2018

@bors retry

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 10, 2018

Contributor

⌛️ Testing commit 98633b4 with merge e1041c6...

Contributor

bors commented Oct 10, 2018

⌛️ Testing commit 98633b4 with merge e1041c6...

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

Auto merge of #54802 - davidtwco:issue-53040, r=pnkfelix
[nll] better error message when returning refs to upvars

Fixes #53040.

r? @nikomatsakis
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 10, 2018

Contributor

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

Contributor

bors commented Oct 10, 2018

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

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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@davidtwco davidtwco deleted the davidtwco:issue-53040 branch Oct 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment