Enable clippy::undocumented_unsafe_blocks warning across the workspace (#10646)

# Objective

Enables warning on `clippy::undocumented_unsafe_blocks` across the
workspace rather than only in `bevy_ecs`, `bevy_transform` and
`bevy_utils`. This adds a little awkwardness in a few areas of code that
have trivial safety or explain safety for multiple unsafe blocks with
one comment however automatically prevents these comments from being
missed.

## Solution

This adds `undocumented_unsafe_blocks = "warn"` to the workspace
`Cargo.toml` and fixes / adds a few missed safety comments. I also added
`#[allow(clippy::undocumented_unsafe_blocks)]` where the safety is
explained somewhere above.

There are a couple of safety comments I added I'm not 100% sure about in
`bevy_animation` and `bevy_render/src/view` and I'm not sure about the
use of `#[allow(clippy::undocumented_unsafe_blocks)]` compared to adding
comments like `// SAFETY: See above`.
This commit is contained in:
TheBigCheese 2023-11-21 02:06:24 +00:00 committed by GitHub
parent 897b13bf9d
commit e67cfdf82b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
13 changed files with 28 additions and 16 deletions

View file

@ -32,6 +32,7 @@ members = [
[workspace.lints.clippy] [workspace.lints.clippy]
type_complexity = "allow" type_complexity = "allow"
doc_markdown = "warn" doc_markdown = "warn"
undocumented_unsafe_blocks = "warn"
[lints] [lints]
workspace = true workspace = true

View file

@ -646,6 +646,7 @@ fn apply_animation(
let Ok(mut transform) = (unsafe { transforms.get_unchecked(target) }) else { let Ok(mut transform) = (unsafe { transforms.get_unchecked(target) }) else {
continue; continue;
}; };
// SAFETY: As above, there can't be other AnimationPlayers with this target so this fetch can't alias
let mut morphs = unsafe { morphs.get_unchecked(target) }; let mut morphs = unsafe { morphs.get_unchecked(target) };
for curve in curves { for curve in curves {
// Some curves have only one keyframe used to set a transform // Some curves have only one keyframe used to set a transform

View file

@ -1,4 +1,3 @@
#![warn(clippy::undocumented_unsafe_blocks)]
#![warn(missing_docs)] #![warn(missing_docs)]
#![doc = include_str!("../README.md")] #![doc = include_str!("../README.md")]

View file

@ -1,4 +1,4 @@
#![allow(clippy::all)] #![allow(clippy::all, clippy::undocumented_unsafe_blocks)]
use glam::{Vec2, Vec3}; use glam::{Vec2, Vec3};

View file

@ -244,14 +244,14 @@ impl<'a, A: IsAligned> PtrMut<'a, A> {
/// Gets a [`PtrMut`] from this with a smaller lifetime. /// Gets a [`PtrMut`] from this with a smaller lifetime.
#[inline] #[inline]
pub fn reborrow(&mut self) -> PtrMut<'_, A> { pub fn reborrow(&mut self) -> PtrMut<'_, A> {
// SAFE: the ptrmut we're borrowing from is assumed to be valid // SAFETY: the ptrmut we're borrowing from is assumed to be valid
unsafe { PtrMut::new(self.0) } unsafe { PtrMut::new(self.0) }
} }
/// Gets an immutable reference from this mutable reference /// Gets an immutable reference from this mutable reference
#[inline] #[inline]
pub fn as_ref(&self) -> Ptr<'_, A> { pub fn as_ref(&self) -> Ptr<'_, A> {
// SAFE: The `PtrMut` type's guarantees about the validity of this pointer are a superset of `Ptr` s guarantees // SAFETY: The `PtrMut` type's guarantees about the validity of this pointer are a superset of `Ptr` s guarantees
unsafe { Ptr::new(self.0) } unsafe { Ptr::new(self.0) }
} }
} }
@ -327,14 +327,14 @@ impl<'a, A: IsAligned> OwningPtr<'a, A> {
/// Gets an immutable pointer from this owned pointer. /// Gets an immutable pointer from this owned pointer.
#[inline] #[inline]
pub fn as_ref(&self) -> Ptr<'_, A> { pub fn as_ref(&self) -> Ptr<'_, A> {
// SAFE: The `Owning` type's guarantees about the validity of this pointer are a superset of `Ptr` s guarantees // SAFETY: The `Owning` type's guarantees about the validity of this pointer are a superset of `Ptr` s guarantees
unsafe { Ptr::new(self.0) } unsafe { Ptr::new(self.0) }
} }
/// Gets a mutable pointer from this owned pointer. /// Gets a mutable pointer from this owned pointer.
#[inline] #[inline]
pub fn as_mut(&mut self) -> PtrMut<'_, A> { pub fn as_mut(&mut self) -> PtrMut<'_, A> {
// SAFE: The `Owning` type's guarantees about the validity of this pointer are a superset of `Ptr` s guarantees // SAFETY: The `Owning` type's guarantees about the validity of this pointer are a superset of `Ptr` s guarantees
unsafe { PtrMut::new(self.0) } unsafe { PtrMut::new(self.0) }
} }
} }

