Skip to content

Resolver: Batched Import Resolution #145108

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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions compiler/rustc_hir/src/def.rs
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,24 @@ impl<T> PerNS<T> {
pub fn iter(&self) -> IntoIter<&T, 3> {
[&self.value_ns, &self.type_ns, &self.macro_ns].into_iter()
}

pub fn into_iter_with(self) -> IntoIter<(Namespace, T), 3> {
[
(Namespace::TypeNS, self.type_ns),
(Namespace::ValueNS, self.value_ns),
(Namespace::MacroNS, self.macro_ns),
]
.into_iter()
}

pub fn iter_with(&self) -> IntoIter<(Namespace, &T), 3> {
[
(Namespace::TypeNS, &self.type_ns),
(Namespace::ValueNS, &self.value_ns),
(Namespace::MacroNS, &self.macro_ns),
]
.into_iter()
}
}

impl<T> ::std::ops::Index<Namespace> for PerNS<T> {
Expand Down
17 changes: 16 additions & 1 deletion compiler/rustc_resolve/src/build_reduced_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,22 @@ impl<'a, 'ra, 'tcx> BuildReducedGraphVisitor<'a, 'ra, 'tcx> {
}
// We don't add prelude imports to the globs since they only affect lexical scopes,
// which are not relevant to import resolution.
ImportKind::Glob { is_prelude: true, .. } => {}
ImportKind::Glob { is_prelude: true, .. } => {
// We do resolve the prelude path so it's set before import resolution.
let path_res = self.r.cm().maybe_resolve_path(
&import.module_path,
None,
&import.parent_scope,
Some(import),
);
if let PathResult::Module(
module_or_uniform_root @ ModuleOrUniformRoot::Module(module),
) = path_res
{
import.imported_module.set(Some(module_or_uniform_root));
self.r.prelude = Some(module);
}
}
ImportKind::Glob { .. } => current_module.globs.borrow_mut().push(import),
_ => unreachable!(),
}
Expand Down
177 changes: 128 additions & 49 deletions compiler/rustc_resolve/src/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@

use std::cell::Cell;
use std::mem;
use std::ops::Deref;

use rustc_ast::NodeId;
use rustc_data_structures::fx::{FxHashSet, FxIndexSet};
use rustc_data_structures::fx::{FxHashSet, FxIndexMap, FxIndexSet};
use rustc_data_structures::intern::Interned;
use rustc_errors::codes::*;
use rustc_errors::{Applicability, MultiSpan, pluralize, struct_span_code_err};
Expand Down Expand Up @@ -41,6 +42,93 @@ use crate::{

type Res = def::Res<NodeId>;

pub(crate) struct ImportResolver<'r, 'ra, 'tcx> {
r: CmResolver<'r, 'ra, 'tcx>, // always immutable
outputs: ImportResolutionOutputs<'ra>,
}

#[derive(Default)]
struct ImportResolutionOutputs<'ra> {
prelude: Option<Module<'ra>>,
indeterminate_imports: Vec<Import<'ra>>,
determined_imports: Vec<Import<'ra>>,
module_glob_importers: FxIndexMap<Module<'ra>, Vec<Import<'ra>>>,
glob_import_bindings:
Vec<(Module<'ra>, BindingKey, NameBinding<'ra>, bool /* warn_ambiguity */)>,
glob_path_res: Vec<(NodeId, PartialRes)>,
single_import_bindings: PerNS<Vec<(Module<'ra>, Import<'ra>, PendingBinding<'ra>)>>,
}
Copy link
Contributor

@petrochenkov petrochenkov Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest restructuring this structure like this:

struct ImportResolutionOutputs {
  indeterminate_imports: Vec<Import>,
  determined_imports: Vec<(Import, SideEffect)>,
}

where SideEffect is a set of changes that need to be applied for that specific import.

Then side effects will be applied in the same order in which the imports are resolved.
It will avoid having superfluous differences with the old algorithm, in emitted diagnostics in particular, and should make debugging the remaining differences easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, nice. That sounds better indeed, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way the diagnostics will also go in the source code order in common case, which is nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And we would thus not need those changes to the stderr files of some tests, very nice.


impl<'r, 'ra, 'tcx> ImportResolver<'r, 'ra, 'tcx> {
pub(crate) fn new(cmr: CmResolver<'r, 'ra, 'tcx>) -> Self {
ImportResolver { r: cmr, outputs: Default::default() }
}

fn into_outputs(self) -> ImportResolutionOutputs<'ra> {
self.outputs
}
}

