Forbid unsafe in most crates in the engine

[james7132]

Objective

Resolves #3824. unsafe code should be the exception, not the norm in Rust. It's obviously needed for various use cases as it's interfacing with platforms and essentially running the borrow checker at runtime in the ECS, but the touted benefits of Bevy is that we are able to heavily leverage Rust's safety, and we should be holding ourselves accountable to that by minimizing our unsafe footprint.

Solution

Deny unsafe_code workspace wide. Add explicit exceptions for the following crates, and forbid it in almost all of the others.

• bevy_ecs - Obvious given how much unsafe is needed to achieve performant results
• bevy_ptr - Works with raw pointers, even more low level than bevy_ecs.
• bevy_render - due to needing to integrate with wgpu
• bevy_window - due to needing to integrate with raw_window_handle
• bevy_utils - Several unsafe utilities used by bevy_ecs. Ideally moved into bevy_ecs instead of made publicly usable.
• bevy_reflect - Required for the unsafe type casting it's doing.
• bevy_transform - for the parallel transform propagation
• bevy_gizmos - For the SystemParam impls it has.
• bevy_assets - To support reflection. Might not be required, not 100% sure yet.
• bevy_mikktspace - due to being a conversion from a C library. Pending safe rewrite.
• bevy_dynamic_plugin - Inherently unsafe due to the dynamic loading nature.

Several uses of unsafe were rewritten, as they did not need to be using them:

• bevy_text - a case of Option::unchecked could be rewritten as a normal for loop and match instead of an iterator.
• bevy_color - the Pod/Zeroable implementations were replaceable with bytemuck's derive macros.

[bushrat011899]

Definitely a good change. Ideally, Bevy would contain zero unsafe code. While that may be impossible, it's still worth minimising the risk.

[alice-i-cecile]

😂 No targeted allows here.

[alice-i-cecile]

Good choice, this is the sort of needless unsafe I was hoping to clean up.

[alice-i-cecile]

@james7132 @JMS55 @rodolphito Meshlets added new unsafe code that will need to be allowed to get this mergeable.

[james7132]

@JMS55 is there a reason why PersistentGpuBufferable is an unsafe trait? Both of it's functions are not used in an unsafe context without existing safety checks.

[JMS55]

Iirc it's because there's no checks for it, but the data has to be 4 byte aligned (or whatever wgpu wants), and it needs to accurately report the size in one function that then gets written in the second.

[james7132]

If that's the case, the APIs on wgpu's end need to be unsafe, which I don't think is the case. As far as CPU-side memory safety goes, this looks safe to me, even if the implementation of the trait is wrong.

[BD103]

Looks good! What's your reason for forbid-ing many of the crates when they already inherit deny from the workspace?

[alice-i-cecile]

Forbid can't be locally allowed. In crates where we really really shouldn't need unsafe, it's a better choice.

[alice-i-cecile]

Merging is blocked on #12728

[james7132]

Allowing it for now and using #12743 to address the soundness issues.