Rust: Fix rustdoc hack allowing async-std docs to build

[yoshuawuyts]

Original issue surfaced in: https://github.com/async-rs/async-std/pull/845 by @dignifiedquire. It appears that sometime after 2020-05-01 nightlies are no longer able to build async-std docs. https://github.com/rust-lang/rust/pull/73566 appears to be a likely cause for this regression.

We use a fairly elaborate macro to generate docs, so we're not particularly surprised that a big refactor may have caused issues there. But it'd still be nice if we could get it to work again.

Error

The error message is fairly long and probably not worth posting in full. Luckily it does not appear to be an ICE, just a regression. This is the build that fails: link.

[jyn514]

The errors are the same ones as for https://github.com/rust-lang/rust/pull/43348. They show up for async-std because all their functions are async, so rustdoc tries to infer the return type by type-checking the body. This didn't happen before #73566 because rustdoc didn't consider async fn to return an impl Trait.

https://github.com/rust-lang/rust/pull/73566#issuecomment-667978932

[jyn514]

Copy/pasting from the PR:

There's not a good way to fix this. We could revert, but then we lose the fixes to intra-doc links and they'd be permanently unstable again.

Maybe we could have a reachability pass for rustdoc that's separate from the one for rustc? https://github.com/rust-lang/rust/pull/73566#issuecomment-653689128 That's going to require a lot of work though, it's not a quick fix. And I don't know if it would actually do what I want.

[lcnr]

So a minimal repro for this would be something like the following?

#[cfg(any(windows, doc))]
fn this() -> u32 {
    3
}

#[cfg(any(unix, doc))]
fn this() -> u32 {
    4
}

pub async fn wow() -> u32 {
    this()
}

[jyn514]

No, that's not right - that errors on the function signatures, not the function bodies:

error[E0428]: the name `this` is defined multiple times
 --> type_error.rs:7:1
  |
