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

rustdoc: refactor how passes are structured, and turn intra-doc-link collection into a pass #52800

Merged
merged 7 commits into from
Aug 5, 2018

Conversation

QuietMisdreavus
Copy link
Member

This builds on #52751 and should not be merged until that finally finishes the bors queue

Part 2 of my passes refactor. This introduces the concept of an "early pass", which is run right before exiting the compiler context. This is important for passes that may want to ask the compiler about things. For example, i took out the intra-doc-link collection and turned it into a early pass. I also made the strip-hidden, strip-private and strip-priv-imports passes occur as early passes, so that intra-doc-link collection wouldn't run on docs that weren't getting printed.

Fixes #51684, technically #51468 too but that version of h2 hits a legit intra-link error after that >_>

r? @rust-lang/rustdoc

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

This comment has been minimized.

@QuietMisdreavus
Copy link
Member Author

An analysis of the bug that caused cfg-if and bytes to break a bunch of crates once we turned resolution errors into a proper lint:

cfg-if v0.1.3 has the following at the top of its crate docs:

//! A macro for defining #[cfg] if-else statements.

bytes v0.4.8 has the following in its crate docs:

//! A `Bytes` handle can be created directly from an existing byte store (such as &[u8]
//! or Vec<u8>), but usually a `BytesMut` is used first and written to. For
//! example:

When rustdoc processes a crate, it currently processes intra-doc-links as soon as the item is "cleaned". This is fairly early in the process, and occurs before any decision of whether to show a given item's documentation has been made. (Sure, we have a sense of whether it's in a public module, but not whether it's been exported somewhere that is public.) Furthermore, whenever it finds a problem resolving a link, it reports an error immediately, again ignoring whether that item is relevant for the current doc run.

Generally, whenever we encounter an external item, we only import its definition and attributes once we know we're going to display it. Either it's being used in a pub use in a public module, or someone put a #[doc(inline)] on it. However, for extern crate statements, we process their attributes just like it were a struct or function. This is a problem only because rustc loads all the inner attributes of a crate and throws them in alongside the attributes tagged onto the extern crate statement. Including their docs.

Which brings us back to cfg-if and bytes. Whenever some project used either of those crates, those links would be treated as local (despite not being written locally) and the error would be reported. Combine this with crates that use #![deny(warnings)] and you have the error that brought down h2 and a few more crates.

My original reading of the issue is still valid, though, even if it's not the bug that created the issue. The other test i added also demonstrates this. A private item with a bad link target on it will currently report an error, despite not appearing in the documentation to create a broken link.

@GuillaumeGomez
Copy link
Member

Looks good to me! Please squash your tidy commit though. ;)

@QuietMisdreavus QuietMisdreavus force-pushed the do-not-pass-go branch 2 times, most recently from 260c641 to 85bf440 Compare July 29, 2018 16:28
@QuietMisdreavus
Copy link
Member Author

Now that #52751 is landed, i've rebased atop master so that commit doesn't show up in this one.

@GuillaumeGomez
Copy link
Member

Then let's get it in!

@bors: r+

@bors
Copy link
Contributor

bors commented Jul 29, 2018

📌 Commit 85bf440f98cd634563d1d9a1e24cd5b6027917fe has been approved by GuillaumeGomez

@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 29, 2018
@bors
Copy link
Contributor

