Deny unsafe code in most of the code base

[alice-i-cecile]

What problem does this solve or what need does it fill?

While undeniably useful and often irreplaceable, unsafe code is inherently challenging to audit and maintain.

Uses of unsafe code outside of the core engine crate should not be necessary; safe APIs for missing functionality should be exposed instead.

What solution would you like?

Add #![deny(unsafe_code)] annotations to most bevy crates at the top of lib.rs.

There are a few clear exceptions:

• bevy_ecs
  • some submodules, like events, should probably forbid unsafe code
• bevy_crevice
• bevy_derive
• bevy_reflect

There are some surprising usages of unsafe that should be handled in one form or another.

• bevy_render: used for working with raw window handles
• bevy_dynamic_plugin + bevy_app
  • this functionality is, as far as I'm aware, entirely unused
• bevy_core: used for fixed timestep run criteria...
• bevy_transform: used for working with entity hierarchies (Entity-entity relations 🌈  #3742 please...)
• bevy_winit and bevy_ui: used to manually impl Send and Sync for external types
• bevy_tasks: used to transmute an executor lifetime to 'static
• bevy_log: used to log messages for Android

Each of these should be rewritten if feasible, or explicitly allowed if not.

Alternatives

We could use #![forbid(unsafe_code)] instead, but the ability to allow unsafe in carefully thought-out one-off instances is probably too valuable for many of these crates.

We could use #![warn(unsafe_code)] instead: keeping it at warning won't be a bother when you want to try things out locally, and Bevy CI will deny warnings.

[mockersf]

It should be at most #![warn(unsafe_code)]: keeping it at warning won't be a bother when you want to try things out locally, and Bevy CI will deny warnings.

I think this lint is better left to code reviews. It's unlikely unsafe code is going to be accepted without providing a reason for it anyway.

[DJMcNab]

Fwiw, I'm in favour of forbid, but then downgraded to warn (or deny broadly equivalently) if it's needed for that crate. The change from forbid to warn should also be a warning that this PR is doing something spooky.

My counterexample to it being code review would be

bevy/crates/bevy_core/src/time/fixed_timestep.rs

Line 193 in 4134577

unsafe fn run_unsafe(&mut self, _input: (), world: &World) -> ShouldRun {

This should (probably?) not be a manual System implementation. (I've not fixed that myself yet since I have other PRs open which would conflict IIRC, and honestly the whole fixed timestep solution needs a more thorough overhaul)

(That being said, this was added in a cart PR, so it's unclear how thoroughly it would have been reviewed anyway; would the change to warn have been caught?)

[alice-i-cecile]

#3893 removes the dynamic plugin functionality, and with it a large amount of unsafe.