Fix ICE in rustdoc when impl is nested in a func

[GoldsteinE]

For cases like

fn foo() {
    impl<E> Trait for Bar<E>
    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.

[GuillaumeGomez]

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

r? fmease

[rustbot]

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

[fmease]

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

[GoldsteinE]

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

[camelid]

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

[Kivooeo]

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

[fmease]

@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.

[fmease]

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

[fmease]

@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]

@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.

[theemathas]

Does this also fix #149089 and #150153?

[Dylan-DPC]

@GoldsteinE any updates on this? thanks

[fmease]

(not the author) I'm currently working on an alternative fix.

[rust-bors]

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

[GuillaumeGomez]

Superseeded by #156413 so closing. Thanks a lot @GoldsteinE!