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

Ignore rustc_private items from std docs #76571

Merged
merged 2 commits into from
Sep 14, 2020

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Sep 10, 2020

By ignoring rustc_private items for non local impl block,
this may fix #74672 and fix #75588 .

This might suppress #76529 if it is simple enough for backport.

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(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 Sep 10, 2020
@jyn514 jyn514 added A-stability Area: issues related to #[stable] and #[unstable] attributes themselves. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Sep 10, 2020
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.

Thanks so much for working on this! Just a few nits.

compiler/rustc_span/src/def_id.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/inline.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/inline.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/inline.rs Show resolved Hide resolved
src/tools/linkchecker/main.rs Outdated Show resolved Hide resolved
@tesuji tesuji changed the title Ignore rustc_private item from std docs Ignore rustc_private items from std docs Sep 10, 2020
@tesuji tesuji force-pushed the rustdoc-private-traits branch 2 times, most recently from 7679606 to 1444a89 Compare September 10, 2020 14:05
src/test/rustdoc/issue-75588.rs Outdated Show resolved Hide resolved
//
// NOTE: This cannot be added to `LINKCHECK_EXCEPTIONS` because the resolved path
// calculated in `check` function is outside `build/<triple>/doc` dir.
// So the `strip_prefix` method just returns the old absolute broken path.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this comment seems fine. nit: this could be else if so you don't need the early return, but won't block on that

@Mark-Simulacrum
Copy link
Member

r? @jyn514

@tesuji
Copy link
Contributor Author

tesuji commented Sep 11, 2020

I removed all the added doc to make this patch smaller to be able to backport.

@tesuji tesuji marked this pull request as ready for review September 11, 2020 03:30
@jyn514
Copy link
Member

jyn514 commented Sep 11, 2020

I removed all the added doc to make this patch smaller to be able to backport.

Oh, I don't think there's need for that, the comments won't hurt anything. It's logic changes I'm worried about backporting.

@bors r+

on getting this into nightly. Now about getting this into beta: @rust-lang/rustdoc I see three options:

  1. Don't backport anything. Beta/stable will be missing Join and other unstable traits (Missing Implementors for Join and Pattern trait in rustdoc #75588), and have some rustc_private traits (impl Deref for EndianSlice appears in std docs #74672). This is IMO the worst outcome.
  2. Backport Particially revert #73771 #76529 which is a simple revert. Beta/stable will have Join, but also have other documentation about rustc_private items like gimli traits. This is the most conservative option: it exposes rustc internals, but it makes sure there's no missing docs.
  3. Backport this PR. Beta/stable will have Join and will not have rustc_private items, but there's the possibility this introduces a bug by accident.

Personally I'm in favor of 3, but I'm willing to be convinced by 2. What do you think?

@bors
Copy link
Contributor

bors commented Sep 11, 2020

📌 Commit ccd76110bfcd0339d70f554f5901443dd551fdbf has been approved by jyn514

@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 Sep 11, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 11, 2020

cc also @alexcrichton who wrote #73771

@jyn514
Copy link
Member

jyn514 commented Sep 11, 2020

@bors r-

CI is failing, but LGTM when fixed, maybe add back the comments while you're at it

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 11, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Sep 11, 2020

Fixed the tidy error,

maybe add back the comments while you're at it

I will add them back in other cleanup PRs.

@rustbot modify labels: -S-waiting-on-author +S-waiting-on-review

@tesuji
Copy link
Contributor Author

tesuji commented Sep 13, 2020

@lzutao please don't force push, github now shows a 4k line diff before and after.

Yeah, I'm sorry. But GitHub could improve their compare page.
At local you could run git range-diff c3d048a...c743fc4, it is a lot cleaner.
My local result of that command:

@@ src/tools/linkchecker/main.rs: fn is_exception(file: &Path, link: &str) -> bool
 +        // calculated in `check` function is outside `build/<triple>/doc` dir.
 +        // So the `strip_prefix` method just returns the old absolute broken path.
 +        if file.ends_with("std/primitive.slice.html") {
-+            if link.ends_with("std/primitive.slice.html") {
++            if link.ends_with("primitive.slice.html") {
 +                return true;
 +            }
 +        }

What change did you make to fix the tidy error?

I simply removed the "std/" prefix, cause windows path (in this case has type lossy &str) usually has \ as path separators.

@jyn514
Copy link
Member

jyn514 commented Sep 13, 2020

GitHub could improve their compare page. At local you could run git range-diff c3d048a...c743fc4, it is a lot cleaner.

Yeah, I've suggested this before (isaacs/github#1834) but it was ignored.

Ok, the change looks fine.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 13, 2020

📌 Commit c743fc4 has been approved by jyn514

@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 Sep 13, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Sep 13, 2020

Yeah, I've suggested this before (isaacs/github#1834) but it was ignored.

Yeah, that's sad. But I think running range-diff still requires more calculation power than just normal diff.
Consider that github processes thousands of diff requests per second (I guess), it could be a burden for their servers.
Still they could have reply that instead of ignoring the issue.

@bors
Copy link
Contributor

bors commented Sep 14, 2020

⌛ Testing commit c743fc4 with merge 356d8ad...

@bors
Copy link
Contributor

bors commented Sep 14, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: jyn514
Pushing 356d8ad to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 14, 2020
@bors bors merged commit 356d8ad into rust-lang:master Sep 14, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 14, 2020
@tesuji tesuji deleted the rustdoc-private-traits branch September 14, 2020 10:23
@tesuji
Copy link
Contributor Author

tesuji commented Sep 14, 2020

Finally! 🎉

@jyn514
Copy link
Member

jyn514 commented Sep 14, 2020

@lzutao to backport this to beta, make a PR with the same commits, but against the beta branch instead of master: https://forge.rust-lang.org/release/beta-backporting.html
Then the rustdoc team will decided whether or not it should be backported.

@jyn514 jyn514 added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 14, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 14, 2020

@rust-lang/rustdoc this is waiting on a decision about backporting: #76571 (comment)

@tesuji

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

Is there any reason why we shouldn't? If not I guess it's fine to approve the backport.

@spastorino
Copy link
Member

discussed in T-compiler meeting.

@rustbot modify labels: beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 17, 2020
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 17, 2020
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.48.0, 1.47.0 Sep 17, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2020
…ulacrum

[beta] backports

* Ignore rustc_private items from std docs rust-lang#76571
* Fix HashMap visualizers in Visual Studio (Code) rust-lang#76389
* Account for version number in NtIdent hack rust-lang#76331
* Account for async functions when suggesting new named lifetime rust-lang#75867
* Fix loading pretty-printers in rust-lldb script rust-lang#76015

This also bumps to the released stable compiler.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 6, 2020
Add some docs to rustdoc::clean::inline and def_id functions

Split from rust-lang#76571 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stability Area: issues related to #[stable] and #[unstable] attributes themselves. beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.

Missing Implementors for Join and Pattern trait in rustdoc impl Deref for EndianSlice appears in std docs
9 participants