Merge #309

309: RestrictedStorage safety fix r=torkleyy a=Aceeri

This shouldn't be allowed in parallel, since that would mean you could have a reader in one thread while you having it mutably in another thread.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/slide-rs/specs/309)
<!-- Reviewable:end -->

src/lib.rs

@@ -214,10 +214,10 @@ pub use shred::{Dispatcher, DispatcherBuilder, Fetch, FetchId, FetchIdMut, Fetch
 pub use shred::AsyncDispatcher;
 
 pub use storage::{BTreeStorage, Change, ChangeEvents, DenseVecStorage, DistinctStorage, Entry,
-                  FlaggedStorage, HashMapStorage, InsertResult, MaskedStorage, NormalRestriction,
-                  NullStorage, OccupiedEntry, ParallelRestriction, ReadStorage, RestrictedStorage,
-                  Storage, StorageEntry, TrackedStorage, UnprotectedStorage, VacantEntry,
-                  VecStorage, WriteStorage};
+                  FlaggedStorage, HashMapStorage, ImmutableParallelRestriction, InsertResult,
+                  MaskedStorage, MutableParallelRestriction, NullStorage, OccupiedEntry, ReadStorage,
+                  RestrictedStorage, SequentialRestriction, Storage, StorageEntry, TrackedStorage,
+                  UnprotectedStorage, VacantEntry, VecStorage, WriteStorage};
 pub use world::{Component, CreateIter, CreateIterAtomic, EntitiesRes, Entity, EntityBuilder,
                 Generation, LazyBuilder, LazyUpdate, World};
 

src/storage/mod.rs

@@ -2,7 +2,7 @@
 
 pub use self::data::{ReadStorage, WriteStorage};
 pub use self::flagged::FlaggedStorage;
-pub use self::restrict::{Entry, NormalRestriction, ParallelRestriction, RestrictedStorage};
+pub use self::restrict::{Entry, SequentialRestriction, MutableParallelRestriction, ImmutableParallelRestriction, RestrictedStorage};
 #[cfg(feature = "serde")]
 pub use self::ser::{MergeError, PackedData};
 pub use self::storages::{BTreeStorage, DenseVecStorage, HashMapStorage, NullStorage, VecStorage};

src/storage/restrict.rs

@@ -10,9 +10,24 @@ use storage::MaskedStorage;
 use world::EntityIndex;
 
 /// Specifies that the `RestrictedStorage` cannot run in parallel.
-pub enum NormalRestriction {}
-/// Specifies that the `RestrictedStorage` can run in parallel.
-pub enum ParallelRestriction {}
+///
+/// A mutable `RestrictedStorage` can call `get`, `get_mut`, `get_unchecked` and
+/// `get_mut_unchecked` for deferred access while an immutable version can only
+/// call the immutable accessors.
+pub enum SequentialRestriction {}
+/// Specifies that the `RestrictedStorage` can run in parallel mutably.
+///
+/// This means the storage can only call `get_mut_unchecked` and `get_unchecked`.
+pub enum MutableParallelRestriction {}
+/// Specifies that the `RestrictedStorage` can run in parallel immutably.
+/// 
+/// This means that the storage can call `get`, `get_unchecked`.
+pub enum ImmutableParallelRestriction {}
+
+/// Restrictions that are allowed to access `RestrictedStorage::get`.
+pub trait ImmutableAliasing: Sized {}
+impl ImmutableAliasing for SequentialRestriction { }
+impl ImmutableAliasing for ImmutableParallelRestriction { }
 
 /// Similar to a `MaskedStorage` and a `Storage` combined, but restricts usage
 /// to only getting and modifying the components. That means nothing that would
@@ -62,20 +77,21 @@ where
 }
 
 unsafe impl<'rf, 'st: 'rf, B, T, R> ParJoin
-    for &'rf mut RestrictedStorage<'rf, 'st, B, T, R, ParallelRestriction>
+    for &'rf mut RestrictedStorage<'rf, 'st, B, T, R, MutableParallelRestriction>
 where
     T: Component,
     R: BorrowMut<T::Storage> + 'rf,
     B: Borrow<BitSet> + 'rf,
 {
 }
 
-unsafe impl<'rf, 'st: 'rf, B, T, R> ParJoin
-    for &'rf RestrictedStorage<'rf, 'st, B, T, R, ParallelRestriction>
+unsafe impl<'rf, 'st: 'rf, B, T, R, RT> ParJoin
+    for &'rf RestrictedStorage<'rf, 'st, B, T, R, RT>
 where
     T: Component,
     R: Borrow<T::Storage> + 'rf,
     B: Borrow<BitSet> + 'rf,
