Refactor path module of bevy_reflect
#8887
Conversation
nicopap
added
C-Code-Quality
|
I like the split a lot, and think this is an improvement in terms of code organization. |
# Objective `ParsedPath` does not need to be mut to access a field of a `Reflect`. Be that access mutable or not. Yet `element_mut` requires a mutable borrow on `self`. ## Solution - Make `element_mut` take a `&self` over a `&mut self`. #8887 fixes this, but this is a major limitation in the API and I'd rather see it merged before 0.11. --- ## Changelog - `ParsedPath::element_mut` and `ParsedPath::reflect_element_mut` now accept a non-mutable `ParsedPath` (only the accessed `Reflect` needs to be mutable)
|
@alice-i-cecile Those types are only public because they are part of I created a wrapper type so that I don't want to document them, it's difficult to come up with a good vocabulary, and it would require renaming every types in the module. And all of this because of fields of an error value. |
|
Sounds good, I think that's a sensible solution <3 |
# Objective `ParsedPath` does not need to be mut to access a field of a `Reflect`. Be that access mutable or not. Yet `element_mut` requires a mutable borrow on `self`. ## Solution - Make `element_mut` take a `&self` over a `&mut self`. bevyengine#8887 fixes this, but this is a major limitation in the API and I'd rather see it merged before 0.11. --- ## Changelog - `ParsedPath::element_mut` and `ParsedPath::reflect_element_mut` now accept a non-mutable `ParsedPath` (only the accessed `Reflect` needs to be mutable)
MrGVSV
left a comment
MrGVSV
left a comment
There was a problem hiding this comment.
Awesome job! This is much cleaner!
Just some nits, but overall looks good!
| pub(super) enum Type { | ||
| Struct, | ||
| TupleStruct, | ||
| Tuple, | ||
| List, | ||
| Array, | ||
| Map, | ||
| Enum, | ||
| Value, | ||
| Unit, | ||
| } |
There was a problem hiding this comment.
This is pretty similar to this code from #7075 and is something I think deserves its own small PR due to how much it seems to want to crop up in these code de-duplications.
bevy/crates/bevy_reflect/src/reflect.rs
Lines 14 to 33 in 7974319
The only difference is that your code here includes a Unit variant (not sure how important it is to keep or not).
And I bring this up more as a note rather than something this PR needs to be blocked on.
There was a problem hiding this comment.
Yeah, I'd really like a TypeShape of sort. I think I copied the ReflectRef definition and removed the values from the variants to make the Type enum. I added Unit later to account for mismatched enum variants (Unit is the variant without value).
There was a problem hiding this comment.
Yeah and actually I wonder if TypeShape is a clearer terminology. We'll probably explore this more in a future PR.
``` 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.
This removes 8 bytes to the size of `Access`.
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.
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.
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
|
@MrGVSV I found a nice way to merge Also thank you for the review, it helped improve the code a lot I think. |
MrGVSV
left a comment
MrGVSV
left a comment
There was a problem hiding this comment.
@MrGVSV I found a nice way to merge
AccessandAccessRef. According tocargo asm, it is also a bit faster, and we also get a bonusparse_staticmethod that removes all but one allocation when buildingParsedPath!Also thank you for the review, it helped improve the code a lot I think.
Awesome! Glad it worked out and great job cleaning this up! 😄
| )] | ||
| Access { ty: TypeShape, access: Access<'a> }, | ||
|
|
||
| #[error("invalid type shape: expected {expected} but found a reflect {actual}")] |
There was a problem hiding this comment.
Replying to this comment:
yeah, but
Typehere is not really a "type" but a "type shape", The error message would read as follow: "expected struct but found tuple". I don't think there is such a thing as a "struct-like" it's either a struct or not a struct.
Well I think it would be more useful for TypeShape::Tuple and TypeShape::List, since those also work for tuple structs and arrays, respectively. It's not required we use -like haha, I just think it would be clearer to users that we're not expecting them to pass only Struct, Tuple, and List types.
nicopap
added
the
S-Ready-For-Final-Review
# Objective - Follow up to #8887 - The parsing code in `bevy_reflect/src/path/mod.rs` could also do with some cleanup ## Solution - Create the `parse.rs` module, move all parsing code to this module - The parsing errors also now keep track of the whole parsed string, and are much more fine-grained ### Detailed changes - Move `PathParser` to `parse.rs` submodule - Rename `token_to_access` to `access_following` (yep, goes from 132 lines to 16) - Move parsing tests into the `parse.rs` file
3 participants
Objective
pathmodule was getting fairly large.AccessRef::read_elementand mut equivalent was very complex and difficult to understand.ReflectPathErrorhad a lot of variants, and was difficult to read.Solution
accessnow has its own moduleread_elementmethods, they were ~200 lines long, they are now ~70 lines long — I didn't change any of the logic. It's really just the same code, but error handling is separated.ReflectPathErrorerrorAccessRefandAccessFully detailed change list
Displayimpl ofParsedPathnow includes prefix dots — this allows simplifying its implementation, and IMO.path.to.fieldis a better way to express a "path" thanpath.to.fieldwhich could suggest we are reading thetofield of a variable namedpathAccessandAccessRef, using aCow<'a, str>. Generated code seems to agree with this decision (ParsedPath::parsesheds 5% of instructions)Access::as_refsince there is no such thing as anAccessRefanymore.AccessRef::to_ownedintoAccessRef::into_owned()since it takes ownership ofselfnow.parse_staticthat doesn't allocate new strings for named fields!bevy_reflectcrate root doc — I saw a few people that weren't aware of path reflection, so I thought it was pertinent to add it to the root docindextooffsetwhen it refers to offset in the path string — There is no more confusion with the other kind of indices in this context, also it's a common naming convention for parsing.read_elementmethods toelement— shorter, but alsoread_element_mutwas a fairly poor nameGetPathtrait methods.Change log
ParsedPath::parse_staticmethod, avoids allocating when parsing&'static str.Migration Guide
If you were matching on the
Err(ReflectPathError)value returned byGetPathandParsedPathmethods, now only the parse-related errors and the offset are publicly accessible. You can always use thefmt::Displayto get a clear error message, but if you need programmatic access to the error types, please open an issue.