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.

Sign up for GitHub

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

Jump to bottom

rustdoc: Always warn when linking from public to private items #74460

Merged
merged 2 commits into from
Jul 23, 2020
Merged

rustdoc: Always warn when linking from public to private items #74460

merged 2 commits into from
Jul 23, 2020

Conversation

dennis-hamester
Copy link
Contributor

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
Copy link
Collaborator

r? @nikomatsakis

(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 Jul 17, 2020
@dennis-hamester
Copy link
Contributor Author

r? @jyn514

@rust-highfive rust-highfive assigned jyn514 and unassigned nikomatsakis Jul 17, 2020
jyn514
jyn514 reviewed Jul 17, 2020
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@jyn514 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
Copy link
Contributor Author

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
Copy link
Member

jyn514 commented Jul 18, 2020

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
Copy link
Member

jyn514 commented Jul 18, 2020

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
Copy link
Contributor Author

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
Copy link
Member

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

@Manishearth
Copy link
Member

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
Copy link
Contributor Author

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

@dennis-hamester
rustdoc: Always warn when linking from public to private items
ed53de0 
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.
@dennis-hamester
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.
@jyn514
Copy link
Member

jyn514 commented Jul 22, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jul 22, 2020

📌 Commit c14641a has been approved by jyn514

@bors
Copy link
Contributor

bors commented Jul 22, 2020

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

@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 Jul 22, 2020
@dennis-hamester
Copy link
Contributor Author

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

@Manishearth Manishearth mentioned this pull request Jul 22, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 23, 2020
@bors
Auto merge of rust-lang#74662 - Manishearth:rollup-jdt7t71, r=Manishe…
e8b55a4 
…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
@bors bors merged commit 02e350f into rust-lang:master Jul 23, 2020
@dennis-hamester dennis-hamester deleted the rustdoc-warn-pub-to-priv branch July 23, 2020 05:02
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

Assignees

@jyn514 jyn514

Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name 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
Milestone
1.47.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@dennis-hamester @rust-highfive @jyn514 @Manishearth @bors @cuviper @nikomatsakis