View file

@ -251,6 +251,7 @@ impl Plugin for RenderPlugin {
app.insert_resource(FutureRendererResources( app.insert_resource(FutureRendererResources(
future_renderer_resources_wrapper.clone(), future_renderer_resources_wrapper.clone(),
)); ));
// SAFETY: Plugins should be set up on the main thread.
unsafe { initialize_render_app(app) }; unsafe { initialize_render_app(app) };
} }
RenderCreation::Automatic(render_creation) => { RenderCreation::Automatic(render_creation) => {
@ -271,8 +272,8 @@ impl Plugin for RenderPlugin {
backends, backends,
dx12_shader_compiler: settings.dx12_shader_compiler.clone(), dx12_shader_compiler: settings.dx12_shader_compiler.clone(),
}); });
// SAFETY: Plugins should be set up on the main thread.
let surface = primary_window.map(|wrapper| unsafe { let surface = primary_window.map(|wrapper| unsafe {
// SAFETY: Plugins should be set up on the main thread.
let handle = wrapper.get_handle(); let handle = wrapper.get_handle();
instance instance
.create_surface(&handle) .create_surface(&handle)
@ -313,6 +314,7 @@ impl Plugin for RenderPlugin {
#[cfg(not(target_arch = "wasm32"))] #[cfg(not(target_arch = "wasm32"))]
futures_lite::future::block_on(async_renderer); futures_lite::future::block_on(async_renderer);
// SAFETY: Plugins should be set up on the main thread.
unsafe { initialize_render_app(app) }; unsafe { initialize_render_app(app) };
} }
} }
@ -453,7 +455,7 @@ unsafe fn initialize_render_app(app: &mut App) {
"An entity was spawned after the entity list was cleared last frame and before the extract schedule began. This is not supported", "An entity was spawned after the entity list was cleared last frame and before the extract schedule began. This is not supported",
); );
// This is safe given the clear_entities call in the past frame and the assert above // SAFETY: This is safe given the clear_entities call in the past frame and the assert above
unsafe { unsafe {
render_app render_app
.world .world

View file

@ -58,6 +58,7 @@ macro_rules! render_resource_wrapper {
// If in future there is a case where a wrapper is required for a non-send/sync type // If in future there is a case where a wrapper is required for a non-send/sync type
// we can implement a macro variant that omits these manual Send + Sync impls // we can implement a macro variant that omits these manual Send + Sync impls
unsafe impl Send for $wrapper_type {} unsafe impl Send for $wrapper_type {}
// SAFETY: As explained above, we ensure correctness by checking that $wgpu_type implements Send and Sync.
unsafe impl Sync for $wrapper_type {} unsafe impl Sync for $wrapper_type {}
const _: () = { const _: () = {
trait AssertSendSyncBound: Send + Sync {} trait AssertSendSyncBound: Send + Sync {}

View file

@ -252,12 +252,15 @@ pub fn prepare_windows(
let surface_data = window_surfaces let surface_data = window_surfaces
.surfaces .surfaces
.entry(window.entity) .entry(window.entity)
.or_insert_with(|| unsafe { .or_insert_with(|| {
// NOTE: On some OSes this MUST be called from the main thread. // SAFETY: The window handles in ExtractedWindows will always be valid objects to create surfaces on
// As of wgpu 0.15, only fallible if the given window is a HTML canvas and obtaining a WebGPU or WebGL2 context fails. let surface = unsafe {
let surface = render_instance // NOTE: On some OSes this MUST be called from the main thread.
.create_surface(&window.handle.get_handle()) // As of wgpu 0.15, only fallible if the given window is a HTML canvas and obtaining a WebGPU or WebGL2 context fails.
.expect("Failed to create wgpu surface"); render_instance
.create_surface(&window.handle.get_handle())
.expect("Failed to create wgpu surface")
};
let caps = surface.get_capabilities(&render_adapter); let caps = surface.get_capabilities(&render_adapter);
let formats = caps.formats; let formats = caps.formats;
// For future HDR output support, we'll need to request a format that supports HDR, // For future HDR output support, we'll need to request a format that supports HDR,

View file

@ -353,12 +353,16 @@ impl TaskPool {
// Any usages of the references passed into `Scope` must be accessed through // Any usages of the references passed into `Scope` must be accessed through
// the transmuted reference for the rest of this function. // the transmuted reference for the rest of this function.
let executor: &async_executor::Executor = &self.executor; let executor: &async_executor::Executor = &self.executor;
// SAFETY: As above, all futures must complete in this function so we can change the lifetime
let executor: &'env async_executor::Executor = unsafe { mem::transmute(executor) }; let executor: &'env async_executor::Executor = unsafe { mem::transmute(executor) };
// SAFETY: As above, all futures must complete in this function so we can change the lifetime
let external_executor: &'env ThreadExecutor<'env> = let external_executor: &'env ThreadExecutor<'env> =
unsafe { mem::transmute(external_executor) }; unsafe { mem::transmute(external_executor) };
// SAFETY: As above, all futures must complete in this function so we can change the lifetime
let scope_executor: &'env ThreadExecutor<'env> = unsafe { mem::transmute(scope_executor) }; let scope_executor: &'env ThreadExecutor<'env> = unsafe { mem::transmute(scope_executor) };
let spawned: ConcurrentQueue<FallibleTask<T>> = ConcurrentQueue::unbounded(); let spawned: ConcurrentQueue<FallibleTask<T>> = ConcurrentQueue::unbounded();
// shadow the variable so that the owned value cannot be used for the rest of the function // shadow the variable so that the owned value cannot be used for the rest of the function
// SAFETY: As above, all futures must complete in this function so we can change the lifetime
let spawned: &'env ConcurrentQueue< let spawned: &'env ConcurrentQueue<
FallibleTask<Result<T, Box<(dyn std::any::Any + Send)>>>, FallibleTask<Result<T, Box<(dyn std::any::Any + Send)>>>,
> = unsafe { mem::transmute(&spawned) }; > = unsafe { mem::transmute(&spawned) };
@ -373,6 +377,7 @@ impl TaskPool {
}; };
// shadow the variable so that the owned value cannot be used for the rest of the function // shadow the variable so that the owned value cannot be used for the rest of the function
// SAFETY: As above, all futures must complete in this function so we can change the lifetime
let scope: &'env Scope<'_, 'env, T> = unsafe { mem::transmute(&scope) }; let scope: &'env Scope<'_, 'env, T> = unsafe { mem::transmute(&scope) };
f(scope); f(scope);

View file

@ -1,5 +1,4 @@
#![warn(missing_docs)] #![warn(missing_docs)]
#![warn(clippy::undocumented_unsafe_blocks)]
#![doc = include_str!("../README.md")] #![doc = include_str!("../README.md")]
pub mod commands; pub mod commands;

View file

@ -1309,6 +1309,7 @@ pub struct GridPlacement {
impl GridPlacement { impl GridPlacement {
pub const DEFAULT: Self = Self { pub const DEFAULT: Self = Self {
start: None, start: None,
// SAFETY: This is trivially safe as 1 is non-zero.
span: Some(unsafe { NonZeroU16::new_unchecked(1) }), span: Some(unsafe { NonZeroU16::new_unchecked(1) }),
end: None, end: None,
}; };

View file

@ -4,7 +4,6 @@
//! //!
#![warn(missing_docs)] #![warn(missing_docs)]
#![warn(clippy::undocumented_unsafe_blocks)]
#[allow(missing_docs)] #[allow(missing_docs)]
pub mod prelude { pub mod prelude {

View file

@ -34,6 +34,7 @@ impl RawHandleWrapper {
// A recommendation for this pattern (and more context) is available here: // A recommendation for this pattern (and more context) is available here:
// https://github.com/rust-windowing/raw-window-handle/issues/59 // https://github.com/rust-windowing/raw-window-handle/issues/59
unsafe impl Send for RawHandleWrapper {} unsafe impl Send for RawHandleWrapper {}
// SAFETY: This is safe for the same reasons as the Send impl above.
unsafe impl Sync for RawHandleWrapper {} unsafe impl Sync for RawHandleWrapper {}
/// A [`RawHandleWrapper`] that cannot be sent across threads. /// A [`RawHandleWrapper`] that cannot be sent across threads.