rustdoc: Always warn when linking from public to private items by dennis-hamester · Pull Request #74460 · rust-lang/rust

dennis-hamester · 2020-07-17T20:49:21Z

Change the logic such that linking from a public to a private item always triggers intra_doc_link_resolution_failure.
Previously, the warning was not emitted when --document-private-items is passed.

This came up during the discussion in #74147 (comment).

rust-highfive · 2020-07-17T20:49:24Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

dennis-hamester · 2020-07-17T20:50:10Z

r? @jyn514

jyn514

Maybe this is a little over-protective after all, the warnings will still show up if you don't pass --document-private-items. I'm willing to be convinced either way.

src/test/rustdoc-ui/issue-74134.rs

dennis-hamester · 2020-07-18T06:47:01Z

Maybe this is a little over-protective after all, the warnings will still show up if you don't pass --document-private-items. I'm willing to be convinced either way.

I don't have a strong opinion on this as well. Your comment in #74147 (comment) did make sense to me and still does imo.

How does the current stabilization plan relate to this? Is it possible to change when a warning triggers after intro-doc links hit stable?

jyn514 · 2020-07-18T14:26:11Z

I think my concern is that the lint name intra_doc_link_resolution_failure makes it look like the link is broken. But with --document-private-items it's actually just a regular warning. Maybe we could distinguish between the two by adding something like this?

# for private docs
note: this link will currently works because you passed `--document-private-items`, but will break without
# for public docs
note: this link will resolve properly if you pass `--document-private items`

How does the current stabilization plan relate to this? Is it possible to change when a warning triggers after intro-doc links hit stable?

See #43466 (comment):

When it comes to lints the general Rust stabilty model is that new warn-by-default lints are always okay, deny-by-default lints should usually go through some cycles of warn-by-default unless there are special circumstances.

jyn514 · 2020-07-18T14:47:16Z

Another alternative is make this a separate lint altogether: warn(intra_doc_link_footguns). But then the lint would be different with and without private items and I'm not sure if that's great either. Sorry to be so wishy washy 😆

dennis-hamester · 2020-07-19T06:13:53Z

I see your point about the lint's name not quite fitting the --document-private-items case. But adding an additional lint just for that "fringe" case seems a bit overkill to me, especially when we already have a lint triggering without --document-private-items. That flag would then only replace one lint with another.

Considering that, the suggested additional note: lines sound like a good middle ground to me. Letting the user know that --document-private-items has an influence here is a good UX improvement imo.

There is of course the third option of doing nothing for now.

@jyn514? Maybe @Manishearth has an opinion as well?

Manishearth · 2020-07-22T15:41:23Z

I think an additional note would work! I think it's fine to consider this a "resolution failure".

Manishearth · 2020-07-22T15:42:11Z

Note that when you attempt to import something private from a different crate, rustc throws a similar resolution error with a note iirc.

dennis-hamester · 2020-07-22T18:20:34Z

I went ahead and added notes for both cases as suggested.

Change the logic such that linking from a public to a private item always triggers intra_doc_link_resolution_failure. Previously, the warning was not emitted when --document-private-items is passed. Also don't rely anymore on the item's visibility, which would falsely trigger the lint now that the check for --document-private-items is gone.

…links The additional note helps explaining why the lint was triggered and that --document-private-items directly influences the link resolution.

jyn514 · 2020-07-22T19:46:36Z

@bors r+

bors · 2020-07-22T19:46:37Z

📌 Commit c14641a has been approved by jyn514

bors · 2020-07-22T19:46:38Z

🌲 The tree is currently closed for pull requests below priority 5, this pull request will be tested once the tree is reopened

dennis-hamester · 2020-07-22T19:46:54Z

Rebased onto master and I also took the opportunity and squashed that one fixup commit.

@ghost

…arth Rollup of 9 pull requests Successful merges: - rust-lang#73783 (Detect when `'static` obligation might come from an `impl`) - rust-lang#73868 (Advertise correct stable version for const control flow) - rust-lang#74460 (rustdoc: Always warn when linking from public to private items) - rust-lang#74538 (Guard against non-monomorphized type_id intrinsic call) - rust-lang#74541 (Add the aarch64-apple-darwin target ) - rust-lang#74600 (Enable perf try builder) - rust-lang#74618 (Do not ICE on assoc type with bad placeholder) - rust-lang#74631 (rustc_target: Add a target spec option for disabling `--eh-frame-hdr`) - rust-lang#74643 (build: Remove unnecessary `cargo:rerun-if-env-changed` annotations) Failed merges: r? @ghost

rust-highfive assigned nikomatsakis Jul 17, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 17, 2020

rust-highfive assigned jyn514 and unassigned nikomatsakis Jul 17, 2020

jyn514 reviewed Jul 17, 2020

View reviewed changes

src/test/rustdoc-ui/issue-74134.rs Outdated Show resolved Hide resolved

jyn514 added A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 17, 2020

dennis-hamester added 2 commits July 22, 2020 21:36

rustdoc: Add explanation when linting against public to private item …

c14641a

…links The additional note helps explaining why the lint was triggered and that --document-private-items directly influences the link resolution.

dennis-hamester force-pushed the rustdoc-warn-pub-to-priv branch from a99e2b1 to c14641a Compare July 22, 2020 19:44

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 Jul 22, 2020

Manishearth mentioned this pull request Jul 22, 2020

Rollup of 9 pull requests #74662

Merged

bors merged commit 02e350f into rust-lang:master Jul 23, 2020

dennis-hamester deleted the rustdoc-warn-pub-to-priv branch July 23, 2020 05:02

cuviper added this to the 1.47.0 milestone May 2, 2024

Uh oh!

Conversation

dennis-hamester commented Jul 17, 2020

Uh oh!

rust-highfive commented Jul 17, 2020

Uh oh!

dennis-hamester commented Jul 17, 2020

Uh oh!

jyn514 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dennis-hamester commented Jul 18, 2020

Uh oh!

jyn514 commented Jul 18, 2020

Uh oh!

jyn514 commented Jul 18, 2020

Uh oh!

dennis-hamester commented Jul 19, 2020

Uh oh!

Manishearth commented Jul 22, 2020

Uh oh!

Manishearth commented Jul 22, 2020

Uh oh!

dennis-hamester commented Jul 22, 2020

Uh oh!

jyn514 commented Jul 22, 2020

Uh oh!

bors commented Jul 22, 2020

Uh oh!

bors commented Jul 22, 2020

Uh oh!

dennis-hamester commented Jul 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants