Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reflect: implement the unique reflect rfc #7207

Merged
merged 34 commits into from
Aug 12, 2024

Conversation

soqb
Copy link
Contributor

@soqb soqb commented Jan 15, 2023

Objective

Solution

  • Implements the RFC.
  • This implementation differs in some ways from the RFC:
    • In the RFC, it was suggested Reflect: Any but PartialReflect: ?Any. During initial implementation I tried this, but we assume the PartialReflect: 'static in a lot of places and the changes required crept out of the scope of this PR.
    • PartialReflect::try_into_reflect originally returned Option<Box<dyn Reflect>> but i changed this to Result<Box<dyn Reflect>, Box<dyn PartialReflect>> since the method takes by value and otherwise there would be no way to recover the type. as_full and as_full_mut both still return Option<&(mut) dyn Reflect>.

Changelog

  • Added PartialReflect.
  • Reflect is now a subtrait of PartialReflect.
  • Moved most methods on Reflect to the new PartialReflect.
  • Added PartialReflect::{as_partial_reflect, as_partial_reflect_mut, into_partial_reflect}.
  • Added PartialReflect::{try_as_reflect, try_as_reflect_mut, try_into_reflect}.
  • Added <dyn PartialReflect>::{try_downcast_ref, try_downcast_mut, try_downcast, try_take} supplementing the methods on dyn Reflect.

Migration Guide

  • Most instances of dyn Reflect should be changed to dyn PartialReflect which is less restrictive, however trait bounds should generally stay as T: Reflect.
  • The new PartialReflect::{as_partial_reflect, as_partial_reflect_mut, into_partial_reflect, try_as_reflect, try_as_reflect_mut, try_into_reflect} methods as well as Reflect::{as_reflect, as_reflect_mut, into_reflect} will need to be implemented for manual implementors of Reflect.

Future Work

@nicopap nicopap self-assigned this Jan 15, 2023
@nicopap
Copy link
Contributor

nicopap commented Jan 15, 2023

Yooo! Awesome. Thank you so much for taking the time to implement the RFC. I'm sorry for not doing it myself. This would unlock the next step for bevy_reflect. I'll at least take the time to review this thoroughfully.

@alice-i-cecile alice-i-cecile added A-Reflection Runtime information about types C-Feature A new feature, making something new possible X-Controversial There is active debate or serious implications around merging this PR labels Jan 15, 2023
@soqb soqb marked this pull request as ready for review January 16, 2023 14:21
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theory hits reality at full speed and hard!

The PR is actually not bad, it's just a lot of simple changes. So I encourage people to check it out. It's just a bit time consuming.

This really questions the necessity/feasibility of unique reflect. With this PR, the problems the RFC sets to solve are not solved. And I'm not sure there is a clear path to actually solve them. Specifically, how could we get rid of the try_downcast_ref() methods on dyn PartialReflect while staying ergonomic?

I really don't want to force users to do my_value.as_full().unwrap().downcast_ref() instead of my_value.try_downcast_ref() everywhere.

I think a bit I missed in the RFC is this:

  • Most things should now accept a dyn PartialReflect rather than
    dyn Reflect.

Should be:

  • Most things should now accept a dyn PartialReflect rather than
    dyn Reflect.
  • Most thing that return dyn Reflect should still return dyn Reflect.

Then unique reflect would make sense, otherwise it's just needless boilerplate.

Is it an achievable goal? If not, I think we should drop the unique Reflect RFC.

If it is achievable (even if not within this PR), then just a few comments and this is good to go.

crates/bevy_asset/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_input/src/gamepad.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/utility.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/reflect.rs Show resolved Hide resolved
crates/bevy_reflect/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/reflect.rs Outdated Show resolved Hide resolved
@soqb
Copy link
Contributor Author

soqb commented Jan 21, 2023

Is it an achievable goal? If not, I think we should drop the unique Reflect RFC.

i think it's probably alright. most of the diff in this PR come from just renaming Reflect to PartialReflect before anything else was changed so it was all a bit hasty.

returning dyn Reflect over dyn PartialReflect where applicable definitely feels like a good practice.

i'll give the changes a once over when i get the time because as you've pointed out a lot of return types and probably some trait bounds are PartialReflect when they should really be Reflect.

