Skip to content

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Apr 5, 2021

Since compiler/ always passes --document-private-items, it's ok to link to items that are private.

Since compiler/ always passes --document-private-items, it's ok to link
to items that are private.
@jyn514 jyn514 added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 5, 2021
@rust-highfive
Copy link
Contributor

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 5, 2021
@jyn514
Copy link
Member Author

jyn514 commented Apr 5, 2021

Note that this came to mind because I fixed an existing bug in the lint: #83849 (comment). So we are missing out on dogfooding a bit. But I don't want people to think they should remove links because of the lint.

@@ -549,6 +549,8 @@ impl Step for Rustc {
// Build cargo command.
let mut cargo = builder.cargo(compiler, Mode::Rustc, SourceType::InTree, target, "doc");
cargo.rustdocflag("--document-private-items");
// Since we always pass --document-private-items, there's no need to warn about linking to private items.
cargo.rustdocflag("-Arustdoc::private-intra-doc-links");
Copy link
Member

Choose a reason for hiding this comment

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

Hm, should this perhaps go in builder::cargo along with the other lint configs, gated on Mode::Rustc?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it should go next to --document-private-items, wherever that is, so people can see they're related. Happy to move --document-private-items to builder::cargo if you think it's useful.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason rustdoc isn't changing the default lint level when that flag is passed?

Copy link
Member Author

@jyn514 jyn514 Apr 5, 2021

Choose a reason for hiding this comment

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

It's actually part of the error message:

error: public documentation for `ModuleData` links to private item `ModuleData::kind`
    |
    |
496 | /// You can use [`ModuleData::kind`] to determine the kind of module this is.
    |
    |
    = note: `-D rustdoc::private-intra-doc-links` implied by `-D warnings`
    = note: this link resolves only because you passed `--document-private-items`, but will break without

I do think it should be something intentional, I expect at least some people only document locally with --document-private-items and I think they should still see the warning unless they opt out. Otherwise the links will be broken on docs.rs.

Copy link
Member

Choose a reason for hiding this comment

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

Hm. I'm not sure I buy that argument; users likely run in CI without document-private-items and so will see the error there (as warnings are likely to be denied). But regardless, that seems like an orthogonal discussion. I'm happy to merge this.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 5, 2021

📌 Commit 0a351ab has been approved by Mark-Simulacrum

@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 5, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 5, 2021
…crum

Document compiler/ with -Aprivate-intra-doc-links

Since compiler/ always passes --document-private-items, it's ok to link to items that are private.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 5, 2021
…crum

Document compiler/ with -Aprivate-intra-doc-links

Since compiler/ always passes --document-private-items, it's ok to link to items that are private.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 5, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#83370 (Add `x.py setup tools` which enables `download-rustc` by default)
 - rust-lang#83489 (Properly suggest deref in else block)
 - rust-lang#83734 (Catch a bad placeholder type error for statics in `extern`s)
 - rust-lang#83814 (expand: Do not ICE when a legacy AST-based macro attribute produces and empty expression)
 - rust-lang#83835 (rustdoc: sort search index items for compression)
 - rust-lang#83849 (rustdoc: Cleanup handling of associated items for intra-doc links)
 - rust-lang#83881 (:arrow_up: rust-analyzer)
 - rust-lang#83885 (Document compiler/ with -Aprivate-intra-doc-links)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d8c04b1 into rust-lang:master Apr 6, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 6, 2021
@jyn514 jyn514 deleted the private-links branch April 6, 2021 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler 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