+    RT: ImmutableAliasing,
 {
 }
 
@@ -85,17 +101,6 @@ where
     R: Borrow<T::Storage>,
     B: Borrow<BitSet>,
 {
-    /// Attempts to get the component related to the entity.
-    ///
-    /// Functions similar to the normal `Storage::get` implementation.
-    pub fn get(&self, entity: Entity) -> Option<&T> {
-        if self.bitset.borrow().contains(entity.id()) && self.entities.is_alive(entity) {
-            Some(unsafe { self.data.borrow().get(entity.id()) })
-        } else {
-            None
-        }
-    }
-
     /// Gets the component related to the current entry without checking whether
     /// the storage has it or not.
     pub fn get_unchecked(&self, entry: &Entry<'rf, T>) -> &T {
@@ -118,7 +123,29 @@ where
     }
 }
 
-impl<'rf, 'st, B, T, R> RestrictedStorage<'rf, 'st, B, T, R, NormalRestriction>
+impl<'rf, 'st, B, T, R, RT> RestrictedStorage<'rf, 'st, B, T, R, RT>
+where
+    T: Component,
+    R: Borrow<T::Storage>,
+    B: Borrow<BitSet>,
+    // Only non parallel and immutable parallel storages can access this.
+    RT: ImmutableAliasing,
+{
+    /// Attempts to get the component related to the entity.
+    ///
+    /// Functions similar to the normal `Storage::get` implementation.
+    /// 
+    /// This only works for non-parallel or immutably parallel `RestrictedStorage`.
+    pub fn get(&self, entity: Entity) -> Option<&T> {
+        if self.bitset.borrow().contains(entity.id()) && self.entities.is_alive(entity) {
+            Some(unsafe { self.data.borrow().get(entity.id()) })
+        } else {
+            None
+        }
+    }
+}
+
+impl<'rf, 'st, B, T, R> RestrictedStorage<'rf, 'st, B, T, R, SequentialRestriction>
 where
     T: Component,
     R: BorrowMut<T::Storage>,
@@ -128,7 +155,8 @@ where
     ///
     /// Functions similar to the normal `Storage::get_mut` implementation.
     ///
-    /// Note: This only works if this is a non-parallel `RestrictedStorage`.
+    /// This only works if this is a non-parallel `RestrictedStorage`,
+    /// otherwise you could access the same component mutably in two different threads.
     pub fn get_mut(&mut self, entity: Entity) -> Option<&mut T> {
         if self.bitset.borrow().contains(entity.id()) && self.entities.is_alive(entity) {
             Some(unsafe { self.data.borrow_mut().get_mut(entity.id()) })
@@ -199,7 +227,7 @@ where
     /// immutable components with this which is safe for parallel by default.
     pub fn restrict<'rf>(
         &'rf self,
-    ) -> RestrictedStorage<'rf, 'st, &BitSet, T, &T::Storage, ParallelRestriction> {
+    ) -> RestrictedStorage<'rf, 'st, &BitSet, T, &T::Storage, ImmutableParallelRestriction> {
         RestrictedStorage {
             bitset: &self.data.mask,
             data: &self.data.inner,
@@ -219,7 +247,7 @@ where
     /// bitset for iteration in `Join`.
     pub fn restrict_mut<'rf>(
         &'rf mut self,
-    ) -> RestrictedStorage<'rf, 'st, &BitSet, T, &mut T::Storage, NormalRestriction> {
+    ) -> RestrictedStorage<'rf, 'st, &BitSet, T, &mut T::Storage, SequentialRestriction> {
         let (mask, data) = self.data.open_mut();
         RestrictedStorage {
             bitset: mask,
@@ -234,7 +262,7 @@ where
     /// aside from the current iteration.
     pub fn par_restrict_mut<'rf>(
         &'rf mut self,
-    ) -> RestrictedStorage<'rf, 'st, &BitSet, T, &mut T::Storage, ParallelRestriction> {
+    ) -> RestrictedStorage<'rf, 'st, &BitSet, T, &mut T::Storage, MutableParallelRestriction> {
         let (mask, data) = self.data.open_mut();
         RestrictedStorage {
             bitset: mask,
@@ -306,3 +334,4 @@ where
         (*self).index()
     }
 }
+