From 9254297acd6a0ebc9af0dbdacc9f2ae8fadc82bd Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Mon, 14 Apr 2025 15:59:48 -0400 Subject: [PATCH] Use never_say_never hack to work around Rust 2024 regression for fn traits (#18804) # Objective After #17967, closures which always panic no longer satisfy various Bevy traits. Principally, this affects observers, systems and commands. While this may seem pointless (systems which always panic are kind of useless), it is distinctly annoying when using the `todo!` macro, or when writing tests that should panic. Fixes #18778. ## Solution - Add failing tests to demonstrate the problem - Add the trick from [`never_say_never`](https://docs.rs/never-say-never/latest/never_say_never/) to name the `!` type on stable Rust - Write looots of docs explaining what the heck is going on and why we've done this terrible thing ## To do Unfortunately I couldn't figure out how to avoid conflicting impls, and I am out of time for today, the week and uh the week after that. Vacation! If you feel like finishing this for me, please submit PRs to my branch and I can review and press the button for it while I'm off. Unless you're Cart, in which case you have write permissions to my branch! - [ ] fix for commands - [ ] fix for systems - [ ] fix for observers - [ ] revert https://github.com/bevyengine/bevy-website/pull/2092/ ## Testing I've added a compile test for these failure cases and a few adjacent non-failing cases (with explicit return types). --------- Co-authored-by: Carter Anderson --- crates/bevy_ecs/src/error/command_handling.rs | 12 ++++ crates/bevy_ecs/src/lib.rs | 1 + crates/bevy_ecs/src/never.rs | 39 ++++++++++++ crates/bevy_ecs/src/schedule/config.rs | 11 ++++ crates/bevy_ecs/src/system/mod.rs | 60 ++++++++++++++++++- crates/bevy_ecs/src/system/observer_system.rs | 28 ++++++--- crates/bevy_ecs/src/system/schedule_system.rs | 6 +- 7 files changed, 145 insertions(+), 12 deletions(-) create mode 100644 crates/bevy_ecs/src/never.rs diff --git a/crates/bevy_ecs/src/error/command_handling.rs b/crates/bevy_ecs/src/error/command_handling.rs index 0e21412832..d85ad4a87e 100644 --- a/crates/bevy_ecs/src/error/command_handling.rs +++ b/crates/bevy_ecs/src/error/command_handling.rs @@ -2,6 +2,7 @@ use core::{any::type_name, fmt}; use crate::{ entity::Entity, + never::Never, system::{entity_command::EntityCommandError, Command, EntityCommand}, world::{error::EntityMutableFetchError, World}, }; @@ -42,6 +43,17 @@ where } } +impl HandleError for C +where + C: Command, +{ + fn handle_error_with(self, _error_handler: fn(BevyError, ErrorContext)) -> impl Command { + move |world: &mut World| { + self.apply(world); + } + } +} + impl HandleError for C where C: Command, diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index eee14c0593..99f95763d5 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -43,6 +43,7 @@ pub mod identifier; pub mod intern; pub mod label; pub mod name; +pub mod never; pub mod observer; pub mod query; #[cfg(feature = "bevy_reflect")] diff --git a/crates/bevy_ecs/src/never.rs b/crates/bevy_ecs/src/never.rs new file mode 100644 index 0000000000..ba814c7006 --- /dev/null +++ b/crates/bevy_ecs/src/never.rs @@ -0,0 +1,39 @@ +//! A workaround for the `!` type in stable Rust. +//! +//! This approach is taken from the [`never_say_never`] crate, +//! reimplemented here to avoid adding a new dependency. +//! +//! This module exists due to a change in [never type fallback inference] in the Rust 2024 edition. +//! This caused failures in `bevy_ecs`'s traits which are implemented for functions +//! (like [`System`](crate::system::System)) when working with panicking closures. +//! +//! Note that using this hack is not recommended in general; +//! by doing so you are knowingly opting out of rustc's stability guarantees. +//! Code that compiles due to this hack may break in future versions of Rust. +//! +//! Please read [issue #18778](https://github.com/bevyengine/bevy/issues/18778) for an explanation of why +//! Bevy has chosen to use this workaround. +//! +//! [`never_say_never`]: https://crates.io/crates/never_say_never +//! [never type fallback inference]: https://doc.rust-lang.org/edition-guide/rust-2024/never-type-fallback.html + +mod fn_ret { + /// A helper trait for naming the ! type. + #[doc(hidden)] + pub trait FnRet { + /// The return type of the function. + type Output; + } + + /// This blanket implementation allows us to name the never type, + /// by using the associated type of this trait for `fn() -> !`. + impl FnRet for fn() -> R { + type Output = R; + } +} + +/// A hacky type alias for the `!` (never) type. +/// +/// This knowingly opts out of rustc's stability guarantees. +/// Read the module documentation carefully before using this! +pub type Never = ! as fn_ret::FnRet>::Output; diff --git a/crates/bevy_ecs/src/schedule/config.rs b/crates/bevy_ecs/src/schedule/config.rs index 8188dc144c..b98205e32b 100644 --- a/crates/bevy_ecs/src/schedule/config.rs +++ b/crates/bevy_ecs/src/schedule/config.rs @@ -3,6 +3,7 @@ use variadics_please::all_tuples; use crate::{ error::Result, + never::Never, schedule::{ auto_insert_apply_deferred::IgnoreDeferred, condition::{BoxedCondition, Condition}, @@ -570,6 +571,16 @@ where } } +impl IntoScheduleConfigs for F +where + F: IntoSystem<(), Never, Marker>, +{ + fn into_configs(self) -> ScheduleConfigs { + let wrapper = InfallibleSystemWrapper::new(IntoSystem::into_system(self)); + ScheduleConfigs::ScheduleConfig(ScheduleSystem::into_config(Box::new(wrapper))) + } +} + /// Marker component to allow for conflicting implementations of [`IntoScheduleConfigs`] #[doc(hidden)] pub struct Fallible; diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index fbb4a458d5..1bdd26add2 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -337,7 +337,8 @@ mod tests { component::{Component, Components}, entity::{Entities, Entity}, error::Result, - prelude::{AnyOf, EntityRef}, + name::Name, + prelude::{AnyOf, EntityRef, Trigger}, query::{Added, Changed, Or, With, Without}, removal_detection::RemovedComponents, resource::Resource, @@ -349,7 +350,7 @@ mod tests { Commands, In, IntoSystem, Local, NonSend, NonSendMut, ParamSet, Query, Res, ResMut, Single, StaticSystemParam, System, SystemState, }, - world::{DeferredWorld, EntityMut, FromWorld, World}, + world::{DeferredWorld, EntityMut, FromWorld, OnAdd, World}, }; use super::ScheduleSystem; @@ -1823,4 +1824,59 @@ mod tests { let mut world = World::new(); run_system(&mut world, sys); } + + // Regression test for + // https://github.com/bevyengine/bevy/issues/18778 + // + // Dear rustc team, please reach out if you encounter this + // in a crater run and we can work something out! + // + // These todo! macro calls should never be removed; + // they're intended to demonstrate real-world usage + // in a way that's clearer than simply calling `panic!` + // + // Because type inference behaves differently for functions and closures, + // we need to test both, in addition to explicitly annotating the return type + // to ensure that there are no upstream regressions there. + #[test] + fn nondiverging_never_trait_impls() { + // This test is a compilation test: + // no meaningful logic is ever actually evaluated. + // It is simply intended to check that the correct traits are implemented + // when todo! or similar nondiverging panics are used. + let mut world = World::new(); + let mut schedule = Schedule::default(); + + fn sys(_query: Query<&Name>) { + todo!() + } + + schedule.add_systems(sys); + schedule.add_systems(|_query: Query<&Name>| {}); + schedule.add_systems(|_query: Query<&Name>| todo!()); + #[expect(clippy::unused_unit, reason = "this forces the () return type")] + schedule.add_systems(|_query: Query<&Name>| -> () { todo!() }); + + fn obs(_trigger: Trigger) { + todo!() + } + + world.add_observer(obs); + world.add_observer(|_trigger: Trigger| {}); + world.add_observer(|_trigger: Trigger| todo!()); + #[expect(clippy::unused_unit, reason = "this forces the () return type")] + world.add_observer(|_trigger: Trigger| -> () { todo!() }); + + fn my_command(_world: &mut World) { + todo!() + } + + world.commands().queue(my_command); + world.commands().queue(|_world: &mut World| {}); + world.commands().queue(|_world: &mut World| todo!()); + #[expect(clippy::unused_unit, reason = "this forces the () return type")] + world + .commands() + .queue(|_world: &mut World| -> () { todo!() }); + } } diff --git a/crates/bevy_ecs/src/system/observer_system.rs b/crates/bevy_ecs/src/system/observer_system.rs index 2cfd99b3f9..d042154631 100644 --- a/crates/bevy_ecs/src/system/observer_system.rs +++ b/crates/bevy_ecs/src/system/observer_system.rs @@ -5,6 +5,7 @@ use crate::{ archetype::ArchetypeComponentId, component::{ComponentId, Tick}, error::Result, + never::Never, prelude::{Bundle, Trigger}, query::Access, schedule::{Fallible, Infallible}, @@ -45,7 +46,7 @@ pub trait IntoObserverSystem: Send + 'st fn into_system(this: Self) -> Self::System; } -impl IntoObserverSystem for S +impl IntoObserverSystem for S where S: IntoSystem, Out, M> + Send + 'static, S::System: ObserverSystem, @@ -66,7 +67,19 @@ where E: Send + Sync + 'static, B: Bundle, { - type System = InfallibleObserverWrapper; + type System = InfallibleObserverWrapper; + + fn into_system(this: Self) -> Self::System { + InfallibleObserverWrapper::new(IntoSystem::into_system(this)) + } +} +impl IntoObserverSystem for S +where + S: IntoSystem, Never, M> + Send + 'static, + E: Send + Sync + 'static, + B: Bundle, +{ + type System = InfallibleObserverWrapper; fn into_system(this: Self) -> Self::System { InfallibleObserverWrapper::new(IntoSystem::into_system(this)) @@ -74,12 +87,12 @@ where } /// A wrapper that converts an observer system that returns `()` into one that returns `Ok(())`. -pub struct InfallibleObserverWrapper { +pub struct InfallibleObserverWrapper { observer: S, - _marker: PhantomData<(E, B)>, + _marker: PhantomData<(E, B, Out)>, } -impl InfallibleObserverWrapper { +impl InfallibleObserverWrapper { /// Create a new `InfallibleObserverWrapper`. pub fn new(observer: S) -> Self { Self { @@ -89,11 +102,12 @@ impl InfallibleObserverWrapper { } } -impl System for InfallibleObserverWrapper +impl System for InfallibleObserverWrapper where - S: ObserverSystem, + S: ObserverSystem, E: Send + Sync + 'static, B: Bundle, + Out: Send + Sync + 'static, { type In = Trigger<'static, E, B>; type Out = Result; diff --git a/crates/bevy_ecs/src/system/schedule_system.rs b/crates/bevy_ecs/src/system/schedule_system.rs index 2b82047c7d..75fad2b7e9 100644 --- a/crates/bevy_ecs/src/system/schedule_system.rs +++ b/crates/bevy_ecs/src/system/schedule_system.rs @@ -12,16 +12,16 @@ use crate::{ use super::{IntoSystem, SystemParamValidationError}; /// A wrapper system to change a system that returns `()` to return `Ok(())` to make it into a [`ScheduleSystem`] -pub struct InfallibleSystemWrapper>(S); +pub struct InfallibleSystemWrapper>(S); -impl> InfallibleSystemWrapper { +impl> InfallibleSystemWrapper { /// Create a new `OkWrapperSystem` pub fn new(system: S) -> Self { Self(IntoSystem::into_system(system)) } } -impl> System for InfallibleSystemWrapper { +impl> System for InfallibleSystemWrapper { type In = (); type Out = Result;