diff --git a/Cargo.toml b/Cargo.toml index a06977e..5e7a392 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,6 +19,7 @@ rust-version = "1.59.0" [dependencies] ahash = "0.7.6" arrayvec = "0.7.2" +atomic_refcell = "0.1.10" # part of public API rayon = { version = "1.5.0", optional = true } shred-derive = { path = "shred-derive", version = "0.6.3", optional = true } smallvec = "1.6.1" diff --git a/benches/bench.rs b/benches/bench.rs index f685557..4d3a20e 100644 --- a/benches/bench.rs +++ b/benches/bench.rs @@ -241,11 +241,14 @@ fn bench_fetching(b: &mut Bencher) { #[bench] fn bench_indirection_refs(b: &mut Bencher) { - use shred::cell::{Ref, TrustCell}; + use shred::cell::{AtomicRef, AtomicRefCell}; use std::ops::Deref; - let cell = TrustCell::new(Box::new(10)); - let refs: Vec>> = std::iter::repeat(cell.borrow()).take(10000).collect(); + let cell = AtomicRefCell::new(Box::new(10)); + let borrow = cell.borrow(); + let refs: Vec>> = std::iter::repeat_with(|| AtomicRef::clone(&borrow)) + .take(10000) + .collect(); b.iter(|| { let sum: usize = refs.iter().map(|v| v.deref().deref()).sum(); @@ -255,13 +258,15 @@ fn bench_indirection_refs(b: &mut Bencher) { #[bench] fn bench_direct_refs(b: &mut Bencher) { - use shred::cell::{Ref, TrustCell}; + use shred::cell::{AtomicRef, AtomicRefCell}; use std::ops::Deref; - let cell = TrustCell::new(Box::new(10)); - let refs: Vec> = std::iter::repeat(cell.borrow().map(Box::as_ref)) - .take(10000) - .collect(); + let cell = AtomicRefCell::new(Box::new(10)); + let mapped_borrow = AtomicRef::map(cell.borrow(), Box::as_ref); + let refs: Vec> = + std::iter::repeat_with(|| AtomicRef::clone(&mapped_borrow)) + .take(10000) + .collect(); b.iter(|| { let sum: usize = refs.iter().map(|v| v.deref()).sum(); diff --git a/examples/dyn_sys_data.rs b/examples/dyn_sys_data.rs index 7d40034..93e5fc8 100644 --- a/examples/dyn_sys_data.rs +++ b/examples/dyn_sys_data.rs @@ -11,7 +11,7 @@ extern crate shred; use ahash::AHashMap as HashMap; use shred::{ - cell::{Ref, RefMut}, + cell::{AtomicRef, AtomicRefMut}, Accessor, AccessorCow, CastFrom, DispatcherBuilder, DynamicSystemData, MetaTable, Read, Resource, ResourceId, System, SystemData, World, }; @@ -54,32 +54,20 @@ impl<'a> System<'a> for DynamicSystem { let reads: Vec<&dyn Reflection> = data .reads .iter() - .map(|resource| { - // explicitly use the type because we're dealing with `&Resource` which is - // implemented by a lot of types; we don't want to accidentally - // get a `&Box` and cast it to a `&Resource`. - let res = Box::as_ref(resource); - - meta.get(res).expect("Not registered in meta table") - }) + .map(|resource| meta.get(&**resource).expect("Not registered in meta table")) .collect(); let writes: Vec<&mut dyn Reflection> = data .writes .iter_mut() .map(|resource| { - // explicitly use the type because we're dealing with `&mut Resource` which is - // implemented by a lot of types; we don't want to accidentally get a - // `&mut Box` and cast it to a `&mut Resource`. - let res = Box::as_mut(resource); - // For some reason this needs a type ascription, otherwise Rust will think it's - // a `&mut (Reflection + '_)` (as opposed to `&mut (Reflection + 'static)`. - let res: &mut dyn Reflection = meta.get_mut(res).expect( + // a `&mut (Reflaction + '_)` (as opposed to `&mut (Reflection + 'static)`. (Note this + // isn't needed in newer rust version but fails on the MSRV of 1.59.0). + let res: &mut dyn Reflection = meta.get_mut(&mut **resource).expect( "Not registered in meta \ table", ); - res }) .collect(); @@ -150,8 +138,8 @@ struct ScriptInput<'a> { struct ScriptSystemData<'a> { meta_table: Read<'a, ReflectionTable>, - reads: Vec>>, - writes: Vec>>, + reads: Vec>, + writes: Vec>, } impl<'a> DynamicSystemData<'a> for ScriptSystemData<'a> { @@ -159,28 +147,38 @@ impl<'a> DynamicSystemData<'a> for ScriptSystemData<'a> { fn setup(_accessor: &Dependencies, _res: &mut World) {} - fn fetch(access: &Dependencies, res: &'a World) -> Self { + fn fetch(access: &Dependencies, world: &'a World) -> Self { let reads = access .reads .iter() .map(|id| { - res.try_fetch_internal(id.clone()) - .expect("bug: the requested resource does not exist") - .borrow() + let id = id.clone(); + // SAFETY: We don't expose mutable reference to the Box or swap it out. + let res = unsafe { world.try_fetch_internal(id) }; + AtomicRef::map( + res.expect("bug: the requested resource does not exist") + .borrow(), + Box::as_ref, + ) }) .collect(); let writes = access .writes .iter() .map(|id| { - res.try_fetch_internal(id.clone()) - .expect("bug: the requested resource does not exist") - .borrow_mut() + let id = id.clone(); + // SAFETY: We don't expose mutable reference to the Box or swap it out. + let res = unsafe { world.try_fetch_internal(id) }; + AtomicRefMut::map( + res.expect("bug: the requested resource does not exist") + .borrow_mut(), + Box::as_mut, + ) }) .collect(); ScriptSystemData { - meta_table: SystemData::fetch(res), + meta_table: SystemData::fetch(world), reads, writes, } diff --git a/src/cell.rs b/src/cell.rs deleted file mode 100644 index 469cd9a..0000000 --- a/src/cell.rs +++ /dev/null @@ -1,671 +0,0 @@ -//! Helper module for some internals, most users don't need to interact with it. - -use core::marker::PhantomData; -use core::ptr::NonNull; -use std::{ - cell::UnsafeCell, - error::Error, - fmt::{Debug, Display, Error as FormatError, Formatter}, - ops::{Deref, DerefMut}, - sync::atomic::{AtomicUsize, Ordering}, - usize, -}; - -macro_rules! borrow_panic { - ($s:expr) => {{ - panic!( - "Tried to fetch data of type {:?}, but it was already borrowed{}.", - ::std::any::type_name::(), - $s, - ) - }}; -} - -/// Marker struct for an invalid borrow error -#[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)] -pub struct InvalidBorrow; - -impl Display for InvalidBorrow { - fn fmt(&self, f: &mut Formatter) -> Result<(), FormatError> { - write!(f, "Tried to borrow when it was illegal") - } -} - -impl Error for InvalidBorrow { - fn description(&self) -> &str { - "This error is returned when you try to borrow immutably when it's already \ - borrowed mutably or you try to borrow mutably when it's already borrowed" - } -} - -/// An immutable reference to data in a `TrustCell`. -/// -/// Access the value via `std::ops::Deref` (e.g. `*val`) -pub struct Ref<'a, T: ?Sized + 'a> { - flag: &'a AtomicUsize, - value: NonNull, -} - -// SAFETY: `Ref<'_, T> acts as a reference. -unsafe impl<'a, T: ?Sized + 'a> Sync for Ref<'a, T> where for<'b> &'b T: Sync {} -unsafe impl<'a, T: ?Sized + 'a> Send for Ref<'a, T> where for<'b> &'b T: Send {} - -impl<'a, T: ?Sized> Ref<'a, T> { - /// Makes a new `Ref` for a component of the borrowed data which preserves - /// the existing borrow. - /// - /// The `TrustCell` is already immutably borrowed, so this cannot fail. - /// - /// This is an associated function that needs to be used as `Ref::map(...)`. - /// A method would interfere with methods of the same name on the contents - /// of a `Ref` used through `Deref`. Further this preserves the borrow of - /// the value and hence does the proper cleanup when it's dropped. - /// - /// # Examples - /// - /// This can be used to avoid pointer indirection when a boxed item is - /// stored in the `TrustCell`. - /// - /// ``` - /// use shred::cell::{Ref, TrustCell}; - /// - /// let cb = TrustCell::new(Box::new(5)); - /// - /// // Borrowing the cell causes the `Ref` to store a reference to the `Box`, which is a - /// // pointer to the value on the heap, not the actual value. - /// let boxed_ref: Ref<'_, Box> = cb.borrow(); - /// assert_eq!(**boxed_ref, 5); // Notice the double deref to get the actual value. - /// - /// // By using `map` we can let `Ref` store a reference directly to the value on the heap. - /// let pure_ref: Ref<'_, usize> = Ref::map(boxed_ref, Box::as_ref); - /// - /// assert_eq!(*pure_ref, 5); - /// ``` - /// - /// We can also use `map` to get a reference to a sub-part of the borrowed - /// value. - /// - /// ```rust - /// # use shred::cell::{TrustCell, Ref}; - /// - /// let c = TrustCell::new((5, 'b')); - /// let b1: Ref<'_, (u32, char)> = c.borrow(); - /// let b2: Ref<'_, u32> = Ref::map(b1, |t| &t.0); - /// assert_eq!(*b2, 5); - /// ``` - pub fn map(self, f: F) -> Ref<'a, U> - where - F: FnOnce(&T) -> &U, - U: ?Sized, - { - let val = Ref { - flag: self.flag, - value: NonNull::from(f(&*self)), - }; - - std::mem::forget(self); - - val - } -} - -impl<'a, T: ?Sized> Deref for Ref<'a, T> { - type Target = T; - - fn deref(&self) -> &T { - // SAFETY: `Ref` holds a shared borrow of the value. - unsafe { self.value.as_ref() } - } -} - -impl<'a, T: ?Sized> Drop for Ref<'a, T> { - fn drop(&mut self) { - self.flag.fetch_sub(1, Ordering::Release); - } -} - -impl<'a, T: ?Sized> Clone for Ref<'a, T> { - fn clone(&self) -> Self { - self.flag.fetch_add(1, Ordering::Release); - Ref { - flag: self.flag, - value: self.value, - } - } -} - -impl<'a, T: ?Sized + Debug> Debug for Ref<'a, T> { - fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FormatError> { - ::fmt(self, f) - } -} - -/// A mutable reference to data in a `TrustCell`. -/// -/// Access the value via `std::ops::DerefMut` (e.g. `*val`) -pub struct RefMut<'a, T: ?Sized + 'a> { - flag: &'a AtomicUsize, - value: NonNull, - // `NonNull` is covariant over `T` but we use it in the place of a mutable reference so we need to be - // invariant over `T`. - marker: PhantomData<&'a mut T>, -} - -// SAFETY: `RefMut<'_, T> acts as a mutable reference. -unsafe impl<'a, T: ?Sized + 'a> Sync for RefMut<'a, T> where for<'b> &'b mut T: Sync {} -unsafe impl<'a, T: ?Sized + 'a> Send for RefMut<'a, T> where for<'b> &'b mut T: Send {} - -impl<'a, T: ?Sized> RefMut<'a, T> { - /// Makes a new `RefMut` for a component of the borrowed data which - /// preserves the existing borrow. - /// - /// The `TrustCell` is already mutably borrowed, so this cannot fail. - /// - /// This is an associated function that needs to be used as - /// `RefMut::map(...)`. A method would interfere with methods of the - /// same name on the contents of a `RefMut` used through `DerefMut`. - /// Further this preserves the borrow of the value and hence does the - /// proper cleanup when it's dropped. - /// - /// # Examples - /// - /// This can also be used to avoid pointer indirection when a boxed item is - /// stored in the `TrustCell`. - /// - /// ``` - /// use shred::cell::{RefMut, TrustCell}; - /// - /// let cb = TrustCell::new(Box::new(5)); - /// - /// // Borrowing the cell causes the `RefMut` to store a reference to the `Box`, which is a - /// // pointer to the value on the heap, and not a reference directly to the value. - /// let boxed_ref: RefMut<'_, Box> = cb.borrow_mut(); - /// assert_eq!(**boxed_ref, 5); // Notice the double deref to get the actual value. - /// - /// // By using `map` we can let `RefMut` store a reference directly to the value on the heap. - /// let pure_ref: RefMut<'_, usize> = RefMut::map(boxed_ref, Box::as_mut); - /// - /// assert_eq!(*pure_ref, 5); - /// ``` - /// - /// We can also use `map` to get a reference to a sub-part of the borrowed - /// value. - /// - /// ```rust - /// # use shred::cell::{TrustCell, RefMut}; - /// - /// let c = TrustCell::new((5, 'b')); - /// - /// let b1: RefMut<'_, (u32, char)> = c.borrow_mut(); - /// let b2: RefMut<'_, u32> = RefMut::map(b1, |t| &mut t.0); - /// assert_eq!(*b2, 5); - /// ``` - pub fn map(mut self, f: F) -> RefMut<'a, U> - where - F: FnOnce(&mut T) -> &mut U, - U: ?Sized, - { - // Extract the values from the `RefMut` through a pointer so that we do not run - // `Drop`. Because the returned `RefMut` has the same lifetime `'a` as - // the given `RefMut`, the lifetime we created through turning the - // pointer into a ref is valid. - let flag = self.flag; - let value = NonNull::from(f(&mut *self)); - - // We have to forget self so that we do not run `Drop`. Further it's safe - // because we are creating a new `RefMut`, with the same flag, which - // will run the cleanup when it's dropped. - std::mem::forget(self); - - RefMut { - flag, - value, - marker: PhantomData, - } - } -} - -impl<'a, T: ?Sized> Deref for RefMut<'a, T> { - type Target = T; - - fn deref(&self) -> &T { - // SAFETY: `RefMut` holds an exclusive borrow of the value and we have a shared borrow of - // `RefMut` here. - unsafe { self.value.as_ref() } - } -} - -impl<'a, T: ?Sized> DerefMut for RefMut<'a, T> { - fn deref_mut(&mut self) -> &mut T { - // SAFETY: `RefMut` holds an exclusive borrow of the value and we have an exclusive borrow - // of `RefMut` here. - unsafe { self.value.as_mut() } - } -} - -impl<'a, T: ?Sized> Drop for RefMut<'a, T> { - fn drop(&mut self) { - self.flag.store(0, Ordering::Release) - } -} - -impl<'a, T: ?Sized + Debug> Debug for RefMut<'a, T> { - fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FormatError> { - ::fmt(self, f) - } -} - -/// A custom cell container that is a `RefCell` with thread-safety. -#[derive(Debug)] -pub struct TrustCell { - flag: AtomicUsize, - inner: UnsafeCell, -} - -impl TrustCell { - /// Create a new cell, similar to `RefCell::new` - pub fn new(val: T) -> Self { - TrustCell { - flag: AtomicUsize::new(0), - inner: UnsafeCell::new(val), - } - } - - /// Consumes this cell and returns ownership of `T`. - pub fn into_inner(self) -> T { - self.inner.into_inner() - } - - /// Get an immutable reference to the inner data. - /// - /// Absence of write accesses is checked at run-time. - /// - /// # Panics - /// - /// This function will panic if there is a mutable reference to the data - /// already in use. - pub fn borrow(&self) -> Ref { - self.check_flag_read() - .unwrap_or_else(|_| borrow_panic!(" mutably")); - - Ref { - flag: &self.flag, - value: unsafe { NonNull::new_unchecked(self.inner.get()) }, - } - } - - /// Get an immutable reference to the inner data. - /// - /// Absence of write accesses is checked at run-time. If access is not - /// possible, an error is returned. - pub fn try_borrow(&self) -> Result, InvalidBorrow> { - self.check_flag_read()?; - - Ok(Ref { - flag: &self.flag, - value: unsafe { NonNull::new_unchecked(self.inner.get()) }, - }) - } - - /// Get a mutable reference to the inner data. - /// - /// Exclusive access is checked at run-time. - /// - /// # Panics - /// - /// This function will panic if there are any references to the data already - /// in use. - pub fn borrow_mut(&self) -> RefMut { - self.check_flag_write() - .unwrap_or_else(|_| borrow_panic!("")); - - RefMut { - flag: &self.flag, - value: unsafe { NonNull::new_unchecked(self.inner.get()) }, - marker: PhantomData, - } - } - - /// Get a mutable reference to the inner data. - /// - /// Exclusive access is checked at run-time. If access is not possible, an - /// error is returned. - pub fn try_borrow_mut(&self) -> Result, InvalidBorrow> { - self.check_flag_write()?; - - Ok(RefMut { - flag: &self.flag, - value: unsafe { NonNull::new_unchecked(self.inner.get()) }, - marker: PhantomData, - }) - } - - /// Gets exclusive access to the inner value, bypassing the Cell. - /// - /// Exclusive access is checked at compile time. - pub fn get_mut(&mut self) -> &mut T { - // safe because we have exclusive access via &mut self - unsafe { &mut *self.inner.get() } - } - - /// Make sure we are allowed to aquire a read lock, and increment the read - /// count by 1 - fn check_flag_read(&self) -> Result<(), InvalidBorrow> { - // Check that no write reference is out, then try to increment the read count - // and return once successful. - loop { - let val = self.flag.load(Ordering::Acquire); - - if val == usize::MAX { - return Err(InvalidBorrow); - } - - if self - .flag - .compare_exchange_weak(val, val + 1, Ordering::AcqRel, Ordering::Acquire) - .is_ok() - { - return Ok(()); - } - } - } - - /// Make sure we are allowed to aquire a write lock, and then set the write - /// lock flag. - fn check_flag_write(&self) -> Result<(), InvalidBorrow> { - // Check we have 0 references out, and then set the ref count to usize::MAX to - // indicate a write lock. - self.flag - .compare_exchange(0, usize::MAX, Ordering::AcqRel, Ordering::Acquire) - .map_err(|_| InvalidBorrow)?; - Ok(()) - } -} - -unsafe impl Sync for TrustCell where T: Sync + Send {} - -impl Default for TrustCell -where - T: Default, -{ - fn default() -> Self { - TrustCell::new(Default::default()) - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn allow_multiple_reads() { - let cell: TrustCell<_> = TrustCell::new(5); - - let a = cell.borrow(); - let b = cell.borrow(); - - assert_eq!(10, *a + *b); - } - - #[test] - fn allow_clone_reads() { - let cell: TrustCell<_> = TrustCell::new(5); - - let a = cell.borrow(); - let b = a.clone(); - - assert_eq!(10, *a + *b); - } - - #[test] - fn allow_single_write() { - let cell: TrustCell<_> = TrustCell::new(5); - - { - let mut a = cell.borrow_mut(); - *a += 2; - *a += 3; - } - - assert_eq!(10, *cell.borrow()); - } - - #[test] - #[should_panic(expected = "but it was already borrowed mutably")] - fn panic_write_and_read() { - let cell: TrustCell<_> = TrustCell::new(5); - - let mut a = cell.borrow_mut(); - *a = 7; - - assert_eq!(7, *cell.borrow()); - } - - #[test] - #[should_panic(expected = "but it was already borrowed")] - fn panic_write_and_write() { - let cell: TrustCell<_> = TrustCell::new(5); - - let mut a = cell.borrow_mut(); - *a = 7; - - assert_eq!(7, *cell.borrow_mut()); - } - - #[test] - #[should_panic(expected = "but it was already borrowed")] - fn panic_read_and_write() { - let cell: TrustCell<_> = TrustCell::new(5); - - let _a = cell.borrow(); - - assert_eq!(7, *cell.borrow_mut()); - } - - #[test] - fn try_write_and_read() { - let cell: TrustCell<_> = TrustCell::new(5); - - let mut a = cell.try_borrow_mut().unwrap(); - *a = 7; - - assert!(cell.try_borrow().is_err()); - } - - #[test] - fn try_write_and_write() { - let cell: TrustCell<_> = TrustCell::new(5); - - let mut a = cell.try_borrow_mut().unwrap(); - *a = 7; - - assert!(cell.try_borrow_mut().is_err()); - } - - #[test] - fn try_read_and_write() { - let cell: TrustCell<_> = TrustCell::new(5); - - let _a = cell.try_borrow().unwrap(); - - assert!(cell.try_borrow_mut().is_err()); - } - - #[test] - fn cloned_borrow_does_not_allow_write() { - let cell: TrustCell<_> = TrustCell::new(5); - - let a = cell.borrow(); - let _b = a.clone(); - drop(a); - - assert!(cell.try_borrow_mut().is_err()); - } - - fn make_ref<'a, T: ?Sized>(flag: &'a AtomicUsize, value: &'a T) -> Ref<'a, T> { - Ref { - flag, - value: NonNull::from(value), - } - } - - #[test] - fn ref_with_non_sized() { - let value = &[2, 3, 4, 5][..]; - let flag = AtomicUsize::new(1); - let r: Ref<'_, [i32]> = make_ref(&flag, value); - - assert_eq!(&*r, &[2, 3, 4, 5][..]); - } - - #[test] - fn ref_with_non_sized_clone() { - let value = &[2, 3, 4, 5][..]; - let flag = AtomicUsize::new(1); - let r: Ref<'_, [i32]> = make_ref(&flag, value); - let rr = r.clone(); - - assert_eq!(&*r, &[2, 3, 4, 5][..]); - assert_eq!(r.flag.load(Ordering::SeqCst), 2); - - assert_eq!(&*rr, &[2, 3, 4, 5][..]); - assert_eq!(rr.flag.load(Ordering::SeqCst), 2); - } - - #[test] - fn ref_with_trait_obj() { - let value = &2i32; - let flag = AtomicUsize::new(1); - let ra: Ref<'_, dyn std::any::Any> = make_ref(&flag, value); - - assert_eq!(ra.downcast_ref::().unwrap(), &2i32); - } - - fn make_ref_mut<'a, T: ?Sized>(flag: &'a AtomicUsize, value: &'a mut T) -> RefMut<'a, T> { - RefMut { - flag, - value: NonNull::from(value), - marker: PhantomData, - } - } - - #[test] - fn ref_mut_with_non_sized() { - let value: &mut [i32] = &mut [2, 3, 4, 5][..]; - let flag = AtomicUsize::new(1); - let mut r: RefMut<'_, [i32]> = make_ref_mut(&flag, value); - - assert_eq!(&mut *r, &mut [2, 3, 4, 5][..]); - } - - #[test] - fn ref_mut_with_trait_obj() { - let value = &mut 2i32; - let flag = AtomicUsize::new(1); - let mut ra: RefMut<'_, dyn std::any::Any> = make_ref_mut(&flag, value); - - assert_eq!(ra.downcast_mut::().unwrap(), &mut 2i32); - } - - #[test] - fn ref_map_box() { - let cell = TrustCell::new(Box::new(10)); - - let r: Ref<'_, Box> = cell.borrow(); - assert_eq!(&**r, &10); - - let rr: Ref<'_, usize> = cell.borrow().map(Box::as_ref); - assert_eq!(&*rr, &10); - } - - #[test] - fn ref_map_preserves_flag() { - let cell = TrustCell::new(Box::new(10)); - - let r: Ref<'_, Box> = cell.borrow(); - assert_eq!(cell.flag.load(Ordering::SeqCst), 1); - let _nr: Ref<'_, usize> = r.map(Box::as_ref); - assert_eq!(cell.flag.load(Ordering::SeqCst), 1); - } - - #[test] - fn ref_map_retains_borrow() { - let cell = TrustCell::new(Box::new(10)); - - let _r: Ref<'_, usize> = cell.borrow().map(Box::as_ref); - assert_eq!(cell.flag.load(Ordering::SeqCst), 1); - - let _rr: Ref<'_, usize> = cell.borrow().map(Box::as_ref); - assert_eq!(cell.flag.load(Ordering::SeqCst), 2); - } - - #[test] - fn ref_map_drops_borrow() { - let cell = TrustCell::new(Box::new(10)); - - let r: Ref<'_, usize> = cell.borrow().map(Box::as_ref); - - assert_eq!(cell.flag.load(Ordering::SeqCst), 1); - drop(r); - assert_eq!(cell.flag.load(Ordering::SeqCst), 0); - } - - #[test] - fn ref_mut_map_box() { - let cell = TrustCell::new(Box::new(10)); - - { - let mut r: RefMut<'_, Box> = cell.borrow_mut(); - assert_eq!(&mut **r, &mut 10); - } - { - let mut rr: RefMut<'_, usize> = cell.borrow_mut().map(Box::as_mut); - assert_eq!(&mut *rr, &mut 10); - } - } - - #[test] - fn ref_mut_map_preserves_flag() { - let cell = TrustCell::new(Box::new(10)); - - let r: RefMut<'_, Box> = cell.borrow_mut(); - assert_eq!(cell.flag.load(Ordering::SeqCst), std::usize::MAX); - let _nr: RefMut<'_, usize> = r.map(Box::as_mut); - assert_eq!(cell.flag.load(Ordering::SeqCst), std::usize::MAX); - } - - #[test] - #[should_panic(expected = "but it was already borrowed")] - fn ref_mut_map_retains_mut_borrow() { - let cell = TrustCell::new(Box::new(10)); - - let _rr: RefMut<'_, usize> = cell.borrow_mut().map(Box::as_mut); - - let _ = cell.borrow_mut(); - } - - #[test] - fn ref_mut_map_drops_borrow() { - let cell = TrustCell::new(Box::new(10)); - - let r: RefMut<'_, usize> = cell.borrow_mut().map(Box::as_mut); - - assert_eq!(cell.flag.load(Ordering::SeqCst), std::usize::MAX); - drop(r); - assert_eq!(cell.flag.load(Ordering::SeqCst), 0); - } - - // Test intended to allow Miri to catch bugs like this scenario: - // https://github.com/rust-lang/rust/issues/63787 - #[test] - fn drop_and_borrow_in_fn_call() { - fn drop_and_borrow(cell: &TrustCell, borrow: Ref<'_, u8>) { - drop(borrow); - *cell.borrow_mut() = 7u8; - } - - let a = TrustCell::new(4u8); - let borrow = a.borrow(); - drop_and_borrow(&a, borrow); - } -} diff --git a/src/lib.rs b/src/lib.rs index 4a28d6f..8bb9184 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -84,7 +84,12 @@ #![deny(unused_must_use, clippy::disallowed_types)] #![warn(missing_docs)] -pub mod cell; +/// Re-exports from [`atomic_refcell`] +/// +/// Mainly for internals, most users don't need to interact with this. +pub mod cell { + pub use atomic_refcell::*; +} mod dispatch; mod meta; diff --git a/src/meta.rs b/src/meta.rs index 2ecb908..7189800 100644 --- a/src/meta.rs +++ b/src/meta.rs @@ -76,6 +76,7 @@ where // Ugly hack that works due to `UnsafeCell` and distinct resources. unsafe { self.world + // SAFETY: We just read the value and don't replace it. .try_fetch_internal(match self.tys.get(index) { Some(&x) => ResourceId::from_type_id(x), None => return None, @@ -141,6 +142,7 @@ where // Ugly hack that works due to `UnsafeCell` and distinct resources. unsafe { self.world + // SAFETY: We don't swap out the Box or expose a mutable reference to it. .try_fetch_internal(match self.tys.get(index) { Some(&x) => ResourceId::from_type_id(x), None => return None, diff --git a/src/world/entry.rs b/src/world/entry.rs index 06ac878..9231fff 100644 --- a/src/world/entry.rs +++ b/src/world/entry.rs @@ -1,7 +1,7 @@ use std::marker::PhantomData; use crate::{ - cell::{RefMut, TrustCell}, + cell::{AtomicRefCell, AtomicRefMut}, world::{FetchMut, Resource, ResourceId}, }; @@ -24,7 +24,7 @@ type StdEntry<'a, K, V> = std::collections::hash_map::Entry<'a, K, V>; /// println!("{:?}", value.0 * 2); /// ``` pub struct Entry<'a, T: 'a> { - inner: StdEntry<'a, ResourceId, TrustCell>>, + inner: StdEntry<'a, ResourceId, AtomicRefCell>>, marker: PhantomData, } @@ -48,8 +48,8 @@ where { let value = self .inner - .or_insert_with(move || TrustCell::new(Box::new(f()))); - let inner = RefMut::map(value.borrow_mut(), Box::as_mut); + .or_insert_with(move || AtomicRefCell::new(Box::new(f()))); + let inner = AtomicRefMut::map(value.borrow_mut(), Box::as_mut); FetchMut { inner, @@ -58,7 +58,9 @@ where } } -pub fn create_entry(e: StdEntry>>) -> Entry { +pub(super) fn create_entry( + e: StdEntry>>, +) -> Entry { Entry { inner: e, marker: PhantomData, diff --git a/src/world/mod.rs b/src/world/mod.rs index 90f2d00..3a4d688 100644 --- a/src/world/mod.rs +++ b/src/world/mod.rs @@ -14,10 +14,8 @@ use std::{ use ahash::AHashMap as HashMap; -use crate::{ - cell::{Ref, RefMut, TrustCell}, - SystemData, -}; +use crate::cell::{AtomicRef, AtomicRefCell, AtomicRefMut}; +use crate::SystemData; use self::entry::create_entry; @@ -35,7 +33,7 @@ mod setup; /// /// * `T`: The type of the resource pub struct Fetch<'a, T: 'a> { - inner: Ref<'a, dyn Resource>, + inner: AtomicRef<'a, dyn Resource>, phantom: PhantomData<&'a T>, } @@ -53,7 +51,7 @@ where impl<'a, T> Clone for Fetch<'a, T> { fn clone(&self) -> Self { Fetch { - inner: self.inner.clone(), + inner: AtomicRef::clone(&self.inner), phantom: PhantomData, } } @@ -68,7 +66,7 @@ impl<'a, T> Clone for Fetch<'a, T> { /// /// * `T`: The type of the resource pub struct FetchMut<'a, T: 'a> { - inner: RefMut<'a, dyn Resource>, + inner: AtomicRefMut<'a, dyn Resource>, phantom: PhantomData<&'a mut T>, } @@ -193,7 +191,7 @@ impl ResourceId { /// Resources are identified by `ResourceId`s, which consist of a `TypeId`. #[derive(Default)] pub struct World { - resources: HashMap>>, + resources: HashMap>>, } impl World { @@ -430,7 +428,7 @@ impl World { let res_id = ResourceId::new::(); self.resources.get(&res_id).map(|r| Fetch { - inner: Ref::map(r.borrow(), Box::as_ref), + inner: AtomicRef::map(r.borrow(), Box::as_ref), phantom: PhantomData, }) } @@ -451,7 +449,7 @@ impl World { id.assert_same_type_id::(); self.resources.get(&id).map(|r| Fetch { - inner: Ref::map(r.borrow(), Box::as_ref), + inner: AtomicRef::map(r.borrow(), Box::as_ref), phantom: PhantomData, }) } @@ -480,7 +478,7 @@ impl World { let res_id = ResourceId::new::(); self.resources.get(&res_id).map(|r| FetchMut { - inner: RefMut::map(r.borrow_mut(), Box::as_mut), + inner: AtomicRefMut::map(r.borrow_mut(), Box::as_mut), phantom: PhantomData, }) } @@ -501,7 +499,7 @@ impl World { id.assert_same_type_id::(); self.resources.get(&id).map(|r| FetchMut { - inner: RefMut::map(r.borrow_mut(), Box::as_mut), + inner: AtomicRefMut::map(r.borrow_mut(), Box::as_mut), phantom: PhantomData, }) } @@ -520,7 +518,7 @@ impl World { { id.assert_same_type_id::(); - self.resources.insert(id, TrustCell::new(Box::new(r))); + self.resources.insert(id, AtomicRefCell::new(Box::new(r))); } /// Internal function for removing resources, should only be used if you @@ -542,7 +540,7 @@ impl World { self.resources .remove(&id) - .map(TrustCell::into_inner) + .map(AtomicRefCell::into_inner) .map(|x: Box| x.downcast()) .map(|x: Result, _>| x.ok().unwrap()) .map(|x| *x) @@ -550,7 +548,15 @@ impl World { /// Internal function for fetching resources, should only be used if you /// know what you're doing. - pub fn try_fetch_internal(&self, id: ResourceId) -> Option<&TrustCell>> { + /// + /// # Safety + /// + /// If this is used to replace the `Box` with a different one, the new one must + /// have a `TypeId` that matches the one in the `ResourceId` provided here. + pub unsafe fn try_fetch_internal( + &self, + id: ResourceId, + ) -> Option<&AtomicRefCell>> { self.resources.get(&id) } @@ -566,7 +572,7 @@ impl World { pub fn get_mut_raw(&mut self, id: ResourceId) -> Option<&mut dyn Resource> { self.resources .get_mut(&id) - .map(TrustCell::get_mut) + .map(AtomicRefCell::get_mut) .map(Box::as_mut) } } @@ -718,7 +724,7 @@ mod tests { #[allow(unused)] #[test] - #[should_panic(expected = "but it was already borrowed")] + #[should_panic(expected = "already immutably borrowed")] fn read_write_fails() { let mut world = World::empty(); world.insert(Res); @@ -729,7 +735,7 @@ mod tests { #[allow(unused)] #[test] - #[should_panic(expected = "but it was already borrowed mutably")] + #[should_panic(expected = "already mutably borrowed")] fn write_read_fails() { let mut world = World::empty(); world.insert(Res);