From d38a8dfdd79f6fd8e1e915aa5e2ddb03ffdaa76a Mon Sep 17 00:00:00 2001 From: Jakob Hellermann Date: Mon, 4 Jul 2022 14:44:24 +0000 Subject: [PATCH] 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 --- crates/bevy_ecs/macros/src/lib.rs | 8 +- crates/bevy_ecs/src/archetype.rs | 8 +- crates/bevy_ecs/src/bundle.rs | 18 +-- crates/bevy_ecs/src/component.rs | 4 +- crates/bevy_ecs/src/entity/mod.rs | 4 + crates/bevy_ecs/src/lib.rs | 1 + crates/bevy_ecs/src/query/filter.rs | 2 +- crates/bevy_ecs/src/query/iter.rs | 71 +++++++---- crates/bevy_ecs/src/query/state.rs | 17 ++- crates/bevy_ecs/src/reflect.rs | 43 ++++--- .../src/schedule/executor_parallel.rs | 1 + crates/bevy_ecs/src/storage/blob_vec.rs | 58 ++++++--- crates/bevy_ecs/src/storage/sparse_set.rs | 20 ++-- crates/bevy_ecs/src/storage/table.rs | 4 +- .../src/system/commands/command_queue.rs | 12 +- .../src/system/commands/parallel_scope.rs | 2 +- crates/bevy_ecs/src/system/function_system.rs | 4 +- crates/bevy_ecs/src/system/query.rs | 46 +++---- crates/bevy_ecs/src/system/system.rs | 2 +- crates/bevy_ecs/src/system/system_param.rs | 72 ++++++----- crates/bevy_ecs/src/world/entity_ref.rs | 60 ++++++---- crates/bevy_ecs/src/world/mod.rs | 112 ++++++++++-------- crates/bevy_ecs/src/world/spawn_batch.rs | 2 +- crates/bevy_ecs/src/world/world_cell.rs | 8 +- crates/bevy_hierarchy/src/child_builder.rs | 8 +- crates/bevy_hierarchy/src/hierarchy.rs | 4 +- crates/bevy_ptr/src/lib.rs | 2 + crates/bevy_time/src/fixed_timestep.rs | 2 +- crates/bevy_ui/src/flex/mod.rs | 2 +- crates/bevy_utils/src/futures.rs | 2 + crates/bevy_window/src/raw_window_handle.rs | 4 +- 31 files changed, 362 insertions(+), 241 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 3273faf9ab1a2..5722872df15e2 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -144,7 +144,7 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream { let struct_name = &ast.ident; TokenStream::from(quote! { - /// SAFE: ComponentId is returned in field-definition-order. [from_components] and [get_components] use field-definition-order + /// SAFETY: ComponentId is returned in field-definition-order. [from_components] and [get_components] use field-definition-order unsafe impl #impl_generics #ecs_path::bundle::Bundle for #struct_name #ty_generics #where_clause { fn component_ids( components: &mut #ecs_path::component::Components, @@ -192,7 +192,7 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream { let index = Index::from(i); param_fn_muts.push(quote! { pub fn #fn_name<'a>(&'a mut self) -> <#param::Fetch as SystemParamFetch<'a, 'a>>::Item { - // SAFE: systems run without conflicts with other systems. + // SAFETY: systems run without conflicts with other systems. // Conflicting params in ParamSet are not accessible at the same time // ParamSets are guaranteed to not conflict with other SystemParams unsafe { @@ -213,13 +213,13 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream { type Fetch = ParamSetState<(#(#param::Fetch,)*)>; } - // SAFE: All parameters are constrained to ReadOnlyFetch, so World is only read + // SAFETY: All parameters are constrained to ReadOnlyFetch, so World is only read unsafe impl<#(#param_fetch: for<'w1, 's1> SystemParamFetch<'w1, 's1>,)*> ReadOnlySystemParamFetch for ParamSetState<(#(#param_fetch,)*)> where #(#param_fetch: ReadOnlySystemParamFetch,)* { } - // SAFE: Relevant parameter ComponentId and ArchetypeComponentId access is applied to SystemMeta. If any ParamState conflicts + // SAFETY: Relevant parameter ComponentId and ArchetypeComponentId access is applied to SystemMeta. If any ParamState conflicts // with any prior access, a panic will occur. unsafe impl<#(#param_fetch: for<'w1, 's1> SystemParamFetch<'w1, 's1>,)*> SystemParamState for ParamSetState<(#(#param_fetch,)*)> diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index 2c01f8c1bb4b2..b5d6b30e3bfa8 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -420,13 +420,13 @@ impl Archetypes { #[inline] pub fn empty(&self) -> &Archetype { - // SAFE: empty archetype always exists + // SAFETY: empty archetype always exists unsafe { self.archetypes.get_unchecked(ArchetypeId::EMPTY.index()) } } #[inline] pub(crate) fn empty_mut(&mut self) -> &mut Archetype { - // SAFE: empty archetype always exists + // SAFETY: empty archetype always exists unsafe { self.archetypes .get_unchecked_mut(ArchetypeId::EMPTY.index()) @@ -435,13 +435,13 @@ impl Archetypes { #[inline] pub fn resource(&self) -> &Archetype { - // SAFE: resource archetype always exists + // SAFETY: resource archetype always exists unsafe { self.archetypes.get_unchecked(ArchetypeId::RESOURCE.index()) } } #[inline] pub(crate) fn resource_mut(&mut self) -> &mut Archetype { - // SAFE: resource archetype always exists + // SAFETY: resource archetype always exists unsafe { self.archetypes .get_unchecked_mut(ArchetypeId::RESOURCE.index()) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 6b81435fc8b4b..8eb7730289f49 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -99,6 +99,10 @@ pub unsafe trait Bundle: Send + Sync + 'static { macro_rules! tuple_impl { ($($name: ident),*) => { + // SAFETY: + // - `Bundle::component_ids` returns the `ComponentId`s for each component type in the + // bundle, in the exact order that `Bundle::get_components` is called. + // - `Bundle::from_components` calls `func` exactly once for each `ComponentId` returned by `Bundle::component_ids`. unsafe impl<$($name: Component),*> Bundle for ($($name,)*) { #[allow(unused_variables)] fn component_ids(components: &mut Components, storages: &mut Storages) -> Vec { @@ -325,7 +329,7 @@ impl BundleInfo { bundle_status.push(ComponentStatus::Mutated); } else { bundle_status.push(ComponentStatus::Added); - // SAFE: component_id exists + // SAFETY: component_id exists let component_info = unsafe { components.get_info_unchecked(component_id) }; match component_info.storage_type() { StorageType::Table => new_table_components.push(component_id), @@ -354,7 +358,7 @@ impl BundleInfo { new_table_components.extend(current_archetype.table_components()); // sort to ignore order while hashing new_table_components.sort(); - // SAFE: all component ids in `new_table_components` exist + // SAFETY: all component ids in `new_table_components` exist table_id = unsafe { storages .tables @@ -492,7 +496,7 @@ impl<'a, 'b> BundleInserter<'a, 'b> { } else if new_archetype.id() == swapped_location.archetype_id { new_archetype } else { - // SAFE: the only two borrowed archetypes are above and we just did collision checks + // SAFETY: the only two borrowed archetypes are above and we just did collision checks &mut *self .archetypes_ptr .add(swapped_location.archetype_id.index()) @@ -567,7 +571,7 @@ impl<'a, 'b> BundleSpawner<'a, 'b> { #[inline] pub unsafe fn spawn(&mut self, bundle: T) -> Entity { let entity = self.entities.alloc(); - // SAFE: entity is allocated (but non-existent), `T` matches this BundleInfo's type + // SAFETY: entity is allocated (but non-existent), `T` matches this BundleInfo's type self.spawn_non_existent(entity, bundle); entity } @@ -599,14 +603,14 @@ impl Bundles { let id = self.bundle_ids.entry(TypeId::of::()).or_insert_with(|| { let component_ids = T::component_ids(components, storages); let id = BundleId(bundle_infos.len()); - // SAFE: T::component_id ensures info was created + // SAFETY: T::component_id ensures info was created let bundle_info = unsafe { initialize_bundle(std::any::type_name::(), component_ids, id, components) }; bundle_infos.push(bundle_info); id }); - // SAFE: index either exists, or was initialized + // SAFETY: index either exists, or was initialized unsafe { self.bundle_infos.get_unchecked(id.0) } } } @@ -623,7 +627,7 @@ unsafe fn initialize_bundle( let mut storage_types = Vec::new(); for &component_id in &component_ids { - // SAFE: component_id exists and is therefore valid + // SAFETY: component_id exists and is therefore valid let component_info = components.get_info_unchecked(component_id); storage_types.push(component_info.storage_type()); } diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index f4e2b61d14784..eb379934efed4 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -474,7 +474,7 @@ impl Components { #[inline] pub fn init_resource(&mut self) -> ComponentId { - // SAFE: The [`ComponentDescriptor`] matches the [`TypeId`] + // SAFETY: The [`ComponentDescriptor`] matches the [`TypeId`] unsafe { self.get_or_insert_resource_with(TypeId::of::(), || { ComponentDescriptor::new_resource::() @@ -484,7 +484,7 @@ impl Components { #[inline] pub fn init_non_send(&mut self) -> ComponentId { - // SAFE: The [`ComponentDescriptor`] matches the [`TypeId`] + // SAFETY: The [`ComponentDescriptor`] matches the [`TypeId`] unsafe { self.get_or_insert_resource_with(TypeId::of::(), || { ComponentDescriptor::new_non_send::(StorageType::default()) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index dcad2664f68cf..2c10a87eb3743 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -591,6 +591,8 @@ impl Entities { // Flushes all reserved entities to an "invalid" state. Attempting to retrieve them will return None // unless they are later populated with a valid archetype. pub fn flush_as_invalid(&mut self) { + // SAFETY: as per `flush` safety docs, the archetype id can be set to [`ArchetypeId::INVALID`] if + // the [`Entity`] has not been assigned to an [`Archetype`][crate::archetype::Archetype], which is the case here unsafe { self.flush(|_entity, location| { location.archetype_id = ArchetypeId::INVALID; @@ -658,6 +660,7 @@ mod tests { fn reserve_entity_len() { let mut e = Entities::default(); e.reserve_entity(); + // SAFETY: entity_location is left invalid unsafe { e.flush(|_, _| {}) }; assert_eq!(e.len(), 1); } @@ -669,6 +672,7 @@ mod tests { assert!(entities.contains(e)); assert!(entities.get(e).is_none()); + // SAFETY: entity_location is left invalid unsafe { entities.flush(|_entity, _location| { // do nothing ... leaving entity location invalid diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 30ec5aa8e72c3..cafacc2a2d929 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -1,3 +1,4 @@ +#![warn(clippy::undocumented_unsafe_blocks)] #![doc = include_str!("../README.md")] #[cfg(target_pointer_width = "16")] diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index ad79241b10980..919b7dd1ec4cd 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -480,7 +480,7 @@ macro_rules! impl_query_filter_tuple { } } - // SAFE: filters are read only + // SAFETY: filters are read only unsafe impl<$($filter: ReadOnlyWorldQuery),*> ReadOnlyWorldQuery for Or<($($filter,)*)> {} }; } diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index 6755026f3ad07..65881a6a93d1c 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -52,6 +52,9 @@ where #[inline(always)] fn next(&mut self) -> Option { + // SAFETY: + // `tables` and `archetypes` belong to the same world that the cursor was initialized for. + // `query_state` is the state that was passed to `QueryIterationCursor::init`. unsafe { self.cursor .next(self.tables, self.archetypes, self.query_state) @@ -150,33 +153,42 @@ where #[inline(always)] fn next(&mut self) -> Option { - unsafe { - for entity in self.entity_iter.by_ref() { - let location = match self.entities.get(*entity.borrow()) { - Some(location) => location, - None => continue, - }; - - if !self - .query_state - .matched_archetypes - .contains(location.archetype_id.index()) - { - continue; - } + for entity in self.entity_iter.by_ref() { + let location = match self.entities.get(*entity.borrow()) { + Some(location) => location, + None => continue, + }; + + if !self + .query_state + .matched_archetypes + .contains(location.archetype_id.index()) + { + continue; + } - let archetype = &self.archetypes[location.archetype_id]; + let archetype = &self.archetypes[location.archetype_id]; + // SAFETY: `archetype` is from the world that `fetch/filter` were created for, + // `fetch_state`/`filter_state` are the states that `fetch/filter` were initialized with + unsafe { self.fetch .set_archetype(&self.query_state.fetch_state, archetype, self.tables); + } + // SAFETY: `table` is from the world that `fetch/filter` were created for, + // `fetch_state`/`filter_state` are the states that `fetch/filter` were initialized with + unsafe { self.filter .set_archetype(&self.query_state.filter_state, archetype, self.tables); - if self.filter.archetype_filter_fetch(location.index) { - return Some(self.fetch.archetype_fetch(location.index)); - } } - None + // SAFETY: set_archetype was called prior. + // `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 + if unsafe { self.filter.archetype_filter_fetch(location.index) } { + // SAFETY: set_archetype was called prior, `location.index` is an archetype index in range of the current archetype + return Some(unsafe { self.fetch.archetype_fetch(location.index) }); + } } + None } fn size_hint(&self) -> (usize, Option) { @@ -289,7 +301,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery, const K: usize> QueryCombinationIter< for<'a> QueryFetch<'a, Q>: Clone, for<'a> QueryFetch<'a, F>: Clone, { - // safety: we are limiting the returned reference to self, + // SAFETY: we are limiting the returned reference to self, // making sure this method cannot be called multiple times without getting rid // of any previously returned unique references first, thus preventing aliasing. unsafe { @@ -374,7 +386,9 @@ struct QueryIterationCursor<'w, 's, Q: WorldQuery, QF: Fetch<'w, State = Q::Stat archetype_id_iter: std::slice::Iter<'s, ArchetypeId>, fetch: QF, filter: QueryFetch<'w, F>, + // length of the table table or length of the archetype, depending on whether both `Q`'s and `F`'s fetches are dense current_len: usize, + // either table row or archetype index, depending on whether both `Q`'s and `F`'s fetches are dense current_index: usize, phantom: PhantomData<(&'w (), Q)>, } @@ -461,6 +475,10 @@ where // NOTE: If you are changing query iteration code, remember to update the following places, where relevant: // QueryIterationCursor, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual + /// # Safety + /// `tables` and `archetypes` must belong to the same world that the [`QueryIterationCursor`] + /// was initialized for. + /// `query_state` must be the same [`QueryState`] that was passed to `init` or `init_empty`. #[inline(always)] unsafe fn next( &mut self, @@ -470,9 +488,12 @@ where ) -> Option { if Self::IS_DENSE { loop { + // we are on the beginning of the query, or finished processing a table, so skip to the next if self.current_index == self.current_len { let table_id = self.table_id_iter.next()?; let table = &tables[*table_id]; + // SAFETY: `table` is from the world that `fetch/filter` were created for, + // `fetch_state`/`filter_state` are the states that `fetch/filter` were initialized with self.fetch.set_table(&query_state.fetch_state, table); self.filter.set_table(&query_state.filter_state, table); self.current_len = table.len(); @@ -480,11 +501,15 @@ where continue; } + // SAFETY: set_table was called prior. + // `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. if !self.filter.table_filter_fetch(self.current_index) { self.current_index += 1; continue; } + // SAFETY: set_table was called prior. + // `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. let item = self.fetch.table_fetch(self.current_index); self.current_index += 1; @@ -495,6 +520,8 @@ where if self.current_index == self.current_len { let archetype_id = self.archetype_id_iter.next()?; let archetype = &archetypes[*archetype_id]; + // SAFETY: `archetype` and `tables` are from the world that `fetch/filter` were created for, + // `fetch_state`/`filter_state` are the states that `fetch/filter` were initialized with self.fetch .set_archetype(&query_state.fetch_state, archetype, tables); self.filter @@ -504,11 +531,15 @@ where continue; } + // SAFETY: set_archetype was called prior. + // `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. if !self.filter.archetype_filter_fetch(self.current_index) { self.current_index += 1; continue; } + // SAFETY: set_archetype was called prior, `current_index` is an archetype index in range of the current archetype + // `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. let item = self.fetch.archetype_fetch(self.current_index); self.current_index += 1; return Some(item); diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index beb4776e1d83b..a1a271da81aa0 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -81,7 +81,7 @@ impl QueryState { /// Checks if the query is empty for the given [`World`], where the last change and current tick are given. #[inline] pub fn is_empty(&self, world: &World, last_change_tick: u32, change_tick: u32) -> bool { - // SAFE: NopFetch does not access any members while &self ensures no one has exclusive access + // SAFETY: NopFetch does not access any members while &self ensures no one has exclusive access unsafe { self.iter_unchecked_manual::>(world, last_change_tick, change_tick) .next() @@ -211,7 +211,7 @@ impl QueryState { ) -> Result<[ROQueryItem<'w, Q>; N], QueryEntityError> { self.update_archetypes(world); - // SAFE: update_archetypes validates the `World` matches + // SAFETY: update_archetypes validates the `World` matches unsafe { self.get_many_read_only_manual( world, @@ -287,7 +287,7 @@ impl QueryState { ) -> Result<[QueryItem<'w, Q>; N], QueryEntityError> { self.update_archetypes(world); - // SAFE: method requires exclusive world access + // SAFETY: method requires exclusive world access // and world has been validated via update_archetypes unsafe { self.get_many_unchecked_manual( @@ -397,7 +397,7 @@ impl QueryState { last_change_tick: u32, change_tick: u32, ) -> Result<[ROQueryItem<'w, Q>; N], QueryEntityError> { - // SAFE: fetch is read-only + // SAFETY: fetch is read-only // and world must be validated let array_of_results = entities.map(|entity| { self.get_unchecked_manual::>( @@ -527,7 +527,7 @@ impl QueryState { &'s mut self, world: &'w World, ) -> QueryCombinationIter<'w, 's, Q, F, K> { - // SAFE: query is read only + // SAFETY: query is read only unsafe { self.update_archetypes(world); self.iter_combinations_unchecked_manual( @@ -550,7 +550,7 @@ impl QueryState { &'s mut self, world: &'w mut World, ) -> QueryCombinationIter<'w, 's, Q, F, K> { - // SAFE: query has unique world access + // SAFETY: query has unique world access unsafe { self.update_archetypes(world); self.iter_combinations_unchecked_manual( @@ -1272,6 +1272,8 @@ mod tests { // It's best to test get_many_unchecked_manual directly, // as it is shared and unsafe // We don't care about aliased mutabilty for the read-only equivalent + + // SAFETY: mutable access is not checked, but we own the world and don't use the query results assert!(unsafe { query_state .get_many_unchecked_manual::<10>( @@ -1284,6 +1286,7 @@ mod tests { }); assert_eq!( + // SAFETY: mutable access is not checked, but we own the world and don't use the query results unsafe { query_state .get_many_unchecked_manual( @@ -1298,6 +1301,7 @@ mod tests { ); assert_eq!( + // SAFETY: mutable access is not checked, but we own the world and don't use the query results unsafe { query_state .get_many_unchecked_manual( @@ -1312,6 +1316,7 @@ mod tests { ); assert_eq!( + // SAFETY: mutable access is not checked, but we own the world and don't use the query results unsafe { query_state .get_many_unchecked_manual( diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index 46871b59a1906..ab4a81412eb84 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/crates/bevy_ecs/src/reflect.rs @@ -69,7 +69,7 @@ impl ReflectComponent { world: &'a mut World, entity: Entity, ) -> Option> { - // SAFE: unique world access + // SAFETY: unique world access unsafe { (self.reflect_component_mut)(world, entity) } } @@ -137,14 +137,18 @@ impl FromType for ReflectComponent { .get::() .map(|c| c as &dyn Reflect) }, - reflect_component_mut: |world, entity| unsafe { - world - .get_entity(entity)? - .get_unchecked_mut::(world.last_change_tick(), world.read_change_tick()) - .map(|c| ReflectMut { - value: c.value as &mut dyn Reflect, - ticks: c.ticks, - }) + reflect_component_mut: |world, entity| { + // SAFETY: reflect_component_mut is an unsafe function pointer used by `reflect_component_unchecked_mut` which promises to never + // produce aliasing mutable references, and reflect_component_mut, which has mutable world access + unsafe { + world + .get_entity(entity)? + .get_unchecked_mut::(world.last_change_tick(), world.read_change_tick()) + .map(|c| ReflectMut { + value: c.value as &mut dyn Reflect, + ticks: c.ticks, + }) + } }, } } @@ -191,7 +195,7 @@ impl ReflectResource { /// Gets the value of this [`Resource`] type from the world as a mutable reflected reference. pub fn reflect_resource_mut<'a>(&self, world: &'a mut World) -> Option> { - // SAFE: unique world access + // SAFETY: unique world access unsafe { (self.reflect_resource_unchecked_mut)(world) } } @@ -205,6 +209,7 @@ impl ReflectResource { &self, world: &'a World, ) -> Option> { + // SAFETY: caller promises to uphold uniqueness guarantees (self.reflect_resource_unchecked_mut)(world) } @@ -234,13 +239,17 @@ impl FromType for ReflectResource { world.remove_resource::(); }, reflect_resource: |world| world.get_resource::().map(|res| res as &dyn Reflect), - reflect_resource_unchecked_mut: |world| unsafe { - world - .get_resource_unchecked_mut::() - .map(|res| ReflectMut { - value: res.value as &mut dyn Reflect, - ticks: res.ticks, - }) + reflect_resource_unchecked_mut: |world| { + // SAFETY: all usages of `reflect_resource_unchecked_mut` guarantee that there is either a single mutable + // reference or multiple immutable ones alive at any given point + unsafe { + world + .get_resource_unchecked_mut::() + .map(|res| ReflectMut { + value: res.value as &mut dyn Reflect, + ticks: res.ticks, + }) + } }, copy_resource: |source_world, destination_world| { let source_resource = source_world.resource::(); diff --git a/crates/bevy_ecs/src/schedule/executor_parallel.rs b/crates/bevy_ecs/src/schedule/executor_parallel.rs index c82924b0e276c..5b9326ce9822d 100644 --- a/crates/bevy_ecs/src/schedule/executor_parallel.rs +++ b/crates/bevy_ecs/src/schedule/executor_parallel.rs @@ -190,6 +190,7 @@ impl ParallelExecutor { .unwrap_or_else(|error| unreachable!("{}", error)); #[cfg(feature = "trace")] let system_guard = system_span.enter(); + // SAFETY: the executor prevents two systems with conflicting access from running simultaneously. unsafe { system.run_unsafe((), world) }; #[cfg(feature = "trace")] drop(system_guard); diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index 1a4404d9b2729..384c26a07dc7a 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -1,6 +1,7 @@ use std::{ alloc::{handle_alloc_error, Layout}, cell::UnsafeCell, + num::NonZeroUsize, ptr::NonNull, }; @@ -14,7 +15,9 @@ pub(super) struct BlobVec { capacity: usize, /// Number of elements, not bytes len: usize, + // the `data` ptr's layout is always `array_layout(item_layout, capacity)` data: NonNull, + // the `swap_scratch` ptr's layout is always `item_layout` swap_scratch: NonNull, // None if the underlying type doesn't need to be dropped drop: Option)>, @@ -93,33 +96,45 @@ impl BlobVec { pub fn reserve_exact(&mut self, additional: usize) { let available_space = self.capacity - self.len; - if available_space < additional { - self.grow_exact(additional - available_space); + if available_space < additional && self.item_layout.size() > 0 { + // SAFETY: `available_space < additional`, so `additional - available_space > 0` + let increment = unsafe { NonZeroUsize::new_unchecked(additional - available_space) }; + // SAFETY: not called for ZSTs + unsafe { self.grow_exact(increment) }; } } - // FIXME: this should probably be an unsafe fn as it shouldn't be called if the layout - // is for a ZST - fn grow_exact(&mut self, increment: usize) { + // SAFETY: must not be called for a ZST item layout + #[warn(unsafe_op_in_unsafe_fn)] // to allow unsafe blocks in unsafe fn + unsafe fn grow_exact(&mut self, increment: NonZeroUsize) { debug_assert!(self.item_layout.size() != 0); - let new_capacity = self.capacity + increment; + let new_capacity = self.capacity + increment.get(); let new_layout = array_layout(&self.item_layout, new_capacity).expect("array layout should be valid"); - unsafe { - let new_data = if self.capacity == 0 { - std::alloc::alloc(new_layout) - } else { + let new_data = if self.capacity == 0 { + // SAFETY: + // - layout has non-zero size as per safety requirement + unsafe { std::alloc::alloc(new_layout) } + } else { + // SAFETY: + // - ptr was be allocated via this allocator + // - the layout of the ptr was `array_layout(self.item_layout, self.capacity)` + // - `item_layout.size() > 0` and `new_capacity > 0`, so the layout size is non-zero + // - "new_size, when rounded up to the nearest multiple of layout.align(), must not overflow (i.e., the rounded value must be less than usize::MAX)", + // since the item size is always a multiple of its align, the rounding cannot happen + // here and the overflow is handled in `array_layout` + unsafe { std::alloc::realloc( self.get_ptr_mut().as_ptr(), array_layout(&self.item_layout, self.capacity) .expect("array layout should be valid"), new_layout.size(), ) - }; + } + }; - self.data = NonNull::new(new_data).unwrap_or_else(|| handle_alloc_error(new_layout)); - } + self.data = NonNull::new(new_data).unwrap_or_else(|| handle_alloc_error(new_layout)); self.capacity = new_capacity; } @@ -274,14 +289,14 @@ impl BlobVec { /// Gets a [`Ptr`] to the start of the vec #[inline] pub fn get_ptr(&self) -> Ptr<'_> { - // SAFE: the inner data will remain valid for as long as 'self. + // SAFETY: the inner data will remain valid for as long as 'self. unsafe { Ptr::new(self.data) } } /// Gets a [`PtrMut`] to the start of the vec #[inline] pub fn get_ptr_mut(&mut self) -> PtrMut<'_> { - // SAFE: the inner data will remain valid for as long as 'self. + // SAFETY: the inner data will remain valid for as long as 'self. unsafe { PtrMut::new(self.data) } } @@ -290,7 +305,7 @@ impl BlobVec { /// # Safety /// The type `T` must be the type of the items in this [`BlobVec`]. pub unsafe fn get_slice(&self) -> &[UnsafeCell] { - // SAFE: the inner data will remain valid for as long as 'self. + // SAFETY: the inner data will remain valid for as long as 'self. std::slice::from_raw_parts(self.data.as_ptr() as *const UnsafeCell, self.len) } @@ -302,6 +317,7 @@ impl BlobVec { if let Some(drop) = self.drop { let layout_size = self.item_layout.size(); for i in 0..len { + // SAFETY: `i * layout_size` is inbounds for the allocation, and the item is left unreachable so it can be safely promoted to an `OwningPtr` unsafe { // NOTE: this doesn't use self.get_unchecked(i) because the debug_assert on index // will panic here due to self.len being set to 0 @@ -317,6 +333,7 @@ impl Drop for BlobVec { fn drop(&mut self) { self.clear(); if self.item_layout.size() > 0 { + // SAFETY: the `swap_scratch` pointer is always allocated using `self.item_layout` unsafe { std::alloc::dealloc(self.swap_scratch.as_ptr(), self.item_layout); } @@ -324,6 +341,7 @@ impl Drop for BlobVec { let array_layout = array_layout(&self.item_layout, self.capacity).expect("array layout should be valid"); if array_layout.size() > 0 { + // SAFETY: data ptr layout is correct, swap_scratch ptr layout is correct unsafe { std::alloc::dealloc(self.get_ptr_mut().as_ptr(), array_layout); } @@ -427,8 +445,9 @@ mod tests { #[test] fn resize_test() { let item_layout = Layout::new::(); - // usize doesn't need dropping + // SAFETY: `drop` fn is `None`, usize doesn't need dropping let mut blob_vec = unsafe { BlobVec::new(item_layout, None, 64) }; + // SAFETY: `i` is a usize, i.e. the type corresponding to `item_layout` unsafe { for i in 0..1_000 { push(&mut blob_vec, i as usize); @@ -458,8 +477,12 @@ mod tests { { let item_layout = Layout::new::(); let drop = drop_ptr::; + // SAFETY: drop is able to drop a value of its `item_layout` let mut blob_vec = unsafe { BlobVec::new(item_layout, Some(drop), 2) }; assert_eq!(blob_vec.capacity(), 2); + // SAFETY: the following code only deals with values of type `Foo`, which satisfies the safety requirement of `push`, `get_mut` and `swap_remove` that the + // values have a layout compatible to the blob vec's `item_layout`. + // Every index is in range. unsafe { let foo1 = Foo { a: 42, @@ -518,6 +541,7 @@ mod tests { fn blob_vec_drop_empty_capacity() { let item_layout = Layout::new::(); let drop = drop_ptr::; + // SAFETY: drop is able to drop a value of its `item_layout` let _ = unsafe { BlobVec::new(item_layout, Some(drop), 0) }; } } diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index 1d5172b64a624..3ca81979dcf1e 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -159,7 +159,7 @@ impl ComponentSparseSet { let dense_index = *dense_index as usize; #[cfg(debug_assertions)] assert_eq!(entity, self.entities[dense_index]); - // SAFE: if the sparse index points to something in the dense vec, it exists + // SAFETY: if the sparse index points to something in the dense vec, it exists unsafe { self.dense.get_data_unchecked(dense_index) } }) } @@ -169,7 +169,7 @@ impl ComponentSparseSet { let dense_index = *self.sparse.get(entity.id())? as usize; #[cfg(debug_assertions)] assert_eq!(entity, self.entities[dense_index]); - // SAFE: if the sparse index points to something in the dense vec, it exists + // SAFETY: if the sparse index points to something in the dense vec, it exists unsafe { Some(( self.dense.get_data_unchecked(dense_index), @@ -183,7 +183,7 @@ impl ComponentSparseSet { let dense_index = *self.sparse.get(entity.id())? as usize; #[cfg(debug_assertions)] assert_eq!(entity, self.entities[dense_index]); - // SAFE: if the sparse index points to something in the dense vec, it exists + // SAFETY: if the sparse index points to something in the dense vec, it exists unsafe { Some(self.dense.get_ticks_unchecked(dense_index)) } } @@ -197,7 +197,7 @@ impl ComponentSparseSet { assert_eq!(entity, self.entities[dense_index]); self.entities.swap_remove(dense_index); let is_last = dense_index == self.dense.len() - 1; - // SAFE: dense_index was just removed from `sparse`, which ensures that it is valid + // SAFETY: dense_index was just removed from `sparse`, which ensures that it is valid let (value, _) = unsafe { self.dense.swap_remove_and_forget_unchecked(dense_index) }; if !is_last { let swapped_entity = self.entities[dense_index]; @@ -218,7 +218,7 @@ impl ComponentSparseSet { assert_eq!(entity, self.entities[dense_index]); self.entities.swap_remove(dense_index); let is_last = dense_index == self.dense.len() - 1; - // SAFE: if the sparse index points to something in the dense vec, it exists + // SAFETY: if the sparse index points to something in the dense vec, it exists unsafe { self.dense.swap_remove_unchecked(dense_index) } if !is_last { let swapped_entity = self.entities[dense_index]; @@ -280,7 +280,7 @@ impl SparseSet { pub fn insert(&mut self, index: I, value: V) { if let Some(dense_index) = self.sparse.get(index.clone()).cloned() { - // SAFE: dense indices stored in self.sparse always exist + // SAFETY: dense indices stored in self.sparse always exist unsafe { *self.dense.get_unchecked_mut(dense_index) = value; } @@ -293,7 +293,7 @@ impl SparseSet { pub fn get_or_insert_with(&mut self, index: I, func: impl FnOnce() -> V) -> &mut V { if let Some(dense_index) = self.sparse.get(index.clone()).cloned() { - // SAFE: dense indices stored in self.sparse always exist + // SAFETY: dense indices stored in self.sparse always exist unsafe { self.dense.get_unchecked_mut(dense_index) } } else { let value = func(); @@ -301,7 +301,7 @@ impl SparseSet { self.sparse.insert(index.clone(), dense_index); self.indices.push(index); self.dense.push(value); - // SAFE: dense index was just populated above + // SAFETY: dense index was just populated above unsafe { self.dense.get_unchecked_mut(dense_index) } } } @@ -323,7 +323,7 @@ impl SparseSet { pub fn get(&self, index: I) -> Option<&V> { self.sparse.get(index).map(|dense_index| { - // SAFE: if the sparse index points to something in the dense vec, it exists + // SAFETY: if the sparse index points to something in the dense vec, it exists unsafe { self.dense.get_unchecked(*dense_index) } }) } @@ -331,7 +331,7 @@ impl SparseSet { pub fn get_mut(&mut self, index: I) -> Option<&mut V> { let dense = &mut self.dense; self.sparse.get(index).map(move |dense_index| { - // SAFE: if the sparse index points to something in the dense vec, it exists + // SAFETY: if the sparse index points to something in the dense vec, it exists unsafe { dense.get_unchecked_mut(*dense_index) } }) } diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index 4b716c6814b04..245f14be02d80 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -42,7 +42,7 @@ impl Column { #[inline] pub(crate) fn with_capacity(component_info: &ComponentInfo, capacity: usize) -> Self { Column { - // SAFE: component_info.drop() is valid for the types that will be inserted. + // SAFETY: component_info.drop() is valid for the types that will be inserted. data: unsafe { BlobVec::new(component_info.layout(), component_info.drop(), capacity) }, ticks: Vec::with_capacity(capacity), } @@ -559,7 +559,7 @@ mod tests { table.add_column(components.get_info(component_id).unwrap()); let entities = (0..200).map(Entity::from_raw).collect::>(); for entity in &entities { - // SAFE: we allocate and immediately set data afterwards + // SAFETY: we allocate and immediately set data afterwards unsafe { let row = table.allocate(*entity); let value: W = W(row); diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index d73b529253f66..fa21351fb2295 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -21,10 +21,10 @@ pub struct CommandQueue { metas: Vec, } -// SAFE: All commands [`Command`] implement [`Send`] +// SAFETY: All commands [`Command`] implement [`Send`] unsafe impl Send for CommandQueue {} -// SAFE: `&CommandQueue` never gives access to the inner commands. +// SAFETY: `&CommandQueue` never gives access to the inner commands. unsafe impl Sync for CommandQueue {} impl CommandQueue { @@ -34,7 +34,7 @@ impl CommandQueue { where C: Command, { - /// SAFE: This function is only every called when the `command` bytes is the associated + /// SAFETY: This function is only every called when the `command` bytes is the associated /// [`Commands`] `T` type. Also this only reads the data via `read_unaligned` so unaligned /// accesses are safe. unsafe fn write_command(command: *mut MaybeUninit, world: &mut World) { @@ -57,7 +57,7 @@ impl CommandQueue { if size > 0 { self.bytes.reserve(size); - // SAFE: The internal `bytes` vector has enough storage for the + // SAFETY: The internal `bytes` vector has enough storage for the // command (see the call the `reserve` above), the vector has // its length set appropriately and can contain any kind of bytes. // In case we're writing a ZST and the `Vec` hasn't allocated yet @@ -83,13 +83,13 @@ impl CommandQueue { // flush the previously queued entities world.flush(); - // SAFE: In the iteration below, `meta.func` will safely consume and drop each pushed command. + // SAFETY: In the iteration below, `meta.func` will safely consume and drop each pushed command. // This operation is so that we can reuse the bytes `Vec`'s internal storage and prevent // unnecessary allocations. unsafe { self.bytes.set_len(0) }; for meta in self.metas.drain(..) { - // SAFE: The implementation of `write_command` is safe for the according Command type. + // SAFETY: The implementation of `write_command` is safe for the according Command type. // It's ok to read from `bytes.as_mut_ptr()` because we just wrote to it in `push`. // The bytes are safely cast to their original type, safely read, and then dropped. unsafe { diff --git a/crates/bevy_ecs/src/system/commands/parallel_scope.rs b/crates/bevy_ecs/src/system/commands/parallel_scope.rs index 41dc9b7289192..02878041c4b19 100644 --- a/crates/bevy_ecs/src/system/commands/parallel_scope.rs +++ b/crates/bevy_ecs/src/system/commands/parallel_scope.rs @@ -68,7 +68,7 @@ impl<'w, 's> SystemParamFetch<'w, 's> for ParallelCommandsState { } } -// SAFE: no component or resource access to report +// SAFETY: no component or resource access to report unsafe impl SystemParamState for ParallelCommandsState { fn init(_: &mut World, _: &mut crate::system::SystemMeta) -> Self { Self::default() diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 9dec1b5f84e3b..356c891dc3b50 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -166,7 +166,7 @@ impl SystemState { Param::Fetch: ReadOnlySystemParamFetch, { self.validate_world_and_update_archetypes(world); - // SAFE: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with. + // SAFETY: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with. unsafe { self.get_unchecked_manual(world) } } @@ -177,7 +177,7 @@ impl SystemState { world: &'w mut World, ) -> >::Item { self.validate_world_and_update_archetypes(world); - // SAFE: World is uniquely borrowed and matches the World this SystemState was created with. + // SAFETY: World is uniquely borrowed and matches the World this SystemState was created with. unsafe { self.get_unchecked_manual(world) } } diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index fd4ecb408e5e9..024926fa9bb89 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -293,7 +293,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn iter(&self) -> QueryIter<'_, 's, Q, ROQueryFetch<'_, Q>, F> { - // SAFE: system runs without conflicts with other systems. + // SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { self.state @@ -323,7 +323,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn iter_mut(&mut self) -> QueryIter<'_, '_, Q, QueryFetch<'_, Q>, F> { - // SAFE: system runs without conflicts with other systems. + // SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { self.state @@ -340,7 +340,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// - if K > N: empty set (no K-sized combinations exist) #[inline] pub fn iter_combinations(&self) -> QueryCombinationIter<'_, '_, Q, F, K> { - // SAFE: system runs without conflicts with other systems. + // SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { self.state.iter_combinations_unchecked_manual( @@ -377,7 +377,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { pub fn iter_combinations_mut( &mut self, ) -> QueryCombinationIter<'_, '_, Q, F, K> { - // SAFE: system runs without conflicts with other systems. + // SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { self.state.iter_combinations_unchecked_manual( @@ -446,7 +446,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// this call does not result in multiple mutable references to the same component #[inline] pub unsafe fn iter_unsafe(&'s self) -> QueryIter<'w, 's, Q, QueryFetch<'w, Q>, F> { - // SEMI-SAFE: system runs without conflicts with other systems. + // SEMI-SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict self.state .iter_unchecked_manual(self.world, self.last_change_tick, self.change_tick) @@ -462,7 +462,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { pub unsafe fn iter_combinations_unsafe( &self, ) -> QueryCombinationIter<'_, '_, Q, F, K> { - // SEMI-SAFE: system runs without conflicts with other systems. + // SEMI-SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict self.state.iter_combinations_unchecked_manual( self.world, @@ -519,7 +519,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn for_each<'this>(&'this self, f: impl FnMut(ROQueryItem<'this, Q>)) { - // SAFE: system runs without conflicts with other systems. + // SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { self.state.for_each_unchecked_manual::, _>( @@ -554,7 +554,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn for_each_mut<'a, FN: FnMut(QueryItem<'a, Q>)>(&'a mut self, f: FN) { - // SAFE: system runs without conflicts with other systems. same-system queries have runtime + // SAFETY: system runs without conflicts with other systems. same-system queries have runtime // borrow checks when they conflict unsafe { self.state.for_each_unchecked_manual::, FN>( @@ -597,7 +597,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { batch_size: usize, f: impl Fn(ROQueryItem<'this, Q>) + Send + Sync + Clone, ) { - // SAFE: system runs without conflicts with other systems. same-system queries have runtime + // SAFETY: system runs without conflicts with other systems. same-system queries have runtime // borrow checks when they conflict unsafe { self.state @@ -625,7 +625,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { batch_size: usize, f: FN, ) { - // SAFE: system runs without conflicts with other systems. same-system queries have runtime + // SAFETY: system runs without conflicts with other systems. same-system queries have runtime // borrow checks when they conflict unsafe { self.state @@ -675,7 +675,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { ) where EntityList::Item: Borrow, { - // SAFE: system runs without conflicts with other systems. + // SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { self.state.many_for_each_unchecked_manual( @@ -721,7 +721,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn get(&self, entity: Entity) -> Result, QueryEntityError> { - // SAFE: system runs without conflicts with other systems. + // SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { self.state.get_unchecked_manual::>( @@ -746,7 +746,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { &self, entities: [Entity; N], ) -> Result<[ROQueryItem<'_, Q>; N], QueryEntityError> { - // SAFE: it is the scheduler's responsibility to ensure that `Query` is never handed out on the wrong `World`. + // SAFETY: it is the scheduler's responsibility to ensure that `Query` is never handed out on the wrong `World`. unsafe { self.state.get_many_read_only_manual( self.world, @@ -823,7 +823,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn get_mut(&mut self, entity: Entity) -> Result, QueryEntityError> { - // SAFE: system runs without conflicts with other systems. + // SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { self.state.get_unchecked_manual::>( @@ -846,7 +846,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { &mut self, entities: [Entity; N], ) -> Result<[QueryItem<'_, Q>; N], QueryEntityError> { - // SAFE: scheduler ensures safe Query world access + // SAFETY: scheduler ensures safe Query world access unsafe { self.state.get_many_unchecked_manual( self.world, @@ -917,7 +917,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { &'s self, entity: Entity, ) -> Result, QueryEntityError> { - // SEMI-SAFE: system runs without conflicts with other systems. + // SEMI-SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict self.state.get_unchecked_manual::>( self.world, @@ -1011,7 +1011,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { &mut self, entity: Entity, ) -> Result, QueryComponentError> { - // SAFE: unique access to query (preventing aliased access) + // SAFETY: unique access to query (preventing aliased access) unsafe { self.get_component_unchecked_mut(entity) } } @@ -1117,6 +1117,9 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn get_single(&self) -> Result, QuerySingleError> { + // SAFETY: + // the query ensures that the components it accesses are not mutably accessible somewhere else + // and the query is read only. unsafe { self.state.get_single_unchecked_manual::>( self.world, @@ -1179,6 +1182,9 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn get_single_mut(&mut self) -> Result, QuerySingleError> { + // SAFETY: + // the query ensures mutable access to the components it accesses, and the query + // is uniquely borrowed unsafe { self.state.get_single_unchecked_manual::>( self.world, @@ -1238,7 +1244,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn contains(&self, entity: Entity) -> bool { - // SAFE: NopFetch does not access any members while &self ensures no one has exclusive access + // SAFETY: NopFetch does not access any members while &self ensures no one has exclusive access unsafe { self.state .get_unchecked_manual::>( @@ -1340,7 +1346,7 @@ impl<'w, 's, Q: ReadOnlyWorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn get_inner(&'s self, entity: Entity) -> Result, QueryEntityError> { - // SAFE: system runs without conflicts with other systems. + // SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { self.state.get_unchecked_manual::>( @@ -1377,7 +1383,7 @@ impl<'w, 's, Q: ReadOnlyWorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn iter_inner(&'s self) -> QueryIter<'w, 's, Q, ROQueryFetch<'w, Q>, F> { - // SAFE: system runs without conflicts with other systems. + // SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { self.state diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 44b719e52617f..2fbeba23820df 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -46,7 +46,7 @@ pub trait System: Send + Sync + 'static { /// Runs the system with the given input in the world. fn run(&mut self, input: Self::In, world: &mut World) -> Self::Out { self.update_archetype_component_access(world); - // SAFE: world and resources are exclusively borrowed + // SAFETY: world and resources are exclusively borrowed unsafe { self.run_unsafe(input, world) } } fn apply_buffers(&mut self, world: &mut World); diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 96a574dd562df..c3a419f0a8e20 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -134,10 +134,10 @@ impl<'w, 's, Q: WorldQuery + 'static, F: WorldQuery + 'static> SystemParam for Q type Fetch = QueryState; } -// SAFE: QueryState is constrained to read-only fetches, so it only reads World. +// SAFETY: QueryState is constrained to read-only fetches, so it only reads World. unsafe impl ReadOnlySystemParamFetch for QueryState {} -// SAFE: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If +// SAFETY: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If // this QueryState conflicts with any prior access, a panic will occur. unsafe impl SystemParamState for QueryState @@ -239,7 +239,7 @@ pub struct Res<'w, T: Resource> { change_tick: u32, } -// SAFE: Res only reads a single World resource +// SAFETY: Res only reads a single World resource unsafe impl ReadOnlySystemParamFetch for ResState {} impl<'w, T: Resource> Debug for Res<'w, T> @@ -305,7 +305,7 @@ impl<'a, T: Resource> SystemParam for Res<'a, T> { type Fetch = ResState; } -// SAFE: Res ComponentId and ArchetypeComponentId access is applied to SystemMeta. If this Res +// SAFETY: Res ComponentId and ArchetypeComponentId access is applied to SystemMeta. If this Res // conflicts with any prior access, a panic will occur. unsafe impl SystemParamState for ResState { fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { @@ -370,9 +370,11 @@ impl<'a, T: Resource> SystemParam for Option> { type Fetch = OptionResState; } -// SAFE: Only reads a single World resource +// SAFETY: Only reads a single World resource unsafe impl ReadOnlySystemParamFetch for OptionResState {} +// SAFETY: this impl defers to `ResState`, which initializes +// and validates the correct world access unsafe impl SystemParamState for OptionResState { fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { Self(ResState::init(world, system_meta)) @@ -411,7 +413,7 @@ impl<'a, T: Resource> SystemParam for ResMut<'a, T> { type Fetch = ResMutState; } -// SAFE: Res ComponentId and ArchetypeComponentId access is applied to SystemMeta. If this Res +// SAFETY: Res ComponentId and ArchetypeComponentId access is applied to SystemMeta. If this Res // conflicts with any prior access, a panic will occur. unsafe impl SystemParamState for ResMutState { fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { @@ -481,6 +483,8 @@ impl<'a, T: Resource> SystemParam for Option> { type Fetch = OptionResMutState; } +// SAFETY: this impl defers to `ResMutState`, which initializes +// and validates the correct world access unsafe impl SystemParamState for OptionResMutState { fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { Self(ResMutState::init(world, system_meta)) @@ -514,10 +518,10 @@ impl<'w, 's> SystemParam for Commands<'w, 's> { type Fetch = CommandQueue; } -// SAFE: Commands only accesses internal state +// SAFETY: Commands only accesses internal state unsafe impl ReadOnlySystemParamFetch for CommandQueue {} -// SAFE: only local state is accessed +// SAFETY: only local state is accessed unsafe impl SystemParamState for CommandQueue { fn init(_world: &mut World, _system_meta: &mut SystemMeta) -> Self { Default::default() @@ -542,7 +546,7 @@ impl<'w, 's> SystemParamFetch<'w, 's> for CommandQueue { } } -/// SAFE: only reads world +/// SAFETY: only reads world unsafe impl ReadOnlySystemParamFetch for WorldState {} /// The [`SystemParamState`] of [`&World`](crate::world::World). @@ -553,6 +557,7 @@ impl<'w> SystemParam for &'w World { type Fetch = WorldState; } +// SAFETY: `read_all` access is set and conflicts result in a panic unsafe impl SystemParamState for WorldState { fn init(_world: &mut World, system_meta: &mut SystemMeta) -> Self { let mut access = Access::default(); @@ -637,7 +642,7 @@ impl<'w, 's> SystemParamFetch<'w, 's> for WorldState { /// ``` pub struct Local<'a, T: Resource>(&'a mut T); -// SAFE: Local only accesses internal state +// SAFETY: Local only accesses internal state unsafe impl ReadOnlySystemParamFetch for LocalState {} impl<'a, T: Resource> Debug for Local<'a, T> @@ -673,7 +678,7 @@ impl<'a, T: Resource + FromWorld> SystemParam for Local<'a, T> { type Fetch = LocalState; } -// SAFE: only local state is accessed +// SAFETY: only local state is accessed unsafe impl SystemParamState for LocalState { fn init(world: &mut World, _system_meta: &mut SystemMeta) -> Self { Self(T::from_world(world)) @@ -741,7 +746,7 @@ impl<'a, T: Component> RemovedComponents<'a, T> { } } -// SAFE: Only reads World components +// SAFETY: Only reads World components unsafe impl ReadOnlySystemParamFetch for RemovedComponentsState {} /// The [`SystemParamState`] of [`RemovedComponents`]. @@ -755,7 +760,7 @@ impl<'a, T: Component> SystemParam for RemovedComponents<'a, T> { type Fetch = RemovedComponentsState; } -// SAFE: no component access. removed component entity collections can be read in parallel and are +// SAFETY: no component access. removed component entity collections can be read in parallel and are // never mutably borrowed during system execution unsafe impl SystemParamState for RemovedComponentsState { fn init(world: &mut World, _system_meta: &mut SystemMeta) -> Self { @@ -803,7 +808,7 @@ pub struct NonSend<'w, T: 'static> { change_tick: u32, } -// SAFE: Only reads a single World non-send resource +// SAFETY: Only reads a single World non-send resource unsafe impl ReadOnlySystemParamFetch for NonSendState {} impl<'w, T> Debug for NonSend<'w, T> @@ -857,7 +862,7 @@ impl<'a, T: 'static> SystemParam for NonSend<'a, T> { type Fetch = NonSendState; } -// SAFE: NonSendComponentId and ArchetypeComponentId access is applied to SystemMeta. If this +// SAFETY: NonSendComponentId and ArchetypeComponentId access is applied to SystemMeta. If this // NonSend conflicts with any prior access, a panic will occur. unsafe impl SystemParamState for NonSendState { fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { @@ -926,9 +931,11 @@ impl<'w, T: 'static> SystemParam for Option> { type Fetch = OptionNonSendState; } -// SAFE: Only reads a single non-send resource +// SAFETY: Only reads a single non-send resource unsafe impl ReadOnlySystemParamFetch for OptionNonSendState {} +// SAFETY: this impl defers to `NonSendState`, which initializes +// and validates the correct world access unsafe impl SystemParamState for OptionNonSendState { fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { Self(NonSendState::init(world, system_meta)) @@ -968,7 +975,7 @@ impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> { type Fetch = NonSendMutState; } -// SAFE: NonSendMut ComponentId and ArchetypeComponentId access is applied to SystemMeta. If this +// SAFETY: NonSendMut ComponentId and ArchetypeComponentId access is applied to SystemMeta. If this // NonSendMut conflicts with any prior access, a panic will occur. unsafe impl SystemParamState for NonSendMutState { fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { @@ -1041,6 +1048,8 @@ impl<'a, T: 'static> SystemParam for Option> { type Fetch = OptionNonSendMutState; } +// SAFETY: this impl defers to `NonSendMutState`, which initializes +// and validates the correct world access unsafe impl SystemParamState for OptionNonSendMutState { fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { Self(NonSendMutState::init(world, system_meta)) @@ -1075,14 +1084,14 @@ impl<'a> SystemParam for &'a Archetypes { type Fetch = ArchetypesState; } -// SAFE: Only reads World archetypes +// SAFETY: Only reads World archetypes unsafe impl ReadOnlySystemParamFetch for ArchetypesState {} /// The [`SystemParamState`] of [`Archetypes`]. #[doc(hidden)] pub struct ArchetypesState; -// SAFE: no component value access +// SAFETY: no component value access unsafe impl SystemParamState for ArchetypesState { fn init(_world: &mut World, _system_meta: &mut SystemMeta) -> Self { Self @@ -1107,14 +1116,14 @@ impl<'a> SystemParam for &'a Components { type Fetch = ComponentsState; } -// SAFE: Only reads World components +// SAFETY: Only reads World components unsafe impl ReadOnlySystemParamFetch for ComponentsState {} /// The [`SystemParamState`] of [`Components`]. #[doc(hidden)] pub struct ComponentsState; -// SAFE: no component value access +// SAFETY: no component value access unsafe impl SystemParamState for ComponentsState { fn init(_world: &mut World, _system_meta: &mut SystemMeta) -> Self { Self @@ -1139,14 +1148,14 @@ impl<'a> SystemParam for &'a Entities { type Fetch = EntitiesState; } -// SAFE: Only reads World entities +// SAFETY: Only reads World entities unsafe impl ReadOnlySystemParamFetch for EntitiesState {} /// The [`SystemParamState`] of [`Entities`]. #[doc(hidden)] pub struct EntitiesState; -// SAFE: no component value access +// SAFETY: no component value access unsafe impl SystemParamState for EntitiesState { fn init(_world: &mut World, _system_meta: &mut SystemMeta) -> Self { Self @@ -1171,14 +1180,14 @@ impl<'a> SystemParam for &'a Bundles { type Fetch = BundlesState; } -// SAFE: Only reads World bundles +// SAFETY: Only reads World bundles unsafe impl ReadOnlySystemParamFetch for BundlesState {} /// The [`SystemParamState`] of [`Bundles`]. #[doc(hidden)] pub struct BundlesState; -// SAFE: no component value access +// SAFETY: no component value access unsafe impl SystemParamState for BundlesState { fn init(_world: &mut World, _system_meta: &mut SystemMeta) -> Self { Self @@ -1228,7 +1237,7 @@ impl SystemChangeTick { } } -// SAFE: Only reads internal system state +// SAFETY: Only reads internal system state unsafe impl ReadOnlySystemParamFetch for SystemChangeTickState {} impl SystemParam for SystemChangeTick { @@ -1239,6 +1248,7 @@ impl SystemParam for SystemChangeTick { #[doc(hidden)] pub struct SystemChangeTickState {} +// SAFETY: `SystemParamTickState` doesn't require any world access unsafe impl SystemParamState for SystemChangeTickState { fn init(_world: &mut World, _system_meta: &mut SystemMeta) -> Self { Self {} @@ -1267,7 +1277,7 @@ macro_rules! impl_system_param_tuple { type Fetch = ($($param::Fetch,)*); } - // SAFE: tuple consists only of ReadOnlySystemParamFetches + // SAFETY: tuple consists only of ReadOnlySystemParamFetches unsafe impl<$($param: ReadOnlySystemParamFetch),*> ReadOnlySystemParamFetch for ($($param,)*) {} #[allow(unused_variables)] @@ -1289,7 +1299,8 @@ macro_rules! impl_system_param_tuple { } } - /// SAFE: implementors of each `SystemParamState` in the tuple have validated their impls + // SAFETY: implementors of each `SystemParamState` in the tuple have validated their impls + #[allow(clippy::undocumented_unsafe_blocks)] // false positive by clippy #[allow(non_snake_case)] unsafe impl<$($param: SystemParamState),*> SystemParamState for ($($param,)*) { #[inline] @@ -1403,7 +1414,7 @@ impl<'w, 's, P: SystemParam> StaticSystemParam<'w, 's, P> { #[doc(hidden)] pub struct StaticSystemParamState(S, PhantomData P>); -// Safe: This doesn't add any more reads, and the delegated fetch confirms it +// SAFETY: This doesn't add any more reads, and the delegated fetch confirms it unsafe impl ReadOnlySystemParamFetch for StaticSystemParamState { @@ -1428,11 +1439,12 @@ where world: &'world World, change_tick: u32, ) -> Self::Item { - // Safe: We properly delegate SystemParamState + // SAFETY: We properly delegate SystemParamState StaticSystemParam(S::get_param(&mut state.0, system_meta, world, change_tick)) } } +// SAFETY: all methods are just delegated to `S`'s `SystemParamState` implementation unsafe impl SystemParamState for StaticSystemParamState { diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index bf2701714190d..7d1051bba1988 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -65,7 +65,7 @@ impl<'w> EntityRef<'w> { #[inline] pub fn get(&self) -> Option<&'w T> { - // SAFE: entity location is valid and returned component is of type T + // SAFETY: entity location is valid and returned component is of type T unsafe { get_component_with_type(self.world, TypeId::of::(), self.entity, self.location) .map(|value| value.deref::()) @@ -76,7 +76,7 @@ impl<'w> EntityRef<'w> { /// detection in custom runtimes. #[inline] pub fn get_change_ticks(&self) -> Option<&'w ComponentTicks> { - // SAFE: entity location is valid + // SAFETY: entity location is valid unsafe { get_ticks_with_type(self.world, TypeId::of::(), self.entity, self.location) .map(|ticks| ticks.deref()) @@ -123,7 +123,7 @@ impl<'w> EntityRef<'w> { #[inline] pub fn get_by_id(&self, component_id: ComponentId) -> Option> { self.world.components().get_info(component_id)?; - // SAFE: entity_location is valid, component_id is valid as checked by the line above + // SAFETY: entity_location is valid, component_id is valid as checked by the line above unsafe { get_component(self.world, component_id, self.entity, self.location) } } } @@ -184,7 +184,7 @@ impl<'w> EntityMut<'w> { #[inline] pub fn get(&self) -> Option<&'_ T> { - // SAFE: lifetimes enforce correct usage of returned borrow + // SAFETY: lifetimes enforce correct usage of returned borrow unsafe { get_component_with_type(self.world, TypeId::of::(), self.entity, self.location) .map(|value| value.deref::()) @@ -193,7 +193,7 @@ impl<'w> EntityMut<'w> { #[inline] pub fn get_mut(&mut self) -> Option> { - // SAFE: world access is unique, and lifetimes enforce correct usage of returned borrow + // SAFETY: world access is unique, and lifetimes enforce correct usage of returned borrow unsafe { self.get_unchecked_mut::() } } @@ -201,7 +201,7 @@ impl<'w> EntityMut<'w> { /// detection in custom runtimes. #[inline] pub fn get_change_ticks(&self) -> Option<&ComponentTicks> { - // SAFE: entity location is valid + // SAFETY: entity location is valid unsafe { get_ticks_with_type(self.world, TypeId::of::(), self.entity, self.location) .map(|ticks| ticks.deref()) @@ -245,7 +245,7 @@ impl<'w> EntityMut<'w> { self.location.archetype_id, change_tick, ); - // SAFE: location matches current entity. `T` matches `bundle_info` + // SAFETY: location matches current entity. `T` matches `bundle_info` unsafe { self.location = bundle_inserter.insert(self.entity, self.location.index, bundle); } @@ -263,6 +263,8 @@ impl<'w> EntityMut<'w> { let bundle_info = self.world.bundles.init_info::(components, storages); let old_location = self.location; + // SAFETY: `archetype_id` exists because it is referenced in the old `EntityLocation` which is valid, + // components exist in `bundle_info` because `Bundles::init_info` initializes a `BundleInfo` containing all components of the bundle type `T` let new_archetype_id = unsafe { remove_bundle_from_archetype( archetypes, @@ -281,12 +283,12 @@ impl<'w> EntityMut<'w> { let old_archetype = &mut archetypes[old_location.archetype_id]; let mut bundle_components = bundle_info.component_ids.iter().cloned(); let entity = self.entity; - // SAFE: bundle components are iterated in order, which guarantees that the component type + // SAFETY: bundle components are iterated in order, which guarantees that the component type // matches let result = unsafe { T::from_components(storages, |storages| { let component_id = bundle_components.next().unwrap(); - // SAFE: entity location is valid and table row is removed below + // SAFETY: entity location is valid and table row is removed below take_component( components, storages, @@ -299,6 +301,7 @@ impl<'w> EntityMut<'w> { }) }; + #[allow(clippy::undocumented_unsafe_blocks)] // TODO: document why this is safe unsafe { Self::move_entity_from_remove::( entity, @@ -350,14 +353,14 @@ impl<'w> EntityMut<'w> { .tables .get_2_mut(old_table_id, new_archetype.table_id()); - // SAFE: old_table_row exists + // SAFETY: old_table_row exists let move_result = if DROP { old_table.move_to_and_drop_missing_unchecked(old_table_row, new_table) } else { old_table.move_to_and_forget_missing_unchecked(old_table_row, new_table) }; - // SAFE: move_result.new_row is a valid position in new_archetype's table + // SAFETY: move_result.new_row is a valid position in new_archetype's table let new_location = new_archetype.allocate(entity, move_result.new_row); // if an entity was moved into this entity's table spot, update its table row @@ -385,6 +388,9 @@ impl<'w> EntityMut<'w> { let bundle_info = self.world.bundles.init_info::(components, storages); let old_location = self.location; + + // SAFETY: `archetype_id` exists because it is referenced in the old `EntityLocation` which is valid, + // components exist in `bundle_info` because `Bundles::init_info` initializes a `BundleInfo` containing all components of the bundle type `T` let new_archetype_id = unsafe { remove_bundle_from_archetype( archetypes, @@ -421,6 +427,7 @@ impl<'w> EntityMut<'w> { } } + #[allow(clippy::undocumented_unsafe_blocks)] // TODO: document why this is safe unsafe { Self::move_entity_from_remove::( entity, @@ -470,7 +477,7 @@ impl<'w> EntityMut<'w> { let sparse_set = world.storages.sparse_sets.get_mut(*component_id).unwrap(); sparse_set.remove(self.entity); } - // SAFE: table rows stored in archetypes always exist + // SAFETY: table rows stored in archetypes always exist moved_entity = unsafe { world.storages.tables[archetype.table_id()].swap_remove_unchecked(table_row) }; @@ -517,7 +524,7 @@ impl<'w> EntityMut<'w> { #[inline] pub fn get_by_id(&self, component_id: ComponentId) -> Option> { self.world.components().get_info(component_id)?; - // SAFE: entity_location is valid, component_id is valid as checked by the line above + // SAFETY: entity_location is valid, component_id is valid as checked by the line above unsafe { get_component(self.world, component_id, self.entity, self.location) } } @@ -532,7 +539,7 @@ impl<'w> EntityMut<'w> { #[inline] pub fn get_mut_by_id(&mut self, component_id: ComponentId) -> Option> { self.world.components().get_info(component_id)?; - // SAFE: entity_location is valid, component_id is valid as checked by the line above + // SAFETY: entity_location is valid, component_id is valid as checked by the line above unsafe { get_mut_by_id(self.world, self.entity, self.location, component_id) } } } @@ -552,14 +559,14 @@ pub(crate) unsafe fn get_component( location: EntityLocation, ) -> Option> { let archetype = &world.archetypes[location.archetype_id]; - // SAFE: component_id exists and is therefore valid + // SAFETY: component_id exists and is therefore valid let component_info = world.components.get_info_unchecked(component_id); match component_info.storage_type() { StorageType::Table => { let table = &world.storages.tables[archetype.table_id()]; let components = table.get_column(component_id)?; let table_row = archetype.entity_table_row(location.index); - // SAFE: archetypes only store valid table_rows and the stored component type is T + // SAFETY: archetypes only store valid table_rows and the stored component type is T Some(components.get_data_unchecked(table_row)) } StorageType::SparseSet => world @@ -589,7 +596,7 @@ unsafe fn get_component_and_ticks( let table = &world.storages.tables[archetype.table_id()]; let components = table.get_column(component_id)?; let table_row = archetype.entity_table_row(location.index); - // SAFE: archetypes only store valid table_rows and the stored component type is T + // SAFETY: archetypes only store valid table_rows and the stored component type is T Some(( components.get_data_unchecked(table_row), components.get_ticks_unchecked(table_row), @@ -617,7 +624,7 @@ unsafe fn get_ticks( let table = &world.storages.tables[archetype.table_id()]; let components = table.get_column(component_id)?; let table_row = archetype.entity_table_row(location.index); - // SAFE: archetypes only store valid table_rows and the stored component type is T + // SAFETY: archetypes only store valid table_rows and the stored component type is T Some(components.get_ticks_unchecked(table_row)) } StorageType::SparseSet => world @@ -655,10 +662,10 @@ unsafe fn take_component<'a>( match component_info.storage_type() { StorageType::Table => { let table = &mut storages.tables[archetype.table_id()]; - // SAFE: archetypes will always point to valid columns + // SAFETY: archetypes will always point to valid columns let components = table.get_column_mut(component_id).unwrap(); let table_row = archetype.entity_table_row(location.index); - // SAFE: archetypes only store valid table_rows and the stored component type is T + // SAFETY: archetypes only store valid table_rows and the stored component type is T components.get_data_unchecked_mut(table_row).promote() } StorageType::SparseSet => storages @@ -768,7 +775,7 @@ unsafe fn remove_bundle_from_archetype( let mut removed_sparse_set_components = Vec::new(); for component_id in bundle_info.component_ids.iter().cloned() { if current_archetype.contains(component_id) { - // SAFE: bundle components were already initialized by bundles.get_info + // SAFETY: bundle components were already initialized by bundles.get_info let component_info = components.get_info_unchecked(component_id); match component_info.storage_type() { StorageType::Table => removed_table_components.push(component_id), @@ -800,7 +807,7 @@ unsafe fn remove_bundle_from_archetype( next_table_id = if removed_table_components.is_empty() { current_archetype.table_id() } else { - // SAFE: all components in next_table_components exist + // SAFETY: all components in next_table_components exist storages .tables .get_id_or_insert(&next_table_components, components) @@ -850,7 +857,7 @@ pub(crate) unsafe fn get_mut( entity: Entity, location: EntityLocation, ) -> Option> { - // SAFE: world access is unique, entity location is valid, and returned component is of type + // SAFETY: world access is unique, entity location is valid, and returned component is of type // T let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); @@ -874,7 +881,7 @@ pub(crate) unsafe fn get_mut_by_id( location: EntityLocation, component_id: ComponentId, ) -> Option { - // SAFE: world access is unique, entity location and component_id required to be valid + // SAFETY: world access is unique, entity location and component_id required to be valid get_component_and_ticks(world, component_id, entity, location).map(|(value, ticks)| { MutUntyped { value: value.assert_unique(), @@ -928,7 +935,7 @@ mod tests { let entity = world.entity(entity); let test_component = entity.get_by_id(component_id).unwrap(); - // SAFE: points to a valid `TestComponent` + // SAFETY: points to a valid `TestComponent` let test_component = unsafe { test_component.deref::() }; assert_eq!(test_component.0, 42); @@ -947,14 +954,15 @@ mod tests { let mut test_component = entity_mut.get_mut_by_id(component_id).unwrap(); { test_component.set_changed(); - // SAFE: `test_component` has unique access of the `EntityMut` and is not used afterwards let test_component = + // SAFETY: `test_component` has unique access of the `EntityMut` and is not used afterwards unsafe { test_component.into_inner().deref_mut::() }; test_component.0 = 43; } let entity = world.entity(entity); let test_component = entity.get_by_id(component_id).unwrap(); + // SAFETY: `TestComponent` is the correct component type let test_component = unsafe { test_component.deref::() }; assert_eq!(test_component.0, 43); diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index d220495a82e1f..97bc75ebbb1b2 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -317,11 +317,11 @@ impl World { self.flush(); match self.entities.alloc_at_without_replacement(entity) { AllocAtWithoutReplacement::Exists(location) => { - // SAFE: `entity` exists and `location` is that entity's location + // SAFETY: `entity` exists and `location` is that entity's location Some(unsafe { EntityMut::new(self, entity, location) }) } AllocAtWithoutReplacement::DidNotExist => { - // SAFE: entity was just allocated + // SAFETY: entity was just allocated Some(unsafe { self.spawn_at_internal(entity) }) } AllocAtWithoutReplacement::ExistsWithWrongGeneration => None, @@ -381,7 +381,7 @@ impl World { #[inline] pub fn get_entity_mut(&mut self, entity: Entity) -> Option { let location = self.entities.get(entity)?; - // SAFE: `entity` exists and `location` is that entity's location + // SAFETY: `entity` exists and `location` is that entity's location Some(unsafe { EntityMut::new(self, entity, location) }) } @@ -413,7 +413,7 @@ impl World { pub fn spawn(&mut self) -> EntityMut { self.flush(); let entity = self.entities.alloc(); - // SAFE: entity was just allocated + // SAFETY: entity was just allocated unsafe { self.spawn_at_internal(entity) } } @@ -423,10 +423,10 @@ impl World { let archetype = self.archetypes.empty_mut(); // PERF: consider avoiding allocating entities in the empty archetype unless needed let table_row = self.storages.tables[archetype.table_id()].allocate(entity); - // SAFE: no components are allocated by archetype.allocate() because the archetype is + // SAFETY: no components are allocated by archetype.allocate() because the archetype is // empty let location = archetype.allocate(entity, table_row); - // SAFE: entity index was just allocated + // SAFETY: entity index was just allocated self.entities .meta .get_unchecked_mut(entity.id() as usize) @@ -507,7 +507,7 @@ impl World { /// ``` #[inline] pub fn get_mut(&mut self, entity: Entity) -> Option> { - // SAFE: lifetimes enforce correct usage of returned borrow + // SAFETY: lifetimes enforce correct usage of returned borrow unsafe { get_mut(self, entity, self.get_entity(entity)?.location()) } } @@ -688,7 +688,7 @@ impl World { #[inline] pub fn insert_resource(&mut self, value: R) { let component_id = self.components.init_resource::(); - // SAFE: component_id just initialized and corresponds to resource of type T + // SAFETY: component_id just initialized and corresponds to resource of type T unsafe { self.insert_resource_with_id(component_id, value) }; } @@ -716,21 +716,21 @@ impl World { pub fn insert_non_send_resource(&mut self, value: R) { self.validate_non_send_access::(); let component_id = self.components.init_non_send::(); - // SAFE: component_id just initialized and corresponds to resource of type R + // SAFETY: component_id just initialized and corresponds to resource of type R unsafe { self.insert_resource_with_id(component_id, value) }; } /// Removes the resource of a given type and returns it, if it exists. Otherwise returns [None]. #[inline] pub fn remove_resource(&mut self) -> Option { - // SAFE: R is Send + Sync + // SAFETY: R is Send + Sync unsafe { self.remove_resource_unchecked() } } #[inline] pub fn remove_non_send_resource(&mut self) -> Option { self.validate_non_send_access::(); - // SAFE: we are on main thread + // SAFETY: we are on main thread unsafe { self.remove_resource_unchecked() } } @@ -747,10 +747,10 @@ impl World { if column.is_empty() { return None; } - // SAFE: if a resource column exists, row 0 exists as well. caller takes ownership of the + // SAFETY: if a resource column exists, row 0 exists as well. caller takes ownership of the // ptr value / drop is called when R is dropped let (ptr, _) = unsafe { column.swap_remove_and_forget_unchecked(0) }; - // SAFE: column is of type R + // SAFETY: column is of type R Some(unsafe { ptr.read::() }) } @@ -778,7 +778,7 @@ impl World { } else { return false; }; - // SAFE: resources table always have row 0 + // SAFETY: resources table always have row 0 let ticks = unsafe { column.get_ticks_unchecked(0).deref() }; ticks.is_added(self.last_change_tick(), self.read_change_tick()) } @@ -795,7 +795,7 @@ impl World { } else { return false; }; - // SAFE: resources table always have row 0 + // SAFETY: resources table always have row 0 let ticks = unsafe { column.get_ticks_unchecked(0).deref() }; ticks.is_changed(self.last_change_tick(), self.read_change_tick()) } @@ -852,14 +852,14 @@ impl World { #[inline] pub fn get_resource(&self) -> Option<&R> { let component_id = self.components.get_resource_id(TypeId::of::())?; - // SAFE: unique world access + // SAFETY: unique world access unsafe { self.get_resource_with_id(component_id) } } /// Gets a mutable reference to the resource of the given type if it exists #[inline] pub fn get_resource_mut(&mut self) -> Option> { - // SAFE: unique world access + // SAFETY: unique world access unsafe { self.get_resource_unchecked_mut() } } @@ -882,7 +882,7 @@ impl World { /// /// # Safety /// This will allow aliased mutable access to the given resource type. The caller must ensure - /// that only one mutable access exists at a time. + /// that there is either only one mutable access or multiple immutable accesses at a time. #[inline] pub unsafe fn get_resource_unchecked_mut(&self) -> Option> { let component_id = self.components.get_resource_id(TypeId::of::())?; @@ -934,7 +934,7 @@ impl World { #[inline] pub fn get_non_send_resource(&self) -> Option<&R> { let component_id = self.components.get_resource_id(TypeId::of::())?; - // SAFE: component id matches type T + // SAFETY: component id matches type T unsafe { self.get_non_send_with_id(component_id) } } @@ -942,7 +942,7 @@ impl World { /// Otherwise returns [None] #[inline] pub fn get_non_send_resource_mut(&mut self) -> Option> { - // SAFE: unique world access + // SAFETY: unique world access unsafe { self.get_non_send_resource_unchecked_mut() } } @@ -951,7 +951,7 @@ impl World { /// /// # Safety /// This will allow aliased mutable access to the given non-send resource type. The caller must - /// ensure that only one mutable access exists at a time. + /// ensure that there is either only one mutable access or multiple immutable accesses at a time. #[inline] pub unsafe fn get_non_send_resource_unchecked_mut(&self) -> Option> { let component_id = self.components.get_resource_id(TypeId::of::())?; @@ -1033,7 +1033,7 @@ impl World { SpawnOrInsert::Insert(ref mut inserter, archetype) if location.archetype_id == archetype => { - // SAFE: `entity` is valid, `location` matches entity, bundle matches inserter + // SAFETY: `entity` is valid, `location` matches entity, bundle matches inserter unsafe { inserter.insert(entity, location.index, bundle) }; } _ => { @@ -1045,7 +1045,7 @@ impl World { location.archetype_id, change_tick, ); - // SAFE: `entity` is valid, `location` matches entity, bundle matches inserter + // SAFETY: `entity` is valid, `location` matches entity, bundle matches inserter unsafe { inserter.insert(entity, location.index, bundle) }; spawn_or_insert = SpawnOrInsert::Insert(inserter, location.archetype_id); @@ -1054,7 +1054,7 @@ impl World { } AllocAtWithoutReplacement::DidNotExist => { if let SpawnOrInsert::Spawn(ref mut spawner) = spawn_or_insert { - // SAFE: `entity` is allocated (but non existent), bundle matches inserter + // SAFETY: `entity` is allocated (but non existent), bundle matches inserter unsafe { spawner.spawn_non_existent(entity, bundle) }; } else { let mut spawner = bundle_info.get_bundle_spawner( @@ -1064,7 +1064,7 @@ impl World { &mut self.storages, change_tick, ); - // SAFE: `entity` is valid, `location` matches entity, bundle matches inserter + // SAFETY: `entity` is valid, `location` matches entity, bundle matches inserter unsafe { spawner.spawn_non_existent(entity, bundle) }; spawn_or_insert = SpawnOrInsert::Spawn(spawner); } @@ -1123,11 +1123,11 @@ impl World { "resource does not exist: {}", std::any::type_name::() ); - // SAFE: if a resource column exists, row 0 exists as well. caller takes ownership of + // SAFETY: if a resource column exists, row 0 exists as well. caller takes ownership of // the ptr value / drop is called when R is dropped unsafe { column.swap_remove_and_forget_unchecked(0) } }; - // SAFE: pointer is of type R + // SAFETY: pointer is of type R // Read the value onto the stack to avoid potential mut aliasing. let mut value = unsafe { ptr.read::() }; let value_mut = Mut { @@ -1148,8 +1148,8 @@ impl World { .unwrap_or_else(|| panic!("resource does not exist: {}", std::any::type_name::())); OwningPtr::make(value, |ptr| { + // SAFETY: pointer is of type R unsafe { - // SAFE: pointer is of type R column.push(ptr, ticks); } }); @@ -1216,12 +1216,12 @@ impl World { let change_tick = self.change_tick(); let column = self.initialize_resource_internal(component_id); if column.is_empty() { - // SAFE: column is of type R and has been allocated above + // SAFETY: column is of type R and has been allocated above OwningPtr::make(value, |ptr| { column.push(ptr, ComponentTicks::new(change_tick)); }); } else { - // SAFE: column is of type R and has already been allocated + // SAFETY: column is of type R and has already been allocated *column.get_data_unchecked_mut(0).deref_mut::() = value; column.get_ticks_unchecked_mut(0).set_changed(change_tick); } @@ -1246,10 +1246,10 @@ impl World { "insert_resource_by_id called with component id which doesn't exist in this world" ) }); - // SAFE: component_id is valid, checked by the lines above + // SAFETY: component_id is valid, checked by the lines above let column = self.initialize_resource_internal(component_id); if column.is_empty() { - // SAFE: column is of type R and has been allocated above + // SAFETY: column is of type R and has been allocated above column.push(value, ComponentTicks::new(change_tick)); } else { let ptr = column.get_data_unchecked_mut(0); @@ -1266,7 +1266,7 @@ impl World { /// `component_id` must be valid for this world #[inline] unsafe fn initialize_resource_internal(&mut self, component_id: ComponentId) -> &mut Column { - // SAFE: resource archetype always exists + // SAFETY: resource archetype always exists let resource_archetype = self .archetypes .archetypes @@ -1294,14 +1294,14 @@ impl World { pub(crate) fn initialize_resource(&mut self) -> ComponentId { let component_id = self.components.init_resource::(); - // SAFE: resource initialized above + // SAFETY: resource initialized above unsafe { self.initialize_resource_internal(component_id) }; component_id } pub(crate) fn initialize_non_send_resource(&mut self) -> ComponentId { let component_id = self.components.init_non_send::(); - // SAFE: resource initialized above + // SAFETY: resource initialized above unsafe { self.initialize_resource_internal(component_id) }; component_id } @@ -1343,12 +1343,12 @@ impl World { /// such as inserting a [Component]. pub(crate) fn flush(&mut self) { let empty_archetype = self.archetypes.empty_mut(); + let table = &mut self.storages.tables[empty_archetype.table_id()]; + // PERF: consider pre-allocating space for flushed entities + // SAFETY: entity is set to a valid location unsafe { - let table = &mut self.storages.tables[empty_archetype.table_id()]; - // PERF: consider pre-allocating space for flushed entities - // SAFE: entity is set to a valid location self.entities.flush(|entity, location| { - // SAFE: no components are allocated by archetype.allocate() because the archetype + // SAFETY: no components are allocated by archetype.allocate() because the archetype // is empty *location = empty_archetype.allocate(entity, table.allocate(entity)); }); @@ -1428,9 +1428,10 @@ impl World { let column = self.get_populated_resource_column(component_id)?; - // SAFE: get_data_ptr requires that the mutability rules are not violated, and the caller promises + // SAFETY: get_data_ptr requires that the mutability rules are not violated, and the caller promises // to only modify the resource while the mutable borrow of the world is valid let ticks = Ticks { + // SAFETY: // - index is in-bounds because the column is initialized and non-empty // - no other reference to the ticks of the same row can exist at the same time component_ticks: unsafe { &mut *column.get_ticks_unchecked(0).get() }, @@ -1439,6 +1440,7 @@ impl World { }; Some(MutUntyped { + // SAFETY: world access is unique, so no other reference can exist at the same time value: unsafe { column.get_data_ptr().assert_unique() }, ticks, }) @@ -1460,7 +1462,7 @@ impl World { if column.is_empty() { return None; } - // SAFE: if a resource column exists, row 0 exists as well + // SAFETY: if a resource column exists, row 0 exists as well unsafe { column.swap_remove_unchecked(0) }; Some(()) @@ -1474,7 +1476,7 @@ impl World { #[inline] pub fn get_by_id(&self, entity: Entity, component_id: ComponentId) -> Option> { self.components().get_info(component_id)?; - // SAFE: entity_location is valid, component_id is valid as checked by the line above + // SAFETY: entity_location is valid, component_id is valid as checked by the line above unsafe { get_component( self, @@ -1497,7 +1499,7 @@ impl World { component_id: ComponentId, ) -> Option> { self.components().get_info(component_id)?; - // SAFE: entity_location is valid, component_id is valid as checked by the line above + // SAFETY: entity_location is valid, component_id is valid as checked by the line above unsafe { get_mut_by_id( self, @@ -1526,7 +1528,9 @@ impl fmt::Debug for World { // TODO: remove allow on lint - https://github.com/bevyengine/bevy/issues/3666 #[allow(clippy::non_send_fields_in_send_ty)] +// SAFETY: all methods on the world ensure that non-send resources are only accessible on the main thread unsafe impl Send for World {} +// SAFETY: all methods on the world ensure that non-send resources are only accessible on the main thread unsafe impl Sync for World {} /// Creates an instance of the type this trait is implemented for @@ -1711,6 +1715,7 @@ mod tests { .unwrap(); let resource = world.get_resource_by_id(component_id).unwrap(); + // SAFETY: `TestResource` is the correct resource type let resource = unsafe { resource.deref::() }; assert_eq!(resource.0, 42); @@ -1728,11 +1733,13 @@ mod tests { { let mut resource = world.get_resource_mut_by_id(component_id).unwrap(); resource.set_changed(); + // SAFETY: `TestResource` is the correct resource type let resource = unsafe { resource.into_inner().deref_mut::() }; resource.0 = 43; } let resource = world.get_resource_by_id(component_id).unwrap(); + // SAFETY: `TestResource` is the correct resource type let resource = unsafe { resource.deref::() }; assert_eq!(resource.0, 43); @@ -1744,7 +1751,7 @@ mod tests { let mut world = World::new(); - // SAFE: the drop function is valid for the layout and the data will be safe to access from any thread + // SAFETY: the drop function is valid for the layout and the data will be safe to access from any thread let descriptor = unsafe { ComponentDescriptor::new_with_layout( "Custom Test Component".to_string(), @@ -1761,11 +1768,14 @@ mod tests { let component_id = world.init_component_with_descriptor(descriptor); let value: [u8; 8] = [0, 1, 2, 3, 4, 5, 6, 7]; - OwningPtr::make(value, |ptr| unsafe { - // SAFE: value is valid for the component layout - world.insert_resource_by_id(component_id, ptr); + OwningPtr::make(value, |ptr| { + // SAFETY: value is valid for the component layout + unsafe { + world.insert_resource_by_id(component_id, ptr); + } }); + // SAFETY: [u8; 8] is the correct type for the resource let data = unsafe { world .get_resource_by_id(component_id) @@ -1785,9 +1795,11 @@ mod tests { let invalid_component_id = ComponentId::new(usize::MAX); let mut world = World::new(); - OwningPtr::make((), |ptr| unsafe { - // SAFE: ptr must be valid for the component_id `invalid_component_id` which is invalid, but checked by `insert_resource_by_id` - world.insert_resource_by_id(invalid_component_id, ptr); + OwningPtr::make((), |ptr| { + // SAFETY: ptr must be valid for the component_id `invalid_component_id` which is invalid, but checked by `insert_resource_by_id` + unsafe { + world.insert_resource_by_id(invalid_component_id, ptr); + } }); } diff --git a/crates/bevy_ecs/src/world/spawn_batch.rs b/crates/bevy_ecs/src/world/spawn_batch.rs index 002e30a2b3f3a..f5e1bd2792e2b 100644 --- a/crates/bevy_ecs/src/world/spawn_batch.rs +++ b/crates/bevy_ecs/src/world/spawn_batch.rs @@ -67,7 +67,7 @@ where fn next(&mut self) -> Option { let bundle = self.inner.next()?; - // SAFE: bundle matches spawner type + // SAFETY: bundle matches spawner type unsafe { Some(self.spawner.spawn(bundle)) } } diff --git a/crates/bevy_ecs/src/world/world_cell.rs b/crates/bevy_ecs/src/world/world_cell.rs index c40a852a0236f..08571bb6c4e51 100644 --- a/crates/bevy_ecs/src/world/world_cell.rs +++ b/crates/bevy_ecs/src/world/world_cell.rs @@ -186,7 +186,7 @@ impl<'w> WorldCell<'w> { let resource_archetype = self.world.archetypes.resource(); let archetype_component_id = resource_archetype.get_archetype_component_id(component_id)?; Some(WorldBorrow::new( - // SAFE: ComponentId matches TypeId + // SAFETY: ComponentId matches TypeId unsafe { self.world.get_resource_with_id(component_id)? }, archetype_component_id, self.access.clone(), @@ -218,7 +218,7 @@ impl<'w> WorldCell<'w> { let resource_archetype = self.world.archetypes.resource(); let archetype_component_id = resource_archetype.get_archetype_component_id(component_id)?; Some(WorldBorrowMut::new( - // SAFE: ComponentId matches TypeId and access is checked by WorldBorrowMut + // SAFETY: ComponentId matches TypeId and access is checked by WorldBorrowMut unsafe { self.world .get_resource_unchecked_mut_with_id(component_id)? @@ -253,7 +253,7 @@ impl<'w> WorldCell<'w> { let resource_archetype = self.world.archetypes.resource(); let archetype_component_id = resource_archetype.get_archetype_component_id(component_id)?; Some(WorldBorrow::new( - // SAFE: ComponentId matches TypeId + // SAFETY: ComponentId matches TypeId unsafe { self.world.get_non_send_with_id(component_id)? }, archetype_component_id, self.access.clone(), @@ -285,7 +285,7 @@ impl<'w> WorldCell<'w> { let resource_archetype = self.world.archetypes.resource(); let archetype_component_id = resource_archetype.get_archetype_component_id(component_id)?; Some(WorldBorrowMut::new( - // SAFE: ComponentId matches TypeId and access is checked by WorldBorrowMut + // SAFETY: ComponentId matches TypeId and access is checked by WorldBorrowMut unsafe { self.world .get_non_send_unchecked_mut_with_id(component_id)? diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 54d65646cf357..4d5687c66aea2 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -345,7 +345,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { let mut builder = WorldChildBuilder { current_entity: None, parent_entities: vec![entity], - // SAFE: self.update_location() is called below. It is impossible to make EntityMut + // SAFETY: self.update_location() is called below. It is impossible to make EntityMut // function calls on `self` within the scope defined here world: unsafe { self.world_mut() }, }; @@ -359,7 +359,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn push_children(&mut self, children: &[Entity]) -> &mut Self { let parent = self.id(); { - // SAFE: parent entity is not modified and its location is updated manually + // SAFETY: parent entity is not modified and its location is updated manually let world = unsafe { self.world_mut() }; for child in children.iter() { world @@ -381,7 +381,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn insert_children(&mut self, index: usize, children: &[Entity]) -> &mut Self { let parent = self.id(); { - // SAFE: parent entity is not modified and its location is updated manually + // SAFETY: parent entity is not modified and its location is updated manually let world = unsafe { self.world_mut() }; for child in children.iter() { world @@ -403,7 +403,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn remove_children(&mut self, children: &[Entity]) -> &mut Self { let parent = self.id(); - // SAFE: This doesn't change the parent's location + // SAFETY: This doesn't change the parent's location let world = unsafe { self.world_mut() }; for child in children.iter() { let mut child = world.entity_mut(*child); diff --git a/crates/bevy_hierarchy/src/hierarchy.rs b/crates/bevy_hierarchy/src/hierarchy.rs index 4d6753e719089..cb4a60597c2ae 100644 --- a/crates/bevy_hierarchy/src/hierarchy.rs +++ b/crates/bevy_hierarchy/src/hierarchy.rs @@ -114,7 +114,7 @@ impl<'w> DespawnRecursiveExt for EntityMut<'w> { ) .entered(); - // SAFE: EntityMut is consumed so even though the location is no longer + // SAFETY: EntityMut is consumed so even though the location is no longer // valid, it cannot be accessed again with the invalid location. unsafe { despawn_with_children_recursive(self.world_mut(), entity); @@ -131,7 +131,7 @@ impl<'w> DespawnRecursiveExt for EntityMut<'w> { ) .entered(); - // SAFE: The location is updated. + // SAFETY: The location is updated. unsafe { despawn_children(self.world_mut(), entity); self.update_location(); diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index e8173f674c47a..3269221896218 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -161,6 +161,7 @@ impl<'a> OwningPtr<'a> { #[inline] pub fn make) -> R, R>(val: T, f: F) -> R { let mut temp = MaybeUninit::new(val); + // SAFETY: `temp.as_mut_ptr()` is a reference to a local value on the stack, so it cannot be null let ptr = unsafe { NonNull::new_unchecked(temp.as_mut_ptr().cast::()) }; f(Self(ptr, PhantomData)) } @@ -233,6 +234,7 @@ impl<'a, T> From<&'a [T]> for ThinSlicePtr<'a, T> { #[inline] fn from(slice: &'a [T]) -> Self { Self { + // SAFETY: a reference can never be null ptr: unsafe { NonNull::new_unchecked(slice.as_ptr() as *mut T) }, #[cfg(debug_assertions)] len: slice.len(), diff --git a/crates/bevy_time/src/fixed_timestep.rs b/crates/bevy_time/src/fixed_timestep.rs index d79f4e6ccf0f3..e37ed7de1be72 100644 --- a/crates/bevy_time/src/fixed_timestep.rs +++ b/crates/bevy_time/src/fixed_timestep.rs @@ -190,7 +190,7 @@ impl System for FixedTimestep { } unsafe fn run_unsafe(&mut self, _input: (), world: &World) -> ShouldRun { - // SAFE: this system inherits the internal system's component access and archetype component + // SAFETY: this system inherits the internal system's component access and archetype component // access, which means the caller has ensured running the internal system is safe self.internal_system.run_unsafe((), world) } diff --git a/crates/bevy_ui/src/flex/mod.rs b/crates/bevy_ui/src/flex/mod.rs index 8bc4a2fe10571..149ecae13cc2c 100644 --- a/crates/bevy_ui/src/flex/mod.rs +++ b/crates/bevy_ui/src/flex/mod.rs @@ -22,7 +22,7 @@ pub struct FlexSurface { taffy: Taffy, } -// SAFE: as long as MeasureFunc is Send + Sync. https://github.com/DioxusLabs/taffy/issues/146 +// SAFETY: as long as MeasureFunc is Send + Sync. https://github.com/DioxusLabs/taffy/issues/146 // TODO: remove allow on lint - https://github.com/bevyengine/bevy/issues/3666 #[allow(clippy::non_send_fields_in_send_ty)] unsafe impl Send for FlexSurface {} diff --git a/crates/bevy_utils/src/futures.rs b/crates/bevy_utils/src/futures.rs index dc0f68d5d1c68..1b752a33c70ff 100644 --- a/crates/bevy_utils/src/futures.rs +++ b/crates/bevy_utils/src/futures.rs @@ -29,5 +29,7 @@ fn noop_raw_waker() -> RawWaker { } fn noop_waker() -> Waker { + // SAFETY: the `RawWakerVTable` is just a big noop and doesn't violate any of the rules in `RawWakerVTable`s documentation + // (which talks about retaining and releasing any "resources", of which there are none in this case) unsafe { Waker::from_raw(noop_raw_waker()) } } diff --git a/crates/bevy_window/src/raw_window_handle.rs b/crates/bevy_window/src/raw_window_handle.rs index 5bc122558d78f..967b3c3301636 100644 --- a/crates/bevy_window/src/raw_window_handle.rs +++ b/crates/bevy_window/src/raw_window_handle.rs @@ -24,7 +24,7 @@ impl RawWindowHandleWrapper { } } -// SAFE: RawWindowHandle is just a normal "raw pointer", which doesn't impl Send/Sync. However the pointer is only +// SAFETY: RawWindowHandle is just a normal "raw pointer", which doesn't impl Send/Sync. However the pointer is only // exposed via an unsafe method that forces the user to make a call for a given platform. (ex: some platforms don't // support doing window operations off of the main thread). // A recommendation for this pattern (and more context) is available here: @@ -41,7 +41,7 @@ unsafe impl Sync for RawWindowHandleWrapper {} /// In many cases, this should only be constructed on the main thread. pub struct ThreadLockedRawWindowHandleWrapper(RawWindowHandle); -// SAFE: the caller has validated that this is a valid context to get RawWindowHandle +// SAFETY: the caller has validated that this is a valid context to get RawWindowHandle // as otherwise an instance of this type could not have been constructed // NOTE: we cannot simply impl HasRawWindowHandle for RawWindowHandleWrapper, // as the `raw_window_handle` method is safe. We cannot guarantee that all calls