Auto merge of #158035 - LorrensP-2158466:import-cycle-det, r=petrochenkov

resolve: Explicit Set for detecting resolution cycles



Instead of using the `borrow_mut` counter of a `RefCell` for a `NameResolution` for detecting cyclic imports during import resolution, we use an explicit recursion stack that keeps track of the current used `NameResolution`s.

Because of the upcoming parallelisation of the import resolution algorithm, the current way cannot used in a parallel context.


r? @petrochenkov

Author: bors

compiler/rustc_resolve/src/ident.rs

@@ -16,7 +16,7 @@ use tracing::{debug, instrument};
 
 use crate::diagnostics::{ParamKindInEnumDiscriminant, ParamKindInNonTrivialAnonConst};
 use crate::hygiene::Macros20NormalizedSyntaxContext;
-use crate::imports::{Import, NameResolution};
+use crate::imports::{Import, NameResolution, cycle_detection};
 use crate::late::{
     ConstantHasGenerics, DiagMetadata, NoConstantGenericsReason, PathSource, Rib, RibKind,
 };
@@ -1148,13 +1148,8 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         ignore_import: Option<Import<'ra>>,
     ) -> Result<Decl<'ra>, ControlFlow<Determinacy, Determinacy>> {
         let key = BindingKey::new(ident, ns);
-        // `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.to_module(), key, orig_ident_span)
-            .try_borrow_mut_unchecked()
-            .map_err(|_| ControlFlow::Continue(Determined))?;
+        let resolution_ref = self.resolution_or_default(module.to_module(), key, orig_ident_span);
+        let resolution = resolution_ref.borrow();
 
         let binding = resolution.non_glob_decl.filter(|b| Some(*b) != ignore_decl);
 
@@ -1175,6 +1170,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             return if accessible { Ok(binding) } else { Err(ControlFlow::Break(Determined)) };
         }
 
+        // We need to detect resolution cycles to avoid infinite recursion. The guard ensures
+        // the resolution is removed when this resolve call ends.
+        let _cycle_guard = cycle_detection::enter_cycle_detector(resolution_ref)
+            .map_err(|_| ControlFlow::Continue(Determined))?;
+
         // Check if one of single imports can still define the name, block if it can.
         if self.reborrow().single_import_can_define_name(
             &resolution,
@@ -1210,13 +1210,8 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         ignore_import: Option<Import<'ra>>,
     ) -> Result<Decl<'ra>, ControlFlow<Determinacy, Determinacy>> {
         let key = BindingKey::new(ident, ns);
-        // `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.to_module(), key, orig_ident_span)
-            .try_borrow_mut_unchecked()
-            .map_err(|_| ControlFlow::Continue(Determined))?;
+        let resolution_ref = self.resolution_or_default(module.to_module(), key, orig_ident_span);
+        let resolution = resolution_ref.borrow();
 
         let binding = resolution.glob_decl.filter(|b| Some(*b) != ignore_decl);
 
@@ -1231,6 +1226,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             );
         }
 
