Suggest impl Trait return type when incorrectly using a generic return type by Noratrieb · Pull Request #89892 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation63 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Address #85991
When there is a type mismatch error and the return type is generic, and that generic parameter is not used in the function parameters, suggest replacing that generic with the impl Trait syntax.
r? @estebank
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.
This is my first PR, so I hope I did everything correctly!
This comment has been minimized.
This comment has been minimized.
As a new Rust programmer, I really want this feature
This comment has been minimized.
I'm done with the changes, you can review it again if you have time
Relevant to the compiler team, which will review and decide on the PR/issue.
label
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a neat idea :) it looks in pretty good shape, just a few suggestions.
I added the notes, also added // run-rustfix to test the suggestions, and squashed all commits into one, since the change is pretty small.
This comment has been minimized.
@Nilstrieb you need to add the .fixed file I think - you might be able to generate it with --bless --run-rustfix.
It does have the fixed code, but the problem is that this fixed code is not compiling successfully because of bad_echo. How can I allow this?
@Nilstrieb I'd suggest making the rustfix test separate from the rest of the tests.
I've squashed it again since this is hopefully the last change
Thank you for your review, I'll implement this next week!
This comment has been minimized.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, more reviews.
Also, can you squash the commits?
| _ => ty.contains(expected, self.tcx), |
|---|
| }); |
| if any_predicate_contains_expected_ty { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't necessarily cover cases where T appears in the bounds, right? So like Option<()>: Trait<T>
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I wasn't able to figure out how I'd do that. Do you have an idea?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of making contains be on Ty, you can make it a visitor. Then just visit both the ty and bounds.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the contains I was able to use the visitor on Ty. How should I visit the PolyTraitRef in the bounds? I found hir::intravisit but wasn't able to find a good way to use it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll just leave a fixme in a test 👍
I'll squash it once it's done.
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
This comment has been minimized.
This comment has been minimized.
Finally did it, I wanted to have TyS::contains as a separate commit at first, but after a little git accident I decided to just squash them anyways
Sorry I didn't get to this before; r=me here after rebase
@bors delegate+
(just do r=jackh726)
This comment has been minimized.
Address rust-lang#85991
Suggest the impl Trait return type syntax if the user tried to return a generic parameter and we get a type mismatch
The suggestion is not emitted if the param appears in the function parameters, and only get the bounds that actually involve T: directly
It also checks whether the generic param is contained in any where bound (where it isn't the self type), and if one is found (like Option<T>: Send), it is not suggested.
This also adds TyS::contains, which recursively vistits the type and looks if the other type is contained anywhere
📌 Commit 4bed748 has been approved by jackh726
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
bors added a commit to rust-lang-ci/rust that referenced this pull request
…askrgr
Rollup of 10 pull requests
Successful merges:
- rust-lang#89892 (Suggest
impl Traitreturn type when incorrectly using a generic return type) - rust-lang#91675 (Add MemTagSanitizer Support)
- rust-lang#92806 (Add more information to
impl Traiterror) - rust-lang#93497 (Pass
--testflag through rustdoc to rustc so#[test]functions can be scraped) - rust-lang#93814 (mips64-openwrt-linux-musl: correct soft-foat)
- rust-lang#93847 (kmc-solid: Use the filesystem thread-safety wrapper)
- rust-lang#93877 (asm: Allow the use of r8-r14 as clobbers on Thumb1)
- rust-lang#93892 (Only mark projection as ambiguous if GAT substs are constrained)
- rust-lang#93915 (Implement --check-cfg option (RFC 3013), take 2)
- rust-lang#93953 (Add the
known-bugtest directive, use it, and do some cleanup)
Failed merges:
r? @ghost
@rustbot modify labels: rollup
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.