Skip to content

bevy_reflect: Fix combined field attributes#9322

Merged
alice-i-cecile merged 2 commits intobevyengine:mainfrom
MrGVSV:reflect-fix-multiple-field-attributes
Aug 7, 2023
Merged

bevy_reflect: Fix combined field attributes#9322
alice-i-cecile merged 2 commits intobevyengine:mainfrom
MrGVSV:reflect-fix-multiple-field-attributes

Conversation

@MrGVSV
Copy link
Copy Markdown
Member

@MrGVSV MrGVSV commented Jul 31, 2023

Objective

It seems the behavior of field attributes was accidentally broken at some point. Take the following code:

#[derive(Reflect)]
struct Foo {
  #[reflect(ignore, default)]
  value: usize
}

The above code should simply mark value as ignored and specify a default behavior. However, what this actually does is discard both. That's especially a problem when we don't want the field to be be given a Reflect or FromReflect bound (which is why we ignore it in the first place).

This only happens when the attributes are combined into one. The following code works properly:

#[derive(Reflect)]
struct Foo {
  #[reflect(ignore)]
  #[reflect(default)]
  value: usize
}

Solution

Cleaned up the field attribute parsing logic to support combined field attributes.


Changelog

  • Fixed a bug where Reflect derive attributes on fields are not able to be combined into a single attribute

@MrGVSV MrGVSV added C-Bug An unexpected or incorrect behavior A-Reflection Runtime information about types labels Jul 31, 2023
@MrGVSV MrGVSV added this to the 0.11.1 milestone Jul 31, 2023
@MrGVSV MrGVSV force-pushed the reflect-fix-multiple-field-attributes branch from a727456 to fa277e3 Compare July 31, 2023 20:39
@MrGVSV MrGVSV force-pushed the reflect-fix-multiple-field-attributes branch from fa277e3 to 3276660 Compare July 31, 2023 20:53
Copy link
Copy Markdown
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

LGTM

@mockersf mockersf 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 Aug 2, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 7, 2023
Merged via the queue into bevyengine:main with commit 84f6b87 Aug 7, 2023
cart pushed a commit that referenced this pull request Aug 10, 2023
# Objective

It seems the behavior of field attributes was accidentally broken at
some point. Take the following code:

```rust
#[derive(Reflect)]
struct Foo {
  #[reflect(ignore, default)]
  value: usize
}
```

The above code should simply mark `value` as ignored and specify a
default behavior. However, what this actually does is discard both.
That's especially a problem when we don't want the field to be be given
a `Reflect` or `FromReflect` bound (which is why we ignore it in the
first place).

This only happens when the attributes are combined into one. The
following code works properly:

```rust
#[derive(Reflect)]
struct Foo {
  #[reflect(ignore)]
  #[reflect(default)]
  value: usize
}
```

## Solution

Cleaned up the field attribute parsing logic to support combined field
attributes.

---

## Changelog

- Fixed a bug where `Reflect` derive attributes on fields are not able
to be combined into a single attribute
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

No open projects
Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants