-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Conversation
This comment has been minimized.
This comment has been minimized.
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>, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. ^^'
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. ;)
07a00fe
to
cca7a31
Compare
@bors r+ |
📌 Commit cca7a31f98358a46679b4a9d821bdb8a4fa08483 has been approved by It is now in the queue for this repository. |
There was a problem hiding this 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
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>, |
There was a problem hiding this comment.
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.
cca7a31
to
2ce7cd9
Compare
@bors r=notriddle,lcnr |
…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`
…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
[rustdoc] Fix infinite loop when retrieving impls for type alias Fixes rust-lang#112515. Reverts rust-lang#112429. r? `@lcnr`
…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`
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`
Fixes #32077.
Thanks a lot to @lcnr who helped me a lot with this fix!
cc @notriddle
r? @lcnr