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

Compare trait references in trait_duplication_in_bounds correctly #13493

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

y21
Copy link
Member

@y21 y21 commented Oct 3, 2024

Fixes #13476
Fixes #11067
Fixes #9915
Fixes #9626

Currently, the trait_duplication_in_bounds lints has a helper type for a trait reference that can be used for comparison and hashing, represented as {trait: Res, generic_args: Vec<Res>}. However, there are a lot of issues with this. For one, a Res can't represent e.g. references, slices, or lots of other types, as well as const generics and associated type equality. In those cases, the lint simply ignores them and has no way of checking if they're actually the same.

So, instead of using Res for this, use SpanlessEq and SpanlessHash for comparisons with the trait path for checking if there are duplicates.

However, using SpanlessEq as is alone lead to a false negative in the test. std::clone::Clone + foo::Clone wasn't recognized as a duplicate, because it has different segments. So this also adds a new "mode" to SpanlessEq which compares by final resolution. (I've been wondering if this can't just be the default but it's quite a large scale change as it affects a lot of lints and I haven't yet looked at all uses of it to see if there are lints that really do care about having exactly the same path segments).

Maybe an alternative would be to turn the hir types/consts into middle types/consts and compare them instead but I'm not sure there's really a good way to do that

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2024

r? @Manishearth

rustbot has assigned @Manishearth.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 3, 2024
@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 3, 2024

📌 Commit d6be597 has been approved by Manishearth

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 3, 2024

⌛ Testing commit d6be597 with merge a01975b...

@bors
Copy link
Contributor

bors commented Oct 3, 2024

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

@bors bors merged commit a01975b into rust-lang:master Oct 3, 2024
8 checks passed
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
4 participants