2 | fn this() -> u32 {
  | ---------------- previous definition of the value `this` here
...
7 | fn this() -> u32 {
  | ^^^^^^^^^^^^^^^^ `this` redefined here
  |
  = note: `this` must be defined only once in the value namespace of this module

This error is because rustdoc is type checking the function bodies.

[jyn514]

A minimal repro is

pub async fn f() {
    content::should::not::matter();
}
error[E0433]: failed to resolve: could not resolve path `content::should::not::matter`
 --> body_error.rs:2:5
  |
2 |     content::should::not::matter();
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ could not resolve path `content::should::not::matter`
  |
  = note: this error was originally ignored because you are running `rustdoc`
  = note: try running again with `rustc` or `cargo check` and you may get a more detailed error

[lcnr]

Ah, and a more realistic example would probably be:

mod windows {
    pub trait WinFoo {
        fn foo(&self) {}
    }

    impl WinFoo for () {}
}

#[cfg(any(windows, doc))]
use windows::*;

mod unix {
    pub trait UnixFoo {
        fn foo(&self) {}
    }

    impl UnixFoo for () {}
}

#[cfg(any(unix, doc))]
use unix::*;

async fn bar() {
    ().foo()
}

Thanks :heart:

[jyn514]

I'm more in favor of writing a specific pass rather than reverting this, too much benefits came from it and we're getting so close to have the intra-doc links stable! (I'm really excited about this feature!)

@GuillaumeGomez I'm not sure why https://github.com/rust-lang/rust/blob/780d6cb1408717f968a21d627cc34c8ffa7f6fe8/src/librustc_privacy/lib.rs#L791-L797 is there currently so I'm afraid to change it. Maybe this is a hack for some specific call of privacy_access_levels and we can fix it for privacy_access_levels generally and only pessimize that call?

[GuillaumeGomez]

I think it might be interesting to ask the compiler team before doing anything in here. They might have a solution we didn't think about. :)

[jyn514]

Ping @rust-lang/compiler - does anyone know why OpaqueTy is special-cased in rustc_privacy? It causes rustdoc to try and type-check function bodies for fn f() -> impl Trait. Backtrace at https://github.com/rust-lang/rust/pull/73566#issuecomment-653689128.

The following patch does _not_ fix it, but that might just be because I'm not understanding EmbargoVisitor:

diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs
index fc00050f405..5855b48c01d 100644
--- a/src/librustc_privacy/lib.rs
+++ b/src/librustc_privacy/lib.rs
@@ -782,8 +782,8 @@ impl Visitor<'tcx> for EmbargoVisitor<'tcx> {
                 // FIXME: This is some serious pessimization intended to workaround deficiencies
                 // in the reachability pass (`middle/reachable.rs`). Types are marked as link-time
                 // reachable if they are returned via `impl Trait`, even from private functions.
-                let exist_level = cmp::max(item_level, Some(AccessLevel::ReachableFromImplTrait));
-                self.reach(item.hir_id, exist_level).generics().predicates().ty();
+                //let exist_level = cmp::max(item_level, Some(AccessLevel::ReachableFromImplTrait));
+                self.reach(item.hir_id, item_level).generics().predicates().ty();
             }
             // Visit everything.
             hir::ItemKind::Const(..)

edit since this is the comment with the ping: there are even _more_ issues than this, see https://github.com/rust-lang/rust/issues/75100#issuecomment-668316246

[Manishearth]

Worth remembering: rustdoc does not care one bit about what the actual type of an impl Trait is, aside from the trait itself, which should already be known since that's explicitly specified (via trait or via the async keyword). So it should theoretically be possible to make this work without solving item bodies.

[Nemo157]

(Though it _might_ be nice if rustdoc indicated leaked auto-traits, which would require knowing the concrete type).

[jyn514]

@Nemo157 knowing the type and allowing type errors are mutually incompatible. So even theoretically it would only be possible if there were no type errors, and it would be a _very_ delicate balance trying to only run typeck if we knew it would succeed.

[Manishearth]

Oh, another thing I should note: everybody_loops had a bunch of problems with impl Trait back in the day, I kinda expected there to be something similar with rustdoc's new item-bodies skipping approach but when we were working on it I was unable to construct an example (looks like async lets you produce one). But it's worth perhaps looking at whatever everybody_loops did to fix this issue.

[jyn514]

everybody_loops skipped the pass entirely, which would still give type errors like we do now. The only thing that changed is that async fn is now considered an impl Trait where it wasn't before.

[Manishearth]

@yoshuawuyts one thing I don't understand: Is this a new error on your CI, or was your CI broken before but ignored? I'm surprised this was only caught now given that the nightly that broke it was months ago, and y'all seem to be running the doc CI run on nightly.

[estebank]

Can we confirm that this hasn't reached beta as well?

[yoshuawuyts]

@Manishearth We've been having some unrelated issues with ARM builds on our CI. That took a while to investigate and eventually disable. My guess is that that's been masking this issue.

That said I haven't done much work on async-std recently, so I may not be completely up to date.

[Manishearth]

Ah, makes sense, thanks.

@estebank It should have hit beta, but the stack of patches that has landed after this depending on it makes it hard to revert, and we should probably just fix it instead.

[jyn514]

@estebank It should have hit beta

No, it hasn't. I made sure I didn't r+ on everybody_loops until the beta release.

$ rustdoc +beta --version
rustdoc 1.46.0-beta.2 (6f959902b 2020-07-23)
$ rustdoc +beta async_error.rs  --edition 2018
# no output
$ rustdoc +nightly async_error.rs  --edition 2018
error[E0433]: failed to resolve: could not resolve path `content::should::not::matter`
 --> async_error.rs:2:5
  |
2 |     content::should::not::matter();
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ could not resolve path `content::should::not::matter`
  |
  = note: this error was originally ignored because you are running `rustdoc`
  = note: try running again with `rustc` or `cargo check` and you may get a more detailed error

[Manishearth]

Oh, right!

[jyn514]

Ok, the cause of this is _different_ from in the original PR. My suspicion is that the original OpaqueTy thing is still an issue that's being covered up by this though.

  69: rustc_session::utils::<impl rustc_session::session::Session>::time
  70: rustc_typeck::check_crate
  71: rustc_middle::ty::context::tls::enter_global
  72: rustc_interface::interface::create_compiler_and_run
  73: rustdoc::core::run_core
...

note: compiler flags: -Z treat-err-as-bug

query stack during panic:
#0 [typeck] type-checking `f`
#1 [mir_built] building MIR for `f`
#2 [unsafety_check_result] unsafety-checking `f`
#3 [mir_const] processing MIR for `f`
#4 [mir_validated] processing `f`
#5 [mir_borrowck] borrow-checking `f`
#6 [type_of] computing type of `f::{{opaque}}#0`
#7 [check_mod_item_types] checking item types in top-level module
end of query stack

I added typeck::check_crate because otherwise some queries would panic if there was an invalid type: https://github.com/rust-lang/rust/pull/73566#issuecomment-656954425, https://github.com/rust-lang/rust/pull/73566/commits/02a24c8e2fd370041a24b7d93e8c3710b7b76015#diff-0de644786a2f1fd88c7d7f44bc3fa4bbR451. Maybe we can fix those queries instead?

[Dylan-DPC]

(nominated so prioritisation discussion not required)

[jyn514]

I special cased the behavior for rustc_privacy for rustdoc in https://github.com/rust-lang/rust/pull/75127 and that seems to be working. So the only issue left is to find a way to avoid running rustc_typeck::check_crate without an ICE: https://github.com/rust-lang/rust/issues/75100#issuecomment-668316246

[jyn514]

I think the relevant code is https://github.com/rust-lang/rust/blob/f07100afc8650101ac924d728521e1a5d0ce9080/src/librustc_typeck/lib.rs#L324-L333 cc @matthewjasper - how hard would that FIXME be to fix?

[jyn514]

It appears it goes all the way back to 2013 🤦
https://github.com/rust-lang/rust/commit/2ea52a38e59b85b4b6998661b38425ce29839aed#diff-206a7ac5579f8f309d586d74431430c9R419
cc @nikomatsakis I guess.

[jyn514]

For context, this is the query that ICEs if rustdoc doesn't run typeck::check_crate:
https://github.com/rust-lang/rust/pull/73566#issuecomment-656954425

[nikomatsakis]

I don't know how hard that FIXME would be to fix -- we've been making steady progress here, I would guess that it's doable. I know @eddyb has been on a bit of a mission lately to remove those fatal panics and things.

[jyn514]

Another option since beta is coming up in ~3 weeks is to just eat the ICE and say that error -> ICE is better than working code to error :/ and then go back and fix the root cause later. The only ICE is from invalid code:

https://github.com/rust-lang/rust/blob/0f5be70ea4c3ec4544cdeb80e3138832e15815f9/src/test/rustdoc-ui/infinite-recursive-type.rs#L1-L4

[spastorino]

Assigning P-critical as discussed as part of the Prioritization Working Group procedure and removing nomination as it was discussed briefly during T-compiler meeting.

[pnkfelix]

Another option since beta is coming up in ~3 weeks is to just eat the ICE and say that error -> ICE is better than working code to error :/ and then go back and fix the root cause later. The only ICE is from invalid code:

https://github.com/rust-lang/rust/blob/0f5be70ea4c3ec4544cdeb80e3138832e15815f9/src/test/rustdoc-ui/infinite-recursive-type.rs#L1-L4

Also, keep in mind that yet another option, which I think is similar (but maybe less risky), is to revert PR# #73566, with the intention of relanding a more robust version of the change once the bugs are ironed out. (The presentation given in an earlier comment's discussion of a revert made it sound to me like it was a decision we'd have to live with forever, which is not the case as far as I can tell.)

• (Of course, a revert of PR #73566 would mean suffering with the bugs that PR fixed for a bit longer on stable Rust.)

[jyn514]

We _could_ revert that PR, but then we'd have to revert every follow-up PR and re-open every issue that depends on it.

• https://github.com/rust-lang/rust/issues/75088
• https://github.com/rust-lang/rust/pull/73101
• https://github.com/rust-lang/rust/issues/71820
• https://github.com/rust-lang/rust/issues/71104
• https://github.com/rust-lang/rust/issues/65983
• https://github.com/rust-lang/rust/issues/73699
• https://github.com/rust-lang/rust/pull/73365

Probably others too - I know there's a lot of intra-doc link PRs that landed after https://github.com/rust-lang/rust/pull/73101.

[jyn514]

I have an _extremely_ hacky fix for this in https://github.com/rust-lang/rust/pull/75127. Hopefully it will be enough to unbreak async-std before the beta release and then the compiler team will have more time to work on a more principled fix.

Anyone on compiler team want to review?

[pnkfelix]

@jyn514 I just reviewed it. I assume the rustdoc team is onboard with the other changes it makes. (Certainly ignoring the resolution errors in those cases seems to be in line with what rustdoc does elsewhere at this point.)

[GuillaumeGomez]

Yup. :)

[spastorino]

@jyn514 now that your PR is merged, is this issue fixed on master? should we beta nominate the PR?. I guess we still want it open to find a proper solution but in that case, shouldn't we lower the priority of this issue?.

[jyn514]

The issue should be fixed but I haven't heard back from @yoshuawuyts to be sure.

There's no need to beta nominate because the bug wasn't present in beta, only on nightly.

Maybe we should open a separate issue for the long-term fix? It shouldn't affect behavior too much, the fix is just really hacky.

[spastorino]

Ok, we can use this issue for the remaining work but let's adjust the priority to P-medium.

[yoshuawuyts]

@jyn514 just built async-std locally with the latest nightly, and it seems to work! -- thanks so much!