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 cannot infer type when using type_alias_impl_trait, while rustc works #65863

Closed
bjorn3 opened this issue Oct 27, 2019 · 17 comments · Fixed by #73566
Closed

Rustdoc cannot infer type when using type_alias_impl_trait, while rustc works #65863

bjorn3 opened this issue Oct 27, 2019 · 17 comments · Fixed by #73566
Labels
C-bug Category: This is a bug. F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` requires-nightly This issue requires a nightly compiler in some way. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@bjorn3
Copy link
Member

bjorn3 commented Oct 27, 2019

#![feature(type_alias_impl_trait)]

pub trait Backend {}

impl Backend for () {}

pub struct Module<T>(T);

pub type BackendImpl = impl Backend;

pub fn make_module() -> Module<BackendImpl> {
    Module(())
}
$ rustc -V
rustc 1.38.0 (625451e37 2019-09-23)
$ cargo doc
error[E0282]: type annotations needed
  --> src/main.rs:13:1
   |
13 | pub type BackendImpl = impl Backend<Product: std::any::Any>;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type

error: aborting due to previous error
$ cargo build
@bjorn3
Copy link
Member Author

bjorn3 commented Oct 27, 2019

@rustbot modify labels: +requires-nightly +F-type_alias_impl_trait

@rustbot rustbot added F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` requires-nightly This issue requires a nightly compiler in some way. labels Oct 27, 2019
@GuillaumeGomez
Copy link
Member

That is... interesting.

@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Oct 27, 2019
@bjorn3
Copy link
Member Author

bjorn3 commented Oct 27, 2019

Forgot to add T-rustdoc :)

@jonas-schievink jonas-schievink added the C-bug Category: This is a bug. label Oct 27, 2019
@Aaron1011
Copy link
Member

This looks like an issue in should_ignore_fn, which is used by Rustdoc to determine which functions should have their bodies replaced by loops.

This check is done on the AST. I think what's happening is that we detect BackendImpl (since it appears as a generic argument to Module), but are unable to determine that BackendImpl is an opaque type (since we haven't resolved paths yet).

I'm not familiar enough with the various phases of the compiler to know if it would be possible to do this check later, once we have ty::Tys available for the function signature (e.g. resolved types).

We may have to largely give up on this check, and actually compile the bodies of most functions. That could mean a fairly large regression in doc build times - but if we can't come up with a way to detect which functions might constrain opaque types, I don't think we have another option.

@Aaron1011
Copy link
Member

Actually, we might be able to check for the presence any impl Trait definitions within a parent scope of the function. This would have a large number of false positives (any impl Traits at the crate root would cause every function to be flagged as 'may define an opaque type'), but it would be better than nothing.

@clarfonthey
Copy link
Contributor

Also ran into this bug, came up with another example:

#![feature(type_alias_impl_trait)]
use core::iter::empty;
type Test = impl Iterator<Item = ()>;
fn test() -> Test {
    empty()
}

Rustdoc error:

error[E0277]: `()` is not an iterator
 --> src/lib.rs:3:1
  |
3 | type Test = impl Iterator<Item = ()>;
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `()` is not an iterator
  |
  = help: the trait `std::iter::Iterator` is not implemented for `()`
  = note: the return type of a function must have a statically known size

@GuillaumeGomez
Copy link
Member

@clarfon Nice one, thanks!

@ollie27 Didn't you fixed something close to this recently? (I might be wrong though...)

@ollie27
Copy link
Member

ollie27 commented Jan 19, 2020

@ollie27 Didn't you fixed something close to this recently? (I might be wrong though...)

I don't think so, no.

@jonhoo
Copy link
Contributor

jonhoo commented Feb 17, 2020

Here's another instance of this bug:

#![feature(type_alias_impl_trait)]
use std::future::Future;

trait Foo {
    type X: Future<Output = ()>;
    fn x() -> Self::X;
}

impl Foo for () {
    type X = impl Future<Output = ()>;
    fn x() -> Self::X {
        async {}
    }
}

@jonhoo
Copy link
Contributor

jonhoo commented Feb 17, 2020

As an interesting aside, when trying to work around this using Box<dyn Trait> under #[cfg(doc)], I kept running into this unimplemented! in librustdoc.

@jonhoo
Copy link
Contributor

jonhoo commented Feb 17, 2020

@Aaron1011 did you start the work of adding a check for the presence of impl Trait definitions within the parent scope, or were you just musing? As crates start adopting impl Trait existentials, this is probably going to become a pretty important use-case to support.

