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 impl Trait return type when incorrectly using a generic return type #89892

Merged
merged 1 commit into from
Feb 19, 2022

Conversation

Noratrieb
Copy link
Member

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

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

This is my first PR, so I hope I did everything correctly!

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@C0RR1T
Copy link

C0RR1T commented Oct 15, 2021

As a new Rust programmer, I really want this feature

@bors

This comment has been minimized.

@Noratrieb
Copy link
Member Author

I'm done with the changes, you can review it again if you have time

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 21, 2021
@Noratrieb
Copy link
Member Author

What's the status on this PR? @estebank

Copy link
Member

@jyn514 jyn514 left a comment

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.

compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs Outdated Show resolved Hide resolved
src/test/ui/return/return-impl-trait.rs Show resolved Hide resolved
compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs Outdated Show resolved Hide resolved
@Noratrieb Noratrieb force-pushed the suggest-return-impl-trait branch 2 times, most recently from 96573f1 to 4572455 Compare November 10, 2021 18:56
@Noratrieb
Copy link
Member Author

Noratrieb commented Nov 10, 2021

I added the notes, also added // run-rustfix to test the suggestions, and squashed all commits into one, since the change is pretty small.

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Nov 10, 2021

@Nilstrieb you need to add the .fixed file I think - you might be able to generate it with --bless --run-rustfix.

@Noratrieb
Copy link
Member Author

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?

@jyn514
Copy link
Member

jyn514 commented Nov 10, 2021

@Nilstrieb I'd suggest making the rustfix test separate from the rest of the tests.

@Noratrieb
Copy link
Member Author

I've squashed it again since this is hopefully the last change

@Noratrieb
Copy link
Member Author

Thank you for your review, I'll implement this next week!

@rust-log-analyzer

This comment has been minimized.

@jackh726
Copy link
Member

jackh726 commented Jan 9, 2022

r? @jackh726

@rust-highfive rust-highfive assigned jackh726 and unassigned estebank Jan 9, 2022
Copy link
Member

@jackh726 jackh726 left a comment

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?

compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs Outdated Show resolved Hide resolved
_ => ty.contains(expected, self.tcx),
});

if any_predicate_contains_expected_ty {
Copy link
Member

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>

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, thought I replied to this before.

There is a hir visitor: https://doc.rust-lang.org/stable/nightly-rustc/rustc_hir/intravisit/trait.Visitor.html

Alternatively, you could lower the trait ref.

I'm also fine it you just want to add this as a test case with a FIXME. This is just suggestion after all and doesn't have to he "perfect"

Copy link
Member Author

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 👍

compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs Outdated Show resolved Hide resolved
@Noratrieb
Copy link
Member Author

I'll squash it once it's done.

@bors
Copy link
Contributor

bors commented Jan 18, 2022

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

@jackh726 jackh726 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 30, 2022
@rust-log-analyzer

This comment has been minimized.

@Noratrieb Noratrieb force-pushed the suggest-return-impl-trait branch 2 times, most recently from 5d2ddbd to 3005d4d Compare January 31, 2022 07:29
@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member Author

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

@bors
Copy link
Contributor

bors commented Feb 15, 2022

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

@jackh726
Copy link
Member

Sorry I didn't get to this before; r=me here after rebase

@jackh726
Copy link
Member

@bors delegate+

(just do r=jackh726)

@bors
Copy link
Contributor

bors commented Feb 15, 2022

✌️ @Nilstrieb can now approve this pull request

@rust-log-analyzer

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
@Noratrieb
Copy link
Member Author

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Feb 18, 2022

📌 Commit 4bed748 has been approved by jackh726

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 18, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2022
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#89892 (Suggest `impl Trait` return type when incorrectly using a generic return type)
 - rust-lang#91675 (Add MemTagSanitizer Support)
 - rust-lang#92806 (Add more information to `impl Trait` error)
 - rust-lang#93497 (Pass `--test` flag 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-bug` test directive, use it, and do some cleanup)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f8b83a2 into rust-lang:master Feb 19, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 19, 2022
@Noratrieb Noratrieb deleted the suggest-return-impl-trait branch February 19, 2022 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

10 participants