From 2c657901605ed5e6e6d3ab52e824f35e25cf4516 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Fri, 25 Jul 2025 22:14:09 +0300 Subject: [PATCH 1/2] resolve: Minimize borrow scopes for `resolutions` --- .../rustc_resolve/src/build_reduced_graph.rs | 6 +- compiler/rustc_resolve/src/check_unused.rs | 4 +- compiler/rustc_resolve/src/diagnostics.rs | 5 +- .../src/effective_visibilities.rs | 4 +- compiler/rustc_resolve/src/imports.rs | 63 +++++++++---------- .../rustc_resolve/src/late/diagnostics.rs | 13 ++-- 6 files changed, 46 insertions(+), 49 deletions(-) diff --git a/compiler/rustc_resolve/src/build_reduced_graph.rs b/compiler/rustc_resolve/src/build_reduced_graph.rs index 83ec037a975d4..13fc068f8a3d2 100644 --- a/compiler/rustc_resolve/src/build_reduced_graph.rs +++ b/compiler/rustc_resolve/src/build_reduced_graph.rs @@ -452,8 +452,10 @@ impl<'a, 'ra, 'tcx> BuildReducedGraphVisitor<'a, 'ra, 'tcx> { self.r.per_ns(|this, ns| { if !type_ns_only || ns == TypeNS { let key = BindingKey::new(target, ns); - let mut resolution = this.resolution(current_module, key).borrow_mut(); - resolution.single_imports.insert(import); + this.resolution(current_module, key) + .borrow_mut() + .single_imports + .insert(import); } }); } diff --git a/compiler/rustc_resolve/src/check_unused.rs b/compiler/rustc_resolve/src/check_unused.rs index 81ee02ac3c71f..b85a814776a7f 100644 --- a/compiler/rustc_resolve/src/check_unused.rs +++ b/compiler/rustc_resolve/src/check_unused.rs @@ -509,9 +509,7 @@ impl Resolver<'_, '_> { let mut check_redundant_imports = FxIndexSet::default(); for module in self.arenas.local_modules().iter() { for (_key, resolution) in self.resolutions(*module).borrow().iter() { - let resolution = resolution.borrow(); - - if let Some(binding) = resolution.best_binding() + if let Some(binding) = resolution.borrow().best_binding() && let NameBindingKind::Import { import, .. } = binding.kind && let ImportKind::Single { id, .. } = import.kind { diff --git a/compiler/rustc_resolve/src/diagnostics.rs b/compiler/rustc_resolve/src/diagnostics.rs index 75eed1e9ad3fc..2251539738c46 100644 --- a/compiler/rustc_resolve/src/diagnostics.rs +++ b/compiler/rustc_resolve/src/diagnostics.rs @@ -2659,10 +2659,9 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { return None; } - let resolutions = self.resolutions(crate_module).borrow(); let binding_key = BindingKey::new(ident, MacroNS); - let resolution = resolutions.get(&binding_key)?; - let binding = resolution.borrow().binding()?; + let binding = + self.resolutions(crate_module).borrow().get(&binding_key)?.borrow().binding()?; let Res::Def(DefKind::Macro(MacroKind::Bang), _) = binding.res() else { return None; }; diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs index 34d1e9552fd71..fe6e5b8e6eb6a 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/compiler/rustc_resolve/src/effective_visibilities.rs @@ -114,9 +114,7 @@ impl<'a, 'ra, 'tcx> EffectiveVisibilitiesVisitor<'a, 'ra, 'tcx> { /// including their whole reexport chains. fn set_bindings_effective_visibilities(&mut self, module_id: LocalDefId) { let module = self.r.expect_module(module_id.to_def_id()); - let resolutions = self.r.resolutions(module); - - for (_, name_resolution) in resolutions.borrow().iter() { + for (_, name_resolution) in self.r.resolutions(module).borrow().iter() { let Some(mut binding) = name_resolution.borrow().binding() else { continue; }; diff --git a/compiler/rustc_resolve/src/imports.rs b/compiler/rustc_resolve/src/imports.rs index 986e703c0d23b..ee82afd967689 100644 --- a/compiler/rustc_resolve/src/imports.rs +++ b/compiler/rustc_resolve/src/imports.rs @@ -651,7 +651,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { for module in self.arenas.local_modules().iter() { for (key, resolution) in self.resolutions(*module).borrow().iter() { let resolution = resolution.borrow(); - let Some(binding) = resolution.best_binding() else { continue }; if let NameBindingKind::Import { import, .. } = binding.kind @@ -1202,41 +1201,39 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { }); return if all_ns_failed { - let resolutions = match module { - ModuleOrUniformRoot::Module(module) => Some(self.resolutions(module).borrow()), - _ => None, - }; - let resolutions = resolutions.as_ref().into_iter().flat_map(|r| r.iter()); - let names = resolutions - .filter_map(|(BindingKey { ident: i, .. }, resolution)| { - if i.name == ident.name { - return None; - } // Never suggest the same name - match *resolution.borrow() { - ref resolution - if let Some(name_binding) = resolution.best_binding() => - { - match name_binding.kind { - NameBindingKind::Import { binding, .. } => { - match binding.kind { - // Never suggest the name that has binding error - // i.e., the name that cannot be previously resolved - NameBindingKind::Res(Res::Err) => None, - _ => Some(i.name), + let names = match module { + ModuleOrUniformRoot::Module(module) => { + self.resolutions(module) + .borrow() + .iter() + .filter_map(|(BindingKey { ident: i, .. }, resolution)| { + if i.name == ident.name { + return None; + } // Never suggest the same name + + let resolution = resolution.borrow(); + if let Some(name_binding) = resolution.best_binding() { + match name_binding.kind { + NameBindingKind::Import { binding, .. } => { + match binding.kind { + // Never suggest the name that has binding error + // i.e., the name that cannot be previously resolved + NameBindingKind::Res(Res::Err) => None, + _ => Some(i.name), + } } + _ => Some(i.name), } - _ => Some(i.name), + } else if resolution.single_imports.is_empty() { + None + } else { + Some(i.name) } - } - NameResolution { ref single_imports, .. } - if single_imports.is_empty() => - { - None - } - _ => Some(i.name), - } - }) - .collect::>(); + }) + .collect() + } + _ => Vec::new(), + }; let lev_suggestion = find_best_match_for_name(&names, ident.name, None).map(|suggestion| { diff --git a/compiler/rustc_resolve/src/late/diagnostics.rs b/compiler/rustc_resolve/src/late/diagnostics.rs index 69095942f5246..203827a9e1a9d 100644 --- a/compiler/rustc_resolve/src/late/diagnostics.rs +++ b/compiler/rustc_resolve/src/late/diagnostics.rs @@ -1461,15 +1461,17 @@ impl<'ast, 'ra, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> { if let PathResult::Module(ModuleOrUniformRoot::Module(module)) = self.resolve_path(mod_path, None, None) { - let resolutions = self.r.resolutions(module).borrow(); - let targets: Vec<_> = resolutions + let targets: Vec<_> = self + .r + .resolutions(module) + .borrow() .iter() .filter_map(|(key, resolution)| { resolution .borrow() .best_binding() .map(|binding| binding.res()) - .and_then(|res| if filter_fn(res) { Some((key, res)) } else { None }) + .and_then(|res| if filter_fn(res) { Some((*key, res)) } else { None }) }) .collect(); if let [target] = targets.as_slice() { @@ -2300,8 +2302,9 @@ impl<'ast, 'ra, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> { return None; } - let resolutions = self.r.resolutions(*module); - let targets = resolutions + let targets = self + .r + .resolutions(*module) .borrow() .iter() .filter_map(|(key, res)| { From e82011255b5feb43cf3379d4d3495629e99c09b1 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sat, 26 Jul 2025 00:21:01 +0300 Subject: [PATCH 2/2] resolve: Do not create `NameResolution`s on access unless necessary --- compiler/rustc_resolve/src/build_reduced_graph.rs | 2 +- compiler/rustc_resolve/src/diagnostics.rs | 3 +-- compiler/rustc_resolve/src/ident.rs | 9 +++++++-- compiler/rustc_resolve/src/imports.rs | 5 ++--- compiler/rustc_resolve/src/late.rs | 6 ++---- compiler/rustc_resolve/src/lib.rs | 13 ++++++++++--- 6 files changed, 23 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_resolve/src/build_reduced_graph.rs b/compiler/rustc_resolve/src/build_reduced_graph.rs index 13fc068f8a3d2..ac64385d66431 100644 --- a/compiler/rustc_resolve/src/build_reduced_graph.rs +++ b/compiler/rustc_resolve/src/build_reduced_graph.rs @@ -452,7 +452,7 @@ impl<'a, 'ra, 'tcx> BuildReducedGraphVisitor<'a, 'ra, 'tcx> { self.r.per_ns(|this, ns| { if !type_ns_only || ns == TypeNS { let key = BindingKey::new(target, ns); - this.resolution(current_module, key) + this.resolution_or_default(current_module, key) .borrow_mut() .single_imports .insert(import); diff --git a/compiler/rustc_resolve/src/diagnostics.rs b/compiler/rustc_resolve/src/diagnostics.rs index 2251539738c46..13cf700fbb207 100644 --- a/compiler/rustc_resolve/src/diagnostics.rs +++ b/compiler/rustc_resolve/src/diagnostics.rs @@ -2660,8 +2660,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { } let binding_key = BindingKey::new(ident, MacroNS); - let binding = - self.resolutions(crate_module).borrow().get(&binding_key)?.borrow().binding()?; + let binding = self.resolution(crate_module, binding_key)?.binding()?; let Res::Def(DefKind::Macro(MacroKind::Bang), _) = binding.res() else { return None; }; diff --git a/compiler/rustc_resolve/src/ident.rs b/compiler/rustc_resolve/src/ident.rs index 71cc68af499db..f5bc46bf05304 100644 --- a/compiler/rustc_resolve/src/ident.rs +++ b/compiler/rustc_resolve/src/ident.rs @@ -848,8 +848,13 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { }; let key = BindingKey::new(ident, ns); - let resolution = - self.resolution(module, key).try_borrow_mut().map_err(|_| (Determined, Weak::No))?; // This happens when there is a cycle of imports. + // `try_borrow_mut` is required to ensure exclusive access, even if the resulting binding + // doesn't need to be mutable. It will fail when there is a cycle of imports, and without + // the exclusive access infinite recursion will crash the compiler with stack overflow. + let resolution = &*self + .resolution_or_default(module, key) + .try_borrow_mut() + .map_err(|_| (Determined, Weak::No))?; // If the primary binding is unusable, search further and return the shadowed glob // binding if it exists. What we really want here is having two separate scopes in diff --git a/compiler/rustc_resolve/src/imports.rs b/compiler/rustc_resolve/src/imports.rs index ee82afd967689..f2de57b4d50ff 100644 --- a/compiler/rustc_resolve/src/imports.rs +++ b/compiler/rustc_resolve/src/imports.rs @@ -469,7 +469,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // Ensure that `resolution` isn't borrowed when defining in the module's glob importers, // during which the resolution might end up getting re-defined via a glob cycle. let (binding, t, warn_ambiguity) = { - let resolution = &mut *self.resolution(module, key).borrow_mut(); + let resolution = &mut *self.resolution_or_default(module, key).borrow_mut(); let old_binding = resolution.binding(); let t = f(self, resolution); @@ -1514,8 +1514,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { let imported_binding = self.import(binding, import); let warn_ambiguity = self .resolution(import.parent_scope.module, key) - .borrow() - .binding() + .and_then(|r| r.binding()) .is_some_and(|binding| binding.warn_ambiguity_recursive()); let _ = self.try_define( import.parent_scope.module, diff --git a/compiler/rustc_resolve/src/late.rs b/compiler/rustc_resolve/src/late.rs index a3a770502ded1..ed9622a0d81cc 100644 --- a/compiler/rustc_resolve/src/late.rs +++ b/compiler/rustc_resolve/src/late.rs @@ -3449,8 +3449,7 @@ impl<'a, 'ast, 'ra, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> { }; ident.span.normalize_to_macros_2_0_and_adjust(module.expansion); let key = BindingKey::new(ident, ns); - let mut binding = - self.r.resolution(module, key).try_borrow().ok().and_then(|r| r.best_binding()); + let mut binding = self.r.resolution(module, key).and_then(|r| r.best_binding()); debug!(?binding); if binding.is_none() { // We could not find the trait item in the correct namespace. @@ -3461,8 +3460,7 @@ impl<'a, 'ast, 'ra, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> { _ => ns, }; let key = BindingKey::new(ident, ns); - binding = - self.r.resolution(module, key).try_borrow().ok().and_then(|r| r.best_binding()); + binding = self.r.resolution(module, key).and_then(|r| r.best_binding()); debug!(?binding); } diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index 27e14e0e62bf0..fe7ed07c46e4e 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -21,7 +21,7 @@ #![recursion_limit = "256"] // tidy-alphabetical-end -use std::cell::{Cell, RefCell}; +use std::cell::{Cell, Ref, RefCell}; use std::collections::BTreeSet; use std::fmt; use std::sync::Arc; @@ -1905,9 +1905,16 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { &mut self, module: Module<'ra>, key: BindingKey, + ) -> Option>> { + self.resolutions(module).borrow().get(&key).map(|resolution| resolution.borrow()) + } + + fn resolution_or_default( + &mut self, + module: Module<'ra>, + key: BindingKey, ) -> &'ra RefCell> { - *self - .resolutions(module) + self.resolutions(module) .borrow_mut() .entry(key) .or_insert_with(|| self.arenas.alloc_name_resolution())