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

Warn on broken intra-doc links added to cross-crate re-exports #77276

Merged
merged 7 commits into from
Oct 9, 2020

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Sep 27, 2020

This emits broken_intra_doc_links for docs applied to pub use statements that point to external items and are inlined.
Does not address #77200 - any existing broken links from the original crate will not show warnings.

r? @jyn514

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 27, 2020
@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Sep 27, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 27, 2020

@GuillaumeGomez can you add an example with a broken link on the original docs, before it was re-exported? Like this:

// crate1
/// [not_found]
pub fn f() {}

// crate2
pub use crate1::f;

cc @rust-lang/rustdoc, this is a change in behavior and undoes #56922
cc @euclio

@jyn514 jyn514 added A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. labels Sep 27, 2020
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Sep 27, 2020

Sure! Just to be sure: we don't want a warning in this case, right?

@jyn514
Copy link
Member

jyn514 commented Sep 27, 2020

I'm not sure about that. Consider the examples from #77200:

As a library author I would want to know about those I think, instead of having them silently fail. At least then you could work around it somehow.

@jyn514
Copy link
Member

jyn514 commented Sep 27, 2020

That said, I'm fine with only reporting lints on new documentation for now so we can get the fix in, this would unblock #77254.

src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved
@Manishearth
Copy link
Member

As a library author I would want to know about those I think, instead of having them silently fail. At least then you could work around it somehow.

I'm wary of reporting bugs that take updating a dependency to fix. As long as it gets reported in the dependency (which will show up in CI logs!) it should be fine IMO

@jyn514
Copy link
Member

jyn514 commented Sep 28, 2020

I'm wary of reporting bugs that take updating a dependency to fix. As long as it gets reported in the dependency (which will show up in CI logs!) it should be fine IMO

Well that's the issue, some of these won't show up in the dependency. So there's no way to see them other than warning on downstream crates, and we're currently silencing those warnings.

@GuillaumeGomez
Copy link
Member Author

Updated!

@jyn514
Copy link
Member

jyn514 commented Sep 28, 2020

@GuillaumeGomez just double checking - this only affects intra-doc links for now, right? Everywhere else ignores use statements? If so r=me and we can debate whether this should lint the original warning in re-exports later (currently it does not): #77276 (comment)

@GuillaumeGomez
Copy link
Member Author

Only intra-doc links as far as I can tell. But all other lints are now able to run checks on them as well, as long as they want to.

@bors: r=jyn514

@bors
Copy link
Contributor

bors commented Sep 28, 2020

📌 Commit 1f80f981ae4b56d3f91d81432c41d0b25eff2eae has been approved by jyn514

@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 28, 2020
@ollie27
Copy link
Member

ollie27 commented Sep 28, 2020

This now displays re-exports for items even when they have been inlined. Here's std for example:

image

That can't be intentional, right?

@jyn514
Copy link
Member

jyn514 commented Sep 28, 2020

@bors r-

No, that wasn't intentional. @GuillaumeGomez can you fix that?

@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 28, 2020
@GuillaumeGomez
Copy link
Member Author

So I just need to check for doc(inline) I assume? I'll do that tomorrow then.

@jyn514
Copy link
Member

jyn514 commented Sep 28, 2020

So I just need to check for doc(inline)

Well, I think the fix needs to happen in render, not in clean. Some parts of rustdoc (intra-doc links) are interested in the use and some (render) are not. So each needs to decide on their own whether to ignore it or not.

@Manishearth
Copy link
Member

Well that's the issue, some of these won't show up in the dependency. So there's no way to see them other than warning on downstream crates, and we're currently silencing those warnings.

Wait, why not? Can you give me an example?

@jyn514
Copy link
Member

jyn514 commented Sep 29, 2020

Wait, why not? Can you give me an example?

#77276 (comment): #77193, #75855. The first one is just a rustdoc bug, but it's possible there are more like it. The second one is a legitimate difference between documenting normally and documenting as a dependency.

@GuillaumeGomez GuillaumeGomez force-pushed the reexported-item-lints branch 3 times, most recently from e1f4847 to 31251ae Compare October 5, 2020 13:10
@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 5, 2020
@jyn514 jyn514 changed the title Reexported item lints Warn on broken intra-doc links added to cross-crate re-exports Oct 5, 2020
@jyn514 jyn514 added the A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name label Oct 5, 2020
@GuillaumeGomez
Copy link
Member Author

Just waiting on you then @ollie27. :)

@bors
Copy link
Contributor

bors commented Oct 9, 2020

☔ The latest upstream changes (presumably #77519) 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

@jyn514
Copy link
Member

jyn514 commented Oct 9, 2020

#77254 is fixed so people can now fix the warnings they get from broken links :)

@GuillaumeGomez
Copy link
Member Author

Since I removed the filter on compiler added imports, we can move on.

@bors: r=jyn514,ollie27

@bors
Copy link
Contributor

bors commented Oct 9, 2020

📌 Commit 7e218bb has been approved by jyn514,ollie27

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

bors commented Oct 9, 2020

⌛ Testing commit 7e218bb with merge 38d911d...

@bors
Copy link
Contributor

bors commented Oct 9, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: jyn514,ollie27
Pushing 38d911d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 9, 2020
@bors bors merged commit 38d911d into rust-lang:master Oct 9, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 9, 2020
@GuillaumeGomez GuillaumeGomez deleted the reexported-item-lints branch October 10, 2020 12:34
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 A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. 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.

7 participants