@Aaron1011
Copy link
Member

@jonhoo: I haven't done any work on this issue as of yet.

@jonhoo
Copy link
Contributor

jonhoo commented Mar 17, 2020

A brief update — I asked the nice people over at docs.rs to run some queries over their failed build logs, and here are crates where this issue shows up:

cratesfyi=> select name, version, releases.id from builds inner join releases on releases.id = builds.rid inner join crates on crates.id = releases.crate_id where output like '%error[E0282]: type annotations needed%';
        name         | version |   id   
---------------------+---------+--------
 small-logger        | 0.2.1   |  17558
 diesel_infer_schema | 0.13.0  |  49451
 diesel_infer_schema | 0.13.0  |  49451
 diesel_infer_schema | 0.12.0  |  44145
 searchspot          | 0.15.2  |  62618
 diesel_infer_schema | 0.14.0  |  54552
 diesel_infer_schema | 0.16.0  |  59094
 searchspot          | 0.14.0  |  59745
 diesel_infer_schema | 0.15.0  |  56265
 searchspot          | 0.15.0  |  62500
 searchspot          | 0.15.1  |  62603
 ipfsapi             | 0.1.6   |  68582
 searchspot          | 0.15.4  |  63062
 ipfsapi             | 0.1.3   |  64146
 ipfsapi             | 0.1.5   |  64240
 ipfsapi             | 0.1.0   |  63153
 ipfsapi             | 0.1.1   |  63479
 ipfsapi             | 0.1.2   |  64075
 ipfsapi             | 0.1.4   |  64157
 pom                 | 2.0.0   |  64739
 ipfsapi             | 0.1.7   |  68726
 fantoccini          | 0.8.1   |  77851
 fantoccini          | 0.8.0   |  71985
 searchspot          | 0.16.0  |  72539
 semverver           | 0.1.0   |  74641
 semverver           | 0.1.1   |  75472
 swc_ecma_parser     | 0.8.0   | 123132
 infer_schema_macros | 1.4.0   | 124335
 infer_schema_macros | 1.4.0   | 124335
 amadeus-parquet     | 0.1.3   | 176821
 amadeus-parquet     | 0.1.4   | 180078
 hcs-rs              | 0.2.1   | 188689
 hcs-rs              | 0.2.0   | 188669
(33 rows)
cratesfyi=> select name, version, releases.id from builds inner join releases on releases.id = builds.rid inner join crates on crates.id = releases.crate_id where output like '%is not implemented for `()`%';
         name          |    version    |   id   
-----------------------+---------------+--------
 reducer               | 1.1.0         | 125897
 amadeus-parquet       | 0.1.1         | 169206
 amadeus-postgres      | 0.1.3         | 176820
 amadeus-parquet       | 0.1.2         | 169266
 yukikaze              | 1.0.0-alpha.5 | 169612
 amadeus-parquet       | 0.1.3         | 176821
 amadeus-commoncrawl   | 0.1.3         | 176822
 amadeus-serde         | 0.1.3         | 176824
 amadeus-aws           | 0.1.3         | 176826
 vicuna                | 0.1.2         | 189614
 amadeus-postgres      | 0.1.4         | 180077
 amadeus-parquet       | 0.1.4         | 180078
 amadeus-commoncrawl   | 0.1.4         | 180079
 amadeus-serde         | 0.1.4         | 180080
 amadeus-aws           | 0.1.4         | 180081
 vicuna                | 0.1.1         | 189190
 amadeus-aws           | 0.1.5         | 189240
 amadeus-parquet       | 0.1.5         | 189242
 nu                    | 0.6.1         | 186499
 vicuna                | 0.1.0         | 189185
 amadeus-serde         | 0.1.5         | 189233
 amadeus-postgres      | 0.1.5         | 189235
 amadeus-commoncrawl   | 0.1.5         | 189237
 vicuna                | 0.1.3         | 189615
 xtra                  | 0.2.1         | 199674
 wasm-reader           | 0.1.0         | 198978
 amadeus-parquet       | 0.1.6         | 193727
 wasm-reader           | 0.1.1         | 200658
 cpp_to_rust_generator | 0.0.0         |  38434
 noria                 | 0.4.0         | 206426
 noria-server          | 0.4.0         | 206463
 noria                 | 0.4.1         | 208596
 wasm-reader           | 0.2.0         | 207153
