Skip to content

Conversation

@hymm
Copy link
Contributor

@hymm hymm commented Dec 2, 2025

Objective

  • Trying to doc most of the unsafe in the ecs crate so we can turn on unsafe_op_in_unsafe_fn.

Solution

  • Unfortunately reviewing the unsafe docs will probably not be trivial if we try to do it all in one pr. There are 400+ warnings when you turn on the lint. So we need to break it up as much as possible as reviewing the safety contracts in some cases isn't the easiest.
  • This pr includes two types of unsafe blocks.
    1. Blocks that already had safety comments, but were missing the unsafe {} block.
    2. Unsafe functions that have the same safety contract as their unsafe parent function and are very short. (Usually just the call to the function).

@BD103 BD103 added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events D-Straightforward Simple bug fixes and API improvements, docs, test and examples D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 3, 2025
entity: Entity,
) -> Result<D::Item<'w, '_>, QueryEntityError> {
self.query_unchecked(world).get_inner(entity)
// SAFETY: Upheld by caller
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm of the opinion that we should explicitly state what safety invariants are being upheld here, that way if those invariants change, for whatever reason, they'll be propagated up during code review, and a desynchronization can be seen within the function definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I keep going back on forth on this. I was actually going to do it the way you suggested at first. But while working on this I've looked through a lot of the existing safety comments and I see a lot of slightly different wording between the safety comment on the inner function and the parent function. This makes it not 100% clear that the safety is being transparently passed or being modified slightly. The mismatch actually makes it harder to verify that the safety contracts are being upheld. So this approach avoids that problem at the cost of what you're pointing out. I really wish we had rust-lang/rfcs#3842, which would fix this issue. I'd appreciate some other opinions here.

There also a lot cases like this https://github.com/bevyengine/bevy/pull/22014/files#diff-5625b402c72b0ac8efddfa3a2b8c1dc873e93f23cb28fc7f639c42e17e2011a2L253 where the safety invariants are on the trait definiion. And the method call is just another instance of the trait method. I think I'll change these to say "SAFETY: This method is called by the same trait method and so has the same safety contract."

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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants