[Merged by Bors] - Make `Reflect` safe to implement #5010

PROMETHIA-27 · 2022-06-14T01:07:09Z

Objective

Currently, Reflect is unsafe to implement because of a contract in which any and any_mut must return self, or downcast will cause UB. This PR makes Reflect safe, makes downcast not use unsafe, and eliminates this contract.

Solution

This PR adds a method to Reflect, any. It also renames the old any to as_any.
any now takes a Box<Self> and returns a Box<dyn Any>.

Changelog

Added:

any() method
represents() method

Changed:

Reflect is now a safe trait
downcast() is now safe
The old any is now called as_any, and any_mut is now as_mut_any

Migration Guide

Reflect derives should not have to change anything
Manual reflect impls will need to remove the unsafe keyword, add any() implementations, and rename the old any and any_mut to as_any and as_mut_any.
Calls to any/any_mut must be changed to as_any/as_mut_any

Points of discussion:

Should renaming any be avoided and instead name the new method any_box?
~~Could there be a performance regression from avoiding the unsafe? I doubt it, but this change does seem to introduce redundant checks.~~
~~Could/should is and type_id() be implemented differently? For example, moving is onto Reflect as an fn(&self, TypeId) -> bool~~

crates/bevy_reflect/src/impls/std.rs

Co-authored-by: Gingeh <[email protected]>

Hopefully fix CI?

alice-i-cecile · 2022-06-14T01:45:19Z

Nice work!

Should renaming any be avoided and instead name the new method any_box?

I think that's fine; this naming scheme is clearer.

Could there be a performance regression from avoiding the unsafe? I doubt it, but this change does seem to introduce redundant checks.

I wouldn't accept this justification to make it unsafe, so I won't block on the answer here.

Could/should is and type_id() be implemented differently? For example, moving is onto Reflect as an fn(&self, TypeId) -> bool

I like the current design quite a bit; I think it's quite clear and should be easy to use.

crates/bevy_reflect/src/reflect.rs

crates/bevy_reflect/src/lib.rs

examples/reflection/trait_reflection.rs

crates/bevy_reflect/src/struct_trait.rs

MrGVSV · 2022-06-14T02:19:32Z

Should renaming any be avoided and instead name the new method any_box?

I agree with the name change. Although, it should be noted that it goes against the convention we've established thus far (DynamicTuple::insert_boxed, DynamicStruct::push_box, etc.)

Could/should is and type_id() be implemented differently? For example, moving is onto Reflect as an fn(&self, TypeId) -> bool

I think it makes sense to move is to Reflect if it's given a default implementation. Otherwise, it's probably fine to just to reflect_value.type_id() == other_type_id.

alice-i-cecile · 2022-06-14T02:20:11Z

I don't remember the conclusion: do we care about implementing Reflect for non-Rust types? If so, we should either split off type_id or have it return an Option.

Co-authored-by: Gino Valente <[email protected]>

PROMETHIA-27 · 2022-06-14T13:05:16Z

Thinking about it more, I'm wondering if downcast should be more directly controlled?

Normal types want to downcast to themselves and report their own TypeID
Dynamic types want to be unable to downcast and report a different TypeID, but optionally
Box wants to downcast to its inner type and report its inner type's TypeId. But what if we want to downcast to the box itself? E.g. we have a Box<dyn Reflect>(i32) behind a Box<dyn Reflect>, downcasting it would give the i32, but what if we just want the inner Box?

Co-authored-by: Gino Valente <[email protected]>

killercup · 2022-06-20T21:57:32Z

Just a comment on the wording: To be consistent with the Rust API guidelines, I'd suggest calling the methods into_any(Box<Self>) -> Box<dyn Any>, as_any(&self) -> &dyn Any, and as_mut_any(&mut self) -> &mut dyn Any. That is also already the naming followed by the methods below.

(That also allows you to deprecate the previous any method and add a blanket impl for a short transition period if that is at all needed.)

into make-reflect-safe

PROMETHIA-27 · 2022-06-21T00:24:23Z

This PR is more or less blocked on the Unique Reflect RFC; that might wildly change what is necessary here/possibly obsolete this PR.

MrGVSV

Like you said, I'm sure things will change a lot with the Unique Reflect RfC, but I left some comments just in case.

My main issue rn is with the inclusion of an underlying_type_id method. It could be worth it, but I'm just having a hard time getting past its possible confusions with proxy types.

MrGVSV · 2022-06-21T01:04:40Z

crates/bevy_reflect/src/reflect.rs


    /// Returns the value as a [`&mut dyn Any`][std::any::Any].
-    fn any_mut(&mut self) -> &mut dyn Any;
+    fn as_mut_any(&mut self) -> &mut dyn Any;


Shouldn't this be as_any_mut to match the naming of other methods in the codebase?

quoting the rust api guidelines:

If the mut qualifier in the name of a conversion method constitutes part of the return type, it should appear as it would appear in the type. For example Vec::as_mut_slice returns a mut slice; it does what it says. This name is preferred over as_slice_mut.

But I could accept reversing it (to as_any_mut) for consistency, as it's not in the scope of this PR to change global naming conventions.

Hm yeah it might be good to rename this to as_any_mut at least to match as_reflect_mut. But we can look into renaming these according to a more Rust-approved convention later.

crates/bevy_reflect/src/reflect.rs

crates/bevy_reflect/src/type_info.rs

crates/bevy_reflect/src/utility.rs

PROMETHIA-27 · 2022-06-21T12:44:43Z

I'd be comfortable waiting for Unique Reflect for this, but if anyone else (@MrGVSV?) would also like to see this move forward first then I'll give this another pass and resolve my issues with it. Mainly I want to redo is, type_id, and downcast to be clearer as to what they mean, probably reusing terminology from the Unique Reflect RFC. In fact I'll probably want to split them roughly like this:
represents: is but for checking if this represents the given type (probably via repr_type_name comparison)
is: Add documentation clarifying that this refers to the underlying type, and what that means.
repr_type_name(): The optional Cow<&str> of the represented type's name.
~~type_name()~~: Replaced by inheriting from TypeName, a new trait with a blanket impl so that no one can violate the rule that this is the exact name of the underlying type.
repr_type_id(): The optional TypeId of the represented type. I may omit this entirely since None can't mean "this value doesn't represent a type", only "this value doesn't know the TypeId of the represented type". Maybe return a Result with multiple variants instead, to be more descriptive?
~~type_id()~~: Replaced by inheriting Any::type_id()
~~downcast~~ into_underlying: Downcasts only from the dyn reflect into the true underlying type as according to is.
~~downcast_ref~~ as_underlying: see above
~~downcast_mut~~ as_mut_underlying: see above
for "downcasting" (I believe the RFC calls this "sidecasting") to the represented type, use FromReflect.
With these changes (and maybe more- splitting the any/reflect methods into blanket impls as well, to force returning the underlying type?) I would also be happy to merge this before Unique Reflect.

MrGVSV · 2022-06-21T20:51:51Z

I'd be comfortable waiting for Unique Reflect for this, but if anyone else (@MrGVSV?) would also like to see this move forward first then I'll give this another pass and resolve my issues with it.

Yeah I think it makes sense to get this merged even before the RFC.

Mainly I want to redo is, type_id, and downcast to be clearer as to what they mean, probably reusing terminology from the Unique Reflect RFC. In fact I'll probably want to split them roughly like this:

I don't think these changes should be part of this PR. They add new functionality that isn't directly in attempts to make Reflect safe.

represents: is but for checking if this represents the given type (probably via repr_type_name comparison)
is: Add documentation clarifying that this refers to the underlying type, and what that means.

I really like the idea to include a represents method. The other suggestions, though, I'm a bit more hesitant on, especially with the Unique Reflect RFC possibly having a large impact on their usage/implementation.

PROMETHIA-27 · 2022-06-21T21:04:04Z

That's fair; I suppose fixing the API is what Unique Reflect is for. I'll start working on this soon.

into make-reflect-safe

MrGVSV · 2022-06-27T00:05:57Z

crates/bevy_reflect/src/reflect.rs


    /// Returns the value as a [`&mut dyn Any`][std::any::Any].
-    fn any_mut(&mut self) -> &mut dyn Any;
+    fn as_mut_any(&mut self) -> &mut dyn Any;


Hm yeah it might be good to rename this to as_any_mut at least to match as_reflect_mut. But we can look into renaming these according to a more Rust-approved convention later.

crates/bevy_reflect/src/reflect.rs

PROMETHIA-27 · 2022-06-27T12:56:47Z

