-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 using type args directly instead of equality constraint #122591
Suggest using type args directly instead of equality constraint #122591
Conversation
☔ The latest upstream changes (presumably #121123) made this pull request unmergeable. Please resolve the merge conflicts. |
@rustbot label -S-waiting-on-review +S-waiting-on-author |
r? @fmease |
d29a5a1
to
560d463
Compare
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #120926) made this pull request unmergeable. Please resolve the merge conflicts. |
3aec774
to
ec93aaa
Compare
HIR ty lowering was modified cc @fmease |
This comment has been minimized.
This comment has been minimized.
ec93aaa
to
274ad2a
Compare
@rustbot label +S-waiting-on-review -S-waiting-on-author |
274ad2a
to
f31eb72
Compare
f31eb72
to
fcdffd6
Compare
@fmease I have updated the code as per your review comments. The main thing to note is that I have changed the approach such that we make the "use directly" suggestion only there is a type param with a matching name. If not, we suggest removal. |
fcdffd6
to
52e9774
Compare
Rebased |
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.
Sorry for taking so long again! I should have more time for review now.
I have a couple of suggestions.
|
||
// Check if the type has a generic param with the | ||
// same name as the assoc type name in type binding | ||
let generics = tcx.generics_of(def_id); |
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.
generics_of
can panic if the definition (identified by the def_id
) can't have generic parameters. Could you check if we can indeed panic here, e.g. if the definition is a type parameter:
fn f<T>() {
let _: T<X = ()>;
}
If it does, we need to add some checks before trying to use generics_of
.
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.
No, this does not ICE
&& let hir::TypeBindingKind::Equality { term } = binding.kind | ||
{ | ||
// Suggests removal of the offending binding | ||
let suggest_removal = |e: &mut Diag<'_>| { |
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.
Note to self: I still need to review this function. I suspect it can be simplified by a lot but let's see.
No worries @fmease. I'll incorporate your suggestions. |
52e9774
to
f7ebad4
Compare
@fmease please review |
@rustbot label -S-waiting-on-author +S-waiting-on-review |
I've left some comments here (linking this from here for visibility). |
👍 @bors r+ rollup |
Rollup of 7 pull requests Successful merges: - rust-lang#120929 (Wrap dyn type with parentheses in suggestion) - rust-lang#122591 (Suggest using type args directly instead of equality constraint) - rust-lang#122598 (deref patterns: lower deref patterns to MIR) - rust-lang#123048 (alloc::Layout: explicitly document size invariant on the type level) - rust-lang#123993 (Do `check_coroutine_obligations` once per typeck root) - rust-lang#124218 (Allow nesting subdiagnostics in #[derive(Subdiagnostic)]) - rust-lang#124285 (Mark ``@RUSTC_BUILTIN`` search path usage as unstable) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#122591 - gurry:122162-impl-type-binding-suggestion, r=fmease Suggest using type args directly instead of equality constraint When type arguments are written erroneously using an equality constraint we suggest specifying them directly without the equality constraint. Fixes rust-lang#122162 Changes the diagnostic in the issue from: ```rust error[E0229]: associated type bindings are not allowed here 9 | impl std::cmp::PartialEq<Rhs = T> for S { | ^^^^^^^ associated type not allowed here | ``` to ```rust error[E0229]: associated type bindings are not allowed here 9 | impl std::cmp::PartialEq<Rhs = T> for S { | ^^^^^^^ associated type not allowed here | help: to use `T` as a generic argument specify it directly | | impl std::cmp::PartialEq<T> for S { | ~ ```
When type arguments are written erroneously using an equality constraint we suggest specifying them directly without the equality constraint.
Fixes #122162
Changes the diagnostic in the issue from:
to