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

bevy_reflect: Reflect remote types #6042

Merged
merged 31 commits into from
Aug 12, 2024

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Sep 21, 2022

Objective

The goal with this PR is to allow the use of types that don't implement Reflect within the reflection API.

Rust's orphan rule prevents implementing a trait on an external type when neither type nor trait are owned by the implementor. This means that if a crate, cool_rust_lib, defines a type, Foo, then a user cannot use it with reflection. What this means is that we have to ignore it most of the time:

#[derive(Reflect)]
struct SomeStruct {
  #[reflect(ignore)]
  data: cool_rust_lib::Foo
}

Obviously, it's impossible to implement Reflect on Foo. But does it have to be?

Most of reflection doesn't deal with concrete types— it's almost all using dyn Reflect. And being very metadata-driven, it should theoretically be possible. I mean, serde does it.

Solution

Special thanks to @danielhenrymantilla for their help reviewing this PR and offering wisdom wrt safety.

Taking a page out of serde's book, this PR adds the ability to easily use "remote types" with reflection. In this context, a "remote type" is the external type for which we have no ability to implement Reflect.

This adds the #[reflect_remote(...)] attribute macro, which is used to generate "remote type wrappers". All you have to do is define the wrapper exactly the same as the remote type's definition:

// Pretend this is our external crate
mod cool_rust_lib {
  #[derive(Default)]
  struct Foo {
    pub value: String
  }
}

#[reflect_remote(cool_rust_lib::Foo)]
struct FooWrapper {
  pub value: String
}

Note: All fields in the external type must be public. This could be addressed with a separate getter/setter attribute either in this PR or in another one.

The macro takes this user-defined item and transforms it into a newtype wrapper around the external type, marking it as #[repr(transparent)]. The fields/variants defined by the user are simply used to build out the reflection impls.

Additionally, it generates an implementation of the new trait, ReflectRemote, which helps prevent accidental misuses of this API.

Therefore, the output generated by the macro would look something like:

#[repr(transparent)]
struct FooWrapper(pub cool_rust_lib::Foo);

impl ReflectRemote for FooWrapper {
  type Remote = cool_rust_lib::Foo;

  // transmutation methods...
}

// reflection impls...
// these will acknowledge and make use of the `value` field

Internally, the reflection API will pass around the FooWrapper and transmute it where necessary. All we have to do is then tell Reflect to do that. So rather than ignoring the field, we tell Reflect to use our wrapper using the #[reflect(remote = ...)] field attribute:

#[derive(Reflect)]
struct SomeStruct {
  #[reflect(remote = FooWrapper)]
  data: cool_rust_lib::Foo
}

Other Macros & Type Data

Because this macro consumes the defined item and generates a new one, we can't just put our macros anywhere. All macros that should be passed to the generated struct need to come below this macro. For example, to derive Default and register its associated type data:

// ✅ GOOD
#[reflect_remote(cool_rust_lib::Foo)]
#[derive(Default)]
#[reflect(Default)]
struct FooWrapper {
  pub value: String
}

// ❌ BAD
#[derive(Default)]
#[reflect_remote(cool_rust_lib::Foo)]
#[reflect(Default)]
struct FooWrapper {
  pub value: String
}

Generics

Generics are forwarded to the generated struct as well. They should also be defined in the same order:

#[reflect_remote(RemoteGeneric<'a, T1, T2>)]
struct GenericWrapper<'a, T1, T2> {
  pub foo: &'a T1,
  pub bar: &'a T2,
}

Naming does not need to match the original definition's. Only order matters here.

Also note that the code above is just a demonstration and doesn't actually compile since we'd need to enforce certain bounds (e.g. T1: Reflect, 'a: 'static, etc.)

Nesting

And, yes, you can nest remote types:

#[reflect_remote(RemoteOuter)]
struct OuterWrapper {
  #[reflect(remote = InnerWrapper)]
  pub inner: RemoteInner
}

#[reflect_remote(RemoteInner)]
struct InnerWrapper(usize);

Assertions

This macro will also generate some compile-time assertions to ensure that the correct types are used. It's important we catch this early so users don't have to wait for something to panic. And it also helps keep our unsafe a little safer.

For example, a wrapper definition that does not match its corresponding remote type will result in an error:

mod external_crate {
  pub struct TheirStruct(pub u32);
}