I've just removed the doc links; I don't think there's a way to make them work. They won't recognize Self as dyn Reflect, and fully qualified <dyn Reflect>:: syntax isn't supported, so there's no way to reference the functions.

alice-i-cecile

Surprisingly simple in practice; those impl files make this look way more involved than it is.

bors r+

# Objective Currently, `Reflect` is unsafe to implement because of a contract in which `any` and `any_mut` must return `self`, or `downcast` will cause UB. This PR makes `Reflect` safe, makes `downcast` not use unsafe, and eliminates this contract. ## Solution This PR adds a method to `Reflect`, `any`. It also renames the old `any` to `as_any`. `any` now takes a `Box<Self>` and returns a `Box<dyn Any>`. --- ## Changelog ### Added: - `any()` method - `represents()` method ### Changed: - `Reflect` is now a safe trait - `downcast()` is now safe - The old `any` is now called `as_any`, and `any_mut` is now `as_mut_any` ## Migration Guide - Reflect derives should not have to change anything - Manual reflect impls will need to remove the `unsafe` keyword, add `any()` implementations, and rename the old `any` and `any_mut` to `as_any` and `as_mut_any`. - Calls to `any`/`any_mut` must be changed to `as_any`/`as_mut_any` ## Points of discussion: - Should renaming `any` be avoided and instead name the new method `any_box`? - ~~Could there be a performance regression from avoiding the unsafe? I doubt it, but this change does seem to introduce redundant checks.~~ - ~~Could/should `is` and `type_id()` be implemented differently? For example, moving `is` onto `Reflect` as an `fn(&self, TypeId) -> bool`~~ Co-authored-by: PROMETHIA-27 <[email protected]>

bors · 2022-06-27T17:12:25Z

Pull request successfully merged into main.

Build succeeded:

cart · 2022-06-27T20:36:18Z