i reckon some of the changes will be better suited to a follow-up PR merged in the same release (i'm not expecting this to be merged by 0.10) and/or when the current situation surrounding reflect_hash/reflect_partial_eq/serializable is resolved but i'll do what i think is reasonable for this PR.

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just some minor issues to address.

crates/bevy_asset/src/reflect.rs Outdated Show resolved Hide resolved
examples/reflection/reflection.rs Show resolved Hide resolved
crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved
@MrGVSV MrGVSV self-requested a review January 31, 2023 22:39
crates/bevy_reflect/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/assets.rs Outdated Show resolved Hide resolved
@nicopap nicopap self-requested a review August 20, 2023 18:47
@nicopap nicopap removed their assignment Aug 26, 2023
@alice-i-cecile alice-i-cecile added this to the 0.12 milestone Aug 29, 2023
fn into_any(self: Box<Self>) -> Box<dyn Any>;
/// Returns the value as a fully-reflected [`&dyn Reflect`](Reflect),
/// or [`None`] if the value does not implement it.
fn try_as_reflect(&self) -> Option<&dyn Reflect>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshed: I think the convention I've seen is that try_*** methods return a Result. I wonder if we should do the same. That might mean adding a new error type for the dynamic types to return.

We could also go back to naming these as_reflect and just return the Option.

Same for the methods on dyn PartialReflect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RFC uses "canonical" for Reflect, so it's probably better to keep to .as_canonical or even .canonical_ref() , considering std::Any has a downcast_ref returning Option<&T>.

"Canonical" risks getting users asking "does this method only works on ubuntu-based linux distributions". But it is probably easier as an implementor to keep consistent, since that's the terminology we settled on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the convention I've seen is that try_*** methods return a Result.

this is certainly the most common form for try_*** methods, but i don't see any reason why Options shouldn't be included. the Try trait is implemented for both types, and so are supported by ?. std's methods try_for_each etc. support Option.

even if we consensus disagrees i would prefer returning a Result<&dyn Reflect, BikeshedUnitReflectUpcastError> than changing the name of the method.

i don't prefer as_canonical at all. it's both vague and conceptual: my rule of thumb for good naming is that users should be able to understand why the method has its name from just its signature and documentation, without doing any wider research and i don't think a method like this should have more than two lines of documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even if we consensus disagrees i would prefer returning a Result<&dyn Reflect, BikeshedUnitReflectUpcastError> than changing the name of the method.

Yeah I agree. It might also set us up for potential error handling in the future.

i don't prefer as_canonical at all. it's both vague and conceptual

Yeah canonical works for the RFC, but in practice I think I like the reflect/partial_reflect relation better. I think it will be a lot easier to grasp for newcomers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The objections make sense. I'm not married to canonical. I think "try_as_foo" is a bit weird though. Maybe "as_reflect" or "try_reflect"

key: Box<dyn Reflect>,
mut value: Box<dyn Reflect>,
) -> Option<Box<dyn Reflect>> {
key: Box<dyn PartialReflect>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most dynamic types don't implement reflect_hash. So maybe until #8695, we should enforce this by making the key dyn Reflect (and same for all other methods that require a hashable key). While this would be more strict, it would also be more helpful since we could catch this error at compile time rather than runtime.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd rather not change this method twice. reflect_hash is quite goofy but i'd rather not induce two sets of churn for an often-unused api with a solution already in the works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't say it's unused— we get a fair number of people inadvertently running into this. But your reasoning makes sense and we can just wait for #8695 to land before slapping on bandaids.

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve, because it makes a much needed change and I don't see anything wrong. But I really think we should reconsider naming.

I looked back at the RFC. And I think the old name was better. This adds a lot of noise to the Reflect API. Basically 70% of the code change here could be removed if we swapped what was renamed.

It's important to have the most common type be the short name, so that when naming things, the additional qualifier in the name shows what's special about it. It builds on how we human pattern-match the world. The other way around is confusing IMO.

So the RFC used CanonReflect. What about:

  • CanonicalReflect or UniqueReflect for what the RFC calls Reflect
  • Reflect for what the RFC calls PartialReflect

/// Function pointer implementing [`ReflectComponent::remove()`].
pub remove: fn(&mut EntityMut),
/// Function pointer implementing [`ReflectComponent::contains()`].
pub contains: fn(EntityRef) -> bool,
/// Function pointer implementing [`ReflectComponent::reflect()`].
pub reflect: fn(EntityRef) -> Option<&dyn Reflect>,
pub reflect: fn(EntityRef) -> Option<&dyn PartialReflect>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The components are always stored as concrete types, might as well return the restricted type.

Suggested change
pub reflect: fn(EntityRef) -> Option<&dyn PartialReflect>,
pub reflect: fn(EntityRef) -> Option<&dyn Reflect>,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i totally agree that this can be changed without much resistance but i'm not sure we want to. currently all ComponentIds map 1-to-1 with Rust types, but if we (and we do want to) support dynamic component types this relationship will change, and i think there's good reasons to want to reflect those components with this api.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think we need a place where we collect all the "either PartialReflect or Reflect" decisions. If you are willing to write a comment in the RFC discussions, I'll take care to update the RFC with such a new section.

/// Function pointer implementing [`ReflectComponent::reflect_mut()`].
pub reflect_mut: for<'a> fn(&'a mut EntityMut<'_>) -> Option<Mut<'a, dyn Reflect>>,
pub reflect_mut: for<'a> fn(&'a mut EntityMut<'_>) -> Option<Mut<'a, dyn PartialReflect>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Suggested change
pub reflect_mut: for<'a> fn(&'a mut EntityMut<'_>) -> Option<Mut<'a, dyn PartialReflect>>,
pub reflect_mut: for<'a> fn(&'a mut EntityMut<'_>) -> Option<Mut<'a, dyn Reflect>>,

/// Function pointer implementing [`ReflectResource::remove()`].
pub remove: fn(&mut World),
/// Function pointer implementing [`ReflectResource::reflect()`].
pub reflect: fn(&World) -> Option<&dyn Reflect>,
pub reflect: fn(&World) -> Option<&dyn PartialReflect>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub reflect: fn(&World) -> Option<&dyn PartialReflect>,
pub reflect: fn(&World) -> Option<&dyn Reflect>,

fn into_any(self: Box<Self>) -> Box<dyn Any>;
/// Returns the value as a fully-reflected [`&dyn Reflect`](Reflect),
/// or [`None`] if the value does not implement it.
fn try_as_reflect(&self) -> Option<&dyn Reflect>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RFC uses "canonical" for Reflect, so it's probably better to keep to .as_canonical or even .canonical_ref() , considering std::Any has a downcast_ref returning Option<&T>.

"Canonical" risks getting users asking "does this method only works on ubuntu-based linux distributions". But it is probably easier as an implementor to keep consistent, since that's the terminology we settled on.

/// #[derive(Reflect)]
/// struct MyTupleStruct(usize);
///
/// let my_tuple_struct: &dyn Reflect = &MyTupleStruct(123);
/// let my_tuple_struct: &dyn PartialReflect = &MyTupleStruct(123);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// let my_tuple_struct: &dyn PartialReflect = &MyTupleStruct(123);
/// let my_tuple_struct: &dyn Reflect = &MyTupleStruct(123);

crates/bevy_reflect/src/type_info.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/bevy_reflect_derive/src/impls/enums.rs Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member

@soqb, are you able to clean this up? This is in the 0.12 milestone and I'd like to move this forward, but it looks like there's more work to be done here.

@soqb
Copy link
Contributor Author

soqb commented Oct 2, 2023

sorry for the late reply, i've had an incredibly busy week but hope to find some time tonight to address what i need to here.

@MrGVSV
Copy link
Member

MrGVSV commented Oct 2, 2023

I approve, because it makes a much needed change and I don't see anything wrong. But I really think we should reconsider naming.

I looked back at the RFC. And I think the old name was better. This adds a lot of noise to the Reflect API. Basically 70% of the code change here could be removed if we swapped what was renamed.

It's important to have the most common type be the short name, so that when naming things, the additional qualifier in the name shows what's special about it. It builds on how we human pattern-match the world. The other way around is confusing IMO.

So the RFC used CanonReflect. What about:

  • CanonicalReflect or UniqueReflect for what the RFC calls Reflect
  • Reflect for what the RFC calls PartialReflect

This was briefly talked about in this thread, but I'd like to push back on this. If I remember right, the reason we chose Reflect and PartialReflect is that it allowed newcomers to associate them to similarly-named traits (e.g. Eq and PartialEq). This association helps demonstrate that Reflect is a stricter subset of PartialReflect, and that PartialReflect is "good enough" for most use cases, again similar to traits like Eq and PartialEq.

However, I will admit that CanonicalReflect or UniqueReflect might better indicate where that strictness comes from (i.e. that it's the actual, canonical type it claims it is). I'm still not a huge fan of the canonical terminology, though. And if we did go with something like this, I feel we may also want to consider naming Reflect to something like DynamicReflect to really indicate that it's a "looser" version of CanonicalReflect.

So my vote is for PartialReflect/Reflect, but that's just one person's opinion so feel free to disagree with me haha

@nicopap
Copy link
Contributor

nicopap commented Oct 2, 2023

Pointing that out to avoid regrets down the line. Unlike PartialOrd or PartialEq, PartialReflect is meant to be used as trait objects, and it is present all over the place.

Instead of seeing PartialReflect as a partial version of Reflect, we can see Reflect as being a specialized PartialReflect. Rust has Iterator and ExactSizeIterator for example, not PartialIterator and Iterator. So it would make sense to have a Reflect and Un1kReflect (silly name used for illustration).

I'm fine with the current compromise though.

@MrGVSV
Copy link
Member

MrGVSV commented Oct 2, 2023

Pointing that out to avoid regrets down the line. Unlike PartialOrd or PartialEq, PartialReflect is meant to be used as trait objects, and it is present all over the place.

I don't think its usage as a trait object prevents us from using the Partial naming scheme. People use PartialEq as bounds and such a lot (and more than they do for Eq), which I think plays well into how we want people to prefer PartialReflect unless they truly need a full Reflect value.

Instead of seeing PartialReflect as a partial version of Reflect, we can see Reflect as being a specialized PartialReflect. Rust has Iterator and ExactSizeIterator for example, not PartialIterator and Iterator. So it would make sense to have a Reflect and Un1kReflect (silly name used for illustration).

I think the difference with Iterator comes from the fact that there's a lot of different Iterator traits in the Rust standard library. None of them could really be a "PartialIterator" since they all have different meanings for what an "iterator" is. We really only have two reflection traits: the stricter one and the looser one.

That being said, I see your point about not needing a DynamicReflect like I suggested. Iterator isn't called BasicIterator or anything like that, so we don't need to do the same for Reflect if we end up using something like CanonicalReflect/UniqueReflect.

I'm fine with the current compromise though.

I left a message on Discord to draw more attention to this issue since the entire PR is a big shift and we want to ensure the community is generally on board with whatever we end up doing.

@soqb
Copy link
Contributor Author

soqb commented Oct 3, 2023

I'm fine with the current compromise though.

i think it boils down to what is the most accessible. even with good docs, UniqueReflect or even {Canonical, Full, Serializing}Reflect are non-obvious names where people are familiar with PartialFoo, if they see it, they'll know very roughly what the name represents.

to me, it seems more helpful to say PartialReflect is Reflect without some of the important bits, compared to UniqueReflect is Reflect with extra important bits. under either naming scheme, people will reach for Reflect first when they are unsure, and under the first scheme, this will more often be adequate.

@alice-i-cecile alice-i-cecile modified the milestones: 0.12, 0.13 Oct 20, 2023
@alice-i-cecile alice-i-cecile added X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed X-Controversial There is active debate or serious implications around merging this PR labels Aug 8, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid: no blocking concerns! I've merged a couple of grammar nits. @soqb, once this is merge conflict free ping me and let's get this merged.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 8, 2024
soqb and others added 3 commits August 9, 2024 19:27
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Aug 9, 2024
@alice-i-cecile
Copy link
Member

A doc link on ReflectSet needs cleaning up before this can merge: https://github.com/bevyengine/bevy/actions/runs/10324580350/job/28584326492?pr=7207#step:6:2042

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 12, 2024
Merged via the queue into bevyengine:main with commit 6ab8767 Aug 12, 2024
26 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Sep 13, 2024
# Objective

Thanks to #7207, we now have a way to validate at the type-level that a
reflected value is actually the type it says it is and not just a
dynamic representation of that type.

`dyn PartialReflect` values _might_ be a dynamic type, but `dyn Reflect`
values are guaranteed to _not_ be a dynamic type.

Therefore, we can start to add methods to `Reflect` that weren't really
possible before. For example, we should now be able to always get a
`&'static TypeInfo`, and not just an `Option<&'static TypeInfo>`.

## Solution

Add the `DynamicTyped` trait.

This trait is similar to `DynamicTypePath` in that it provides a way to
use the non-object-safe `Typed` trait in an object-safe way.

And since all types that derive `Reflect` will also derive `Typed`, we
can safely add `DynamicTyped` as a supertrait of `Reflect`. This allows
us to use it when just given a `dyn Reflect` trait object.

## Testing

You can test locally by running:

```
cargo test --package bevy_reflect
```

---

## Showcase

`Reflect` now has a supertrait of `DynamicTyped`, allowing `TypeInfo` to
be retrieved from a `dyn Reflect` trait object without having to unwrap
anything!

```rust
let value: Box<dyn Reflect> = Box::new(String::from("Hello!"));

// BEFORE
let info: &'static TypeInfo = value.get_represented_type_info().unwrap();

// AFTER
let info: &'static TypeInfo = value.reflect_type_info();
```

## Migration Guide

`Reflect` now has a supertrait of `DynamicTyped`. If you were manually
implementing `Reflect` and did not implement `Typed`, you will now need
to do so.
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1671 if you'd like to help out.

@Shatur
Copy link
Contributor

Shatur commented Nov 6, 2024

Looks like this PR missing in the migration guide for some reason.

Shatur added a commit to ironpeak/bevy_replicon that referenced this pull request Nov 6, 2024
Looks like it should be `PartialReflect` after all:
bevyengine/bevy#7207
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

6 participants