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

Don't lint multiple_inherent_impl with generic arguments #7089

Merged
merged 1 commit into from
May 18, 2021

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Apr 15, 2021

fixes: #5772
changelog: Treat different generic arguments as different types in multiple_inherent_impl

@rust-highfive
Copy link

r? @camsteffen

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 15, 2021
@Jarcho Jarcho force-pushed the multiple_impls_generic branch from 84f8cac to 23ddf7a Compare April 15, 2021 16:46
@camsteffen
Copy link
Contributor

I don't think we should totally skip types with generic arguments. Instead we should detect if two impls have the exact same generic arguments. Since this is a restriction lint, some false positives are okay and even preferable to false negatives IMO.

@Jarcho
Copy link
Contributor Author

Jarcho commented Apr 25, 2021

I'll keep this open while i look into it.

@Jarcho Jarcho force-pushed the multiple_impls_generic branch 2 times, most recently from 637cabb to d962f37 Compare April 25, 2021 03:50
@Jarcho
Copy link
Contributor Author

Jarcho commented Apr 25, 2021

That should do it. I'm not sure if that's the right way to add a dependency on indexmap.

@Jarcho Jarcho force-pushed the multiple_impls_generic branch from d962f37 to 0bb198c Compare April 25, 2021 03:53
clippy_lints/src/lib.rs Outdated Show resolved Hide resolved
@camsteffen camsteffen added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels May 4, 2021
@Jarcho Jarcho force-pushed the multiple_impls_generic branch 2 times, most recently from d6841da to d693380 Compare May 10, 2021 15:38
Copy link
Contributor

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

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

Looking good! Just need some comments as it's a little hard to parse.

clippy_lints/src/inherent_impl.rs Show resolved Hide resolved
clippy_lints/src/inherent_impl.rs Show resolved Hide resolved
@Jarcho Jarcho force-pushed the multiple_impls_generic branch 2 times, most recently from 57d488a to bd230fa Compare May 12, 2021 20:37
@camsteffen
Copy link
Contributor

Please squash commits and r=me

@camsteffen
Copy link
Contributor

@bors delegate+

@bors
Copy link
Contributor

bors commented May 16, 2021

✌️ @Jarcho can now approve this pull request

Treat different generic arguments as different types.
Allow the lint to be ignored on the type definition, or any impl blocks.
@Jarcho Jarcho force-pushed the multiple_impls_generic branch from bd230fa to 760f703 Compare May 18, 2021 15:47
@Jarcho
Copy link
Contributor Author

Jarcho commented May 18, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 18, 2021

📌 Commit 760f703 has been approved by Jarcho

@bors
Copy link
Contributor

bors commented May 18, 2021

⌛ Testing commit 760f703 with merge aa1959b...

@bors
Copy link
Contributor

bors commented May 18, 2021

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

@bors bors merged commit aa1959b into rust-lang:master May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multiple_inherent_impl false positive
5 participants