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

[rustdoc] List matching impls on type aliases #112429

Merged
merged 3 commits into from
Jun 9, 2023

Conversation

GuillaumeGomez
Copy link
Member

Fixes #32077.

Thanks a lot to @lcnr who helped me a lot with this fix!

cc @notriddle
r? @lcnr

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 8, 2023
@rust-log-analyzer

This comment has been minimized.

src/librustdoc/html/render/print_item.rs Outdated Show resolved Hide resolved
fn render_assoc_items_inner(
mut w: &mut dyn fmt::Write,
cx: &mut Context<'_>,
containing_item: &clean::Item,
it: DefId,
what: AssocItemRender<'_>,
derefs: &mut DefIdSet,
aliased_type: Option<DefId>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure threading this parameter through all these function calls is probably cheaper than using let is_alias = tcx.def_kind(it) == DefKind::TyAlias and using let aliased_type = tcx.type_of(it) to resolve it, since the calling functions already have this information.

But is pulling the information out of TyCtxt expensive enough to be worth having a function with seven parameters?

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Jun 8, 2023

Choose a reason for hiding this comment

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

We need a variable to tell us that this is an alias. From this, since we already have the DefId, why not using it directly?

EDIT: Just re-read your comment. I'm not sure to like it much simply because we re-compute an information we already have. But that's pretty much my only argument against it. ^^'

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Jun 8, 2023

Choose a reason for hiding this comment

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

I edited my comment, I don't know how but I misread your comment the first time. Sorry about that.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's not a lot of computation going into it, though. It's looking information up in a hash table.

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's not the part worrying me but since I can't put the finger on it, I'll just remove the newly added argument and let's see how it goes. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah found it! I need it because of Some(aliased_type) => cache.impls.get(&aliased_type).unwrap_or(&empty),. We can't retrieve the DefId easily from a Ty. The only method I found to get it is ty_adt_id but it doesn't cover all the cases we need.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. Thanks for the info!

Copy link
Contributor

Choose a reason for hiding this comment

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

Having aliased_type separately here feels off to me and may at least deserve a comment, but that's unrelated to the type system itself so I am fine with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

The doc comment for aliased_type is already on the function. ;)

@notriddle
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 8, 2023

📌 Commit cca7a31f98358a46679b4a9d821bdb8a4fa08483 has been approved by notriddle

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 8, 2023
@notriddle
Copy link
Contributor

Wait, no. You wanted a review from @lcnr

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 8, 2023
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

r=me after nit

src/librustdoc/html/render/mod.rs Show resolved Hide resolved
fn render_assoc_items_inner(
mut w: &mut dyn fmt::Write,
cx: &mut Context<'_>,
containing_item: &clean::Item,
it: DefId,
what: AssocItemRender<'_>,
derefs: &mut DefIdSet,
aliased_type: Option<DefId>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Having aliased_type separately here feels off to me and may at least deserve a comment, but that's unrelated to the type system itself so I am fine with that.

@GuillaumeGomez
Copy link
Member Author

@bors r=notriddle,lcnr

@bors
Copy link
Contributor

bors commented Jun 9, 2023

📌 Commit 2ce7cd9 has been approved by notriddle,lcnr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 9, 2023
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 9, 2023
…otriddle,lcnr

[rustdoc] List matching impls on type aliases

Fixes rust-lang#32077.

Thanks a lot to `@lcnr` who helped me a lot with this fix!

cc `@notriddle`
r? `@lcnr`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 9, 2023
…llaumeGomez

Rollup of 3 pull requests

Successful merges:

 - rust-lang#112260 (Improve document of `unsafe_code` lint)
 - rust-lang#112429 ([rustdoc] List matching impls on type aliases)
 - rust-lang#112442 (Deduplicate identical region constraints in new solver)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f83c8e4 into rust-lang:master Jun 9, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 9, 2023
@GuillaumeGomez GuillaumeGomez deleted the ty-alias-impls branch June 9, 2023 18:54
@GuillaumeGomez GuillaumeGomez restored the ty-alias-impls branch June 12, 2023 09:07
@GuillaumeGomez GuillaumeGomez deleted the ty-alias-impls branch June 12, 2023 09:07
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 12, 2023
[rustdoc] Fix infinite loop when retrieving impls for type alias

Fixes rust-lang#112515.
Reverts rust-lang#112429.

r? `@lcnr`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 8, 2023
…l-list, r=GuillaumeGomez

rustdoc: list matching impls on type aliases

Fixes rust-lang#32077

Fixes rust-lang#99952

Remake of rust-lang#112429

Partially reverts rust-lang#112543, but keeps the test case.

This version of the PR avoids the infinite loop by structurally matching types instead of using full unification. This version does not support type alias trait bounds, but the compiler does not enforce those anyway (rust-lang#21903).

r? `@GuillaumeGomez`

CC `@lcnr`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 8, 2023
Rollup merge of rust-lang#115201 - notriddle:notriddle/type-alias-impl-list, r=GuillaumeGomez

rustdoc: list matching impls on type aliases

Fixes rust-lang#32077

Fixes rust-lang#99952

Remake of rust-lang#112429

Partially reverts rust-lang#112543, but keeps the test case.

This version of the PR avoids the infinite loop by structurally matching types instead of using full unification. This version does not support type alias trait bounds, but the compiler does not enforce those anyway (rust-lang#21903).

r? `@GuillaumeGomez`

CC `@lcnr`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rustdoc doesn't handle impl section of type definitions
6 participants