Skip to content

Commit 2e3e0e3

Browse files
hymmhymm
committed
nonnull pointer deref safety
1 parent ffc66f1 commit 2e3e0e3

File tree

3 files changed

+114
-35
lines changed

3 files changed

+114
-35
lines changed

‎crates/bevy_ecs/src/bundle/insert.rs‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,10 @@ impl<'w> BundleInserter<'w> {
166166
let mut deferred_world = world.into_deferred();
167167

168168
if insert_mode == InsertMode::Replace {
169+
// SAFETY:
170+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
171+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
172+
// changes to the world, so the data is valid for the lifetime of `Self`
169173
let archetype = archetype.as_ref();
170174
if archetype.has_replace_observer() {
171175
// SAFETY: the REPLACE event_key corresponds to the Replace event's type
@@ -188,11 +192,19 @@ impl<'w> BundleInserter<'w> {
188192
}
189193
}
190194

191-
let table = table.as_mut();
195+
// SAFETY:
196+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
197+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
198+
// changes to the world, so the data is valid for the lifetime of `Self`
199+
let table = unsafe { table.as_mut() };
192200

193-
// SAFETY: Archetype gets borrowed when running the on_replace observers above,
201+
// SAFETY:
202+
// * Archetype gets borrowed when running the on_replace observers above,
194203
// so this reference can only be promoted from shared to &mut down here, after they have been ran
195-
let archetype = archetype.as_mut();
204+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
205+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
206+
// changes to the world, so the data is valid for the lifetime of `Self`
207+
let archetype = unsafe { archetype.as_mut() };
196208

197209
match archetype_move_type {
198210
ArchetypeMoveType::SameArchetype => {
@@ -343,7 +355,11 @@ impl<'w> BundleInserter<'w> {
343355
caller: MaybeLocation,
344356
relationship_hook_mode: RelationshipHookMode,
345357
) -> EntityLocation {
346-
let archetype_after_insert = self.archetype_after_insert.as_ref();
358+
// SAFETY:
359+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
360+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
361+
// changes to the world, so the data is valid for the lifetime of `Self`
362+
let archetype_after_insert = unsafe { self.archetype_after_insert.as_ref() };
347363

348364
let (new_archetype, new_location) = {
349365
// Non-generic prelude extracted to improve compile time by minimizing monomorphized code.
@@ -360,7 +376,12 @@ impl<'w> BundleInserter<'w> {
360376
&mut self.archetype_move_type,
361377
);
362378

363-
self.bundle_info.as_ref().write_components(
379+
// SAFETY:
380+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
381+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
382+
// changes to the world, so the data is valid for the lifetime of `Self`
383+
let bundle_info = unsafe { self.bundle_info.as_ref() };
384+
bundle_info.write_components(
364385
table,
365386
sparse_sets,
366387
archetype_after_insert,

‎crates/bevy_ecs/src/bundle/remove.rs‎

Lines changed: 74 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -142,11 +142,20 @@ impl<'w> BundleRemover<'w> {
142142
// SAFETY: We only keep access to archetype/bundle data.
143143
let mut deferred_world = self.world.into_deferred();
144144
let bundle_components_in_archetype = || {
145-
self.bundle_info
146-
.as_ref()
145+
// SAFETY:
146+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
147+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
148+
// changes to the world, so the data is valid for the lifetime of `Self`
149+
let (bundle_info, old_archetype) =
150+
(self.bundle_info.as_ref(), self.old_archetype.as_ref());
151+
bundle_info
147152
.iter_explicit_components()
148-
.filter(|component_id| self.old_archetype.as_ref().contains(*component_id))
153+
.filter(|component_id| old_archetype.contains(*component_id))
149154
};
155+
// SAFETY:
156+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
157+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
158+
// changes to the world, so the data is valid for the lifetime of `Self`
150159
if self.old_archetype.as_ref().has_replace_observer() {
151160
let components = bundle_components_in_archetype().collect::<Vec<_>>();
152161
// SAFETY: the REPLACE event_key corresponds to the Replace event's type
@@ -160,12 +169,20 @@ impl<'w> BundleRemover<'w> {
160169
);
161170
}
162171
deferred_world.trigger_on_replace(
172+
// SAFETY:
173+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
174+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
175+
// changes to the world, so the data is valid for the lifetime of `Self`
163176
self.old_archetype.as_ref(),
164177
entity,
165178
bundle_components_in_archetype(),
166179
caller,
167180
self.relationship_hook_mode,
168181
);
182+
// SAFETY:
183+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
184+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
185+
// changes to the world, so the data is valid for the lifetime of `Self`
169186
if self.old_archetype.as_ref().has_remove_observer() {
170187
let components = bundle_components_in_archetype().collect::<Vec<_>>();
171188
// SAFETY: the REMOVE event_key corresponds to the Remove event's type
@@ -179,6 +196,10 @@ impl<'w> BundleRemover<'w> {
179196
);
180197
}
181198
deferred_world.trigger_on_remove(
199+
// SAFETY:
200+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
201+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
202+
// changes to the world, so the data is valid for the lifetime of `Self`
182203
self.old_archetype.as_ref(),
183204
entity,
184205
bundle_components_in_archetype(),
@@ -196,18 +217,34 @@ impl<'w> BundleRemover<'w> {
196217
// SAFETY: There is no conflicting access for this scope.
197218
.map(|(old, _)| unsafe { &mut *old.as_ptr() }),
198219
&world.components,
199-
self.bundle_info.as_ref().explicit_components(),
220+
// SAFETY:
221+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
222+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
223+
// changes to the world, so the data is valid for the lifetime of `Self`
224+
unsafe { self.bundle_info.as_ref() }.explicit_components(),
200225
);
201226

202227
// Handle sparse set removes
203-
for component_id in self.bundle_info.as_ref().iter_explicit_components() {
204-
if self.old_archetype.as_ref().contains(component_id) {
228+
// SAFETY:
229+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
230+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
231+
// changes to the world, so the data is valid for the lifetime of `Self`
232+
for component_id in unsafe { self.bundle_info.as_ref() }.iter_explicit_components() {
233+
// SAFETY:
234+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
235+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
236+
// changes to the world, so the data is valid for the lifetime of `Self`
237+
if unsafe { self.old_archetype.as_ref() }.contains(component_id) {
205238
world.removed_components.write(component_id, entity);
206239

207240
// Make sure to drop components stored in sparse sets.
208241
// Dense components are dropped later in `move_to_and_drop_missing_unchecked`.
209242
if let Some(StorageType::SparseSet) =
210-
self.old_archetype.as_ref().get_storage_type(component_id)
243+
// SAFETY:
244+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
245+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
246+
// changes to the world, so the data is valid for the lifetime of `Self`
247+
unsafe { self.old_archetype.as_ref() }.get_storage_type(component_id)
211248
{
212249
world
213250
.storages
@@ -222,10 +259,12 @@ impl<'w> BundleRemover<'w> {
222259
}
223260

224261
// Handle archetype change
225-
let remove_result = self
226-
.old_archetype
227-
.as_mut()
228-
.swap_remove(location.archetype_row);
262+
let remove_result =
263+
// SAFETY:
264+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
265+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
266+
// changes to the world, so the data is valid for the lifetime of `Self`
267+
unsafe { self.old_archetype.as_mut() }.swap_remove(location.archetype_row);
229268
// if an entity was moved into this entity's archetype row, update its archetype row
230269
if let Some(swapped_entity) = remove_result.swapped_entity {
231270
let swapped_location = world.entities.get_spawned(swapped_entity).unwrap();
@@ -244,28 +283,36 @@ impl<'w> BundleRemover<'w> {
244283
// Handle table change
245284
let new_location = if let Some((mut old_table, mut new_table)) = self.old_and_new_table {
246285
let move_result = if needs_drop {
286+
// SAFETY:
287+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
288+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
289+
// changes to the world, so the data is valid for the lifetime of `Self`
290+
let old_table = unsafe { old_table.as_mut() };
291+
247292
// SAFETY: old_table_row exists
248293
unsafe {
249294
old_table
250-
.as_mut()
251295
.move_to_and_drop_missing_unchecked(location.table_row, new_table.as_mut())
252296
}
253297
} else {
298+
// SAFETY:
299+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
300+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
301+
// changes to the world, so the data is valid for the lifetime of `Self`
302+
let (old_table, new_table) = unsafe { (old_table.as_mut(), new_table.as_mut()) };
254303
// SAFETY: old_table_row exists
255304
unsafe {
256-
old_table.as_mut().move_to_and_forget_missing_unchecked(
257-
location.table_row,
258-
new_table.as_mut(),
259-
)
305+
old_table.move_to_and_forget_missing_unchecked(location.table_row, new_table)
260306
}
261307
};
262308

309+
// SAFETY:
310+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
311+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
312+
// changes to the world, so the data is valid for the lifetime of `Self`
313+
let new_archetype = unsafe { self.new_archetype.as_mut() };
263314
// SAFETY: move_result.new_row is a valid position in new_archetype's table
264-
let new_location = unsafe {
265-
self.new_archetype
266-
.as_mut()
267-
.allocate(entity, move_result.new_row)
268-
};
315+
let new_location = unsafe { new_archetype.allocate(entity, move_result.new_row) };
269316

270317
// if an entity was moved into this entity's table row, update its table row
271318
if let Some(swapped_entity) = move_result.swapped_entity {
@@ -286,10 +333,13 @@ impl<'w> BundleRemover<'w> {
286333

287334
new_location
288335
} else {
336+
// SAFETY:
337+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
338+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
339+
// changes to the world, so the data is valid for the lifetime of `Self`
340+
let new_archetype = unsafe { self.new_archetype.as_mut() };
289341
// The tables are the same
290-
self.new_archetype
291-
.as_mut()
292-
.allocate(entity, location.table_row)
342+
new_archetype.allocate(entity, location.table_row)
293343
};
294344

295345
// SAFETY: The entity is valid and has been moved to the new location already.

‎crates/bevy_ecs/src/bundle/spawner.rs‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -94,11 +94,17 @@ impl<'w> BundleSpawner<'w> {
9494
bundle: MovingPtr<'_, T>,
9595
caller: MaybeLocation,
9696
) -> EntityLocation {
97-
// SAFETY: We do not make any structural changes to the archetype graph through self.world so these pointers always remain valid
98-
let bundle_info = self.bundle_info.as_ref();
97+
// SAFETY:
98+
// * Pointer was created from a reference in `Self::new_with_id` and so is `dereferencable`.
99+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
100+
// changes to the world, so the data is valid for the lifetime of `Self`
101+
let bundle_info = unsafe { self.bundle_info.as_ref() };
99102
let location = {
100-
let table = self.table.as_mut();
101-
let archetype = self.archetype.as_mut();
103+
// SAFETY:
104+
// * Pointers are dereferencable because they were created from a reference in `Self::new_with_id`.
105+
// * `Self`'s lifetime is tied to an exclusive reference to `World` and it does not make structural
106+
// changes to the world, so the data is valid for the lifetime of `Self`
107+
let (table, archetype) = unsafe { (self.table.as_mut(), self.archetype.as_mut()) };
102108

103109
// SAFETY: Mutable references do not alias and will be dropped after this block
104110
let (sparse_sets, entities) = {
@@ -126,8 +132,10 @@ impl<'w> BundleSpawner<'w> {
126132

127133
// SAFETY: We have no outstanding mutable references to world as they were dropped
128134
let mut deferred_world = unsafe { self.world.into_deferred() };
129-
// SAFETY: `DeferredWorld` cannot provide mutable access to `Archetypes`.
130-
let archetype = self.archetype.as_ref();
135+
// SAFETY:
136+
// * Dereferencable because it was created from an exclusive reference in `Self::new_with_id`.
137+
// * `DeferredWorld` does not provide mutable access to `Archetypes`, so it is safe to hold a reference to an archetype.
138+
let archetype = unsafe { self.archetype.as_ref() };
131139
// SAFETY: All components in the bundle are guaranteed to exist in the World
132140
// as they must be initialized before creating the BundleInfo.
133141
unsafe {

0 commit comments

Comments
 (0)