Skip to content
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

Add beginner friendly lifetime elision hint to E0623 #90179

Merged
merged 1 commit into from
Nov 4, 2021

Conversation

Noratrieb
Copy link
Member

@Noratrieb Noratrieb commented Oct 22, 2021

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

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

@rust-highfive
Copy link
Collaborator

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-highfive rust-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.

Copy link
Contributor

@estebank estebank left a comment

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?

@Noratrieb
Copy link
Member Author

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
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 26, 2021

📌 Commit 76ec1aa has been approved by estebank

@bors 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 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
Member

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

@bors 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 27, 2021
@Noratrieb
Copy link
Member Author

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
Contributor

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

@apiraino apiraino 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
Member Author

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

@estebank
Copy link
Contributor

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

@Noratrieb
Copy link
Member Author

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
Contributor

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

@Noratrieb
Copy link
Member Author

Yes, even with the flag

@Noratrieb
Copy link
Member Author

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
Member

@bors retry rollup=iffy

@bors bors 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
Member Author

:(

@lqd
Copy link
Member

lqd commented Nov 2, 2021

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 Noratrieb force-pushed the lifetime-elision-mismatch-hint branch from 510e2e3 to e8b6cc3 Compare November 2, 2021 21:10
@Noratrieb
Copy link
Member Author

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
Member Author

oops, that was an accident

@Noratrieb
Copy link
Member Author

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 :(

@camelid camelid 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
Contributor

estebank commented Nov 3, 2021

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 Noratrieb force-pushed the lifetime-elision-mismatch-hint branch from 2e91e9e to 4b9e460 Compare November 3, 2021 19:11
@Noratrieb
Copy link
Member Author

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

@estebank
Copy link
Contributor

estebank commented Nov 4, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Nov 4, 2021

📌 Commit 4b9e460 has been approved by estebank

@bors 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 Nov 4, 2021
@bors
Copy link
Contributor

bors commented Nov 4, 2021

⌛ Testing commit 4b9e460 with merge e60e19b...

@bors
Copy link
Contributor

bors commented Nov 4, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 4, 2021
@bors bors merged commit e60e19b into rust-lang:master Nov 4, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 4, 2021
@rust-timer
Copy link
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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant 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.