crates/bevy_reflect/src/reflect.rs

    pub fn downcast<T: Reflect>(self: Box<dyn Reflect>) -> Result<Box<T>, Box<dyn Reflect>> {
-        // SAFE?: Same approach used by std::any::Box::downcast. ReflectValue is always Any and type
-        // has been checked.
        if self.is::<T>() {


Downcast does this check internally. We should probably remove it from here.

We can't without changing the error type from Box<dyn Reflect> to Box<dyn Any>

Spun out into #5120.

# Objective Currently, `Reflect` is unsafe to implement because of a contract in which `any` and `any_mut` must return `self`, or `downcast` will cause UB. This PR makes `Reflect` safe, makes `downcast` not use unsafe, and eliminates this contract. ## Solution This PR adds a method to `Reflect`, `any`. It also renames the old `any` to `as_any`. `any` now takes a `Box<Self>` and returns a `Box<dyn Any>`. --- ## Changelog ### Added: - `any()` method - `represents()` method ### Changed: - `Reflect` is now a safe trait - `downcast()` is now safe - The old `any` is now called `as_any`, and `any_mut` is now `as_mut_any` ## Migration Guide - Reflect derives should not have to change anything - Manual reflect impls will need to remove the `unsafe` keyword, add `any()` implementations, and rename the old `any` and `any_mut` to `as_any` and `as_mut_any`. - Calls to `any`/`any_mut` must be changed to `as_any`/`as_mut_any` ## Points of discussion: - Should renaming `any` be avoided and instead name the new method `any_box`? - ~~Could there be a performance regression from avoiding the unsafe? I doubt it, but this change does seem to introduce redundant checks.~~ - ~~Could/should `is` and `type_id()` be implemented differently? For example, moving `is` onto `Reflect` as an `fn(&self, TypeId) -> bool`~~ Co-authored-by: PROMETHIA-27 <[email protected]>

PROMETHIA-27 added 5 commits June 13, 2022 20:03

Initial commit

9356871

Made core changes to Reflect

c764de1

Updated impls until it compiles again

9a71ebf

Got tests compiling and passing

47e6802

Added downcast test

479e63d

PROMETHIA-27 requested a review from MrGVSV June 14, 2022 01:07

Gingeh reviewed Jun 14, 2022

View reviewed changes

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

Remove missed safety comment

ff804f7

Co-authored-by: Gingeh <[email protected]>

alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Jun 14, 2022

alice-i-cecile added this to the Bevy 0.8 milestone Jun 14, 2022

Updated example to use Reflect::type_id()

4611cc9

Hopefully fix CI?

alice-i-cecile approved these changes Jun 14, 2022

View reviewed changes

MrGVSV reviewed Jun 14, 2022

View reviewed changes

PROMETHIA-27 and others added 2 commits June 13, 2022 22:53

Type ID -> TypeId

1916a2f

Co-authored-by: Gino Valente <[email protected]>

Made type_id() impl default

4bcfc1f

PROMETHIA-27 and others added 2 commits June 14, 2022 10:05

Removed unused imports

2a7d0f0

Type Name -> type name

6a04778

Co-authored-by: Gino Valente <[email protected]>

alice-i-cecile requested a review from MrGVSV June 20, 2022 19:10

PROMETHIA-27 and others added 4 commits June 20, 2022 20:14

Merge branch 'bevyengine:main' into make-reflect-safe

cad5074

change to better naming scheme

05d01f9

Merge branch 'make-reflect-safe' of https://github.com/PROMETHIA-27/bevy

1073a84

into make-reflect-safe

fixing ambiguities

48803b3

MrGVSV reviewed Jun 21, 2022

View reviewed changes

Remove unnecessary TypeId imports

e79a8c3

PROMETHIA-27 and others added 7 commits June 26, 2022 13:42

Add represents method to dyn Reflect

bde44f2

underlying_type_id -> type_id

ced94c7

Merge branch 'bevyengine:main' into make-reflect-safe

4df85c4

Merge branch 'make-reflect-safe' of https://github.com/PROMETHIA-27/bevy

03c97b4

into make-reflect-safe

Fix slightly broken test

cec23c0

Un-fixing broken test

2ae01fb

underlying_type_id -> type_id

481823f

MrGVSV approved these changes Jun 27, 2022

View reviewed changes

PROMETHIA-27 added 2 commits June 26, 2022 21:34

Doc links

29a9379

as_mut_any -> as_any_mut

7b66ee8

alice-i-cecile approved these changes Jun 27, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 27, 2022

MrGVSV approved these changes Jun 27, 2022

View reviewed changes

Removed invalid doc links

2f557c1

alice-i-cecile approved these changes Jun 27, 2022

View reviewed changes

bors bot changed the title ~~Make Reflect safe to implement~~ [Merged by Bors] - Make Reflect safe to implement Jun 27, 2022

bors bot closed this Jun 27, 2022

PROMETHIA-27 deleted the make-reflect-safe branch June 27, 2022 17:13

cart reviewed Jun 27, 2022

View reviewed changes

alice-i-cecile mentioned this pull request Jun 27, 2022

Remove redundant check in Reflect::downcast method #5120

Open

Uh oh!

Uh oh!

[Merged by Bors] - Make Reflect safe to implement #5010

[Merged by Bors] - Make Reflect safe to implement #5010

Uh oh!

Conversation

PROMETHIA-27 commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Changelog

Added:

Changed:

Migration Guide

Points of discussion:

Uh oh!

Uh oh!

alice-i-cecile commented Jun 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MrGVSV commented Jun 14, 2022

Uh oh!

alice-i-cecile commented Jun 14, 2022

Uh oh!

PROMETHIA-27 commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

killercup commented Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PROMETHIA-27 commented Jun 21, 2022

Uh oh!

MrGVSV left a comment

Choose a reason for hiding this comment

Uh oh!

MrGVSV Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

PROMETHIA-27 Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrGVSV Jun 27, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PROMETHIA-27 commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrGVSV commented Jun 21, 2022

Uh oh!

PROMETHIA-27 commented Jun 21, 2022

Uh oh!

MrGVSV Jun 27, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PROMETHIA-27 commented Jun 27, 2022

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Jun 27, 2022

Uh oh!

cart Jun 27, 2022

Choose a reason for hiding this comment

Uh oh!

PROMETHIA-27 Jun 27, 2022

Choose a reason for hiding this comment

Uh oh!

[Merged by Bors] - Make `Reflect` safe to implement #5010

[Merged by Bors] - Make `Reflect` safe to implement #5010

PROMETHIA-27 commented Jun 14, 2022 •

edited

Loading

PROMETHIA-27 commented Jun 14, 2022 •

edited

Loading

killercup commented Jun 20, 2022 •

edited

Loading

PROMETHIA-27 Jun 21, 2022 •

edited

Loading

PROMETHIA-27 commented Jun 21, 2022 •

edited

Loading