Skip to content

Commit

Permalink
bevy_ecs address trivial cases of unsafe_op_in_unsafe_fn (#11861)
Browse files Browse the repository at this point in the history
# Objective

- Part of #11590
- Fix `unsafe_op_in_unsafe_fn` for trivial cases in bevy_ecs

## Solution

Fix `unsafe_op_in_unsafe_fn` in bevy_ecs for trivial cases, i.e., add an
`unsafe` block when the safety comment already exists or add a comment
like "The invariants are uphold by the caller".

---------

Co-authored-by: James Liu <contact@jamessliu.com>
  • Loading branch information
tguichaoua and james7132 committed Feb 22, 2024
1 parent a491bce commit 33c7a22
Show file tree
Hide file tree
Showing 22 changed files with 459 additions and 263 deletions.
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/archetype.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ impl BundleComponentStatus for AddBundle {
#[inline]
unsafe fn get_status(&self, index: usize) -> ComponentStatus {
// SAFETY: caller has ensured index is a valid bundle index for this bundle
*self.bundle_status.get_unchecked(index)
unsafe { *self.bundle_status.get_unchecked(index) }
}
}

Expand Down
23 changes: 15 additions & 8 deletions crates/bevy_ecs/src/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,9 @@ unsafe impl<C: Component> Bundle for C {
F: for<'a> FnMut(&'a mut T) -> OwningPtr<'a>,
Self: Sized,
{
let ptr = func(ctx);
// Safety: The id given in `component_ids` is for `Self`
func(ctx).read()
unsafe { ptr.read() }
}
}

Expand Down Expand Up @@ -224,9 +225,10 @@ macro_rules! tuple_impl {
where
F: FnMut(&mut T) -> OwningPtr<'_>
{
// Rust guarantees that tuple calls are evaluated 'left to right'.
#[allow(unused_unsafe)]
// SAFETY: Rust guarantees that tuple calls are evaluated 'left to right'.
// https://doc.rust-lang.org/reference/expressions.html#evaluation-order-of-operands
($(<$name as Bundle>::from_components(ctx, func),)*)
unsafe { ($(<$name as Bundle>::from_components(ctx, func),)*) }
}
}

