Skip to content

Commit d38a8df

Browse files
add more SAFETY comments and lint for missing ones in bevy_ecs (#4835)
# Objective `SAFETY` comments are meant to be placed before `unsafe` blocks and should contain the reasoning of why in this case the usage of unsafe is okay. This is useful when reading the code because it makes it clear which assumptions are required for safety, and makes it easier to spot possible unsoundness holes. It also forces the code writer to think of something to write and maybe look at the safety contracts of any called unsafe methods again to double-check their correct usage. There's a clippy lint called `undocumented_unsafe_blocks` which warns when using a block without such a comment. ## Solution - since clippy expects `SAFETY` instead of `SAFE`, rename those - add `SAFETY` comments in more places - for the last remaining 3 places, add an `#[allow()]` and `// TODO` since I wasn't comfortable enough with the code to justify their safety - add ` #![warn(clippy::undocumented_unsafe_blocks)]` to `bevy_ecs` ### Note for reviewers The first commit only renames `SAFETY` to `SAFE` so it doesn't need a thorough review. https://github.com/bevyengine/bevy/pull/4835/files/cb042a416ecbe5e7d74797449969e064d8a5f13c..55cef2d6fa3aa634667a60f6d5abc16f43f16298 is the diff for all other changes. ### Safety comments where I'm not too familiar with the code https://github.com/bevyengine/bevy/blob/774012ece50e4add4fcc8324ec48bbecf5546c3c/crates/bevy_ecs/src/entity/mod.rs#L540-L546 https://github.com/bevyengine/bevy/blob/774012ece50e4add4fcc8324ec48bbecf5546c3c/crates/bevy_ecs/src/world/entity_ref.rs#L249-L252 ### Locations left undocumented with a `TODO` comment https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/schedule/executor_parallel.rs#L196-L199 https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/world/entity_ref.rs#L287-L289 https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/world/entity_ref.rs#L413-L415 Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>
1 parent 4d05eb1 commit d38a8df

31 files changed

+362
-241
lines changed

crates/bevy_ecs/macros/src/lib.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream {
144144
let struct_name = &ast.ident;
145145

146146
TokenStream::from(quote! {
147-
/// SAFE: ComponentId is returned in field-definition-order. [from_components] and [get_components] use field-definition-order
147+
/// SAFETY: ComponentId is returned in field-definition-order. [from_components] and [get_components] use field-definition-order
148148
unsafe impl #impl_generics #ecs_path::bundle::Bundle for #struct_name #ty_generics #where_clause {
149149
fn component_ids(
150150
components: &mut #ecs_path::component::Components,
@@ -192,7 +192,7 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream {
192192
let index = Index::from(i);
193193
param_fn_muts.push(quote! {
194194
pub fn #fn_name<'a>(&'a mut self) -> <#param::Fetch as SystemParamFetch<'a, 'a>>::Item {
195-
// SAFE: systems run without conflicts with other systems.
195+
// SAFETY: systems run without conflicts with other systems.
196196
// Conflicting params in ParamSet are not accessible at the same time
197197
// ParamSets are guaranteed to not conflict with other SystemParams
198198
unsafe {
@@ -213,13 +213,13 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream {
213213
type Fetch = ParamSetState<(#(#param::Fetch,)*)>;
214214
}
215215

216-
// SAFE: All parameters are constrained to ReadOnlyFetch, so World is only read
216+
// SAFETY: All parameters are constrained to ReadOnlyFetch, so World is only read
217217

218218
unsafe impl<#(#param_fetch: for<'w1, 's1> SystemParamFetch<'w1, 's1>,)*> ReadOnlySystemParamFetch for ParamSetState<(#(#param_fetch,)*)>
219219
where #(#param_fetch: ReadOnlySystemParamFetch,)*
220220
{ }
221221

222-
// SAFE: Relevant parameter ComponentId and ArchetypeComponentId access is applied to SystemMeta. If any ParamState conflicts
222+
// SAFETY: Relevant parameter ComponentId and ArchetypeComponentId access is applied to SystemMeta. If any ParamState conflicts
223223
// with any prior access, a panic will occur.
224224

225225
unsafe impl<#(#param_fetch: for<'w1, 's1> SystemParamFetch<'w1, 's1>,)*> SystemParamState for ParamSetState<(#(#param_fetch,)*)>

crates/bevy_ecs/src/archetype.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -420,13 +420,13 @@ impl Archetypes {
420420

421421
#[inline]
422422
pub fn empty(&self) -> &Archetype {
423-
// SAFE: empty archetype always exists
423+
// SAFETY: empty archetype always exists
424424
unsafe { self.archetypes.get_unchecked(ArchetypeId::EMPTY.index()) }
425425
}
426426

427427
#[inline]
428428
pub(crate) fn empty_mut(&mut self) -> &mut Archetype {
429-
// SAFE: empty archetype always exists
429+
// SAFETY: empty archetype always exists
430430
unsafe {
431431
self.archetypes
432432
.get_unchecked_mut(ArchetypeId::EMPTY.index())
@@ -435,13 +435,13 @@ impl Archetypes {
435435

436436
#[inline]
437437
pub fn resource(&self) -> &Archetype {
438-
// SAFE: resource archetype always exists
438+
// SAFETY: resource archetype always exists
439439
unsafe { self.archetypes.get_unchecked(ArchetypeId::RESOURCE.index()) }
440440
}
441441

442442
#[inline]
443443
pub(crate) fn resource_mut(&mut self) -> &mut Archetype {
444-
// SAFE: resource archetype always exists
444+
// SAFETY: resource archetype always exists
445445
unsafe {
446446
self.archetypes
447447
.get_unchecked_mut(ArchetypeId::RESOURCE.index())

crates/bevy_ecs/src/bundle.rs

+11-7
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,10 @@ pub unsafe trait Bundle: Send + Sync + 'static {
9999

100100
macro_rules! tuple_impl {
101101
($($name: ident),*) => {
102+
// SAFETY:
103+
// - `Bundle::component_ids` returns the `ComponentId`s for each component type in the
104+
// bundle, in the exact order that `Bundle::get_components` is called.
105+
// - `Bundle::from_components` calls `func` exactly once for each `ComponentId` returned by `Bundle::component_ids`.
102106
unsafe impl<$($name: Component),*> Bundle for ($($name,)*) {
103107
#[allow(unused_variables)]
104108
fn component_ids(components: &mut Components, storages: &mut Storages) -> Vec<ComponentId> {
@@ -325,7 +329,7 @@ impl BundleInfo {
325329
bundle_status.push(ComponentStatus::Mutated);
326330
} else {
327331
bundle_status.push(ComponentStatus::Added);
328-
// SAFE: component_id exists
332+
// SAFETY: component_id exists
329333
let component_info = unsafe { components.get_info_unchecked(component_id) };
330334
match component_info.storage_type() {
331335
StorageType::Table => new_table_components.push(component_id),
@@ -354,7 +358,7 @@ impl BundleInfo {
354358
new_table_components.extend(current_archetype.table_components());
355359
// sort to ignore order while hashing
356360
new_table_components.sort();
357-
// SAFE: all component ids in `new_table_components` exist
361+
// SAFETY: all component ids in `new_table_components` exist
358362
table_id = unsafe {
359363
storages
360364
.tables
@@ -492,7 +496,7 @@ impl<'a, 'b> BundleInserter<'a, 'b> {
492496
} else if new_archetype.id() == swapped_location.archetype_id {
493497
new_archetype
494498
} else {
495-
// SAFE: the only two borrowed archetypes are above and we just did collision checks
499+
// SAFETY: the only two borrowed archetypes are above and we just did collision checks
496500
&mut *self
497501
.archetypes_ptr
498502
.add(swapped_location.archetype_id.index())
@@ -567,7 +571,7 @@ impl<'a, 'b> BundleSpawner<'a, 'b> {
567571
#[inline]
568572
pub unsafe fn spawn<T: Bundle>(&mut self, bundle: T) -> Entity {
569573
let entity = self.entities.alloc();
570-
// SAFE: entity is allocated (but non-existent), `T` matches this BundleInfo's type
574+
// SAFETY: entity is allocated (but non-existent), `T` matches this BundleInfo's type
571575
self.spawn_non_existent(entity, bundle);
572576
entity
573577
}
@@ -599,14 +603,14 @@ impl Bundles {
599603
let id = self.bundle_ids.entry(TypeId::of::<T>()).or_insert_with(|| {
600604
let component_ids = T::component_ids(components, storages);
601605
let id = BundleId(bundle_infos.len());
602-
// SAFE: T::component_id ensures info was created
606+
// SAFETY: T::component_id ensures info was created
603607
let bundle_info = unsafe {
604608
initialize_bundle(std::any::type_name::<T>(), component_ids, id, components)
605609
};
606610
bundle_infos.push(bundle_info);
607611
id
608612
});
609-
// SAFE: index either exists, or was initialized
613+
// SAFETY: index either exists, or was initialized
610614
unsafe { self.bundle_infos.get_unchecked(id.0) }
611615
}
612616
}
@@ -623,7 +627,7 @@ unsafe fn initialize_bundle(
623627
let mut storage_types = Vec::new();
624628

625629
for &component_id in &component_ids {
626-
// SAFE: component_id exists and is therefore valid
630+
// SAFETY: component_id exists and is therefore valid
627631
let component_info = components.get_info_unchecked(component_id);
628632
storage_types.push(component_info.storage_type());
629633
}

crates/bevy_ecs/src/component.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -474,7 +474,7 @@ impl Components {
474474

475475
#[inline]
476476
pub fn init_resource<T: Resource>(&mut self) -> ComponentId {
477-
// SAFE: The [`ComponentDescriptor`] matches the [`TypeId`]
477+
// SAFETY: The [`ComponentDescriptor`] matches the [`TypeId`]
478478
unsafe {
479479
self.get_or_insert_resource_with(TypeId::of::<T>(), || {
480480
ComponentDescriptor::new_resource::<T>()
@@ -484,7 +484,7 @@ impl Components {
484484

485485
#[inline]
486486
pub fn init_non_send<T: Any>(&mut self) -> ComponentId {
487-
// SAFE: The [`ComponentDescriptor`] matches the [`TypeId`]
487+
// SAFETY: The [`ComponentDescriptor`] matches the [`TypeId`]
488488
unsafe {
489489
self.get_or_insert_resource_with(TypeId::of::<T>(), || {
490490
ComponentDescriptor::new_non_send::<T>(StorageType::default())

crates/bevy_ecs/src/entity/mod.rs

+4
Original file line numberDiff line numberDiff line change
@@ -591,6 +591,8 @@ impl Entities {
591591
// Flushes all reserved entities to an "invalid" state. Attempting to retrieve them will return None
592592
// unless they are later populated with a valid archetype.
593593
pub fn flush_as_invalid(&mut self) {
594+
// SAFETY: as per `flush` safety docs, the archetype id can be set to [`ArchetypeId::INVALID`] if
595+
// the [`Entity`] has not been assigned to an [`Archetype`][crate::archetype::Archetype], which is the case here
594596
unsafe {
595597
self.flush(|_entity, location| {
596598
location.archetype_id = ArchetypeId::INVALID;
@@ -658,6 +660,7 @@ mod tests {
658660
fn reserve_entity_len() {
659661
let mut e = Entities::default();
660662
e.reserve_entity();
663+
// SAFETY: entity_location is left invalid
661664
unsafe { e.flush(|_, _| {}) };
662665
assert_eq!(e.len(), 1);
663666
}
@@ -669,6 +672,7 @@ mod tests {
669672
assert!(entities.contains(e));
670673
assert!(entities.get(e).is_none());
671674

675+
// SAFETY: entity_location is left invalid
672676
unsafe {
673677
entities.flush(|_entity, _location| {
674678
// do nothing ... leaving entity location invalid

crates/bevy_ecs/src/lib.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#![warn(clippy::undocumented_unsafe_blocks)]
12
#![doc = include_str!("../README.md")]
23

34
#[cfg(target_pointer_width = "16")]

crates/bevy_ecs/src/query/filter.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -480,7 +480,7 @@ macro_rules! impl_query_filter_tuple {
480480
}
481481
}
482482

483-
// SAFE: filters are read only
483+
// SAFETY: filters are read only
484484
unsafe impl<$($filter: ReadOnlyWorldQuery),*> ReadOnlyWorldQuery for Or<($($filter,)*)> {}
485485
};
486486
}

crates/bevy_ecs/src/query/iter.rs

+51-20
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,9 @@ where
5252

5353
#[inline(always)]
5454
fn next(&mut self) -> Option<Self::Item> {
55+
// SAFETY:
56+
// `tables` and `archetypes` belong to the same world that the cursor was initialized for.
57+
// `query_state` is the state that was passed to `QueryIterationCursor::init`.
5558
unsafe {
5659
self.cursor
5760
.next(self.tables, self.archetypes, self.query_state)
@@ -150,33 +153,42 @@ where
150153

151154
#[inline(always)]
152155
fn next(&mut self) -> Option<Self::Item> {
153-
unsafe {
154-
for entity in self.entity_iter.by_ref() {
155-
let location = match self.entities.get(*entity.borrow()) {
156-
Some(location) => location,
157-
None => continue,
158-
};
159-
160-
if !self
161-
.query_state
162-
.matched_archetypes
163-
.contains(location.archetype_id.index())
164-
{
165-
continue;
166-
}
156+
for entity in self.entity_iter.by_ref() {
157+
let location = match self.entities.get(*entity.borrow()) {
158+
Some(location) => location,
159+
None => continue,
160+
};
161+
162+
if !self
163+
.query_state
164+
.matched_archetypes
165+
.contains(location.archetype_id.index())
166+
{
167+
continue;
168+
}
167169

168-
let archetype = &self.archetypes[location.archetype_id];
170+
let archetype = &self.archetypes[location.archetype_id];
169171

172+
// SAFETY: `archetype` is from the world that `fetch/filter` were created for,
173+
// `fetch_state`/`filter_state` are the states that `fetch/filter` were initialized with
174+
unsafe {
170175
self.fetch
171176
.set_archetype(&self.query_state.fetch_state, archetype, self.tables);
177+
}
178+
// SAFETY: `table` is from the world that `fetch/filter` were created for,
179+
// `fetch_state`/`filter_state` are the states that `fetch/filter` were initialized with
180+
unsafe {
172181
self.filter
173182
.set_archetype(&self.query_state.filter_state, archetype, self.tables);
174-
if self.filter.archetype_filter_fetch(location.index) {
175-
return Some(self.fetch.archetype_fetch(location.index));
176-
}
177183
}
178-
None
184+
// SAFETY: set_archetype was called prior.
185+
// `location.index` is an archetype index row in range of the current archetype, because if it was not, the match above would have `continue`d
186+
if unsafe { self.filter.archetype_filter_fetch(location.index) } {
187+
// SAFETY: set_archetype was called prior, `location.index` is an archetype index in range of the current archetype
188+
return Some(unsafe { self.fetch.archetype_fetch(location.index) });
189+
}
179190
}
191+
None
180192
}
181193

182194
fn size_hint(&self) -> (usize, Option<usize>) {
@@ -289,7 +301,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery, const K: usize> QueryCombinationIter<
289301
for<'a> QueryFetch<'a, Q>: Clone,
290302
for<'a> QueryFetch<'a, F>: Clone,
291303
{
292-
// safety: we are limiting the returned reference to self,
304+
// SAFETY: we are limiting the returned reference to self,
293305
// making sure this method cannot be called multiple times without getting rid
294306
// of any previously returned unique references first, thus preventing aliasing.
295307
unsafe {
@@ -374,7 +386,9 @@ struct QueryIterationCursor<'w, 's, Q: WorldQuery, QF: Fetch<'w, State = Q::Stat
374386
archetype_id_iter: std::slice::Iter<'s, ArchetypeId>,
375387
fetch: QF,
376388
filter: QueryFetch<'w, F>,
389+
// length of the table table or length of the archetype, depending on whether both `Q`'s and `F`'s fetches are dense
377390
current_len: usize,
391+
// either table row or archetype index, depending on whether both `Q`'s and `F`'s fetches are dense
378392
current_index: usize,
379393
phantom: PhantomData<(&'w (), Q)>,
380394
}
@@ -461,6 +475,10 @@ where
461475

462476
// NOTE: If you are changing query iteration code, remember to update the following places, where relevant:
463477
// QueryIterationCursor, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual
478+
/// # Safety
479+
/// `tables` and `archetypes` must belong to the same world that the [`QueryIterationCursor`]
480+
/// was initialized for.
481+
/// `query_state` must be the same [`QueryState`] that was passed to `init` or `init_empty`.
464482
#[inline(always)]
465483
unsafe fn next(
466484
&mut self,
@@ -470,21 +488,28 @@ where
470488
) -> Option<QF::Item> {
471489
if Self::IS_DENSE {
472490
loop {
491+
// we are on the beginning of the query, or finished processing a table, so skip to the next
473492
if self.current_index == self.current_len {
474493
let table_id = self.table_id_iter.next()?;
475494
let table = &tables[*table_id];
495+
// SAFETY: `table` is from the world that `fetch/filter` were created for,
496+
// `fetch_state`/`filter_state` are the states that `fetch/filter` were initialized with
476497
self.fetch.set_table(&query_state.fetch_state, table);
477498
self.filter.set_table(&query_state.filter_state, table);
478499
self.current_len = table.len();
479500
self.current_index = 0;
480501
continue;
481502
}
482503

504+
// SAFETY: set_table was called prior.
505+
// `current_index` is a table row in range of the current table, because if it was not, then the if above would have been executed.
483506
if !self.filter.table_filter_fetch(self.current_index) {
484507
self.current_index += 1;
485508
continue;
486509
}
487510

511+
// SAFETY: set_table was called prior.
512+
// `current_index` is a table row in range of the current table, because if it was not, then the if above would have been executed.
488513
let item = self.fetch.table_fetch(self.current_index);
489514

490515
self.current_index += 1;
@@ -495,6 +520,8 @@ where
495520
if self.current_index == self.current_len {
496521
let archetype_id = self.archetype_id_iter.next()?;
497522
let archetype = &archetypes[*archetype_id];
523+
// SAFETY: `archetype` and `tables` are from the world that `fetch/filter` were created for,
524+
// `fetch_state`/`filter_state` are the states that `fetch/filter` were initialized with
498525
self.fetch
499526
.set_archetype(&query_state.fetch_state, archetype, tables);
500527
self.filter
@@ -504,11 +531,15 @@ where
504531
continue;
505532
}
506533

534+
// SAFETY: set_archetype was called prior.
535+
// `current_index` is an archetype index row in range of the current archetype, because if it was not, then the if above would have been executed.
507536
if !self.filter.archetype_filter_fetch(self.current_index) {
508537
self.current_index += 1;
509538
continue;
510539
}
511540

541+
// SAFETY: set_archetype was called prior, `current_index` is an archetype index in range of the current archetype
542+
// `current_index` is an archetype index row in range of the current archetype, because if it was not, then the if above would have been executed.
512543
let item = self.fetch.archetype_fetch(self.current_index);
513544
self.current_index += 1;
514545
return Some(item);

0 commit comments

Comments
 (0)