From 5b134ce1789c227703786f6efe40bc105abddfa1 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Sun, 18 Jun 2023 17:23:44 +0200 Subject: [PATCH 01/11] Add leading dot to Display for ParsedPath ``` field[0].foo.bar ``` vs ``` .field[0].foo.bar ``` - Both are valid as input for parsing - The one with the leading dot is more consistant, as in the current implementation, the hatch will be present if the leading access is a field index. - IMO having the leading dot demonstrates better we are accessing the field `field`, rather than index 0 of variable `field`. - The logic for displaying with the leading dot is simpler, we don't need to discriminate the first element. Also implement Display for Access. --- crates/bevy_reflect/src/path.rs | 49 +++++++++++++++++---------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/crates/bevy_reflect/src/path.rs b/crates/bevy_reflect/src/path.rs index a9fd7ffe2febe..37a3a1e79fbb8 100644 --- a/crates/bevy_reflect/src/path.rs +++ b/crates/bevy_reflect/src/path.rs @@ -412,32 +412,33 @@ impl ParsedPath { } } -impl fmt::Display for ParsedPath { +impl fmt::Display for Access { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - for (idx, (access, _)) in self.0.iter().enumerate() { - match access { - Access::Field(field) => { - if idx != 0 { - Token::DOT.fmt(f)?; - } - f.write_str(field.as_str())?; - } - Access::FieldIndex(index) => { - Token::CROSSHATCH.fmt(f)?; - index.fmt(f)?; - } - Access::TupleIndex(index) => { - if idx != 0 { - Token::DOT.fmt(f)?; - } - index.fmt(f)?; - } - Access::ListIndex(index) => { - Token::OPEN_BRACKET.fmt(f)?; - index.fmt(f)?; - Token::CLOSE_BRACKET.fmt(f)?; - } + match self { + Access::Field(field) => { + Token::DOT.fmt(f)?; + f.write_str(field.as_ref()) } + Access::FieldIndex(index) => { + Token::CROSSHATCH.fmt(f)?; + index.fmt(f) + } + Access::TupleIndex(index) => { + Token::DOT.fmt(f)?; + index.fmt(f) + } + Access::ListIndex(index) => { + Token::OPEN_BRACKET.fmt(f)?; + index.fmt(f)?; + Token::CLOSE_BRACKET.fmt(f) + } + } + } +} +impl fmt::Display for ParsedPath { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + for (access, _) in self.0.iter() { + write!(f, "{access}")?; } Ok(()) } From e5ce83060984138985c318bcee4d549d0456cef7 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Sun, 18 Jun 2023 17:29:50 +0200 Subject: [PATCH 02/11] Improve reflect path.rs tests --- crates/bevy_reflect/src/path.rs | 97 ++++++++++++++++++--------------- 1 file changed, 53 insertions(+), 44 deletions(-) diff --git a/crates/bevy_reflect/src/path.rs b/crates/bevy_reflect/src/path.rs index 37a3a1e79fbb8..58613c06021cd 100644 --- a/crates/bevy_reflect/src/path.rs +++ b/crates/bevy_reflect/src/path.rs @@ -857,54 +857,69 @@ mod tests { Struct { value: char }, } + fn a_sample() -> A { + A { + w: 1, + x: B { + foo: 10, + bar: C { baz: 3.14 }, + }, + y: vec![C { baz: 1.0 }, C { baz: 2.0 }], + z: D(E(10.0, 42)), + unit_variant: F::Unit, + tuple_variant: F::Tuple(123, 321), + struct_variant: F::Struct { value: 'm' }, + array: [86, 75, 309], + tuple: (true, 1.23), + } + } + + fn access_field(field: &'static str) -> Access { + Access::Field(field.to_string()) + } + #[test] fn parsed_path_parse() { assert_eq!( &*ParsedPath::parse("w").unwrap().0, - &[(Access::Field("w".to_string()), 1)] + &[(access_field("w"), 1)] ); assert_eq!( &*ParsedPath::parse("x.foo").unwrap().0, - &[ - (Access::Field("x".to_string()), 1), - (Access::Field("foo".to_string()), 2) - ] + &[(access_field("x"), 1), (access_field("foo"), 2)] ); assert_eq!( &*ParsedPath::parse("x.bar.baz").unwrap().0, &[ - (Access::Field("x".to_string()), 1), - (Access::Field("bar".to_string()), 2), - (Access::Field("baz".to_string()), 6) + (access_field("x"), 1), + (access_field("bar"), 2), + (access_field("baz"), 6) ] ); assert_eq!( &*ParsedPath::parse("y[1].baz").unwrap().0, &[ - (Access::Field("y".to_string()), 1), + (access_field("y"), 1), (Access::ListIndex(1), 2), - (Access::Field("baz".to_string()), 5) + (access_field("baz"), 5) ] ); assert_eq!( &*ParsedPath::parse("z.0.1").unwrap().0, &[ - (Access::Field("z".to_string()), 1), + (access_field("z"), 1), (Access::TupleIndex(0), 2), (Access::TupleIndex(1), 4), ] ); assert_eq!( &*ParsedPath::parse("x#0").unwrap().0, - &[ - (Access::Field("x".to_string()), 1), - (Access::FieldIndex(0), 2), - ] + &[(access_field("x"), 1), (Access::FieldIndex(0), 2)] ); assert_eq!( &*ParsedPath::parse("x#0#1").unwrap().0, &[ - (Access::Field("x".to_string()), 1), + (access_field("x"), 1), (Access::FieldIndex(0), 2), (Access::FieldIndex(1), 4) ] @@ -913,20 +928,7 @@ mod tests { #[test] fn parsed_path_get_field() { - let a = A { - w: 1, - x: B { - foo: 10, - bar: C { baz: 3.14 }, - }, - y: vec![C { baz: 1.0 }, C { baz: 2.0 }], - z: D(E(10.0, 42)), - unit_variant: F::Unit, - tuple_variant: F::Tuple(123, 321), - struct_variant: F::Struct { value: 'm' }, - array: [86, 75, 309], - tuple: (true, 1.23), - }; + let a = a_sample(); let b = ParsedPath::parse("w").unwrap(); let c = ParsedPath::parse("x.foo").unwrap(); @@ -1003,20 +1005,7 @@ mod tests { #[test] fn reflect_path() { - let mut a = A { - w: 1, - x: B { - foo: 10, - bar: C { baz: 3.14 }, - }, - y: vec![C { baz: 1.0 }, C { baz: 2.0 }], - z: D(E(10.0, 42)), - unit_variant: F::Unit, - tuple_variant: F::Tuple(123, 321), - struct_variant: F::Struct { value: 'm' }, - array: [86, 75, 309], - tuple: (true, 1.23), - }; + let mut a = a_sample(); assert_eq!(*a.path::("w").unwrap(), 1); assert_eq!(*a.path::("x.foo").unwrap(), 10); @@ -1076,4 +1065,24 @@ mod tests { Err(ReflectPathError::IndexParseError(_)) )); } + + #[test] + fn accept_leading_tokens() { + assert_eq!( + &*ParsedPath::parse(".w").unwrap().0, + &[(access_field("w"), 1)] + ); + assert_eq!( + &*ParsedPath::parse("#0.foo").unwrap().0, + &[(Access::FieldIndex(0), 1), (access_field("foo"), 3)] + ); + assert_eq!( + &*ParsedPath::parse(".5").unwrap().0, + &[(Access::TupleIndex(5), 1)] + ); + assert_eq!( + &*ParsedPath::parse("[0].bar").unwrap().0, + &[(Access::ListIndex(0), 1), (access_field("bar"), 4)] + ); + } } From 49ed09f2f3d30171b21962fe1555f79b268c4db7 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Sun, 18 Jun 2023 17:30:35 +0200 Subject: [PATCH 03/11] Use a Box over String for Access::Field This removes 8 bytes to the size of `Access`. --- crates/bevy_reflect/src/path.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bevy_reflect/src/path.rs b/crates/bevy_reflect/src/path.rs index 58613c06021cd..2e0b08cf96917 100644 --- a/crates/bevy_reflect/src/path.rs +++ b/crates/bevy_reflect/src/path.rs @@ -451,7 +451,7 @@ impl fmt::Display for ParsedPath { /// A path is composed of multiple accesses in sequence. #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] enum Access { - Field(String), + Field(Box), FieldIndex(usize), TupleIndex(usize), ListIndex(usize), @@ -485,7 +485,7 @@ enum AccessRef<'a> { impl<'a> AccessRef<'a> { fn to_owned(&self) -> Access { match self { - Self::Field(value) => Access::Field(value.to_string()), + Self::Field(value) => Access::Field(value.to_string().into_boxed_str()), Self::FieldIndex(value) => Access::FieldIndex(*value), Self::TupleIndex(value) => Access::TupleIndex(*value), Self::ListIndex(value) => Access::ListIndex(*value), @@ -875,7 +875,7 @@ mod tests { } fn access_field(field: &'static str) -> Access { - Access::Field(field.to_string()) + Access::Field(field.to_string().into_boxed_str()) } #[test] From 60ea04d2a7b80776c23ebb8e2f68dff811882339 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Sun, 18 Jun 2023 17:41:25 +0200 Subject: [PATCH 04/11] Add a section mentioning `GetPath` in bevy_reflect The root doc for bevy_reflect doesn't mention `GetPath`. It's a fairly useful featuree and I've seen people be surprised to learn it exists. --- crates/bevy_reflect/src/lib.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs index 9a70da803f215..49a8781f29e20 100644 --- a/crates/bevy_reflect/src/lib.rs +++ b/crates/bevy_reflect/src/lib.rs @@ -229,6 +229,31 @@ //! //! All primitives and simple types implement `FromReflect` by relying on their [`Default`] implementation. //! +//! # Path navigation +//! +//! The [`GetPath`] trait allows accessing arbitrary nested fields of a [`Reflect`] type. +//! +//! Using [`GetPath`], it is possible to use a path strings to access a specific field +//! of a reflected type. +//! +//! ``` +//! # use bevy_reflect::{Reflect, GetPath}; +//! #[derive(Reflect)] +//! struct MyStruct { +//! value: Vec> +//! } +//! +//! let my_struct = MyStruct { +//! value: vec![None, None, Some(123)], +//! }; +//! assert_eq!( +//! my_struct.path::(".value[2].0").unwrap(), +//! &123, +//! ); +//! ``` +//! +//! Check the [`GetPath`] documentation for more details. +//! //! # Type Registration //! //! This crate also comes with a [`TypeRegistry`] that can be used to store and retrieve additional type metadata at runtime, From 28b04a9f5c326ed0b26db33339a11127976bc739 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Sun, 18 Jun 2023 20:19:28 +0200 Subject: [PATCH 05/11] Split path, better error handling, clean element This commit: - Splits the `path.rs` module of `bevy_reflect` in two. the `Access` structs are now defined in their own submodules - Revamps error handling - Separate Parse error from access errors - rename "index" to offset. "offset" is more commonly used to designate parsing position, and it can't be confused with the other kind of indexing done in this module - Instead of having one error variant per possible mismatching type, create a single error with a "expected" and "actual" type. - The error values now contain more details about the nature of the error. - Refactor the `read_element{_mut}` methods - They are now about 70 lines, while previously they were 188 lines - They are much more readable now. - Rename them to `element{_mut}`, since the "read" is a bit misleading. - They accept a `self` rather than `&self`, since AccessRef is Copy now. - We also rewrite Display for Access in term of write! rather than sequential write.fmt() - Rename `to_ref` to `as_ref` - Make AccessRef Copy: because it's a very small type. The downside is that it doesn't benefit from niching in the error type anymore. --- crates/bevy_reflect/src/path/access.rs | 240 +++++++++++ .../bevy_reflect/src/{path.rs => path/mod.rs} | 398 ++++-------------- 2 files changed, 318 insertions(+), 320 deletions(-) create mode 100644 crates/bevy_reflect/src/path/access.rs rename crates/bevy_reflect/src/{path.rs => path/mod.rs} (64%) diff --git a/crates/bevy_reflect/src/path/access.rs b/crates/bevy_reflect/src/path/access.rs new file mode 100644 index 0000000000000..cc62971190966 --- /dev/null +++ b/crates/bevy_reflect/src/path/access.rs @@ -0,0 +1,240 @@ +use std::fmt; + +use super::ReflectPathError; +use crate::{Reflect, ReflectMut, ReflectRef, VariantType}; +use thiserror::Error; + +type InnerResult = Result, AccessError<'static>>; + +#[derive(Debug, PartialEq, Eq, Error)] +pub enum AccessError<'a> { + #[error( + "the current {ty} doesn't have the {} {}", + access.kind(), + access.display_value(), + )] + Access { ty: Type, access: AccessRef<'a> }, + + #[error("invalid type shape: expected {expected} but found a reflect {actual}")] + Type { expected: Type, actual: Type }, + + #[error("invalid enum access: expected {expected} variant but found {actual} variant")] + Enum { expected: Type, actual: Type }, +} + +impl<'a> AccessError<'a> { + fn with_offset(self, offset: usize) -> ReflectPathError<'a> { + let error = self; + ReflectPathError::InvalidAccess { offset, error } + } +} +impl AccessError<'static> { + fn bad_enum(expected: Type, actual: impl Into) -> Self { + let actual = actual.into(); + AccessError::Enum { expected, actual } + } + fn bad_type(expected: Type, actual: impl Into) -> Self { + let actual = actual.into(); + AccessError::Type { expected, actual } + } +} + +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum Type { + Struct, + TupleStruct, + Tuple, + List, + Array, + Map, + Enum, + Value, + Unit, +} + +impl fmt::Display for Type { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let name = match self { + Type::Struct => "struct", + Type::TupleStruct => "tuple struct", + Type::Tuple => "tuple", + Type::List => "list", + Type::Array => "array", + Type::Map => "map", + Type::Enum => "enum", + Type::Value => "value", + Type::Unit => "unit", + }; + write!(f, "{name}") + } +} +impl<'a> From> for Type { + fn from(value: ReflectRef<'a>) -> Self { + match value { + ReflectRef::Struct(_) => Type::Struct, + ReflectRef::TupleStruct(_) => Type::TupleStruct, + ReflectRef::Tuple(_) => Type::Tuple, + ReflectRef::List(_) => Type::List, + ReflectRef::Array(_) => Type::Array, + ReflectRef::Map(_) => Type::Map, + ReflectRef::Enum(_) => Type::Enum, + ReflectRef::Value(_) => Type::Value, + } + } +} +impl From for Type { + fn from(value: VariantType) -> Self { + match value { + VariantType::Struct => Type::Struct, + VariantType::Tuple => Type::Tuple, + VariantType::Unit => Type::Unit, + } + } +} + +/// A singular owned element access within a path. +/// +/// Can be applied to a `dyn Reflect` to get a reference to the targeted element. +/// +/// A path is composed of multiple accesses in sequence. +#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub(super) enum Access { + Field(Box), + FieldIndex(usize), + TupleIndex(usize), + ListIndex(usize), +} + +impl fmt::Display for Access { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Access::Field(field) => write!(f, ".{field}"), + Access::FieldIndex(index) => write!(f, "#{index}"), + Access::TupleIndex(index) => write!(f, ".{index}"), + Access::ListIndex(index) => write!(f, "[{index}]"), + } + } +} + +/// A singular borrowed element access within a path. +/// +/// Can be applied to a `dyn Reflect` to get a reference to the targeted element. +/// +/// Does not own the backing store it's sourced from. +/// For an owned version, you can convert one to an [`Access`] with [`AccessRef::to_owned`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum AccessRef<'a> { + Field(&'a str), + FieldIndex(usize), + TupleIndex(usize), + ListIndex(usize), +} + +impl Access { + pub(super) fn as_ref(&self) -> AccessRef<'_> { + match self { + Self::Field(value) => AccessRef::Field(value), + Self::FieldIndex(value) => AccessRef::FieldIndex(*value), + Self::TupleIndex(value) => AccessRef::TupleIndex(*value), + Self::ListIndex(value) => AccessRef::ListIndex(*value), + } + } +} + +impl<'a> AccessRef<'a> { + pub(super) fn to_owned(self) -> Access { + match self { + Self::Field(value) => Access::Field(value.to_string().into_boxed_str()), + Self::FieldIndex(value) => Access::FieldIndex(value), + Self::TupleIndex(value) => Access::TupleIndex(value), + Self::ListIndex(value) => Access::ListIndex(value), + } + } + + fn display_value(&self) -> &dyn fmt::Display { + match self { + Self::Field(value) => value, + Self::FieldIndex(value) | Self::TupleIndex(value) | Self::ListIndex(value) => value, + } + } + fn kind(&self) -> &'static str { + match self { + Self::Field(_) => "field", + Self::FieldIndex(_) => "field index", + Self::TupleIndex(_) | Self::ListIndex(_) => "index", + } + } + + pub(super) fn element( + self, + base: &dyn Reflect, + offset: usize, + ) -> Result<&dyn Reflect, ReflectPathError<'a>> { + let ty = base.reflect_ref().into(); + self.element_inner(base) + .and_then(|maybe| maybe.ok_or(AccessError::Access { ty, access: self })) + .map_err(|err| err.with_offset(offset)) + } + fn element_inner(self, base: &dyn Reflect) -> InnerResult<&dyn Reflect> { + use ReflectRef::*; + match (self, base.reflect_ref()) { + (Self::Field(field), Struct(struct_ref)) => Ok(struct_ref.field(field)), + (Self::Field(field), Enum(enum_ref)) => match enum_ref.variant_type() { + VariantType::Struct => Ok(enum_ref.field(field)), + actual => Err(AccessError::bad_enum(Type::Struct, actual)), + }, + (Self::FieldIndex(index), Struct(struct_ref)) => Ok(struct_ref.field_at(index)), + (Self::FieldIndex(index), Enum(enum_ref)) => match enum_ref.variant_type() { + VariantType::Struct => Ok(enum_ref.field_at(index)), + actual => Err(AccessError::bad_enum(Type::Struct, actual)), + }, + (Self::TupleIndex(index), TupleStruct(tuple)) => Ok(tuple.field(index)), + (Self::TupleIndex(index), Tuple(tuple)) => Ok(tuple.field(index)), + (Self::TupleIndex(index), Enum(enum_ref)) => match enum_ref.variant_type() { + VariantType::Tuple => Ok(enum_ref.field_at(index)), + actual => Err(AccessError::bad_enum(Type::Tuple, actual)), + }, + (Self::ListIndex(index), List(list)) => Ok(list.get(index)), + (Self::ListIndex(index), Array(list)) => Ok(list.get(index)), + (Self::ListIndex(_), actual) => Err(AccessError::bad_type(Type::List, actual)), + (_, actual) => Err(AccessError::bad_type(Type::Struct, actual)), + } + } + + pub(super) fn element_mut( + self, + base: &mut dyn Reflect, + offset: usize, + ) -> Result<&mut dyn Reflect, ReflectPathError<'a>> { + let ty = base.reflect_ref().into(); + self.element_inner_mut(base) + .and_then(|maybe| maybe.ok_or(AccessError::Access { ty, access: self })) + .map_err(|err| err.with_offset(offset)) + } + fn element_inner_mut(self, base: &mut dyn Reflect) -> InnerResult<&mut dyn Reflect> { + use ReflectMut::*; + let base_kind: Type = base.reflect_ref().into(); + match (self, base.reflect_mut()) { + (Self::Field(field), Struct(struct_mut)) => Ok(struct_mut.field_mut(field)), + (Self::Field(field), Enum(enum_mut)) => match enum_mut.variant_type() { + VariantType::Struct => Ok(enum_mut.field_mut(field)), + actual => Err(AccessError::bad_enum(Type::Struct, actual)), + }, + (Self::FieldIndex(index), Struct(struct_mut)) => Ok(struct_mut.field_at_mut(index)), + (Self::FieldIndex(index), Enum(enum_mut)) => match enum_mut.variant_type() { + VariantType::Struct => Ok(enum_mut.field_at_mut(index)), + actual => Err(AccessError::bad_enum(Type::Struct, actual)), + }, + (Self::TupleIndex(index), TupleStruct(tuple)) => Ok(tuple.field_mut(index)), + (Self::TupleIndex(index), Tuple(tuple)) => Ok(tuple.field_mut(index)), + (Self::TupleIndex(index), Enum(enum_mut)) => match enum_mut.variant_type() { + VariantType::Tuple => Ok(enum_mut.field_at_mut(index)), + actual => Err(AccessError::bad_enum(Type::Tuple, actual)), + }, + (Self::ListIndex(index), List(list)) => Ok(list.get_mut(index)), + (Self::ListIndex(index), Array(list)) => Ok(list.get_mut(index)), + (Self::ListIndex(_), _) => Err(AccessError::bad_type(Type::List, base_kind)), + (_, _) => Err(AccessError::bad_type(Type::Struct, base_kind)), + } + } +} diff --git a/crates/bevy_reflect/src/path.rs b/crates/bevy_reflect/src/path/mod.rs similarity index 64% rename from crates/bevy_reflect/src/path.rs rename to crates/bevy_reflect/src/path/mod.rs index 2e0b08cf96917..b2d8a741f6dc6 100644 --- a/crates/bevy_reflect/src/path.rs +++ b/crates/bevy_reflect/src/path/mod.rs @@ -1,50 +1,44 @@ +mod access; + use std::fmt; use std::num::ParseIntError; -use crate::{Reflect, ReflectMut, ReflectRef, VariantType}; +use crate::Reflect; +pub use access::AccessError; +use access::{Access, AccessRef}; use thiserror::Error; -/// An error returned from a failed path string query. +type ParseResult = Result; + +/// A parse error for a path string query. #[derive(Debug, PartialEq, Eq, Error)] -pub enum ReflectPathError<'a> { - #[error("expected an identifier at index {index}")] - ExpectedIdent { index: usize }, - #[error("the current struct doesn't have a field with the name `{field}`")] - InvalidField { index: usize, field: &'a str }, - #[error("the current struct doesn't have a field at the given index")] - InvalidFieldIndex { index: usize, field_index: usize }, - #[error("the current tuple struct doesn't have a field with the index {tuple_struct_index}")] - InvalidTupleStructIndex { - index: usize, - tuple_struct_index: usize, - }, - #[error("the current tuple doesn't have a field with the index {tuple_index}")] - InvalidTupleIndex { index: usize, tuple_index: usize }, - #[error("the current struct variant doesn't have a field with the name `{field}`")] - InvalidStructVariantField { index: usize, field: &'a str }, - #[error("the current tuple variant doesn't have a field with the index {tuple_variant_index}")] - InvalidTupleVariantIndex { - index: usize, - tuple_variant_index: usize, - }, - #[error("the current list doesn't have a value at the index {list_index}")] - InvalidListIndex { index: usize, list_index: usize }, +pub enum ReflectPathParseError { + #[error("expected an identifier at offset {offset}")] + ExpectedIdent { offset: usize }, + #[error("encountered an unexpected token `{token}`")] - UnexpectedToken { index: usize, token: &'a str }, + UnexpectedToken { offset: usize, token: &'static str }, + #[error("expected token `{token}`, but it wasn't there.")] - ExpectedToken { index: usize, token: &'a str }, - #[error("expected a struct, but found a different reflect value")] - ExpectedStruct { index: usize }, - #[error("expected a list, but found a different reflect value")] - ExpectedList { index: usize }, - #[error("expected a struct variant, but found a different reflect value")] - ExpectedStructVariant { index: usize }, - #[error("expected a tuple variant, but found a different reflect value")] - ExpectedTupleVariant { index: usize }, + ExpectedToken { offset: usize, token: &'static str }, + #[error("failed to parse a usize")] IndexParseError(#[from] ParseIntError), +} +/// An error returned from a failed path string query. +#[derive(Debug, PartialEq, Eq, Error)] +pub enum ReflectPathError<'a> { + #[error("{error}")] + InvalidAccess { + /// Position in the path string. + offset: usize, + error: AccessError<'a>, + }, #[error("failed to downcast to the path result to the given type")] InvalidDowncast, + + #[error(transparent)] + Parse(#[from] ReflectPathParseError), } /// A trait which allows nested [`Reflect`] values to be retrieved with path strings. @@ -260,7 +254,7 @@ impl GetPath for dyn Reflect { ) -> Result<&'r dyn Reflect, ReflectPathError<'p>> { let mut current: &dyn Reflect = self; for (access, current_index) in PathParser::new(path) { - current = access?.read_element(current, current_index)?; + current = access?.element(current, current_index)?; } Ok(current) } @@ -271,7 +265,7 @@ impl GetPath for dyn Reflect { ) -> Result<&'r mut dyn Reflect, ReflectPathError<'p>> { let mut current: &mut dyn Reflect = self; for (access, current_index) in PathParser::new(path) { - current = access?.read_element_mut(current, current_index)?; + current = access?.element_mut(current, current_index)?; } Ok(current) } @@ -360,7 +354,7 @@ impl ParsedPath { ) -> Result<&'r dyn Reflect, ReflectPathError<'p>> { let mut current = root; for (access, current_index) in self.0.iter() { - current = access.to_ref().read_element(current, *current_index)?; + current = access.as_ref().element(current, *current_index)?; } Ok(current) } @@ -376,7 +370,7 @@ impl ParsedPath { ) -> Result<&'r mut dyn Reflect, ReflectPathError<'p>> { let mut current = root; for (access, current_index) in self.0.iter() { - current = access.to_ref().read_element_mut(current, *current_index)?; + current = access.as_ref().element_mut(current, *current_index)?; } Ok(current) } @@ -412,29 +406,6 @@ impl ParsedPath { } } -impl fmt::Display for Access { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Access::Field(field) => { - Token::DOT.fmt(f)?; - f.write_str(field.as_ref()) - } - Access::FieldIndex(index) => { - Token::CROSSHATCH.fmt(f)?; - index.fmt(f) - } - Access::TupleIndex(index) => { - Token::DOT.fmt(f)?; - index.fmt(f) - } - Access::ListIndex(index) => { - Token::OPEN_BRACKET.fmt(f)?; - index.fmt(f)?; - Token::CLOSE_BRACKET.fmt(f) - } - } - } -} impl fmt::Display for ParsedPath { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { for (access, _) in self.0.iter() { @@ -444,243 +415,6 @@ impl fmt::Display for ParsedPath { } } -/// A singular owned element access within a path. -/// -/// Can be applied to a `dyn Reflect` to get a reference to the targeted element. -/// -/// A path is composed of multiple accesses in sequence. -#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] -enum Access { - Field(Box), - FieldIndex(usize), - TupleIndex(usize), - ListIndex(usize), -} - -impl Access { - fn to_ref(&self) -> AccessRef<'_> { - match self { - Self::Field(value) => AccessRef::Field(value), - Self::FieldIndex(value) => AccessRef::FieldIndex(*value), - Self::TupleIndex(value) => AccessRef::TupleIndex(*value), - Self::ListIndex(value) => AccessRef::ListIndex(*value), - } - } -} - -/// A singular borrowed element access within a path. -/// -/// Can be applied to a `dyn Reflect` to get a reference to the targeted element. -/// -/// Does not own the backing store it's sourced from. -/// For an owned version, you can convert one to an [`Access`]. -#[derive(Debug)] -enum AccessRef<'a> { - Field(&'a str), - FieldIndex(usize), - TupleIndex(usize), - ListIndex(usize), -} - -impl<'a> AccessRef<'a> { - fn to_owned(&self) -> Access { - match self { - Self::Field(value) => Access::Field(value.to_string().into_boxed_str()), - Self::FieldIndex(value) => Access::FieldIndex(*value), - Self::TupleIndex(value) => Access::TupleIndex(*value), - Self::ListIndex(value) => Access::ListIndex(*value), - } - } - - fn read_element<'r>( - &self, - current: &'r dyn Reflect, - current_index: usize, - ) -> Result<&'r dyn Reflect, ReflectPathError<'a>> { - match (self, current.reflect_ref()) { - (Self::Field(field), ReflectRef::Struct(reflect_struct)) => reflect_struct - .field(field) - .ok_or(ReflectPathError::InvalidField { - index: current_index, - field, - }), - (Self::FieldIndex(field_index), ReflectRef::Struct(reflect_struct)) => reflect_struct - .field_at(*field_index) - .ok_or(ReflectPathError::InvalidFieldIndex { - index: current_index, - field_index: *field_index, - }), - (Self::TupleIndex(tuple_index), ReflectRef::TupleStruct(reflect_struct)) => { - reflect_struct.field(*tuple_index).ok_or( - ReflectPathError::InvalidTupleStructIndex { - index: current_index, - tuple_struct_index: *tuple_index, - }, - ) - } - (Self::TupleIndex(tuple_index), ReflectRef::Tuple(reflect_tuple)) => reflect_tuple - .field(*tuple_index) - .ok_or(ReflectPathError::InvalidTupleIndex { - index: current_index, - tuple_index: *tuple_index, - }), - (Self::ListIndex(list_index), ReflectRef::List(reflect_list)) => reflect_list - .get(*list_index) - .ok_or(ReflectPathError::InvalidListIndex { - index: current_index, - list_index: *list_index, - }), - (Self::ListIndex(list_index), ReflectRef::Array(reflect_list)) => reflect_list - .get(*list_index) - .ok_or(ReflectPathError::InvalidListIndex { - index: current_index, - list_index: *list_index, - }), - (Self::ListIndex(_), _) => Err(ReflectPathError::ExpectedList { - index: current_index, - }), - (Self::Field(field), ReflectRef::Enum(reflect_enum)) => { - match reflect_enum.variant_type() { - VariantType::Struct => { - reflect_enum - .field(field) - .ok_or(ReflectPathError::InvalidField { - index: current_index, - field, - }) - } - _ => Err(ReflectPathError::ExpectedStructVariant { - index: current_index, - }), - } - } - (Self::FieldIndex(field_index), ReflectRef::Enum(reflect_enum)) => { - match reflect_enum.variant_type() { - VariantType::Struct => reflect_enum.field_at(*field_index).ok_or( - ReflectPathError::InvalidFieldIndex { - index: current_index, - field_index: *field_index, - }, - ), - _ => Err(ReflectPathError::ExpectedStructVariant { - index: current_index, - }), - } - } - (Self::TupleIndex(tuple_variant_index), ReflectRef::Enum(reflect_enum)) => { - match reflect_enum.variant_type() { - VariantType::Tuple => reflect_enum.field_at(*tuple_variant_index).ok_or( - ReflectPathError::InvalidTupleVariantIndex { - index: current_index, - tuple_variant_index: *tuple_variant_index, - }, - ), - _ => Err(ReflectPathError::ExpectedTupleVariant { - index: current_index, - }), - } - } - _ => Err(ReflectPathError::ExpectedStruct { - index: current_index, - }), - } - } - - fn read_element_mut<'r>( - &self, - current: &'r mut dyn Reflect, - current_index: usize, - ) -> Result<&'r mut dyn Reflect, ReflectPathError<'a>> { - match (self, current.reflect_mut()) { - (Self::Field(field), ReflectMut::Struct(reflect_struct)) => reflect_struct - .field_mut(field) - .ok_or(ReflectPathError::InvalidField { - index: current_index, - field, - }), - (Self::FieldIndex(field_index), ReflectMut::Struct(reflect_struct)) => reflect_struct - .field_at_mut(*field_index) - .ok_or(ReflectPathError::InvalidFieldIndex { - index: current_index, - field_index: *field_index, - }), - (Self::TupleIndex(tuple_index), ReflectMut::TupleStruct(reflect_struct)) => { - reflect_struct.field_mut(*tuple_index).ok_or( - ReflectPathError::InvalidTupleStructIndex { - index: current_index, - tuple_struct_index: *tuple_index, - }, - ) - } - (Self::TupleIndex(tuple_index), ReflectMut::Tuple(reflect_tuple)) => reflect_tuple - .field_mut(*tuple_index) - .ok_or(ReflectPathError::InvalidTupleIndex { - index: current_index, - tuple_index: *tuple_index, - }), - (Self::ListIndex(list_index), ReflectMut::List(reflect_list)) => reflect_list - .get_mut(*list_index) - .ok_or(ReflectPathError::InvalidListIndex { - index: current_index, - list_index: *list_index, - }), - (Self::ListIndex(list_index), ReflectMut::Array(reflect_list)) => reflect_list - .get_mut(*list_index) - .ok_or(ReflectPathError::InvalidListIndex { - index: current_index, - list_index: *list_index, - }), - (Self::ListIndex(_), _) => Err(ReflectPathError::ExpectedList { - index: current_index, - }), - (Self::Field(field), ReflectMut::Enum(reflect_enum)) => { - match reflect_enum.variant_type() { - VariantType::Struct => { - reflect_enum - .field_mut(field) - .ok_or(ReflectPathError::InvalidField { - index: current_index, - field, - }) - } - _ => Err(ReflectPathError::ExpectedStructVariant { - index: current_index, - }), - } - } - (Self::FieldIndex(field_index), ReflectMut::Enum(reflect_enum)) => { - match reflect_enum.variant_type() { - VariantType::Struct => reflect_enum.field_at_mut(*field_index).ok_or( - ReflectPathError::InvalidFieldIndex { - index: current_index, - field_index: *field_index, - }, - ), - _ => Err(ReflectPathError::ExpectedStructVariant { - index: current_index, - }), - } - } - (Self::TupleIndex(tuple_variant_index), ReflectMut::Enum(reflect_enum)) => { - match reflect_enum.variant_type() { - VariantType::Tuple => reflect_enum.field_at_mut(*tuple_variant_index).ok_or( - ReflectPathError::InvalidTupleVariantIndex { - index: current_index, - tuple_variant_index: *tuple_variant_index, - }, - ), - _ => Err(ReflectPathError::ExpectedTupleVariant { - index: current_index, - }), - } - } - _ => Err(ReflectPathError::ExpectedStruct { - index: current_index, - }), - } - } -} - struct PathParser<'a> { path: &'a str, index: usize, @@ -732,8 +466,8 @@ impl<'a> PathParser<'a> { Some(ident) } - fn token_to_access(&mut self, token: Token<'a>) -> Result, ReflectPathError<'a>> { - let current_index = self.index; + fn token_to_access(&mut self, token: Token<'a>) -> ParseResult> { + let current_offset = self.index; match token { Token::Dot => { if let Some(Token::Ident(value)) = self.next_token() { @@ -742,8 +476,8 @@ impl<'a> PathParser<'a> { .map(AccessRef::TupleIndex) .or(Ok(AccessRef::Field(value))) } else { - Err(ReflectPathError::ExpectedIdent { - index: current_index, + Err(ReflectPathParseError::ExpectedIdent { + offset: current_offset, }) } } @@ -751,8 +485,8 @@ impl<'a> PathParser<'a> { if let Some(Token::Ident(value)) = self.next_token() { Ok(AccessRef::FieldIndex(value.parse::()?)) } else { - Err(ReflectPathError::ExpectedIdent { - index: current_index, + Err(ReflectPathParseError::ExpectedIdent { + offset: current_offset, }) } } @@ -760,22 +494,22 @@ impl<'a> PathParser<'a> { let access = if let Some(Token::Ident(value)) = self.next_token() { AccessRef::ListIndex(value.parse::()?) } else { - return Err(ReflectPathError::ExpectedIdent { - index: current_index, + return Err(ReflectPathParseError::ExpectedIdent { + offset: current_offset, }); }; if !matches!(self.next_token(), Some(Token::CloseBracket)) { - return Err(ReflectPathError::ExpectedToken { - index: current_index, + return Err(ReflectPathParseError::ExpectedToken { + offset: current_offset, token: Token::OPEN_BRACKET_STR, }); } Ok(access) } - Token::CloseBracket => Err(ReflectPathError::UnexpectedToken { - index: current_index, + Token::CloseBracket => Err(ReflectPathParseError::UnexpectedToken { + offset: current_offset, token: Token::CLOSE_BRACKET_STR, }), Token::Ident(value) => value @@ -787,7 +521,7 @@ impl<'a> PathParser<'a> { } impl<'a> Iterator for PathParser<'a> { - type Item = (Result, ReflectPathError<'a>>, usize); + type Item = (ParseResult>, usize); fn next(&mut self) -> Option { let token = self.next_token()?; @@ -819,6 +553,7 @@ mod tests { use super::*; use crate as bevy_reflect; use crate::*; + use access::Type; #[derive(Reflect)] struct A { @@ -1034,35 +769,58 @@ mod tests { assert_eq!( a.reflect_path("x.notreal").err().unwrap(), - ReflectPathError::InvalidField { - index: 2, - field: "notreal" + ReflectPathError::InvalidAccess { + offset: 2, + error: AccessError::Access { + ty: Type::Struct, + access: AccessRef::Field("notreal"), + }, } ); assert_eq!( a.reflect_path("unit_variant.0").err().unwrap(), - ReflectPathError::ExpectedTupleVariant { index: 13 } + ReflectPathError::InvalidAccess { + offset: 13, + error: AccessError::Enum { + actual: Type::Unit, + expected: Type::Tuple + }, + } ); assert_eq!( a.reflect_path("x..").err().unwrap(), - ReflectPathError::ExpectedIdent { index: 2 } + ReflectPathError::Parse(ReflectPathParseError::ExpectedIdent { offset: 2 }) ); assert_eq!( a.reflect_path("x[0]").err().unwrap(), - ReflectPathError::ExpectedList { index: 2 } + ReflectPathError::InvalidAccess { + offset: 2, + error: AccessError::Type { + actual: Type::Struct, + expected: Type::List + }, + } ); assert_eq!( a.reflect_path("y.x").err().unwrap(), - ReflectPathError::ExpectedStruct { index: 2 } + ReflectPathError::InvalidAccess { + offset: 2, + error: AccessError::Type { + actual: Type::List, + expected: Type::Struct + }, + } ); assert!(matches!( a.reflect_path("y[badindex]"), - Err(ReflectPathError::IndexParseError(_)) + Err(ReflectPathError::Parse( + ReflectPathParseError::IndexParseError(_) + )) )); } From d7efd76d1bc48688ef6a59f6932c2f173816a958 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Mon, 19 Jun 2023 18:10:46 +0200 Subject: [PATCH 06/11] Do not make access error public --- crates/bevy_reflect/src/path/access.rs | 40 +++++++++++++------------- crates/bevy_reflect/src/path/mod.rs | 22 ++++++++------ 2 files changed, 33 insertions(+), 29 deletions(-) diff --git a/crates/bevy_reflect/src/path/access.rs b/crates/bevy_reflect/src/path/access.rs index cc62971190966..eb1da25b362ea 100644 --- a/crates/bevy_reflect/src/path/access.rs +++ b/crates/bevy_reflect/src/path/access.rs @@ -4,10 +4,10 @@ use super::ReflectPathError; use crate::{Reflect, ReflectMut, ReflectRef, VariantType}; use thiserror::Error; -type InnerResult = Result, AccessError<'static>>; +type InnerResult = Result, Error<'static>>; #[derive(Debug, PartialEq, Eq, Error)] -pub enum AccessError<'a> { +pub(super) enum Error<'a> { #[error( "the current {ty} doesn't have the {} {}", access.kind(), @@ -22,20 +22,20 @@ pub enum AccessError<'a> { Enum { expected: Type, actual: Type }, } -impl<'a> AccessError<'a> { +impl<'a> Error<'a> { fn with_offset(self, offset: usize) -> ReflectPathError<'a> { - let error = self; + let error = super::AccessError(self); ReflectPathError::InvalidAccess { offset, error } } } -impl AccessError<'static> { +impl Error<'static> { fn bad_enum(expected: Type, actual: impl Into) -> Self { let actual = actual.into(); - AccessError::Enum { expected, actual } + Error::Enum { expected, actual } } fn bad_type(expected: Type, actual: impl Into) -> Self { let actual = actual.into(); - AccessError::Type { expected, actual } + Error::Type { expected, actual } } } @@ -123,7 +123,7 @@ impl fmt::Display for Access { /// Does not own the backing store it's sourced from. /// For an owned version, you can convert one to an [`Access`] with [`AccessRef::to_owned`]. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum AccessRef<'a> { +pub(super) enum AccessRef<'a> { Field(&'a str), FieldIndex(usize), TupleIndex(usize), @@ -172,7 +172,7 @@ impl<'a> AccessRef<'a> { ) -> Result<&dyn Reflect, ReflectPathError<'a>> { let ty = base.reflect_ref().into(); self.element_inner(base) - .and_then(|maybe| maybe.ok_or(AccessError::Access { ty, access: self })) + .and_then(|maybe| maybe.ok_or(Error::Access { ty, access: self })) .map_err(|err| err.with_offset(offset)) } fn element_inner(self, base: &dyn Reflect) -> InnerResult<&dyn Reflect> { @@ -181,23 +181,23 @@ impl<'a> AccessRef<'a> { (Self::Field(field), Struct(struct_ref)) => Ok(struct_ref.field(field)), (Self::Field(field), Enum(enum_ref)) => match enum_ref.variant_type() { VariantType::Struct => Ok(enum_ref.field(field)), - actual => Err(AccessError::bad_enum(Type::Struct, actual)), + actual => Err(Error::bad_enum(Type::Struct, actual)), }, (Self::FieldIndex(index), Struct(struct_ref)) => Ok(struct_ref.field_at(index)), (Self::FieldIndex(index), Enum(enum_ref)) => match enum_ref.variant_type() { VariantType::Struct => Ok(enum_ref.field_at(index)), - actual => Err(AccessError::bad_enum(Type::Struct, actual)), + actual => Err(Error::bad_enum(Type::Struct, actual)), }, (Self::TupleIndex(index), TupleStruct(tuple)) => Ok(tuple.field(index)), (Self::TupleIndex(index), Tuple(tuple)) => Ok(tuple.field(index)), (Self::TupleIndex(index), Enum(enum_ref)) => match enum_ref.variant_type() { VariantType::Tuple => Ok(enum_ref.field_at(index)), - actual => Err(AccessError::bad_enum(Type::Tuple, actual)), + actual => Err(Error::bad_enum(Type::Tuple, actual)), }, (Self::ListIndex(index), List(list)) => Ok(list.get(index)), (Self::ListIndex(index), Array(list)) => Ok(list.get(index)), - (Self::ListIndex(_), actual) => Err(AccessError::bad_type(Type::List, actual)), - (_, actual) => Err(AccessError::bad_type(Type::Struct, actual)), + (Self::ListIndex(_), actual) => Err(Error::bad_type(Type::List, actual)), + (_, actual) => Err(Error::bad_type(Type::Struct, actual)), } } @@ -208,7 +208,7 @@ impl<'a> AccessRef<'a> { ) -> Result<&mut dyn Reflect, ReflectPathError<'a>> { let ty = base.reflect_ref().into(); self.element_inner_mut(base) - .and_then(|maybe| maybe.ok_or(AccessError::Access { ty, access: self })) + .and_then(|maybe| maybe.ok_or(Error::Access { ty, access: self })) .map_err(|err| err.with_offset(offset)) } fn element_inner_mut(self, base: &mut dyn Reflect) -> InnerResult<&mut dyn Reflect> { @@ -218,23 +218,23 @@ impl<'a> AccessRef<'a> { (Self::Field(field), Struct(struct_mut)) => Ok(struct_mut.field_mut(field)), (Self::Field(field), Enum(enum_mut)) => match enum_mut.variant_type() { VariantType::Struct => Ok(enum_mut.field_mut(field)), - actual => Err(AccessError::bad_enum(Type::Struct, actual)), + actual => Err(Error::bad_enum(Type::Struct, actual)), }, (Self::FieldIndex(index), Struct(struct_mut)) => Ok(struct_mut.field_at_mut(index)), (Self::FieldIndex(index), Enum(enum_mut)) => match enum_mut.variant_type() { VariantType::Struct => Ok(enum_mut.field_at_mut(index)), - actual => Err(AccessError::bad_enum(Type::Struct, actual)), + actual => Err(Error::bad_enum(Type::Struct, actual)), }, (Self::TupleIndex(index), TupleStruct(tuple)) => Ok(tuple.field_mut(index)), (Self::TupleIndex(index), Tuple(tuple)) => Ok(tuple.field_mut(index)), (Self::TupleIndex(index), Enum(enum_mut)) => match enum_mut.variant_type() { VariantType::Tuple => Ok(enum_mut.field_at_mut(index)), - actual => Err(AccessError::bad_enum(Type::Tuple, actual)), + actual => Err(Error::bad_enum(Type::Tuple, actual)), }, (Self::ListIndex(index), List(list)) => Ok(list.get_mut(index)), (Self::ListIndex(index), Array(list)) => Ok(list.get_mut(index)), - (Self::ListIndex(_), _) => Err(AccessError::bad_type(Type::List, base_kind)), - (_, _) => Err(AccessError::bad_type(Type::Struct, base_kind)), + (Self::ListIndex(_), _) => Err(Error::bad_type(Type::List, base_kind)), + (_, _) => Err(Error::bad_type(Type::Struct, base_kind)), } } } diff --git a/crates/bevy_reflect/src/path/mod.rs b/crates/bevy_reflect/src/path/mod.rs index b2d8a741f6dc6..25b0f73a9e7d6 100644 --- a/crates/bevy_reflect/src/path/mod.rs +++ b/crates/bevy_reflect/src/path/mod.rs @@ -4,12 +4,16 @@ use std::fmt; use std::num::ParseIntError; use crate::Reflect; -pub use access::AccessError; use access::{Access, AccessRef}; use thiserror::Error; type ParseResult = Result; +/// An error specific to accessing a field/index on a `Reflect`. +#[derive(Debug, PartialEq, Eq, Error)] +#[error(transparent)] +pub struct AccessError<'a>(access::Error<'a>); + /// A parse error for a path string query. #[derive(Debug, PartialEq, Eq, Error)] pub enum ReflectPathParseError { @@ -771,10 +775,10 @@ mod tests { a.reflect_path("x.notreal").err().unwrap(), ReflectPathError::InvalidAccess { offset: 2, - error: AccessError::Access { + error: AccessError(access::Error::Access { ty: Type::Struct, access: AccessRef::Field("notreal"), - }, + }), } ); @@ -782,10 +786,10 @@ mod tests { a.reflect_path("unit_variant.0").err().unwrap(), ReflectPathError::InvalidAccess { offset: 13, - error: AccessError::Enum { + error: AccessError(access::Error::Enum { actual: Type::Unit, expected: Type::Tuple - }, + }), } ); @@ -798,10 +802,10 @@ mod tests { a.reflect_path("x[0]").err().unwrap(), ReflectPathError::InvalidAccess { offset: 2, - error: AccessError::Type { + error: AccessError(access::Error::Type { actual: Type::Struct, expected: Type::List - }, + }), } ); @@ -809,10 +813,10 @@ mod tests { a.reflect_path("y.x").err().unwrap(), ReflectPathError::InvalidAccess { offset: 2, - error: AccessError::Type { + error: AccessError(access::Error::Type { actual: Type::List, expected: Type::Struct - }, + }), } ); From eeca51853af231073be38ee297e6121dbbdbc080 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Mon, 19 Jun 2023 18:11:51 +0200 Subject: [PATCH 07/11] Don't forget to make 'Type' private --- crates/bevy_reflect/src/path/access.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_reflect/src/path/access.rs b/crates/bevy_reflect/src/path/access.rs index eb1da25b362ea..1639cf2bfd70d 100644 --- a/crates/bevy_reflect/src/path/access.rs +++ b/crates/bevy_reflect/src/path/access.rs @@ -40,7 +40,7 @@ impl Error<'static> { } #[derive(Debug, Copy, Clone, PartialEq, Eq)] -pub enum Type { +pub(super) enum Type { Struct, TupleStruct, Tuple, From dba74e173f629b1826551a4863ccae2d9301f139 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Mon, 19 Jun 2023 18:13:39 +0200 Subject: [PATCH 08/11] More space between error types Co-authored-by: Alice Cecile --- crates/bevy_reflect/src/path/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/bevy_reflect/src/path/mod.rs b/crates/bevy_reflect/src/path/mod.rs index 25b0f73a9e7d6..0b36ec12efca4 100644 --- a/crates/bevy_reflect/src/path/mod.rs +++ b/crates/bevy_reflect/src/path/mod.rs @@ -29,6 +29,7 @@ pub enum ReflectPathParseError { #[error("failed to parse a usize")] IndexParseError(#[from] ParseIntError), } + /// An error returned from a failed path string query. #[derive(Debug, PartialEq, Eq, Error)] pub enum ReflectPathError<'a> { From b10006974f05554d9c0e340cda5c20c09c090327 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Wed, 5 Jul 2023 09:36:13 +0200 Subject: [PATCH 09/11] Implement MrGVSV suggestions (round 1) Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com> --- crates/bevy_reflect/src/lib.rs | 4 +- crates/bevy_reflect/src/path/access.rs | 117 ++++++++++++++----------- crates/bevy_reflect/src/path/mod.rs | 18 ++-- 3 files changed, 74 insertions(+), 65 deletions(-) diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs index 49a8781f29e20..bb0a012888d6e 100644 --- a/crates/bevy_reflect/src/lib.rs +++ b/crates/bevy_reflect/src/lib.rs @@ -233,7 +233,7 @@ //! //! The [`GetPath`] trait allows accessing arbitrary nested fields of a [`Reflect`] type. //! -//! Using [`GetPath`], it is possible to use a path strings to access a specific field +//! Using `GetPath`, it is possible to use a path string to access a specific field //! of a reflected type. //! //! ``` @@ -252,8 +252,6 @@ //! ); //! ``` //! -//! Check the [`GetPath`] documentation for more details. -//! //! # Type Registration //! //! This crate also comes with a [`TypeRegistry`] that can be used to store and retrieve additional type metadata at runtime, diff --git a/crates/bevy_reflect/src/path/access.rs b/crates/bevy_reflect/src/path/access.rs index 1639cf2bfd70d..05b8dd625c9ca 100644 --- a/crates/bevy_reflect/src/path/access.rs +++ b/crates/bevy_reflect/src/path/access.rs @@ -1,6 +1,6 @@ use std::fmt; -use super::ReflectPathError; +use super::{AccessError, ReflectPathError}; use crate::{Reflect, ReflectMut, ReflectRef, VariantType}; use thiserror::Error; @@ -13,34 +13,43 @@ pub(super) enum Error<'a> { access.kind(), access.display_value(), )] - Access { ty: Type, access: AccessRef<'a> }, + Access { + ty: TypeShape, + access: AccessRef<'a>, + }, #[error("invalid type shape: expected {expected} but found a reflect {actual}")] - Type { expected: Type, actual: Type }, + Type { + expected: TypeShape, + actual: TypeShape, + }, #[error("invalid enum access: expected {expected} variant but found {actual} variant")] - Enum { expected: Type, actual: Type }, + Enum { + expected: TypeShape, + actual: TypeShape, + }, } impl<'a> Error<'a> { fn with_offset(self, offset: usize) -> ReflectPathError<'a> { - let error = super::AccessError(self); + let error = AccessError(self); ReflectPathError::InvalidAccess { offset, error } } } impl Error<'static> { - fn bad_enum(expected: Type, actual: impl Into) -> Self { + fn bad_enum_variant(expected: TypeShape, actual: impl Into) -> Self { let actual = actual.into(); Error::Enum { expected, actual } } - fn bad_type(expected: Type, actual: impl Into) -> Self { + fn bad_type(expected: TypeShape, actual: impl Into) -> Self { let actual = actual.into(); Error::Type { expected, actual } } } #[derive(Debug, Copy, Clone, PartialEq, Eq)] -pub(super) enum Type { +pub(super) enum TypeShape { Struct, TupleStruct, Tuple, @@ -52,42 +61,42 @@ pub(super) enum Type { Unit, } -impl fmt::Display for Type { +impl fmt::Display for TypeShape { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let name = match self { - Type::Struct => "struct", - Type::TupleStruct => "tuple struct", - Type::Tuple => "tuple", - Type::List => "list", - Type::Array => "array", - Type::Map => "map", - Type::Enum => "enum", - Type::Value => "value", - Type::Unit => "unit", + TypeShape::Struct => "struct", + TypeShape::TupleStruct => "tuple struct", + TypeShape::Tuple => "tuple", + TypeShape::List => "list", + TypeShape::Array => "array", + TypeShape::Map => "map", + TypeShape::Enum => "enum", + TypeShape::Value => "value", + TypeShape::Unit => "unit", }; write!(f, "{name}") } } -impl<'a> From> for Type { +impl<'a> From> for TypeShape { fn from(value: ReflectRef<'a>) -> Self { match value { - ReflectRef::Struct(_) => Type::Struct, - ReflectRef::TupleStruct(_) => Type::TupleStruct, - ReflectRef::Tuple(_) => Type::Tuple, - ReflectRef::List(_) => Type::List, - ReflectRef::Array(_) => Type::Array, - ReflectRef::Map(_) => Type::Map, - ReflectRef::Enum(_) => Type::Enum, - ReflectRef::Value(_) => Type::Value, + ReflectRef::Struct(_) => TypeShape::Struct, + ReflectRef::TupleStruct(_) => TypeShape::TupleStruct, + ReflectRef::Tuple(_) => TypeShape::Tuple, + ReflectRef::List(_) => TypeShape::List, + ReflectRef::Array(_) => TypeShape::Array, + ReflectRef::Map(_) => TypeShape::Map, + ReflectRef::Enum(_) => TypeShape::Enum, + ReflectRef::Value(_) => TypeShape::Value, } } } -impl From for Type { +impl From for TypeShape { fn from(value: VariantType) -> Self { match value { - VariantType::Struct => Type::Struct, - VariantType::Tuple => Type::Tuple, - VariantType::Unit => Type::Unit, + VariantType::Struct => TypeShape::Struct, + VariantType::Tuple => TypeShape::Tuple, + VariantType::Unit => TypeShape::Unit, } } } @@ -116,6 +125,17 @@ impl fmt::Display for Access { } } +impl Access { + pub(super) fn as_ref(&self) -> AccessRef<'_> { + match self { + Self::Field(value) => AccessRef::Field(value), + Self::FieldIndex(value) => AccessRef::FieldIndex(*value), + Self::TupleIndex(value) => AccessRef::TupleIndex(*value), + Self::ListIndex(value) => AccessRef::ListIndex(*value), + } + } +} + /// A singular borrowed element access within a path. /// /// Can be applied to a `dyn Reflect` to get a reference to the targeted element. @@ -130,17 +150,6 @@ pub(super) enum AccessRef<'a> { ListIndex(usize), } -impl Access { - pub(super) fn as_ref(&self) -> AccessRef<'_> { - match self { - Self::Field(value) => AccessRef::Field(value), - Self::FieldIndex(value) => AccessRef::FieldIndex(*value), - Self::TupleIndex(value) => AccessRef::TupleIndex(*value), - Self::ListIndex(value) => AccessRef::ListIndex(*value), - } - } -} - impl<'a> AccessRef<'a> { pub(super) fn to_owned(self) -> Access { match self { @@ -175,29 +184,30 @@ impl<'a> AccessRef<'a> { .and_then(|maybe| maybe.ok_or(Error::Access { ty, access: self })) .map_err(|err| err.with_offset(offset)) } + fn element_inner(self, base: &dyn Reflect) -> InnerResult<&dyn Reflect> { use ReflectRef::*; match (self, base.reflect_ref()) { (Self::Field(field), Struct(struct_ref)) => Ok(struct_ref.field(field)), (Self::Field(field), Enum(enum_ref)) => match enum_ref.variant_type() { VariantType::Struct => Ok(enum_ref.field(field)), - actual => Err(Error::bad_enum(Type::Struct, actual)), + actual => Err(Error::bad_enum_variant(TypeShape::Struct, actual)), }, (Self::FieldIndex(index), Struct(struct_ref)) => Ok(struct_ref.field_at(index)), (Self::FieldIndex(index), Enum(enum_ref)) => match enum_ref.variant_type() { VariantType::Struct => Ok(enum_ref.field_at(index)), - actual => Err(Error::bad_enum(Type::Struct, actual)), + actual => Err(Error::bad_enum_variant(TypeShape::Struct, actual)), }, (Self::TupleIndex(index), TupleStruct(tuple)) => Ok(tuple.field(index)), (Self::TupleIndex(index), Tuple(tuple)) => Ok(tuple.field(index)), (Self::TupleIndex(index), Enum(enum_ref)) => match enum_ref.variant_type() { VariantType::Tuple => Ok(enum_ref.field_at(index)), - actual => Err(Error::bad_enum(Type::Tuple, actual)), + actual => Err(Error::bad_enum_variant(TypeShape::Tuple, actual)), }, (Self::ListIndex(index), List(list)) => Ok(list.get(index)), (Self::ListIndex(index), Array(list)) => Ok(list.get(index)), - (Self::ListIndex(_), actual) => Err(Error::bad_type(Type::List, actual)), - (_, actual) => Err(Error::bad_type(Type::Struct, actual)), + (Self::ListIndex(_), actual) => Err(Error::bad_type(TypeShape::List, actual)), + (_, actual) => Err(Error::bad_type(TypeShape::Struct, actual)), } } @@ -211,30 +221,31 @@ impl<'a> AccessRef<'a> { .and_then(|maybe| maybe.ok_or(Error::Access { ty, access: self })) .map_err(|err| err.with_offset(offset)) } + fn element_inner_mut(self, base: &mut dyn Reflect) -> InnerResult<&mut dyn Reflect> { use ReflectMut::*; - let base_kind: Type = base.reflect_ref().into(); + let base_kind: TypeShape = base.reflect_ref().into(); match (self, base.reflect_mut()) { (Self::Field(field), Struct(struct_mut)) => Ok(struct_mut.field_mut(field)), (Self::Field(field), Enum(enum_mut)) => match enum_mut.variant_type() { VariantType::Struct => Ok(enum_mut.field_mut(field)), - actual => Err(Error::bad_enum(Type::Struct, actual)), + actual => Err(Error::bad_enum_variant(TypeShape::Struct, actual)), }, (Self::FieldIndex(index), Struct(struct_mut)) => Ok(struct_mut.field_at_mut(index)), (Self::FieldIndex(index), Enum(enum_mut)) => match enum_mut.variant_type() { VariantType::Struct => Ok(enum_mut.field_at_mut(index)), - actual => Err(Error::bad_enum(Type::Struct, actual)), + actual => Err(Error::bad_enum_variant(TypeShape::Struct, actual)), }, (Self::TupleIndex(index), TupleStruct(tuple)) => Ok(tuple.field_mut(index)), (Self::TupleIndex(index), Tuple(tuple)) => Ok(tuple.field_mut(index)), (Self::TupleIndex(index), Enum(enum_mut)) => match enum_mut.variant_type() { VariantType::Tuple => Ok(enum_mut.field_at_mut(index)), - actual => Err(Error::bad_enum(Type::Tuple, actual)), + actual => Err(Error::bad_enum_variant(TypeShape::Tuple, actual)), }, (Self::ListIndex(index), List(list)) => Ok(list.get_mut(index)), (Self::ListIndex(index), Array(list)) => Ok(list.get_mut(index)), - (Self::ListIndex(_), _) => Err(Error::bad_type(Type::List, base_kind)), - (_, _) => Err(Error::bad_type(Type::Struct, base_kind)), + (Self::ListIndex(_), _) => Err(Error::bad_type(TypeShape::List, base_kind)), + (_, _) => Err(Error::bad_type(TypeShape::Struct, base_kind)), } } } diff --git a/crates/bevy_reflect/src/path/mod.rs b/crates/bevy_reflect/src/path/mod.rs index 0b36ec12efca4..41e1c3d04f4d8 100644 --- a/crates/bevy_reflect/src/path/mod.rs +++ b/crates/bevy_reflect/src/path/mod.rs @@ -14,7 +14,7 @@ type ParseResult = Result; #[error(transparent)] pub struct AccessError<'a>(access::Error<'a>); -/// A parse error for a path string query. +/// A parse error for a path string. #[derive(Debug, PartialEq, Eq, Error)] pub enum ReflectPathParseError { #[error("expected an identifier at offset {offset}")] @@ -558,7 +558,7 @@ mod tests { use super::*; use crate as bevy_reflect; use crate::*; - use access::Type; + use access::TypeShape; #[derive(Reflect)] struct A { @@ -777,7 +777,7 @@ mod tests { ReflectPathError::InvalidAccess { offset: 2, error: AccessError(access::Error::Access { - ty: Type::Struct, + ty: TypeShape::Struct, access: AccessRef::Field("notreal"), }), } @@ -788,8 +788,8 @@ mod tests { ReflectPathError::InvalidAccess { offset: 13, error: AccessError(access::Error::Enum { - actual: Type::Unit, - expected: Type::Tuple + actual: TypeShape::Unit, + expected: TypeShape::Tuple }), } ); @@ -804,8 +804,8 @@ mod tests { ReflectPathError::InvalidAccess { offset: 2, error: AccessError(access::Error::Type { - actual: Type::Struct, - expected: Type::List + actual: TypeShape::Struct, + expected: TypeShape::List }), } ); @@ -815,8 +815,8 @@ mod tests { ReflectPathError::InvalidAccess { offset: 2, error: AccessError(access::Error::Type { - actual: Type::List, - expected: Type::Struct + actual: TypeShape::List, + expected: TypeShape::Struct }), } ); From 9651fc16f8954284d7f8e24f372a1c6ce4fcb7f2 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Wed, 5 Jul 2023 10:54:55 +0200 Subject: [PATCH 10/11] Merge Access and AccessRef using a Cow mooooo --- crates/bevy_reflect/src/path/access.rs | 122 ++++++++++--------------- crates/bevy_reflect/src/path/mod.rs | 40 +++++--- 2 files changed, 73 insertions(+), 89 deletions(-) diff --git a/crates/bevy_reflect/src/path/access.rs b/crates/bevy_reflect/src/path/access.rs index 05b8dd625c9ca..204e99a472986 100644 --- a/crates/bevy_reflect/src/path/access.rs +++ b/crates/bevy_reflect/src/path/access.rs @@ -1,4 +1,4 @@ -use std::fmt; +use std::{borrow::Cow, fmt}; use super::{AccessError, ReflectPathError}; use crate::{Reflect, ReflectMut, ReflectRef, VariantType}; @@ -13,10 +13,7 @@ pub(super) enum Error<'a> { access.kind(), access.display_value(), )] - Access { - ty: TypeShape, - access: AccessRef<'a>, - }, + Access { ty: TypeShape, access: Access<'a> }, #[error("invalid type shape: expected {expected} but found a reflect {actual}")] Type { @@ -36,6 +33,10 @@ impl<'a> Error<'a> { let error = AccessError(self); ReflectPathError::InvalidAccess { offset, error } } + + fn access(ty: TypeShape, access: Access<'a>) -> Self { + Self::Access { ty, access } + } } impl Error<'static> { fn bad_enum_variant(expected: TypeShape, actual: impl Into) -> Self { @@ -101,20 +102,18 @@ impl From for TypeShape { } } -/// A singular owned element access within a path. +/// A singular element access within a path. /// /// Can be applied to a `dyn Reflect` to get a reference to the targeted element. -/// -/// A path is composed of multiple accesses in sequence. -#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] -pub(super) enum Access { - Field(Box), +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub(super) enum Access<'a> { + Field(Cow<'a, str>), FieldIndex(usize), TupleIndex(usize), ListIndex(usize), } -impl fmt::Display for Access { +impl fmt::Display for Access<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Access::Field(field) => write!(f, ".{field}"), @@ -125,35 +124,10 @@ impl fmt::Display for Access { } } -impl Access { - pub(super) fn as_ref(&self) -> AccessRef<'_> { - match self { - Self::Field(value) => AccessRef::Field(value), - Self::FieldIndex(value) => AccessRef::FieldIndex(*value), - Self::TupleIndex(value) => AccessRef::TupleIndex(*value), - Self::ListIndex(value) => AccessRef::ListIndex(*value), - } - } -} - -/// A singular borrowed element access within a path. -/// -/// Can be applied to a `dyn Reflect` to get a reference to the targeted element. -/// -/// Does not own the backing store it's sourced from. -/// For an owned version, you can convert one to an [`Access`] with [`AccessRef::to_owned`]. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub(super) enum AccessRef<'a> { - Field(&'a str), - FieldIndex(usize), - TupleIndex(usize), - ListIndex(usize), -} - -impl<'a> AccessRef<'a> { - pub(super) fn to_owned(self) -> Access { +impl<'a> Access<'a> { + pub(super) fn into_owned(self) -> Access<'static> { match self { - Self::Field(value) => Access::Field(value.to_string().into_boxed_str()), + Self::Field(value) => Access::Field(value.to_string().into()), Self::FieldIndex(value) => Access::FieldIndex(value), Self::TupleIndex(value) => Access::TupleIndex(value), Self::ListIndex(value) => Access::ListIndex(value), @@ -174,78 +148,78 @@ impl<'a> AccessRef<'a> { } } - pub(super) fn element( - self, - base: &dyn Reflect, + pub(super) fn element<'r>( + &self, + base: &'r dyn Reflect, offset: usize, - ) -> Result<&dyn Reflect, ReflectPathError<'a>> { + ) -> Result<&'r dyn Reflect, ReflectPathError<'a>> { let ty = base.reflect_ref().into(); self.element_inner(base) - .and_then(|maybe| maybe.ok_or(Error::Access { ty, access: self })) + .and_then(|maybe| maybe.ok_or(Error::access(ty, self.clone()))) .map_err(|err| err.with_offset(offset)) } - fn element_inner(self, base: &dyn Reflect) -> InnerResult<&dyn Reflect> { + fn element_inner<'r>(&self, base: &'r dyn Reflect) -> InnerResult<&'r dyn Reflect> { use ReflectRef::*; match (self, base.reflect_ref()) { - (Self::Field(field), Struct(struct_ref)) => Ok(struct_ref.field(field)), + (Self::Field(field), Struct(struct_ref)) => Ok(struct_ref.field(field.as_ref())), (Self::Field(field), Enum(enum_ref)) => match enum_ref.variant_type() { - VariantType::Struct => Ok(enum_ref.field(field)), + VariantType::Struct => Ok(enum_ref.field(field.as_ref())), actual => Err(Error::bad_enum_variant(TypeShape::Struct, actual)), }, - (Self::FieldIndex(index), Struct(struct_ref)) => Ok(struct_ref.field_at(index)), - (Self::FieldIndex(index), Enum(enum_ref)) => match enum_ref.variant_type() { + (&Self::FieldIndex(index), Struct(struct_ref)) => Ok(struct_ref.field_at(index)), + (&Self::FieldIndex(index), Enum(enum_ref)) => match enum_ref.variant_type() { VariantType::Struct => Ok(enum_ref.field_at(index)), actual => Err(Error::bad_enum_variant(TypeShape::Struct, actual)), }, - (Self::TupleIndex(index), TupleStruct(tuple)) => Ok(tuple.field(index)), - (Self::TupleIndex(index), Tuple(tuple)) => Ok(tuple.field(index)), - (Self::TupleIndex(index), Enum(enum_ref)) => match enum_ref.variant_type() { + (&Self::TupleIndex(index), TupleStruct(tuple)) => Ok(tuple.field(index)), + (&Self::TupleIndex(index), Tuple(tuple)) => Ok(tuple.field(index)), + (&Self::TupleIndex(index), Enum(enum_ref)) => match enum_ref.variant_type() { VariantType::Tuple => Ok(enum_ref.field_at(index)), actual => Err(Error::bad_enum_variant(TypeShape::Tuple, actual)), }, - (Self::ListIndex(index), List(list)) => Ok(list.get(index)), - (Self::ListIndex(index), Array(list)) => Ok(list.get(index)), - (Self::ListIndex(_), actual) => Err(Error::bad_type(TypeShape::List, actual)), + (&Self::ListIndex(index), List(list)) => Ok(list.get(index)), + (&Self::ListIndex(index), Array(list)) => Ok(list.get(index)), + (&Self::ListIndex(_), actual) => Err(Error::bad_type(TypeShape::List, actual)), (_, actual) => Err(Error::bad_type(TypeShape::Struct, actual)), } } - pub(super) fn element_mut( - self, - base: &mut dyn Reflect, + pub(super) fn element_mut<'r>( + &self, + base: &'r mut dyn Reflect, offset: usize, - ) -> Result<&mut dyn Reflect, ReflectPathError<'a>> { + ) -> Result<&'r mut dyn Reflect, ReflectPathError<'a>> { let ty = base.reflect_ref().into(); self.element_inner_mut(base) - .and_then(|maybe| maybe.ok_or(Error::Access { ty, access: self })) + .and_then(|maybe| maybe.ok_or(Error::access(ty, self.clone()))) .map_err(|err| err.with_offset(offset)) } - fn element_inner_mut(self, base: &mut dyn Reflect) -> InnerResult<&mut dyn Reflect> { + fn element_inner_mut<'r>(&self, base: &'r mut dyn Reflect) -> InnerResult<&'r mut dyn Reflect> { use ReflectMut::*; - let base_kind: TypeShape = base.reflect_ref().into(); + let base_shape: TypeShape = base.reflect_ref().into(); match (self, base.reflect_mut()) { - (Self::Field(field), Struct(struct_mut)) => Ok(struct_mut.field_mut(field)), + (Self::Field(field), Struct(struct_mut)) => Ok(struct_mut.field_mut(field.as_ref())), (Self::Field(field), Enum(enum_mut)) => match enum_mut.variant_type() { - VariantType::Struct => Ok(enum_mut.field_mut(field)), + VariantType::Struct => Ok(enum_mut.field_mut(field.as_ref())), actual => Err(Error::bad_enum_variant(TypeShape::Struct, actual)), }, - (Self::FieldIndex(index), Struct(struct_mut)) => Ok(struct_mut.field_at_mut(index)), - (Self::FieldIndex(index), Enum(enum_mut)) => match enum_mut.variant_type() { + (&Self::FieldIndex(index), Struct(struct_mut)) => Ok(struct_mut.field_at_mut(index)), + (&Self::FieldIndex(index), Enum(enum_mut)) => match enum_mut.variant_type() { VariantType::Struct => Ok(enum_mut.field_at_mut(index)), actual => Err(Error::bad_enum_variant(TypeShape::Struct, actual)), }, - (Self::TupleIndex(index), TupleStruct(tuple)) => Ok(tuple.field_mut(index)), - (Self::TupleIndex(index), Tuple(tuple)) => Ok(tuple.field_mut(index)), - (Self::TupleIndex(index), Enum(enum_mut)) => match enum_mut.variant_type() { + (&Self::TupleIndex(index), TupleStruct(tuple)) => Ok(tuple.field_mut(index)), + (&Self::TupleIndex(index), Tuple(tuple)) => Ok(tuple.field_mut(index)), + (&Self::TupleIndex(index), Enum(enum_mut)) => match enum_mut.variant_type() { VariantType::Tuple => Ok(enum_mut.field_at_mut(index)), actual => Err(Error::bad_enum_variant(TypeShape::Tuple, actual)), }, - (Self::ListIndex(index), List(list)) => Ok(list.get_mut(index)), - (Self::ListIndex(index), Array(list)) => Ok(list.get_mut(index)), - (Self::ListIndex(_), _) => Err(Error::bad_type(TypeShape::List, base_kind)), - (_, _) => Err(Error::bad_type(TypeShape::Struct, base_kind)), + (&Self::ListIndex(index), List(list)) => Ok(list.get_mut(index)), + (&Self::ListIndex(index), Array(list)) => Ok(list.get_mut(index)), + (&Self::ListIndex(_), _) => Err(Error::bad_type(TypeShape::List, base_shape)), + (_, _) => Err(Error::bad_type(TypeShape::Struct, base_shape)), } } } diff --git a/crates/bevy_reflect/src/path/mod.rs b/crates/bevy_reflect/src/path/mod.rs index 41e1c3d04f4d8..0ad33ae62b697 100644 --- a/crates/bevy_reflect/src/path/mod.rs +++ b/crates/bevy_reflect/src/path/mod.rs @@ -4,7 +4,7 @@ use std::fmt; use std::num::ParseIntError; use crate::Reflect; -use access::{Access, AccessRef}; +use access::Access; use thiserror::Error; type ParseResult = Result; @@ -291,7 +291,7 @@ pub struct ParsedPath( /// index of the start of the access within the parsed path string. /// /// The index is mainly used for more helpful error reporting. - Box<[(Access, usize)]>, + Box<[(Access<'static>, usize)]>, ); impl ParsedPath { @@ -343,7 +343,17 @@ impl ParsedPath { pub fn parse(string: &str) -> Result> { let mut parts = Vec::new(); for (access, idx) in PathParser::new(string) { - parts.push((access?.to_owned(), idx)); + parts.push((access?.into_owned(), idx)); + } + Ok(Self(parts.into_boxed_slice())) + } + + /// Similar to [`Self::parse`] but only works on `&'static str` + /// and does not allocate per named field. + pub fn parse_static(string: &'static str) -> Result> { + let mut parts = Vec::new(); + for (access, idx) in PathParser::new(string) { + parts.push((access?, idx)); } Ok(Self(parts.into_boxed_slice())) } @@ -359,7 +369,7 @@ impl ParsedPath { ) -> Result<&'r dyn Reflect, ReflectPathError<'p>> { let mut current = root; for (access, current_index) in self.0.iter() { - current = access.as_ref().element(current, *current_index)?; + current = access.element(current, *current_index)?; } Ok(current) } @@ -375,7 +385,7 @@ impl ParsedPath { ) -> Result<&'r mut dyn Reflect, ReflectPathError<'p>> { let mut current = root; for (access, current_index) in self.0.iter() { - current = access.as_ref().element_mut(current, *current_index)?; + current = access.element_mut(current, *current_index)?; } Ok(current) } @@ -471,15 +481,15 @@ impl<'a> PathParser<'a> { Some(ident) } - fn token_to_access(&mut self, token: Token<'a>) -> ParseResult> { + fn token_to_access(&mut self, token: Token<'a>) -> ParseResult> { let current_offset = self.index; match token { Token::Dot => { if let Some(Token::Ident(value)) = self.next_token() { value .parse::() - .map(AccessRef::TupleIndex) - .or(Ok(AccessRef::Field(value))) + .map(Access::TupleIndex) + .or(Ok(Access::Field(value.into()))) } else { Err(ReflectPathParseError::ExpectedIdent { offset: current_offset, @@ -488,7 +498,7 @@ impl<'a> PathParser<'a> { } Token::CrossHatch => { if let Some(Token::Ident(value)) = self.next_token() { - Ok(AccessRef::FieldIndex(value.parse::()?)) + Ok(Access::FieldIndex(value.parse::()?)) } else { Err(ReflectPathParseError::ExpectedIdent { offset: current_offset, @@ -497,7 +507,7 @@ impl<'a> PathParser<'a> { } Token::OpenBracket => { let access = if let Some(Token::Ident(value)) = self.next_token() { - AccessRef::ListIndex(value.parse::()?) + Access::ListIndex(value.parse::()?) } else { return Err(ReflectPathParseError::ExpectedIdent { offset: current_offset, @@ -519,14 +529,14 @@ impl<'a> PathParser<'a> { }), Token::Ident(value) => value .parse::() - .map(AccessRef::TupleIndex) - .or(Ok(AccessRef::Field(value))), + .map(Access::TupleIndex) + .or(Ok(Access::Field(value.into()))), } } } impl<'a> Iterator for PathParser<'a> { - type Item = (ParseResult>, usize); + type Item = (ParseResult>, usize); fn next(&mut self) -> Option { let token = self.next_token()?; @@ -615,7 +625,7 @@ mod tests { } fn access_field(field: &'static str) -> Access { - Access::Field(field.to_string().into_boxed_str()) + Access::Field(field.into()) } #[test] @@ -778,7 +788,7 @@ mod tests { offset: 2, error: AccessError(access::Error::Access { ty: TypeShape::Struct, - access: AccessRef::Field("notreal"), + access: access_field("notreal"), }), } ); From 9a0db765389090b6986419098d6f234aa1838e72 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Wed, 5 Jul 2023 15:40:29 +0200 Subject: [PATCH 11/11] Remove unecessary lifetime + consistent naming --- crates/bevy_reflect/src/path/mod.rs | 64 ++++++++++++----------------- 1 file changed, 26 insertions(+), 38 deletions(-) diff --git a/crates/bevy_reflect/src/path/mod.rs b/crates/bevy_reflect/src/path/mod.rs index 0ad33ae62b697..6ea95da503b11 100644 --- a/crates/bevy_reflect/src/path/mod.rs +++ b/crates/bevy_reflect/src/path/mod.rs @@ -190,19 +190,16 @@ pub trait GetPath { /// /// To retrieve a statically typed reference, use /// [`path`][GetPath::path]. - fn reflect_path<'r, 'p>( - &'r self, - path: &'p str, - ) -> Result<&'r dyn Reflect, ReflectPathError<'p>>; + fn reflect_path<'p>(&self, path: &'p str) -> Result<&dyn Reflect, ReflectPathError<'p>>; /// Returns a mutable reference to the value specified by `path`. /// /// To retrieve a statically typed mutable reference, use /// [`path_mut`][GetPath::path_mut]. - fn reflect_path_mut<'r, 'p>( - &'r mut self, + fn reflect_path_mut<'p>( + &mut self, path: &'p str, - ) -> Result<&'r mut dyn Reflect, ReflectPathError<'p>>; + ) -> Result<&mut dyn Reflect, ReflectPathError<'p>>; /// Returns a statically typed reference to the value specified by `path`. /// @@ -211,7 +208,7 @@ pub trait GetPath { /// (which may be the case when using dynamic types like [`DynamicStruct`]). /// /// [`DynamicStruct`]: crate::DynamicStruct - fn path<'r, 'p, T: Reflect>(&'r self, path: &'p str) -> Result<&'r T, ReflectPathError<'p>> { + fn path<'p, T: Reflect>(&self, path: &'p str) -> Result<&T, ReflectPathError<'p>> { self.reflect_path(path).and_then(|p| { p.downcast_ref::() .ok_or(ReflectPathError::InvalidDowncast) @@ -225,10 +222,7 @@ pub trait GetPath { /// (which may be the case when using dynamic types like [`DynamicStruct`]). /// /// [`DynamicStruct`]: crate::DynamicStruct - fn path_mut<'r, 'p, T: Reflect>( - &'r mut self, - path: &'p str, - ) -> Result<&'r mut T, ReflectPathError<'p>> { + fn path_mut<'p, T: Reflect>(&mut self, path: &'p str) -> Result<&mut T, ReflectPathError<'p>> { self.reflect_path_mut(path).and_then(|p| { p.downcast_mut::() .ok_or(ReflectPathError::InvalidDowncast) @@ -237,40 +231,34 @@ pub trait GetPath { } impl GetPath for T { - fn reflect_path<'r, 'p>( - &'r self, - path: &'p str, - ) -> Result<&'r dyn Reflect, ReflectPathError<'p>> { + fn reflect_path<'p>(&self, path: &'p str) -> Result<&dyn Reflect, ReflectPathError<'p>> { (self as &dyn Reflect).reflect_path(path) } - fn reflect_path_mut<'r, 'p>( - &'r mut self, + fn reflect_path_mut<'p>( + &mut self, path: &'p str, - ) -> Result<&'r mut dyn Reflect, ReflectPathError<'p>> { + ) -> Result<&mut dyn Reflect, ReflectPathError<'p>> { (self as &mut dyn Reflect).reflect_path_mut(path) } } impl GetPath for dyn Reflect { - fn reflect_path<'r, 'p>( - &'r self, - path: &'p str, - ) -> Result<&'r dyn Reflect, ReflectPathError<'p>> { + fn reflect_path<'p>(&self, path: &'p str) -> Result<&dyn Reflect, ReflectPathError<'p>> { let mut current: &dyn Reflect = self; - for (access, current_index) in PathParser::new(path) { - current = access?.element(current, current_index)?; + for (access, offset) in PathParser::new(path) { + current = access?.element(current, offset)?; } Ok(current) } - fn reflect_path_mut<'r, 'p>( - &'r mut self, + fn reflect_path_mut<'p>( + &mut self, path: &'p str, - ) -> Result<&'r mut dyn Reflect, ReflectPathError<'p>> { + ) -> Result<&mut dyn Reflect, ReflectPathError<'p>> { let mut current: &mut dyn Reflect = self; - for (access, current_index) in PathParser::new(path) { - current = access?.element_mut(current, current_index)?; + for (access, offset) in PathParser::new(path) { + current = access?.element_mut(current, offset)?; } Ok(current) } @@ -342,8 +330,8 @@ impl ParsedPath { /// pub fn parse(string: &str) -> Result> { let mut parts = Vec::new(); - for (access, idx) in PathParser::new(string) { - parts.push((access?.into_owned(), idx)); + for (access, offset) in PathParser::new(string) { + parts.push((access?.into_owned(), offset)); } Ok(Self(parts.into_boxed_slice())) } @@ -352,8 +340,8 @@ impl ParsedPath { /// and does not allocate per named field. pub fn parse_static(string: &'static str) -> Result> { let mut parts = Vec::new(); - for (access, idx) in PathParser::new(string) { - parts.push((access?, idx)); + for (access, offset) in PathParser::new(string) { + parts.push((access?, offset)); } Ok(Self(parts.into_boxed_slice())) } @@ -368,8 +356,8 @@ impl ParsedPath { root: &'r dyn Reflect, ) -> Result<&'r dyn Reflect, ReflectPathError<'p>> { let mut current = root; - for (access, current_index) in self.0.iter() { - current = access.element(current, *current_index)?; + for (access, offset) in self.0.iter() { + current = access.element(current, *offset)?; } Ok(current) } @@ -384,8 +372,8 @@ impl ParsedPath { root: &'r mut dyn Reflect, ) -> Result<&'r mut dyn Reflect, ReflectPathError<'p>> { let mut current = root; - for (access, current_index) in self.0.iter() { - current = access.element_mut(current, *current_index)?; + for (access, offset) in self.0.iter() { + current = access.element_mut(current, *offset)?; } Ok(current) }