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

Cannot reflect struct with Option fields #4054

Closed
inodentry opened this issue Feb 27, 2022 · 3 comments
Closed

Cannot reflect struct with Option fields #4054

inodentry opened this issue Feb 27, 2022 · 3 comments
Labels
A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@inodentry
Copy link
Contributor

Bevy cannot derive Reflect on structs that contain Option<T> fields, even though T is Reflect.

This compiles:

#[derive(Component, Reflect)]
struct MyComponent {
    sz: Size<Val>,
}

(Size<Val> is Reflect)

This does not:

#[derive(Component, Reflect)]
struct MyComponent {
    sz: Option<Size<Val>>,
}
error[E0277]: the trait bound `bevy::prelude::Size<bevy::prelude::Val>: bevy::reflect::erased_serde::private::serde::Serialize` is not satisfied
 --> src/main.rs:3:21
  |
3 | #[derive(Component, Reflect)]
  |                     ^^^^^^^ the trait `bevy::reflect::erased_serde::private::serde::Serialize` is not implemented for `bevy::prelude::Size<bevy::prelude::Val>`
  |
  = note: required because of the requirements on the impl of `bevy::prelude::Reflect` for `std::option::Option<bevy::prelude::Size<bevy::prelude::Val>>`
  = note: required for the cast to the object type `dyn bevy::prelude::Reflect`
  = note: this error originates in the derive macro `Reflect` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `for<'de> bevy::prelude::Size<bevy::prelude::Val>: Deserialize<'de>` is not satisfied
 --> src/main.rs:3:21
  |
3 | #[derive(Component, Reflect)]
  |                     ^^^^^^^ the trait `for<'de> Deserialize<'de>` is not implemented for `bevy::prelude::Size<bevy::prelude::Val>`
  |
  = note: required because of the requirements on the impl of `bevy::prelude::Reflect` for `std::option::Option<bevy::prelude::Size<bevy::prelude::Val>>`
  = note: required for the cast to the object type `dyn bevy::prelude::Reflect`
  = note: this error originates in the derive macro `Reflect` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `bevy::prelude::Size<bevy::prelude::Val>: bevy::reflect::erased_serde::private::serde::Serialize` is not satisfied
 --> src/main.rs:3:21
  |
3 | #[derive(Component, Reflect)]
  |                     ^^^^^^^ the trait `bevy::reflect::erased_serde::private::serde::Serialize` is not implemented for `bevy::prelude::Size<bevy::prelude::Val>`
  |
  = note: required because of the requirements on the impl of `bevy::prelude::Reflect` for `std::option::Option<bevy::prelude::Size<bevy::prelude::Val>>`
  = note: required for the cast to the object type `(dyn bevy::prelude::Reflect + 'static)`
  = note: this error originates in the derive macro `Reflect` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `for<'de> bevy::prelude::Size<bevy::prelude::Val>: Deserialize<'de>` is not satisfied
 --> src/main.rs:3:21
  |
3 | #[derive(Component, Reflect)]
  |                     ^^^^^^^ the trait `for<'de> Deserialize<'de>` is not implemented for `bevy::prelude::Size<bevy::prelude::Val>`
  |
  = note: required because of the requirements on the impl of `bevy::prelude::Reflect` for `std::option::Option<bevy::prelude::Size<bevy::prelude::Val>>`
  = note: required for the cast to the object type `(dyn bevy::prelude::Reflect + 'static)`
  = note: this error originates in the derive macro `Reflect` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0599]: the method `clone_value` exists for enum `std::option::Option<bevy::prelude::Size<bevy::prelude::Val>>`, but its trait bounds were not satisfied
   --> src/main.rs:3:21
    |
3   | #[derive(Component, Reflect)]
    |                     ^^^^^^^ method cannot be called on `std::option::Option<bevy::prelude::Size<bevy::prelude::Val>>` due to unsatisfied trait bounds
    |
   ::: /home/iyes/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs:515:1
    |
515 | pub enum Option<T> {
    | ------------------ doesn't satisfy `_: bevy::prelude::Reflect`
    |
   ::: /home/iyes/.cargo/registry/src/github.com-1ecc6299db9ec823/bevy_math-0.6.0/src/geometry.rs:8:1
    |
8   | pub struct Size<T: Reflect + PartialEq = f32> {
    | ---------------------------------------------
    | |
    | doesn't satisfy `_: Deserialize<'de>`
    | doesn't satisfy `_: bevy::reflect::erased_serde::private::serde::Serialize`
    |
note: the following trait bounds were not satisfied because of the requirements of the implementation of `bevy::prelude::Reflect` for `_`:
      `bevy::prelude::Size<bevy::prelude::Val>: bevy::reflect::erased_serde::private::serde::Serialize`
      `bevy::prelude::Size<bevy::prelude::Val>: Deserialize<'de>`
   --> src/main.rs:3:21
    |
3   | #[derive(Component, Reflect)]
    |                     ^^^^^^^
4   | struct MyTest {
    |        ^^^^^^
    = note: this error originates in the derive macro `Reflect` (in Nightly builds, run with -Z macro-backtrace for more info)
@inodentry inodentry added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Feb 27, 2022
@Davier
Copy link
Contributor

Davier commented Feb 27, 2022

Reflect is implemented there for Option:

impl_reflect_value!(Option<T: Serialize + Clone + for<'de> Deserialize<'de> + Reflect + 'static>(Serialize, Deserialize));

Since it is an enum, we currently have to reflect "as a value", and implement Serialize and Deserialize.
Instead of implementing Serialize and Deserialize on Size<Val>, I think the easiest fix is to also reflect your component as a value:

#[derive(Component, Reflect, Serialize, Deserialize)]
#[reflect_value(Component, Serialize, Deserialize)]
struct MyComponent {
    sz: Option<Size<Val>>,
}

Eventually #1347 will fix this issue.

@alice-i-cecile
Copy link
Member

#4761 is the follow-up PR that will fix this.

bors bot pushed a commit that referenced this issue Jul 21, 2022
… generic types (#5197)

# Objective

Some generic types like `Option<T>`, `Vec<T>` and `HashMap<K, V>` implement `Reflect` when where their generic types `T`/`K`/`V` implement `Serialize + for<'de> Deserialize<'de>`.
This is so that in their `GetTypeRegistration` impl they can insert the `ReflectSerialize` and `ReflectDeserialize` type data structs.

This has the annoying side effect that if your struct contains a `Option<NonSerdeStruct>` you won't be able to derive reflect (#4054).

## Solution

- remove the `Serialize + Deserialize` bounds on wrapper types
  - this means that `ReflectSerialize` and `ReflectDeserialize` will no longer be inserted even for `.register::<Option<DoesImplSerde>>()`
- add `register_type_data<T, D>` shorthand for `registry.get_mut(T).insert(D::from_type<T>())`
- require users to register their specific generic types **and the serde types** separately like
```rust
        .register_type::<Option<String>>()
        .register_type_data::<Option<String>, ReflectSerialize>()
        .register_type_data::<Option<String>, ReflectDeserialize>()

```
I believe this is the best we can do for extensibility and convenience without specialization.


## Changelog

- `.register_type` for generic types like `Option<T>`, `Vec<T>`, `HashMap<K, V>` will no longer insert `ReflectSerialize` and `ReflectDeserialize` type data. Instead you need to register it separately for concrete generic types like so:
```rust
        .register_type::<Option<String>>()
        .register_type_data::<Option<String>, ReflectSerialize>()
        .register_type_data::<Option<String>, ReflectDeserialize>()
```

TODO: more docs and tweaks to the scene example to demonstrate registering generic types.
inodentry pushed a commit to IyesGames/bevy that referenced this issue Aug 8, 2022
… generic types (bevyengine#5197)

# Objective

Some generic types like `Option<T>`, `Vec<T>` and `HashMap<K, V>` implement `Reflect` when where their generic types `T`/`K`/`V` implement `Serialize + for<'de> Deserialize<'de>`.
This is so that in their `GetTypeRegistration` impl they can insert the `ReflectSerialize` and `ReflectDeserialize` type data structs.

This has the annoying side effect that if your struct contains a `Option<NonSerdeStruct>` you won't be able to derive reflect (bevyengine#4054).

## Solution

- remove the `Serialize + Deserialize` bounds on wrapper types
  - this means that `ReflectSerialize` and `ReflectDeserialize` will no longer be inserted even for `.register::<Option<DoesImplSerde>>()`
- add `register_type_data<T, D>` shorthand for `registry.get_mut(T).insert(D::from_type<T>())`
- require users to register their specific generic types **and the serde types** separately like
```rust
        .register_type::<Option<String>>()
        .register_type_data::<Option<String>, ReflectSerialize>()
        .register_type_data::<Option<String>, ReflectDeserialize>()

```
I believe this is the best we can do for extensibility and convenience without specialization.


## Changelog

- `.register_type` for generic types like `Option<T>`, `Vec<T>`, `HashMap<K, V>` will no longer insert `ReflectSerialize` and `ReflectDeserialize` type data. Instead you need to register it separately for concrete generic types like so:
```rust
        .register_type::<Option<String>>()
        .register_type_data::<Option<String>, ReflectSerialize>()
        .register_type_data::<Option<String>, ReflectDeserialize>()
```

TODO: more docs and tweaks to the scene example to demonstrate registering generic types.
james7132 pushed a commit to james7132/bevy that referenced this issue Oct 28, 2022
… generic types (bevyengine#5197)

# Objective

Some generic types like `Option<T>`, `Vec<T>` and `HashMap<K, V>` implement `Reflect` when where their generic types `T`/`K`/`V` implement `Serialize + for<'de> Deserialize<'de>`.
This is so that in their `GetTypeRegistration` impl they can insert the `ReflectSerialize` and `ReflectDeserialize` type data structs.

This has the annoying side effect that if your struct contains a `Option<NonSerdeStruct>` you won't be able to derive reflect (bevyengine#4054).

## Solution

- remove the `Serialize + Deserialize` bounds on wrapper types
  - this means that `ReflectSerialize` and `ReflectDeserialize` will no longer be inserted even for `.register::<Option<DoesImplSerde>>()`
- add `register_type_data<T, D>` shorthand for `registry.get_mut(T).insert(D::from_type<T>())`
- require users to register their specific generic types **and the serde types** separately like
```rust
        .register_type::<Option<String>>()
        .register_type_data::<Option<String>, ReflectSerialize>()
        .register_type_data::<Option<String>, ReflectDeserialize>()

```
I believe this is the best we can do for extensibility and convenience without specialization.


## Changelog

- `.register_type` for generic types like `Option<T>`, `Vec<T>`, `HashMap<K, V>` will no longer insert `ReflectSerialize` and `ReflectDeserialize` type data. Instead you need to register it separately for concrete generic types like so:
```rust
        .register_type::<Option<String>>()
        .register_type_data::<Option<String>, ReflectSerialize>()
        .register_type_data::<Option<String>, ReflectDeserialize>()
```

TODO: more docs and tweaks to the scene example to demonstrate registering generic types.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
… generic types (bevyengine#5197)

# Objective

Some generic types like `Option<T>`, `Vec<T>` and `HashMap<K, V>` implement `Reflect` when where their generic types `T`/`K`/`V` implement `Serialize + for<'de> Deserialize<'de>`.
This is so that in their `GetTypeRegistration` impl they can insert the `ReflectSerialize` and `ReflectDeserialize` type data structs.

This has the annoying side effect that if your struct contains a `Option<NonSerdeStruct>` you won't be able to derive reflect (bevyengine#4054).

## Solution

- remove the `Serialize + Deserialize` bounds on wrapper types
  - this means that `ReflectSerialize` and `ReflectDeserialize` will no longer be inserted even for `.register::<Option<DoesImplSerde>>()`
- add `register_type_data<T, D>` shorthand for `registry.get_mut(T).insert(D::from_type<T>())`
- require users to register their specific generic types **and the serde types** separately like
```rust
        .register_type::<Option<String>>()
        .register_type_data::<Option<String>, ReflectSerialize>()
        .register_type_data::<Option<String>, ReflectDeserialize>()

```
I believe this is the best we can do for extensibility and convenience without specialization.


## Changelog

- `.register_type` for generic types like `Option<T>`, `Vec<T>`, `HashMap<K, V>` will no longer insert `ReflectSerialize` and `ReflectDeserialize` type data. Instead you need to register it separately for concrete generic types like so:
```rust
        .register_type::<Option<String>>()
        .register_type_data::<Option<String>, ReflectSerialize>()
        .register_type_data::<Option<String>, ReflectDeserialize>()
```

TODO: more docs and tweaks to the scene example to demonstrate registering generic types.
@TheNeikos
Copy link
Contributor

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-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

No branches or pull requests

4 participants