-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Make E0121's suggestion more robust (+ fix E0308's suggestion) #80847
Conversation
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. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
The only thing that jumps up to me is that @bors delegate+ |
✌️ @PatchMixolydic can now approve this pull request |
I've made the check recursive for algebraic data types, arrays, slices, function pointers, raw pointers, references, and tuples, which seems to cover every |
!? I rebased onto master before pushing; not sure how this happened... |
Not sure if I should approve this since it seems to cause a diagnostics regression with generic ADTs [and because of the long stream of commits that seems erroneous]: $ cargo +nightly -Vv
cargo 1.50.0-nightly (75d5d8cff 2020-12-22)
release: 1.50.0
commit-hash: 75d5d8cffe3464631f82dcd3c470b78dc1dda8bb
commit-date: 2020-12-22
$ cargo +nightly check
Checking rust-issue v0.1.0 (/tmp/rust-issue)
error[E0308]: mismatched types
--> src/main.rs:2:5
|
2 | Some(0i32)
| ^^^^^^^^^^ expected `()`, found enum `Option`
|
= note: expected unit type `()`
found enum `Option<i32>`
help: try adding a semicolon
|
2 | Some(0i32);
| ^
help: try adding a return type
|
1 | fn expected_unit_got_option_i32() -> Option<i32> {
| ^^^^^^^^^^^^^^
error: aborting due to previous error
For more information about this error, try `rustc --explain E0308`.
error: could not compile `rust-issue`
To learn more, run the command again with --verbose.
$ cargo +stage1 check
Checking rust-issue v0.1.0 (/tmp/rust-issue)
error[E0308]: mismatched types
--> src/main.rs:2:5
|
1 | fn expected_unit_got_option_i32() {
| - possibly return type missing here?
2 | Some(0i32)
| ^^^^^^^^^^- help: try adding a semicolon: `;`
| |
| expected `()`, found enum `Option`
|
= note: expected unit type `()`
found enum `Option<i32>`
error: aborting due to previous error
For more information about this error, try `rustc --explain E0308`.
error: could not compile `rust-issue`
To learn more, run the command again with --verbose. Note that the suggestion is missing with this PR applied. I'm not sure if that's a blocker, but it might cause issues for rust-analyzer or other such tools. |
Oh boy! That's quite a tangle you got there! Git is so hard to use :-/ I've caused similar problems in the past to the point that I've dropped PRs and started again. You might be able to get away with doing a |
@@ -1545,7 +1545,7 @@ fn fn_sig(tcx: TyCtxt<'_>, def_id: DefId) -> ty::PolyFnSig<'_> { | |||
let ret_ty = fn_sig.output(); | |||
if ret_ty != tcx.ty_error() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this check can now be removed thanks to is_suggestable
accounting for Error
.
It's surprising that the test with Can you add debug statements to see what the type parameters are? It could be that at this point it hasn't figured out that it would be |
I have; it seems that it hasn't resolved types yet (I tried using |
This comment has been minimized.
This comment has been minimized.
??? |
I'm not entirely sure how to get |
☔ The latest upstream changes (presumably #81461) made this pull request unmergeable. Please resolve the merge conflicts. |
Ping from triage - @PatchMixolydic @rustbot label: -S-waiting-on-review +S-waiting-on-author |
@PatchMixolydic Ping from triage, CI is still red here. |
Sorry about the inactivity; I've been fairly busy with college and general burnout. I'll get on this in a couple days, time permitting. |
Quick update: I'm planning on fixing the build issues tomorrow. Might also look into fixing the test failure, but I'm still concerned that it might be over my head. Once again, sorry about the long absence. Life has been rough lately 😓 |
Before, both E0308 and E0121 suggested using the unnameable type of a generator as a return type. This fixes this by making Ty::is_suggestable return `false` for generators and by making E0121 use `Ty::is_suggestable` to prevent this from happening in the future. Fixes #80844.
This comment has been minimized.
This comment has been minimized.
The job Click to see the possible cause of the failure (guessed by this bot)
|
Oh dear, that's quite a few more test failures than I expected 😰 Going to go to bed (haven't slept in over 24 hours) and take a glance at this after I wake up, but I might be in over my head. It might ultimately be better for me (or someone smarter than me) to cherrypick/replicate these changes on a new branch and make a new PR for that. Considering how many type hints get outright removed by these changes, a different approach might be necessary anyway (or perhaps there was an issue during the rebase that wasn't flagged by Git). |
sleep defects are hard Looking at the test failures, it seems like something fundamental broke. I hope that this might just need a manual rebase, but as you might be able to guess at this point I'm probably not the best person for the job due to my extremely warped schedule @rustbot modify labels +S-inactive |
Ping from triage: @PatchMixolydic Thank you! |
Before, both E0308 and E0121 suggested using the unnameable type of a generator as a return type. This fixes this by making
Ty::is_suggestable
returnfalse
for generators and by making E0121 useTy::is_suggestable
to prevent this from happening in the future.Fixes #80844.