Enable clippy::check-private-items so that missing_safety_doc will apply to private functions as well (bevyengine#15161)

Enabled `check-private-items` in `clippy.toml` and then fixed the
resulting errors. Most of these were simply misformatted and of the
remaining:
- ~Added `#[allow(clippy::missing_safety_doc)]` to~ Removed unsafe from
a pair of functions in `bevy_utils/futures` which are only unsafe so
that they can be passed to a function which requires `unsafe fn`
- Removed `unsafe` from `UnsafeWorldCell::observers` as from what I can
tell it is always safe like `components`, `bundles` etc. (this should be
checked)
- Added safety docs to:
- `Bundles::get_storage_unchecked`: Based on the function that writes to
`dynamic_component_storages`
- `Bundles::get_storages_unchecked`: Based on the function that writes
to `dynamic_bundle_storages`
   - `QueryIterationCursor::init_empty`: Duplicated from `init`
- `QueryIterationCursor::peek_last`: Thanks Giooschi (also added
internal unsafe blocks)
   - `tests::drop_ptr`: Moved safety comment out to the doc string
 
This lint would also apply to `missing_errors_doc`, `missing_panics_doc`
and `unnecessary_safety_doc` if we chose to enable any of those at some
point, although there is an open
[issue](rust-lang/rust-clippy#13074) to
separate these options.

Author: 13ros27

clippy.toml

@@ -11,6 +11,8 @@ doc-valid-idents = [
   "..",
 ]
 
+check-private-items = true
+
 disallowed-methods = [
   { path = "f32::powi", reason = "use bevy_math::ops::FloatPow::squared, bevy_math::ops::FloatPow::cubed, or bevy_math::ops::powf instead for libm determinism" },
   { path = "f32::log", reason = "use bevy_math::ops::ln, bevy_math::ops::log2, or bevy_math::ops::log10 instead for libm determinism" },

crates/bevy_ecs/src/bundle.rs

@@ -1279,7 +1279,7 @@ impl<'w> BundleSpawner<'w> {
         unsafe { &mut self.world.world_mut().entities }
     }
 
-    /// # Safety:
+    /// # Safety
     /// - `Self` must be dropped after running this function as it may invalidate internal pointers.
     #[inline]
     pub(crate) unsafe fn flush_commands(&mut self) {
@@ -1344,18 +1344,22 @@ impl Bundles {
     }
 
     /// # Safety
-    /// A `BundleInfo` with the given `BundleId` must have been initialized for this instance of `Bundles`.
+    /// A [`BundleInfo`] with the given [`BundleId`] must have been initialized for this instance of `Bundles`.
     pub(crate) unsafe fn get_unchecked(&self, id: BundleId) -> &BundleInfo {
         self.bundle_infos.get_unchecked(id.0)
     }
 
+    /// # Safety
+    /// This [`BundleId`] must have been initialized with a single [`Component`] (via [`init_component_info`](Self::init_dynamic_info))
     pub(crate) unsafe fn get_storage_unchecked(&self, id: BundleId) -> StorageType {
         *self
             .dynamic_component_storages
             .get(&id)
             .debug_checked_unwrap()
     }
 
+    /// # Safety
+    /// This [`BundleId`] must have been initialized with multiple [`Component`]s (via [`init_dynamic_info`](Self::init_dynamic_info))
     pub(crate) unsafe fn get_storages_unchecked(&mut self, id: BundleId) -> &mut Vec<StorageType> {
         self.dynamic_bundle_storages
             .get_mut(&id)

crates/bevy_ecs/src/component.rs

@@ -738,7 +738,7 @@ impl Debug for ComponentDescriptor {
 }
 
 impl ComponentDescriptor {
-    /// # SAFETY
+    /// # Safety
     ///
     /// `x` must point to a valid value of type `T`.
     unsafe fn drop_ptr<T>(x: OwningPtr<'_>) {

crates/bevy_ecs/src/query/iter.rs

@@ -1290,7 +1290,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter, I: Iterator<Item: Borrow<Entity>>>
         }
     }
 
-    /// Safety:
+    /// # Safety
     /// All arguments must stem from the same valid `QueryManyIter`.
     ///
     /// The lifetime here is not restrictive enough for Fetch with &mut access,
@@ -1578,7 +1578,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter, const K: usize> QueryCombinationIter<
         }
     }
 
-    /// Safety:
+    /// # Safety
     /// The lifetime here is not restrictive enough for Fetch with &mut access,
     /// as calling `fetch_next_aliased_unchecked` multiple times can produce multiple
     /// references to the same component, leading to unique reference aliasing.
@@ -1733,6 +1733,9 @@ impl<D: QueryData, F: QueryFilter> Clone for QueryIterationCursor<'_, '_, D, F>
 }
 
 impl<'w, 's, D: QueryData, F: QueryFilter> QueryIterationCursor<'w, 's, D, F> {
+    /// # Safety
+    /// - `world` must have permission to access any of the components registered in `query_state`.
+    /// - `world` must be the same one used to initialize `query_state`.
     unsafe fn init_empty(
         world: UnsafeWorldCell<'w>,
         query_state: &'s QueryState<D, F>,
@@ -1781,25 +1784,41 @@ impl<'w, 's, D: QueryData, F: QueryFilter> QueryIterationCursor<'w, 's, D, F> {
         }
     }
 
-    /// retrieve item returned from most recent `next` call again.
+    /// Retrieve item returned from most recent `next` call again.
+    ///
+    /// # Safety
+    /// The result of `next` and any previous calls to `peek_last` with this row must have been
+    /// dropped to prevent aliasing mutable references.
     #[inline]
     unsafe fn peek_last(&mut self) -> Option<D::Item<'w>> {
         if self.current_row > 0 {
             let index = self.current_row - 1;
             if self.is_dense {
-                let entity = self.table_entities.get_unchecked(index);
-                Some(D::fetch(
-                    &mut self.fetch,
-                    *entity,
-                    TableRow::from_usize(index),
-                ))
+                // SAFETY: This must have been called previously in `next` as `current_row > 0`
+                let entity = unsafe { self.table_entities.get_unchecked(index) };
+                // SAFETY:
+                //  - `set_table` must have been called previously either in `next` or before it.
+                //  - `*entity` and `index` are in the current table.
+                unsafe {
+                    Some(D::fetch(
+                        &mut self.fetch,
+                        *entity,
+                        TableRow::from_usize(index),
+                    ))
+                }
             } else {
-                let archetype_entity = self.archetype_entities.get_unchecked(index);
-                Some(D::fetch(
-                    &mut self.fetch,
-                    archetype_entity.id(),
-                    archetype_entity.table_row(),
-                ))
+                // SAFETY: This must have been called previously in `next` as `current_row > 0`
+                let archetype_entity = unsafe { self.archetype_entities.get_unchecked(index) };
+                // SAFETY:
+                //  - `set_archetype` must have been called previously either in `next` or before it.
+                //  - `archetype_entity.id()` and `archetype_entity.table_row()` are in the current archetype.
+                unsafe {
+                    Some(D::fetch(
+                        &mut self.fetch,
+                        archetype_entity.id(),
+                        archetype_entity.table_row(),
+                    ))
+                }
             }
         } else {
             None

crates/bevy_ecs/src/query/state.rs

@@ -123,7 +123,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
     ///
     /// Consider using `as_readonly` or `as_nop` instead which are safe functions.
     ///
-    /// # SAFETY
+    /// # Safety
     ///
     /// `NewD` must have a subset of the access that `D` does and match the exact same archetypes/tables
     /// `NewF` must have a subset of the access that `F` does and match the exact same archetypes/tables

crates/bevy_ecs/src/storage/blob_vec.rs

@@ -507,8 +507,12 @@ mod tests {
     use super::BlobVec;
     use std::{alloc::Layout, cell::RefCell, mem::align_of, rc::Rc};
 
+    /// # Safety
+    ///
+    /// The pointer `x` must point to a valid value of type `T` and it must be safe to drop this value.
     unsafe fn drop_ptr<T>(x: OwningPtr<'_>) {
-        // SAFETY: The pointer points to a valid value of type `T` and it is safe to drop this value.
+        // SAFETY: It is guaranteed by the caller that `x` points to a
+        //         valid value of type `T` and it is safe to drop this value.
         unsafe {
             x.drop_as::<T>();
         }

crates/bevy_ecs/src/system/system_param.rs

@@ -1923,7 +1923,7 @@ pub struct DynSystemParam<'w, 's> {
 }
 
 impl<'w, 's> DynSystemParam<'w, 's> {
-    /// # SAFETY
+    /// # Safety
     /// - `state` must be a `ParamState<T>` for some inner `T: SystemParam`.
     /// - The passed [`UnsafeWorldCell`] must have access to any world data registered
     ///   in [`init_state`](SystemParam::init_state) for the inner system param.
@@ -1998,7 +1998,7 @@ impl<'w, 's> DynSystemParam<'w, 's> {
     }
 }
 
-/// # SAFETY
+/// # Safety
 /// - `state` must be a `ParamState<T>` for some inner `T: SystemParam`.
 /// - The passed [`UnsafeWorldCell`] must have access to any world data registered
 ///   in [`init_state`](SystemParam::init_state) for the inner system param.

crates/bevy_ecs/src/world/entity_ref.rs

@@ -1130,7 +1130,7 @@ impl<'w> EntityWorldMut<'w> {
 
     /// Remove the components of `bundle` from `entity`.
     ///
-    /// SAFETY:
+    /// # Safety
     /// - A `BundleInfo` with the corresponding `BundleId` must have been initialized.
     #[allow(clippy::too_many_arguments)]
     unsafe fn remove_bundle(&mut self, bundle: BundleId) -> EntityLocation {
@@ -1519,7 +1519,8 @@ impl<'w> EntityWorldMut<'w> {
     }
 }
 
-/// SAFETY: all components in the archetype must exist in world
+/// # Safety
+/// All components in the archetype must exist in world
 unsafe fn trigger_on_replace_and_on_remove_hooks_and_observers(
     deferred_world: &mut DeferredWorld,
     archetype: &Archetype,
@@ -2387,6 +2388,8 @@ impl<'w, B> EntityRefExcept<'w, B>
 where
     B: Bundle,
 {
+    /// # Safety
+    /// Other users of `UnsafeEntityCell` must only have mutable access to the components in `B`.
     pub(crate) unsafe fn new(entity: UnsafeEntityCell<'w>) -> Self {
         Self {
             entity,
@@ -2466,6 +2469,8 @@ impl<'w, B> EntityMutExcept<'w, B>
 where
     B: Bundle,
 {
+    /// # Safety
+    /// Other users of `UnsafeEntityCell` must not have access to any components not in `B`.
     pub(crate) unsafe fn new(entity: UnsafeEntityCell<'w>) -> Self {
         Self {
             entity,

crates/bevy_ecs/src/world/unsafe_world_cell.rs

@@ -248,7 +248,7 @@ impl<'w> UnsafeWorldCell<'w> {
     }
 
     /// Retrieves this world's [`Observers`] collection.
-    pub(crate) unsafe fn observers(self) -> &'w Observers {
+    pub(crate) fn observers(self) -> &'w Observers {
         // SAFETY:
         // - we only access world metadata
         &unsafe { self.world_metadata() }.observers
@@ -1002,7 +1002,7 @@ impl<'w> UnsafeEntityCell<'w> {
 
 impl<'w> UnsafeWorldCell<'w> {
     #[inline]
-    /// # Safety:
+    /// # Safety
     /// - the returned `Table` is only used in ways that this [`UnsafeWorldCell`] has permission for.
     /// - the returned `Table` is only used in ways that would not conflict with any existing borrows of world data.
     unsafe fn fetch_table(self, location: EntityLocation) -> Option<&'w Table> {
@@ -1013,7 +1013,7 @@ impl<'w> UnsafeWorldCell<'w> {
     }
 
     #[inline]
-    /// # Safety:
+    /// # Safety
     /// - the returned `ComponentSparseSet` is only used in ways that this [`UnsafeWorldCell`] has permission for.
     /// - the returned `ComponentSparseSet` is only used in ways that would not conflict with any existing
     ///   borrows of world data.

crates/bevy_render/src/lib.rs

@@ -447,7 +447,8 @@ fn extract(main_world: &mut World, render_world: &mut World) {
     main_world.insert_resource(ScratchMainWorld(scratch_world));
 }
 
-/// SAFETY: this function must be called from the main thread.
+/// # Safety
+/// This function must be called from the main thread.
 unsafe fn initialize_render_app(app: &mut App) {
     app.init_resource::<ScratchMainWorld>();
 

crates/bevy_utils/src/futures.rs

@@ -36,10 +36,10 @@ pub fn check_ready<F: Future + Unpin>(future: &mut F) -> Option<F::Output> {
     }
 }
 
-unsafe fn noop_clone(_data: *const ()) -> RawWaker {
+fn noop_clone(_data: *const ()) -> RawWaker {
     noop_raw_waker()
 }
-unsafe fn noop(_data: *const ()) {}
+fn noop(_data: *const ()) {}
 
 const NOOP_WAKER_VTABLE: RawWakerVTable = RawWakerVTable::new(noop_clone, noop, noop, noop);