async-std docs no longer build on recent nightlies

[yoshuawuyts]

Original issue surfaced in: async-rs/async-std#845 by @dignifiedquire. It appears that sometime after 2020-05-01 nightlies are no longer able to build async-std docs. #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 #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.

#73566 (comment)

[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? #73566 (comment) 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 ❤️

[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

rust/src/librustc_privacy/lib.rs

Lines 791 to 797 in 780d6cb

	hir::ItemKind::OpaqueTy(..) => {
	// 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();
	}

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 #73566 (comment).

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 #75100 (comment)

[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: #73566 (comment), 02a24c8#diff-0de644786a2f1fd88c7d7f44bc3fa4bbR451. Maybe we can fix those queries instead?

[Dylan-DPC-zz]

(nominated so prioritisation discussion not required)

[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!

[jyn514]

I'm going to close this, no one has a suggested any alternative approach and people are continuing to add if ! actually_rustdoc to the codebase.