#[reflect_remote(external_crate::TheirStruct)]
struct MyStruct(pub String); // ERROR: expected type `u32` but found `String`
Generated Assertion
const _: () = {
  #[allow(non_snake_case)]
  #[allow(unused_variables)]
  #[allow(unused_assignments)]
  #[allow(unreachable_patterns)]
  #[allow(clippy::multiple_bound_locations)]
  fn assert_wrapper_definition_matches_remote_type(
    mut __remote__: external_crate::TheirStruct,
  ) {
    __remote__.0 = (|| -> ::core::option::Option<String> { None })().unwrap();
  }
};

Additionally, using the incorrect type in a #[reflect(remote = ...)] attribute should result in an error:

mod external_crate {
  pub struct TheirFoo(pub u32);
  pub struct TheirBar(pub i32);
}

#[reflect_remote(external_crate::TheirFoo)]
struct MyFoo(pub u32);

#[reflect_remote(external_crate::TheirBar)]
struct MyBar(pub i32);

#[derive(Reflect)]
struct MyStruct {
  #[reflect(remote = MyBar)] // ERROR: expected type `TheirFoo` but found struct `TheirBar`
  foo: external_crate::TheirFoo
}
Generated Assertion
const _: () = {
    struct RemoteFieldAssertions;
    impl RemoteFieldAssertions {
        #[allow(non_snake_case)]
        #[allow(clippy::multiple_bound_locations)]
        fn assert__foo__is_valid_remote() {
            let _: <MyBar as bevy_reflect::ReflectRemote>::Remote = (|| -> ::core::option::Option<external_crate::TheirFoo> {
              None
            })().unwrap();
        }
    }
};

Discussion

There are a couple points that I think still need discussion or validation.

  • 1. Any shenanigans

    If we wanted to downcast our remote type from a dyn Reflect, we'd have to first downcast to the wrapper then extract the inner type. This PR has a commit that addresses this by making all the Reflect::*any methods return the inner type rather than the wrapper type. This allows us to downcast directly to our remote type.

    However, I'm not sure if this is something we want to do. For unknowing users, it could be confusing and seemingly inconsistent. Is it worth keeping? Or should this behavior be removed?

    I think this should be fine. The remote wrapper is an implementation detail and users should not need to downcast to the wrapper type. Feel free to let me know if there are other opinions on this though!

  • 2. Implementing Deref/DerefMut and From

    We don't currently do this, but should we implement other traits on the generated transparent struct? We could implement Deref/DerefMut to easily access the inner type. And we could implement From for easier conversion between the two types (e.g. T: Into<Foo>). As mentioned in the comments, we probably don't need to do this. Again, the remote wrapper is an implementation detail, and should generally not be used directly.

  • 3. Should we define a getter/setter field attribute in this PR as well or leave it for a future one? I think this should be saved for a future PR

  • 4. Any foreseeable issues with this implementation?

Alternatives

One alternative to defining our own ReflectRemote would be to use bytemuck's TransparentWrapper (as suggested by @danielhenrymantilla).

This is definitely a viable option, as ReflectRemote is pretty much the same thing as TransparentWrapper. However, the cost would be bringing in a new crate— though, it is already in use in a few other sub-crates like bevy_render.

I think we're okay just defining ReflectRemote ourselves, but we can go the bytemuck route if we'd prefer offloading that work to another crate.


Changelog

  • Added the #[reflect_remote(...)] attribute macro to allow Reflect to be used on remote types
  • Added ReflectRemote trait for ensuring proper remote wrapper usage

@MrGVSV MrGVSV added C-Feature A new feature, making something new possible A-Reflection Runtime information about types labels Sep 21, 2022
@BoxyUwU BoxyUwU self-requested a review September 21, 2022 08:00
@MrGVSV MrGVSV force-pushed the reflect-remote branch 2 times, most recently from e2383bb to f86db5e Compare October 19, 2022 07:31
@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Nov 19, 2022
@StarArawn
Copy link
Contributor

This sounds really useful for any sort of editor or inspector in the future. Especially if a particular bevy plugin or non-bevy crate does not use the reflect types. I don't feel qualified to fully review this, as I don't know much about bevy's reflection stuff, but this PR is something that is needed!

@ndarilek
Copy link
Contributor

ndarilek commented Dec 7, 2022

I definitely need this for bevy_synthizer. Some of my types wrap theirs so I can use them as components. Unfortunately, for the moment I can't use any of these types in scenes, which is important since some of these components are audio-related properties that require specific tweaking per entity.

@MrGVSV MrGVSV force-pushed the reflect-remote branch 3 times, most recently from bdac2e4 to d1f0286 Compare December 9, 2022 03:09
@tim-blackbird
Copy link
Contributor

It appears that nesting remote reflect with FromReflect fails.
Example:

use bevy::reflect::reflect_remote;

mod remote {
    pub struct A;

    pub struct B {
        pub a: A,
    }
}

#[reflect_remote(remote::A, FromReflect)]
struct LocalA;

#[reflect_remote(remote::B, FromReflect)] // Error: the trait `FromReflect` is not implemented for `remote::A`
struct LocalB {
    #[reflect(remote = "LocalA")]
    pub a: remote::A,
}

@tim-blackbird
Copy link
Contributor

tim-blackbird commented Dec 20, 2022

Attributes on fields are lost when #[reflect_remote] is added.

mod remote {
    pub struct A {
        #[cfg(oof)]
        pub a: i32,
    }
}

#[reflect_remote(remote::A)]
pub struct LocalA {
    #[cfg(oof)] // Should be grayed out in IDE but isn't after adding reflect_remote
    pub a: i32, // Error: no field `a` on type `remote::A`
}

@MrGVSV
Copy link
Member Author

MrGVSV commented Jan 1, 2023

@devil-ira The nesting issue should be resolved (as well as an issue with generics that I discovered while trying to fix it).

Unfortunately, I'm not sure how to go about fixing the attribute issue. It seems the best way to handle this (without adding complexity/specialized logic) would be to just add the attribute to the entire item:

mod remote {
    pub struct A {
        #[cfg(oof)]
        pub a: i32,
    }
}

#[cfg(not(oof))]
#[reflect_remote(remote::A)]
pub struct LocalA {}

#[cfg(oof)]
#[reflect_remote(remote::A)]
pub struct LocalA {
    pub a: i32,
}

I think if we want to fix this, it should probably happen in a followup PR.

bors bot pushed a commit that referenced this pull request Jan 2, 2023
# Objective

There isn't really a way to test that code using bevy_reflect compiles or doesn't compile for certain scenarios. This would be especially useful for macro-centric PRs like #6511 and #6042.

## Solution

Using `bevy_ecs_compile_fail_tests` as reference, added the `bevy_reflect_compile_fail_tests` crate.

Currently, this crate contains a very simple test case. This is so that we can get the basic foundation of this crate agreed upon and merged so that more tests can be added by other PRs.

### Open Questions

- [x] Should this be added to CI? (Answer: Yes)

---

## Changelog

- Added the `bevy_reflect_compile_fail_tests` crate for testing compilation errors
@MrGVSV
Copy link
Member Author

MrGVSV commented Jan 3, 2023

Added two more things:

  • Compile tests for some scenarios
  • Compile-time assertions that ensure a given field is using the correct remote wrapper type

@MrGVSV MrGVSV force-pushed the reflect-remote branch 2 times, most recently from 65b1c56 to 3462335 Compare January 3, 2023 05:46
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

There isn't really a way to test that code using bevy_reflect compiles or doesn't compile for certain scenarios. This would be especially useful for macro-centric PRs like bevyengine#6511 and bevyengine#6042.

## Solution

Using `bevy_ecs_compile_fail_tests` as reference, added the `bevy_reflect_compile_fail_tests` crate.

Currently, this crate contains a very simple test case. This is so that we can get the basic foundation of this crate agreed upon and merged so that more tests can be added by other PRs.

### Open Questions

- [x] Should this be added to CI? (Answer: Yes)

---

## Changelog

- Added the `bevy_reflect_compile_fail_tests` crate for testing compilation errors
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

There isn't really a way to test that code using bevy_reflect compiles or doesn't compile for certain scenarios. This would be especially useful for macro-centric PRs like bevyengine#6511 and bevyengine#6042.

## Solution

Using `bevy_ecs_compile_fail_tests` as reference, added the `bevy_reflect_compile_fail_tests` crate.

Currently, this crate contains a very simple test case. This is so that we can get the basic foundation of this crate agreed upon and merged so that more tests can be added by other PRs.

### Open Questions

- [x] Should this be added to CI? (Answer: Yes)

---

## Changelog

- Added the `bevy_reflect_compile_fail_tests` crate for testing compilation errors
@robtfm robtfm mentioned this pull request Feb 3, 2023
2 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2023

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

@alice-i-cecile
Copy link
Member

I like the last minute reflect remote value addition! Let's get this merged :)

auto-merge was automatically disabled August 12, 2024 18:43

Head branch was pushed to by a user without write access

auto-merge was automatically disabled August 12, 2024 18:57

Head branch was pushed to by a user without write access

@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 6183b56 Aug 12, 2024
30 checks passed
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 D-Complex Quite challenging from either a design or technical perspective. Ask for help! D-Unsafe Touches with unsafe code in some way 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-Contentious There are nontrivial implications that should be thought through
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.