[nll] borrows that must be valid for a free lifetime should explain why #54229

davidtwco · 2018-09-14T13:48:37Z

Fixes #52534.

r? @nikomatsakis

src/test/ui/regions/regions-nested-fns-2.nll.stderr

nikomatsakis · 2018-09-14T14:41:31Z

r? @nikomatsakis

src/librustc_mir/borrow_check/error_reporting.rs

src/test/ui/issues/issue-30438-c.nll.stderr

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

davidtwco · 2018-09-18T10:44:37Z

Rebased.

src/test/ui/regions/regions-nested-fns-2.nll.stderr

nikomatsakis

Seems good overall

src/librustc/util/ppaux.rs

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

src/test/ui/issues/issue-30438-c.nll.stderr

question: do you think it's good to talk about 'z here? I'm torn.

(Also, man, it'd be nice if we could just underline 'y here.)

I've changed this so that it only underlines the 'y. Looking into the types to work out what the 'z is used for seems like it would be challenging.

src/librustc_mir/borrow_check/error_reporting.rs

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

src/librustc_mir/borrow_check/error_reporting.rs

I don't follow this logic 100% — this is true if the region in question is anonymous, but that's not known here, right?

And this only looks at the first lifetime?

I've updated the logic to handle this case correctly.

nikomatsakis · 2018-09-18T16:32:00Z

src/test/ui/nll/borrowed-universal-error-2.stderr

I'm a bit torn about highlighting x here. I mean, it doesn't turn out to matter what type x has, right? Though maybe that is hard for us to see right now.

The intention with highlighting the argument is to show that through the elision rules, it is expected to be the same lifetime as the return type. This isn't useful in this case where the argument and return type have the lifetimes specified.

I've added a note that should clarify this intent for the cases where elision rules are relevant and a help for the named lifetime cases.

src/librustc_mir/borrow_check/error_reporting.rs

estebank · 2018-09-19T00:17:50Z

src/librustc/ty/sty.rs

Aside: The difference above is... unfortunate.

I'm not sure I understand what you mean?

has_name vs is_named

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

estebank

found 'em

src/librustc_mir/borrow_check/error_reporting.rs

Previously, explain_borrow would emit an error with the explanation of the a borrow. Now, it returns a enum with what the explanation for the borrow is and any relevant spans or information such that the calling code can choose to emit the same note/suggestion as before by calling the emit method on the new enum.

Previously, region naming would always highlight the source of the region name it found. Now, region naming returns the name as part of a larger structure that encodes the source of the region naming such that a region name can be optionally added to the diagnostic.

For cases where there are references in the parameters and in the the outputs that do not match, and where no closures are involved, this commit introduces an improved error that mentions (or synthesizes) a name for the regions involved to better illustrate why the borrow does not live long enough.

Adds improved messages for closures where returned type does not match the inferred return lifetime of the closure.

Start mentioning function name that the variable is valid within in notes to provide context.

New test has multiple parameters in a closure with longer names in order to clarify the issues relating to odd spans.

Fixes the off-by-one span issue where closure argument spans were pointing to the token after the argument.

This error can only occur within a function when a borrow of data owned within the function is returned; and when there are arguments that could have been returned instead. Therefore, it is always applicable to add a specific note that links to the relevant rust documentation about dangling references.

Changed `highlight_region_with_region` function(s) to `highlight_region_with_bound_region` to be more specific and less ambigious.

Enhances annotation logic to properly consider named lifetimes where lifetime elision rules that were previously implemented would not apply. Further, adds new help and note messages to diagnostics and highlights only lifetime when dealing with named lifetimes.

Error now correctly checks whether the borrow that does not live long enough is being returned before annotating the error with the arguments and return type from the signature - as this would not be relevant if the borrow was not being returned.

davidtwco · 2018-09-23T12:44:03Z

@bors r=pnkfelix

bors · 2018-09-23T12:44:04Z

📌 Commit b342f00 has been approved by pnkfelix

bors · 2018-09-23T15:01:00Z

⌛ Testing commit b342f00 with merge f49f6e7...

@nikomatsakis

[nll] borrows that must be valid for a free lifetime should explain why Fixes #52534. r? @nikomatsakis

bors · 2018-09-23T17:49:12Z

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

…ed by PR rust-lang#54164.

pnkfelix · 2018-09-28T09:35:56Z

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

