diff --git a/compiler/rustc_data_structures/src/sync.rs b/compiler/rustc_data_structures/src/sync.rs index 85e030c8aa0c5..6b5526c626612 100644 --- a/compiler/rustc_data_structures/src/sync.rs +++ b/compiler/rustc_data_structures/src/sync.rs @@ -25,22 +25,18 @@ use std::collections::HashMap; use std::hash::{BuildHasher, Hash}; -pub use parking_lot::{ - MappedRwLockReadGuard as MappedReadGuard, MappedRwLockWriteGuard as MappedWriteGuard, - RwLockReadGuard as ReadGuard, RwLockWriteGuard as WriteGuard, -}; - pub use self::atomic::AtomicU64; pub use self::freeze::{FreezeLock, FreezeReadGuard, FreezeWriteGuard}; #[doc(no_inline)] -pub use self::lock::{Lock, LockGuard, Mode}; +pub use self::lock::{Lock, LockGuard}; pub use self::mode::{ - FromDyn, check_dyn_thread_safe, is_dyn_thread_safe, set_dyn_thread_safe_mode, + FromDyn, Mode, check_dyn_thread_safe, is_dyn_thread_safe, set_dyn_thread_safe_mode, }; pub use self::parallel::{ broadcast, par_fns, par_for_each_in, par_for_each_slice, par_join, par_map, parallel_guard, spawn, try_par_for_each_in, }; +pub use self::rw_lock::{MappedReadGuard, MappedWriteGuard, ReadGuard, RwLock, WriteGuard}; pub use self::vec::{AppendOnlyIndexVec, AppendOnlyVec}; pub use self::worker_local::{Registry, WorkerLocal}; pub use crate::marker::*; @@ -48,6 +44,7 @@ pub use crate::marker::*; mod freeze; mod lock; mod parallel; +mod rw_lock; mod vec; mod worker_local; @@ -68,6 +65,12 @@ mod mode { use crate::sync::{DynSend, DynSync}; + #[derive(Clone, Copy, PartialEq)] + pub enum Mode { + NoSync, + Sync, + } + const UNINITIALIZED: u8 = 0; const DYN_NOT_THREAD_SAFE: u8 = 1; const DYN_THREAD_SAFE: u8 = 2; @@ -149,10 +152,6 @@ mod mode { } } -/// This makes locks panic if they are already held. -/// It is only useful when you are running in a single thread -const ERROR_CHECKING: bool = false; - #[derive(Default)] #[repr(align(64))] pub struct CacheAligned(pub T); @@ -168,58 +167,3 @@ impl HashMapExt for HashMap self.entry(key).and_modify(|old| assert!(*old == value)).or_insert(value); } } - -#[derive(Debug, Default)] -pub struct RwLock(parking_lot::RwLock); - -impl RwLock { - #[inline(always)] - pub fn new(inner: T) -> Self { - RwLock(parking_lot::RwLock::new(inner)) - } - - #[inline(always)] - pub fn into_inner(self) -> T { - self.0.into_inner() - } - - #[inline(always)] - pub fn get_mut(&mut self) -> &mut T { - self.0.get_mut() - } - - #[inline(always)] - pub fn read(&self) -> ReadGuard<'_, T> { - if ERROR_CHECKING { - self.0.try_read().expect("lock was already held") - } else { - self.0.read() - } - } - - #[inline(always)] - pub fn try_write(&self) -> Result, ()> { - self.0.try_write().ok_or(()) - } - - #[inline(always)] - pub fn write(&self) -> WriteGuard<'_, T> { - if ERROR_CHECKING { - self.0.try_write().expect("lock was already held") - } else { - self.0.write() - } - } - - #[inline(always)] - #[track_caller] - pub fn borrow(&self) -> ReadGuard<'_, T> { - self.read() - } - - #[inline(always)] - #[track_caller] - pub fn borrow_mut(&self) -> WriteGuard<'_, T> { - self.write() - } -} diff --git a/compiler/rustc_data_structures/src/sync/lock.rs b/compiler/rustc_data_structures/src/sync/lock.rs index f183af0c0dabd..eac0c931cb7ba 100644 --- a/compiler/rustc_data_structures/src/sync/lock.rs +++ b/compiler/rustc_data_structures/src/sync/lock.rs @@ -1,19 +1,13 @@ //! This module implements a lock which only uses synchronization if `might_be_dyn_thread_safe` is true. //! It implements `DynSend` and `DynSync` instead of the typical `Send` and `Sync` traits. -use std::{fmt, hint}; - -#[derive(Clone, Copy, PartialEq)] -pub enum Mode { - NoSync, - Sync, -} - use std::cell::{Cell, UnsafeCell}; use std::marker::PhantomData; use std::mem::ManuallyDrop; use std::ops::{Deref, DerefMut}; +use std::{fmt, hint}; +use mode::Mode; use parking_lot::RawMutex; use parking_lot::lock_api::RawMutex as _; diff --git a/compiler/rustc_data_structures/src/sync/parallel.rs b/compiler/rustc_data_structures/src/sync/parallel.rs index f8ddde4298c40..1442baa337b37 100644 --- a/compiler/rustc_data_structures/src/sync/parallel.rs +++ b/compiler/rustc_data_structures/src/sync/parallel.rs @@ -185,12 +185,18 @@ pub fn par_for_each_in>( }); } -// FIXME: actually make parallel and `T: DynSend` -pub fn par_for_each_slice(items: &mut [T], for_each: impl Fn(&mut T)) { +pub fn par_for_each_slice( + items: &mut [T], + for_each: impl Fn(&mut T) + DynSync + DynSend, +) { parallel_guard(|guard| { - items.iter_mut().for_each(|i| { - guard.run(|| for_each(i)); - }); + if let Some(proof) = mode::check_dyn_thread_safe() { + par_slice(items, guard, |i| for_each(i), proof) + } else { + items.iter_mut().for_each(|i| { + guard.run(|| for_each(i)); + }); + } }); } diff --git a/compiler/rustc_data_structures/src/sync/rw_lock.rs b/compiler/rustc_data_structures/src/sync/rw_lock.rs new file mode 100644 index 0000000000000..3502b878b93f1 --- /dev/null +++ b/compiler/rustc_data_structures/src/sync/rw_lock.rs @@ -0,0 +1,402 @@ +use core::fmt; +use std::cell::{Cell, UnsafeCell}; +use std::marker::PhantomData; +use std::mem::ManuallyDrop; +use std::ops::{Deref, DerefMut}; +use std::{hint, mem}; + +use parking_lot::RawRwLock; +use parking_lot::lock_api::RawRwLock as _; + +use crate::sync::mode::{self, Mode}; + +// Mimic `RefCell` borrow counting +type BorrowCounter = isize; +const UNUSED: BorrowCounter = 0; + +#[inline(always)] +const fn is_writing(x: BorrowCounter) -> bool { + x < UNUSED +} + +#[inline(always)] +const fn is_reading(x: BorrowCounter) -> bool { + x > UNUSED +} + +/// A guard holding shared access to a `RwLock` which is in a read-locked state. +#[must_use = "if unused the Lock will immediately unlock"] +pub struct ReadGuard<'a, T> { + rw_lock: &'a RwLock, + marker: PhantomData<&'a T>, + + /// The synchronization mode of the lock. This is explicitly passed to let LLVM relate it + /// to the original lock operation. + mode: Mode, +} + +/// # SAFETY +/// +/// - union_access: `mode` and `mode_union` must be in the same state +/// - unlock: `mode_union` must be in a read-locked state +#[inline(always)] +unsafe fn drop_read_lock(mode: Mode, mode_union: &ModeUnion) { + match mode { + Mode::NoSync => { + let borrow = unsafe { &mode_union.no_sync }; + debug_assert!(is_reading(borrow.get())); + borrow.update(|b| b - 1); + } + Mode::Sync => unsafe { mode_union.sync.unlock_shared() }, + } +} + +impl<'a, T: 'a> Drop for ReadGuard<'a, T> { + fn drop(&mut self) { + // SAFETY (union access): We get `self.mode` from the lock operation so it is consistent + // with the `lock.mode` state. This means we access the right union fields. + // SAFETY (unlock): We know that the rwlock is read-locked as this type is a proof of that. + unsafe { + drop_read_lock(self.mode, &self.rw_lock.mode_union); + } + } +} + +impl<'a, T: 'a> Deref for ReadGuard<'a, T> { + type Target = T; + #[inline] + fn deref(&self) -> &T { + // SAFETY: We have shared access to the shared access of this type, + // so we can give out a shared reference. + unsafe { &*self.rw_lock.data.get() } + } +} + +#[must_use = "if unused the RwLock will immediately unlock"] +pub struct MappedReadGuard<'a, T> { + mode_union: &'a ModeUnion, + data: *const T, + marker: PhantomData<&'a T>, + + /// The synchronization mode of the lock. This is explicitly passed to let LLVM relate it + /// to the original lock operation. + mode: Mode, +} + +impl<'a, T: 'a> Drop for MappedReadGuard<'a, T> { + fn drop(&mut self) { + // SAFETY (union access): We get `self.mode` from the lock operation so it is consistent + // with the `lock.mode` state. This means we access the right union fields. + // SAFETY (unlock): We know that the rwlock is read-locked as this type is a proof of that. + unsafe { + drop_read_lock(self.mode, self.mode_union); + } + } +} + +impl<'a, T: 'a> Deref for MappedReadGuard<'a, T> { + type Target = T; + #[inline] + fn deref(&self) -> &T { + // SAFETY: We have shared access to the shared access of this type, + // so we can give out a shared reference. + unsafe { &*self.data } + } +} + +impl<'a, T: 'a> ReadGuard<'a, T> { + pub fn map(s: Self, f: F) -> MappedReadGuard<'a, U> + where + F: FnOnce(&T) -> &U, + { + let mode = s.mode; + let mode_union = &s.rw_lock.mode_union; + let data = f(unsafe { &*s.rw_lock.data.get() }); + mem::forget(s); + MappedReadGuard { mode_union, data, marker: PhantomData, mode } + } +} + +/// A guard holding exclusive access to a `RwLock` which is in a write-locked state. +#[must_use = "if unused the Lock will immediately unlock"] +pub struct WriteGuard<'a, T> { + rw_lock: &'a RwLock, + marker: PhantomData<&'a mut T>, + + /// The synchronization mode of the lock. This is explicitly passed to let LLVM relate it + /// to the original lock operation. + mode: Mode, +} + +/// # SAFETY +/// +/// - union_access: `mode` and `mode_union` must be in the same state +/// - unlock: `mode_union` must be in a write-locked state +#[inline(always)] +unsafe fn drop_write_lock(mode: Mode, mode_union: &ModeUnion) { + // SAFETY: Caller guarantees union_access. + match mode { + Mode::NoSync => { + let borrow = unsafe { &mode_union.no_sync }; + debug_assert!(is_writing(borrow.get())); + borrow.update(|b| b + 1); + } + // SAFETY: Caller guarantees write-locked state. + Mode::Sync => unsafe { mode_union.sync.unlock_exclusive() }, + } +} + +impl<'a, T: 'a> Drop for WriteGuard<'a, T> { + fn drop(&mut self) { + // SAFETY (union access): We get `mode` from the lock operation so it is consistent + // with the `lock.mode` state. This means we access the right union fields. + // SAFETY (unlock): We know that the rwlock is write-locked as this type is a proof of that. + unsafe { drop_write_lock(self.mode, &self.rw_lock.mode_union) }; + } +} + +impl<'a, T: 'a> Deref for WriteGuard<'a, T> { + type Target = T; + #[inline] + fn deref(&self) -> &T { + // SAFETY: We have shared access to the shared access of this type, + // so we can give out a shared reference. + unsafe { &*self.rw_lock.data.get() } + } +} + +impl<'a, T: 'a> DerefMut for WriteGuard<'a, T> { + #[inline] + fn deref_mut(&mut self) -> &mut T { + // SAFETY: We have exclusive access to the exclusive access of this type, + // so we can give out a exclusive reference. + unsafe { &mut *self.rw_lock.data.get() } + } +} + +#[must_use = "if unused the RwLock will immediately unlock"] +pub struct MappedWriteGuard<'a, T> { + mode_union: &'a ModeUnion, + data: *mut T, + marker: PhantomData<&'a mut T>, + + /// The synchronization mode of the lock. This is explicitly passed to let LLVM relate it + /// to the original lock operation. + mode: Mode, +} + +impl<'a, T: 'a> Drop for MappedWriteGuard<'a, T> { + fn drop(&mut self) { + // SAFETY (union access): We get `self.mode` from the lock operation so it is consistent + // with the `lock.mode` state. This means we access the right union fields. + // SAFETY (unlock): We know that the rwlock is write-locked as this type is a proof of that. + unsafe { + drop_write_lock(self.mode, &self.mode_union); + } + } +} + +impl<'a, T: 'a> Deref for MappedWriteGuard<'a, T> { + type Target = T; + #[inline] + fn deref(&self) -> &T { + // SAFETY: We have shared access to the exclusive access of this type, + // so we can give out a shared reference. + unsafe { &*self.data } + } +} + +impl<'a, T: 'a> DerefMut for MappedWriteGuard<'a, T> { + #[inline] + fn deref_mut(&mut self) -> &mut T { + // SAFETY: We have exclusive access to the exclusive access of this type, + // so we can give out a exclusive reference. + unsafe { &mut *self.data } + } +} + +impl<'a, T: 'a> WriteGuard<'a, T> { + pub fn map(s: Self, f: F) -> MappedWriteGuard<'a, U> + where + F: FnOnce(&mut T) -> &mut U, + { + let mode = s.mode; + let mode_union = &s.rw_lock.mode_union; + // SAFETY: We have owned access to the exclusive access of this type, + // so we can give out a exclusive reference. + let data = f(unsafe { &mut *s.rw_lock.data.get() }); + mem::forget(s); + MappedWriteGuard { mode_union, data, marker: PhantomData, mode } + } +} + +union ModeUnion { + /// Mimics the borrow counting of `RefCell` that is only used if `RwLock.mode` is `NoSync`. + no_sync: ManuallyDrop>, + + /// A RwLock of parking_lot that is only used if `RwLock.mode` is `Sync` + sync: ManuallyDrop, +} + +pub struct RwLock { + mode: Mode, + + mode_union: ModeUnion, + data: UnsafeCell, +} + +/// This makes locks panic if they are already held. +/// It is only useful when you are running in a single thread +const ERROR_CHECKING: bool = false; + +impl RwLock { + #[inline(always)] + pub fn new(inner: T) -> Self { + let (mode, mode_union) = if mode::might_be_dyn_thread_safe() { + hint::cold_path(); + // Create the lock with synchronization enabled using the `RawRwLock` type. + (Mode::Sync, ModeUnion { sync: ManuallyDrop::new(RawRwLock::INIT) }) + } else { + // Create the lock with synchronization disabled. + (Mode::NoSync, ModeUnion { no_sync: ManuallyDrop::new(Cell::new(UNUSED)) }) + }; + RwLock { mode, mode_union, data: UnsafeCell::new(inner) } + } + + #[inline(always)] + pub fn into_inner(self) -> T { + self.data.into_inner() + } + + #[inline(always)] + pub fn get_mut(&mut self) -> &mut T { + self.data.get_mut() + } + + #[inline(always)] + pub fn read(&self) -> ReadGuard<'_, T> { + if ERROR_CHECKING { + self.try_read().expect("lock was already held") + } else { + let mode = self.mode; + + // SAFETY: This is safe since the union fields are used in accordance with `self.mode`. + match mode { + Mode::NoSync => { + let borrow = unsafe { &self.mode_union.no_sync }; + if is_writing(borrow.get()) { + panic!("RwLock already mutably borrowed"); + } + borrow.update(|b| b + 1); + } + Mode::Sync => unsafe { self.mode_union.sync.lock_shared() }, + } + ReadGuard { rw_lock: self, marker: PhantomData, mode } + } + } + + #[inline(always)] + pub fn try_read(&self) -> Result, ()> { + let mode = self.mode; + + // SAFETY: This is safe since the union fields are used in accordance with `self.mode`. + match mode { + Mode::NoSync => { + let borrow = unsafe { &self.mode_union.no_sync }; + let is_reading = !is_writing(borrow.get()); + if is_reading { + borrow.update(|b| b + 1); + } + is_reading + } + Mode::Sync => unsafe { self.mode_union.sync.try_lock_shared() }, + } + .then(|| ReadGuard { rw_lock: self, marker: PhantomData, mode }) + .ok_or(()) + } + + #[inline(always)] + pub fn write(&self) -> WriteGuard<'_, T> { + if ERROR_CHECKING { + self.try_write().expect("lock was already held") + } else { + let mode = self.mode; + + // SAFETY: This is safe since the union fields are used in accordance with `self.mode`. + match mode { + Mode::NoSync => { + let borrow = unsafe { &self.mode_union.no_sync }; + if borrow.get() != UNUSED { + panic!("lock was already held") + } + borrow.replace(UNUSED - 1); + } + Mode::Sync => unsafe { self.mode_union.sync.lock_exclusive() }, + } + WriteGuard { rw_lock: self, marker: PhantomData, mode } + } + } + + #[inline(always)] + pub fn try_write(&self) -> Result, ()> { + let mode = self.mode; + + // SAFETY: This is safe since the union fields are used in accordance with `self.mode`. + match mode { + Mode::NoSync => { + let borrow = unsafe { &self.mode_union.no_sync }; + let unused = borrow.get() == UNUSED; + if unused { + borrow.replace(UNUSED - 1); + } + unused + } + Mode::Sync => unsafe { self.mode_union.sync.try_lock_exclusive() }, + } + .then(|| WriteGuard { rw_lock: self, marker: PhantomData, mode }) + .ok_or(()) + } + + #[inline(always)] + #[track_caller] + pub fn borrow(&self) -> ReadGuard<'_, T> { + self.read() + } + + #[inline(always)] + #[track_caller] + pub fn borrow_mut(&self) -> WriteGuard<'_, T> { + self.write() + } +} + +impl Default for RwLock { + fn default() -> Self { + RwLock::new(T::default()) + } +} + +impl fmt::Debug for RwLock { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let mut debug = f.debug_struct("RwLock"); + match self.try_read() { + Ok(guard) => debug.field("data", &&*guard).finish(), + Err(()) => { + // Additional format_args! here is to remove quotes around in debug output. + debug.field("data", &format_args!("")).finish() + } + } + } +} + +impl<'a, T: fmt::Debug + 'a> fmt::Debug for MappedWriteGuard<'a, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Debug::fmt(&**self, f) + } +} + +impl<'a, T: fmt::Debug + 'a> fmt::Debug for MappedReadGuard<'a, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Debug::fmt(&**self, f) + } +} diff --git a/compiler/rustc_data_structures/src/sync/vec.rs b/compiler/rustc_data_structures/src/sync/vec.rs index d35e2c61f1e19..b3ba7ecc5ea65 100644 --- a/compiler/rustc_data_structures/src/sync/vec.rs +++ b/compiler/rustc_data_structures/src/sync/vec.rs @@ -2,6 +2,8 @@ use std::marker::PhantomData; use rustc_index::Idx; +use crate::sync::RwLock; + #[derive(Default)] pub struct AppendOnlyIndexVec { vec: elsa::sync::LockFreeFrozenVec, @@ -26,7 +28,7 @@ impl AppendOnlyIndexVec { #[derive(Default)] pub struct AppendOnlyVec { - vec: parking_lot::RwLock>, + vec: RwLock>, } impl AppendOnlyVec { diff --git a/compiler/rustc_resolve/src/build_reduced_graph.rs b/compiler/rustc_resolve/src/build_reduced_graph.rs index 8f8b2807522e0..4ad1fc9985201 100644 --- a/compiler/rustc_resolve/src/build_reduced_graph.rs +++ b/compiler/rustc_resolve/src/build_reduced_graph.rs @@ -5,7 +5,6 @@ //! unexpanded macros in the fragment are visited and registered. //! Imports are also considered items and placed into modules here, but not resolved yet. -use std::cell::RefMut; use std::sync::Arc; use rustc_ast::visit::{self, AssocCtxt, Visitor, WalkItemKind}; @@ -16,6 +15,7 @@ use rustc_ast::{ }; use rustc_attr_parsing::AttributeParser; use rustc_data_structures::fx::FxIndexMap; +use rustc_data_structures::sync::WriteGuard; use rustc_expand::base::{ResolverExpand, SyntaxExtension, SyntaxExtensionKind}; use rustc_hir::Attribute; use rustc_hir::attrs::{AttributeKind, MacroUseArgs}; @@ -134,7 +134,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { fn get_extern_module_with_lock( &self, def_id: DefId, - map_lock: &mut RefMut<'_, FxIndexMap>>, + map_lock: &mut WriteGuard<'_, FxIndexMap>>, ) -> Option> { if let module @ Some(..) = map_lock.get(&def_id) { return module.copied(); diff --git a/compiler/rustc_resolve/src/check_unused.rs b/compiler/rustc_resolve/src/check_unused.rs index 4c5c591ad94e3..ba66c864c0a55 100644 --- a/compiler/rustc_resolve/src/check_unused.rs +++ b/compiler/rustc_resolve/src/check_unused.rs @@ -558,8 +558,8 @@ impl Resolver<'_, '_> { let unused_imports = visitor.unused_imports; let mut check_redundant_imports = FxIndexSet::default(); for module in &self.local_modules { - for (_key, resolution) in self.resolutions(module.to_module()).borrow().iter() { - if let Some(decl) = resolution.borrow().best_decl() + for (_key, resolution) in self.resolutions(module.to_module()).borrow(self).iter() { + if let Some(decl) = resolution.borrow(self).best_decl() && let DeclKind::Import { import, .. } = decl.kind && let ImportKind::Single { id, .. } = import.kind { diff --git a/compiler/rustc_resolve/src/diagnostics/impls.rs b/compiler/rustc_resolve/src/diagnostics/impls.rs index 0b6b9899f48a6..7de64b1289470 100644 --- a/compiler/rustc_resolve/src/diagnostics/impls.rs +++ b/compiler/rustc_resolve/src/diagnostics/impls.rs @@ -1483,7 +1483,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // Never recommend deprecated helper attributes. } Scope::MacroRules(macro_rules_scope) => { - if let MacroRulesScope::Def(macro_rules_def) = macro_rules_scope.get() { + if let MacroRulesScope::Def(macro_rules_def) = *macro_rules_scope.read() { let res = macro_rules_def.decl.res(); if filter_fn(res) { suggestions.push(TypoSuggestion::new( @@ -1870,11 +1870,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // If so, we have to disambiguate the potential import suggestions by making // the paths *global* (i.e., by prefixing them with `::`). let needs_disambiguation = - self.resolutions(parent_scope.module).borrow().iter().any( + self.resolutions(parent_scope.module).borrow(self).iter().any( |(key, name_resolution)| { if key.ns == TypeNS && key.ident == *ident - && let Some(decl) = name_resolution.borrow().best_decl() + && let Some(decl) = name_resolution.borrow(self).best_decl() { match decl.res() { // No disambiguation needed if the identically named item we @@ -3605,7 +3605,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { let mut res = false; let m = r.expect_module(parent_module); if m.is_local() { - for importer in m.glob_importers.borrow().iter() { + for importer in m.glob_importers.borrow(r).iter() { if let Some(next_parent_module) = importer.parent_scope.module.opt_def_id() { if next_parent_module == module diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs index 90927492195b0..cd6b1fffbd18f 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/compiler/rustc_resolve/src/effective_visibilities.rs @@ -125,8 +125,8 @@ 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()); - for (_, name_resolution) in self.r.resolutions(module).borrow().iter() { - let Some(decl) = name_resolution.borrow().best_decl() else { + for (_, name_resolution) in self.r.resolutions(module).borrow(self.r).iter() { + let Some(decl) = name_resolution.borrow(self.r).best_decl() else { continue; }; self.update_decl_chain(decl, ParentId::Def(module_id)); @@ -309,8 +309,8 @@ impl<'a, 'ra, 'tcx> EffectiveVisibilitiesVisitor<'a, 'ra, 'tcx> { ) { if self.macro_reachable.insert((module_def_id, defining_mod)) { let module = self.r.expect_module(module_def_id.to_def_id()); - for (_, name_resolution) in self.r.resolutions(module).borrow().iter() { - let Some(decl) = name_resolution.borrow().best_decl() else { + for (_, name_resolution) in self.r.resolutions(module).borrow(self.r).iter() { + let Some(decl) = name_resolution.borrow(self.r).best_decl() else { continue; }; diff --git a/compiler/rustc_resolve/src/ident.rs b/compiler/rustc_resolve/src/ident.rs index d18180ea64afc..145c32594035e 100644 --- a/compiler/rustc_resolve/src/ident.rs +++ b/compiler/rustc_resolve/src/ident.rs @@ -142,11 +142,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // used to avoid long scope chains, see the comments on `MacroRulesScopeRef`. // As another consequence of this optimization visitors never observe invocation // scopes for macros that were already expanded. - let mut scope = macro_rules_scope.get(); + let mut scope = *macro_rules_scope.borrow(); while let MacroRulesScope::Invocation(invoc_id) = scope { if let Some(next) = self.output_macro_rules_scopes.get(&invoc_id) { - scope = next.get(); - macro_rules_scope.set(scope); + scope = *next.borrow(); + *macro_rules_scope.borrow_mut() = scope; } else { break; } @@ -187,7 +187,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { } } Scope::DeriveHelpersCompat => Scope::MacroRules(parent_scope.macro_rules), - Scope::MacroRules(macro_rules_scope) => match macro_rules_scope.get() { + Scope::MacroRules(macro_rules_scope) => match *macro_rules_scope.read() { MacroRulesScope::Def(binding) => { Scope::MacroRules(binding.parent_macro_rules_scope) } @@ -592,7 +592,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { } result } - Scope::MacroRules(macro_rules_scope) => match macro_rules_scope.get() { + Scope::MacroRules(macro_rules_scope) => match *macro_rules_scope.read() { MacroRulesScope::Def(macro_rules_def) if ident == macro_rules_def.ident => { Ok(macro_rules_def.decl) } @@ -714,7 +714,9 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { } Scope::MacroUsePrelude => match self.macro_use_prelude.get(&ident.name).cloned() { Some(decl) => Ok(decl), - None => Err(Determinacy::determined(!self.graph_root.has_unexpanded_invocations())), + None => { + Err(Determinacy::determined(!self.graph_root.has_unexpanded_invocations(&self))) + } }, Scope::BuiltinAttrs => match self.builtin_attr_decls.get(&ident.name) { Some(decl) => Ok(*decl), @@ -727,9 +729,9 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { finalize.is_some(), ) { Some(decl) => Ok(decl), - None => { - Err(Determinacy::determined(!self.graph_root.has_unexpanded_invocations())) - } + None => Err(Determinacy::determined( + !self.graph_root.has_unexpanded_invocations(&self), + )), } } Scope::ExternPreludeFlags => { @@ -1158,7 +1160,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { if let Some(finalize) = finalize { // finalize implies that the module is fully expanded - assert!(!module.has_unexpanded_invocations()); + assert!(!module.has_unexpanded_invocations(&self)); return self.get_mut().finalize_module_binding( ident, orig_ident_span, @@ -1195,7 +1197,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { } // Check if one of unexpanded macros can still define the name. - if module.has_unexpanded_invocations() { + if module.has_unexpanded_invocations(&self) { return Err(ControlFlow::Continue(Undetermined)); } @@ -1224,7 +1226,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { if let Some(finalize) = finalize { // finalize implies that the module is fully expanded - assert!(!module.has_unexpanded_invocations()); + assert!(!module.has_unexpanded_invocations(&self)); return self.get_mut().finalize_module_binding( ident, orig_ident_span, @@ -1268,7 +1270,10 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // and prohibit access to macro-expanded `macro_export` macros instead (unless restricted // shadowing is enabled, see `macro_expanded_macro_export_errors`). if let Some(binding) = binding { - return if binding.determined() || ns == MacroNS || shadowing == Shadowing::Restricted { + return if binding.determined(&self) + || ns == MacroNS + || shadowing == Shadowing::Restricted + { let accessible = self.is_accessible_from(binding.vis(), parent_scope.module); if accessible { Ok(binding) } else { Err(ControlFlow::Break(Determined)) } } else { @@ -1283,13 +1288,13 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // scopes we return `Undetermined` with `ControlFlow::Continue`. // Check if one of unexpanded macros can still define the name, // if it can then our "no resolution" result is not determined and can be invalidated. - if module.has_unexpanded_invocations() { + if module.has_unexpanded_invocations(&self) { return Err(ControlFlow::Continue(Undetermined)); } // Check if one of glob imports can still define the name, // if it can then our "no resolution" result is not determined and can be invalidated. - for glob_import in module.globs.borrow().iter() { + for glob_import in module.globs.borrow(&self).iter() { if ignore_import == Some(*glob_import) { continue; } diff --git a/compiler/rustc_resolve/src/imports.rs b/compiler/rustc_resolve/src/imports.rs index 3f2e24995deab..adfae983792f3 100644 --- a/compiler/rustc_resolve/src/imports.rs +++ b/compiler/rustc_resolve/src/imports.rs @@ -1005,8 +1005,8 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { pub(crate) fn lint_reexports(&mut self, exported_ambiguities: FxHashSet>) { for module in &self.local_modules { - for (key, resolution) in self.resolutions(module.to_module()).borrow().iter() { - let resolution = resolution.borrow(); + for (key, resolution) in self.resolutions(module.to_module()).borrow(self).iter() { + let resolution = resolution.borrow(self); let Some(binding) = resolution.best_decl() else { continue }; // Report "cannot reexport" errors for exotic cases involving macros 2.0 @@ -1491,7 +1491,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { let names = match module { ModuleOrUniformRoot::Module(module) => { self.resolutions(module) - .borrow() + .borrow(self) .iter() .filter_map(|(BindingKey { ident: i, .. }, resolution)| { if i.name == ident.name { @@ -1501,7 +1501,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { return None; } // `use _` is never valid - let resolution = resolution.borrow(); + let resolution = resolution.borrow(self); if let Some(name_binding) = resolution.best_decl() { match name_binding.kind { DeclKind::Import { source_decl, .. } => { @@ -1809,10 +1809,10 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { let import_bindings = match imported_module { ModuleOrUniformRoot::Module(module) if module != import.parent_scope.module => self .resolutions(module) - .borrow() + .borrow(self) .iter() .filter_map(|(key, resolution)| { - let res = resolution.borrow(); + let res = resolution.borrow(self); let decl = res.determined_decl()?; let mut key = *key; let scope = match key.ident.ctxt.update_unchecked(|ctxt| { diff --git a/compiler/rustc_resolve/src/late/diagnostics.rs b/compiler/rustc_resolve/src/late/diagnostics.rs index 5260f724385f8..7dc45aa38cf88 100644 --- a/compiler/rustc_resolve/src/late/diagnostics.rs +++ b/compiler/rustc_resolve/src/late/diagnostics.rs @@ -188,11 +188,11 @@ impl<'ast, 'ra, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> { assoc_name: Symbol, ) -> Option { let module = self.r.get_module(trait_def_id)?; - self.r.resolutions(module).borrow().iter().find_map(|(key, resolution)| { + self.r.resolutions(module).borrow(self.r).iter().find_map(|(key, resolution)| { if key.ident.name != assoc_name { return None; } - let resolution = resolution.borrow(); + let resolution = resolution.borrow(self.r); let binding = resolution.best_decl()?; match binding.res() { Res::Def(DefKind::AssocTy, def_id) => Some(def_id), @@ -614,7 +614,7 @@ impl<'ast, 'ra, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> { && self .r .resolutions(module) - .borrow() + .borrow(self.r) .iter() .any(|(key, _r)| key.ident.name == following_seg.ident.name) } else { @@ -1127,9 +1127,9 @@ impl<'ast, 'ra, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> { fn lookup_doc_alias_name(&mut self, path: &[Segment], ns: Namespace) -> Option<(DefId, Ident)> { let find_doc_alias_name = |r: &mut Resolver<'ra, '_>, m: Module<'ra>, item_name: Symbol| { - for resolution in r.resolutions(m).borrow().values() { + for resolution in r.resolutions(m).borrow(r).values() { let Some(did) = - resolution.borrow().best_decl().and_then(|binding| binding.res().opt_def_id()) + resolution.borrow(r).best_decl().and_then(|binding| binding.res().opt_def_id()) else { continue; }; @@ -1867,10 +1867,10 @@ impl<'ast, 'ra, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> { let targets: Vec<_> = self .r .resolutions(module) - .borrow() + .borrow(self.r) .iter() .filter_map(|(key, resolution)| { - let resolution = resolution.borrow(); + let resolution = resolution.borrow(self.r); resolution.best_decl().map(|binding| binding.res()).and_then(|res| { if filter_fn(res) { Some((key.ident.name, resolution.orig_ident_span, res)) @@ -2730,9 +2730,11 @@ impl<'ast, 'ra, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> { let targets = self .r .resolutions(*module) - .borrow() + .borrow(self.r) .iter() - .filter_map(|(key, res)| res.borrow().best_decl().map(|binding| (key, binding.res()))) + .filter_map(|(key, res)| { + res.borrow(self.r).best_decl().map(|binding| (key, binding.res())) + }) .filter(|(_, res)| match (kind, res) { (AssocItemKind::Const(..), Res::Def(DefKind::AssocConst { .. }, _)) => true, (AssocItemKind::Fn(_), Res::Def(DefKind::AssocFn, _)) => true, @@ -2933,7 +2935,7 @@ impl<'ast, 'ra, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> { let module = self.r.expect_module(def_id); self.r .resolutions(module) - .borrow() + .borrow(self.r) .iter() .any(|(key, _)| key.ident.name == following_seg.ident.name) } diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index 111eae6a4134f..c548daf1b0d50 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -21,10 +21,9 @@ #![recursion_limit = "256"] // tidy-alphabetical-end -use std::cell::Ref; use std::collections::BTreeSet; use std::ops::ControlFlow; -use std::sync::{Arc, Once}; +use std::sync::{Arc, OnceLock}; use std::{fmt, mem}; use diagnostics::{ParamKindInEnumDiscriminant, ParamKindInNonTrivialAnonConst}; @@ -81,7 +80,7 @@ use crate::diagnostics::impls::{ ImportSuggestion, LabelSuggestion, OnUnknownData, StructCtor, Suggestion, }; use crate::imports::{ImportResolution, NameResolutionRef}; -use crate::ref_mut::{CmCell, CmRefCell}; +use crate::ref_mut::{CmCell, CmRef, CmRefCell}; mod build_reduced_graph; mod check_unused; @@ -633,7 +632,25 @@ impl BindingKey { } } -type Resolutions<'ra> = CmRefCell>>; +type ResolutionTable<'ra> = CmRefCell>>; + +type ExternResolutions<'ra> = OnceLock>; +type LocalResolutions<'ra> = ResolutionTable<'ra>; + +enum Resolutions<'ra> { + Local(LocalResolutions<'ra>), + Extern(ExternResolutions<'ra>), +} + +impl<'ra> Resolutions<'ra> { + fn new(local: bool) -> Self { + if local { + Resolutions::Local(Default::default()) + } else { + Resolutions::Extern(Default::default()) + } + } +} /// One node in the tree of modules. /// @@ -655,8 +672,7 @@ struct ModuleData<'ra> { /// Mapping between names and their (possibly in-progress) resolutions in this module. /// Resolutions in modules from other crates are not populated until accessed. lazy_resolutions: Resolutions<'ra>, - /// True if this is a module from other crate that needs to be populated on access. - populate_on_access: Once, + /// Used to disambiguate underscore items (`const _: T = ...`) in the module. underscore_disambiguator: CmCell, @@ -710,6 +726,7 @@ impl<'ra> ModuleData<'ra> { vis: Visibility, arenas: &'ra ResolverArenas<'ra>, ) -> Self { + let lazy_resolutions = Resolutions::new(kind.is_local()); let self_decl = match kind { ModuleKind::Def(def_kind, def_id, ..) => { let expn_id = expansion.as_local().unwrap_or(LocalExpnId::ROOT); @@ -720,8 +737,7 @@ impl<'ra> ModuleData<'ra> { ModuleData { parent, kind, - lazy_resolutions: Default::default(), - populate_on_access: Once::new(), + lazy_resolutions, underscore_disambiguator: CmCell::new(0), unexpanded_invocations: Default::default(), no_implicit_prelude, @@ -754,8 +770,8 @@ impl<'ra> ModuleData<'ra> { self.kind.is_local() } - fn has_unexpanded_invocations(&self) -> bool { - !self.unexpanded_invocations.borrow().is_empty() + fn has_unexpanded_invocations<'tcx>(&self, r: &Resolver<'ra, 'tcx>) -> bool { + !self.unexpanded_invocations.borrow(r).is_empty() } fn res(&self) -> Option { @@ -779,8 +795,10 @@ impl<'ra> Module<'ra> { resolver: &R, mut f: impl FnMut(&R, IdentKey, Span, Namespace, Decl<'ra>), ) { - for (key, name_resolution) in resolver.as_ref().resolutions(self).borrow().iter() { - let name_resolution = name_resolution.borrow(); + for (key, name_resolution) in + resolver.as_ref().resolutions(self).borrow(resolver.as_ref()).iter() + { + let name_resolution = name_resolution.borrow(resolver.as_ref()); if let Some(decl) = name_resolution.best_decl() { f(resolver, key.ident, name_resolution.orig_ident_span, key.ns, decl); } @@ -792,8 +810,10 @@ impl<'ra> Module<'ra> { resolver: &mut R, mut f: impl FnMut(&mut R, IdentKey, Span, Namespace, Decl<'ra>), ) { - for (key, name_resolution) in resolver.as_mut().resolutions(self).borrow().iter() { - let name_resolution = name_resolution.borrow(); + for (key, name_resolution) in + resolver.as_mut().resolutions(self).borrow(resolver.as_mut()).iter() + { + let name_resolution = name_resolution.borrow(resolver.as_mut()); if let Some(decl) = name_resolution.best_decl() { f(resolver, key.ident, name_resolution.orig_ident_span, key.ns, decl); } @@ -1239,10 +1259,11 @@ impl<'ra> DeclData<'ra> { /// the declaration may not be as "determined" as we think. /// FIXME: relationship between this function and similar `NameResolution::determined_decl` /// is unclear. - fn determined(&self) -> bool { + fn determined<'tcx>(&self, r: &Resolver<'ra, 'tcx>) -> bool { match &self.kind { DeclKind::Import { source_decl, import, .. } if import.is_glob() => { - !import.parent_scope.module.has_unexpanded_invocations() && source_decl.determined() + !import.parent_scope.module.has_unexpanded_invocations(r) + && source_decl.determined(r) } _ => true, } @@ -1992,7 +2013,9 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { /// Currently only dependent on `assert_speculative`, if `assert_speculative` is false, /// the resolver will allow mutation; otherwise, it will be immutable. fn cm(&mut self) -> CmResolver<'_, 'ra, 'tcx> { - CmResolver::new(self, !self.assert_speculative) + // SAFETY: we know that `assert_speculative` is true whenever we are using the + // resolver in a multi-threaded context, so this is safe. + unsafe { CmResolver::new(self, !self.assert_speculative) } } /// Runs the function on each namespace. @@ -2109,7 +2132,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { found_traits: &mut Vec>, ) { module.ensure_traits(self); - let traits = module.traits.borrow(); + let traits = module.traits.borrow(self); for &(trait_name, trait_binding, trait_module, lint_ambiguous) in traits.as_ref().unwrap().iter() { @@ -2134,7 +2157,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { match (trait_module, assoc_item) { (Some(trait_module), Some((name, ns))) => self .resolutions(trait_module) - .borrow() + .borrow(self) .iter() .any(|(key, _name_resolution)| key.ns == ns && key.ident.name == name), _ => true, @@ -2159,23 +2182,24 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { self.tcx.hir_arena.alloc_slice(&import_ids) } - fn resolutions(&self, module: Module<'ra>) -> &'ra Resolutions<'ra> { - if !module.is_local() { - // as long as 1 thread is building this external table, all other threads will wait - module.populate_on_access.call_once(|| { - *module.lazy_resolutions.borrow_mut_unchecked() = - self.build_reduced_graph_external(module.expect_extern()); - }); + fn resolutions(&self, module: Module<'ra>) -> &'ra ResolutionTable<'ra> { + match &module.0.0.lazy_resolutions { + Resolutions::Extern(extern_res) => { + // Only 1 thread will actually build the table + extern_res.get_or_init(|| { + CmRefCell::new(self.build_reduced_graph_external(module.expect_extern())) + }) + } + Resolutions::Local(local_res) => local_res, } - &module.0.0.lazy_resolutions } fn resolution( &self, module: Module<'ra>, key: BindingKey, - ) -> Option>> { - self.resolutions(module).borrow().get(&key).map(|resolution| resolution.0.borrow()) + ) -> Option>> { + self.resolutions(module).borrow(self).get(&key).map(|resolution| resolution.0.borrow(self)) } #[track_caller] @@ -2416,7 +2440,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ) -> Option> { let entry = self.extern_prelude.get(&ident); entry.and_then(|entry| entry.flag_decl.as_ref()).and_then(|flag_decl| { - let (pending_decl, finalized, is_open) = flag_decl.get(); + let (pending_decl, finalized, is_open) = *flag_decl.read(); let decl = match pending_decl { PendingDecl::Ready(decl) => { if finalize && !finalized && !is_open { @@ -2451,7 +2475,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { } } }; - flag_decl.set((PendingDecl::Ready(decl), finalize || finalized, is_open)); + *flag_decl.write() = (PendingDecl::Ready(decl), finalize || finalized, is_open); decl.or_else(|| finalize.then_some(self.dummy_decl)) }) } @@ -2793,13 +2817,33 @@ type CmResolver<'r, 'ra, 'tcx> = ref_mut::RefOrMut<'r, Resolver<'ra, 'tcx>>; // FIXME: These are cells for caches that can be populated even during speculative resolution, // and should be replaced with mutexes, atomics, or other synchronized data when migrating to // parallel name resolution. -use std::cell::{Cell as CacheCell, RefCell as CacheRefCell}; - +use rustc_data_structures::sync::{RwLock as CacheCell, RwLock as CacheRefCell}; + +/// The [`Resolver`] has different resolution phases, which share most of the resolution logic. +/// Some of these phases mutate the resolver (`&mut Resolver`) during the resolution together with. +/// However we are implementing import resolution (one of these phases), often referred to as +/// "speculative resolution", to a parallel algorithm, which requires 2 things to change: +/// - A `&Resolver`, because cannot mutate any fields in it. +/// - Some fields with interior mutability may not be changed during import resolution, as +/// this can conflict with other in process resolutions (so locks are not the solution). +/// But these fields do require interior mutability during the other phases. +/// +/// Because refactoring the entire name resolution code to be split into "immutable" and "mutable" +/// logic, we opted for a more "developer friendly" and very unsafe approach using data structures +/// that are immutable during import resolution. This module is the collection of 3 data structures +/// that offer use the above 2 points: +/// - a `RefOrMut<'a, T>` smart pointer that gates a `&'a mut T` through a flag +/// (i.e. are we in import res?). +/// - `CmCell` and `CmRefCell`, which are condiotionally mutable through a flag. +/// +/// We hope one day to not have to do this :D mod ref_mut { use std::cell::{BorrowMutError, Cell, Ref, RefCell, RefMut}; - use std::fmt; use std::marker::PhantomData; use std::ops::Deref; + use std::{fmt, mem, ops}; + + use rustc_data_structures::sync::{DynSend, DynSync}; use crate::Resolver; @@ -2810,9 +2854,26 @@ mod ref_mut { // a `&mut T`. p: *mut T, mutable: bool, + // `RefOrMut` technically holds a `&'a mut T` _marker: PhantomData<&'a mut T>, } + // SAFETY: `RefOrMut` can only be constructed via `RefOrMut::new`, which is `unsafe`. + // Its safety contract requires the caller to guarantee that any instance which might be + // observed from more than one thread is constructed with `mutable = false`. + // + // Given that contract holds: + // - `DynSync`: a shared `RefOrMut` only ever yields `&T`, so letting multiple threads + // read through it concurrently is just ordinary shared-reference aliasing; no + // thread can obtain `&mut T` through it, so there is no data race to worry about. + // - `DynSend`: sending a shared `RefOrMut` to another thread only transfers the ability + // to take `&T` there too, which is equally sound. A mutable `RefOrMut` is never actually + // moved across threads in practice, because the contract of `new` forbids it. + // + // Thus, these `impl`s rely entirely on safe usage of `RefOrMut::new`. + unsafe impl<'a, T: DynSync> DynSync for RefOrMut<'a, T> {} + unsafe impl<'a, T: DynSync> DynSend for RefOrMut<'a, T> {} + impl<'a, T> Deref for RefOrMut<'a, T> { type Target = T; @@ -2830,7 +2891,22 @@ mod ref_mut { } impl<'a, T> RefOrMut<'a, T> { - pub(crate) fn new(p: &'a mut T, mutable: bool) -> Self { + /// Constructs a new `RefOrMut` from an exclusive reference, choosing up front + /// whether it will be used as an exclusive (`&mut T`) or a shared (`&T`), done + /// via `mutable`. + /// + /// # Safety + /// + /// `RefOrMut<'a, T>` implements `DynSync` and `DynSend`, for any + /// `T: DynSync`. However, this is only safe if this `RefOrMut` is used as a + /// shared reference: + /// + /// If the resulting `RefOrMut` may ever be accessed from, or sent to, more than + /// one thread, it **must** be constructed with `mutable = false`, and therefore can + /// only ever be used to obtain shared access (`&T`) for its entire lifetime. + /// + /// Violating this is undefined behavior. + pub(crate) unsafe fn new(p: &'a mut T, mutable: bool) -> Self { RefOrMut { p, mutable, _marker: PhantomData } } @@ -2839,6 +2915,7 @@ mod ref_mut { !self.mutable, "Tried to reborrow a mutable `RefOrMut` through shared reference." ); + // Safe because of contract in `RefOrMut::new`. RefOrMut { p: self.p, mutable: self.mutable, _marker: PhantomData } } @@ -2859,6 +2936,8 @@ mod ref_mut { // SAFETY: // - `RefOrMut` is only constructable through a `&mut T` and we // have tested that it may indeed be used as a `&mut T` in this match. + // - `RefOrMut::new` also guarantees that this exclusive reference is never + // obtained from multiple threads. true => unsafe { self.p.as_mut_unchecked() }, } } @@ -2868,6 +2947,12 @@ mod ref_mut { #[derive(Default)] pub(crate) struct CmCell(Cell); + // SAFETY: `CmCell` is `Sync` only because every path that can call `Cell::set` + // (i.e. `CmCell::set`, and `update`, which is built on top of it) first checks + // `r.assert_speculative` and panics if it's set, refusing to mutate. Soundness + // therefore depends on proper usage of the `assert_speculative` field in the `Resolver`. + unsafe impl DynSync for CmCell {} + impl fmt::Debug for CmCell { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_tuple("CmCell").field(&self.get()).finish() @@ -2911,28 +2996,50 @@ mod ref_mut { } } - /// A wrapper around a [`RefCell`] that only allows writes (mutable borrows) based on a condition in the resolver. + pub(crate) enum CmRef<'b, T> { + /// A "speculative" reference that is not tracked, assuming correct usage of + /// `Resolver::assert_speculative`. + Speculative(&'b T), + + /// Normal runtime borrow checking using `RefCell` semantics. Only used when not + /// in speculative resolution. + Normal(Ref<'b, T>), + } + + impl<'b, T: 'b> ops::Deref for CmRef<'b, T> { + type Target = T; + + fn deref(&self) -> &Self::Target { + match self { + CmRef::Speculative(r) => r, + CmRef::Normal(r) => r.deref(), + } + } + } + + // Soundness depends entirely on proper usage of the `assert_speculative` field in the + // `Resolver`. This means that whenever we hold a `CmRef::Normal` or `cell::RefMut` and + // flip the switch on `assert_speculative` we also drop these borrows. + /// A wrapper around `RefCell` semantics that only allows writes (mutable borrows) based on a condition in the resolver. #[derive(Default)] - pub(crate) struct CmRefCell(RefCell); + pub(crate) struct CmRefCell { + inner: RefCell, + } + + // SAFETY: `CmRefCell` is `Sync` only because every path borrows first checks + // `r.assert_speculative` and panics if it's set, refusing to mutate anything underlying, + // including any borrow counters. + // Soundness therefore depends entirely on proper usage of the `assert_speculative` field in the `Resolver`. + unsafe impl DynSync for CmRefCell {} impl CmRefCell { pub(crate) fn new(value: T) -> CmRefCell { - CmRefCell(RefCell::new(value)) - } - - #[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() + CmRefCell { inner: RefCell::new(value) } } #[track_caller] pub(crate) fn borrow_mut<'ra, 'tcx>(&self, r: &Resolver<'ra, 'tcx>) -> RefMut<'_, T> { - if r.assert_speculative { - panic!("not allowed to mutably borrow a `CmRefCell` during speculative resolution"); - } - self.0.borrow_mut() + self.try_borrow_mut(r).expect("`CmRefCell` is already borrowed") } #[track_caller] @@ -2943,21 +3050,33 @@ mod ref_mut { if r.assert_speculative { panic!("not allowed to mutably borrow a `CmRefCell` during speculative resolution"); } - self.0.try_borrow_mut() + self.inner.try_borrow_mut() } #[track_caller] - pub(crate) fn borrow(&self) -> Ref<'_, T> { - self.0.borrow() + pub(crate) fn borrow<'ra, 'tcx>(&self, r: &Resolver<'ra, 'tcx>) -> CmRef<'_, T> { + if r.assert_speculative { + // SAFETY: We rely on the fact that we can't mutably borrow during speculative resolution. + // + // We do get a "special protection" if we ever mutably borrowed this `CmRefCell`, + // then switch to `assert_speculative = true` and try this borrow, it will fail. + // + // NB: The other way does not have this "protection", which makes me question this all. + // (see comment above `CmRefCell`). + CmRef::Speculative(unsafe { + self.inner + .try_borrow_unguarded() + .expect("Mutably borrowed during speculative resolution") + }) + } else { + CmRef::Normal(self.inner.borrow()) + } } } impl CmRefCell { pub(crate) fn take<'ra, 'tcx>(&self, r: &Resolver<'ra, 'tcx>) -> T { - if r.assert_speculative { - panic!("not allowed to mutate a CmRefCell during speculative resolution"); - } - self.0.take() + mem::take(&mut *self.borrow_mut(r)) } } } diff --git a/compiler/rustc_resolve/src/macros.rs b/compiler/rustc_resolve/src/macros.rs index f2c26caa58aab..b3bd10094d593 100644 --- a/compiler/rustc_resolve/src/macros.rs +++ b/compiler/rustc_resolve/src/macros.rs @@ -543,7 +543,7 @@ impl<'ra, 'tcx> ResolverExpand for Resolver<'ra, 'tcx> { star_span: Span, ) -> Result)>, Indeterminate> { let target_trait = self.expect_module(trait_def_id); - if target_trait.has_unexpanded_invocations() { + if target_trait.has_unexpanded_invocations(self) { return Err(Indeterminate); } // FIXME: Instead of waiting try generating all trait methods, and pruning