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

[WIP] Distinguish between links on inner and outer attributes #78611

Closed
wants to merge 1 commit into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Oct 31, 2020

Normally, rustdoc will resolve //! docs in the scope inside the module
and /// in the scope of the parent. But if there are modules with
inner docs, it would previously resolve all of the links inside the module.

This now distinguishes between links that came from inside the module
and links that came from outside. The intra-doc links part works, but
unfortunately the rest of rustdoc assumes there is only one canonical
resolution for a link and won't look for more than one on the same item.

  • Store the attribute style on each DocFragment
  • Don't combine attributes with different styles
  • Calculate the parent module in only one place
  • Remove outdated and fixed FIXME comments

This came from a failed attempt to work on #78591 and there's some debugging for that left over. Let me know if I should remove it.

r? @GuillaumeGomez - do you have suggestions for how to pass this information to the rest of rustdoc? For context, the issue is that given a list of links like

vec![ItemLink { link: "a", did: Some(DefId(0)) }, ItemLink { link: "a", did: Some(DefId(1)) }]

rustdoc::html will always pick the first resolution in the array. Maybe the link should also say whether it's an inner or outer link, and have html distinguish too?

cc @Manishearth - technically this is a breaking change, but I consider the old behavior a bug.

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name labels Oct 31, 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 31, 2020
@jyn514

This comment has been minimized.

Normally, rustdoc will resolve `//!` docs in the scope inside the module
and `///` in the scope of the parent. But if there are modules with
inner docs, it would previously resolve _all_ of the links inside the module.

This now distinguishes between links that came from inside the module
and links that came from outside. The intra-doc links part works, but
unfortunately the rest of rustdoc assumes there is only one canonical
resolution for a link and won't look for more than one on the same item.

- Store the attribute style on each DocFragment
- Don't combine attributes with different styles
- Calculate the parent module in only one place
- Remove outdated and fixed FIXME comments
@GuillaumeGomez
Copy link
Member

Question: wouldn't it be simpler to first treat inner docs and then outer docs for each item? Therefore, you could change the parent_node depending on the type and makes your life a bit easier no? (and that could even allow to have a smaller fold_item function :p)

@jyn514
Copy link
Member Author

jyn514 commented Nov 1, 2020

first treat inner docs and then outer docs for each item

This is what it does currently, no? Or do you mean to have separate clean::Items for inner and outer attributes? That seems fragile, I don't know how the rest of rustdoc will react to duplicate items.

@GuillaumeGomez
Copy link
Member

From what I understand, not exactly. We have a weird mix between the two and then treat them as one. What I meant was to literally try to split both handling so that the parent id isn't "special".

Comment on lines +892 to +900
let base_node = if !seen_inner_docs && item.is_mod() && attr.style == AttrStyle::Inner {
// FIXME(jynelson): once `Self` handling is cleaned up I think we can get rid
// of `mod_ids` altogether
self.mod_ids.push(item.def_id);
seen_inner_docs = true;
Some(item.def_id)
} else {
parent_node
};
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to also update parent_node or any following attributes will still use the outer scope.

Suggested change
let base_node = if !seen_inner_docs && item.is_mod() && attr.style == AttrStyle::Inner {
// FIXME(jynelson): once `Self` handling is cleaned up I think we can get rid
// of `mod_ids` altogether
self.mod_ids.push(item.def_id);
seen_inner_docs = true;
Some(item.def_id)
} else {
parent_node
};
if !seen_inner_docs && item.is_mod() && attr.style == AttrStyle::Inner {
// FIXME(jynelson): once `Self` handling is cleaned up I think we can get rid
// of `mod_ids` altogether
self.mod_ids.push(item.def_id);
seen_inner_docs = true;
parent_node = Some(item.def_id);
}

and then change base_node to parent_node below.

@bors
Copy link
Contributor

bors commented Nov 12, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@crlf0710
Copy link
Member

Triage: CI is still red here.

@crlf0710 crlf0710 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 28, 2020
@jyn514
Copy link
Member Author

jyn514 commented Nov 28, 2020

I'm not convinced this is actually useful ... It doesn't fix the ICE and a lot of the rest of rustdoc assumes this is the same. It might be easier to just forbid intra-doc-links to appear on both inner and outer attributes.

@jyn514 jyn514 added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 29, 2020
@jyn514 jyn514 marked this pull request as draft November 29, 2020 00:37
@jyn514
Copy link
Member Author

jyn514 commented Jan 22, 2021

I don't plan to follow up on this.

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 S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

5 participants