(33 rows)

So looks like this isn't too widespread yet, but it's clearly something that "real" crates are running into :)

@jonhoo
Copy link
Contributor

jonhoo commented Jun 7, 2020

For those watching, when #72080 (which fixes #73061) lands, the workaround for this is basically:

#![feature(type_alias_impl_trait)]
use core::iter::empty;

#[cfg(not(doc))]
type Test = impl Iterator<Item = ()>;

#[cfg(doc)]
type Test = MockIterator<()>;
#[cfg(doc)]
struct MockIterator<T>(std::marker::PhantomType<T>);
#[cfg(doc)]
impl<T> Iterator for MockIterator<T> {
  type Item = T;
  fn next(&mut self) -> Self::Item { unreachable!() }
}

fn test() -> Test {
    empty()
}

It ain't pretty, but it'll make things work. You can even potentially re-use the mocker for multiple traits that you're using behind impl. See mit-pdos/noria@062b880 for a larger example of this change. H/T to @alecmocatta for suggesting this workaround in constellation-rs/amadeus#54.

@jyn514
Copy link
Member

jyn514 commented Jul 9, 2020

I expect this to be fixed by #73566, I'll add a test case there.

jyn514 added a commit to jyn514/rust that referenced this issue Jul 15, 2020
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 16, 2020
Don't run `everybody_loops` for rustdoc; instead ignore resolution errors

r? @eddyb
cc @petrochenkov, @GuillaumeGomez, @Manishearth, @ecstatic-morse, @marmeladema

