Doc trivial ecs unsafe

[hymm]

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).

[james7132]

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.

[hymm]

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."

[zjp-CN]

It's glad to see safety tags are attractive to bevy!

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

As the author of the safety tags RFC and its prototype safety-tool, I recently got the unsafe call trees since Artisan-Lab/tag-std#84 for our draft tagged fork of Asterinas and Rust for Linux to help trace the propagation of safety properties and do better on writing/reviewing/auditing the safety documention and justification.

I'm presenting a talk this weekend. The slides are being iterated on and will be public as soon as possible. The idea may look like the following for this R4L function:

A tag spec is defined as:

[tag.PointToField]
args = [ "ptr", "field", "adt" ]
desc = "The {ptr} must be a pointer to a {field} in {adt}."

And you can see unsafe { &(*Group::<Parent>::container_of(this)).data } has two unsafe operations

• calling the unsafe container_of function
• dereferencing the raw pointer (and getting a reference)

The obligation of the unsafe call is delegated to the caller, while dereferencing is also guarenteed by the caller's ValidPtr property.

[chescock]

I'd appreciate some other opinions here.

This doesn't exactly address the question here, but my opinion is that mediocre safety comments that let us turn on unsafe_op_in_unsafe_fn are better than perfect safety comments in some places but no safety comments in others. Even bad safety comments would draw your attention to the right places! So I think we should charge ahead with merging these PRs, and then have a more leisurely debate elsewhere about which safety comment style is better.

I'll also note that the safety requirements for this particular function are pretty vague:

This does not check for mutable query correctness. To be safe, make sure mutable queries have unique access to the components they query.

So I'm not sure there is any way to write a precise comment here right now!

[chescock]

I think this one, in DeferredWorld::get_param, is also relying on init_access having registered the access correctly. I might either reword this to mention that, similar to <&World>::get_param, or punt it to a future PR in this series.

Well, I guess that's also true for the other get_param changes here, but those are all delegating both init_access and get_param so they feel a lot more like a pass-through.

[chescock]

I'd appreciate some other opinions here.

This doesn't exactly address the question here, but my opinion is that mediocre safety comments that let us turn on unsafe_op_in_unsafe_fn are better than perfect safety comments in some places but no safety comments in others. Even bad safety comments would draw your attention to the right places! So I think we should charge ahead with merging these PRs, and then have a more leisurely debate elsewhere about which safety comment style is better.

I'll also note that the safety requirements for this particular function are pretty vague:

This does not check for mutable query correctness. To be safe, make sure mutable queries have unique access to the components they query.

So I'm not sure there is any way to write a precise comment here right now!

[alice-i-cecile]

my opinion is that mediocre safety comments that let us turn on unsafe_op_in_unsafe_fn are better than perfect safety comments in some places but no safety comments in others.

I share this opinion. Boy do I want safety tags though.