adding reflection for Cow<'static, [T]> #7454

EliasPrescott · 2023-02-01T01:50:05Z

Objective

Implementing reflection for Cow<'static, [T]>
Hopefully fixes bevy_reflect: Support Cow<'static, [T]> #7429

Solution

Implementing Reflect, Typed, GetTypeRegistration, and FromReflect for Cow<'static, [T]>

Notes

I have not used bevy_reflection much yet, so I may not fully understand all the use cases. This is also my first attempt at contributing, so I would appreciate any feedback or recommendations for changes. I tried to add cases for using Cow<'static, str> and Cow<'static, [u8]> to some of the bevy_reflect tests, but I can't guarantee those tests are comprehensive enough.

github-actions · 2023-02-01T01:50:19Z

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

MrGVSV

Welcome and thank you for your contribution @EliasPrescott (especially for taking the time to add tests)! Left a few comments 🙂

MrGVSV · 2023-02-01T03:54:29Z

crates/bevy_reflect/src/impls/std.rs

@@ -1039,6 +1039,130 @@ impl FromReflect for Cow<'static, str> {
    }
 }

+impl<T: Clone + std::marker::Sync + std::marker::Send + std::hash::Hash + std::cmp::PartialEq>


Nit: I don't think we need to fully qualify these since they're already in-scope (same for the other impls). By pure convention, I think we really only qualify std::any::type_name.

Suggested change

impl<T: Clone + std::marker::Sync + std::marker::Send + std::hash::Hash + std::cmp::PartialEq>

impl<T: Clone + Sync + Send + Hash + PartialEq>

MrGVSV · 2023-02-01T04:05:43Z

crates/bevy_reflect/src/impls/std.rs

+    fn reflect_ref(&self) -> ReflectRef {
+        ReflectRef::Value(self)
+    }
+
+    fn reflect_mut(&mut self) -> ReflectMut {
+        ReflectMut::Value(self)
+    }
+
+    fn reflect_owned(self: Box<Self>) -> ReflectOwned {
+        ReflectOwned::Value(self)
+    }


I think we should consider making this implement Array and return Reflect***::Array instead. A slice acts more like an array than a pure value (you could argue that str should also work like this, but I think that's a special enough case).

This also means we should be able to replace the Clone, Hash, and PartialEq bounds on T with a simple Reflect bound (and FromReflect for the FromReflect impl). I'm not 100% sure if doing that will work as easily as I'm making it sound haha, but I believe that going the Array route is probably the better option.

MrGVSV · 2023-02-01T07:11:57Z

crates/bevy_reflect/src/impls/std.rs

@@ -1039,6 +1039,131 @@ impl FromReflect for Cow<'static, str> {
    }
 }

+impl<T: FromReflect + Clone + Send + Sync> Array for Cow<'static, [T]> {


I don't think the Clone bound is necessary (for any of the impls). I'd recommend taking a look at the trait implementations for [T; N].

I thought Clone would be necessary, because otherwise I get the error: [T]: ToOwned is not satisfied in Cow<'static, [T]>.

Here are the docs on Cow where it mentions the ToOwned bound. I would like to remove the Clone bounds if possible, but that's the only way I've been able to sneak past the type checker so far.

Are you sure that the Send and Sync bounds are necessary?

Are you sure that the Send and Sync bounds are necessary?

Good point, they should be added by the FromReflect bound already.

Thanks for pointing that out. I removed them in the latest commit. I think I added them before the FromReflect bound was there and I didn't realize they were no longer needed.

crates/bevy_reflect/src/impls/std.rs

MrGVSV · 2023-02-01T07:16:48Z

crates/bevy_reflect/src/impls/std.rs

+impl<T: FromReflect + Clone + Send + Sync> Typed for Cow<'static, [T]> {
+    fn type_info() -> &'static TypeInfo {
+        static CELL: NonGenericTypeInfoCell = NonGenericTypeInfoCell::new();
+        CELL.get_or_set(|| TypeInfo::Array(ArrayInfo::new::<Self, T>(0)))


Hm, it's kinda weird that we'd set the capacity to 0 here. I wonder if there's a better way of doing this? 🤔

Yeah I was going to ask about that. I thought a slice type didn't have a capacity, unless we consider the hardware's max isize as a capacity.

Trying to implement this almost made me feel like it'd be more natural to be returning a Reflect***::List instead of the Array case. I haven't tried that yet, but that could keep us from deciding on a random capacity.

Can you insert/remove data on a slice? If so then yeah List would be better. Otherwise, I’m not sure of the best approach...

You can't insert/remove data on a regular slice, but you can on a Cow slice by doing a to_mut and then inserting/removing.

Basically a Cow<'static, [T]> is a Vec<T> that can optionally store ALL of its data in static memory. Therefore it's closer to being a Vec<T> than [T; N], so it should be treated as a List in reflection.

@ItsDoot I added your implementation of List for Cow<'static, [T]> from #7455 and switched the Array reflections to use the List implementation. By the way, I'm sorry if I hijacked your issue. I should have left a comment asking to claim it first.

crates/bevy_reflect/src/impls/std.rs

Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>

ItsDoot

Looks fine overall, just one major issue and a few nits.

ItsDoot · 2023-02-01T21:25:09Z

crates/bevy_reflect/src/impls/std.rs

+    }
+
+    fn drain(self: Box<Self>) -> Vec<Box<dyn Reflect>> {
+        self.iter().map(Reflect::clone_value).collect()


You can save it from doing an unnecessary clone if you have a Cow::Owned by switching this to self.into_owned().into_iter(). That would only clone the data if it's currently borrowed, otherwise it would reuse the owned data. Although that currently triggers a false-positive in clippy, so it will need an #[allow] tag added.

Thanks for pointing that out! Also, thank you for the allow tag idea. I was having lots of trouble getting around that Clippy lint.

ItsDoot · 2023-02-01T21:25:48Z

crates/bevy_reflect/src/impls/std.rs

@@ -1039,6 +1039,131 @@ impl FromReflect for Cow<'static, str> {
    }
 }

+impl<T: FromReflect + Clone + Send + Sync> Array for Cow<'static, [T]> {


Are you sure that the Send and Sync bounds are necessary?

ItsDoot · 2023-02-01T21:29:24Z

crates/bevy_reflect/src/impls/std.rs

+impl<T: FromReflect + Clone + Send + Sync> Typed for Cow<'static, [T]> {
+    fn type_info() -> &'static TypeInfo {
+        static CELL: NonGenericTypeInfoCell = NonGenericTypeInfoCell::new();
+        CELL.get_or_set(|| TypeInfo::Array(ArrayInfo::new::<Self, T>(0)))


Basically a Cow<'static, [T]> is a Vec<T> that can optionally store ALL of its data in static memory. Therefore it's closer to being a Vec<T> than [T; N], so it should be treated as a List in reflection.

ItsDoot

One more.

ItsDoot · 2023-02-01T21:39:21Z

crates/bevy_reflect/src/impls/std.rs

+    }
+}
+
+impl<T: FromReflect + Clone + Send + Sync> GetTypeRegistration for Cow<'static, [T]> {


ReflectDeserialize and ReflectSerialize require that the type implements Deserialize and Serialize, respectively.

Suggested change

impl<T: FromReflect + Clone + Send + Sync> GetTypeRegistration for Cow<'static, [T]> {

impl<'de, T: FromReflect + Clone + Serialize + Deserialize<'de>> GetTypeRegistration for Cow<'static, [T]> {

However... what if we want to be able to reflect one that isn't serializable? Now that's not possible.

I added the Deserialize and Serialize trait bounds. Thankfully, I had just read this post that talks about higher-ranked trait bounds, because that came in very handy here.

I disagree that we should add Serialize and Deserialize bounds. Again, I recommend looking at the implementation for [T; N] rather than for Cow<'static, str>. The reason we don't have those trait bounds there is because it really limits what T can be.

ReflectSerialize and ReflectDeserialize can always be registered manually (like we do with many other types).

That makes sense. I changed the GetTypeRegistration impl to match the other generic types in std.rs.

…unds

MrGVSV · 2023-02-03T06:04:18Z

crates/bevy_reflect/src/impls/std.rs

+
+impl<T: FromReflect + Clone> Typed for Cow<'static, [T]> {
+    fn type_info() -> &'static TypeInfo {
+        static CELL: NonGenericTypeInfoCell = NonGenericTypeInfoCell::new();


I think this should use the GenericTypeInfoCell, right?

Yes, I believe so. Thanks for pointing that out!

github-actions · 2023-02-03T08:11:34Z

Example alien_cake_addict failed to run, please try running it locally and check the result.

MrGVSV

LGTM! Thanks for working on this! 😄

MrGVSV · 2023-02-03T15:14:03Z

bors try

bors · 2023-02-03T15:29:19Z

try

Build succeeded:

EliasPrescott · 2023-02-03T17:26:23Z

LGTM! Thanks for working on this! 😄

Thank you so much for helping me! Also, thank you to @ItsDoot and @MinerSebas for reviewing and suggesting changes. You were all very helpful and I learned a lot!

MrGVSV · 2023-03-28T01:01:35Z

@EliasPrescott Sorry for the delay with this one. Could you rebase this PR on current main? I think it might be broken after #7467

EliasPrescott · 2023-03-28T04:18:17Z

@EliasPrescott Sorry for the delay with this one. Could you rebase this PR on current main? I think it might be broken after #7467

No worries! I synced my fork with main and updated the impl<T: FromReflect + Clone> List for Cow<'static, [T]> block to match the changes. Let me know if you see anything else that needs changed. Thanks!

alice-i-cecile · 2023-06-12T18:57:55Z

Fixed the merge conflicts for you. Once I have another approval that I didn't screw that up I'll merge this.

MrGVSV · 2023-06-12T19:00:06Z

@alice-i-cecile there still seems to be some CI errors

alice-i-cecile · 2023-06-12T21:47:09Z

Ah, formatting issues. @EliasPrescott want to format and push for us?

ItsDoot

Looks good otherwise, from what I can recall.

crates/bevy_reflect/src/impls/std.rs

…r [T]

…_to_owned

EliasPrescott · 2023-06-17T14:05:22Z

crates/bevy_reflect/src/impls/std.rs

+impl<T: PathOnly> PathOnly for [T] where [T]: ToOwned {}
+
+impl<T: TypePath> TypePath for [T]
+where
+    [T]: ToOwned,
+{
+    fn type_path() -> &'static str {
+        static CELL: GenericTypePathCell = GenericTypePathCell::new();
+        CELL.get_or_insert::<Self, _>(|| format!("[{}]", <T>::type_path()))
+    }
+
+    fn short_type_path() -> &'static str {
+        static CELL: GenericTypePathCell = GenericTypePathCell::new();
+        CELL.get_or_insert::<Self, _>(|| format!("[{}]", <T>::short_type_path()))
+    }
+}
+
+impl<T: ToOwned> PathOnly for T {}


@MrGVSV @ItsDoot

This is new code that I had to add after merging the latest changes into my fork. I want feedback on it especially because I'm not sure if adding blanket impls like this is a good practice or not.

Also, I wanted to ask if I need to expand the type returned from type_path() for [T]. As far as I know, the slice type doesn't actually live in any namespace, but if it did, we could return something like core::slice::<[T]> instead.

This looks good to me. The blanket impl should be fine as far as I can tell. I'll be honest, I'm not sure why we have this trait over just using ToOwned directly, but either way this should be fine.

MrGVSV · 2023-06-19T14:56:23Z

crates/bevy_reflect/src/impls/std.rs

+impl<T: PathOnly> PathOnly for [T] where [T]: ToOwned {}
+
+impl<T: TypePath> TypePath for [T]
+where
+    [T]: ToOwned,
+{
+    fn type_path() -> &'static str {
+        static CELL: GenericTypePathCell = GenericTypePathCell::new();
+        CELL.get_or_insert::<Self, _>(|| format!("[{}]", <T>::type_path()))
+    }
+
+    fn short_type_path() -> &'static str {
+        static CELL: GenericTypePathCell = GenericTypePathCell::new();
+        CELL.get_or_insert::<Self, _>(|| format!("[{}]", <T>::short_type_path()))
+    }
+}
+
+impl<T: ToOwned> PathOnly for T {}


This looks good to me. The blanket impl should be fine as far as I can tell. I'll be honest, I'm not sure why we have this trait over just using ToOwned directly, but either way this should be fine.

# Objective - Implementing reflection for Cow<'static, [T]> - Hopefully fixes bevyengine#7429 ## Solution - Implementing Reflect, Typed, GetTypeRegistration, and FromReflect for Cow<'static, [T]> --- ## Notes I have not used bevy_reflection much yet, so I may not fully understand all the use cases. This is also my first attempt at contributing, so I would appreciate any feedback or recommendations for changes. I tried to add cases for using Cow<'static, str> and Cow<'static, [u8]> to some of the bevy_reflect tests, but I can't guarantee those tests are comprehensive enough. --------- Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

adding reflection for Cow<'static, [T]>

Verified

This commit was signed with the committer’s verified signature.

JamesNK James Newton-King

GPG key ID: 0A66B2F456BF5526

Verified
Learn about vigilant mode

018c84c

MrGVSV added C-Usability A-Reflection labels Feb 1, 2023

MrGVSV reviewed Feb 1, 2023

View reviewed changes

MrGVSV mentioned this pull request Feb 1, 2023

bevy_reflect: Implement Reflect and associated traits for Cow<'static, [T]> #7455

Closed

EliasPrescott added 2 commits February 1, 2023 00:34

switching Cow<[T]> to reflect as Array

Verified

This commit was signed with the committer’s verified signature.

JamesNK James Newton-King

GPG key ID: 0A66B2F456BF5526

Verified
Learn about vigilant mode

a417689

fixing formatting