bors commented Jul 29, 2018

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout do-not-pass-go (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self do-not-pass-go --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
Auto-merging src/librustdoc/lib.rs
CONFLICT (content): Merge conflict in src/librustdoc/lib.rs
Auto-merging src/librustdoc/core.rs
Automatic merge failed; fix conflicts and then commit the result.

@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 Jul 29, 2018
@kennytm
Copy link
Member

kennytm commented Jul 30, 2018

@bors r-

Wrongly rescheduled.

@QuietMisdreavus
Copy link
Member Author

Rebased.

@QuietMisdreavus QuietMisdreavus added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 1, 2018
@QuietMisdreavus
Copy link
Member Author

@bors p=1

It seems that any time someone touches the current intra-doc links functionality, this PR gets a merge conflict because of how it's moving the code around. I just had to rebase again.

@QuietMisdreavus
Copy link
Member Author

@bors r=GuillaumeGomez

@bors
Copy link
Contributor

bors commented Aug 3, 2018

📌 Commit e47dea72472b2f2f4b978b72b95b63edbb1d93e8 has been approved by GuillaumeGomez

@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 Aug 3, 2018
@bors
Copy link
Contributor

bors commented Aug 3, 2018

⌛ Testing commit e47dea72472b2f2f4b978b72b95b63edbb1d93e8 with merge 1580002f9fa813f468c31d391b00d12399b8507b...

@bors
Copy link
Contributor

bors commented Aug 3, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 3, 2018
@QuietMisdreavus
Copy link
Member Author

QuietMisdreavus commented Aug 3, 2018

------------------------------------------
stderr:
------------------------------------------
error: Unrecognized option: 'invalid-arg-foo'
error: kaboom
  --> compile-error.rs:12:5
   |
12 |     compile_error!("kaboom");
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to previous error
error: kaboom
  --> compile-error.rs:12:5
   |
12 |     compile_error!("kaboom");
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
thread 'main' panicked at 'encountered error with `-Z treat_err_as_bug', librustc_errors\lib.rs:478:13
note: Run with `RUST_BACKTRACE=1` for a backtrace.
error: internal compiler error: unexpected panic
note: the compiler unexpectedly panicked. this is a bug.
note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
note: rustc 1.30.0-dev running on x86_64-pc-windows-msvc
note: compiler flags: -Z treat-err-as-bug
error: Unrecognized option: 'invalid-arg-foo'
error: kaboom
  --> compile-error.rs:12:5
   |
12 |     compile_error!("kaboom");
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
make: *** [Makefile:11: all] Error 1
------------------------------------------
thread '[run-make] run-make-fulldeps\exit-code' panicked at 'explicit panic', tools\compiletest\src\runtest.rs:3149:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.
failures:
    [run-make] run-make-fulldeps\exit-code
test result: FAILED. 187 passed; 1 failed; 1 ignored; 0 measured; 0 filtered out

...did i mess with rustdoc's exit code somehow? was that modified in the last week and i somehow missed it in my rebases?

EDIT: Wait, that's rustc's exit code. It's the only one being run with -Z treat-err-as-bug, and it's claiming to be rustc anyway. This PR doesn't touch anything outside librustdoc; what's going on? I'm reading this wrong, it's the stderr of every run in that makefile.

@QuietMisdreavus
Copy link
Member Author

Ohhhh, i see it now. The run-make-fulldeps/exit-code test tries to document a crate with this, so that it will exit with an error:

#![deny(intra_doc_link_resolution_failure)]

/// [intradoc::failure]
fn main() {
    println!("Hello, world!");
}

However, now that we're not running docs for private items, the lint that was being emitted there is no longer being emitted. I'll update the test.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

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 @TimNN. (Feature Requests)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:43:49] ....................................................................................................
[00:43:51] ....................................................................................................
[00:43:54] ....................................................................................................
[00:43:57] ....................................................................................................
[00:43:59] iiiiiiiii...........................................................................................
[00:44:05] ....................................................................................................
[00:44:09] .....i..............................................................................................
[00:44:11] .........i..........................................................................................
[00:44:14] ....................................................................................................

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 @TimNN. (Feature Requests)

@QuietMisdreavus
Copy link
Member Author

QuietMisdreavus commented Aug 5, 2018

Broadcast message from root@travis-job-d3ffdbb6-fb99-47ea-a99b-4def481baca7

	(unknown) at 4:28 ...




The system is going down for power off NOW!

[01:06:20] 
[01:06:20] Session terminated, terminating shell...make: *** wait: No child processes.  Stop.
[01:06:20] make: *** Waiting for unfinished jobs....
[01:06:20] make: *** wait: No child processes.  Stop.
[01:06:20]  ...terminated.

I'm restarting the travis build.

EDIT: I've been told travis people are looking at it, so i'll link their issue here so it's saved. travis-ci/travis-ci#4924

@QuietMisdreavus
Copy link
Member Author

Travis is green, so let's get this rolling again...

@bors r=GuillaumeGomez

@bors
Copy link
Contributor

bors commented Aug 5, 2018

📌 Commit e332985 has been approved by GuillaumeGomez

@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 Aug 5, 2018
@bors
Copy link
Contributor

bors commented Aug 5, 2018

⌛ Testing commit e332985 with merge 73c7873...

bors added a commit that referenced this pull request Aug 5, 2018
rustdoc: refactor how passes are structured, and turn intra-doc-link collection into a pass

This builds on #52751 and should not be merged until that finally finishes the bors queue

Part 2 of my passes refactor. This introduces the concept of an "early pass", which is run right before exiting the compiler context. This is important for passes that may want to ask the compiler about things. For example, i took out the intra-doc-link collection and turned it into a early pass. I also made the `strip-hidden`, `strip-private` and `strip-priv-imports` passes occur as early passes, so that intra-doc-link collection wouldn't run on docs that weren't getting printed.

Fixes #51684, technically #51468 too but that version of `h2` hits a legit intra-link error after that `>_>`

r? @rust-lang/rustdoc
@bors
Copy link
Contributor

bors commented Aug 5, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: GuillaumeGomez
Pushing 73c7873 to master...

@bors bors merged commit e332985 into rust-lang:master Aug 5, 2018
@QuietMisdreavus QuietMisdreavus deleted the do-not-pass-go branch February 12, 2020 00:40
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants