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

Model world borrow in Query more accurately #5588

Closed
wants to merge 1 commit into from
Closed

Model world borrow in Query more accurately #5588

wants to merge 1 commit into from

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Aug 5, 2022

Objective

Query is an extremely error prone type to implement things on as the &'world World field lets safe code do lots of unsound things with.

Solution

newtype the &'world World and provide an API that more closely matches what the &'world World is representing (See module and type level docs on QueryWorldBorrows for more info)

This diff mostly ended up being doc comments and safety comments 😆

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Aug 6, 2022
@BoxyUwU BoxyUwU force-pushed the query_world_borrows_small branch from ac8e54d to 5a08b7d Compare August 6, 2022 10:52
@alice-i-cecile
Copy link
Member

I like this direction a lot; I found the lifetimes here incredibly confusing and error prone when working on PRs that modify Query.

@BoxyUwU BoxyUwU mentioned this pull request Aug 7, 2022
Closed
@jakobhellermann jakobhellermann mentioned this pull request Sep 12, 2022
Closed
@alice-i-cecile alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Jan 8, 2023
@BoxyUwU BoxyUwU force-pushed the query_world_borrows_small branch from 5a6da79 to c51ebdf Compare January 9, 2023 09:43
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Jan 9, 2023

for a reason why this PR is important see: #6214 which added a Debug impl to Query which passes an &World out to safe code which could very quickly turn into unsoundness if someone helpfully makes the Debug impl on world try to show you components. With this PR it requires a line of unsafe code which helpfully points out to me that we need to be careful about what gets accessed from the &World and I can very quickly see that we are not

@BoxyUwU BoxyUwU added the P-Unsound A bug that results in undefined compiler behavior label Jan 9, 2023
@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Jan 9, 2023
@BoxyUwU BoxyUwU force-pushed the query_world_borrows_small branch 3 times, most recently from 465bce6 to 7a3df82 Compare January 11, 2023 10:04
@BoxyUwU BoxyUwU force-pushed the query_world_borrows_small branch from 7a3df82 to 4da01d4 Compare January 11, 2023 10:11
@BoxyUwU BoxyUwU marked this pull request as draft January 11, 2023 10:34
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Jan 11, 2023

(going to put this as a draft PR til I rewrite some doc comments)

@BoxyUwU BoxyUwU removed this from the 0.10 milestone Feb 13, 2023
@BoxyUwU BoxyUwU added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Mar 3, 2023
@BoxyUwU BoxyUwU closed this Mar 24, 2023
@CGMossa
Copy link
Contributor

CGMossa commented Mar 24, 2023

Why was this closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Complex Quite challenging from either a design or technical perspective. Ask for help! P-Unsound A bug that results in undefined compiler behavior S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BoxyUwU @alice-i-cecile @CGMossa