Verified

This commit was signed with the committer’s verified signature.

JamesNK James Newton-King

GPG key ID: 0A66B2F456BF5526

Verified
Learn about vigilant mode

1ab3653

MrGVSV suggested changes Feb 1, 2023

View reviewed changes

making suggested fixes

Verified

This commit was signed with the committer’s verified signature.

JamesNK James Newton-King

GPG key ID: 0A66B2F456BF5526

Verified
Learn about vigilant mode

7d0f308

MinerSebas reviewed Feb 1, 2023

View reviewed changes

crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved

ItsDoot suggested changes Feb 1, 2023

View reviewed changes

EliasPrescott added 3 commits February 1, 2023 17:17

switching reflection type to list and adding Serialize/Deserialize bo…

Verified

This commit was signed with the committer’s verified signature.

JamesNK James Newton-King

GPG key ID: 0A66B2F456BF5526

Verified
Learn about vigilant mode

2621bc7

…unds

simplifying Reflect implementations for Cow<[T]>

Verified

This commit was signed with the committer’s verified signature.

JamesNK James Newton-King

GPG key ID: 0A66B2F456BF5526

Verified
Learn about vigilant mode

5a5a872

Removing serialize bounds and improving Cow<[u8]> test

60209c6

MrGVSV suggested changes Feb 3, 2023

View reviewed changes

switching to use GenericTypeInfoCell

db130db

MrGVSV approved these changes Feb 3, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Feb 3, 2023

Try #7454:

b6d7993

MrGVSV requested review from ItsDoot and MinerSebas and removed request for ItsDoot and MinerSebas February 13, 2023 06:45

ItsDoot approved these changes Feb 14, 2023

View reviewed changes

EliasPrescott added 2 commits March 27, 2023 20:22

Merge branch 'bevyengine:main' into adding-cow-slice-reflection

f3f5c41

fixing rebase errors

3effb2c

MrGVSV added this to the 0.11 milestone Apr 23, 2023

MrGVSV added the S-Ready-For-Final-Review label May 30, 2023

alice-i-cecile approved these changes Jun 5, 2023

View reviewed changes

alice-i-cecile added this pull request to the merge queue Jun 5, 2023

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Jun 5, 2023

Merge branch 'main' into adding-cow-slice-reflection

4857bf3

alice-i-cecile requested review from MinerSebas, ItsDoot and MrGVSV June 12, 2023 18:57

alice-i-cecile and others added 2 commits June 12, 2023 18:28

Cargo fmt

61feb35

Merge branch 'bevyengine:main' into adding-cow-slice-reflection

337e148

ItsDoot approved these changes Jun 17, 2023

View reviewed changes

crates/bevy_reflect/src/impls/std.rs Show resolved Hide resolved

EliasPrescott added 2 commits June 17, 2023 08:05

updating reflect impl and adding blanket PathOnly + TypePath impls fo…

2ec5cc2

…r [T]

fixing formatting and adding comment explaining the allow unnecessary…

7cc8ea2

…_to_owned

EliasPrescott commented Jun 17, 2023

View reviewed changes

MrGVSV approved these changes Jun 19, 2023

View reviewed changes

alice-i-cecile added this pull request to the merge queue Jun 19, 2023

Merged via the queue into bevyengine:main with commit e6b655f Jun 19, 2023

	impl<T: Clone + std::marker::Sync + std::marker::Send + std::hash::Hash + std::cmp::PartialEq>
	impl<T: Clone + Sync + Send + Hash + PartialEq>

	impl<T: FromReflect + Clone + Send + Sync> GetTypeRegistration for Cow<'static, [T]> {
	impl<'de, T: FromReflect + Clone + Serialize + Deserialize<'de>> GetTypeRegistration for Cow<'static, [T]> {

adding reflection for Cow<'static, [T]> #7454

adding reflection for Cow<'static, [T]> #7454

Conversation

EliasPrescott commented Feb 1, 2023

Objective

Solution

Notes

github-actions bot commented Feb 1, 2023

MrGVSV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EliasPrescott Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ItsDoot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ItsDoot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrGVSV Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 3, 2023

MrGVSV left a comment

Choose a reason for hiding this comment

MrGVSV commented Feb 3, 2023

bors bot commented Feb 3, 2023

try

EliasPrescott commented Feb 3, 2023

MrGVSV commented Mar 28, 2023

EliasPrescott commented Mar 28, 2023

alice-i-cecile commented Jun 12, 2023

MrGVSV commented Jun 12, 2023

alice-i-cecile commented Jun 12, 2023

ItsDoot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EliasPrescott Feb 1, 2023 •

edited

Loading

MrGVSV Feb 2, 2023 •

edited

Loading