downcast_raw returns NonNull

[lenaschoenburg]

Motivation

Resolves #1073, quoting from there:

If we changed this to Option<NonNull<()>> we could benefit from niche optimization, saving us the option discriminant and making this return a single word pointer. That may in turn let us benefit from additional optimizations. This could make downcasting slightly faster.

Solution

I've made the change to the function signature and let rustc guide me through the process of fixing all callers. This was mostly straightforward but it is unsafe code so I'm not sure the changes here are correct.

[lenaschoenburg]

Seems like I'll need to use core::ptr::NonNull instead of std::ptr::NonNull.

[hawkw]

This looks great to me, thanks for working on it!

I had one major suggestion, which is that we could avoid the use of NonNull::new_unchecked by using the From<&T> impl for NonNull<T>. The difference is that new_unchecked is unsafe to call, because the pointer may be null, while From<&T> is safe, because &T is never null. In this code, all the uses of new_unchecked are in fact safe, because the raw pointers are always immediately constructed from a never-null &T, so it doesn't actually matter which one we use, but it might be a little nicer to have the additional safety net of making it clear that we are relying on Rust invariants here.

Up to you if you want to take that advice — this code is correct either way.

[lenaschoenburg]

I like all your suggestions and learned about cast so thanks for the great code review! I'll work on it again tomorrow.

[lenaschoenburg]

Okay, I think I applied all your suggestions. I also simplified .cast::<()>() to just .cast(), I think that's clear enough for code readers and it certainly is for rustc.

[hawkw]

🙌