Skip to content

Add beginner friendly lifetime elision hint to E0623 - #90179

Merged
bors merged 1 commit into
rust-lang:masterfrom
Noratrieb:lifetime-elision-mismatch-hint
Nov 4, 2021
Merged

Add beginner friendly lifetime elision hint to E0623#90179
bors merged 1 commit into
rust-lang:masterfrom
Noratrieb:lifetime-elision-mismatch-hint

Conversation

@Noratrieb

@NoratriebNoratrieb commented Oct 22, 2021

Copy link
Copy Markdown
Member

Address #90170

Suggest adding a new lifetime parameter when two elided lifetimes should match up but don't.

Example:

error[E0623]: lifetime mismatch
--> $DIR/issue-90170-elision-mismatch.rs:2:35
|
LL | fn foo(slice_a: &mut [u8], slice_b: &mut [u8]) {
| --------- --------- these two types are declared with different lifetimes...
LL | core::mem::swap(&mut slice_a, &mut slice_b);
| ^^^^^^^^^^^^ ...but data from `slice_b` flows into `slice_a` here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: explicitly declare a lifetime and assign it to both
|
LL | fn foo<'a>(slice_a: &'a mut [u8], slice_b: &'a mut [u8]) {
| ++++ ++ ++

for

fnfoo(slice_a:&mut[u8],slice_b:&mut[u8]){
core::mem::swap(&mut slice_a,&mut slice_b);}

@rust-highfive

Copy link
Copy Markdown
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 22, 2021
@rust-log-analyzer

This comment has been minimized.

@estebankestebank left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for doing this! It is great! I have some nitpicks below, but none of them are a dealbreaker. Would you have time to address them?

Comment threadsrc/test/ui/lifetimes/issue-90170-elision-mismatch.rs Outdated
Comment threadsrc/test/ui/lifetimes/issue-90170-elision-mismatch.rs Outdated
@Noratrieb

Copy link
Copy Markdown
MemberAuthor

I will remove the tests that cause the double error reporting so that rustfix can apply the correct fix for all test cases. I'll add some other ones that shoudn't have this issue but test the new suggestions just as well.

@rust-log-analyzer

This comment has been minimized.

@estebank

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Oct 26, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 76ec1aa has been approved by estebank

@borsbors 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 26, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 26, 2021
…-hint, r=estebank
Add beginner friendly lifetime elision hint to E0623
Address rust-lang#90170
Suggest adding a new lifetime parameter when two elided lifetimes should match up but don't.
Example:
```
error[E0623]: lifetime mismatch
--> $DIR/issue-90170-elision-mismatch.rs:2:35
|
LL | fn foo(slice_a: &mut [u8], slice_b: &mut [u8]) {
| --------- --------- these two types are declared with different lifetimes...
LL | core::mem::swap(&mut slice_a, &mut slice_b);
| ^^^^^^^^^^^^ ...but data from `slice_b` flows into `slice_a` here
|
= note: Each elided lifetime in input position becomes a distinct lifetime.
help: Explicitly declare a lifetime and assign it to both
|
LL | fn foo<'a>(slice_a: &'a mut [u8], slice_b: &'a mut [u8]) {
| ++++ ++ ++
```
for
```rust
fn foo(slice_a: &mut [u8], slice_b: &mut [u8]) {
core::mem::swap(&mut slice_a, &mut slice_b);
}
```
@matthiaskrgr

Copy link
Copy Markdown
Member

Some tests failed in a rollup: #90325 (comment)
@bors r-

@borsbors 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 27, 2021
@Noratrieb

Copy link
Copy Markdown
MemberAuthor

I'm a bit confused about why the rollup failed, I updated my fork and my local repository, I have the most recent commit of master, but the tests pass.

@estebank

Copy link
Copy Markdown
Contributor

Have you tried doing git checkout master && git pull && git checkout lifetime-elision-mismatch-hint && git rebase master and then rerunning --bless?

@apirainoapiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 28, 2021
@Noratrieb

Copy link
Copy Markdown
MemberAuthor

I tried it now, the tests still pass and --bless doesn't change anything.

@estebank

Copy link
Copy Markdown
Contributor

Can you run ./x.py test src/test/ui --bless --compare-mode=nll?

@Noratrieb

Copy link
Copy Markdown
MemberAuthor

I ran it, the test still passed.

I also cloned my fork into a fresh directory again and rebuilt everything, but the lifetime tests still pass.

@estebank

Copy link
Copy Markdown
Contributor

Even with --compare-mode=nll? That's super weird. :(

@Noratrieb

Copy link
Copy Markdown
MemberAuthor

Yes, even with the flag

@Noratrieb

Copy link
Copy Markdown
MemberAuthor

Should we try to include this in the next rollup again and see whether it works, or what's your idea for it?

@matthiaskrgr

Copy link
Copy Markdown
Member

@bors retry rollup=iffy

@borsbors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 31, 2021
@Noratrieb

Copy link
Copy Markdown
MemberAuthor

:(

@lqd

lqd commented Nov 2, 2021

Copy link
Copy Markdown
Member

Repeating my comment from zulip here in case you missed it.

This PR adds a new test, and it apparently has different diagnostics under NLL, making the PR fail as you've noticed in the previous comments. The fact that the command Esteban showed works on your machine makes me think that you could locally have the file that the UI tests expect ?

That command should have created a src/test/ui/lifetimes/issue-90170-elision-mismatch.nll.stderr file, can you check if you have it ?

@Noratrieb
Noratriebforce-pushed the lifetime-elision-mismatch-hint branch from 510e2e3 to e8b6cc3CompareNovember 2, 2021 21:10
@Noratrieb

Copy link
Copy Markdown
MemberAuthor

Also repeat from zulip

Oh I think I just found the issue. Two tests have always been failing (ui/linkage-attr/issue-10755.rs and ui/process/process-spawn-nonexistent.rs) (I should probably open an issue) so I've just been ignoring that fact. I just added the ignore-test attribute to them and turns out, there is some more stuff that is ran after they fail, for example thje NLL tests. I've now got the .nll.stderr and I hope this will fix everything! Oh man this is so dumb, but I'm glad that I finally fixed it, thank you for your help!

@rust-log-analyzer

This comment has been minimized.

@Noratrieb

Copy link
Copy Markdown
MemberAuthor

oops, that was an accident

@Noratrieb

Copy link
Copy Markdown
MemberAuthor

i hope i fixed everything, I'm sorry that the last commit is now on top, but it's gits fault for making rebase conflicts so weird :(

@camelidcamelid added A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` A-lifetimes Area: Lifetimes / regions labels Nov 2, 2021
@estebank

Copy link
Copy Markdown
Contributor

Last nitpick: if you do a rebase again and squash all commits into one, it will be easier to look at the involved files history in the future and there will be no longer any "weird log history". I normally leave the commits independent during review and squash into commits containing logical grouped changes. In this case a single commit should be fine. If you try locally and find difficulties doing this, don't worry, let me know and I'll merge as is, but I'd prefer squashing these.

Suggest adding a new lifetime parameter when two elided lifetimes should match up but don't
Issue rust-lang#90170
This also changes the tests introduced by the previous commits because of another rustc issue (rust-lang#90258)
@Noratrieb
Noratriebforce-pushed the lifetime-elision-mismatch-hint branch from 2e91e9e to 4b9e460CompareNovember 3, 2021 19:11
@Noratrieb

Copy link
Copy Markdown
MemberAuthor

So, that's it, let's hope this finally works!😅

@estebank

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Nov 4, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 4b9e460 has been approved by estebank

@borsbors 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 Nov 4, 2021
@bors

bors commented Nov 4, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 4b9e460 with merge e60e19b...

@borsbors mentioned this pull request Nov 4, 2021
@bors

bors commented Nov 4, 2021

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: estebank
Pushing e60e19b to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Nov 4, 2021
@bors
bors merged commit e60e19b into rust-lang:masterNov 4, 2021
@rustbotrustbot added this to the 1.58.0 milestone Nov 4, 2021
@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (e60e19b): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

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

Labels

A-diagnosticsArea: Messages for errors, warnings, and lintsA-lifetimesArea: Lifetimes / regionsA-suggestion-diagnosticsArea: Suggestions generated by the compiler applied by `cargo fix`merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

12 participants

@Noratrieb@rust-highfive@rust-log-analyzer@estebank@bors@matthiaskrgr@lqd@rust-timer@JohnTitor@apiraino@camelid@rustbot