Skip to content

[rustdoc] Add support for associated items in "jump to def" feature #135771

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jan 20, 2025

Fixes #135485.

r? @fmease

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 20, 2025
@GuillaumeGomez
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 20, 2025
@bors
Copy link
Collaborator

bors commented Jan 20, 2025

⌛ Trying commit 0a62c9e with merge 884d495...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2025
…try>

[PERF for "jump to def"] Re-enable "jump to def" feature on rustc docs

This PR is NOT meant to be merged. `@fmease` and I are using it to check perfs on the "jump to def" feature.

r? `@fmease`
@bors
Copy link
Collaborator

bors commented Jan 20, 2025

☀️ Try build successful - checks-actions
Build commit: 884d495 (884d4952c02212f92d6d2dd3e1c804fd1aa1cd24)

@rust-timer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Second commit is the interesting part: it overloads visit_qpath.

@@ -231,6 +231,24 @@ impl<'tcx> Visitor<'tcx> for SpanMapVisitor<'tcx> {
self.handle_pat(p);
}

fn visit_qpath(&mut self, qpath: &QPath<'tcx>, id: HirId, span: Span) {
Copy link
Member

@fmease fmease Jan 20, 2025

Choose a reason for hiding this comment

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

You should now be able to remove the visit_pat and handle_pat methods entirely thanks to this overwrite. Unless I'm forgetting things.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately no because of this case:

match self {
    // Doesn't work.
    Self::Ok(_) => {}
    // Works.
    MyEnum::Err(_) => {}
    // Doesn't work
    Self::Some(_) => {}
    // Works.
    Self::None => {}
}

Copy link
Member

Choose a reason for hiding this comment

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

Right, I'm pretty sure that there's a way to do it (by using the maybe_typeck_results pattern I mentioned back then on Zulip). After all, the cleanup part is all about getting rid of {visit,handle}_pat for me personally.

I'm gonna be afk in a sec, so I can't explain the maybe_typeck_results approach rn and I don't know if you can decipher my past rambling on Zulip. Well, you can check out rustc since it uses the "maybe_typeck_results pattern" quite frequently.

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 fine. Gonna give it a try and if I can't gonna let you do it since you seem to have a clear implementation in mind.

Copy link
Member

Choose a reason for hiding this comment

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

I'll experiment later. Ofc, you can always try reading through hir::intravisit and see which methods don't get called in this PR which do get called on master, maybe there's a more obvious 'bug'.

Copy link
Member Author

Choose a reason for hiding this comment

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

I uncovered the maybe_typeck_results dark arcanes and fixed it. I wonder if there is a shorter way though, code still seems longer than it needs to be...

Copy link
Member

Choose a reason for hiding this comment

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

I'll check that soon. In any case, since this fixes things / adds support for more path types, I'd say it'd be fine even if we couldn't simplify it.

Also, with this method rustdoc should now be able to resolve fully-qualified paths (<$Type as $TraitRef>::$assoc), too, and that in all positions (expr, pat, type). On master, we don't support those at all iirc. However, I haven't double-checked if your impl is sufficient or if you still need to hook up some things. Could you add tests for them if this PR makes them work?

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (884d495): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 1.9%, secondary 2.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.9% [0.8%, 2.9%] 4
Regressions ❌
(secondary)
2.6% [1.3%, 4.1%] 5
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.9% [0.8%, 2.9%] 4

Cycles

Results (secondary 2.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.1% [2.1%, 2.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 767.44s -> 767.097s (-0.04%)
Artifact size: 325.97 MiB -> 326.00 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 20, 2025
@GuillaumeGomez
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 20, 2025
@bors
Copy link
Collaborator

bors commented Jan 20, 2025

⌛ Trying commit bcd4e2e with merge 4f36aac...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2025
…try>

[PERF for "jump to def"] Re-enable "jump to def" feature on rustc docs

This PR is NOT meant to be merged. `@fmease` and I are using it to check perfs on the "jump to def" feature.

Used for rust-lang#135485.

r? `@fmease`
@bors
Copy link
Collaborator

bors commented Jan 20, 2025

☀️ Try build successful - checks-actions
Build commit: 4f36aac (4f36aac4606855ed0367291dac471c76ff3e5851)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4f36aac): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 1.8%, secondary 0.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.8% [1.8%, 1.8%] 1
Regressions ❌
(secondary)
1.9% [1.8%, 1.9%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.7% [-2.7%, -2.7%] 1
All ❌✅ (primary) 1.8% [1.8%, 1.8%] 1

Cycles

Results (secondary -2.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-2.9%, -2.9%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 766.257s -> 767.122s (0.11%)
Artifact size: 325.96 MiB -> 325.97 MiB (0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 20, 2025
@fmease fmease 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 25, 2025
@GuillaumeGomez
Copy link
Member Author

Made more changes which had some interesting side-effects: we now have items in a same path that can generate links. However, projections still link to where the item is defined in the trait and not in the trait implementation, which is a bit sad...

You can check it here.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented May 6, 2025

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

@GuillaumeGomez
Copy link
Member Author

Fixed merge conflict.

@fmease
Copy link
Member

fmease commented May 18, 2025

Did it get linkified?

rust-lang:master

Context Kind <T as C>::X <T>::X <T as C>::X(…) <T>::X(…)
Item Ty n/a n/a
Item Ct n/a (mGCA) n/a n/a
Item Fn (RTN) n/a n/a
Expr Ty n/a n/a
Expr Ct
Expr Fn
Pat Ty n/a n/a n/a n/a
Pat Ct n/a (mGCA) n/a n/a
Pat Fn n/a n/a n/a n/a

GuillaumeGomez:jump-to-def-perf

Context Kind <T as C>::X <T>::X <T as C>::X(…) <T>::X(…)
Item Ty n/a n/a
Item Ct n/a (mGCA) n/a n/a
Item Fn (RTN) n/a n/a
Expr Ty n/a n/a
Expr Ct
Expr Fn
Pat Ty n/a n/a n/a n/a
Pat Ct n/a (mGCA) n/a n/a
Pat Fn n/a n/a n/a n/a

Code Under Test

#![feature(return_type_notation)]

pub trait Trait {
    type Assoc;
    const ASSOC: usize;
    const ASSOC1: fn();
    fn assoc() -> impl Sized;
}
impl Trait for () {
    type Assoc = ();
    const ASSOC: usize = 0;
    const ASSOC1: fn() = || {};
    fn assoc() -> impl Sized {}
}

pub struct Item0<T: Trait>(pub <T as Trait>::Assoc);
pub struct Item1<T: Trait>(pub T::Assoc);

pub fn expr0<T: Trait>() { let _: <T as Trait>::Assoc; }
pub fn expr1<T: Trait>() { let _: T::Assoc; }

pub struct Item2([(); <() as Trait>::ASSOC]);

pub struct Item3<T: Trait>(T) where <T as Trait>::assoc(..):;
pub struct Item4<T: Trait>(T) where T::assoc(..):;

pub fn expr2<T: Trait>() { _ = <T as Trait>::ASSOC; }
pub fn expr3<T: Trait>() { _ = T::ASSOC; }

pub fn expr4<T: Trait>() { _ = <T as Trait>::assoc; _ = <T as Trait>::assoc(); }
pub fn expr5<T: Trait>() { _ = T::assoc; _ = T::assoc(); }

pub fn pat0() { match 0 { <() as Trait>::ASSOC => {}, _ => {} } }

pub fn expr6<T: Trait>() { _ = <T as Trait>::ASSOC1(); }
pub fn expr7<T: Trait>() { _ = T::ASSOC1(); }

@fmease
Copy link
Member

fmease commented Aug 1, 2025

There are still cases that don't work as seen in the table I've created. We can always iterate on this approach.

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Aug 1, 2025

📌 Commit 687ac3f has been approved by fmease

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 Aug 1, 2025
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 1, 2025
…=fmease

[rustdoc] Add support for associated items in "jump to def" feature

Fixes rust-lang#135485.

r? `@fmease`
bors added a commit that referenced this pull request Aug 1, 2025
Rollup of 10 pull requests

Successful merges:

 - #132748 (get rid of some false negatives in rustdoc::broken_intra_doc_links)
 - #135771 ([rustdoc] Add support for associated items in "jump to def" feature)
 - #143360 (loop match: error on `#[const_continue]` outside `#[loop_match]`)
 - #143662 ([rustdoc] Display unsafe attrs with edition 2024 `unsafe()` wrappers.)
 - #143900 ([rustdoc] Correctly handle `should_panic` doctest attribute and fix `--no-run` test flag on the 2024 edition)
 - #144614 (Fortify RemoveUnneededDrops test.)
 - #144703 ([test][AIX] ignore extern_weak linkage test)
 - #144738 (Remove the omit_gdb_pretty_printer_section attribute)
 - #144756 (detect infinite recursion with tail calls in ctfe)
 - #144766 (Add human readable name "Cygwin")

r? `@ghost`
`@rustbot` modify labels: rollup
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 1, 2025
…=fmease

[rustdoc] Add support for associated items in "jump to def" feature

Fixes rust-lang#135485.

r? ``@fmease``
bors added a commit that referenced this pull request Aug 1, 2025
Rollup of 11 pull requests

Successful merges:

 - #132748 (get rid of some false negatives in rustdoc::broken_intra_doc_links)
 - #135771 ([rustdoc] Add support for associated items in "jump to def" feature)
 - #143360 (loop match: error on `#[const_continue]` outside `#[loop_match]`)
 - #143662 ([rustdoc] Display unsafe attrs with edition 2024 `unsafe()` wrappers.)
 - #143900 ([rustdoc] Correctly handle `should_panic` doctest attribute and fix `--no-run` test flag on the 2024 edition)
 - #144478 (Improve formatting of doc code blocks)
 - #144703 ([test][AIX] ignore extern_weak linkage test)
 - #144747 (compiletest: Improve diagnostics for line annotation mismatches 2)
 - #144756 (detect infinite recursion with tail calls in ctfe)
 - #144766 (Add human readable name "Cygwin")
 - #144782 (Properly pass path to staged `rustc` to `compiletest` self-tests)

r? `@ghost`
`@rustbot` modify labels: rollup
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 1, 2025
…=fmease

[rustdoc] Add support for associated items in "jump to def" feature

Fixes rust-lang#135485.

r? ```@fmease```
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 1, 2025
…=fmease

[rustdoc] Add support for associated items in "jump to def" feature

Fixes rust-lang#135485.

r? ````@fmease````
bors added a commit that referenced this pull request Aug 2, 2025
Rollup of 12 pull requests

Successful merges:

 - #132748 (get rid of some false negatives in rustdoc::broken_intra_doc_links)
 - #135771 ([rustdoc] Add support for associated items in "jump to def" feature)
 - #143360 (loop match: error on `#[const_continue]` outside `#[loop_match]`)
 - #143662 ([rustdoc] Display unsafe attrs with edition 2024 `unsafe()` wrappers.)
 - #144478 (Improve formatting of doc code blocks)
 - #144703 ([test][AIX] ignore extern_weak linkage test)
 - #144747 (compiletest: Improve diagnostics for line annotation mismatches 2)
 - #144766 (Add human readable name "Cygwin")
 - #144782 (Properly pass path to staged `rustc` to `compiletest` self-tests)
 - #144786 (Cleanup the definition of `group_type`)
 - #144796 (Add my previous commit name to .mailmap)
 - #144797 (Update safety comment for new_unchecked in niche_types)

r? `@ghost`
`@rustbot` modify labels: rollup
samueltardieu added a commit to samueltardieu/rust that referenced this pull request Aug 2, 2025
…=fmease

[rustdoc] Add support for associated items in "jump to def" feature

Fixes rust-lang#135485.

r? `````@fmease`````
bors added a commit that referenced this pull request Aug 2, 2025
Rollup of 18 pull requests

Successful merges:

 - #132748 (get rid of some false negatives in rustdoc::broken_intra_doc_links)
 - #135771 ([rustdoc] Add support for associated items in "jump to def" feature)
 - #143360 (loop match: error on `#[const_continue]` outside `#[loop_match]`)
 - #143662 ([rustdoc] Display unsafe attrs with edition 2024 `unsafe()` wrappers.)
 - #143771 (Constify some more `Result` functions)
 - #143900 ([rustdoc] Correctly handle `should_panic` doctest attribute and fix `--no-run` test flag on the 2024 edition)
 - #144185 (Document guarantees of poisoning)
 - #144395 (update fortanix tests)
 - #144478 (Improve formatting of doc code blocks)
 - #144614 (Fortify RemoveUnneededDrops test.)
 - #144703 ([test][AIX] ignore extern_weak linkage test)
 - #144747 (compiletest: Improve diagnostics for line annotation mismatches 2)
 - #144756 (detect infinite recursion with tail calls in ctfe)
 - #144766 (Add human readable name "Cygwin")
 - #144782 (Properly pass path to staged `rustc` to `compiletest` self-tests)
 - #144786 (Cleanup the definition of `group_type`)
 - #144796 (Add my previous commit name to .mailmap)
 - #144797 (Update safety comment for new_unchecked in niche_types)

Failed merges:

 - #144805 (compiletest: Preliminary cleanup of `ProcRes` printing/unwinding)

r? `@ghost`
`@rustbot` modify labels: rollup
@GuillaumeGomez
Copy link
Member Author

Panicked in #144799.

@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 Aug 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overload visit_qpath instead of using visit_pat in rustdoc "jump to def" implementation
7 participants