-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add repeated_where_clause_or_trait_bound
lint
#8703
Add repeated_where_clause_or_trait_bound
lint
#8703
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon. Please see the contribution instructions for more information. |
[
repeated_where_clause_or_trait_bound]
`[
repeated_where_clause_or_trait_bound]
92d2b01
to
2f518f9
Compare
[
repeated_where_clause_or_trait_bound]
[repeated_where_clause_or_trait_bound]
[repeated_where_clause_or_trait_bound]
[repeated_where_clause_or_trait_bound]
[repeated_where_clause_or_trait_bound]
[repeated_where_clause_or_trait_bound]
[repeated_where_clause_or_trait_bound]
[
repeated_where_clause_or_trait_bound]
`
[
repeated_where_clause_or_trait_bound]
`repeated_where_clause_or_trait_bound
lint
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.
Can this lint avoid catching a case like this?
use std::ops::Add;
fn add<T: Add<u32> + Add<u64>>() {}
Other cases like this: https://github.com/rayon-rs/rayon/blob/v1.5.0/src/iter/product.rs#L10
https://github.com/rayon-rs/rayon/blob/v1.5.0/src/iter/product.rs#L10
Good catch @giraffate! This also uncovers a bug in the existing TRAIT_DUPLICATION_IN_BOUNDS lint. I'll have a think about it because it is a recursive problem. ie. bounds contains other bounds. Do you have any suggestions for doing an easy comparison? The only solution that has occurred to me at the moment is to collect the nested generic params into some sort of nested structure and collect something comparable so I can compare that rather than the Res. I was thinking it might be easier to get the whole text of a bounds span for a generic and compare that. |
☔ The latest upstream changes (presumably #8788) made this pull request unmergeable. Please resolve the merge conflicts. |
You should rebase your branch on top of the current master. Some things changed with how generics are lowered in rustc just a few days ago. You might have to revisit some parts of the code. This might help with the |
9b329ed
to
89192b9
Compare
Thanks for the heads up @flip1995. I have pushed a fixup that addresses the changes. I left it unsquashed so how the generic lowering works now could be discussed. I wasn't able to fix an issue with supporting the unstable associated_type_bounds feature. #![feature(associated_type_bounds)]
fn flatten_twice_good<T>(iter: T) -> Flatten<Flatten<T>>
where
T: Iterator<Item: IntoIterator<Item: IntoIterator>>,
{
iter.flatten().flatten()
} c9c31f3 handled this because bounds could contain their nested bounds. I haven't been able to follow how WhereBoundPredicate can nest WhereBoundPredicates yet. |
I wouldn't worry too much about supporting this feature with this lint. I'd suggest to just leave a |
e96177a
to
6040fa6
Compare
Squashed and added the fixme. There is one other outstanding issue I haven't been able to figure out. This case should be detected. rust-clippy/tests/ui/repeated_where_clause_or_trait_bound.rs Lines 40 to 42 in 6040fa6
Doing some debugging on the Generic, no predicates exist. This only happens for the self trait bound and not the where clause. I haven't dug any further than that. |
For your example you should find those bounds in the |
Thanks for the tip @flip1995. I got access to an |
@rustbot ready |
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.
Thank you for the review @flip1995 offered to do the final review (probably after their vacation :))
I think the final group should be |
@dswij I put it in the nursery group because the other trait bound ones were put in there. I didn't have the confidence to say that the new lint was ready when the original lints it was based off were moved to nursery 😅. Original group was Offtopic: What is the process of growing out of nursery? I'm going to pick up #8757 |
It depends on the reason why the lint was moved to the nursery group. The general rule is, that lints can be moved to their respective group once all issues have been resolved. In this case I remember somebody working on fixing the issues. I'm just not sure if they've finished their work. It could be, that they've been fixed on the rust side and will be enabled again during the weekend. I'll try to keep an eye on it :) |
☔ The latest upstream changes (presumably #8856) made this pull request unmergeable. Please resolve the merge conflicts. |
e6b34d7
to
5724208
Compare
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.
3566b58
to
cc419e5
Compare
Thanks for the review @flip1995! I have addressed your comments. |
☔ The latest upstream changes (presumably #9132) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
So sorry, I completely forgot about this PR... I would do the renaming now, it shouldn't be too much work. If you lost the motivation to work on this after that long time, I'll address this myself in order not to lose this awesome work.
clippy_lints/src/trait_bounds.rs
Outdated
|
||
span_lint_and_sugg( | ||
cx, | ||
REPEATED_WHERE_CLAUSES_OR_TRAIT_BOUNDS, |
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.
Merging the lints would just mean to change this line and adapt the documentation of the other lint a bit. I don't really want to merge a new lint, that will be renamed later, because if we mistime things, the temporary lint name will get into stable and we have to go through the renaming procedure or backport things.
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.
@flip1995 ah got it. Of course the lint would be just under the same name! I was thinking it had to be done in the same pass.
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.
Merging under the existing lint name raises some extra noise with UI test errors since the original lint "catches" some extra errors (#8757). Since this is actually a bug, I have left a comment that the errors generated in stderr
should be removed once #8757 is fixed.
With regards to fixing, I've just turned off rustfix since I can't get the fixed file to compile without fixing the original lint.
Once this PR is merged I can work on reducing the noise in the errors so that it's not reporting the same errors twice slightly differently. This sounds ok to me since the lint is back in nursery.
@flip1995 I can address it. I had it in my head that "merging" the lint meant doing it in the same pass. I should be able to get it done this weekend. Thanks for calling it awesome! |
cc419e5
to
a5255ef
Compare
@rustbot ready |
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.
Thanks for addressing this so quickly. I have a few small comments left. Overall this LGTM 👍
// This should not warn but currently does see #8757 | ||
fn good_generic<T: GenericTrait<u64> + GenericTrait<u32>>(arg0: T) { | ||
unimplemented!(); | ||
} |
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.
If this is the only test case that trips up rustfix, I'd suggest to move it into its own file named something like trait_duplication_in_bounds_8757.rs
. That way, you can keep rustfix enabled and still have a regression test for the issue.
Or are there other cases that would break rustfix?
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.
Yes, there are other cases that break rustfix.
The existing lint of trait_duplication_in_bounds
raises a lint for pretty much all the new examples added. I believe the original intention of the lint is supposed to only be for traits appearing in both trait bounds and where clauses as that seems to be the only kinds of tests. I think it's a bug that it picks up duplicates within trait bounds and where clauses. This is evident because the errors produced for these cases are misleading.
Example:
fn bad_foo<T: Clone + Clone + Clone + Copy, U: Clone + Copy>(arg0: T, argo1: U) {
unimplemented!();
}
error: this trait bound is already specified in the where clause
--> $DIR/trait_duplication_in_bounds.rs:103:19
|
LL | fn bad_foo<T: Clone + Clone + Clone + Copy, U: Clone + Copy>(arg0: T, argo1: U) {
| ^^^^^
|
= help: consider removing this trait bound
this trait bound is already specified in the where clause
It's error refers to a where clause but there aren't any.
I can fix this issue along with #8757 .
At that time I can split the fixables and unfixables like I see done with the other test files.
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.
Ah ok, that's interesting. It seems that the old lint already triggers on the cases, the newly added lint code should check for, just with a worse error message + suggestion. I think this is even worse than #8757. Since this lint is in nursery
I'd be ok merging this. But I would open an issue about it, even when you start working on resolving this right away.
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.
Ok will do 👍🏼
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.
Issue here #9151
a5255ef
to
a0c2da9
Compare
Can you squash some of the commits please? After that this is good to go 👍 |
a0c2da9
to
188e4a8
Compare
188e4a8
to
8878d67
Compare
@flip1995 squashed. Thanks for the review! |
Thanks! Sorry again for forgetting about this PR @bors r+ |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
I thought I would try and scratch my own itch for #8674.
Res
the correct way for ensuring we have the same trait?I tried to use
GenericParam::bounds_span_for_suggestions
but it only gave me an empty span at the end of the spans.I tried
WhereClause::span_for_predicates_or_empty_place
and it included the comma.Res
orDef
.changelog: Add
[`repeated_where_clause_or_trait_bound`]
lint.