Skip to content

Commit 2730c6e

Browse files
authored
Remove panic from code gen of an enum's FromReflect derive. (#24068)
# Objective - I noticed this while reviewing #24048. Our current FromReflect derive macro adds a panic for cases when a PartialReflect-ed enum includes a variant we weren't expecting. This can happen for a few reasons, e.g., a variant of the enum has been removed. In any case, **crashing the program** is not the correct answer, since we already have an appropriate response to a failed conversion: just return None! ## Solution - Return None instead of panicking. I also checked if there are any other panics, and it looks like none in the codegen. The only remaining panics have to do with some auto registration file stuff, which seems appropriate to panic on. ## Testing - Added a unit test.
1 parent 4b77775 commit 2730c6e

2 files changed

Lines changed: 24 additions & 1 deletion

File tree

crates/bevy_reflect/derive/src/from_reflect.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ pub(crate) fn impl_enum(reflect_enum: &ReflectEnum) -> proc_macro2::TokenStream
9595
{
9696
match #bevy_reflect_path::enums::Enum::variant_name(#ref_value) {
9797
#match_branches
98-
name => panic!("variant with name `{}` does not exist on enum `{}`", name, <Self as #bevy_reflect_path::TypePath>::type_path()),
98+
name => #FQOption::None,
9999
}
100100
} else {
101101
#FQOption::None

crates/bevy_reflect/src/lib.rs

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1720,6 +1720,29 @@ mod tests {
17201720
);
17211721
}
17221722

1723+
#[test]
1724+
fn enum_from_reflect_does_not_panic() {
1725+
#[derive(Reflect, PartialEq, Eq, Debug)]
1726+
enum A {
1727+
Hot,
1728+
Cold,
1729+
}
1730+
1731+
#[derive(Reflect, PartialEq, Eq, Debug)]
1732+
enum B {
1733+
Hot,
1734+
Cold,
1735+
Warm,
1736+
}
1737+
1738+
// There's no difference between the reflected data of these enum variants - they are named
1739+
// the same, so we are able to convert them.
1740+
assert_eq!(A::from_reflect(&B::Hot), Some(A::Hot));
1741+
assert_eq!(A::from_reflect(&B::Cold), Some(A::Cold));
1742+
// This variant doesn't exist in `A`, so it should not be converted.
1743+
assert_eq!(A::from_reflect(&B::Warm), None);
1744+
}
1745+
17231746
#[test]
17241747
fn reflect_partial_cmp_array_length_difference() {
17251748
use core::cmp::Ordering;

0 commit comments

Comments
 (0)