AnyOf soundness fix #14013
Merged
AnyOf soundness fix #14013
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alice-i-cecile
added
C-Bug
alice-i-cecile
approved these changes
Jun 25, 2024
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
I prefer this solution :)
hymm
added
S-Ready-For-Final-Review
Member
|
@cBournhonesque can you amend your commit message to do that? Edit the commit message, and then force push. See https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors for the incantation. |
Co-authored-by: cBournhonesque <charlesbour@gmail.com> Co-authored-by: Gingeh <39150378+Gingeh@users.noreply.github.com>
# Objective - Fix issue bevyengine#13821 ## Solution - Rewrote the test to ensure that it actually tests the functionality correctly. Then made the par_read function correctly change the values of self.reader.last_event_count. ## Testing - Rewrote the test for par_read to run the system schedule twice, checking the output each time --------- Co-authored-by: Martín Maita <47983254+mnmaita@users.noreply.github.com>
cBournhonesque
force-pushed
the
cb/fix-anyof
branch
from
fb3d443 to
10c450e
Compare
Contributor
Author
|
I think I managed to update the authors in the commit; however a Macos tests fails for some reason? It doesn't use AnyOf |
Contributor
|
Something went wrong with the last rebase. Seeing changes in this pr that are unrelated. |
hymm
reviewed
Jun 25, 2024
alice-i-cecile
added
S-Waiting-on-Author
SkiFire13
reviewed
Jun 25, 2024
alice-i-cecile
added
S-Needs-Review
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
I'd like to see more tests: this is pretty gnarly and the consequences of mistakes are rough.
alice-i-cecile
approved these changes
Jun 25, 2024
alice-i-cecile
removed
the
S-Needs-Review
alice-i-cecile
added
the
S-Ready-For-Final-Review
mockersf
pushed a commit
that referenced
this pull request
Jun 26, 2024
# Objective Fixes #13993 PR inspired by #14007 to accomplish the same thing, but maybe in a clearer fashion. @Gingeh feel free to take my changes and add them to your PR, I don't want to steal any credit --------- Co-authored-by: Gingeh <39150378+Gingeh@users.noreply.github.com> Co-authored-by: Bob Gardner <rgardner@inworld.ai> Co-authored-by: Martín Maita <47983254+mnmaita@users.noreply.github.com>
Merged
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 27, 2024
# Objective - Fixes a correctness error introduced in #14013 ... ## Solution I've been playing around a lot of with the access code and I realized that I introduced a soundness error when trying to simplify the code. When we have a `Or<(With<A>, With<B>)>` filter, we cannot call ``` let mut intermediate = FilteredAccess::default(); $name::update_component_access($name, &mut intermediate); _new_access.append_or(&intermediate); ``` because that's just equivalent to adding the new components as `Or` clauses. For example if the existing `filter_sets` was `vec![With<C>]`, we would then get `vec![With<C>, With<A>, With<B>]` which translates to `A or B or C`. Instead what we want is `(A and B) or (A and C)`, so we need to have each new OR clause compose with the existing access like so: ``` let mut intermediate = _access.clone(); // if we previously had a With<C> in the filter_set, this will become `With<C> AND With<A>` $name::update_component_access($name, &mut intermediate); _new_access.append_or(&intermediate); ``` ## Testing - Added a unit test that is broken in main, but passes in this PR
mockersf
pushed a commit
that referenced
this pull request
Jun 27, 2024
# Objective - Fixes a correctness error introduced in #14013 ... ## Solution I've been playing around a lot of with the access code and I realized that I introduced a soundness error when trying to simplify the code. When we have a `Or<(With<A>, With<B>)>` filter, we cannot call ``` let mut intermediate = FilteredAccess::default(); $name::update_component_access($name, &mut intermediate); _new_access.append_or(&intermediate); ``` because that's just equivalent to adding the new components as `Or` clauses. For example if the existing `filter_sets` was `vec![With<C>]`, we would then get `vec![With<C>, With<A>, With<B>]` which translates to `A or B or C`. Instead what we want is `(A and B) or (A and C)`, so we need to have each new OR clause compose with the existing access like so: ``` let mut intermediate = _access.clone(); // if we previously had a With<C> in the filter_set, this will become `With<C> AND With<A>` $name::update_component_access($name, &mut intermediate); _new_access.append_or(&intermediate); ``` ## Testing - Added a unit test that is broken in main, but passes in this PR
zmbush
pushed a commit
to zmbush/bevy
that referenced
this pull request
Jul 3, 2024
# Objective - Fixes a correctness error introduced in bevyengine#14013 ... ## Solution I've been playing around a lot of with the access code and I realized that I introduced a soundness error when trying to simplify the code. When we have a `Or<(With<A>, With<B>)>` filter, we cannot call ``` let mut intermediate = FilteredAccess::default(); $name::update_component_access($name, &mut intermediate); _new_access.append_or(&intermediate); ``` because that's just equivalent to adding the new components as `Or` clauses. For example if the existing `filter_sets` was `vec![With<C>]`, we would then get `vec![With<C>, With<A>, With<B>]` which translates to `A or B or C`. Instead what we want is `(A and B) or (A and C)`, so we need to have each new OR clause compose with the existing access like so: ``` let mut intermediate = _access.clone(); // if we previously had a With<C> in the filter_set, this will become `With<C> AND With<A>` $name::update_component_access($name, &mut intermediate); _new_access.append_or(&intermediate); ``` ## Testing - Added a unit test that is broken in main, but passes in this PR
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Fixes #13993
PR inspired by #14007 to accomplish the same thing, but maybe in a clearer fashion.
@Gingeh feel free to take my changes and add them to your PR, I don't want to steal any credit