Don't steal the resolver when lowering HIR; instead store an immutable resolver in TyCtxt

[jyn514]

Motivation

Currently, the resolver is stolen when lowering to HIR. This means that it cannot be used at all after creating a TyCtxt. In particular, rustdoc wants to use it afterwards for resolving intra-doc links. The current hack is to clone the whole resolver, and use the clone to look up links. This is causing numerous bugs; the most common is that the CStore of the clone is out of sync with the CStore of the TyCtxt, causing ICEs when looking up a DefId that's only present in the clone.

For more context, see rust-lang/rust#83761.

Proposal

Instead of cloning the resolver or requiring ownership: https://github.com/rust-lang/rust/blob/f434217aab9abf583ebc928b97ab4116921137aa/compiler/rustc_interface/src/passes.rs#L143-L148
Remove the Steal from Queries::expansion. Use expansion().peek_mut() when initially constructing the HIR, then never modify the resolver again. This can be enforced by using a trait that only allows immutable access:

diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs
index 970e669c16f..cf3e9109910 100644
--- a/compiler/rustc_middle/src/ty/context.rs
+++ b/compiler/rustc_middle/src/ty/context.rs
@@ -932,6 +932,10 @@ fn deref(&self) -> &Self::Target {
     }
 }

+pub trait Resolver {
+    fn resolver_outputs(&self) -> &ty::ResolverOutputs;
+}
+
 pub struct GlobalCtxt<'tcx> {
     pub arena: &'tcx WorkerLocal<Arena<'tcx>>,

@@ -1129,7 +1133,7 @@ pub fn create_global_ctxt(
         s: &'tcx Session,
         lint_store: Lrc<dyn Any + sync::Send + sync::Sync>,
         arena: &'tcx WorkerLocal<Arena<'tcx>>,
-        resolutions: ty::ResolverOutputs,
+        resolver: &'tcx impl Resolver,
         krate: &'tcx hir::Crate<'tcx>,
         dep_graph: DepGraph,
         on_disk_cache: Option<query::OnDiskCache<'tcx>>,

Previous discussion (copied from the linked issue):

jyn514: Could the tcx store the resolver itself?

petrochenkov: It may be possible.
Resolver contains a lot of early compilation stuff like NodeIds that is out of place in tcx though.
Also it may be preferable to finish all the resolution activities as early as possible (at least those that may result in loading new crates).
I think we may end up with some scheme working similarly to this if incremental compilation and queries advance so much that we'll start skipping resolution of paths if they are not actually used, but that would probably be a very different resolver.

I think as long as the TyCtxt doesn't have mutable access to the resolver it won't change things very much, it will only interact with DefIds and not NodeIds. I'm not suggesting that we make path resolution incremental as part of this change (although it may make it easier to do later).

Alternatives

Change rustdoc to do all path resolution before the TyCtxt is created. I do not think this is feasible, I spent a good two weeks on it with very little progress (see the history of jyn514/rust@8d3d43f). I am no longer willing to work on it and I highly doubt anyone else is.

Mentors or Reviewers

@petrochenkov

Process

The main points of the Major Change Process are as follows:

• File an issue describing the proposal.
• A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
  • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
  • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
• Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

[rustbot]

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

[xldenis]

Would this allow a user to perform resolution with just a TyCtxt on hand?

[bjorn3]

It shouldn't IMO. Resolution is not tracked by incr comp. We are currently in the process of making all state that is not tracked by incr comp inaccessible from TyCtxt because of several bugs where we forgot to mark the queries accessing such state as eval_always causing miscompilations or crashes with incr comp: rust-lang/rust#85941

[jyn514]

Vadim Petrochenkov and I talked this over and we have a plan for making this work without major changes to the resolver; he convinced me it will take a very long time for the resolver to be part of incremental, and I want to try and fix these issues without having to wait.