Fix ICE in rustdoc when impl is nested in a func by GoldsteinE · Pull Request #147911 · rust-lang/rust (original) (raw)

For cases like

fn foo() { impl Trait for Bar where E: Baz {} }

hir_enclosing_body_owner() in rustdoc considered fn foo() to be the “enclosing body” of E, which caused to to produce HirIds for E with owner set to fn foo() instead of the impl. This instead treats E as having no enclosing body.

r? @GuillaumeGomez as the author of this logic, I’m not 100% I understood it correctly.

Fixes #147882.
Fixes #147057.

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.

T-rustdoc-frontend

Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.

labels

Oct 20, 2025

I'll side step here and let @fmease review this as they're the expert of this topic.

r? fmease

fmease is currently at their maximum review capacity.
They may take a while to respond.

Haven't looked at the impl yet; I guess this also fixes #147057 cuz #147882 is likely a dupe of the former?

I’ve checked this, and yes, it fixes #147057. Updated the top post accordingly.

Choose a reason for hiding this comment

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

Thanks for looking into this! I suspect a more principled fix would be to get rid of this function and use tcx.typeck(tcx.hir_get_parent_item(hir_id).def_id) at its call-sites instead of tcx.typeck_body(<result of this func>). Could you try that instead and see if that works?

View changes since this review

rustbot 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

Nov 9, 2025

For context: I recently had a conversation with @fmease, and they mentioned they're planning to refactor or revise this part of the code, which they had approved in the review. So, I think we should wait for their input on this matter

@camelid, typeck also only works on things with bodies. But I agree that a more principled approach should be chosen, I just didn't have the time to investigate what would be best in order to propose something here.

Choose a reason for hiding this comment

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

Please move this test to tests/rustdoc/jump-to-def/

@camelid, typeck also only works on things with bodies. But I agree that a more principled should be chosen, I just didn't have the time to investigate what would be best in order to propose something here.

In general, we currently can't obtain the resolution (DefId) of arbitrary type-relative things given the HIR alone. Namely of ones located in non-bodies / item signatures / places where we don't perform type inference. For bodies we have typeck but for non-bodies we would only have HIR ty lowering (which we certainly don't want to do again) or ditching the HIR and going through the rustc_middle::ty::Ty provided by type_of (which is lossy as long as we don't have lazy type aliases by default).

It's been a while since I looked at this part of the code but yeah for now it's okay to ignore TypeRelative QPaths "in ItemCtxts" … which this PR + other parts of the code kinda do but I'm certain it can be expressed a lot more robustly and nicely.

Also CC this beautiful table I've created: #135771 (comment).

@camelid, typeck also only works on things with bodies. But I agree that a more principled should be chosen, I just didn't have the time to investigate what would be best in order to propose something here.

In general, we currently can't obtain the resolution (DefId) of arbitrary type-relative things given the HIR alone. Namely of ones located in non-bodies / item signatures / places where we don't perform type inference. For bodies we have typeck but for non-bodies we would only have HIR ty lowering (which we certainly don't want to do again) or ditching the HIR and going through the rustc_middle::ty::Ty provided by type_of (which is lossy as long as we don't have lazy type aliases by default).

It's been a while since I looked at this part of the code but yeah for now it's okay to ignore TypeRelative QPaths "in ItemCtxts" … which this PR + other parts of the code kinda do but I'm certain it can be expressed a lot more robustly and nicely.

Also CC this beautiful table I've created: #135771 (comment).

Ah, right, I forgot about this annoyance.... Do you know what we currently do to resolve QPaths in the rest of rustdoc (not jump-to-def)? E.g. when rendering types that are trait assoc items etc.