Skip to content

Doc trivial ecs unsafe #22014

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alice-i-cecile merged 1 commit into from
Dec 15, 2025
+186 −95
Merged

Doc trivial ecs unsafe #22014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions crates/bevy_ecs/src/bundle/impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,14 @@ macro_rules! tuple_impl {
bevy_ptr::deconstruct_moving_ptr!({
let tuple { $($index: $alias,)* } = ptr;
});
#[allow(
unused_unsafe,
reason = "Zero-length tuples will generate a function body equivalatent to (); however, this macro is meant for all applicable tuples, and as such it makes no sense to rewrite it just for that case."
)]
// SAFETY: Caller ensures requirements for calling `get_components` are met.
$( $name::get_components($alias, func); )*
unsafe {
$( $name::get_components($alias, func); )*
}
}

#[allow(
Expand All @@ -151,8 +157,14 @@ macro_rules! tuple_impl {
bevy_ptr::deconstruct_moving_ptr!({
let MaybeUninit::<tuple> { $($index: $alias,)* } = ptr;
});
#[allow(
unused_unsafe,
reason = "Zero-length tuples will generate a function body equivalent to `()`; however, this macro is meant for all applicable tuples, and as such it makes no sense to rewrite it just for that case."
)]
// SAFETY: Caller ensures requirements for calling `apply_effect` are met.
$( $name::apply_effect($alias, entity); )*
unsafe {
$( $name::apply_effect($alias, entity); )*
}
}
}

Expand Down
16 changes: 9 additions & 7 deletions crates/bevy_ecs/src/bundle/insert.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,13 +535,15 @@ impl BundleInfo {
};
};
// SAFETY: ids in self must be valid
let (new_archetype_id, is_new_created) = archetypes.get_id_or_insert(
components,
observers,
table_id,
table_components,
sparse_set_components,
);
let (new_archetype_id, is_new_created) = unsafe {
archetypes.get_id_or_insert(
components,
observers,
table_id,
table_components,
sparse_set_components,
)
};

// Add an edge from the old archetype to the new archetype.
archetypes[archetype_id]
Expand Down
23 changes: 13 additions & 10 deletions crates/bevy_ecs/src/component/required.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,11 +390,12 @@ impl Components {
let old_required_count = required_components.all.len();

// SAFETY: the caller guarantees that `requiree` is valid in `self`.
self.required_components_scope(requiree, |this, required_components| {
// SAFETY: the caller guarantees that `required` is valid for type `R` in `self`
unsafe { required_components.register_by_id(required, this, constructor) };
});

unsafe {
self.required_components_scope(requiree, |this, required_components| {
// SAFETY: the caller guarantees that `required` is valid for type `R` in `self`
required_components.register_by_id(required, this, constructor);
});
}
// Third step: update the required components and required_by of all the indirect requirements/requirees.

// Borrow again otherwise it conflicts with the `self.required_components_scope` call.
Expand Down Expand Up @@ -435,11 +436,13 @@ impl Components {
// Skip the first one (requiree) because we already updates it.
for &indirect_requiree in &new_requiree_components[1..] {
// SAFETY: `indirect_requiree` comes from `self` so it must be valid.
self.required_components_scope(indirect_requiree, |this, required_components| {
// Rebuild the inherited required components.
// SAFETY: `required_components` comes from `self`, so all its components must have be valid in `self`.
unsafe { required_components.rebuild_inherited_required_components(this) };
});
unsafe {
self.required_components_scope(indirect_requiree, |this, required_components| {
// Rebuild the inherited required components.
// SAFETY: `required_components` comes from `self`, so all its components must have be valid in `self`.
required_components.rebuild_inherited_required_components(this);
});
}
}

// Update the `required_by` of all the components that were newly required (directly or indirectly).
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/entity/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -993,7 +993,7 @@ impl Entities {
) -> Option<EntityLocation> {
self.ensure_index_index_is_valid(index);
// SAFETY: We just did `ensure_index`
self.update_existing_location(index, location)
unsafe { self.update_existing_location(index, location) }
}

/// Ensures the index is within the bounds of [`Self::meta`], expanding it if necessary.
Expand Down
15 changes: 9 additions & 6 deletions crates/bevy_ecs/src/query/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1099,7 +1099,8 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
world: UnsafeWorldCell<'w>,
entity: Entity,
) -> Result<D::Item<'w, '_>, QueryEntityError> {
self.query_unchecked(world).get_inner(entity)
// SAFETY: Upheld by caller
Copy link
Member

@james7132 james7132 Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm of the opinion that we should explicitly state what safety invariants are being upheld here, that way if those invariants change, for whatever reason, they'll be propagated up during code review, and a desynchronization can be seen within the function definition.

Copy link
Contributor Author

@hymm hymm Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I keep going back on forth on this. I was actually going to do it the way you suggested at first. But while working on this I've looked through a lot of the existing safety comments and I see a lot of slightly different wording between the safety comment on the inner function and the parent function. This makes it not 100% clear that the safety is being transparently passed or being modified slightly. The mismatch actually makes it harder to verify that the safety contracts are being upheld. So this approach avoids that problem at the cost of what you're pointing out. I really wish we had rust-lang/rfcs#3842, which would fix this issue. I'd appreciate some other opinions here.

There also a lot cases like this https://github.com/bevyengine/bevy/pull/22014/files#diff-5625b402c72b0ac8efddfa3a2b8c1dc873e93f23cb28fc7f639c42e17e2011a2L253 where the safety invariants are on the trait definiion. And the method call is just another instance of the trait method. I think I'll change these to say "SAFETY: This method is called by the same trait method and so has the same safety contract."

Copy link

@zjp-CN zjp-CN Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's glad to see safety tags are attractive to bevy!

we should explicitly state what safety invariants are being upheld here, that way if those invariants change, for whatever reason, they'll be propagated up during code review, and a desynchronization can be seen within the function definition

As the author of the safety tags RFC and its prototype safety-tool, I recently got the unsafe call trees since Artisan-Lab/tag-std#84 for our draft tagged fork of Asterinas and Rust for Linux to help trace the propagation of safety properties and do better on writing/reviewing/auditing the safety documention and justification.

I'm presenting a talk this weekend. The slides are being iterated on and will be public as soon as possible. The idea may look like the following for this R4L function:

A tag spec is defined as:

[tag.PointToField]
args = [ "ptr", "field", "adt" ]
desc = "The {ptr} must be a pointer to a {field} in {adt}."

And you can see unsafe { &(*Group::<Parent>::container_of(this)).data } has two unsafe operations

  • calling the unsafe container_of function
  • dereferencing the raw pointer (and getting a reference)

The obligation of the unsafe call is delegated to the caller, while dereferencing is also guarenteed by the caller's ValidPtr property.

Copy link
Contributor

@chescock chescock Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd appreciate some other opinions here.

This doesn't exactly address the question here, but my opinion is that mediocre safety comments that let us turn on unsafe_op_in_unsafe_fn are better than perfect safety comments in some places but no safety comments in others. Even bad safety comments would draw your attention to the right places! So I think we should charge ahead with merging these PRs, and then have a more leisurely debate elsewhere about which safety comment style is better.

I'll also note that the safety requirements for this particular function are pretty vague:

This does not check for mutable query correctness. To be safe, make sure mutable queries have unique access to the components they query.

So I'm not sure there is any way to write a precise comment here right now!

Copy link
Contributor Author

@hymm hymm Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be more precise in this instance. James was asking for this to be changed to something like

  • mutable query correctness is upheld by the caller.

If there was more than one point then it would look like

// Safety:
// * "contract 1" is upheld by the caller
// * "contract 2" is upheld by the caller

and then you'd look at the Safety section of get_unchecked and see

/// # Safety
/// * contract 1
/// * contract 2

So there'd be a 1 to 1 correspondence to each contract.

It all has to be manually kept in sync and we already have issues where it's hard to tell that "contract 1" in the safety comment directly corresponds to safety "contract 1" in the doc comment because they're described slightly differently.

unsafe { self.query_unchecked(world) }.get_inner(entity)
}

/// Returns an [`Iterator`] over the query results for the given [`World`].
Expand Down Expand Up @@ -1314,7 +1315,8 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
&'s mut self,
world: UnsafeWorldCell<'w>,
) -> QueryIter<'w, 's, D, F> {
self.query_unchecked(world).into_iter()
// SAFETY: Upheld by caller
unsafe { self.query_unchecked(world) }.into_iter()
}

/// Returns an [`Iterator`] over all possible combinations of `K` query results for the
Expand All @@ -1333,7 +1335,8 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
&'s mut self,
world: UnsafeWorldCell<'w>,
) -> QueryCombinationIter<'w, 's, D, F, K> {
self.query_unchecked(world).iter_combinations_inner()
// SAFETY: Upheld by caller
unsafe { self.query_unchecked(world) }.iter_combinations_inner()
}

/// Returns a parallel iterator over the query results for the given [`World`].
Expand Down Expand Up @@ -1755,7 +1758,8 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
&mut self,
world: UnsafeWorldCell<'w>,
) -> Result<D::Item<'w, '_>, QuerySingleError> {
self.query_unchecked(world).single_inner()
// SAFETY: Upheld by caller
unsafe { self.query_unchecked(world) }.single_inner()
}