impl<'ra> ImportResolutionOutputs<'ra> {
fn commit<'tcx>(self, r: &mut Resolver<'ra, 'tcx>) {
r.indeterminate_imports = self.indeterminate_imports;
r.determined_imports.extend(self.determined_imports);

// It's possible this particular round didn't set the prelude, so we should not
// unset it in the main resolver.
if self.prelude.is_some() {
r.prelude = self.prelude;
}

for (module, glob_importers) in self.module_glob_importers {
module.glob_importers.borrow_mut().extend(glob_importers);
}

for (module, key, binding, warn_ambiguity) in self.glob_import_bindings {
let _ = r.try_define_local(module, key.ident.0, key.ns, binding, warn_ambiguity);
}

for (id, res) in self.glob_path_res {
r.record_partial_res(id, res);
}

for (ns, import_bindings) in self.single_import_bindings.into_iter_with() {
for (parent, import, pending_binding) in import_bindings {
let ImportKind::Single { target, ref bindings, .. } = import.kind else {
unreachable!();
};
match pending_binding {
PendingBinding::Ready(Some(binding)) => {
r.define_binding_local(parent, target, ns, binding);
}
PendingBinding::Ready(None) => {
let key = BindingKey::new(target, ns);
r.update_local_resolution(parent, key, false, |_, resolution| {
resolution.single_imports.swap_remove(&import);
});
}
_ => {}
}
bindings[ns].set(pending_binding);
}
}
}
}

impl<'r, 'ra, 'tcx> Deref for ImportResolver<'r, 'ra, 'tcx> {
type Target = Resolver<'ra, 'tcx>;

fn deref(&self) -> &Self::Target {
self.r.deref()
}
}

impl<'r, 'ra, 'tcx> AsRef<Resolver<'ra, 'tcx>> for ImportResolver<'r, 'ra, 'tcx> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it is convenient to have the self: &mut ImportResolver methods because it makes the diff easier to read.
But right before merge we'll need to move the methods to ImportResolver itself, and remove the Deref/AsRef impls.

fn as_ref(&self) -> &Resolver<'ra, 'tcx> {
self.r.as_ref()
}
}

/// A [`NameBinding`] in the process of being resolved.
#[derive(Clone, Copy, Default, PartialEq)]
pub(crate) enum PendingBinding<'ra> {
Expand Down Expand Up @@ -552,22 +640,33 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
/// Resolves all imports for the crate. This method performs the fixed-
/// point iteration.
pub(crate) fn resolve_imports(&mut self) {
self.assert_speculative = true;
let mut prev_indeterminate_count = usize::MAX;
let mut indeterminate_count = self.indeterminate_imports.len() * 3;
while indeterminate_count < prev_indeterminate_count {
prev_indeterminate_count = indeterminate_count;
indeterminate_count = 0;
for import in mem::take(&mut self.indeterminate_imports) {
let import_indeterminate_count = self.cm().resolve_import(import);
indeterminate_count += import_indeterminate_count;
match import_indeterminate_count {
0 => self.determined_imports.push(import),
_ => self.indeterminate_imports.push(import),
}
let batch = mem::take(&mut self.indeterminate_imports);
self.assert_speculative = true;
let (outputs, count) = ImportResolver::new(self.cm()).resolve_batch(batch);
self.assert_speculative = false;
indeterminate_count = count;
outputs.commit(self);
}
}

fn resolve_batch<'r>(
mut self: ImportResolver<'r, 'ra, 'tcx>,
batch: Vec<Import<'ra>>,
) -> (ImportResolutionOutputs<'ra>, usize) {
let mut indeterminate_count = 0;
for import in batch {
let import_indeterminate_count = self.resolve_import(import);
indeterminate_count += import_indeterminate_count;
match import_indeterminate_count {
0 => self.outputs.determined_imports.push(import),
_ => self.outputs.indeterminate_imports.push(import),
}
}
self.assert_speculative = false;
(self.into_outputs(), indeterminate_count)
}

pub(crate) fn finalize_imports(&mut self) {
Expand Down Expand Up @@ -840,16 +939,17 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
///
/// Meanwhile, if resolve successful, the resolved bindings are written
/// into the module.
fn resolve_import<'r>(mut self: CmResolver<'r, 'ra, 'tcx>, import: Import<'ra>) -> usize {
fn resolve_import<'r>(self: &mut ImportResolver<'r, 'ra, 'tcx>, import: Import<'ra>) -> usize {
debug!(
"(resolving import for module) resolving import `{}::...` in `{}`",
"(resolving import for module) resolving import `{}::{}` in `{}`",
Segment::names_to_string(&import.module_path),
import_kind_to_string(&import.kind),
module_to_string(import.parent_scope.module).unwrap_or_else(|| "???".to_string()),
);
let module = if let Some(module) = import.imported_module.get() {
module
} else {
let path_res = self.reborrow().maybe_resolve_path(
let path_res = self.r.reborrow().maybe_resolve_path(
&import.module_path,
None,
&import.parent_scope,
Expand All @@ -869,16 +969,14 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
(source, target, bindings, type_ns_only)
}
ImportKind::Glob { .. } => {
// FIXME: Use mutable resolver directly as a hack, this should be an output of
// specualtive resolution.
self.get_mut_unchecked().resolve_glob_import(import);
self.resolve_glob_import(import);
return 0;
}
_ => unreachable!(),
};

let mut indeterminate_count = 0;
self.per_ns_cm(|this, ns| {
self.r.reborrow().per_ns_cm(|this, ns| {
if !type_ns_only || ns == TypeNS {
if bindings[ns].get() != PendingBinding::Pending {
return;
Expand All @@ -891,7 +989,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
Some(import),
);
let parent = import.parent_scope.module;
let binding = match binding_result {
let pending_binding = match binding_result {
Ok(binding) => {
if binding.is_assoc_item()
&& !this.tcx.features().import_trait_associated_functions()
Expand All @@ -906,39 +1004,21 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
}
// We need the `target`, `source` can be extracted.
let imported_binding = this.import(binding, import);
// FIXME: Use mutable resolver directly as a hack, this should be an output of
// specualtive resolution.
this.get_mut_unchecked().define_binding_local(
parent,
target,
ns,
imported_binding,
);
PendingBinding::Ready(Some(imported_binding))
}
Err(Determinacy::Determined) => {
// Don't remove underscores from `single_imports`, they were never added.
if target.name != kw::Underscore {
let key = BindingKey::new(target, ns);
// FIXME: Use mutable resolver directly as a hack, this should be an output of
// specualtive resolution.
this.get_mut_unchecked().update_local_resolution(
parent,
key,
false,
|_, resolution| {
resolution.single_imports.swap_remove(&import);
},
);
if target.name == kw::Underscore {
return;
}
PendingBinding::Ready(None)
}
Err(Determinacy::Undetermined) => {
indeterminate_count += 1;
PendingBinding::Pending
return;
}
};
bindings[ns].set(binding);
self.outputs.single_import_bindings[ns].push((parent, import, pending_binding));
}
});

Expand Down Expand Up @@ -1483,7 +1563,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
false
}

fn resolve_glob_import(&mut self, import: Import<'ra>) {
fn resolve_glob_import<'r>(self: &mut ImportResolver<'r, 'ra, 'tcx>, import: Import<'ra>) {
// This function is only called for glob imports.
let ImportKind::Glob { id, is_prelude, .. } = import.kind else { unreachable!() };

Expand All @@ -1505,12 +1585,12 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
if module == import.parent_scope.module {
return;
} else if is_prelude {
self.prelude = Some(module);
self.outputs.prelude = Some(module);
return;
}

// Add to module's glob_importers
module.glob_importers.borrow_mut().push(import);
self.outputs.module_glob_importers.entry(module).or_default().push(import);

// Ensure that `resolutions` isn't borrowed during `try_define`,
// since it might get updated via a glob cycle.
Expand All @@ -1534,18 +1614,17 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
.resolution(import.parent_scope.module, key)
.and_then(|r| r.binding())
.is_some_and(|binding| binding.warn_ambiguity_recursive());
let _ = self.try_define_local(
self.outputs.glob_import_bindings.push((
import.parent_scope.module,
key.ident.0,
key.ns,
key,
imported_binding,
warn_ambiguity,
);
));
}
}

// Record the destination of this import
self.record_partial_res(id, PartialRes::new(module.res().unwrap()));
self.outputs.glob_path_res.push((id, PartialRes::new(module.res().unwrap())));
}

// Miscellaneous post-processing, including recording re-exports,
Expand Down
6 changes: 0 additions & 6 deletions compiler/rustc_resolve/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2501,12 +2501,6 @@ mod ref_mut {
true => self.p,
}
}

/// Returns a mutable reference to the inner value without checking if
/// it's in a mutable state.
pub(crate) fn get_mut_unchecked(&mut self) -> &mut T {
self.p
}
}
}

Expand Down
8 changes: 3 additions & 5 deletions tests/ui/imports/ambiguous-9.rs
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is definitely wrong.

Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,16 @@ pub mod dsl {
mod range {
pub fn date_range() {}
}
pub use self::range::*; //~ WARNING ambiguous glob re-exports
pub use self::range::*;
use super::prelude::*;
}

pub mod prelude {
mod t {
pub fn date_range() {}
}
pub use self::t::*; //~ WARNING ambiguous glob re-exports
pub use super::dsl::*;
pub use self::t::*;
pub use super::dsl::*; //~ WARNING ambiguous glob re-exports
}

use dsl::*;
Expand All @@ -23,6 +23,4 @@ fn main() {
date_range();
//~^ ERROR `date_range` is ambiguous
//~| WARNING this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
//~| ERROR `date_range` is ambiguous
//~| WARNING this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
}
Loading
Loading