Panic safe component drop#24393
Open
SpecificProtagonist wants to merge 7 commits into
Open
Conversation
SpecificProtagonist
added
C-Bug
Bluefinger
reviewed
May 22, 2026
| last_element_index: usize, | ||
| row: TableRow, | ||
| ) { | ||
| self.added_ticks |
Contributor
There was a problem hiding this comment.
If we have unchecked/unsafe code that we are touching, we should be trying to gradually wrapping these with unsafe blocks, so we can eventually enable the unsafe_op_in_unsafe_fn lint.
Contributor
Author
There was a problem hiding this comment.
This PR doesn't really affect existing safety requirements, so adding extra unsafe blocks around existing unsafe code would just make this PR harder to review. I'll do a separate PR for more unsafe_op_in_unsafe_fn though :)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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 #2860
bevy_ecs is not panic-safe in regards to dropping components. Note that #12929 has since made some cases safe by permanently disabling the drop function in some cases, but others remained.
See also #20368 for lack of panic safety in required component constructors. The fix (#20933) to that differs a bit in that a panic in a required component constructor means we have to move the entity to a different archetype than originally planned, which is a bit more involved.
Solution
When a panic occurs while making changes that must be atomic (such as editing entity metadata or moving around component data), catch any panics originating from component drop functions and resume unwinding at the end.
While I've added a panics section to the doc comments of relevant internal functions, I have not done so for any of the safe wrappers.
Testing
Added the
panic_in_component_droptest based on the linked issue.