From a341e1a570f2cb6e4adccd8d805328b6036735d1 Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Fri, 13 Jan 2023 11:47:31 -0500
Subject: [PATCH 01/23] add `bevy_utils::on_drop`

---
 crates/bevy_utils/src/lib.rs | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/crates/bevy_utils/src/lib.rs b/crates/bevy_utils/src/lib.rs
index 2a522c087900f..b74ea5a73a053 100644
--- a/crates/bevy_utils/src/lib.rs
+++ b/crates/bevy_utils/src/lib.rs
@@ -34,6 +34,7 @@ use std::{
     future::Future,
     hash::{BuildHasher, Hash, Hasher},
     marker::PhantomData,
+    mem::ManuallyDrop,
     ops::Deref,
     pin::Pin,
 };
@@ -233,3 +234,26 @@ impl<K: Hash + Eq + PartialEq + Clone, V> PreHashMapExt<K, V> for PreHashMap<K,
         }
     }
 }
+
+/// A type which calls a function when dropped.
+/// This can be used to ensure that cleanup code is run even in case of a panic.
+pub struct OnDrop<F: FnOnce()> {
+    callback: ManuallyDrop<F>,
+}
+
+impl<F: FnOnce()> OnDrop<F> {
+    /// Returns an object that will invoke the specified callback when dropped.
+    pub fn new(callback: F) -> Self {
+        Self {
+            callback: ManuallyDrop::new(callback),
+        }
+    }
+}
+
+impl<F: FnOnce()> Drop for OnDrop<F> {
+    fn drop(&mut self) {
+        // SAFETY: We may move out of `self`, since this instance can never be observed after it's dropped.
+        let callback = unsafe { ManuallyDrop::take(&mut self.callback) };
+        callback();
+    }
+}

From fd99fc00d050253ccc64e555ff0dcd644e840e39 Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Fri, 13 Jan 2023 11:53:24 -0500
Subject: [PATCH 02/23] simpify ownership for `BlobVec::replace_unchecked`

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index 7b9437f384218..0609c964d239f 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -6,6 +6,7 @@ use std::{
 };
 
 use bevy_ptr::{OwningPtr, Ptr, PtrMut};
+use bevy_utils::OnDrop;
 
 /// A flat, type-erased data storage type
 ///
