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.

Sign up for GitHub

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

Jump to bottom

Fixed derived reflect outputting incorrect where clause. Fixes #7989 #7991

Closed
wants to merge 4 commits into from
Closed

Fixed derived reflect outputting incorrect where clause. Fixes #7989 #7991

wants to merge 4 commits into from

Conversation

CoffeeVampir3
Copy link

Objective

  • Fixes a bug where derived reflect was emitting an incorrect where clause. Specifically, a comma was omitted. Though this fix works for the specific case I had, I'm not sure it is the correct fix in general. Please review the PR changes and ensure this is the correct place where the comma should be appended.
  • Fixes Issue with specific reflect breaking after upgrading from 0.9-0.10 #7989

Solution

  • Added the missing comma in the proc macro utility that builds the where clause. This is the only change.

Their is no change or migration required for this fix.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

Welcome, new contributor!

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

@alice-i-cecile alice-i-cecile added this to the 0.10.1 milestone Mar 9, 2023
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Reflection Runtime information about types labels Mar 9, 2023
@james7132
Copy link
Member

Do you mind adding a minimum reproduction to the tests in bevy_reflect to ensure that if this gets broken in the future, our tests fail to compile?

@CoffeeVampir3
Copy link
Author

CoffeeVampir3 commented Mar 9, 2023 •
edited
Loading

Do you mind adding a minimum reproduction to the tests in bevy_reflect to ensure that if this gets broken in the future, our tests fail to compile?

I have no problem doing this, but it appears from the test cases:

handle::Handle<T>: Reflect` is not satisfied
error: expected one of `{`, lifetime, or type, found `,`
   --> crates/bevy_asset/src/handle.rs:105:21
    |
105 | #[derive(Component, Reflect, FromReflect)]
    |                     ^^^^^^^ expected one of `{`, lifetime, or type
    |
    = note: this error originates in the derive macro `Reflect` (in Nightly builds, run with -Z macro-backtrace for more info)

Seems the fix also unfixed some things. Trying to reason about the differences here but I'm not seeing why these similar cases end up breaking in different ways.

@MrGVSV
Copy link
Member

MrGVSV commented Mar 9, 2023 •
edited
Loading

I think I got it working by doing the following:

  1. Updating the utility function to exclude the optional comma in the user-defined where clause: 
    let mut generic_where_clause = if let Some(where_clause) = where_clause {
    // This removes the optional, user-defined trailing comma from the equation
    let predicates = where_clause.predicates.iter();
    quote! {where #(#predicates,)*}
} else if !(active_types.is_empty() && ignored_types.is_empty()) {
    quote! {where}
} else {
    quote! {}
};
  2. Updating the FromReflect impls to use this function as well instead of providing that logic themselves: 
    let where_from_reflect_clause = extend_where_clause(
    where_clause,
    &WhereClauseOptions {
        active_types: reflect_struct.active_types().into_boxed_slice(),
        ignored_types: reflect_struct.ignored_types().into_boxed_slice(),
        active_trait_bounds: quote!(#bevy_reflect_path::FromReflect),
        ignored_trait_bounds: quote!(#FQDefault),
    },
);

MrGVSV
MrGVSV reviewed Mar 9, 2023
View reviewed changes
crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs

let where_from_reflect_clause = extend_where_clause(
where_clause,
&WhereClauseOptions {
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to import this type

crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs
#(#field_types: #bevy_reflect_path::FromReflect,)*
});

let where_from_reflect_clause = extend_where_clause(
Copy link
Member

Choose a reason for hiding this comment

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

This should also be done for enums as well

@MrGVSV MrGVSV mentioned this pull request Mar 10, 2023
Merged
@MrGVSV
Copy link
Member

MrGVSV commented Mar 10, 2023

Closing in favor of #8014

@MrGVSV MrGVSV closed this Mar 10, 2023
@mockersf mockersf removed this from the 0.10.1 milestone Mar 13, 2023
cart pushed a commit that referenced this pull request Mar 27, 2023
@MrGVSV
bevy_reflect: Fix trailing comma breaking derives (#8014)
5e5a305 
# Objective

Fixes #7989

Based on #7991 by @CoffeeVampir3

## Solution

There were three parts to this issue:
1. `extend_where_clause` did not account for the optionality of a where
clause's trailing comma
    ```rust
    // OKAY
    struct Foo<T> where T: Asset, {/* ... */}
    // ERROR
    struct Foo<T> where T: Asset {/* ... */}
    ```
2. `FromReflect` derive logic was not actively using
`extend_where_clause` which led to some inconsistencies (enums weren't
adding _any_ additional bounds even)
3. Using `extend_where_clause` in the `FromReflect` derive logic meant
we had to optionally add `Default` bounds to ignored fields iff the
entire item itself was not already `Default` (otherwise the definition
for `Handle<T>` wouldn't compile since `HandleType` doesn't impl
`Default` but `Handle<T>` itself does)

---

## Changelog

- Fixed issue where a missing trailing comma could break the reflection
derives
mockersf pushed a commit that referenced this pull request Mar 27, 2023
@MrGVSV @mockersf
bevy_reflect: Fix trailing comma breaking derives (#8014)
d1a6e91 
# Objective

Fixes #7989

Based on #7991 by @CoffeeVampir3

## Solution

There were three parts to this issue:
1. `extend_where_clause` did not account for the optionality of a where
clause's trailing comma
    ```rust
    // OKAY
    struct Foo<T> where T: Asset, {/* ... */}
    // ERROR
    struct Foo<T> where T: Asset {/* ... */}
    ```
2. `FromReflect` derive logic was not actively using
`extend_where_clause` which led to some inconsistencies (enums weren't
adding _any_ additional bounds even)
3. Using `extend_where_clause` in the `FromReflect` derive logic meant
we had to optionally add `Default` bounds to ignored fields iff the
entire item itself was not already `Default` (otherwise the definition
for `Handle<T>` wouldn't compile since `HandleType` doesn't impl
`Default` but `Handle<T>` itself does)

---

## Changelog

- Fixed issue where a missing trailing comma could break the reflection
derives
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrGVSV MrGVSV MrGVSV left review comments

Assignees
No one assigned
Labels
A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Issue with specific reflect breaking after upgrading from 0.9-0.10
5 participants
@CoffeeVampir3 @james7132 @MrGVSV @alice-i-cecile @mockersf