Don't run everybody_loops for rustdoc; instead ignore resolution errors

[jyn514]

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

Blocked on #73743 Merged.
Blocked on crater run. Crater popped up some ICEs (now fixed). See crater run, ICEs.
Blocked on #74070 so that we don't make typeck_tables_of public when it shouldn't be. Merged.

Closes #71820, closes #71104, closes #65863.

What is the motivation for this change?

As seen from a lengthy trail of PRs and issues (#73532, #73103, #71820, #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 #73101).

This PR starts by removing everybody_loops, fixing #71104 and #71820. However, that brings back the bugs seen originally in #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:

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 (Some NodeId/LocalDefId don't have a corresponding HirId #71104)
• Functions returning impl Trait (Exclude impl Trait functions from everybody_loops. #43878) These ended up not resolving anyway with this PR.
• const fn, because loop {} in const fn is unstable (--unpretty everybody_loops is not compatible with const fn. #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:

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 #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 #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 rustc_lint: only query typeck_tables_of when a lint needs it. #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 rustc_lint: only query typeck_tables_of when a lint needs it. #73743. However, of the rustc lints, now only missing_docs is run and no other lints: Don't run everybody_loops for rustdoc; instead ignore resolution errors #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 Don't run everybody_loops for rustdoc; instead ignore resolution errors #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 Don't run everybody_loops for rustdoc; instead ignore resolution errors #73566 (comment)

• This makes fn typeck_tables_of in rustc_typeck public. This is not desired behavior, but needs the changes from Use for<'tcx> fn pointers in Providers, instead of having Providers<'tcx>. #74070 in order to be fixed. Reverted.

[jyn514]

I'm not sure if perf runs are a thing for rustdoc, but if so I would be very curious whether this helps or hurts performance.

[Manishearth]

This is a neat approach! Looks like there's still some work to be done for missing_docs, but I like this idea!

(I'd defer to eddy on the actual review)

[Manishearth]

I am not yet sure how to bring back missing_docs without running typeck

Why do we need typeck for this lint? I'm confused: can we not run this lint on the HIR?

[jyn514]

Why do we need typeck for this lint? I'm confused: can we not run this lint on the HIR?

We don't need typeck, but it's being run anyway. Currently typeck is run unconditionally for all lints:

rust/src/librustc_lint/late.rs

Lines 107 to 113 in d4e143e

	fn visit_nested_body(&mut self, body: hir::BodyId) {
	let old_tables = self.context.tables;
	self.context.tables = self.context.tcx.body_tables(body);
	let body = self.context.tcx.hir().body(body);
	self.visit_body(body);
	self.context.tables = old_tables;
	}

body_tables calls typeck_tables_of.

[pnkfelix]

r? @pnkfelix

[Manishearth]

We don't need typeck, but it's being run anyway. Currently typeck is run unconditionally for all lints:

Wait, but, we want to run the missing_docs lint in rustc, not rustdoc, yes? So how does this matter? The only issue is that missing_docs won't complain about cross-platform docs, which seems okay

[pnkfelix]

We don't need typeck, but it's being run anyway. Currently typeck is run unconditionally for all lints:

Wait, but, we want to run the missing_docs lint in rustc, not rustdoc, yes? So how does this matter? The only issue is that missing_docs won't complain about cross-platform docs, which seems okay

To be clear: Are you suggesting that @jyn514 just revise their PR to disable the missing_docs lint solely when rustdoc is running (i.e. baseed on sess.opts.actually_rustdoc, as is done elsewhere)?

I just wanted to make sure I'm interpreting your feedback the right way.

[Manishearth]

To be clear: Are you suggesting that @jyn514 just revise their PR to disable the missing_docs lint solely when rustdoc is running (i.e. baseed on sess.opts.actually_rustdoc, as is done elsewhere)?

So I was under the impression that rustdoc doesn't run normal lints. Turns out, it does, just some very whitelisted ones

rust/src/librustdoc/core.rs

Lines 306 to 311 in 1a4e2b6

	let intra_link_resolution_failure_name = lint::builtin::INTRA_DOC_LINK_RESOLUTION_FAILURE.name;
	let missing_docs = rustc_lint::builtin::MISSING_DOCS.name;
	let missing_doc_example = rustc_lint::builtin::MISSING_DOC_CODE_EXAMPLES.name;
	let private_doc_tests = rustc_lint::builtin::PRIVATE_DOC_TESTS.name;
	let no_crate_level_docs = rustc_lint::builtin::MISSING_CRATE_LEVEL_DOCS.name;
	let invalid_codeblock_attribute_name = rustc_lint::builtin::INVALID_CODEBLOCK_ATTRIBUTE.name;

Given this mechanism it seems like this just disables the reporting of some lints, it's not clear how this disables them from running, though. Maybe I'm misreading this.

Either way! It should be possible to write missing_docs as a builtin rustdoc pass if we so wish, so it relies on rustdoc's lint framework, not rustc's. This would mean having two potentially different versions of the lint running, though.

[jyn514]

Given this mechanism it seems like this just disables the reporting of some lints, it's not clear how this disables them from running, though. Maybe I'm misreading this.

As far as I understand, all the lints were previously run by analysis(). Since I removed the call to analysis(), that means that any lints from rustc that rustdoc wants have to be opted-into explicitly.

Either way! It should be possible to write missing_docs as a builtin rustdoc pass if we so wish, so it relies on rustdoc's lint framework, not rustc's. This would mean having two potentially different versions of the lint running, though.

That seems a lot more complicated than making typeck opt-out for lints 😅

[jyn514]

To be clear: Are you suggesting that @jyn514 just revise their PR to disable the missing_docs lint solely when rustdoc is running (i.e. baseed on sess.opts.actually_rustdoc, as is done elsewhere)?

To remove the missing_docs lint I'd only have to remove the call to rustc_lint::MissingDoc (no special casing the compiler needed). I don't think that's desired behavior though.

[Manishearth]

As far as I understand, all the lints were previously run by analysis()

aha! I see.

That seems a lot more complicated than making typeck opt-out for lints sweat_smile

Right, but you still have the problem that all lints are being run when you do this, not just the ones you care about.

What I'm trying to suggest is that we manually run the missing_docs pass, either by reusing the same code or rewriting it, as part of rustdoc directly.

[jyn514]

Right, but you still have the problem that all lints are being run when you do this, not just the ones you care about.

I don't think that's the case? The missing_doc pass is only run here, I would be very surprised if that caused the full suite of lints to be emitted. This is beyond my knowledge of the compiler though.

[Manishearth]

Oh, I see. Never mind.

I guess it's okayish to take that approach, I'm just concerned that it will make the lints more brittle. The missing docs lint pass is quite straightforward, it should be super easy to replicate it as a visitor or a doc pass.