From 5824f4f24c69da0895d8a23eee4e7a8cfb31226c Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Sat, 8 Feb 2025 11:43:26 +0100 Subject: [PATCH 1/6] feat(utils): add `OptionExt` trait for `Result` conversion Adds the following trait, and implements it for `Option`: ``` pub trait OptionExt { fn unpack(self) -> Result; fn assume>>(self, err: E) -> Result>; } ``` This allows for a more concise and readable way of handling `Option` values in Bevy systems that return `Result`s, by applying the question mark operator: ``` fn my_system(world: &World) -> bevy::ecs::Result { world.get::(Entity::PLACEHOLDER).unpack()?; world.get_resource::().assume("My resource is missing")?; Ok(()) } ``` In future commits, more of Bevy's API will be converted to return `Result`s directly, but there will always be (non-)Bevy methods that return `Option` values, so this extension trait will have its place, regardless. Signed-off-by: Jean Mertz --- crates/bevy_utils/src/lib.rs | 4 ++ crates/bevy_utils/src/option_ext.rs | 88 +++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+) create mode 100644 crates/bevy_utils/src/option_ext.rs diff --git a/crates/bevy_utils/src/lib.rs b/crates/bevy_utils/src/lib.rs index 4b7d91a40eee2..a446f3e551599 100644 --- a/crates/bevy_utils/src/lib.rs +++ b/crates/bevy_utils/src/lib.rs @@ -20,6 +20,8 @@ extern crate alloc; /// This includes the most common types in this crate, re-exported for your convenience. pub mod prelude { pub use crate::default; + #[cfg(feature = "std")] + pub use crate::option_ext::{NoneError, OptionExt as _}; } pub mod synccell; @@ -28,6 +30,8 @@ pub mod syncunsafecell; mod default; mod once; #[cfg(feature = "std")] +mod option_ext; +#[cfg(feature = "std")] mod parallel_queue; #[doc(hidden)] diff --git a/crates/bevy_utils/src/option_ext.rs b/crates/bevy_utils/src/option_ext.rs new file mode 100644 index 0000000000000..632b96d177e69 --- /dev/null +++ b/crates/bevy_utils/src/option_ext.rs @@ -0,0 +1,88 @@ +use crate::alloc::boxed::Box; +use crate::std::{error::Error, fmt}; + +/// A custom type which implements [`Error`], used to indicate that an `Option` was `None`. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct NoneError; + +impl fmt::Display for NoneError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "Unexpected None value.") + } +} + +impl Error for NoneError {} + +/// Extension trait for [`Option`]. +pub trait OptionExt { + /// Convert an `Option` to a `Result`. + fn unpack(self) -> Result; + + /// Convert an `Option` to a `Result>`. + fn assume>>(self, err: E) -> Result>; +} + +/// Extensions for [`Option`]. +impl OptionExt for Option { + /// Convert an `Option` to a `Result`. + fn unpack(self) -> Result { + self.ok_or(NoneError) + } + + /// Convert an `Option` to a `Result>`. + fn assume>>(self, err: E) -> Result> { + self.ok_or_else(|| err.into()) + } +} +#[cfg(test)] +mod tests { + use super::*; + use crate::std::string::ToString; + + #[test] + fn test_unpack_some() { + let value: Option = Some(10); + assert_eq!(value.unpack(), Ok(10)); + } + + #[test] + fn test_unpack_none() { + let value: Option = None; + let err = value.unpack().unwrap_err(); + assert_eq!(err.to_string(), "Unexpected None value."); + } + + #[test] + fn test_assume_some() { + let value: Option = Some(20); + + match value.assume("Error message") { + Ok(value) => assert_eq!(value, 20), + Err(err) => panic!("Unexpected error: {err}"), + } + } + + #[test] + fn test_assume_none_with_str() { + let value: Option = None; + let err = value.assume("index 1 should exist").unwrap_err(); + assert_eq!(err.to_string(), "index 1 should exist"); + } + + #[test] + fn test_assume_none_with_custom_error() { + #[derive(Debug)] + struct MyError; + + impl fmt::Display for MyError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "My custom error") + } + } + impl Error for MyError {} + + let value: Option = None; + let err = value.assume(MyError).unwrap_err(); + assert_eq!(err.to_string(), "My custom error"); + } +} From e0c37048450b086a15715238f9bd76e37c2c4573 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 10 Feb 2025 20:20:09 +0100 Subject: [PATCH 2/6] fixup! feat(utils): add `OptionExt` trait for `Result` conversion Signed-off-by: Jean Mertz --- crates/bevy_utils/src/lib.rs | 9 +++++---- crates/bevy_utils/src/option_ext.rs | 4 +++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/crates/bevy_utils/src/lib.rs b/crates/bevy_utils/src/lib.rs index a446f3e551599..6cb5f77fb350c 100644 --- a/crates/bevy_utils/src/lib.rs +++ b/crates/bevy_utils/src/lib.rs @@ -20,24 +20,25 @@ extern crate alloc; /// This includes the most common types in this crate, re-exported for your convenience. pub mod prelude { pub use crate::default; - #[cfg(feature = "std")] - pub use crate::option_ext::{NoneError, OptionExt as _}; + #[cfg(feature = "alloc")] + pub use crate::option_ext::OptionExt as _; } +#[cfg(feature = "alloc")] +pub mod option_ext; pub mod synccell; pub mod syncunsafecell; mod default; mod once; #[cfg(feature = "std")] -mod option_ext; -#[cfg(feature = "std")] mod parallel_queue; #[doc(hidden)] pub use once::OnceFlag; pub use default::default; +pub use option_ext::{NoneError, OptionExt as _}; #[cfg(feature = "std")] pub use parallel_queue::*; diff --git a/crates/bevy_utils/src/option_ext.rs b/crates/bevy_utils/src/option_ext.rs index 632b96d177e69..79986407a2057 100644 --- a/crates/bevy_utils/src/option_ext.rs +++ b/crates/bevy_utils/src/option_ext.rs @@ -1,5 +1,7 @@ +//! Extensions to the [`Option`] type used in Bevy. + use crate::alloc::boxed::Box; -use crate::std::{error::Error, fmt}; +use core::{error::Error, fmt}; /// A custom type which implements [`Error`], used to indicate that an `Option` was `None`. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] From f384475fce03193cce9f7cbd110c3ea6e0396d40 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 10 Feb 2025 21:50:36 +0100 Subject: [PATCH 3/6] fixup! feat(utils): add `OptionExt` trait for `Result` conversion Signed-off-by: Jean Mertz --- crates/bevy_utils/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_utils/src/lib.rs b/crates/bevy_utils/src/lib.rs index 6cb5f77fb350c..ab29724490b49 100644 --- a/crates/bevy_utils/src/lib.rs +++ b/crates/bevy_utils/src/lib.rs @@ -38,7 +38,8 @@ mod parallel_queue; pub use once::OnceFlag; pub use default::default; -pub use option_ext::{NoneError, OptionExt as _}; +#[cfg(feature = "alloc")] +pub use option_ext::NoneError; #[cfg(feature = "std")] pub use parallel_queue::*; From 210faa7418348289342a517c183eca8e6f7c30c7 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Wed, 12 Feb 2025 00:02:15 +0100 Subject: [PATCH 4/6] resolve review feedback Signed-off-by: Jean Mertz --- crates/bevy_ecs/src/result.rs | 33 ++++++++++ crates/bevy_ecs/src/result/assume.rs | 59 ++++++++++++++++++ crates/bevy_ecs/src/result/unpack.rs | 49 +++++++++++++++ crates/bevy_utils/src/lib.rs | 6 -- crates/bevy_utils/src/option_ext.rs | 90 ---------------------------- 5 files changed, 141 insertions(+), 96 deletions(-) create mode 100644 crates/bevy_ecs/src/result/assume.rs create mode 100644 crates/bevy_ecs/src/result/unpack.rs delete mode 100644 crates/bevy_utils/src/option_ext.rs diff --git a/crates/bevy_ecs/src/result.rs b/crates/bevy_ecs/src/result.rs index ef5eace90b07e..7bc471896050c 100644 --- a/crates/bevy_ecs/src/result.rs +++ b/crates/bevy_ecs/src/result.rs @@ -61,6 +61,33 @@ //! If you need special handling of individual fallible systems, you can use Bevy's [`system piping //! feature`] to capture the `Result` output of the system and handle it accordingly. //! +//! # Conveniently returning `Result` +//! +//! The [`Unpack`] and [`Assume`] traits can be used to unpack and assume any value into a +//! [`Result`] that can be handled by Rust's `?` operator. This makes working with fallible systems +//! more ergonomic. +//! +//! See: +//! +//! By default, these traits are implemented for [`Option`]: +//! +//! ```rust +//! # use bevy_ecs::prelude::*; +//! # #[derive(Component)] +//! # struct MyComponent; +//! use bevy_ecs::result::{Assume, Unpack}; +//! +//! fn my_system(world: &World) -> Result { +//! world.get::(Entity::PLACEHOLDER).unpack()?; +//! +//! world.get::(Entity::PLACEHOLDER).assume("MyComponent exists")?; +//! +//! Ok(()) +//! } +//! +//! # bevy_ecs::system::assert_is_system(my_system); +//! ``` +//! //! [`Schedule`]: crate::schedule::Schedule //! [`panic`]: panic() //! [`World`]: crate::world::World @@ -70,9 +97,15 @@ //! [`App::set_system_error_handler`]: ../../bevy_app/struct.App.html#method.set_system_error_handler //! [`system piping feature`]: crate::system::In +mod assume; +mod unpack; + use crate::{component::Tick, resource::Resource}; use alloc::{borrow::Cow, boxed::Box}; +pub use assume::Assume; +pub use unpack::Unpack; + /// A dynamic error type for use in fallible systems. pub type Error = Box; diff --git a/crates/bevy_ecs/src/result/assume.rs b/crates/bevy_ecs/src/result/assume.rs new file mode 100644 index 0000000000000..e99c0d7eb1b36 --- /dev/null +++ b/crates/bevy_ecs/src/result/assume.rs @@ -0,0 +1,59 @@ +use super::Error; + +/// Assume that `Self` is `T`, otherwise return the provided error. +pub trait Assume { + /// The error type returned by [`Assume::assume`]. + type Error; + + /// Convert `Self` to a `Result`. + fn assume>(self, err: E) -> Result; +} + +impl Assume for Option { + type Error = Error; + + fn assume>(self, err: E) -> Result { + self.ok_or_else(|| err.into()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::std::string::ToString; + use core::{error::Error, fmt}; + + #[test] + fn test_assume_some() { + let value: Option = Some(20); + + match value.assume("Error message") { + Ok(value) => assert_eq!(value, 20), + Err(err) => panic!("Unexpected error: {err}"), + } + } + + #[test] + fn test_assume_none_with_str() { + let value: Option = None; + let err = value.assume("index 1 should exist").unwrap_err(); + assert_eq!(err.to_string(), "index 1 should exist"); + } + + #[test] + fn test_assume_none_with_custom_error() { + #[derive(Debug)] + struct MyError; + + impl fmt::Display for MyError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "My custom error") + } + } + impl Error for MyError {} + + let value: Option = None; + let err = value.assume(MyError).unwrap_err(); + assert_eq!(err.to_string(), "My custom error"); + } +} diff --git a/crates/bevy_ecs/src/result/unpack.rs b/crates/bevy_ecs/src/result/unpack.rs new file mode 100644 index 0000000000000..31148f854008f --- /dev/null +++ b/crates/bevy_ecs/src/result/unpack.rs @@ -0,0 +1,49 @@ +use core::{error::Error, fmt}; + +/// Unpack `Self` to `T`, otherwise return [`Unpack::Error`]. +pub trait Unpack { + /// The error type returned by [`Unpack::unpack`]. + type Error; + + /// Convert `Self` to a `Result`. + fn unpack(self) -> Result; +} + +impl Unpack for Option { + type Error = NoneError; + + fn unpack(self) -> Result { + self.ok_or(NoneError) + } +} + +/// A custom type which implements [`Error`], used to indicate that an [`Option`] was [`None`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct NoneError; + +impl fmt::Display for NoneError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "Unexpected None value.") + } +} + +impl Error for NoneError {} + +#[cfg(test)] +mod tests { + use super::*; + use crate::std::string::ToString; + + #[test] + fn test_unpack_some() { + let value: Option = Some(10); + assert_eq!(value.unpack(), Ok(10)); + } + + #[test] + fn test_unpack_none() { + let value: Option = None; + let err = value.unpack().unwrap_err(); + assert_eq!(err.to_string(), "Unexpected None value."); + } +} diff --git a/crates/bevy_utils/src/lib.rs b/crates/bevy_utils/src/lib.rs index ab29724490b49..4b7d91a40eee2 100644 --- a/crates/bevy_utils/src/lib.rs +++ b/crates/bevy_utils/src/lib.rs @@ -20,12 +20,8 @@ extern crate alloc; /// This includes the most common types in this crate, re-exported for your convenience. pub mod prelude { pub use crate::default; - #[cfg(feature = "alloc")] - pub use crate::option_ext::OptionExt as _; } -#[cfg(feature = "alloc")] -pub mod option_ext; pub mod synccell; pub mod syncunsafecell; @@ -38,8 +34,6 @@ mod parallel_queue; pub use once::OnceFlag; pub use default::default; -#[cfg(feature = "alloc")] -pub use option_ext::NoneError; #[cfg(feature = "std")] pub use parallel_queue::*; diff --git a/crates/bevy_utils/src/option_ext.rs b/crates/bevy_utils/src/option_ext.rs deleted file mode 100644 index 79986407a2057..0000000000000 --- a/crates/bevy_utils/src/option_ext.rs +++ /dev/null @@ -1,90 +0,0 @@ -//! Extensions to the [`Option`] type used in Bevy. - -use crate::alloc::boxed::Box; -use core::{error::Error, fmt}; - -/// A custom type which implements [`Error`], used to indicate that an `Option` was `None`. -#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct NoneError; - -impl fmt::Display for NoneError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "Unexpected None value.") - } -} - -impl Error for NoneError {} - -/// Extension trait for [`Option`]. -pub trait OptionExt { - /// Convert an `Option` to a `Result`. - fn unpack(self) -> Result; - - /// Convert an `Option` to a `Result>`. - fn assume>>(self, err: E) -> Result>; -} - -/// Extensions for [`Option`]. -impl OptionExt for Option { - /// Convert an `Option` to a `Result`. - fn unpack(self) -> Result { - self.ok_or(NoneError) - } - - /// Convert an `Option` to a `Result>`. - fn assume>>(self, err: E) -> Result> { - self.ok_or_else(|| err.into()) - } -} -#[cfg(test)] -mod tests { - use super::*; - use crate::std::string::ToString; - - #[test] - fn test_unpack_some() { - let value: Option = Some(10); - assert_eq!(value.unpack(), Ok(10)); - } - - #[test] - fn test_unpack_none() { - let value: Option = None; - let err = value.unpack().unwrap_err(); - assert_eq!(err.to_string(), "Unexpected None value."); - } - - #[test] - fn test_assume_some() { - let value: Option = Some(20); - - match value.assume("Error message") { - Ok(value) => assert_eq!(value, 20), - Err(err) => panic!("Unexpected error: {err}"), - } - } - - #[test] - fn test_assume_none_with_str() { - let value: Option = None; - let err = value.assume("index 1 should exist").unwrap_err(); - assert_eq!(err.to_string(), "index 1 should exist"); - } - - #[test] - fn test_assume_none_with_custom_error() { - #[derive(Debug)] - struct MyError; - - impl fmt::Display for MyError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "My custom error") - } - } - impl Error for MyError {} - - let value: Option = None; - let err = value.assume(MyError).unwrap_err(); - assert_eq!(err.to_string(), "My custom error"); - } -} From 130af8b528ca49254930f0800c3d2e5c7f5c61ab Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Wed, 12 Feb 2025 00:07:44 +0100 Subject: [PATCH 5/6] Update crates/bevy_ecs/src/result.rs Co-authored-by: Alice Cecile --- crates/bevy_ecs/src/result.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/result.rs b/crates/bevy_ecs/src/result.rs index 7bc471896050c..1408c082dfb82 100644 --- a/crates/bevy_ecs/src/result.rs +++ b/crates/bevy_ecs/src/result.rs @@ -63,7 +63,7 @@ //! //! # Conveniently returning `Result` //! -//! The [`Unpack`] and [`Assume`] traits can be used to unpack and assume any value into a +//! The [`Unpack`] and [`Assume`] traits can be used to transform any value into a //! [`Result`] that can be handled by Rust's `?` operator. This makes working with fallible systems //! more ergonomic. //! From 650378dbbd431f2a43f2c05d089935494e374976 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Wed, 12 Feb 2025 00:19:11 +0100 Subject: [PATCH 6/6] resolve review feedback Signed-off-by: Jean Mertz --- crates/bevy_ecs/src/result.rs | 9 ++++++--- crates/bevy_ecs/src/result/assume.rs | 7 +++++++ crates/bevy_ecs/src/result/unpack.rs | 9 ++++++++- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/result.rs b/crates/bevy_ecs/src/result.rs index 1408c082dfb82..12bfe37a06166 100644 --- a/crates/bevy_ecs/src/result.rs +++ b/crates/bevy_ecs/src/result.rs @@ -63,9 +63,12 @@ //! //! # Conveniently returning `Result` //! -//! The [`Unpack`] and [`Assume`] traits can be used to transform any value into a -//! [`Result`] that can be handled by Rust's `?` operator. This makes working with fallible systems -//! more ergonomic. +//! The [`Unpack`] and [`Assume`] traits can be used to transform any value into a [`Result`] that +//! can be handled by Rust's `?` operator. This makes working with fallible systems more ergonomic. +//! +//! [`Unpack::unpack`] should be used in place of `unwrap` (to quickly get a value), while +//! [`Assume::assume`] should be used in place of `expect` (if you want a custom error explaining +//! the assumption that was violated). //! //! See: //! diff --git a/crates/bevy_ecs/src/result/assume.rs b/crates/bevy_ecs/src/result/assume.rs index e99c0d7eb1b36..fa429fa6c618d 100644 --- a/crates/bevy_ecs/src/result/assume.rs +++ b/crates/bevy_ecs/src/result/assume.rs @@ -1,8 +1,15 @@ use super::Error; /// Assume that `Self` is `T`, otherwise return the provided error. +/// +/// This can be a drop-in replacement for `expect`, combined with the question mark operator and +/// [`Result`](super::Result) return type, to get the same ergonomics as `expect` but without the +/// panicking behavior (when using a non-panicking error handler). pub trait Assume { /// The error type returned by [`Assume::assume`]. + /// + /// Typically implements the [`Error`] trait, allowing it to match Bevy's fallible system + /// [`Result`](super::Result) return type. type Error; /// Convert `Self` to a `Result`. diff --git a/crates/bevy_ecs/src/result/unpack.rs b/crates/bevy_ecs/src/result/unpack.rs index 31148f854008f..272f23976dce2 100644 --- a/crates/bevy_ecs/src/result/unpack.rs +++ b/crates/bevy_ecs/src/result/unpack.rs @@ -1,8 +1,15 @@ use core::{error::Error, fmt}; /// Unpack `Self` to `T`, otherwise return [`Unpack::Error`]. +/// +/// This can be a drop-in replacement for `unwrap`, combined with the question mark operator and +/// [`Result`](super::Result) return type, to get the same ergonomics as `unwrap` but without the +/// panicking behavior (when using a non-panicking error handler). pub trait Unpack { /// The error type returned by [`Unpack::unpack`]. + /// + /// Typically implements the [`Error`] trait, allowing it to match Bevy's fallible system + /// [`Result`](super::Result) return type. type Error; /// Convert `Self` to a `Result`. @@ -17,7 +24,7 @@ impl Unpack for Option { } } -/// A custom type which implements [`Error`], used to indicate that an [`Option`] was [`None`]. +/// An [`Error`] which indicates that an [`Option`] was [`None`]. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct NoneError;