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

Consider warning when intra-doc links fail on a cross-crate re-export #77200

Closed
jyn514 opened this issue Sep 25, 2020 · 5 comments
Closed

Consider warning when intra-doc links fail on a cross-crate re-export #77200

jyn514 opened this issue Sep 25, 2020 · 5 comments
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 I-needs-decision Issue: In need of a decision. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Sep 25, 2020

This is the exact opposite of #56922 😆

There have been a lot of silent failures popping up WRT cross-crate re-exports (#77193, #75855, #76106), and they're hard to notice because the failure is completely swallowed, you have to manually inspect the generated docs.

The rationale in #56922 was that

Since the downstream crate has no way to fix the resolution failure in the upstream crate, it shouldn't be reported, much like capping lints.

This is only true if the failure existed in the upstream crate. If it only appears on the re-export, it will fail completely silently for both the original crate and the current one. This is especially bad when you're adding additional docs on the re-export:

/// [broken_link]
pub use std::string::String;

It could also appear for things outside of the control of the new crate, though: #75855

I think it also makes sense to warn for rustdoc bugs, since otherwise the user never knows to report them and we can't fix them.

@rust-lang/rustdoc what do you think?

cc @euclio

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. I-needs-decision Issue: In need of a decision. A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name labels Sep 25, 2020
@jyn514
Copy link
Member Author

jyn514 commented Sep 25, 2020

See also my write-up in #77193 (comment): possibly we could make [broken_link] warn while still hiding failures from the original crate (although I'm not sure how). That still wouldn't catch rustdoc bugs, though.

@GuillaumeGomez
Copy link
Member

If your crate reexports an item, the reexport itself is part of your crate, so if there is a broken link in the documentation you added on it, we should definitely emit a warning.

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 9, 2020
… r=jyn514,ollie27

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

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

r? `@jyn514`
@camelid camelid added the A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate label Oct 12, 2020
@dhardy
Copy link
Contributor

dhardy commented Dec 7, 2020

cargo-deadlinks does report the broken link. I guess that tool should be obsolete now maybe?

Is it not possible to just resolve the issue by considering links in doc inherited via a re-export as being relative to the crate it was defined in? E.g. the doc in rand::distributions::Uniform references Rng (rand::Rng), which does not exist in rand_distr, causing an error when the doc on re-export rand_distr::Uniform is checked. But if rand_distr::Rng did exist allowing the link to be resolved, it would be wrong (if this was not also a re-export).

Edit: I guess this should be in a new issue maybe — but finding related issues on this repo is hard enough as it is. 🤷

@jyn514
Copy link
Member Author

jyn514 commented Dec 7, 2020

cargo-deadlinks does report the broken link. I guess that tool should be obsolete now maybe?

(disclaimer: I'm the current maintainer of deadlinks.) cargo-deadlinks is not obsolete for a few reasons:

  1. It can be used on non-rust projects, as a normal linkchecker.
  2. People still use manual links, intra-doc links are not (and will never be) a hard requirement. So there's still value in checking those.
  3. Rustdoc doesn't always report link failures, for example the ones in this issue.

Is it not possible to just resolve the issue by considering links in doc inherited via a re-export as being relative to the crate it was defined in?

This is #74481 and is specific to facade crates. Other crates will break if you change the scope you resolve them in for re-exports, see #65983.

JohnTitor added a commit to JohnTitor/rust that referenced this issue 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.
@jyn514
Copy link
Member Author

jyn514 commented Oct 11, 2021

I think it's pretty clear by now this won't happen - fortunately most of the bugs in rustdoc have been fixed.

@jyn514 jyn514 closed this as completed Oct 11, 2021
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 I-needs-decision Issue: In need of a decision. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants