Fix intra-doc links for `Self` on cross-crate items and primitives by jyn514 · Pull Request #76467 · rust-lang/rust

jyn514 · 2020-09-08T04:58:18Z

Remove the difference between parent_item and current_item; these
should never have been different.
Remove current_item from resolve and variant_field so that
Self is only substituted in one place at the very start.
Resolve the current item as a DefId, not a HirId. This is what
actually fixed the bug.

Hacks:

clean uses TypedefItem when it really should be
AssociatedTypeItem. I tried fixing this without success and hacked
around it instead (see comments)
This second-guesses the to_string() impl since it wants
fully-qualified paths. Possibly there's a better way to do this.

Manishearth · 2020-09-08T05:02:05Z

@bors r+

much nicer, thanks

bors · 2020-09-08T05:02:06Z

📌 Commit c90f2ff207aaa3ae45421add0215690095730ccb has been approved by Manishearth

jyn514 · 2020-09-08T05:55:09Z

@bors r-

Failed documenting libstd:

error: unresolved link to `sync::condvar::Condvar::notify_one`
   --> library/std/src/sync/condvar.rs:530:25
    |
530 |     /// [`notify_one`]: Self::notify_one
    |                         ^^^^^^^^^^^^^^^^ unresolved link
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`

error: aborting due to 15 previous errors

error: Could not document `std`.

jyn514 · 2020-09-08T13:22:50Z

The issue is that the hack I used for Impls isn't working. I called tcx.type_of(def_id), which is fine, but then used Debug output for it which doesn't seem to form a valid rust path. Let me try converting the HirId of the type to a DefId and calling item_name on that.

jyn514 · 2020-09-08T13:30:39Z

Hmm ... I'm starting to wonder if we should be doing textual substitution at all for Self::. Maybe we should just get the DefId and skip straight to associated items and variants? Need to figure out how that would work for primitives.

jyn514 · 2020-09-08T13:35:49Z

Ok yeah my sketch of an idea is that instead of turning this into a string, I turn it into a Res. Then I separate out all the logic for associated items, etc. and call that directly when I see Self. This would have to introduce the idea of a 'partial resolution' - I did part of that in #75756 but I can rewrite some of it here I guess :/

Actually to make this even more consistent, I could strip Self:: out of path_str altogether and always have a partial_res: Option<Res> that I pass to resolve, then there's very little logic that needs to change. This will screw with the link displayed even more though, I'd have to make a separate variable that distinguishes the original link from 'path we're looking up'.

jyn514 · 2020-09-08T13:58:12Z

Another possible hack is to prepend crate:: for local items. But I'm not really comfortable depending on Debug internals like that.

jyn514 · 2020-09-13T22:19:32Z

Ok, I have the rustdoc side of this mostly working, but I'm not sure how to pass the partial resolution to rustc_resolve.

@petrochenkov do you know how I would tell resolve 'Self should refer to this DefId'? It's not available on resolve_str_path_error, and I don't see a 'resolve_with_partial_res' function or anything like that.

jyn514 · 2020-09-13T22:33:58Z

I did find resolve_self, but that looks like it's for self, the current module, not Self, the current type.

jyn514 · 2020-09-15T14:08:04Z

Hold on a second ... Self always refers to a type. So any path segment following will be either

a variant or field, or
an associated item

So resolving that path segment doesn't need to go through resolve at all! It has all the info it needs available from tcx.

Petrochenkov, sorry for the ping, I think I figured it out.

@euclio

Refactor and fix intra-doc link diagnostics, and fix links to primitives Closes rust-lang#76925, closes rust-lang#76693, closes rust-lang#76692. Originally I only meant to fix rust-lang#76925. But the hack with `has_primitive` was so bad it was easier to fix the primitive issues than to try and work around it. Note that this still has one bug: `std::primitive::i32::MAX` does not resolve. However, this fixes the ICE so I'm fine with fixing the link in a later PR. This is part of a series of refactors to make rust-lang#76467 possible. This is best reviewed commit-by-commit; it has detailed commit messages. r? @euclio

Refactor and fix intra-doc link diagnostics, and fix links to primitives Closes rust-lang#76925, closes rust-lang#76693, closes rust-lang#76692. Originally I only meant to fix rust-lang#76925. But the hack with `has_primitive` was so bad it was easier to fix the primitive issues than to try and work around it. Note that this still has one bug: `std::primitive::i32::MAX` does not resolve. However, this fixes the ICE so I'm fine with fixing the link in a later PR. This is part of a series of refactors to make rust-lang#76467 possible. This is best reviewed commit-by-commit; it has detailed commit messages. r? `@euclio`

jyn514 · 2020-11-29T18:14:15Z

Rather than trying to rewrite all of intra-doc links at once, I changed this to use fully-qualified paths when stringifying DefIds. This is less reliable than using the DefId directly, but should let this land (and unblock #77700) without needing giant refactors.

I still need to add a test case for #77875 (comment), I haven't been able to make an MCVE.

jyn514 · 2020-11-29T18:16:29Z

cc @da-x - is there a better way to get the fully-qualified path of a type than the current hack? https://github.com/rust-lang/rust/blob/b2fe71488ffdaf9eadaabd25b5fd24e56512277a/src/librustdoc/passes/collect_intra_doc_links.rs#L852-L859

- Remove the difference between `parent_item` and `current_item`; these should never have been different. - Remove `current_item` from `resolve` and `variant_field` so that `Self` is only substituted in one place at the very start. - Resolve the current item as a `DefId`, not a `HirId`. This is what actually fixed the bug. Hacks: - `clean` uses `TypedefItem` when it _really_ should be `AssociatedTypeItem`. I tried fixing this without success and hacked around it instead (see comments) - This stringifies DefIds, then resolves them a second time. This is really silly and rustdoc should just use DefIds throughout. Fixing this is a larger task than I want to take on right now.

jyn514 · 2020-11-29T19:57:29Z

@Manishearth this is actually ready to go this time 😆

Manishearth · 2020-11-30T01:56:56Z

@bors r+

bors · 2020-11-30T01:56:58Z

📌 Commit 2b17f02 has been approved by Manishearth

Fix intra-doc links for `Self` on cross-crate items and primitives - Remove the difference between `parent_item` and `current_item`; these should never have been different. - Remove `current_item` from `resolve` and `variant_field` so that `Self` is only substituted in one place at the very start. - Resolve the current item as a `DefId`, not a `HirId`. This is what actually fixed the bug. Hacks: - `clean` uses `TypedefItem` when it _really_ should be `AssociatedTypeItem`. I tried fixing this without success and hacked around it instead (see comments) - This second-guesses the `to_string()` impl since it wants fully-qualified paths. Possibly there's a better way to do this.

bors · 2020-11-30T09:01:01Z

⌛ Testing commit 2b17f02 with merge b7ebc6b...

bors · 2020-11-30T11:24:11Z

☀️ Test successful - checks-actions
Approved by: Manishearth
Pushing b7ebc6b to master...

jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. C-bug Category: This is a bug. A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name labels Sep 8, 2020

This comment has been minimized.

Sign in to view

rust-highfive assigned Manishearth Sep 8, 2020

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

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 Sep 8, 2020

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 Sep 8, 2020

jyn514 mentioned this pull request Sep 10, 2020

Resolve items for cross-crate imports relative to the original module #73101

Merged

This comment has been minimized.

Sign in to view

jyn514 force-pushed the intra-link-self branch from c90f2ff to 4296022 Compare September 13, 2020 20:34

This comment has been minimized.

Sign in to view

jyn514 force-pushed the intra-link-self branch from 4296022 to 6d6b311 Compare September 15, 2020 12:07

jyn514 mentioned this pull request Sep 20, 2020

Refactor and fix intra-doc link diagnostics, and fix links to primitives #76955

Merged

This comment has been minimized.

Sign in to view

jyn514 force-pushed the intra-link-self branch from 6d6b311 to 82453c3 Compare September 27, 2020 18:38

jyn514 force-pushed the intra-link-self branch from 837dc4e to b2fe714 Compare November 29, 2020 18:12

jyn514 changed the title ~~Cleanup intra-doc link handling of Self~~ Fix intra-doc links for Self on cross-crate items and primitives Nov 29, 2020

jyn514 added 2 commits November 29, 2020 13:40

Remove TypeKind hack in favor of with_crate_prefix

aa8c9b0

jyn514 force-pushed the intra-link-self branch from b2fe714 to aa8c9b0 Compare November 29, 2020 18:42

Add test for cross-crate Self

2b17f02

jyn514 marked this pull request as ready for review November 29, 2020 19:56

jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 29, 2020

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 Nov 30, 2020

Dylan-DPC-zz mentioned this pull request Nov 30, 2020

Rollup of 7 pull requests #79550

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 30, 2020

bors merged commit b7ebc6b into rust-lang:master Nov 30, 2020

rustbot added this to the 1.50.0 milestone Nov 30, 2020

jyn514 deleted the intra-link-self branch November 30, 2020 12:25

jyn514 mentioned this pull request Dec 2, 2020

Self does not work in intra-doc links for primitives #75809

Closed

jyn514 mentioned this pull request Dec 12, 2020

Dogfood str_split_once() #79809

Merged

jyn514 mentioned this pull request Jan 10, 2021

Self on its own does not resolve in intra-doc links #77732

Closed

jyn514 mentioned this pull request Feb 17, 2021

rustdoc: Confusing intra-doc handling of Self in enum #82209

Closed

jyn514 mentioned this pull request May 9, 2021

Resolve Self in a submodule when the type is not in scope #84999

Closed

Uh oh!

Conversation

jyn514 commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Manishearth commented Sep 8, 2020

Uh oh!

bors commented Sep 8, 2020

Uh oh!

jyn514 commented Sep 8, 2020

Uh oh!

jyn514 commented Sep 8, 2020

Uh oh!

jyn514 commented Sep 8, 2020

Uh oh!

jyn514 commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jyn514 commented Sep 8, 2020

Uh oh!

This comment has been minimized.

jyn514 commented Sep 13, 2020

Uh oh!

jyn514 commented Sep 13, 2020

Uh oh!

This comment has been minimized.

jyn514 commented Sep 15, 2020

Uh oh!

This comment has been minimized.

jyn514 commented Nov 29, 2020

Uh oh!

jyn514 commented Nov 29, 2020

Uh oh!

jyn514 commented Nov 29, 2020

Uh oh!

Manishearth commented Nov 30, 2020

Uh oh!

bors commented Nov 30, 2020

Uh oh!

bors commented Nov 30, 2020

Uh oh!

bors commented Nov 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jyn514 commented Sep 8, 2020 •

edited

Loading

jyn514 commented Sep 8, 2020 •

edited

Loading