@@ -158,25 +159,22 @@ impl BlobVec {
         // in the collection), so we get a double drop. To prevent that, we set len to 0 until we're
         // done.
         let old_len = self.len;
-        let ptr = self.get_unchecked_mut(index).promote().as_ptr();
+        let drop = self.drop;
         self.len = 0;
+
+        let old_value = self.get_unchecked_mut(index).promote();
+        let dst_addr = old_value.as_ptr();
+        let src_addr = value.as_ptr();
         // Drop the old value, then write back, justifying the promotion
         // If the drop impl for the old value panics then we run the drop impl for `value` too.
-        if let Some(drop) = self.drop {
-            struct OnDrop<F: FnMut()>(F);
-            impl<F: FnMut()> Drop for OnDrop<F> {
-                fn drop(&mut self) {
-                    (self.0)();
-                }
-            }
-            let value = value.as_ptr();
-            let on_unwind = OnDrop(|| (drop)(OwningPtr::new(NonNull::new_unchecked(value))));
+        if let Some(drop) = drop {
+            let on_unwind = OnDrop::new(|| (drop)(value));
 
-            (drop)(OwningPtr::new(NonNull::new_unchecked(ptr)));
+            (drop)(old_value);
 
             core::mem::forget(on_unwind);
         }
-        std::ptr::copy_nonoverlapping::<u8>(value.as_ptr(), ptr, self.item_layout.size());
+        std::ptr::copy_nonoverlapping::<u8>(src_addr, dst_addr, self.item_layout.size());
         self.len = old_len;
     }
 

From 6b349722b6cf86c533d5a7cd89c5f47222821923 Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Fri, 13 Jan 2023 12:07:24 -0500
Subject: [PATCH 03/23] improve clarity for `BlobVec::replace_unchecked`

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 28 ++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index 0609c964d239f..9c1119c8a36ba 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -154,27 +154,41 @@ impl BlobVec {
     /// [`BlobVec`]'s `item_layout`
     pub unsafe fn replace_unchecked(&mut self, index: usize, value: OwningPtr<'_>) {
         debug_assert!(index < self.len());
-        // If `drop` panics, then when the collection is dropped during stack unwinding, the
-        // collection's `Drop` impl will call `drop` again for the old value (which is still stored
-        // in the collection), so we get a double drop. To prevent that, we set len to 0 until we're
-        // done.
-        let old_len = self.len;
+
         let drop = self.drop;
+
+        // Temporarily set the length to zero, so that uninitialized bytes won't
+        // get observed in case this function panics.
+        let old_len = self.len;
         self.len = 0;
 
+        // Transfer ownership of the old value out of the vector, so it can be dropped.
+        // SAFETY:
+        // * The caller has promized that `index` fits in this vector.
+        // * The storage location will get overwritten with `value` later, which ensures
+        //   that the element will not get observed or double dropped later.
+        // * If a panic occurs, `self.len` will remain `0`, which ensures a double-drop
+        //   does not occur. Instead, all elements will be forgotten.
         let old_value = self.get_unchecked_mut(index).promote();
+
         let dst_addr = old_value.as_ptr();
         let src_addr = value.as_ptr();
-        // Drop the old value, then write back, justifying the promotion
-        // If the drop impl for the old value panics then we run the drop impl for `value` too.
+
         if let Some(drop) = drop {
+            // This closusure will run in case `drop()` panics, which is similar to
+            // the `try ... catch` construct in languages like C++.
+            // This ensures that `value` does not get forgotten in case of a panic.
             let on_unwind = OnDrop::new(|| (drop)(value));
 
             (drop)(old_value);
 
+            // If the above code does not panic, make sure that `value` doesn't get dropped.
             core::mem::forget(on_unwind);
         }
+        // Copy the new value into the vector, overwriting the previous value which has been moved.
         std::ptr::copy_nonoverlapping::<u8>(src_addr, dst_addr, self.item_layout.size());
+
+        // Now that each entry in the vector is fully initialized again, restore its length.
         self.len = old_len;
     }
 

From 0d6fa320080c66e030d0e3b8eec3827068dde79e Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Fri, 13 Jan 2023 12:19:27 -0500
Subject: [PATCH 04/23] add safety comments to `copy_nonoverlapping`

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index 9c1119c8a36ba..cf24422913879 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -186,6 +186,13 @@ impl BlobVec {
             core::mem::forget(on_unwind);
         }
         // Copy the new value into the vector, overwriting the previous value which has been moved.
+        // SAFETY:
+        // - `src_addr` and `dst_addr` were obtained from `OwningPtr`s, which ensures they are
+        //   valid for both reads and writes.
+        // - The data behind `src_addr` will only be dropped if the above branch panics,
+        //   so it must still be initialized and it is safe to transfer ownership into the vector.
+        // - `src_addr` and `dst_addr` were obtained from different memory locations,
+        //   both of which we have exclusive access to, so they are guaranteed not to overlap.
         std::ptr::copy_nonoverlapping::<u8>(src_addr, dst_addr, self.item_layout.size());
 
         // Now that each entry in the vector is fully initialized again, restore its length.

From 2d3e1bfb32ae38bd014c88ecaf89d6e47f1f471e Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Fri, 13 Jan 2023 12:22:26 -0500
Subject: [PATCH 05/23] avoid a `debug_assert`

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index cf24422913879..323c632a103d7 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -156,6 +156,7 @@ impl BlobVec {
         debug_assert!(index < self.len());
 
         let drop = self.drop;
+        let size = self.item_layout.size();
 
         // Temporarily set the length to zero, so that uninitialized bytes won't
         // get observed in case this function panics.
@@ -169,7 +170,7 @@ impl BlobVec {
         //   that the element will not get observed or double dropped later.
         // * If a panic occurs, `self.len` will remain `0`, which ensures a double-drop
         //   does not occur. Instead, all elements will be forgotten.
-        let old_value = self.get_unchecked_mut(index).promote();
+        let old_value = self.get_ptr_mut().byte_add(index * size).promote();
 
         let dst_addr = old_value.as_ptr();
         let src_addr = value.as_ptr();
@@ -193,7 +194,7 @@ impl BlobVec {
         //   so it must still be initialized and it is safe to transfer ownership into the vector.
         // - `src_addr` and `dst_addr` were obtained from different memory locations,
         //   both of which we have exclusive access to, so they are guaranteed not to overlap.
-        std::ptr::copy_nonoverlapping::<u8>(src_addr, dst_addr, self.item_layout.size());
+        std::ptr::copy_nonoverlapping::<u8>(src_addr, dst_addr, size);
 
         // Now that each entry in the vector is fully initialized again, restore its length.
         self.len = old_len;

From 27bd505e6d5a92977cd63f4f0f626e6ccc579492 Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Fri, 13 Jan 2023 14:01:12 -0500
Subject: [PATCH 06/23] remove unnecessary parentheses

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index 323c632a103d7..47c6fc8d0a1fd 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -179,9 +179,9 @@ impl BlobVec {
             // This closusure will run in case `drop()` panics, which is similar to
             // the `try ... catch` construct in languages like C++.
             // This ensures that `value` does not get forgotten in case of a panic.
-            let on_unwind = OnDrop::new(|| (drop)(value));
+            let on_unwind = OnDrop::new(|| drop(value));
 
-            (drop)(old_value);
+            drop(old_value);
 
             // If the above code does not panic, make sure that `value` doesn't get dropped.
             core::mem::forget(on_unwind);

From 542a820a4c4ee49461126335bb0955790f26e1ae Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Fri, 13 Jan 2023 23:30:12 -0500
Subject: [PATCH 07/23] simplify pointer names

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index 47c6fc8d0a1fd..cf9353b606410 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -172,8 +172,8 @@ impl BlobVec {
         //   does not occur. Instead, all elements will be forgotten.
         let old_value = self.get_ptr_mut().byte_add(index * size).promote();
 
-        let dst_addr = old_value.as_ptr();
-        let src_addr = value.as_ptr();
+        let dst = old_value.as_ptr();
+        let src = value.as_ptr();
 
         if let Some(drop) = drop {
             // This closusure will run in case `drop()` panics, which is similar to
@@ -188,13 +188,13 @@ impl BlobVec {
         }
         // Copy the new value into the vector, overwriting the previous value which has been moved.
         // SAFETY:
-        // - `src_addr` and `dst_addr` were obtained from `OwningPtr`s, which ensures they are
+        // - `src` and `dst` were obtained from `OwningPtr`s, which ensures they are
         //   valid for both reads and writes.
-        // - The data behind `src_addr` will only be dropped if the above branch panics,
+        // - The data behind `src` will only be dropped if the above branch panics,
         //   so it must still be initialized and it is safe to transfer ownership into the vector.
-        // - `src_addr` and `dst_addr` were obtained from different memory locations,
+        // - `src` and `dst` were obtained from different memory locations,
         //   both of which we have exclusive access to, so they are guaranteed not to overlap.
-        std::ptr::copy_nonoverlapping::<u8>(src_addr, dst_addr, size);
+        std::ptr::copy_nonoverlapping::<u8>(src, dst, size);
 
         // Now that each entry in the vector is fully initialized again, restore its length.
         self.len = old_len;

From f5b9267f512cd464b6d29e1b5fd757990a5cd1b6 Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Fri, 13 Jan 2023 23:31:05 -0500
Subject: [PATCH 08/23] 'promized' -> 'ensured'

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index cf9353b606410..4d4cee58b41f4 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -165,7 +165,7 @@ impl BlobVec {
 
         // Transfer ownership of the old value out of the vector, so it can be dropped.
         // SAFETY:
-        // * The caller has promized that `index` fits in this vector.
+        // * The caller ensures that `index` fits in this vector.
         // * The storage location will get overwritten with `value` later, which ensures
         //   that the element will not get observed or double dropped later.
         // * If a panic occurs, `self.len` will remain `0`, which ensures a double-drop

From 83bebebfa2a5a3f9cefa5d0ef9a83750afe53c77 Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Fri, 13 Jan 2023 23:45:14 -0500
Subject: [PATCH 09/23] 'data' -> 'value'

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index 4d4cee58b41f4..bb1f9f68895de 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -190,7 +190,7 @@ impl BlobVec {
         // SAFETY:
         // - `src` and `dst` were obtained from `OwningPtr`s, which ensures they are
         //   valid for both reads and writes.
-        // - The data behind `src` will only be dropped if the above branch panics,
+        // - The value behind `src` will only be dropped if the above branch panics,
         //   so it must still be initialized and it is safe to transfer ownership into the vector.
         // - `src` and `dst` were obtained from different memory locations,
         //   both of which we have exclusive access to, so they are guaranteed not to overlap.

From 0552eb4a8e740d54b7ca360b780643cd95d9e3a1 Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Sat, 14 Jan 2023 00:08:27 -0500
Subject: [PATCH 10/23] branch the function earlier

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 46 ++++++++++++++-----------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index bb1f9f68895de..b642e9520278c 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -155,27 +155,28 @@ impl BlobVec {
     pub unsafe fn replace_unchecked(&mut self, index: usize, value: OwningPtr<'_>) {
         debug_assert!(index < self.len());
 
-        let drop = self.drop;
         let size = self.item_layout.size();
 
-        // Temporarily set the length to zero, so that uninitialized bytes won't
-        // get observed in case this function panics.
-        let old_len = self.len;
-        self.len = 0;
-
-        // Transfer ownership of the old value out of the vector, so it can be dropped.
-        // SAFETY:
-        // * The caller ensures that `index` fits in this vector.
-        // * The storage location will get overwritten with `value` later, which ensures
-        //   that the element will not get observed or double dropped later.
-        // * If a panic occurs, `self.len` will remain `0`, which ensures a double-drop
-        //   does not occur. Instead, all elements will be forgotten.
-        let old_value = self.get_ptr_mut().byte_add(index * size).promote();
-
-        let dst = old_value.as_ptr();
+        // Pointer to the value in the vector that will get replaced.
+        let dst;
         let src = value.as_ptr();
 
-        if let Some(drop) = drop {
+        if let Some(drop) = self.drop {
+            // Temporarily set the length to zero, so that uninitialized bytes won't
+            // get observed in case a panic causes this function to exit early.
+            let old_len = self.len;
+            self.len = 0;
+
+            // Transfer ownership of the old value out of the vector, so it can be dropped.
+            // SAFETY:
+            // * The caller ensures that `index` fits in this vector.
+            // * The storage location will get overwritten with `value` later, which ensures
+            //   that the element will not get observed or double dropped later.
+            // * If a panic occurs, `self.len` will remain `0`, which ensures a double-drop
+            //   does not occur. Instead, all elements will be forgotten.
+            let old_value = self.get_ptr_mut().byte_add(index * size).promote();
+            dst = old_value.as_ptr();
+
             // This closusure will run in case `drop()` panics, which is similar to
             // the `try ... catch` construct in languages like C++.
             // This ensures that `value` does not get forgotten in case of a panic.
@@ -185,7 +186,15 @@ impl BlobVec {
 
             // If the above code does not panic, make sure that `value` doesn't get dropped.
             core::mem::forget(on_unwind);
+
+            // Make the vector's contents observable again, since panics are no longer possible.
+            self.len = old_len;
+        } else {
+            // SAFETY:
+            // - The caller ensures that `index` fits in this vector.
+            dst = self.get_ptr_mut().as_ptr().add(index * size);
         }
+
         // Copy the new value into the vector, overwriting the previous value which has been moved.
         // SAFETY:
         // - `src` and `dst` were obtained from `OwningPtr`s, which ensures they are
@@ -195,9 +204,6 @@ impl BlobVec {
         // - `src` and `dst` were obtained from different memory locations,
         //   both of which we have exclusive access to, so they are guaranteed not to overlap.
         std::ptr::copy_nonoverlapping::<u8>(src, dst, size);
-
-        // Now that each entry in the vector is fully initialized again, restore its length.
-        self.len = old_len;
     }
 
     /// Pushes a value to the [`BlobVec`].

From 0e97ee0276747b84944cc95452578484f457e765 Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Sat, 14 Jan 2023 00:08:46 -0500
Subject: [PATCH 11/23] use consistent bullet points

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index b642e9520278c..bb3863dfc5ec4 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -169,10 +169,10 @@ impl BlobVec {
 
             // Transfer ownership of the old value out of the vector, so it can be dropped.
             // SAFETY:
-            // * The caller ensures that `index` fits in this vector.
-            // * The storage location will get overwritten with `value` later, which ensures
+            // - The caller ensures that `index` fits in this vector.
+            // - The storage location will get overwritten with `value` later, which ensures
             //   that the element will not get observed or double dropped later.
-            // * If a panic occurs, `self.len` will remain `0`, which ensures a double-drop
+            // - If a panic occurs, `self.len` will remain `0`, which ensures a double-drop
             //   does not occur. Instead, all elements will be forgotten.
             let old_value = self.get_ptr_mut().byte_add(index * size).promote();
             dst = old_value.as_ptr();

From da86e26fa31240b40c3294994ea77dcf2a8fa39f Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Sat, 14 Jan 2023 00:09:03 -0500
Subject: [PATCH 12/23] add a comment about alignment

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index bb3863dfc5ec4..39ca3f91b2a6d 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -170,6 +170,8 @@ impl BlobVec {
             // Transfer ownership of the old value out of the vector, so it can be dropped.
             // SAFETY:
             // - The caller ensures that `index` fits in this vector.
+            // - `size` must be a multiple of the erased type's alignment,
+            //   so adding a multiple of `size` will preserve alignment.
             // - The storage location will get overwritten with `value` later, which ensures
             //   that the element will not get observed or double dropped later.
             // - If a panic occurs, `self.len` will remain `0`, which ensures a double-drop

From 4f03b28eaa5c800d6fd9bc311d2e435aadf4d8c6 Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Sun, 15 Jan 2023 08:16:11 -0500
Subject: [PATCH 13/23] Apply suggestions from code review

Co-authored-by: Boxy <supbscripter@gmail.com>
Co-authored-by: James Liu <contact@jamessliu.com>
---
 crates/bevy_ecs/src/storage/blob_vec.rs | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index 39ca3f91b2a6d..559046d76f767 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -162,8 +162,9 @@ impl BlobVec {
         let src = value.as_ptr();
 
         if let Some(drop) = self.drop {
-            // Temporarily set the length to zero, so that uninitialized bytes won't
-            // get observed in case a panic causes this function to exit early.
+            // Temporarily set the length to zero, so that if `drop` panics the caller
+            // will not be left with a `BlobVec` containing a dropped element within
+            // its initialized range.
             let old_len = self.len;
             self.len = 0;
 
@@ -179,7 +180,7 @@ impl BlobVec {
             let old_value = self.get_ptr_mut().byte_add(index * size).promote();
             dst = old_value.as_ptr();
 
-            // This closusure will run in case `drop()` panics, which is similar to
+            // This closure will run in case `drop()` panics, which is similar to
             // the `try ... catch` construct in languages like C++.
             // This ensures that `value` does not get forgotten in case of a panic.
             let on_unwind = OnDrop::new(|| drop(value));

From 49a8e188da644237d4d97e3d208ebbc2a01f891e Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Sun, 15 Jan 2023 08:39:46 -0500
Subject: [PATCH 14/23] make a comment more compact

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index 559046d76f767..22bfd1e421621 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -193,8 +193,7 @@ impl BlobVec {
             // Make the vector's contents observable again, since panics are no longer possible.
             self.len = old_len;
         } else {
-            // SAFETY:
-            // - The caller ensures that `index` fits in this vector.
+            // SAFETY: The caller ensures that `index` fits in this vector.
             dst = self.get_ptr_mut().as_ptr().add(index * size);
         }
 

From 6cfbeef880d30a51b969edf9bb0b6e6732c9ec2b Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Sun, 15 Jan 2023 08:54:07 -0500
Subject: [PATCH 15/23] add conversions from lifetimed pointers to `NonNull`

---
 crates/bevy_ptr/src/lib.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs
index d793d62606438..39d26fec23637 100644
--- a/crates/bevy_ptr/src/lib.rs
+++ b/crates/bevy_ptr/src/lib.rs
@@ -78,6 +78,12 @@ macro_rules! impl_ptr {
             }
         }
 
+        impl<'a, A: IsAligned> From<$ptr<'a, A>> for NonNull<u8> {
+            fn from(ptr: $ptr<'a, A>) -> Self {
+                ptr.0
+            }
+        }
+
         impl<A: IsAligned> $ptr<'_, A> {
             /// Calculates the offset from a pointer.
             /// As the pointer is type-erased, there is no size information available. The provided

From 6a1af0270144644023860867789585374f55104d Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Sun, 15 Jan 2023 08:54:56 -0500
Subject: [PATCH 16/23] deduplicate pointer code

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index 22bfd1e421621..1d53914ac32fd 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -158,7 +158,11 @@ impl BlobVec {
         let size = self.item_layout.size();
 
         // Pointer to the value in the vector that will get replaced.
-        let dst;
+        // SAFETY:
+        // - The caller ensures that `index` fits in this vector.
+        // - `size` must be a multiple of the erased type's alignment,
+        //   so adding a multiple of `size` will preserve alignment.
+        let dst = NonNull::from(self.get_ptr_mut().byte_add(index * size));
         let src = value.as_ptr();
 
         if let Some(drop) = self.drop {
@@ -170,15 +174,11 @@ impl BlobVec {
 
             // Transfer ownership of the old value out of the vector, so it can be dropped.
             // SAFETY:
-            // - The caller ensures that `index` fits in this vector.
-            // - `size` must be a multiple of the erased type's alignment,
-            //   so adding a multiple of `size` will preserve alignment.
             // - The storage location will get overwritten with `value` later, which ensures
             //   that the element will not get observed or double dropped later.
             // - If a panic occurs, `self.len` will remain `0`, which ensures a double-drop
             //   does not occur. Instead, all elements will be forgotten.
-            let old_value = self.get_ptr_mut().byte_add(index * size).promote();
-            dst = old_value.as_ptr();
+            let old_value = OwningPtr::new(dst);
 
             // This closure will run in case `drop()` panics, which is similar to
             // the `try ... catch` construct in languages like C++.
@@ -192,9 +192,6 @@ impl BlobVec {
 
             // Make the vector's contents observable again, since panics are no longer possible.
             self.len = old_len;
-        } else {
-            // SAFETY: The caller ensures that `index` fits in this vector.
-            dst = self.get_ptr_mut().as_ptr().add(index * size);
         }
 
         // Copy the new value into the vector, overwriting the previous value which has been moved.
@@ -205,7 +202,7 @@ impl BlobVec {
         //   so it must still be initialized and it is safe to transfer ownership into the vector.
         // - `src` and `dst` were obtained from different memory locations,
         //   both of which we have exclusive access to, so they are guaranteed not to overlap.
-        std::ptr::copy_nonoverlapping::<u8>(src, dst, size);
+        std::ptr::copy_nonoverlapping::<u8>(src, dst.as_ptr(), size);
     }
 
     /// Pushes a value to the [`BlobVec`].

From fd2e89da4a103e00dda7b4e753eee9e10affb943 Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Sun, 15 Jan 2023 09:18:24 -0500
Subject: [PATCH 17/23] add a doctest to `OnDrop`

---
 crates/bevy_utils/src/lib.rs | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/crates/bevy_utils/src/lib.rs b/crates/bevy_utils/src/lib.rs
index b74ea5a73a053..6fada8bf98967 100644
--- a/crates/bevy_utils/src/lib.rs
+++ b/crates/bevy_utils/src/lib.rs
@@ -237,6 +237,34 @@ impl<K: Hash + Eq + PartialEq + Clone, V> PreHashMapExt<K, V> for PreHashMap<K,
 
 /// A type which calls a function when dropped.
 /// This can be used to ensure that cleanup code is run even in case of a panic.
+///
+/// # Examples
+///
+/// ```
+/// # use bevy_utils::OnDrop;
+/// # fn test_panic(do_panic: bool, log: impl FnOnce(&str)) {
+/// // This will print a message when the variable `_catch` gets dropped,
+/// // even if a panic occurs before we reach the end of this scope.
+/// // This is similar to a `try ... catch` block in languages such as C++.
+/// let _catch = OnDrop::new(|| log("Oops, a panic occured and this function didn't complete!"));
+///
+/// // Some code that may panic...
+/// // ...
+/// # if do_panic { panic!() }
+///
+/// // Make sure the message only gets printed if a panic occurs.
+/// // If we remove this line, then the message will be printed regardless of whether a panic occurs
+/// // -- similar to a `try ... finally` block.
+/// std::mem::forget(_catch);
+/// # }
+/// #
+/// # test_panic(false, |_| unreachable!());
+/// # let mut did_log = false;
+/// # std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+/// #   test_panic(true, |_| did_log = true);
+/// # }));
+/// # assert!(did_log);
+/// ```
 pub struct OnDrop<F: FnOnce()> {
     callback: ManuallyDrop<F>,
 }

From 0ba22ab94e5e56aee776e755630d24a30c984c6b Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Sun, 15 Jan 2023 09:22:23 -0500
Subject: [PATCH 18/23] simplify a comment for `OnDrop`

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index 1d53914ac32fd..e7415032156a8 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -180,9 +180,8 @@ impl BlobVec {
             //   does not occur. Instead, all elements will be forgotten.
             let old_value = OwningPtr::new(dst);
 
-            // This closure will run in case `drop()` panics, which is similar to
-            // the `try ... catch` construct in languages like C++.
-            // This ensures that `value` does not get forgotten in case of a panic.
+            // This closure will run in case `drop()` panics,
+            // which ensures that `value` does not get forgotten.
             let on_unwind = OnDrop::new(|| drop(value));
 
             drop(old_value);

From 86aa36897a7a4e4886f6f3adf16d57cbf87e05e9 Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Sun, 15 Jan 2023 09:23:02 -0500
Subject: [PATCH 19/23] add a comment about a missing safety invariant

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index e7415032156a8..d077f1077016b 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -174,6 +174,8 @@ impl BlobVec {
 
             // Transfer ownership of the old value out of the vector, so it can be dropped.
             // SAFETY:
+            // - `dst` was obtained from a `PtrMut` in this vector, which ensures it is non-null,
+            //   well-aligned for the underlying type, and has proper provenance.
             // - The storage location will get overwritten with `value` later, which ensures
             //   that the element will not get observed or double dropped later.
             // - If a panic occurs, `self.len` will remain `0`, which ensures a double-drop

From 80c6771333a4bff25007d241fb9b5b1f38031bfc Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Sun, 15 Jan 2023 09:35:27 -0500
Subject: [PATCH 20/23] modify a slightly inaccurate comment

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index d077f1077016b..8a86357284268 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -195,7 +195,7 @@ impl BlobVec {
             self.len = old_len;
         }
 
-        // Copy the new value into the vector, overwriting the previous value which has been moved.
+        // Copy the new value into the vector, overwriting the previous value.
         // SAFETY:
         // - `src` and `dst` were obtained from `OwningPtr`s, which ensures they are
         //   valid for both reads and writes.

From 5baa23734ede86fbd8d9eff851a2757c85c7d1c9 Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Sun, 15 Jan 2023 09:55:09 -0500
Subject: [PATCH 21/23] add a note about unwinding

---
 crates/bevy_utils/src/lib.rs | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/crates/bevy_utils/src/lib.rs b/crates/bevy_utils/src/lib.rs
index 6fada8bf98967..e54065eb5f6bf 100644
--- a/crates/bevy_utils/src/lib.rs
+++ b/crates/bevy_utils/src/lib.rs
@@ -238,6 +238,10 @@ impl<K: Hash + Eq + PartialEq + Clone, V> PreHashMapExt<K, V> for PreHashMap<K,
 /// A type which calls a function when dropped.
 /// This can be used to ensure that cleanup code is run even in case of a panic.
 ///
+/// Note that this only works for panics that [unwind](https://doc.rust-lang.org/nomicon/unwinding.html)
+/// -- any code within `OnDrop` will be skipped if a panic does not unwind.
+/// In most cases, this will just work.
+///
 /// # Examples
 ///
 /// ```

From 0ed0b2ad7795f39c1ecd971d17b5bfd09662d300 Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Sun, 15 Jan 2023 11:14:33 -0500
Subject: [PATCH 22/23] simplify an unsafe expression

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index 8a86357284268..ee8152af0a94a 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -155,14 +155,9 @@ impl BlobVec {
     pub unsafe fn replace_unchecked(&mut self, index: usize, value: OwningPtr<'_>) {
         debug_assert!(index < self.len());
 
-        let size = self.item_layout.size();
-
         // Pointer to the value in the vector that will get replaced.
-        // SAFETY:
-        // - The caller ensures that `index` fits in this vector.
-        // - `size` must be a multiple of the erased type's alignment,
-        //   so adding a multiple of `size` will preserve alignment.
-        let dst = NonNull::from(self.get_ptr_mut().byte_add(index * size));
+        // SAFETY: The caller ensures that `index` fits in this vector.
+        let dst = NonNull::from(self.get_unchecked_mut(index));
         let src = value.as_ptr();
 
         if let Some(drop) = self.drop {
@@ -203,7 +198,7 @@ impl BlobVec {
         //   so it must still be initialized and it is safe to transfer ownership into the vector.
         // - `src` and `dst` were obtained from different memory locations,
         //   both of which we have exclusive access to, so they are guaranteed not to overlap.
-        std::ptr::copy_nonoverlapping::<u8>(src, dst.as_ptr(), size);
+        std::ptr::copy_nonoverlapping::<u8>(src, dst.as_ptr(), self.item_layout.size());
     }
 
     /// Pushes a value to the [`BlobVec`].

From c21fc09ce5eba4b7892dbc0209bc4a49659e886b Mon Sep 17 00:00:00 2001
From: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Date: Sun, 15 Jan 2023 11:23:36 -0500
Subject: [PATCH 23/23] make pointer names more verbose

---
 crates/bevy_ecs/src/storage/blob_vec.rs | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs
index ee8152af0a94a..aa7b3718aba08 100644
--- a/crates/bevy_ecs/src/storage/blob_vec.rs
+++ b/crates/bevy_ecs/src/storage/blob_vec.rs
@@ -157,8 +157,8 @@ impl BlobVec {
 
         // Pointer to the value in the vector that will get replaced.
         // SAFETY: The caller ensures that `index` fits in this vector.
-        let dst = NonNull::from(self.get_unchecked_mut(index));
-        let src = value.as_ptr();
+        let destination = NonNull::from(self.get_unchecked_mut(index));
+        let source = value.as_ptr();
 
         if let Some(drop) = self.drop {
             // Temporarily set the length to zero, so that if `drop` panics the caller
@@ -169,13 +169,13 @@ impl BlobVec {
 
             // Transfer ownership of the old value out of the vector, so it can be dropped.
             // SAFETY:
-            // - `dst` was obtained from a `PtrMut` in this vector, which ensures it is non-null,
+            // - `destination` was obtained from a `PtrMut` in this vector, which ensures it is non-null,
             //   well-aligned for the underlying type, and has proper provenance.
             // - The storage location will get overwritten with `value` later, which ensures
             //   that the element will not get observed or double dropped later.
             // - If a panic occurs, `self.len` will remain `0`, which ensures a double-drop
             //   does not occur. Instead, all elements will be forgotten.
-            let old_value = OwningPtr::new(dst);
+            let old_value = OwningPtr::new(destination);
 
             // This closure will run in case `drop()` panics,
             // which ensures that `value` does not get forgotten.
@@ -192,13 +192,13 @@ impl BlobVec {
 
         // Copy the new value into the vector, overwriting the previous value.
         // SAFETY:
-        // - `src` and `dst` were obtained from `OwningPtr`s, which ensures they are
+        // - `source` and `destination` were obtained from `OwningPtr`s, which ensures they are
         //   valid for both reads and writes.
-        // - The value behind `src` will only be dropped if the above branch panics,
+        // - The value behind `source` will only be dropped if the above branch panics,
         //   so it must still be initialized and it is safe to transfer ownership into the vector.
-        // - `src` and `dst` were obtained from different memory locations,
+        // - `source` and `destination` were obtained from different memory locations,
         //   both of which we have exclusive access to, so they are guaranteed not to overlap.
-        std::ptr::copy_nonoverlapping::<u8>(src, dst.as_ptr(), self.item_layout.size());
+        std::ptr::copy_nonoverlapping::<u8>(source, destination.as_ptr(), self.item_layout.size());
     }
 
     /// Pushes a value to the [`BlobVec`].