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

Add new multiple_bound_locations lint #12259

Merged
merged 3 commits into from Feb 24, 2024

Conversation

GuillaumeGomez
Copy link
Member

Fixes #7181.

r? @llogiq

changelog: Add new multiple_bound_locations lint

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 10, 2024
@GuillaumeGomez
Copy link
Member Author

Fixed CI :)

@bors
Copy link
Collaborator

bors commented Feb 19, 2024

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

@GuillaumeGomez
Copy link
Member Author

Fixed conflict.

r? @flip1995

@rustbot rustbot assigned flip1995 and unassigned llogiq Feb 19, 2024
@flip1995
Copy link
Member

flip1995 commented Feb 23, 2024

I won't get to reviewing this in the next 1-2 weeks, as I have a lot on my plate right now (Clippy and day_job), so:

r? clippy

@rustbot

This comment was marked as outdated.

@rustbot rustbot assigned llogiq and unassigned flip1995 Feb 23, 2024
@flip1995
Copy link
Member

That was unlucky.

r? @blyxyas as you are back to reviewing 😅

@rustbot rustbot assigned blyxyas and unassigned llogiq Feb 23, 2024
@llogiq
Copy link
Contributor

llogiq commented Feb 23, 2024

r? @llogiq

Sorry for the delay, I was quite sick in the last two weeks.

@rustbot rustbot assigned llogiq and unassigned blyxyas Feb 23, 2024
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

As usual, the code looks good (I merely found a typo in the docs), and I'd like more tests. 😉

/// Check if a generic is defined both in the bound predicate and in the `where` clause.
///
/// ### Why is this bad?
/// It can be confusing for developpers when seeing bounds for a generic in multiple places.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// It can be confusing for developpers when seeing bounds for a generic in multiple places.
/// It can be confusing for developers when seeing bounds for a generic in multiple places.

Typo

@@ -0,0 +1,24 @@
#![warn(clippy::multiple_bound_locations)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also like to see tests for the various possible combinations of generic impls with generic methods.

@GuillaumeGomez GuillaumeGomez force-pushed the multiple-bound-locations branch 2 times, most recently from e8e666f to 4dc952c Compare February 23, 2024 12:43
@GuillaumeGomez
Copy link
Member Author

Sorry for the delay, I was quite sick in the last two weeks.

Hope you're feeling better. :-/

As usual, the code looks good (I merely found a typo in the docs), and I'd like more tests. 😉

Fixed the typo and extended tests.

{
}

struct B;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if B had generics, say one type and one lifetime, just to mix it up a bit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure to see what you want to test here. ^^'

Copy link
Contributor

Choose a reason for hiding this comment

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

Say what if you had struct B<F>(F); and then an impl<F> B<F> { fn foo(_f: F) -> Self where F: std::fmt::Display { todo!() } }?

Copy link
Member Author

Choose a reason for hiding this comment

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

It won't emit the lint but I can add a check for it if you want?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added it.


struct C<F>(F);

impl<F> C<F> {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we had another bound on F here?

Also: Should that even lint? Perhaps it's useful to have a secondary bound for just one method of the impl?

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 have no opinion about this, hence why I didn't add checks on impl blocks. So up to you. :)

@GuillaumeGomez
Copy link
Member Author

And fixed CI. :)

@llogiq
Copy link
Contributor

llogiq commented Feb 24, 2024

Thank you!

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 24, 2024

📌 Commit 762448b has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Feb 24, 2024

⌛ Testing commit 762448b with merge a2c1d56...

@bors
Copy link
Collaborator

bors commented Feb 24, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing a2c1d56 to master...

@bors bors merged commit a2c1d56 into rust-lang:master Feb 24, 2024
8 checks passed
@GuillaumeGomez GuillaumeGomez deleted the multiple-bound-locations branch February 24, 2024 13:54
@bors bors mentioned this pull request Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New lint: multiple-bound-locations
6 participants