Various cleanups (bevyengine#2046)

This includes a few safety improvements and a variety of other cleanups. See the individual commits.

crates/bevy_asset/src/handle.rs

@@ -68,7 +68,8 @@ where
     #[reflect(ignore)]
     handle_type: HandleType,
     #[reflect(ignore)]
-    marker: PhantomData<T>,
+    // NOTE: PhantomData<fn() -> T> gives this safe Send/Sync impls
+    marker: PhantomData<fn() -> T>,
 }
 
 enum HandleType {
@@ -229,10 +230,6 @@ impl<T: Asset> Clone for Handle<T> {
     }
 }
 
-// SAFE: T is phantom data and Handle::id is an integer
-unsafe impl<T: Asset> Send for Handle<T> {}
-unsafe impl<T: Asset> Sync for Handle<T> {}
-
 /// A non-generic version of [Handle]
 ///
 /// This allows handles to be mingled in a cross asset context. For example, storing `Handle<A>` and

crates/bevy_core/src/bytes.rs

@@ -12,11 +12,7 @@ pub trait Bytes {
 }
 
 /// A trait that indicates that it is safe to cast the type to a byte array reference.
-pub unsafe trait Byteable
-where
-    Self: Sized,
-{
-}
+pub unsafe trait Byteable: Copy + Sized {}
 
 impl<T> Bytes for T
 where
