Description
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
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 mergeon either the MCP or the PR.
- Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a
- 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.