[nll] borrows that must be valid for a free lifetime should explain why #54229
Conversation
This comment has been minimized.
This comment has been minimized.
davidtwco
force-pushed
the
issue-52534
branch
from
2a59d2a
to
4951ce1
Compare
This comment has been minimized.
This comment has been minimized.
davidtwco
force-pushed
the
issue-52534
branch
from
4951ce1
to
4b1c2a1
Compare
davidtwco
force-pushed
the
issue-52534
branch
from
4b1c2a1
to
53b03f6
Compare
This comment has been minimized.
This comment has been minimized.
bors
added
the
S-waiting-on-author
davidtwco
force-pushed
the
issue-52534
branch
from
53b03f6
to
dbc105a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
davidtwco
force-pushed
the
issue-52534
branch
from
dbc105a
to
e210978
Compare
This comment has been minimized.
This comment has been minimized.
davidtwco
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
davidtwco
force-pushed
the
issue-52534
branch
from
1978c5d
to
078fa7f
Compare
|
Rebased. |
davidtwco
changed the title
| @@ -1,17 +1,19 @@ | |||
| error[E0597]: `x` does not live long enough | |||
| --> $DIR/issue-30438-c.rs:19:5 | |||
| | | |||
| LL | fn silly<'y, 'z>(_s: &'y Test<'z>) -> &'y <Test<'z> as Trait>::Out where 'z: 'static { | |||
| | ------------ ---------------------------- has type `&'y <Test<'z> as Trait>::Out` | |||
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
| // | ||
| // If there is more than one argument and this error is being reported, that | ||
| // means there must be a self parameter - as otherwise there would be an error | ||
| // from lifetime elision and not this. So we highlight the self parameter. |
There was a problem hiding this comment.
I don't follow this logic 100% — this is true if the region in question is anonymous, but that's not known here, right?
There was a problem hiding this comment.
And this only looks at the first lifetime?
There was a problem hiding this comment.
I've updated the logic to handle this case correctly.
| LL | fn foo<'a>(x: &'a (u32,)) -> &'a u32 { | ||
| | ---------- ------- has type `&'a u32` | ||
| | | | ||
| | has type `&'a (u32,)` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| pub fn has_name(&self) -> bool { | ||
| match *self { | ||
| RegionKind::ReEarlyBound(ebr) => ebr.has_name(), | ||
| RegionKind::ReLateBound(_, br) => br.is_named(), |
There was a problem hiding this comment.
Aside: The difference above is... unfortunate.
There was a problem hiding this comment.
I'm not sure I understand what you mean?
This comment has been minimized.
This comment has been minimized.
bors
added
S-waiting-on-bors
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
force-pushed
the
issue-52534
branch
from
125607f
to
b342f00
Compare
|
@bors r=pnkfelix |
|
📌 Commit b342f00 has been approved by |
[nll] borrows that must be valid for a free lifetime should explain why Fixes #52534. r? @nikomatsakis
|
☀️ Test successful - status-appveyor, status-travis |
|
|
||
| match reason { | ||
| Liveness { local, location, in_loop } => { | ||
| match find_use::find(mir, regioncx, tcx, region_sub, context.loc) { |
There was a problem hiding this comment.
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..." 😆
Fixes #52534.
r? @nikomatsakis