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

Make QueryParIter::for_each_unchecked private #8848

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Jun 15, 2023

Objective

  • The function QueryParIter::for_each_unchecked is a footgun: the only ways to use it soundly can be done in safe code using for_each or for_each_mut. See this discussion on discord.

Solution

  • Make for_each_unchecked private.

Changelog

  • Removed QueryParIter::for_each_unchecked. All use-cases of this method were either unsound or doable in safe code using for_each or for_each_mut.

Migration Guide

The method QueryParIter::for_each_unchecked has been removed -- use for_each or for_each_mut instead. If your use case can not be achieved using either of these, then your code was likely unsound.

If you have a use-case for for_each_unchecked that you believe is sound, please open an issue.

@JoJoJet JoJoJet added A-ECS Entities, components, systems, and events P-Unsound A bug that results in undefined compiler behavior C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jun 15, 2023
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.

I agree with this analysis. Changelog would be clearer if it talked about how this is no longer public, rather than that it was removed IMO. Took me a bit to figure out exactly what happened looking at the diff!

@alice-i-cecile alice-i-cecile 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 Jun 15, 2023
@JoJoJet
Copy link
Member Author

JoJoJet commented Jun 15, 2023

My reasoning is that from the perspective of someone using bevy_ecs, this method has effectively been removed. I agree it's probably good to change the title though.

@JoJoJet JoJoJet changed the title Remove QueryParIter::for_each_unchecked Make QueryParIter::for_each_unchecked private Jun 15, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 15, 2023
Merged via the queue into bevyengine:main with commit 5291110 Jun 15, 2023
@JoJoJet JoJoJet deleted the par-for-each-unchecked branch June 15, 2023 20:34
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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide P-Unsound A bug that results in undefined compiler 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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants