Skip to content

Model world borrow in Query more accurately#5588

Closed
BoxyUwU wants to merge 1 commit intobevyengine:mainfrom
BoxyUwU:query_world_borrows_small
Closed

Model world borrow in Query more accurately#5588
BoxyUwU wants to merge 1 commit intobevyengine:mainfrom
BoxyUwU:query_world_borrows_small

Conversation

@BoxyUwU
Copy link
Copy Markdown
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 😆

BoxyUwU
BoxyUwU commented Aug 5, 2022
View reviewed changes
@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
alice-i-cecile reviewed Aug 7, 2022
View reviewed changes
@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Aug 7, 2022

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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