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

Add mutual exclusion safety info on filter_fetch #9836

Merged

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Sep 18, 2023

Objective

Currently, in bevy, it's valid to do Query<&mut Foo, Changed<Foo>>.

This assumes that filter_fetch and fetch are mutually exclusive, because of the mutable reference to the tick that Mut<Foo> implies and the reference that Changed<Foo> implies. However nothing guarantees that.

Solution

Documenting this assumption as a safety invariant is the least thing.

In various WorldQuery implementations, we assume that `filter_fetch`
and `fetch` are mutually exclusive, however nothing guarentees that.
Documenting this assumption as a safety invariant is the least thing.
@nicopap nicopap added A-ECS Entities, components, systems, and events P-Unsound A bug that results in undefined compiler behavior C-Docs An addition or correction to our documentation labels Sep 18, 2023
@nicopap
Copy link
Contributor Author

nicopap commented Sep 18, 2023

Actually I reviewed the filter WorldQuery impls and didn't find any where this is assumed.

Nevermind I'm silly. See updated description

@hymm
Copy link
Contributor

hymm commented Sep 19, 2023

do we need to update the safety comment on the call sites for filter_fetch too?

@nicopap
Copy link
Contributor Author

nicopap commented Sep 19, 2023

There is already a bunch of missing safety comments in the call sites (because it's inside unsafe functions and no one bothered to do the unsafe blocks within unsafe functions), so it would be difficult to justify just adding the safety comment for the mutual exclusion rule.

@james7132 james7132 added this pull request to the merge queue Sep 19, 2023
Merged via the queue into bevyengine:main with commit 9e52697 Sep 19, 2023
25 checks passed
@nicopap nicopap deleted the query-fetch-filter-mut-excl-safety branch September 20, 2023 13:07
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

Currently, in bevy, it's valid to do `Query<&mut Foo, Changed<Foo>>`.

This assumes that `filter_fetch` and `fetch` are mutually exclusive,
because of the mutable reference to the tick that `Mut<Foo>` implies and
the reference that `Changed<Foo>` implies. However nothing guarantees
that.

## Solution

Documenting this assumption as a safety invariant is the least thing.
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-Docs An addition or correction to our documentation P-Unsound A bug that results in undefined compiler behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants