intra-doc: Use the impl's assoc item where possible #92680

camelid · 2022-01-08T23:02:38Z

Before, the trait's associated item would be used. Now, the impl's
associated item is used. The only exception is for impls that use
default values for associated items set by the trait. In that case,
the trait's associated item is still used.

As an example of the old and new behavior, take this code:

trait MyTrait {
    type AssocTy;
}

impl MyTrait for String {
    type AssocTy = u8;
}

Before, when resolving a link to String::AssocTy,
resolve_associated_trait_item would return the associated item for
MyTrait::AssocTy. Now, it would return the associated item for
<String as MyTrait>::AssocTy, as it claims in its docs.

r? @petrochenkov

rust-highfive · 2022-01-08T23:02:40Z

Some changes occurred in clean/types.rs.

cc @camelid

camelid · 2022-01-08T23:03:06Z

Only the last commit is new; the others are from the PRs this is blocked on.

camelid · 2022-01-10T17:52:41Z

@petrochenkov I know this PR is blocked, but since the queue is moving very slowly, could you review it a bit anyway? Only the last commit is new, so you can just look at that one.

Before, the trait's associated item would be used. Now, the impl's associated item is used. The only exception is for impls that use default values for associated items set by the trait. In that case, the trait's associated item is still used. As an example of the old and new behavior, take this code: trait MyTrait { type AssocTy; } impl MyTrait for String { type AssocTy = u8; } Before, when resolving a link to `String::AssocTy`, `resolve_associated_trait_item` would return the associated item for `MyTrait::AssocTy`. Now, it would return the associated item for `<String as MyTrait>::AssocTy`, as it claims in its docs.

camelid · 2022-01-19T00:06:40Z

Ok, this is ready for review now! I rebased to get rid of the commits that have since landed on master.

petrochenkov · 2022-01-20T13:57:52Z

src/librustdoc/passes/collect_intra_doc_links.rs

                    ItemFragment(FragmentKind::Method, def_id)
+                } else {
+                    ItemFragment(FragmentKind::TyMethod, def_id)


What is the difference between Method and TyMethod?
(The whole FragmentKind enum doesn't make much sense to me.)

A method is a method that has a body, while a tymethod is a prototype. The distinction is unnecessary and I already have an open PR that merges them.

What doesn't make sense to you about FragmentKind? I'm happy to try to explain it.

What doesn't make sense to you about FragmentKind?

The set of variants it has.
Why are associated items (including variants) combined with fields?
What are those text pieces that fn render produce and why do they need to be different?

A FragmentKind represents some sort of "projection" out of a type. So, since fields are conceptually "children" of a type, as are variants and associated items, they get a FragmentKind.

The ultimate reason why FragmentKind exists is that rustdoc only generates independent pages for some kinds of items. For example, structs and free functions get their own pages, but fields, variants, and associated items are just sections on their parent type's page. In most places, this is captured by returning a Res for the page and a FragmentKind representing the section within that page. Then the Res is turned into a path like ../foo/struct.Bar.html and the FragmentKind is turned into a URL fragment like #structfield.baz.

Does that explanation help?

Yes, that makes it more clear.

petrochenkov · 2022-01-20T14:06:12Z

src/librustdoc/passes/collect_intra_doc_links.rs

+/// not overriden in the impl.
+///
+/// This is just a wrapper around [`TyCtxt::impl_item_implementor_ids()`] and
+/// [`TyCtxt::associated_item()`] (with some helpful logging added).


The day you wrote this code was likely the last day this logging was helpful, so if I maintained this code I'd remove all of it and inline the function. But there are apparently people that are ok with keeping this kind of useless noise around their code.

I agree that the logging is a bit noisy, but the code relating to associated item lookup has had some subtle issues. I think this will make it easier to debug those, as I expect more bugs to be found later on.

The main "subtle issue" I see here is that rustdoc shouldn't be doing this work at all, and delegate all the associated item resolution to methods in rustc_typeck, until that is done you will always have subtle and not so subtle issues.

Sure, I think replacing custom code with calls to rustc is a great idea.

petrochenkov · 2022-01-20T14:18:49Z

@bors r+

bors · 2022-01-20T14:18:51Z

📌 Commit 29a2d6b has been approved by petrochenkov

bors · 2022-01-20T14:18:52Z

🌲 The tree is currently closed for pull requests below priority 600. This pull request will be tested once the tree is reopened.

…askrgr Rollup of 13 pull requests Successful merges: - rust-lang#89747 (Add MaybeUninit::(slice_)as_bytes(_mut)) - rust-lang#89764 (Fix variant index / discriminant confusion in uninhabited enum branching) - rust-lang#91606 (Stabilize `-Z print-link-args` as `--print link-args`) - rust-lang#91694 (rustdoc: decouple stability and const-stability) - rust-lang#92183 (Point at correct argument when async fn output type lifetime disagrees with signature) - rust-lang#92582 (improve `_` constants in item signature handling) - rust-lang#92680 (intra-doc: Use the impl's assoc item where possible) - rust-lang#92704 (Change lint message to be stronger for &T -> &mut T transmute) - rust-lang#92861 (Rustdoc mobile: put out-of-band info on its own line) - rust-lang#92992 (Help optimize out backtraces when disabled) - rust-lang#93038 (Fix star handling in block doc comments) - rust-lang#93108 (:arrow_up: rust-analyzer) - rust-lang#93112 (Fix CVE-2022-21658) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

camelid added the A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name label Jan 8, 2022

rust-highfive assigned petrochenkov Jan 8, 2022

rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 8, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 8, 2022

petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2022

camelid force-pushed the assoc-item-cleanup branch from 90f065a to 7523d5e Compare January 11, 2022 03:25

camelid force-pushed the assoc-item-cleanup branch from 7523d5e to 29a2d6b Compare January 19, 2022 00:05

camelid marked this pull request as ready for review January 19, 2022 00:06

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jan 19, 2022

petrochenkov reviewed Jan 20, 2022

View reviewed changes

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 Jan 20, 2022

matthiaskrgr mentioned this pull request Jan 20, 2022

Rollup of 13 pull requests #93119

Merged

bors merged commit 1839829 into rust-lang:master Jan 20, 2022

rustbot added this to the 1.60.0 milestone Jan 20, 2022

camelid deleted the assoc-item-cleanup branch January 21, 2022 01:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

intra-doc: Use the impl's assoc item where possible #92680

intra-doc: Use the impl's assoc item where possible #92680

camelid commented Jan 8, 2022 •

edited

Loading

rust-highfive commented Jan 8, 2022

camelid commented Jan 8, 2022

camelid commented Jan 10, 2022

camelid commented Jan 19, 2022

petrochenkov Jan 20, 2022

camelid Jan 20, 2022

petrochenkov Jan 21, 2022

camelid Jan 21, 2022

petrochenkov Jan 21, 2022

petrochenkov Jan 20, 2022

camelid Jan 20, 2022

petrochenkov Jan 21, 2022

camelid Jan 21, 2022

petrochenkov commented Jan 20, 2022

bors commented Jan 20, 2022

bors commented Jan 20, 2022

intra-doc: Use the impl's assoc item where possible #92680

intra-doc: Use the impl's assoc item where possible #92680

Conversation

camelid commented Jan 8, 2022 • edited Loading

rust-highfive commented Jan 8, 2022

camelid commented Jan 8, 2022

camelid commented Jan 10, 2022

camelid commented Jan 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Jan 20, 2022

bors commented Jan 20, 2022

bors commented Jan 20, 2022

camelid commented Jan 8, 2022 •

edited

Loading