Expand Down Expand Up @@ -466,7 +468,8 @@ impl BundleInfo {
// the target table contains the component.
unsafe { table.get_column_mut(component_id).debug_checked_unwrap() };
// SAFETY: bundle_component is a valid index for this bundle
match bundle_component_status.get_status(bundle_component) {
let status = unsafe { bundle_component_status.get_status(bundle_component) };
match status {
ComponentStatus::Added => {
column.initialize(table_row, component_ptr, change_tick);
}
Expand Down Expand Up @@ -702,9 +705,11 @@ impl<'a, 'b> BundleInserter<'a, 'b> {
new_archetype
} else {
// SAFETY: the only two borrowed archetypes are above and we just did collision checks
&mut *self
.archetypes_ptr
.add(swapped_location.archetype_id.index())
unsafe {
&mut *self
.archetypes_ptr
.add(swapped_location.archetype_id.index())
}
};

self.entities.set(
Expand Down Expand Up @@ -787,7 +792,9 @@ impl<'a, 'b> BundleSpawner<'a, 'b> {
pub unsafe fn spawn<T: Bundle>(&mut self, bundle: T) -> Entity {
let entity = self.entities.alloc();
// SAFETY: entity is allocated (but non-existent), `T` matches this BundleInfo's type
self.spawn_non_existent(entity, bundle);
unsafe {
self.spawn_non_existent(entity, bundle);
}
entity
}
}
Expand Down
15 changes: 10 additions & 5 deletions crates/bevy_ecs/src/change_detection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -427,8 +427,10 @@ impl<'w> Ticks<'w> {
this_run: Tick,
) -> Self {
Self {
added: cells.added.deref(),
changed: cells.changed.deref(),
// SAFETY: Caller ensures there is no mutable access to the cell.
added: unsafe { cells.added.deref() },
// SAFETY: Caller ensures there is no mutable access to the cell.
changed: unsafe { cells.changed.deref() },
last_run,
this_run,
}
Expand All @@ -452,8 +454,10 @@ impl<'w> TicksMut<'w> {
this_run: Tick,
) -> Self {
Self {
added: cells.added.deref_mut(),
changed: cells.changed.deref_mut(),
// SAFETY: Caller ensures there is no alias to the cell.
added: unsafe { cells.added.deref_mut() },
// SAFETY: Caller ensures there is no alias to the cell.
changed: unsafe { cells.changed.deref_mut() },
last_run,
this_run,
}
Expand Down Expand Up @@ -879,7 +883,8 @@ impl<'w> MutUntyped<'w> {
/// - `T` must be the erased pointee type for this [`MutUntyped`].
pub unsafe fn with_type<T>(self) -> Mut<'w, T> {
Mut {
value: self.value.deref_mut(),
// SAFETY: `value` is `Aligned` and caller ensures the pointee type is `T`.
value: unsafe { self.value.deref_mut() },
ticks: self.ticks,
}
}
Expand Down
18 changes: 13 additions & 5 deletions crates/bevy_ecs/src/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,9 +357,14 @@ impl std::fmt::Debug for ComponentDescriptor {
}

impl ComponentDescriptor {
// SAFETY: The pointer points to a valid value of type `T` and it is safe to drop this value.
/// # SAFETY
///
/// `x` must points to a valid value of type `T`.
unsafe fn drop_ptr<T>(x: OwningPtr<'_>) {
x.drop_as::<T>();
// SAFETY: Contract is required to be upheld by the caller.
unsafe {
x.drop_as::<T>();
}
}

/// Create a new `ComponentDescriptor` for the type `T`.
Expand Down Expand Up @@ -542,7 +547,8 @@ impl Components {
#[inline]
pub unsafe fn get_info_unchecked(&self, id: ComponentId) -> &ComponentInfo {
debug_assert!(id.index() < self.components.len());
self.components.get_unchecked(id.0)
// SAFETY: The caller ensures `id` is valid.
unsafe { self.components.get_unchecked(id.0) }
}

/// Type-erased equivalent of [`Components::component_id()`].
Expand Down Expand Up @@ -763,8 +769,10 @@ impl<'a> TickCells<'a> {
#[inline]
pub(crate) unsafe fn read(&self) -> ComponentTicks {
ComponentTicks {
added: self.added.read(),
changed: self.changed.read(),
// SAFETY: The callers uphold the invariants for `read`.
added: unsafe { self.added.read() },
// SAFETY: The callers uphold the invariants for `read`.
changed: unsafe { self.changed.read() },
}
}
}
Expand Down
14 changes: 10 additions & 4 deletions crates/bevy_ecs/src/entity/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -741,7 +741,8 @@ impl Entities {
#[inline]
pub(crate) unsafe fn set(&mut self, index: u32, location: EntityLocation) {
// SAFETY: Caller guarantees that `index` a valid entity index
self.meta.get_unchecked_mut(index as usize).location = location;
let meta = unsafe { self.meta.get_unchecked_mut(index as usize) };
meta.location = location;
}

/// Increments the `generation` of a freed [`Entity`]. The next entity ID allocated with this
Expand Down Expand Up @@ -848,9 +849,14 @@ impl Entities {
let free_cursor = self.free_cursor.get_mut();
*free_cursor = 0;
self.meta.reserve(count);
// the EntityMeta struct only contains integers, and it is valid to have all bytes set to u8::MAX
self.meta.as_mut_ptr().write_bytes(u8::MAX, count);
self.meta.set_len(count);
// SAFETY: The EntityMeta struct only contains integers, and it is valid to have all bytes set to u8::MAX
unsafe {
self.meta.as_mut_ptr().write_bytes(u8::MAX, count);
}
// SAFETY: We have reserved `count` elements above and we have initialized values from index 0 to `count`.
unsafe {
self.meta.set_len(count);
}

self.len = count as u32;
}
Expand Down
Loading

0 comments on commit 33c7a22

Please sign in to comment.