~~Blocked on rust-lang#73743 Merged.
~~Blocked on crater run.~~ Crater popped up some ICEs ([now fixed](rust-lang#73566 (comment))). See [crater run](https://crater-reports.s3.amazonaws.com/pr-73566/index.html), [ICEs](rust-lang#73566 (comment)).
~~Blocked on rust-lang#74070 so that we don't make typeck_tables_of public when it shouldn't be.~~ Merged.

Closes rust-lang#71820, closes rust-lang#71104, closes rust-lang#65863.

## What is the motivation for this change?

As seen from a lengthy trail of PRs and issues (rust-lang#73532, rust-lang#73103, rust-lang#71820, rust-lang#71104), `everybody_loops` is causing bugs in rustdoc. The main issue is that it does not preserve the validity of the `DefId` tree, meaning that operations on DefIds may unexpectedly fail when called later. This is blocking intra-doc links (see rust-lang#73101).

This PR starts by removing `everybody_loops`, fixing rust-lang#71104 and rust-lang#71820. However, that brings back the bugs seen originally in rust-lang#43348: Since libstd documents items for all platforms, the function bodies sometimes do not type check. Here are the errors from documenting `libstd` with `everybody_loops` disabled and no other changes:

```rust
error[E0433]: failed to resolve: could not find `handle` in `sys`
  --> src/libstd/sys/windows/ext/process.rs:13:27
   |
13 |         let handle = sys::handle::Handle::new(handle as *mut _);
   |                           ^^^^^^ could not find `handle` in `sys`

error[E0425]: cannot find function `symlink_inner` in module `sys::fs`
   --> src/libstd/sys/windows/ext/fs.rs:544:14
    |
544 |     sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), false)
    |              ^^^^^^^^^^^^^ not found in `sys::fs`

error[E0425]: cannot find function `symlink_inner` in module `sys::fs`
   --> src/libstd/sys/windows/ext/fs.rs:564:14
    |
564 |     sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), true)
    |              ^^^^^^^^^^^^^ not found in `sys::fs`
```

## Why does this need changes to `rustc_resolve`?

Normally, this could be avoided by simply not calling the `typeck_item_bodies` pass. However, the errors above happen before type checking, in name resolution itself. Since name resolution is intermingled with macro expansion, and rustdoc needs expansion to happen before it knows all items to be documented, there needs to be someway to ignore _resolution_ errors in function bodies.

An alternative solution suggested by @petrochenkov was to not run `everybody_loops` on anything containing a nested `DefId`. This would solve some of the immediate issues, but isn't bullet-proof: the following functions still could not be documented if the items in the body failed to resolve:

- Functions containing a nested `DefId` (rust-lang#71104)
- ~~Functions returning `impl Trait` (rust-lang#43878 These ended up not resolving anyway with this PR.
- ~~`const fn`, because `loop {}` in `const fn` is unstable (rust-lang#43636 `const_loop` was just stabilized.

This also isn't exactly what rustdoc wants, which is to avoid looking at function bodies in the first place.

## What changes were made?

The hack implemented in this PR is to add an option to ignore all resolution errors in function bodies. This is enabled only for rustdoc. Since resolution errors are ignored, the MIR generated will be invalid, as can be seen in the following ICE:

```rust
error: internal compiler error: broken MIR in DefId(0:11 ~ doc_cfg[8787]::uses_target_feature[0]) ("return type"): bad type [type error]
  --> /home/joshua/src/rust/src/test/rustdoc/doc-cfg.rs:51:1
   |
51 | / pub unsafe fn uses_target_feature() {
52 | |     content::should::be::irrelevant();
53 | | }
   | |_^
```

Fortunately, rustdoc does not need to access MIR in order to generate documentation. Therefore this also removes the call to `analyze()` in `rustdoc::run_core`. This has the side effect of not generating all lints by default. Most lints are safe to ignore (does rustdoc really need to run liveness analysis?) but `missing_docs` in particular is disabled when it should not be. Re-running `missing_docs` specifically does not help, because it causes the typechecking pass to be run, bringing back the errors from rust-lang#24658:

```
error[E0599]: no method named `into_handle` found for struct `sys::unix::pipe::AnonPipe` in the current scope
  --> src/libstd/sys/windows/ext/process.rs:71:27
   |
71 |         self.into_inner().into_handle().into_raw() as *mut _
   |                           ^^^^^^^^^^^ method not found in `sys::unix::pipe::AnonPipe`
   |
```

Because of rust-lang#73743, we only run typeck on demand. So this only causes an issue for functions returning `impl Trait`, which were already special cased by `ReplaceFunctionWithBody`. However, it now considers `async fn f() -> T` to be considered `impl Future<Output = T>`, where before it was considered to have a concrete `T` type.

## How will this affect future changes to rustdoc?

- Any new changes to rustdoc will not be able to perform type checking without bringing back resolution errors in function bodies.
    + As a corollary, any new lints cannot require or perform type checking. In some cases this may require refactoring other parts of the compiler to perform type-checking only on-demand, see for example rust-lang#73743.
    + As a corollary, rustdoc can never again call `tcx.analysis()` unless this PR is reverted altogether.

## Current status

- ~~I am not yet sure how to bring back `missing_docs` without running typeck. @eddyb suggested allowing lints to opt-out of type-checking, which would probably be another rabbit hole.~~ The opt-out was implemented in rust-lang#73743. However, of the rustc lints, now _only_ missing_docs is run and no other lints: rust-lang#73566 (comment). We need a team decision on whether that's an acceptable tradeoff. Note that all rustdoc lints are still run (`intra_doc_link_resolution_failure`, etc). **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment)
- ~~The implementation of optional errors in `rustc_resolve` is very brute force, it should probably be moved from `LateResolver` to `Resolver` to avoid duplicating the logic in many places.~~ I'm mostly happy with it now.

- This no longer allows errors in `async fn f() -> T`. This caused breakage in 50 crates out of a full crater run, all of which (that I looked at) didn't compile when run with rustc directly. In other words, it used to be that they could not be compiled but could still be documented; now they can't be documented either. This needs a decision from the rustdoc team on whether this is acceptable breakage. **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment)
- ~~This makes `fn typeck_tables_of` in `rustc_typeck` public. This is not desired behavior, but needs the changes from rust-lang#74070 in order to be fixed.~~ Reverted.
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 16, 2020
Don't run `everybody_loops` for rustdoc; instead ignore resolution errors

r? @eddyb
cc @petrochenkov, @GuillaumeGomez, @Manishearth, @ecstatic-morse, @marmeladema

~~Blocked on rust-lang#73743 Merged.
~~Blocked on crater run.~~ Crater popped up some ICEs ([now fixed](rust-lang#73566 (comment))). See [crater run](https://crater-reports.s3.amazonaws.com/pr-73566/index.html), [ICEs](rust-lang#73566 (comment)).
~~Blocked on rust-lang#74070 so that we don't make typeck_tables_of public when it shouldn't be.~~ Merged.

Closes rust-lang#71820, closes rust-lang#71104, closes rust-lang#65863.

## What is the motivation for this change?

As seen from a lengthy trail of PRs and issues (rust-lang#73532, rust-lang#73103, rust-lang#71820, rust-lang#71104), `everybody_loops` is causing bugs in rustdoc. The main issue is that it does not preserve the validity of the `DefId` tree, meaning that operations on DefIds may unexpectedly fail when called later. This is blocking intra-doc links (see rust-lang#73101).

This PR starts by removing `everybody_loops`, fixing rust-lang#71104 and rust-lang#71820. However, that brings back the bugs seen originally in rust-lang#43348: Since libstd documents items for all platforms, the function bodies sometimes do not type check. Here are the errors from documenting `libstd` with `everybody_loops` disabled and no other changes:

```rust
error[E0433]: failed to resolve: could not find `handle` in `sys`
  --> src/libstd/sys/windows/ext/process.rs:13:27
   |
13 |         let handle = sys::handle::Handle::new(handle as *mut _);
   |                           ^^^^^^ could not find `handle` in `sys`

error[E0425]: cannot find function `symlink_inner` in module `sys::fs`
   --> src/libstd/sys/windows/ext/fs.rs:544:14
    |
544 |     sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), false)
    |              ^^^^^^^^^^^^^ not found in `sys::fs`

error[E0425]: cannot find function `symlink_inner` in module `sys::fs`
   --> src/libstd/sys/windows/ext/fs.rs:564:14
    |
564 |     sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), true)
    |              ^^^^^^^^^^^^^ not found in `sys::fs`
```

## Why does this need changes to `rustc_resolve`?

Normally, this could be avoided by simply not calling the `typeck_item_bodies` pass. However, the errors above happen before type checking, in name resolution itself. Since name resolution is intermingled with macro expansion, and rustdoc needs expansion to happen before it knows all items to be documented, there needs to be someway to ignore _resolution_ errors in function bodies.

An alternative solution suggested by @petrochenkov was to not run `everybody_loops` on anything containing a nested `DefId`. This would solve some of the immediate issues, but isn't bullet-proof: the following functions still could not be documented if the items in the body failed to resolve:

- Functions containing a nested `DefId` (rust-lang#71104)
- ~~Functions returning `impl Trait` (rust-lang#43878 These ended up not resolving anyway with this PR.
- ~~`const fn`, because `loop {}` in `const fn` is unstable (rust-lang#43636 `const_loop` was just stabilized.

This also isn't exactly what rustdoc wants, which is to avoid looking at function bodies in the first place.

## What changes were made?

The hack implemented in this PR is to add an option to ignore all resolution errors in function bodies. This is enabled only for rustdoc. Since resolution errors are ignored, the MIR generated will be invalid, as can be seen in the following ICE:

```rust
error: internal compiler error: broken MIR in DefId(0:11 ~ doc_cfg[8787]::uses_target_feature[0]) ("return type"): bad type [type error]
  --> /home/joshua/src/rust/src/test/rustdoc/doc-cfg.rs:51:1
   |
51 | / pub unsafe fn uses_target_feature() {
52 | |     content::should::be::irrelevant();
53 | | }
   | |_^
```

Fortunately, rustdoc does not need to access MIR in order to generate documentation. Therefore this also removes the call to `analyze()` in `rustdoc::run_core`. This has the side effect of not generating all lints by default. Most lints are safe to ignore (does rustdoc really need to run liveness analysis?) but `missing_docs` in particular is disabled when it should not be. Re-running `missing_docs` specifically does not help, because it causes the typechecking pass to be run, bringing back the errors from rust-lang#24658:

```
error[E0599]: no method named `into_handle` found for struct `sys::unix::pipe::AnonPipe` in the current scope
  --> src/libstd/sys/windows/ext/process.rs:71:27
   |
71 |         self.into_inner().into_handle().into_raw() as *mut _
   |                           ^^^^^^^^^^^ method not found in `sys::unix::pipe::AnonPipe`
   |
```

Because of rust-lang#73743, we only run typeck on demand. So this only causes an issue for functions returning `impl Trait`, which were already special cased by `ReplaceFunctionWithBody`. However, it now considers `async fn f() -> T` to be considered `impl Future<Output = T>`, where before it was considered to have a concrete `T` type.

## How will this affect future changes to rustdoc?

- Any new changes to rustdoc will not be able to perform type checking without bringing back resolution errors in function bodies.
    + As a corollary, any new lints cannot require or perform type checking. In some cases this may require refactoring other parts of the compiler to perform type-checking only on-demand, see for example rust-lang#73743.
    + As a corollary, rustdoc can never again call `tcx.analysis()` unless this PR is reverted altogether.

## Current status

- ~~I am not yet sure how to bring back `missing_docs` without running typeck. @eddyb suggested allowing lints to opt-out of type-checking, which would probably be another rabbit hole.~~ The opt-out was implemented in rust-lang#73743. However, of the rustc lints, now _only_ missing_docs is run and no other lints: rust-lang#73566 (comment). We need a team decision on whether that's an acceptable tradeoff. Note that all rustdoc lints are still run (`intra_doc_link_resolution_failure`, etc). **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment)
- ~~The implementation of optional errors in `rustc_resolve` is very brute force, it should probably be moved from `LateResolver` to `Resolver` to avoid duplicating the logic in many places.~~ I'm mostly happy with it now.

- This no longer allows errors in `async fn f() -> T`. This caused breakage in 50 crates out of a full crater run, all of which (that I looked at) didn't compile when run with rustc directly. In other words, it used to be that they could not be compiled but could still be documented; now they can't be documented either. This needs a decision from the rustdoc team on whether this is acceptable breakage. **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment)
- ~~This makes `fn typeck_tables_of` in `rustc_typeck` public. This is not desired behavior, but needs the changes from rust-lang#74070 in order to be fixed.~~ Reverted.
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 16, 2020
Don't run `everybody_loops` for rustdoc; instead ignore resolution errors

r? @eddyb
cc @petrochenkov, @GuillaumeGomez, @Manishearth, @ecstatic-morse, @marmeladema

~~Blocked on rust-lang#73743 Merged.
~~Blocked on crater run.~~ Crater popped up some ICEs ([now fixed](rust-lang#73566 (comment))). See [crater run](https://crater-reports.s3.amazonaws.com/pr-73566/index.html), [ICEs](rust-lang#73566 (comment)).
~~Blocked on rust-lang#74070 so that we don't make typeck_tables_of public when it shouldn't be.~~ Merged.

Closes rust-lang#71820, closes rust-lang#71104, closes rust-lang#65863.

## What is the motivation for this change?

As seen from a lengthy trail of PRs and issues (rust-lang#73532, rust-lang#73103, rust-lang#71820, rust-lang#71104), `everybody_loops` is causing bugs in rustdoc. The main issue is that it does not preserve the validity of the `DefId` tree, meaning that operations on DefIds may unexpectedly fail when called later. This is blocking intra-doc links (see rust-lang#73101).

This PR starts by removing `everybody_loops`, fixing rust-lang#71104 and rust-lang#71820. However, that brings back the bugs seen originally in rust-lang#43348: Since libstd documents items for all platforms, the function bodies sometimes do not type check. Here are the errors from documenting `libstd` with `everybody_loops` disabled and no other changes:

```rust
error[E0433]: failed to resolve: could not find `handle` in `sys`
  --> src/libstd/sys/windows/ext/process.rs:13:27
   |
13 |         let handle = sys::handle::Handle::new(handle as *mut _);
   |                           ^^^^^^ could not find `handle` in `sys`

error[E0425]: cannot find function `symlink_inner` in module `sys::fs`
   --> src/libstd/sys/windows/ext/fs.rs:544:14
    |
544 |     sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), false)
    |              ^^^^^^^^^^^^^ not found in `sys::fs`

error[E0425]: cannot find function `symlink_inner` in module `sys::fs`
   --> src/libstd/sys/windows/ext/fs.rs:564:14
    |
564 |     sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), true)
    |              ^^^^^^^^^^^^^ not found in `sys::fs`
```

## Why does this need changes to `rustc_resolve`?

Normally, this could be avoided by simply not calling the `typeck_item_bodies` pass. However, the errors above happen before type checking, in name resolution itself. Since name resolution is intermingled with macro expansion, and rustdoc needs expansion to happen before it knows all items to be documented, there needs to be someway to ignore _resolution_ errors in function bodies.

An alternative solution suggested by @petrochenkov was to not run `everybody_loops` on anything containing a nested `DefId`. This would solve some of the immediate issues, but isn't bullet-proof: the following functions still could not be documented if the items in the body failed to resolve:

- Functions containing a nested `DefId` (rust-lang#71104)
- ~~Functions returning `impl Trait` (rust-lang#43878 These ended up not resolving anyway with this PR.
- ~~`const fn`, because `loop {}` in `const fn` is unstable (rust-lang#43636 `const_loop` was just stabilized.

This also isn't exactly what rustdoc wants, which is to avoid looking at function bodies in the first place.

## What changes were made?

The hack implemented in this PR is to add an option to ignore all resolution errors in function bodies. This is enabled only for rustdoc. Since resolution errors are ignored, the MIR generated will be invalid, as can be seen in the following ICE:

```rust
error: internal compiler error: broken MIR in DefId(0:11 ~ doc_cfg[8787]::uses_target_feature[0]) ("return type"): bad type [type error]
  --> /home/joshua/src/rust/src/test/rustdoc/doc-cfg.rs:51:1
   |
51 | / pub unsafe fn uses_target_feature() {
52 | |     content::should::be::irrelevant();
53 | | }
   | |_^
```

Fortunately, rustdoc does not need to access MIR in order to generate documentation. Therefore this also removes the call to `analyze()` in `rustdoc::run_core`. This has the side effect of not generating all lints by default. Most lints are safe to ignore (does rustdoc really need to run liveness analysis?) but `missing_docs` in particular is disabled when it should not be. Re-running `missing_docs` specifically does not help, because it causes the typechecking pass to be run, bringing back the errors from rust-lang#24658:

```
error[E0599]: no method named `into_handle` found for struct `sys::unix::pipe::AnonPipe` in the current scope
  --> src/libstd/sys/windows/ext/process.rs:71:27
   |
71 |         self.into_inner().into_handle().into_raw() as *mut _
   |                           ^^^^^^^^^^^ method not found in `sys::unix::pipe::AnonPipe`
   |
```

Because of rust-lang#73743, we only run typeck on demand. So this only causes an issue for functions returning `impl Trait`, which were already special cased by `ReplaceFunctionWithBody`. However, it now considers `async fn f() -> T` to be considered `impl Future<Output = T>`, where before it was considered to have a concrete `T` type.

## How will this affect future changes to rustdoc?

- Any new changes to rustdoc will not be able to perform type checking without bringing back resolution errors in function bodies.
    + As a corollary, any new lints cannot require or perform type checking. In some cases this may require refactoring other parts of the compiler to perform type-checking only on-demand, see for example rust-lang#73743.
    + As a corollary, rustdoc can never again call `tcx.analysis()` unless this PR is reverted altogether.

## Current status

- ~~I am not yet sure how to bring back `missing_docs` without running typeck. @eddyb suggested allowing lints to opt-out of type-checking, which would probably be another rabbit hole.~~ The opt-out was implemented in rust-lang#73743. However, of the rustc lints, now _only_ missing_docs is run and no other lints: rust-lang#73566 (comment). We need a team decision on whether that's an acceptable tradeoff. Note that all rustdoc lints are still run (`intra_doc_link_resolution_failure`, etc). **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment)
- ~~The implementation of optional errors in `rustc_resolve` is very brute force, it should probably be moved from `LateResolver` to `Resolver` to avoid duplicating the logic in many places.~~ I'm mostly happy with it now.

- This no longer allows errors in `async fn f() -> T`. This caused breakage in 50 crates out of a full crater run, all of which (that I looked at) didn't compile when run with rustc directly. In other words, it used to be that they could not be compiled but could still be documented; now they can't be documented either. This needs a decision from the rustdoc team on whether this is acceptable breakage. **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment)
- ~~This makes `fn typeck_tables_of` in `rustc_typeck` public. This is not desired behavior, but needs the changes from rust-lang#74070 in order to be fixed.~~ Reverted.
@bors bors closed this as completed in c23f045 Jul 16, 2020
jonhoo added a commit to mit-pdos/noria that referenced this issue Jul 27, 2020
@jyn514
Copy link
Member

jyn514 commented Nov 17, 2020

As an interesting aside, when trying to work around this using Box<dyn Trait> under #[cfg(doc)], I kept running into this unimplemented! in librustdoc.

@jonhoo Did you ever open a bug for this? It would be nice to fix.

@jonhoo
Copy link
Contributor

jonhoo commented Nov 17, 2020

@jyn514 Hmm, not sure actually -- it's a while ago now 😅

nytopop added a commit to nytopop/hyperbole that referenced this issue Nov 18, 2020
The rustdoc issue which this workaround was working around has since
been resolved.

related: rust-lang/rust#65863
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` requires-nightly This issue requires a nightly compiler in some way. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Development

Successfully merging a pull request may close this issue.

9 participants