Improve ThinSlicePtr API

[BD103]

Objective

• Although unsafe, ThinSlicePtr::get() doesn't signal that it doesn't perform bounds checking unless you are familiar with the API.
• ThinSlicePtr::get() takes self, not &self, meaning it consumes the ThinSlicePtr each time get() is called. This does not match the behavior of slice.get(), which takes &self.
  • This small issue was hidden by the fact that ThinSlicePtr implements Copy. This isn't a large issue because it all compiles down to the same machine code, but there's no point to get() requiring self.
• ThinSlicePtr could use better documentation, and could be improved a little in some areas.

Solution

• Rename ThinSlicePtr::get() to get_unchecked(), making the API mirror slice.get_unchecked(). The old get() is kept as a deprecated method, to be removed in 1.19.0.
• Make the new slice.get_unchecked() take &self rather than self.
• Add more documentation and slightly re-arrange code, while maintaining original behavior.

Testing

• Any unintentional changes should be caught by the Rust Compiler and Miri!

[BD103]

This is my first time writing a migration guide using the new system. (Crazy, right?!) Let me know if I did anything wrong :)

[kfc35]

non-blocking comment: I noticed you removed the #[cfg(debug_assertions)] line that was above this previously (in the deprecated function). With debug_assert!, as far as I can read from documentation, that line is actually not needed, right? It seems like there are other places in the codebase where this redundant line is placed above a debug_assert!

(new here and new to Rust!)

[BD103]

Yup! I removed the #[cfg(...)] because it was redundant. The reason is because debug_assert!() actually compiles down to:

if cfg!(debug_assertions) {
    assert!(...);
}

Adding #[cfg(debug_assertions)] to this if statement is redundant because we'd be checking if debug_assertions is enabled twice!

It seems like there are other places in the codebase where this redundant line is placed above a debug_assert!

Where else did you find cases like this? I can open up a follow-up issue for getting rid of them too. :)

[kfc35]

Hopefully this link works for you: https://github.com/search?q=repo%3Abevyengine%2Fbevy+%22%23%5Bcfg%28debug_assertions%29%5D%22+debug_assert%21&type=code (if not, you can search for usages via typing repo:bevyengine/bevy "#[cfg(debug_assertions)]" debug_assert!) in the search bar). You may have to expand the matches per file since GitHub likes to consolidate them.

It seems there are a few other places, although in my search I did find a useful comment in unsafe_world_cell.rs above one usage:

        // This annotation is needed because the
        // allows_mutable_access field doesn't exist otherwise.
        // Kinda weird, since debug_assert would never be called,
        // but CI complained in https://github.com/bevyengine/bevy/pull/17393

So I wonder if your pull request might run into this issue because self.len is also only defined in the debug config? Something to keep in mind if the CI complains about it

[BD103]

Actually, that's a really good point. I just tested it and my branch fails to check with --release:

error[E0609]: no field `len` on type `&ThinSlicePtr<'a, T>`
    --> crates/bevy_ptr/src/lib.rs:1097:36
     |
1097 |         debug_assert!(index < self.len, "tried to index out-of-bounds of a slice");
     |                                    ^^^ unknown field
     |
     = note: available fields are: `ptr`, `_marker`

I replaced debug_assert!() with a #[cfg(debug_assertions)] assert!() call instead, which does the same thing in debug mode but will skip compiling index < self.len in release mode. Thanks for pointing this out!

[kfc35]

New here so my approval may not mean as much, but it LGTM!