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

Deduplicate bounds on associated types when deriving #92793

Closed

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Jan 11, 2022

polonius uses a trait with a few associated types to define which newtyped indexes represent CFG points, loans, etc. rustc makes no effort to deduplicate bounds when deriving common traits on structures whose fields mention associated types, resulting in very large, redundant where clauses like this one. I find this mildly annoying when reading documentation.

This PR adds deduplication of simple paths (no generic arguments or qualified self types) to the derive machinery. As a result, the derived impl I linked above has only a single bound for each associated type, as one might expect.

A remnant from the before times...
"Simple" uses have no qualified self type or generic parameters. They're
just a path (e.g. `T::Item`).
@rust-highfive
Copy link
Collaborator

r? @wesleywiser

(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 Jan 11, 2022
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 11, 2022
@ecstatic-morse
Copy link
Contributor Author

I didn't see a test for derived impls on declarations referencing associated types. Am I missing something obvious? AFAIK, there's no way to test that deduplication is actually occurring in a UI test, but we should have something to ensure that I haven't broken anything.

@rust-log-analyzer

This comment has been minimized.

@wesleywiser
Copy link
Member

I didn't see a test for derived impls on declarations referencing associated types. Am I missing something obvious? AFAIK, there's no way to test that deduplication is actually occurring in a UI test, but we should have something to ensure that I haven't broken anything.

I found derive-assoc-type-not-impl.rs which seems slightly related. It would be good to have more comprehensive tests for derives with associated types.

Since you noticed the issue via rustdoc, perhaps a rustdoc test would be able to show the deduplication is occurring?

@ecstatic-morse ecstatic-morse added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 20, 2022
@bors
Copy link
Contributor

bors commented Jan 21, 2022

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

@JohnCSimon
Copy link
Member

Ping from Triage:
@ecstatic-morse this has sat idle for a month with an approval... can you please post the status of this PR? Thanks.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 20, 2022
@Dylan-DPC
Copy link
Member

Closing this as inactive

@Dylan-DPC Dylan-DPC closed this Aug 1, 2022
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants