Don't generate fake IDs in ReplaceBodyWithLoop #73532
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It turns out that this has not been working for who knows how long.
Previously:
```
pub fn h() { 1 + 2; }
```
After this change:
```
pub fn h() { loop {} }
```
This only affected the pass when run with the command line
pretty-printing option, so rustdoc was still replacing bodies with
`loop {}`.
rust-highfive
added
the
S-waiting-on-review
|
Most of |
|
This did not fix the panics in #73101. I'm not sure why since it got rid of the fake parents. I think I might need to update the |
|
For context, this is what fn main() {
{
macro m { () => { } }
}
loop { }
}and this is what it does after: fn main() {
macro m { () => { } }
loop { }
} |
|
The other error is while documenting libcore: error[E0412]: cannot find type `BorrowedPlaceholder` in this scope
--> src/libcore/fmt/mod.rs:2205:32
|
2205 | impl Debug for BorrowedPlaceholder {
| ^^^^^^^^^^^^^^^^^^^ not found in this scopeThis is for the following snippet: 2195 #[stable(feature = "rust1", since = "1.0.0")]
2196 impl<T: ?Sized + Debug> Debug for RefCell<T> {
2197 fn fmt(&self, f: &mut Formatter<'_>) -> Result {
2198 match self.try_borrow() {
2199 Ok(borrow) => f.debug_struct("RefCell").field("value", &borrow).f
2199 inish(),
2200 Err(_) => {
2201 // The RefCell is mutably borrowed so we can't look at its va
2201 lue
2202 // here. Show a placeholder instead.
2203 struct BorrowedPlaceholder;
2204
2205 impl Debug for BorrowedPlaceholder {
2206 fn fmt(&self, f: &mut Formatter<'_>) -> Result {
2207 f.write_str("<borrowed>")
2208 }
2209 }
2210
2211 f.debug_struct("RefCell").field("value", &BorrowedPlaceholder 2211 ).finish()
2212 }
2213 }
2214 }
2215 }I'm moving the |
|
The job 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 |
|
It seems like the DefId for the macro is being generated before this pass? Do you know where that happens? I think the AST pass needs to be run before the DefId is generated or else the DefId parent and the HirId parent won't match up. |
|
Ok yeah, DefIds are generated when macros are expanded: rust/src/librustc_interface/passes.rs Line 302 in 21ddf4d which eventually calls It makes sense to me that macros are expanded first (because they could generate new functions) but I'm not sure how DefIds fit in here. Maybe this pass can be moved after expansion but before the DefIds are generated? |
|
Sounds good to me. |
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
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 pull request
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 pull request
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 pull request
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up to #73103 with less hackiness and special-casing.
Closes #71820, closes #71104. Should fix #73101, I'm testing locally now.
Currently this includes the changes from #73523 but I can remove those without trouble.
r? @ecstatic-morse