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

rustdoc: Use DiagnosticInfo in more parts of intra-doc links #83875

Merged
merged 8 commits into from
Apr 6, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Apr 5, 2021

This makes the code a lot less verbose.

This is separated into lots of tiny commits because it was easier for me that way, but the overall diff isn't that big if you want to read it at once.

r? @bugadani

@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-diagnostics Area: Messages for errors, warnings, and lints A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2021
@jyn514
Copy link
Member Author

jyn514 commented Apr 5, 2021

@bors delegate=bugadani

@bors
Copy link
Contributor

bors commented Apr 5, 2021

✌️ @bugadani can now approve this pull request

@bors

This comment has been minimized.

Copy link
Contributor

@bugadani bugadani left a comment

Choose a reason for hiding this comment

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

Most of this is straight forward unless I missed a subtle change somewhere where the transformation is not equivalent.

IIRC when we were working on this part, I was considering going all-in with DiagnosticInfo but I think I didn't feel comfortable enough with the change in L1121 (the one I commented on). I think that one might need some double checking.

I think there might still exist a slight inconsistency as DiagnosticInfo is passed by value in most of the places, but by ref in a few others. No big deal though IMO.

path_str,
dox,
ori_link.range,
diag_info,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might be a slight change in behaviour as path_str != ori_link.link. I don't remember the details but if this doesn't trip a test, maybe we should have one that covers it?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a bug to ever pass in path_str here, otherwise it won't show the anchor at the end. Here's an example with the old diagnostic:

warning: `usize` contains an anchor, but links to builtin types are already anchored
 --> anchor.rs:1:6
  |
1 | //! [prim@usize#x]
  |      ^^^^^^^^^^^^ contains invalid anchor
  |
  = note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default

I'll add a test.

FWIW, the reason I pass path_str separately from diag_info in some places is exactly so the caller isn't responsible for remembering to update it.

@jyn514
Copy link
Member Author

jyn514 commented Apr 5, 2021

I think there might still exist a slight inconsistency as DiagnosticInfo is passed by value in most of the places, but by ref in a few others. No big deal though IMO.

Yeah, I ran into some borrow checker issues ... I really wish there were some fix for rust-lang/rfcs#2848.

ori_link contains anchors, path_str does not. It's important that
anchor_failure be passed a link with the anchors still present.
This gets rid of a lot of parameters, as well as fixing a diagnostic
bug.
It's not the range of the full link, it's only a partial range.
@@ -43,3 +43,7 @@ pub fn enum_link() {}
/// [u32#hello]
//~^ ERROR `u32#hello` contains an anchor
pub fn x() {}

/// [prim@usize#x]
Copy link
Contributor

@bugadani bugadani Apr 5, 2021

Choose a reason for hiding this comment

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

Actually, why is this an error? Shouldn't this link to an anchor in a page like primitive.usize.html? I guess this also goes for the test case above ([u32#hello]). I think I would find a link to usize#examples-3 actually reasonable. What am I missing?

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 is #83083, it's a bug but pretty hard to fix.

@bugadani
Copy link
Contributor

bugadani commented Apr 6, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Apr 6, 2021

📌 Commit a86a740 has been approved by bugadani

@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 Apr 6, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 6, 2021
rustdoc: Use DiagnosticInfo in more parts of intra-doc links

This makes the code a lot less verbose.

This is separated into lots of tiny commits because it was easier for me that way, but the overall diff isn't that big if you want to read it at once.

r? `@bugadani`
@bors
Copy link
Contributor

bors commented Apr 6, 2021

⌛ Testing commit a86a740 with merge 16143d1...

@bors
Copy link
Contributor

bors commented Apr 6, 2021

☀️ Test successful - checks-actions
Approved by: bugadani
Pushing 16143d1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 6, 2021
@bors bors merged commit 16143d1 into rust-lang:master Apr 6, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 6, 2021
@jyn514 jyn514 deleted the diag_info branch April 6, 2021 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name 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.

4 participants