+        // We need to detect resolution cycles to avoid infinite recursion. The guard ensures
+        // the resolution is removed when this resolve call ends.
+        let _cycle_guard = cycle_detection::enter_cycle_detector(resolution_ref)
+            .map_err(|_| ControlFlow::Continue(Determined))?;
+
         // Check if one of single imports can still define the name,
         // if it can then our result is not determined and can be invalidated.
         if self.reborrow().single_import_can_define_name(

compiler/rustc_resolve/src/imports.rs

@@ -32,7 +32,7 @@ use crate::diagnostics::{
     ConsiderMarkingAsPubCrate,
 };
 use crate::error_helper::{OnUnknownData, Suggestion};
-use crate::ref_mut::CmCell;
+use crate::ref_mut::{CmCell, CmRefCell};
 use crate::{
     AmbiguityError, BindingKey, CmResolver, Decl, DeclData, DeclKind, Determinacy, Finalize,
     IdentKey, ImportSuggestion, ImportSummary, LocalModule, ModuleOrUniformRoot, ParentScope,
@@ -292,6 +292,8 @@ pub(crate) struct NameResolution<'ra> {
     pub orig_ident_span: Span,
 }
 
+pub(crate) type NameResolutionRef<'ra> = Interned<'ra, CmRefCell<NameResolution<'ra>>>;
+
 impl<'ra> NameResolution<'ra> {
     pub(crate) fn new(orig_ident_span: Span) -> Self {
         NameResolution { single_imports: FxIndexSet::default(), orig_ident_span, .. }
@@ -320,6 +322,57 @@ impl<'ra> NameResolution<'ra> {
     }
 }
 
+// module to keep the TLS private and only accessible through the function `enter_cycle_detector`.
+pub(crate) mod cycle_detection {
+    use std::ptr;
+
+    use crate::CacheRefCell;
+    use crate::imports::NameResolutionRef;
+
+    thread_local!(
+        /// During import resolution, recursive imports can form cycles.
+        /// This set stores the active resolution stack for the current thread.
+        /// So it's essentially a recursion stack.
+        ///
+        /// The key is the interned address of a `RefCell<NameResolution<'ra>>` allocated
+        /// in the `Resolver Arenas` (lifetime `'ra`), it is thus stable and allows casting
+        /// to a `*const ()` for comparison. This is done because we can't use lifetimes
+        /// other than `'static` in thread local storage.
+        static ACTIVE_RESOLUTIONS: CacheRefCell<Vec<*const ()>> = Default::default();
+    );
+
+    pub(crate) struct ActiveResolutionGuard {
+        key: *const (),
+    }
+
+    impl Drop for ActiveResolutionGuard {
+        fn drop(&mut self) {
+            ACTIVE_RESOLUTIONS.with_borrow_mut(|ar| {
+                // Only this guard is allowed to remove this key.
+                assert!(
+                    Some(self.key) == ar.pop(),
+                    "This guard should be the only one removing this key"
+                );
+            });
+        }
+    }
+
+    /// Returns `Err(())` if a cycle is detected, otherwise this returns a
+    /// guard that will remove the resolution when dropped.
+    pub(crate) fn enter_cycle_detector<'ra>(
+        resolution: NameResolutionRef<'ra>,
+    ) -> Result<ActiveResolutionGuard, ()> {
+        let key = ptr::from_ref(resolution.0).cast::<()>();
+        ACTIVE_RESOLUTIONS.with_borrow_mut(|ar| {
+            if ar.contains(&key) {
+                return Err(());
+            }
+            ar.push(key);
+            Ok(ActiveResolutionGuard { key })
+        })
+    }
+}
+
 /// An error that may be transformed into a diagnostic later. Used to combine multiple unresolved
 /// import errors within the same use tree into a single diagnostic.
 #[derive(Debug, Clone)]
@@ -640,6 +693,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         let (binding, t) = {
             let resolution = &mut *self
                 .resolution_or_default(module.to_module(), key, orig_ident_span)
+                .0
                 .borrow_mut(self);
             let old_decl = resolution.determined_decl();
             let old_vis = old_decl.map(|d| d.vis());

compiler/rustc_resolve/src/lib.rs

@@ -77,6 +77,7 @@ use smallvec::{SmallVec, smallvec};
 use tracing::{debug, instrument};
 
 use crate::error_helper::OnUnknownData;
+use crate::imports::NameResolutionRef;
 use crate::ref_mut::{CmCell, CmRefCell};
 
 mod build_reduced_graph;
@@ -639,7 +640,7 @@ impl BindingKey {
     }
 }
 
-type Resolutions<'ra> = CmRefCell<FxIndexMap<BindingKey, &'ra CmRefCell<NameResolution<'ra>>>>;
+type Resolutions<'ra> = CmRefCell<FxIndexMap<BindingKey, NameResolutionRef<'ra>>>;
 
 /// One node in the tree of modules.
 ///
@@ -1597,11 +1598,10 @@ impl<'ra> ResolverArenas<'ra> {
     fn alloc_import(&'ra self, import: ImportData<'ra>) -> Import<'ra> {
         Interned::new_unchecked(self.imports.alloc(import))
     }
-    fn alloc_name_resolution(
-        &'ra self,
-        orig_ident_span: Span,
-    ) -> &'ra CmRefCell<NameResolution<'ra>> {
-        self.name_resolutions.alloc(CmRefCell::new(NameResolution::new(orig_ident_span)))
+    fn alloc_name_resolution(&'ra self, orig_ident_span: Span) -> NameResolutionRef<'ra> {
+        Interned::new_unchecked(
+            self.name_resolutions.alloc(CmRefCell::new(NameResolution::new(orig_ident_span))),
+        )
     }
     fn alloc_macro_rules_scope(&'ra self, scope: MacroRulesScope<'ra>) -> MacroRulesScopeRef<'ra> {
         self.dropless.alloc(CacheCell::new(scope))
@@ -2213,16 +2213,17 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         module: Module<'ra>,
         key: BindingKey,
     ) -> Option<Ref<'ra, NameResolution<'ra>>> {
-        self.resolutions(module).borrow().get(&key).map(|resolution| resolution.borrow())
+        self.resolutions(module).borrow().get(&key).map(|resolution| resolution.0.borrow())
     }
 
     fn resolution_or_default(
         &self,
         module: Module<'ra>,
         key: BindingKey,
         orig_ident_span: Span,
-    ) -> &'ra CmRefCell<NameResolution<'ra>> {
-        self.resolutions(module)
+    ) -> NameResolutionRef<'ra> {
+        *self
+            .resolutions(module)
             .borrow_mut_unchecked()
             .entry(key)
             .or_insert_with(|| self.arenas.alloc_name_resolution(orig_ident_span))
@@ -2833,8 +2834,6 @@ type CmResolver<'r, 'ra, 'tcx> = ref_mut::RefOrMut<'r, Resolver<'ra, 'tcx>>;
 // parallel name resolution.
 use std::cell::{Cell as CacheCell, RefCell as CacheRefCell};
 
-// FIXME: `*_unchecked` methods in the module below should be eliminated in the process
-// of migration to parallel name resolution.
 mod ref_mut {
     use std::cell::{BorrowMutError, Cell, Ref, RefCell, RefMut};
     use std::fmt;
@@ -2942,6 +2941,8 @@ mod ref_mut {
         }
 
         #[track_caller]
+        // FIXME: this should be eliminated in the process of migration
+        // to parallel name resolution.
         pub(crate) fn borrow_mut_unchecked(&self) -> RefMut<'_, T> {
             self.0.borrow_mut()
         }
@@ -2954,10 +2955,6 @@ mod ref_mut {
             self.0.borrow_mut()
         }
 
-        pub(crate) fn try_borrow_mut_unchecked(&self) -> Result<RefMut<'_, T>, BorrowMutError> {
-            self.0.try_borrow_mut()
-        }
-
         #[track_caller]
         pub(crate) fn try_borrow_mut<'ra, 'tcx>(
             &self,