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

Fix relationship macro for multiple named members fields #18530

Merged
merged 5 commits into from
Mar 27, 2025

Conversation

krunchington
Copy link
Contributor

Objective

Fixes #18466

Solution

Updated the macro generation pattern to place the comma in the correct place in the pattern.

Testing

  • Tried named and unnamed fields in combination, and used rust expand macro tooling to see the generated code and verify its correctness (see screenshots in example below)

Showcase

Screenshot showing expanded macro with multiple named fields
image

Screenshot showing expanded macro with single unnamed field
image

Migration Guide

n/a

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Good fix, but needs a regression test please :)

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged D-Macros Code that generates Rust code labels Mar 25, 2025
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Mar 25, 2025
@krunchington
Copy link
Contributor Author

Good fix, but needs a regression test please :)

Gotcha I'm not familiar with testing macros but I'll poke around in this tmrw night if I can get time!

@alice-i-cecile
Copy link
Member

For a test, just add some dead code in a struct that checks that this compiles. Or use a doc test, which can be a bit nicer.

@krunchington
Copy link
Contributor Author

For a test, just add some dead code in a struct that checks that this compiles. Or use a doc test, which can be a bit nicer.

Got it, sounds straightforward!

Copy link

@CHATALOT1 CHATALOT1 left a comment

Choose a reason for hiding this comment

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

Having tested with my own similar error case, I believe the same fix is necessary on line 781 also to avoid the same bug occurring in the relationship_target macro.

@Bleachfuel
Copy link
Contributor

woops sorry, messed up when implementing it for enum's

Copy link
Contributor

@Bleachfuel Bleachfuel left a comment

Choose a reason for hiding this comment

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

Also for line 781

@krunchington
Copy link
Contributor Author

Having tested with my own similar error case, I believe the same fix is necessary on line 781 also to avoid the same bug occurring in the relationship_target macro.

Good catch!

@krunchington krunchington force-pushed the relationship-macro-fix branch from 10f06ef to 4f4b993 Compare March 26, 2025 02:55
@krunchington
Copy link
Contributor Author

Extra case handled in derive_relationship_target, and I added doc tests. I didn't put much extra doc content in them though because they're private and so don't show on the public crates docsite I believe? I also had to add bevy_ecs as a dev dependency to handle this case for the imports.

@krunchington
Copy link
Contributor Author

krunchington commented Mar 26, 2025

Hm, these CI failures don't look related to my change. Did I inherit them by rebasing against main?

They're all in the bevy_reflect crate which I didn't touch

@mockersf
Copy link
Member

Hm, these CI failures don't look related to my change. Did I inherit them by rebasing against main?

They're all in the bevy_reflect crate which I didn't touch

I think it's because you're changing a macro crate which are compiled differently. And you can't depend on bevy_ecs in it. so you should probably move your examples to bevy_ecs

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@krunchington krunchington force-pushed the relationship-macro-fix branch from 841a809 to d1497ac Compare March 27, 2025 02:25
@krunchington
Copy link
Contributor Author

I think it's because you're changing a macro crate which are compiled differently. And you can't depend on bevy_ecs in it. so you should probably move your examples to bevy_ecs

Thanks. I opted for actual test content rather than adding a bunch of cases to doc tests that might just be noise. Hoping this covers the cases appropriately but happy to adjust if folks have suggestions for alternate approaches!

@alice-i-cecile alice-i-cecile requested a review from mockersf March 27, 2025 04:48
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Mar 27, 2025
@mockersf mockersf added this pull request to the merge queue Mar 27, 2025
Merged via the queue into bevyengine:main with commit 83ffc90 Mar 27, 2025
33 checks passed
mockersf pushed a commit that referenced this pull request Mar 27, 2025
# Objective

Fixes #18466 

## Solution

Updated the macro generation pattern to place the comma in the correct
place in the pattern.

## Testing

- Tried named and unnamed fields in combination, and used rust expand
macro tooling to see the generated code and verify its correctness (see
screenshots in example below)

---

## Showcase

Screenshot showing expanded macro with multiple named fields

![image](https://github.com/user-attachments/assets/7ecd324c-10ba-4b23-9b53-b94da03567d3)

Screenshot showing expanded macro with single unnamed field

![image](https://github.com/user-attachments/assets/be72f061-5f07-4d19-b5f6-7ff6c35ec679)

## Migration Guide

n/a
@krunchington krunchington deleted the relationship-macro-fix branch March 28, 2025 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior D-Macros Code that generates Rust code D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relationship annotation does not allow more than additional field
5 participants