/// Returns a query result when there is exactly one entity matching the query,
Expand All @@ -1780,8 +1784,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
// SAFETY:
// - The caller ensured we have the correct access to the world.
// - The caller ensured that the world matches.
self.query_unchecked_manual_with_ticks(world, last_run, this_run)
.single_inner()
unsafe { self.query_unchecked_manual_with_ticks(world, last_run, this_run) }.single_inner()
}
}

Expand Down
6 changes: 4 additions & 2 deletions crates/bevy_ecs/src/schedule/executor/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,8 @@ mod __rust_begin_short_backtrace {
system: &mut ScheduleSystem,
world: UnsafeWorldCell,
) -> Result<(), RunSystemError> {
let result = system.run_unsafe((), world);
// SAFETY: Upheld by caller
let result = unsafe { system.run_unsafe((), world) };
// Call `black_box` to prevent this frame from being tail-call optimized away
black_box(());
result
Expand All @@ -276,7 +277,8 @@ mod __rust_begin_short_backtrace {
world: UnsafeWorldCell,
) -> Result<O, RunSystemError> {
// Call `black_box` to prevent this frame from being tail-call optimized away
black_box(system.run_unsafe((), world))
// SAFETY: Upheld by caller
black_box(unsafe { system.run_unsafe((), world) })
}

#[cfg(feature = "std")]
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/spawn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ impl<R: Relationship, L: SpawnableList<R>> DynamicBundle for SpawnRelatedBundle<
// called exactly once for each component being fetched with the correct `StorageType`
// - `Effect: !NoBundleEffect`, which means the caller is responsible for calling this type's `apply_effect`
// at least once before returning to safe code.
<R::RelationshipTarget as DynamicBundle>::get_components(target, func);
unsafe { <R::RelationshipTarget as DynamicBundle>::get_components(target, func) };
// Forget the pointer so that the value is available in `apply_effect`.
mem::forget(ptr);
}
Expand Down Expand Up @@ -372,7 +372,7 @@ impl<R: Relationship, B: Bundle> DynamicBundle for SpawnOneRelated<R, B> {
// called exactly once for each component being fetched with the correct `StorageType`
// - `Effect: !NoBundleEffect`, which means the caller is responsible for calling this type's `apply_effect`
// at least once before returning to safe code.
<R::RelationshipTarget as DynamicBundle>::get_components(target, func);
unsafe { <R::RelationshipTarget as DynamicBundle>::get_components(target, func) };
// Forget the pointer so that the value is available in `apply_effect`.
mem::forget(ptr);
}
Expand Down
3 changes: 2 additions & 1 deletion crates/bevy_ecs/src/storage/blob_array.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ impl BlobArray {
capacity,
}
} else {
let mut arr = Self::with_capacity(item_layout, drop_fn, 0);
// SAFETY: Upheld by caller
let mut arr = unsafe { Self::with_capacity(item_layout, drop_fn, 0) };
// SAFETY: `capacity` > 0
unsafe { arr.alloc(NonZeroUsize::new_unchecked(capacity)) }
arr
Expand Down
3 changes: 2 additions & 1 deletion crates/bevy_ecs/src/storage/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ impl<T> VecExtensions<T> for Vec<T> {
// SAFETY: We replace self[index] with the last element. The caller must ensure that
// both the last element and `index` must be valid and cannot point to the same place.
unsafe { core::ptr::copy_nonoverlapping(base_ptr.add(len - 1), base_ptr.add(index), 1) };
self.set_len(len - 1);
// SAFETY: Upheld by caller
unsafe { self.set_len(len - 1) };
value
}
}
15 changes: 10 additions & 5 deletions crates/bevy_ecs/src/storage/table/column.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,23 +336,26 @@ impl Column {
/// - `T` must match the type of data that's stored in this [`Column`]
/// - `len` must match the actual length of this column (number of elements stored)
pub unsafe fn get_data_slice<T>(&self, len: usize) -> &[UnsafeCell<T>] {
self.data.get_sub_slice(len)
// SAFETY: Upheld by caller
unsafe { self.data.get_sub_slice(len) }
}

/// Get a slice to the added [`ticks`](Tick) in this [`Column`].
///
/// # Safety
/// - `len` must match the actual length of this column (number of elements stored)
pub unsafe fn get_added_ticks_slice(&self, len: usize) -> &[UnsafeCell<Tick>] {
self.added_ticks.as_slice(len)
// SAFETY: Upheld by caller
unsafe { self.added_ticks.as_slice(len) }
}

/// Get a slice to the changed [`ticks`](Tick) in this [`Column`].
///
/// # Safety
/// - `len` must match the actual length of this column (number of elements stored)
pub unsafe fn get_changed_ticks_slice(&self, len: usize) -> &[UnsafeCell<Tick>] {
self.changed_ticks.as_slice(len)
// SAFETY: Upheld by caller
unsafe { self.changed_ticks.as_slice(len) }
}

/// Get a slice to the calling locations that last changed each value in this [`Column`]
Expand Down Expand Up @@ -402,7 +405,8 @@ impl Column {
/// `row` must be within the range `[0, self.len())`.
#[inline]
pub unsafe fn get_added_tick_unchecked(&self, row: TableRow) -> &UnsafeCell<Tick> {
self.added_ticks.get_unchecked(row.index())
// SAFETY: Upheld by caller
unsafe { self.added_ticks.get_unchecked(row.index()) }
}

/// Fetches the "changed" change detection tick for the value at `row`
Expand All @@ -412,7 +416,8 @@ impl Column {
/// `row` must be within the range `[0, self.len())`.
#[inline]
pub unsafe fn get_changed_tick_unchecked(&self, row: TableRow) -> &UnsafeCell<Tick> {
self.changed_ticks.get_unchecked(row.index())
// SAFETY: Upheld by caller
unsafe { self.changed_ticks.get_unchecked(row.index()) }
}

/// Fetches the change detection ticks for the value at `row`.
Expand Down
13 changes: 8 additions & 5 deletions crates/bevy_ecs/src/system/combinator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,11 +373,14 @@ where
input: SystemIn<'_, Self>,
world: UnsafeWorldCell,
) -> Result<Self::Out, RunSystemError> {
let value = self.a.run_unsafe(input, world)?;
// `Self::validate_param_unsafe` already validated the first system,
// but we still need to validate the second system once the first one runs.
self.b.validate_param_unsafe(world)?;
self.b.run_unsafe(value, world)
// SAFETY: Upheld by caller
unsafe {
let value = self.a.run_unsafe(input, world)?;
// `Self::validate_param_unsafe` already validated the first system,
// but we still need to validate the second system once the first one runs.
self.b.validate_param_unsafe(world)?;
self.b.run_unsafe(value, world)
}
}

#[cfg(feature = "hotpatching")]
Expand Down
28 changes: 17 additions & 11 deletions crates/bevy_ecs/src/system/commands/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,14 @@ const _: () = {
system_meta: &bevy_ecs::system::SystemMeta,
world: UnsafeWorldCell,
) -> Result<(), SystemParamValidationError> {
<__StructFieldsAlias as bevy_ecs::system::SystemParam>::validate_param(
&mut state.state,
system_meta,
world,
)
// SAFETY: Upheld by caller
unsafe {
<__StructFieldsAlias as bevy_ecs::system::SystemParam>::validate_param(
&mut state.state,
system_meta,
world,
)
}
}

#[inline]
Expand All @@ -196,12 +199,15 @@ const _: () = {
world: UnsafeWorldCell<'w>,
change_tick: bevy_ecs::change_detection::Tick,
) -> Self::Item<'w, 's> {
let params = <__StructFieldsAlias as bevy_ecs::system::SystemParam>::get_param(
&mut state.state,
system_meta,
world,
change_tick,
);
// SAFETY: Upheld by caller
let params = unsafe {
<__StructFieldsAlias as bevy_ecs::system::SystemParam>::get_param(
&mut state.state,
system_meta,
world,
change_tick,
)
};
Commands {
queue: InternalQueue::CommandQueue(params.0),
allocator: params.1,
Expand Down
12 changes: 8 additions & 4 deletions crates/bevy_ecs/src/system/schedule_system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ where
_input: SystemIn<'_, Self>,
world: UnsafeWorldCell,
) -> Result<Self::Out, RunSystemError> {
self.system.run_unsafe(&mut self.value, world)
// SAFETY: Upheld by caller
unsafe { self.system.run_unsafe(&mut self.value, world) }
}

#[cfg(feature = "hotpatching")]
Expand All @@ -87,7 +88,8 @@ where
&mut self,
world: UnsafeWorldCell,
) -> Result<(), SystemParamValidationError> {
self.system.validate_param_unsafe(world)
// SAFETY: Upheld by caller
unsafe { self.system.validate_param_unsafe(world) }
}

fn initialize(&mut self, world: &mut World) -> FilteredAccessSet {
Expand Down Expand Up @@ -163,7 +165,8 @@ where
.value
.as_mut()
.expect("System input value was not found. Did you forget to initialize the system before running it?");
self.system.run_unsafe(value, world)
// SAFETY: Upheld by caller
unsafe { self.system.run_unsafe(value, world) }
}

#[cfg(feature = "hotpatching")]
Expand All @@ -184,7 +187,8 @@ where
&mut self,
world: UnsafeWorldCell,
) -> Result<(), SystemParamValidationError> {
self.system.validate_param_unsafe(world)
// SAFETY: Upheld by caller
unsafe { self.system.validate_param_unsafe(world) }
}

fn initialize(&mut self, world: &mut World) -> FilteredAccessSet {
Expand Down
Loading