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

Resolve intra-doc links on additional documentation for re-exports in lexical scope #77519

Merged
merged 3 commits into from
Oct 9, 2020

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Oct 4, 2020

Fixes #77254.

  • Preserve the parent module of DocFragments

    • Add parent_module to DocFragment

    • Require the parent_module of the item being inlined

    • Preserve the hir_id for ExternCrates so rustdoc can find the parent module later

    • Take an optional parent_module for build_impl and merge_attrs.
      Preserve the difference between parent modules for each doc-comment.

    • Support a single additional re-exports in from_ast. Originally this took a vec but I ended up not using it.

    • Don't require the parent_module for all impls, just inlined items

      In particular, this will be None whenever the attribute is not on a
      re-export.

    • Only store the parent_module, not the HirId

      When re-exporting a re-export, the HirId is not available. Fortunately,
      collect_intra_doc_links doesn't actually need all the info from a
      HirId, just the parent module.

  • Introduce Divider

    This distinguishes between documentation on the original from docs on the re-export.

  • Use the new module information for intra-doc links

    • Make the parent module conditional on whether the docs are on a re-export

    • Make resolve_link take &Item instead of &mut Item

      Previously the borrow checker gave an error about multiple mutable
      borrows, because dox borrowed from item.

    • Fix crate:: for re-exports

      crate means something different depending on where the attribute
      came from.

    • Make it work for #[doc] attributes too

      This required combining several attributes as one so they would keep
      the links.

r? @GuillaumeGomez

@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Oct 4, 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 Oct 4, 2020
@jyn514

This comment has been minimized.

@jyn514
Copy link
Member Author

jyn514 commented Oct 4, 2020

run collapse docs on one module at a time.

One suggestion @Nemo157 had was to add a new DocFragmentKind::Divider which distinguishes between doc-comments on the original item and the comments on the export. Then collapse_docs wouldn't need to change at all, since it already distinguishes between attributes of different kinds.

@jyn514
Copy link
Member Author

jyn514 commented Oct 4, 2020

add a new DocFragmentKind::Divider

This seems to have worked pretty well.

@jyn514
Copy link
Member Author

jyn514 commented Oct 4, 2020

It works! Just need to add test cases.

@jyn514
Copy link
Member Author

jyn514 commented Oct 4, 2020

The FIXME by the new behavior in intra-doc links will break a lot of code in practice, see e.g. https://docs.rs/async-compression/0.3.5/src/async_compression/lib.rs.html#1-240. Another alternative is to combine all consecutive attributes with the same module id into the same markdown link.

I don't plan to support that sort of inter-dependence across re-exports, though.

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 4, 2020
@bors

This comment has been minimized.

@jyn514 jyn514 force-pushed the track-doc-er branch 3 times, most recently from 5f60702 to 3f84863 Compare October 8, 2020 04:15
@jyn514 jyn514 changed the title [WIP] Preserve the parent module of DocFragments Preserve the parent module of DocFragments Oct 8, 2020
@jyn514 jyn514 changed the title Preserve the parent module of DocFragments Resolve intra-doc links on additional documentation for re-exports in lexical scope Oct 8, 2020
@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 Oct 8, 2020
@jyn514
Copy link
Member Author

jyn514 commented Oct 8, 2020

This is ready for review.

@jyn514 jyn514 added the A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate label Oct 8, 2020
- Add `parent_module` to `DocFragment`
- Require the `parent_module` of the item being inlined
- Preserve the hir_id for ExternCrates so rustdoc can find the parent module later
- Take an optional `parent_module` for `build_impl` and `merge_attrs`.
  Preserve the difference between parent modules for each doc-comment.
- Support arbitrarily many re-exports in from_ast. In retrospect this is
  probably not used and could be simplified to a single
  `Option<(Attrs, DefId)>`.
- Don't require the parent_module for all `impl`s, just inlined items

  In particular, this will be `None` whenever the attribute is not on a
  re-export.

- Only store the parent_module, not the HirId

  When re-exporting a re-export, the HirId is not available. Fortunately,
  `collect_intra_doc_links` doesn't actually need all the info from a
  HirId, just the parent module.
This distinguishes between documentation on the original from docs on
the re-export
- Make the parent module conditional on whether the docs are on a re-export
- Make `resolve_link` take `&Item` instead of `&mut Item`

  Previously the borrow checker gave an error about multiple mutable
  borrows, because `dox` borrowed from `item`.

- Fix `crate::` for re-exports

  `crate` means something different depending on where the attribute
  came from.

- Make it work for `#[doc]` attributes too

  This required combining several attributes as one so they would keep
  the links.
@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 8, 2020

📌 Commit e39a860 has been approved by GuillaumeGomez

@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 Oct 8, 2020
@bors
Copy link
Contributor

bors commented Oct 9, 2020

⌛ Testing commit e39a860 with merge 9ba1d21...

@bors
Copy link
Contributor

bors commented Oct 9, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: GuillaumeGomez
Pushing 9ba1d21 to master...

@dylni
Copy link
Contributor

dylni commented Oct 11, 2020

Thanks! This is working well except for #77783.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 2, 2021
Update intra-doc link documentation to match the implementation

r? `@Manishearth`
cc `@camelid` `@m-ou-se`

Relevant PRs:
- rust-lang#74489
- rust-lang#80181
- rust-lang#76078
- rust-lang#77519
- rust-lang#73101

Relevant issues:
- rust-lang#78800
- rust-lang#77200
- rust-lang#77199 / rust-lang#54191

I haven't documented things that I consider 'just bugs', like rust-lang#77732, but I have documented features that aren't implemented, like rust-lang#78800.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name 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.

Additional docs on pub re-exports resolve intra-doc links relative to the original module
6 participants