[Merged by Bors] - Fix unsoundness for propagate_recursive
#7003
Conversation
james7132
added
A-Hierarchy
crates/bevy_transform/src/systems.rs
Outdated
| *child, | ||
| changed, | ||
| ); | ||
| // SAFETY: Assuming the hierarchy is consistent, we can be sure that each `child` entity |
There was a problem hiding this comment.
Thinking on this a bit more. I'm still not satisfied with this safety comment. We can't assume that the hierarchy is consistent and tree/forest-like. That's what the assertion in the recursive function is for. Not sure what the best wording here is, since the validation for the safety invariant is asserted (recursively) in the called function itself, but is only safe under the assumption that individual entire trees are not aliased from the roots.
There was a problem hiding this comment.
We could document propagate_recursive with the invariant that it will panic if the hierarchy is malformed, and then cite that invariant in this safety comment. Does that seem right?
There was a problem hiding this comment.
We should definitely document the panic on propagate_recursive, but the safety justification here is only valid if both the uniqueness of the roots AND the panic are included. The safety guarantee requires both the hierarchy to be not malformed (or panic if it is), and for the tree from the root down to be uniquely accessed from a single thread for it to be valid.
There was a problem hiding this comment.
I ended up rewriting this comment and the docs for propagate_recursive entirely. Lmk if thats an improvement.
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
bors r+ |
# Objective Fix #6983. ## Solution Mark the function `propagate_recursive` as unsafe, and specify the safety invariants through doc comments.
|
Build failed (retrying...): |
# Objective Fix #6983. ## Solution Mark the function `propagate_recursive` as unsafe, and specify the safety invariants through doc comments.
bors
bot
changed the title
propagate_recursivepropagate_recursive
# Objective Fix bevyengine#6983. ## Solution Mark the function `propagate_recursive` as unsafe, and specify the safety invariants through doc comments.
# Objective Fix bevyengine#6983. ## Solution Mark the function `propagate_recursive` as unsafe, and specify the safety invariants through doc comments.
Objective
Fix #6983.
Solution
Mark the function
propagate_recursiveas unsafe, and specify the safety invariants through doc comments.