Skip to content

Fix QueryData derive codegen #19750

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
Merged
merged 1 commit into from
Jun 20, 2025
Merged

Fix QueryData derive codegen #19750

merged 1 commit into from
Jun 20, 2025
+1 −1

Conversation

ecoskey
Copy link
Contributor

@ecoskey ecoskey commented Jun 20, 2025 •
edited
Loading

Custom derived QueryData impls currently generate Item structs with the lifetimes swapped, which blows up the borrow checker sometimes.

See: https://discord.com/channels/691052431525675048/749335865876021248/1385509416086011914

could add a regression test, TBH I don't know the error well enough to do that minimally. Seems like it's that both lifetimes on QueryData::Item need to be covariant, but I'm not sure.

@ecoskey ecoskey requested a review from Victoronz June 20, 2025 07:23
@ecoskey ecoskey added C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-ECS Entities, components, systems, and events labels Jun 20, 2025
Victoronz
Victoronz approved these changes Jun 20, 2025
View reviewed changes
Copy link
Contributor

@Victoronz Victoronz left a comment

Choose a reason for hiding this comment

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

Off by one error, begone!

@tychedelia tychedelia 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-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 20, 2025
Bluefinger
Bluefinger approved these changes Jun 20, 2025
View reviewed changes
Copy link
Contributor

@Bluefinger Bluefinger left a comment

Choose a reason for hiding this comment

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

This will fix the issue I encountered, many thanks!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 20, 2025
Merged via the queue into bevyengine:main with commit 8e1d005 Jun 20, 2025
43 checks passed
@ecoskey ecoskey deleted the query_data_macro_fix branch June 20, 2025 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bluefinger Bluefinger Bluefinger approved these changes

@janhohenheim janhohenheim janhohenheim approved these changes

@tychedelia tychedelia tychedelia approved these changes

@Victoronz Victoronz Victoronz approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ecoskey @Bluefinger @janhohenheim @tychedelia @Victoronz @alice-i-cecile