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

suggest lifetime for closure parameter type when mismatch #105888

Merged
merged 4 commits into from
Apr 16, 2023

Conversation

skyzh
Copy link
Contributor

@skyzh skyzh commented Dec 19, 2022

This is a draft PR, will add test cases later and be ready for review.

This PR fixes #105675 by adding a diagnostics suggestion. Also a partial fix to #105528.

The following code will have a compile error now:

fn const_if_unit(input: bool) -> impl for<'a> FnOnce(&'a ()) -> usize {
    let x = |_| 1;
    x
}

Before this PR:

error[E0308]: mismatched types
 --> src/lib.rs:3:5
  |
3 |     x
  |     ^ one type is more general than the other
  |
  = note: expected trait `for<'a> FnOnce<(&'a (),)>`
             found trait `FnOnce<(&(),)>`
note: this closure does not fulfill the lifetime requirements
 --> src/lib.rs:2:13
  |
2 |     let x = |_| 1;
  |             ^^^

error: implementation of `FnOnce` is not general enough
 --> src/lib.rs:3:5
  |
3 |     x
  |     ^ implementation of `FnOnce` is not general enough
  |
  = note: closure with signature `fn(&'2 ()) -> usize` must implement `FnOnce<(&'1 (),)>`, for any lifetime `'1`...
  = note: ...but it actually implements `FnOnce<(&'2 (),)>`, for some specific lifetime `'2`

For more information about this error, try `rustc --explain E0308`.
error: could not compile `rust-test` due to 2 previous errors

After this PR:

error[E0308]: mismatched types
 --> src/lib.rs:3:5
  |
3 |     x
  |     ^ one type is more general than the other
  |
  = note: expected trait `for<'a> FnOnce<(&'a (),)>`
             found trait `FnOnce<(&(),)>`
note: this closure does not fulfill the lifetime requirements
 --> src/lib.rs:2:13
  |
2 |     let x = |_| 1;
  |             ^^^
help: consider changing the type of the closure parameters
  |
2 |     let x = |_: &_| 1;
  |             ~~~~~~~

error: implementation of `FnOnce` is not general enough
 --> src/lib.rs:3:5
  |
3 |     x
  |     ^ implementation of `FnOnce` is not general enough
  |
  = note: closure with signature `fn(&'2 ()) -> usize` must implement `FnOnce<(&'1 (),)>`, for any lifetime `'1`...
  = note: ...but it actually implements `FnOnce<(&'2 (),)>`, for some specific lifetime `'2`

For more information about this error, try `rustc --explain E0308`.
error: could not compile `rust-test` due to 2 previous errors

After applying the suggestion, it compiles. The suggestion might not always be correct as the generation procedure of that suggestion is quite simple...

@rustbot
Copy link
Collaborator

rustbot commented Dec 19, 2022

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 19, 2022
@skyzh
Copy link
Contributor Author

skyzh commented Dec 19, 2022

@rustbot label +A-suggestion-diagnostics +T-compiler

@rustbot rustbot added the A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` label Dec 19, 2022
@skyzh
Copy link
Contributor Author

skyzh commented Dec 19, 2022

Originally I thought we should fix this bug and make the original code compiles. But it seems that we will need to decide whether _ is for <'a> or some specific lifetime in advance before going into the borrow checker. Therefore making a diagnostics suggestion might be a better fix... I guess?

@skyzh skyzh force-pushed the skyzh/suggest-lifetime-closure branch from 7c339b0 to db651f7 Compare December 19, 2022 05:29
@rust-log-analyzer

This comment has been minimized.

@skyzh skyzh marked this pull request as ready for review December 21, 2022 04:37
@skyzh
Copy link
Contributor Author

skyzh commented Dec 21, 2022

Test case updated. This suggestion may also fix some previous issues. e.g, for src/test/ui/lifetimes/issue-79187-2.stderr, the suggested fix actually works.

@skyzh skyzh force-pushed the skyzh/suggest-lifetime-closure branch from 8c397d3 to 2b43cba Compare December 21, 2022 04:46
@apiraino
Copy link
Contributor

I assume the last round of review is finished and the review flag can be switched. @skyzh Feel free to request a review with @rustbot ready, thanks!

@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2023
@skyzh
Copy link
Contributor Author

skyzh commented Jan 18, 2023

@rustbot ready

@rustbot rustbot 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 Jan 18, 2023
@skyzh
Copy link
Contributor Author

skyzh commented Jan 18, 2023

r? @compiler-errors

@rustbot rustbot assigned compiler-errors and unassigned TaKO8Ki Jan 18, 2023
@bors
Copy link
Contributor

bors commented Jan 25, 2023

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

@compiler-errors
Copy link
Member

This needs a rebase. Marking this as waiting-on-author. Please use @rustbot ready to mark this as ready.

@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 2, 2023
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 13, 2023

📌 Commit c025483 has been approved by compiler-errors

It is now in the queue for this repository.

@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 Apr 13, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 14, 2023
…re, r=compiler-errors

suggest lifetime for closure parameter type when mismatch

This is a draft PR, will add test cases later and be ready for review.

This PR fixes rust-lang#105675 by adding a diagnostics suggestion. Also a partial fix to rust-lang#105528.

The following code will have a compile error now:

```
fn const_if_unit(input: bool) -> impl for<'a> FnOnce(&'a ()) -> usize {
    let x = |_| 1;
    x
}
```

Before this PR:

```
error[E0308]: mismatched types
 --> src/lib.rs:3:5
  |
3 |     x
  |     ^ one type is more general than the other
  |
  = note: expected trait `for<'a> FnOnce<(&'a (),)>`
             found trait `FnOnce<(&(),)>`
note: this closure does not fulfill the lifetime requirements
 --> src/lib.rs:2:13
  |
2 |     let x = |_| 1;
  |             ^^^

error: implementation of `FnOnce` is not general enough
 --> src/lib.rs:3:5
  |
3 |     x
  |     ^ implementation of `FnOnce` is not general enough
  |
  = note: closure with signature `fn(&'2 ()) -> usize` must implement `FnOnce<(&'1 (),)>`, for any lifetime `'1`...
  = note: ...but it actually implements `FnOnce<(&'2 (),)>`, for some specific lifetime `'2`

For more information about this error, try `rustc --explain E0308`.
error: could not compile `rust-test` due to 2 previous errors
```

After this PR:

```
error[E0308]: mismatched types
 --> src/lib.rs:3:5
  |
3 |     x
  |     ^ one type is more general than the other
  |
  = note: expected trait `for<'a> FnOnce<(&'a (),)>`
             found trait `FnOnce<(&(),)>`
note: this closure does not fulfill the lifetime requirements
 --> src/lib.rs:2:13
  |
2 |     let x = |_| 1;
  |             ^^^
help: consider changing the type of the closure parameters
  |
2 |     let x = |_: &_| 1;
  |             ~~~~~~~

error: implementation of `FnOnce` is not general enough
 --> src/lib.rs:3:5
  |
3 |     x
  |     ^ implementation of `FnOnce` is not general enough
  |
  = note: closure with signature `fn(&'2 ()) -> usize` must implement `FnOnce<(&'1 (),)>`, for any lifetime `'1`...
  = note: ...but it actually implements `FnOnce<(&'2 (),)>`, for some specific lifetime `'2`

For more information about this error, try `rustc --explain E0308`.
error: could not compile `rust-test` due to 2 previous errors
```

After applying the suggestion, it compiles. The suggestion might not always be correct as the generation procedure of that suggestion is quite simple...
@Dylan-DPC
Copy link
Member

@bors r-

failed in rollup

@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 Apr 14, 2023
skyzh added 4 commits April 14, 2023 11:39
Signed-off-by: Alex Chi <iskyzh@gmail.com>
Signed-off-by: Alex Chi <iskyzh@gmail.com>
Signed-off-by: Alex Chi <iskyzh@gmail.com>
@skyzh skyzh force-pushed the skyzh/suggest-lifetime-closure branch from c025483 to 90dc6fe Compare April 14, 2023 15:43
@skyzh
Copy link
Contributor Author

skyzh commented Apr 14, 2023

@rustbot ready

Looks like a merge conflict 🙈 Fixed

@rustbot rustbot 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 Apr 14, 2023
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 15, 2023

📌 Commit 90dc6fe has been approved by compiler-errors

It is now in the queue for this repository.

@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 Apr 15, 2023
@bors
Copy link
Contributor

bors commented Apr 16, 2023

⌛ Testing commit 90dc6fe with merge 2a71115...

@bors
Copy link
Contributor

bors commented Apr 16, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 2a71115 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 16, 2023
@bors bors merged commit 2a71115 into rust-lang:master Apr 16, 2023
@rustbot rustbot added this to the 1.71.0 milestone Apr 16, 2023
@skyzh skyzh deleted the skyzh/suggest-lifetime-closure branch April 16, 2023 05:45
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2a71115): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.5% [0.8%, 4.8%] 6
Regressions ❌
(secondary)
2.3% [1.4%, 2.8%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.5% [0.8%, 4.8%] 6

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.2% [-3.4%, -1.2%] 9
All ❌✅ (primary) - - 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Confusing diagnostic complains about lifetimes in a closure, when parameter type is missing
10 participants