@@ -46,7 +42,7 @@ pub trait FromBytes {
 
 impl<T> FromBytes for T
 where
-    T: Byteable + Copy,
+    T: Byteable,
 {
     fn from_bytes(bytes: &[u8]) -> Self {
         assert_eq!(
@@ -79,13 +75,6 @@ where
     }
 }
 
-unsafe impl<T> Byteable for [T]
-where
-    Self: Sized,
-    T: Byteable,
-{
-}
-
 unsafe impl<T, const N: usize> Byteable for [T; N] where T: Byteable {}
 
 unsafe impl Byteable for u8 {}
@@ -154,7 +143,7 @@ where
 
 impl<T> Bytes for Vec<T>
 where
-    T: Sized + Byteable,
+    T: Byteable,
 {
     fn write_bytes(&self, buffer: &mut [u8]) {
         let bytes = self.as_slice().as_bytes();
@@ -168,7 +157,7 @@ where
 
 impl<T> FromBytes for Vec<T>
 where
-    T: Sized + Copy + Byteable,
+    T: Byteable,
 {
     fn from_bytes(bytes: &[u8]) -> Self {
         assert_eq!(

crates/bevy_core/src/time/fixed_timestep.rs

@@ -168,7 +168,7 @@ impl System for FixedTimestep {
         self.internal_system.is_send()
     }
 
-    unsafe fn run_unsafe(&mut self, _input: Self::In, world: &World) -> Self::Out {
+    unsafe fn run_unsafe(&mut self, _input: (), world: &World) -> ShouldRun {
         // SAFE: 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)

crates/bevy_derive/src/app_plugin.rs

@@ -8,9 +8,7 @@ pub fn derive_dynamic_plugin(input: TokenStream) -> TokenStream {
 
     TokenStream::from(quote! {
         #[no_mangle]
-        pub extern "C" fn _create_plugin() -> *mut bevy::app::Plugin {
-            // TODO: without this the assembly does nothing. why is that the case?
-            print!("");
+        pub extern "C" fn _bevy_create_plugin() -> *mut bevy::app::Plugin {
             // make sure the constructor is the correct type.
             let object = #struct_name {};
             let boxed = Box::new(object);

crates/bevy_dynamic_plugin/src/loader.rs

@@ -2,23 +2,32 @@ use libloading::{Library, Symbol};
 
 use bevy_app::{AppBuilder, CreatePlugin, Plugin};
 
-/// Dynamically links a plugin a the given path. The plugin must export the [CreatePlugin] function.
-pub fn dynamically_load_plugin(path: &str) -> (Library, Box<dyn Plugin>) {
-    unsafe {
-        let lib = Library::new(path).unwrap();
-        let func: Symbol<CreatePlugin> = lib.get(b"_create_plugin").unwrap();
-        let plugin = Box::from_raw(func());
-        (lib, plugin)
-    }
+/// Dynamically links a plugin a the given path. The plugin must export a function with the
+/// [`CreatePlugin`] signature named `_bevy_create_plugin`.
+///
+/// # Safety
+///
+/// The specified plugin must be linked against the exact same libbevy.so as this program.
+/// In addition the `_bevy_create_plugin` symbol must not be manually created, but instead created
+/// by deriving `DynamicPlugin` on a unit struct implementing [`Plugin`].
+pub unsafe fn dynamically_load_plugin(path: &str) -> (Library, Box<dyn Plugin>) {
+    let lib = Library::new(path).unwrap();
+    let func: Symbol<CreatePlugin> = lib.get(b"_bevy_create_plugin").unwrap();
+    let plugin = Box::from_raw(func());
+    (lib, plugin)
 }
 
 pub trait DynamicPluginExt {
-    fn load_plugin(&mut self, path: &str) -> &mut Self;
+    /// # Safety
+    ///
+    /// Same as [`dynamically_load_plugin`].
+    unsafe fn load_plugin(&mut self, path: &str) -> &mut Self;
 }
 
 impl DynamicPluginExt for AppBuilder {
-    fn load_plugin(&mut self, path: &str) -> &mut Self {
-        let (_lib, plugin) = dynamically_load_plugin(path);
+    unsafe fn load_plugin(&mut self, path: &str) -> &mut Self {
+        let (lib, plugin) = dynamically_load_plugin(path);
+        std::mem::forget(lib); // Ensure that the library is not automatically unloaded
         plugin.build(self);
         self
     }

crates/bevy_ecs/src/schedule/run_criteria.rs

@@ -404,7 +404,7 @@ impl System for RunOnce {
         true
     }
 
-    unsafe fn run_unsafe(&mut self, _input: Self::In, _world: &World) -> Self::Out {
+    unsafe fn run_unsafe(&mut self, (): (), _world: &World) -> ShouldRun {
         if self.ran {
             ShouldRun::No
         } else {

crates/bevy_render/src/render_graph/graph.rs

@@ -355,7 +355,7 @@ mod tests {
     }
 
     #[test]
-    pub fn test_graph_edges() {
+    fn test_graph_edges() {
         let mut graph = RenderGraph::default();
         let a_id = graph.add_node("A", TestNode::new(0, 1));
         let b_id = graph.add_node("B", TestNode::new(0, 1));
@@ -411,7 +411,7 @@ mod tests {
     }
 
     #[test]
-    pub fn test_get_node_typed() {
+    fn test_get_node_typed() {
         struct MyNode {
             value: usize,
         }
@@ -443,7 +443,7 @@ mod tests {
     }
 
     #[test]
-    pub fn test_slot_already_occupied() {
+    fn test_slot_already_occupied() {
         let mut graph = RenderGraph::default();
 
         graph.add_node("A", TestNode::new(0, 1));
@@ -463,7 +463,7 @@ mod tests {
     }
 
     #[test]
-    pub fn test_edge_already_exists() {
+    fn test_edge_already_exists() {
         let mut graph = RenderGraph::default();
 
         graph.add_node("A", TestNode::new(0, 1));

crates/bevy_tasks/src/countdown_event.rs

@@ -82,50 +82,56 @@ impl CountdownEvent {
     }
 }
 
-#[test]
-pub fn countdown_event_ready_after() {
-    let countdown_event = CountdownEvent::new(2);
-    countdown_event.decrement();
-    countdown_event.decrement();
-    futures_lite::future::block_on(countdown_event.listen());
-}
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn countdown_event_ready_after() {
+        let countdown_event = CountdownEvent::new(2);
+        countdown_event.decrement();
+        countdown_event.decrement();
+        futures_lite::future::block_on(countdown_event.listen());
+    }
 
-#[test]
-pub fn countdown_event_ready() {
-    let countdown_event = CountdownEvent::new(2);
-    countdown_event.decrement();
-    let countdown_event_clone = countdown_event.clone();
-    let handle =
-        std::thread::spawn(move || futures_lite::future::block_on(countdown_event_clone.listen()));
+    #[test]
+    fn countdown_event_ready() {
+        let countdown_event = CountdownEvent::new(2);
+        countdown_event.decrement();
+        let countdown_event_clone = countdown_event.clone();
+        let handle = std::thread::spawn(move || {
+            futures_lite::future::block_on(countdown_event_clone.listen())
+        });
 
-    // Pause to give the new thread time to start blocking (ugly hack)
-    std::thread::sleep(instant::Duration::from_millis(100));
+        // Pause to give the new thread time to start blocking (ugly hack)
+        std::thread::sleep(instant::Duration::from_millis(100));
 
-    countdown_event.decrement();
-    handle.join().unwrap();
-}
+        countdown_event.decrement();
+        handle.join().unwrap();
+    }
 
-#[test]
-pub fn event_resets_if_listeners_are_cleared() {
-    let event = Event::new();
-
-    // notify all listeners
-    let listener1 = event.listen();
-    event.notify(std::usize::MAX);
-    futures_lite::future::block_on(listener1);
-
-    // If all listeners are notified, the structure should now be cleared. We're free to listen
-    // again
-    let listener2 = event.listen();
-    let listener3 = event.listen();
-
-    // Verify that we are still blocked
-    assert_eq!(
-        false,
-        listener2.wait_timeout(instant::Duration::from_millis(10))
-    );
-
-    // Notify all and verify the remaining listener is notified
-    event.notify(std::usize::MAX);
-    futures_lite::future::block_on(listener3);
+    #[test]
+    fn event_resets_if_listeners_are_cleared() {
+        let event = Event::new();
+
+        // notify all listeners
+        let listener1 = event.listen();
+        event.notify(std::usize::MAX);
+        futures_lite::future::block_on(listener1);
+
+        // If all listeners are notified, the structure should now be cleared. We're free to listen
+        // again
+        let listener2 = event.listen();
+        let listener3 = event.listen();
+
+        // Verify that we are still blocked
+        assert_eq!(
+            false,
+            listener2.wait_timeout(instant::Duration::from_millis(10))
+        );
+
+        // Notify all and verify the remaining listener is notified
+        event.notify(std::usize::MAX);
+        futures_lite::future::block_on(listener3);
+    }
 }

crates/bevy_tasks/src/task_pool.rs

@@ -279,7 +279,7 @@ mod tests {
     };
 
     #[test]
-    pub fn test_spawn() {
+    fn test_spawn() {
         let pool = TaskPool::new();
 
         let foo = Box::new(42);
@@ -310,7 +310,7 @@ mod tests {
     }
 
     #[test]
-    pub fn test_mixed_spawn_local_and_spawn() {
+    fn test_mixed_spawn_local_and_spawn() {
         let pool = TaskPool::new();
 
         let foo = Box::new(42);
@@ -355,7 +355,7 @@ mod tests {
     }
 
     #[test]
-    pub fn test_thread_locality() {
+    fn test_thread_locality() {
         let pool = Arc::new(TaskPool::new());
         let count = Arc::new(AtomicI32::new(0));
         let barrier = Arc::new(Barrier::new(101));

crates/bevy_ui/src/flex/mod.rs

@@ -21,6 +21,18 @@ pub struct FlexSurface {
     stretch: Stretch,
 }
 
+// SAFE: as long as MeasureFunc is Send + Sync. https://github.com/vislyhq/stretch/issues/69
+unsafe impl Send for FlexSurface {}
+unsafe impl Sync for FlexSurface {}
+
+fn _assert_send_sync_flex_surface_impl_safe() {
+    fn _assert_send_sync<T: Send + Sync>() {}
+    _assert_send_sync::<HashMap<Entity, stretch::node::Node>>();
+    _assert_send_sync::<HashMap<WindowId, stretch::node::Node>>();
+    // FIXME https://github.com/vislyhq/stretch/issues/69
+    // _assert_send_sync::<Stretch>();
+}
+
 impl fmt::Debug for FlexSurface {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         f.debug_struct("FlexSurface")
@@ -183,10 +195,6 @@ pub enum FlexError {
     StretchError(stretch::Error),
 }
 
-// SAFE: as long as MeasureFunc is Send + Sync. https://github.com/vislyhq/stretch/issues/69
-unsafe impl Send for FlexSurface {}
-unsafe impl Sync for FlexSurface {}
-
 #[allow(clippy::too_many_arguments)]
 pub fn flex_node_system(
     windows: Res<Windows>,

crates/bevy_wgpu/src/wgpu_renderer.rs

@@ -99,6 +99,7 @@ impl WgpuRenderer {
             {
                 let winit_windows = world.get_resource::<bevy_winit::WinitWindows>().unwrap();
                 let winit_window = winit_windows.get_window(window.id()).unwrap();
+                // SAFE: The raw window handle created from a `winit::Window` is always valid.
                 let surface = unsafe { self.instance.create_surface(winit_window.deref()) };
                 render_resource_context.set_window_surface(window.id(), surface);
             }