-
-        match reason {
-            Liveness { local, location, in_loop } => {
+         match find_use::find(mir, regioncx, tcx, region_sub, context.loc) {


pnkfelix's brain, five days after code lands: "Hey, this isn't indented right... hmm and maybe I might have chosen a name besides emit for that method over there... who reviewed this, anyway !?!?! Oh. Oh. It was me. I reviewed it..." 😆

cc rust-lang/rust#54229.

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

This comment has been minimized.

Sign in to view

davidtwco force-pushed the issue-52534 branch from 2a59d2a to 4951ce1 Compare September 14, 2018 14:01

This comment has been minimized.

Sign in to view

davidtwco force-pushed the issue-52534 branch from 4951ce1 to 4b1c2a1 Compare September 14, 2018 14:22

csmoe reviewed Sep 14, 2018

View reviewed changes

src/test/ui/regions/regions-nested-fns-2.nll.stderr Outdated Show resolved Hide resolved

rust-highfive assigned nikomatsakis Sep 14, 2018

davidtwco force-pushed the issue-52534 branch from 4b1c2a1 to 53b03f6 Compare September 14, 2018 14:52

estebank reviewed Sep 14, 2018

View reviewed changes

src/librustc_mir/borrow_check/error_reporting.rs Outdated Show resolved Hide resolved

estebank reviewed Sep 14, 2018

View reviewed changes

src/test/ui/issues/issue-30438-c.nll.stderr Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 14, 2018

davidtwco force-pushed the issue-52534 branch from 53b03f6 to dbc105a Compare September 14, 2018 21:16

This comment has been minimized.

Sign in to view

davidtwco force-pushed the issue-52534 branch from dbc105a to e210978 Compare September 16, 2018 13:34

This comment has been minimized.

Sign in to view

estebank reviewed Sep 17, 2018

View reviewed changes

src/test/ui/nll/issue-52534-1.stderr Outdated Show resolved Hide resolved

davidtwco added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 18, 2018

This comment has been minimized.

Sign in to view

davidtwco force-pushed the issue-52534 branch from 1978c5d to 078fa7f Compare September 18, 2018 10:43

davidtwco changed the title ~~WIP: [nll] borrows that must be valid for a free lifetime should explain why~~ [nll] borrows that must be valid for a free lifetime should explain why Sep 18, 2018

nikomatsakis reviewed Sep 18, 2018

View reviewed changes

src/test/ui/regions/regions-nested-fns-2.nll.stderr Outdated Show resolved Hide resolved

nikomatsakis requested changes Sep 18, 2018

View reviewed changes

estebank reviewed Sep 19, 2018

View reviewed changes

src/test/ui/nll/issue-52534.stderr Outdated Show resolved Hide resolved

src/test/ui/nll/issue-52534.stderr Outdated Show resolved Hide resolved

estebank reviewed Sep 19, 2018

View reviewed changes

src/librustc_mir/borrow_check/error_reporting.rs Outdated Show resolved Hide resolved

src/librustc_mir/borrow_check/error_reporting.rs Outdated Show resolved Hide resolved

src/librustc_mir/borrow_check/error_reporting.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

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

davidtwco added 12 commits September 23, 2018 13:32

Refactor MirBorrowckCtxt to take infcx instead of tcx.

650a61c

Improve borrow errors for closures.

9eb8d11

Adds improved messages for closures where returned type does not match the inferred return lifetime of the closure.

Improve 'dropped here' note.

876774b

Start mentioning function name that the variable is valid within in notes to provide context.

Added multiple parameter closure test.

97bbcab

New test has multiple parameters in a closure with longer names in order to clarify the issues relating to odd spans.

Fixed off-by-one span.

3becbbc

Fixes the off-by-one span issue where closure argument spans were pointing to the token after the argument.

Renamed ppaux highlight region hook.

0eabba8

Changed `highlight_region_with_region` function(s) to `highlight_region_with_bound_region` to be more specific and less ambigious.

Only annotate if borrow is returned.

b342f00

Error now correctly checks whether the borrow that does not live long enough is being returned before annotating the error with the arguments and return type from the signature - as this would not be relevant if the borrow was not being returned.

davidtwco force-pushed the issue-52534 branch from 125607f to b342f00 Compare September 23, 2018 12:43

bors added a commit that referenced this pull request Sep 23, 2018

Auto merge of #54229 - davidtwco:issue-52534, r=pnkfelix

f49f6e7

[nll] borrows that must be valid for a free lifetime should explain why Fixes #52534. r? @nikomatsakis

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

This was referenced Sep 23, 2018

[NLL] Be more permissive when checking access due to Match #53438

Merged

[NLL] Rework checking for borrows conflicting with drops #54509

Merged

NLL: Improve move error loop detection (was "First shot at #54015") #54343

Merged

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Sep 24, 2018

Updated test added from PR rust-lang#54229 to account for changes add…

cd36fcf

…ed by PR rust-lang#54164.

pnkfelix reviewed Sep 28, 2018

View reviewed changes

davidtwco deleted the issue-52534 branch September 28, 2018 09:53

topecongiro added a commit to topecongiro/rustfmt that referenced this pull request Oct 6, 2018

Fix empty types being inserted to closure

003fc73

cc rust-lang/rust#54229.

topecongiro mentioned this pull request Oct 6, 2018

Cargo update rust-lang/rustfmt#3079

Merged

[nll] borrows that must be valid for a free lifetime should explain why #54229

[nll] borrows that must be valid for a free lifetime should explain why #54229

Uh oh!

Conversation

davidtwco commented Sep 14, 2018

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

nikomatsakis commented Sep 14, 2018

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

This comment has been minimized.

davidtwco commented Sep 18, 2018

Uh oh!

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

estebank left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

davidtwco commented Sep 23, 2018

Uh oh!

bors commented Sep 23, 2018

Uh oh!

bors commented Sep 23, 2018

Uh oh!

bors commented Sep 23, 2018

Uh oh!

pnkfelix Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

pnkfelix Sep 28, 2018 •

edited

Loading