Skip to content
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

Fix intra-doc links for Self on cross-crate items and primitives #76467

Merged
merged 3 commits into from
Nov 30, 2020

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Sep 8, 2020

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

@jyn514 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
@rust-highfive

This comment has been minimized.

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

@bors r+

much nicer, thanks

@bors
Copy link
Contributor

bors commented Sep 8, 2020

📌 Commit c90f2ff207aaa3ae45421add0215690095730ccb has been approved by Manishearth

@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 Sep 8, 2020
@jyn514
Copy link
Member Author

jyn514 commented Sep 8, 2020

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

@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 Sep 8, 2020
@jyn514
Copy link
Member Author

jyn514 commented Sep 8, 2020

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
Copy link
Member Author

jyn514 commented Sep 8, 2020

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
Copy link
Member Author

jyn514 commented Sep 8, 2020

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
Copy link
Member Author

jyn514 commented Sep 8, 2020

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

@bors

This comment has been minimized.

@jyn514
Copy link
Member Author

jyn514 commented Sep 13, 2020

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
Copy link
Member Author

jyn514 commented Sep 13, 2020

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

@bors

This comment has been minimized.

@jyn514
Copy link
Member Author

jyn514 commented Sep 15, 2020

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.

RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 25, 2020
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
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2020
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`
@bors

This comment has been minimized.

@jyn514
Copy link
Member Author

jyn514 commented Nov 29, 2020

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
Copy link
Member Author

jyn514 commented Nov 29, 2020

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

@jyn514 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
- 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 jyn514 marked this pull request as ready for review November 29, 2020 19:56
@jyn514
Copy link
Member Author

jyn514 commented Nov 29, 2020

@Manishearth this is actually ready to go this time 😆

@jyn514 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
@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 30, 2020

📌 Commit 2b17f02 has been approved by Manishearth

@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 Nov 30, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 30, 2020
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
Copy link
Contributor

bors commented Nov 30, 2020

⌛ Testing commit 2b17f02 with merge b7ebc6b...

@bors
Copy link
Contributor

bors commented Nov 30, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 30, 2020
@bors bors merged commit b7ebc6b into rust-lang:master Nov 30, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 30, 2020
@jyn514 jyn514 deleted the intra-link-self branch November 30, 2020 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name C-bug Category: This is a bug. C-cleanup Category: PRs that clean code up or issues documenting cleanup. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants