Uh oh!
There was an error while loading. Please reload this page.
Warn if linking to a private item - #72771
Merged
Merged
Conversation
MemberAuthor
Changes that should be made before merging:
|
MemberAuthor
Output for the example in #72769: |
rust-highfive
commented
May 30, 2020
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
jyn514force-pushed
the
rustdoc
branch
2 times, most recently
from
May 30, 2020 16:42
835e8d3 to
c093b26Comparejyn514
commented
May 30, 2020
MemberAuthor
My workaround to avoid the bugs from the markdown parser means that we no longer give a warning for each time the same broken link is seen, only the first time. Not sure how to fix this. |
rust-highfive
commented
May 30, 2020
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
jyn514force-pushed
the
rustdoc
branch
2 times, most recently
from
May 31, 2020 15:56
e1574be to
250da53Comparejyn514
commented
May 31, 2020
MemberAuthor
jyn514
commented
May 31, 2020
MemberAuthor
The way this works is by
|
rust-highfive
commented
May 31, 2020
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
jyn514
commented
Jun 12, 2020
MemberAuthor
Status: this is waiting on pulldown-cmark/pulldown-cmark#445. However, I think that bug was pre-existing (I just happened to notice while working on this PR). @GuillaumeGomez if I fix the test failures does this look good? I can add an ignored test case for the bug I spotted and fix it in a follow-up PR at some point. |
jyn514
commented
Jun 12, 2020
MemberAuthor
I opened #73264 for the secondary issue, it's not actually related to this PR. |
rust-highfive
commented
Jun 12, 2020
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
jyn514
commented
Jun 13, 2020
MemberAuthor
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author |
Manishearth
approved these changes
Jun 26, 2020
Manishearth
left a comment
Member
There was a problem hiding this comment.
r=me for this, but please clean up the commit history
might also be worth being a little bit more verbose in debug!()
Do you intend to wait for the pulldown-cmark PR to land?
GuillaumeGomez
requested changes
Jun 26, 2020
Uh oh!
There was an error while loading. Please reload this page.
- Pass around document_private a lot more - Add tests + Add tests for intra-doc links to private items + Add ignored tests for warnings in reference links
This was referenced Jun 26, 2020
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 27, 2020
…arth Rollup of 12 pull requests Successful merges: - rust-lang#72771 (Warn if linking to a private item) - rust-lang#72937 (Fortanix SGX target libunwind build process changes) - rust-lang#73485 (Perform obligation deduplication to avoid buggy `ExistentialMismatch`) - rust-lang#73529 (Add liballoc impl SpecFromElem for i8) - rust-lang#73579 (add missing doc links) - rust-lang#73627 (Shortcuts for min/max on double-ended BTreeMap/BTreeSet iterators) - rust-lang#73691 (Bootstrap: detect Windows based on sys.platform) - rust-lang#73694 (Document the Self keyword) - rust-lang#73718 (Document the super keyword) - rust-lang#73728 (Document some invariants correctly/more) - rust-lang#73738 (Remove irrelevant comment) - rust-lang#73765 (Remove blank line) Failed merges: r? @ghost
This was referenced Jun 27, 2020
dennis-hamester
commented
Jul 7, 2020
Contributor
With this MR, the following produces a warning: structPrivate;pubstructPublic{/// A private field with a [`Private`] type.private:Private,}Was this intentional? The field itself is not public, so it should be fine to link to a private type, right? |
GuillaumeGomez
commented
Jul 7, 2020
Member
I think warn in this case is valid as well. You link to a private item, so wether or not you're using it on a private item shouldn't matter in my opinion. You can see it as a case of "this item was public but is not private (but mistake or not), and rustdoc is warning about items linked by it and to it". |
jyn514
commented
Jul 7, 2020
MemberAuthor
@GuillaumeGomez I disagree. One of the use cases I was thinking about here was @dennis-hamester can you open an issue for this behavior? |
Manishearth
commented
Jul 7, 2020
Member
One potential fix is to only emit this warning for docs that are going to be emitted in the current run. Linking private types is useful in document-private-items: when that's enabled we shoudl disable this lint, and when that's disabled we should only emit the lint for docs that are going to be rendered |
dennis-hamester
commented
Jul 7, 2020
Contributor
I use this "pattern" a lot in my crates, to provide documentation for private items when building with Actually, I also tend to deny Does that mean linking to a private type is generally discouraged? Is there a better way? I'd like to provide documentation for internals as well, not just for the public api. |
Member
I agree with @jyn514, people use document-private-items and we shouldn't warn then.
It does, because if you use it on a private item then it's only going to be rendered with document-private-items, where privacy doesn't matter. |
GuillaumeGomez
commented
Jul 7, 2020
Member
In case the |
GuillaumeGomez
commented
Jul 7, 2020
Member
Hum, on second thought, it might make no sense to complain if it's a link from a private item. But I still that linking to a private item should emit a warning (unless the |
dennis-hamester
commented
Jul 7, 2020
Contributor
I opened #74134 and discussion should probably continue there. |
Manishearth
commented
Jul 7, 2020
Member
@GuillaumeGomez Yes, we should complain if you link to a private item from a public item, in non-private-document mode. The private-to-private links only make sense with document-private-items anyway, and it's weird to warn about them when that's disabled where they don't matter. |
GuillaumeGomez
commented
Jul 7, 2020
Member
@Manishearth Absolutely, took me a few minutes to see it. |
dennis-hamester added a commit
to dennis-hamester/rust
that referenced
this pull request
Jul 8, 2020
Fixesrust-lang#74134 After PR rust-lang#72771 this would trigger an intra_doc_link_resolution_failure warning when rustdoc is invoked without --document-private-items. Links from private items to private types are however never actually generated in that case and thus shouldn't produce a warning. These links are in fact a very useful tool to document crate internals. Tests are added for all 4 combinations of public/private items and link targets. Test 1 is the case mentioned above and fails without this commit. Tests 2 - 4 passed before already but are added nonetheless to prevent regressions.
Manishearth added a commit
to Manishearth/rust
that referenced
this pull request
Jul 13, 2020
…jyn514 rustdoc: Allow linking from private items to private types Fixesrust-lang#74134 After PR rust-lang#72771 this would trigger an intra_doc_link_resolution_failure warning when rustdoc is invoked without --document-private-items. Links from private items to private types are however never actually generated in that case and thus shouldn't produce a warning. These links are in fact a very useful tool to document crate internals. Tests are added for all 4 combinations of public/private items and link targets. Test 1 is the case mentioned above and fails without this commit. Tests 2 - 4 passed before already but are added nonetheless to prevent regressions.
Manishearth added a commit
to Manishearth/rust
that referenced
this pull request
Jul 13, 2020
…jyn514 rustdoc: Allow linking from private items to private types Fixesrust-lang#74134 After PR rust-lang#72771 this would trigger an intra_doc_link_resolution_failure warning when rustdoc is invoked without --document-private-items. Links from private items to private types are however never actually generated in that case and thus shouldn't produce a warning. These links are in fact a very useful tool to document crate internals. Tests are added for all 4 combinations of public/private items and link targets. Test 1 is the case mentioned above and fails without this commit. Tests 2 - 4 passed before already but are added nonetheless to prevent regressions.
Manishearth added a commit
to Manishearth/rust
that referenced
this pull request
Jul 13, 2020
…jyn514 rustdoc: Allow linking from private items to private types Fixesrust-lang#74134 After PR rust-lang#72771 this would trigger an intra_doc_link_resolution_failure warning when rustdoc is invoked without --document-private-items. Links from private items to private types are however never actually generated in that case and thus shouldn't produce a warning. These links are in fact a very useful tool to document crate internals. Tests are added for all 4 combinations of public/private items and link targets. Test 1 is the case mentioned above and fails without this commit. Tests 2 - 4 passed before already but are added nonetheless to prevent regressions.
Manishearth added a commit
to Manishearth/rust
that referenced
this pull request
Jul 13, 2020
…jyn514 rustdoc: Allow linking from private items to private types Fixesrust-lang#74134 After PR rust-lang#72771 this would trigger an intra_doc_link_resolution_failure warning when rustdoc is invoked without --document-private-items. Links from private items to private types are however never actually generated in that case and thus shouldn't produce a warning. These links are in fact a very useful tool to document crate internals. Tests are added for all 4 combinations of public/private items and link targets. Test 1 is the case mentioned above and fails without this commit. Tests 2 - 4 passed before already but are added nonetheless to prevent regressions.
Manishearth added a commit
to Manishearth/rust
that referenced
this pull request
Jul 14, 2020
…jyn514 rustdoc: Allow linking from private items to private types Fixesrust-lang#74134 After PR rust-lang#72771 this would trigger an intra_doc_link_resolution_failure warning when rustdoc is invoked without --document-private-items. Links from private items to private types are however never actually generated in that case and thus shouldn't produce a warning. These links are in fact a very useful tool to document crate internals. Tests are added for all 4 combinations of public/private items and link targets. Test 1 is the case mentioned above and fails without this commit. Tests 2 - 4 passed before already but are added nonetheless to prevent regressions.
Manishearth added a commit
to Manishearth/rust
that referenced
this pull request
Jul 14, 2020
…jyn514 rustdoc: Allow linking from private items to private types Fixesrust-lang#74134 After PR rust-lang#72771 this would trigger an intra_doc_link_resolution_failure warning when rustdoc is invoked without --document-private-items. Links from private items to private types are however never actually generated in that case and thus shouldn't produce a warning. These links are in fact a very useful tool to document crate internals. Tests are added for all 4 combinations of public/private items and link targets. Test 1 is the case mentioned above and fails without this commit. Tests 2 - 4 passed before already but are added nonetheless to prevent regressions.
Manishearth added a commit
to Manishearth/rust
that referenced
this pull request
Jul 14, 2020
…jyn514 rustdoc: Allow linking from private items to private types Fixesrust-lang#74134 After PR rust-lang#72771 this would trigger an intra_doc_link_resolution_failure warning when rustdoc is invoked without --document-private-items. Links from private items to private types are however never actually generated in that case and thus shouldn't produce a warning. These links are in fact a very useful tool to document crate internals. Tests are added for all 4 combinations of public/private items and link targets. Test 1 is the case mentioned above and fails without this commit. Tests 2 - 4 passed before already but are added nonetheless to prevent regressions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#72769
r? @GuillaumeGomez