Rework the safety invariants of Global a bit
#750
Conversation
lilizoey
added
quality-of-life
There was a problem hiding this comment.
Thank you!
Previously it was relatively easy to add #![forbid(unsafe)] by just replacing unreachable_unchecked with unreachable!. Could this still technically be done, if we ever wanted to (e.g. new instead of new_unchecked, unwrap instead of unwrap_unchecked)?
| pub struct GlobalGuard<'a, T> { | ||
| mutex_guard: MutexGuard<'a, InitState<T>>, | ||
| } | ||
| mod global_guard { |
There was a problem hiding this comment.
I don't think this extra module is necessary, the separator is enough 🙂
There was a problem hiding this comment.
the extra module is there to ensure the mutex_guard field isn't accessible outside of a very limited set of functions. of course it isn't technically necessary, but by doing this we keep the safety critical logic more local since anything that wants to construct the GlobalGuard must go through either new or new_unchecked.
There was a problem hiding this comment.
I see, OK. Could you add this as a comment before mod?
Something like // Encapsulate private fields. or so is enough.
godot-ffi/src/global.rs
Outdated
| fn initialized(&self) -> Option<&T> { | ||
| match self { | ||
| InitState::Initialized(t) => t, | ||
| _ => { | ||
| // SAFETY: This method is only called from a guard, which can only be obtained in Initialized state. | ||
| unsafe { std::hint::unreachable_unchecked() } | ||
| } | ||
| InitState::Initialized(t) => Some(t), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
Nice change, as it moves the entire unsafety to the caller and doesn't make assumptions about where it's called from 👍
Name also makes sense, maybe as_initialized[_mut] would be even clearer, to indicate the reference return?
godot-ffi/src/global.rs
Outdated
| Poisoned { | ||
| circumvent: Option<GlobalGuard<'a, T>>, | ||
| }, |
There was a problem hiding this comment.
This weakens the API type safety towards the user -- I don't think there's a case where this can be None?
There was a problem hiding this comment.
i dont think so, but it is a bit hard to be 100% confident about it. The only time it can be None is if there is a panic when the InitState mutex is locked, but hasn't been set to Initialized yet, and also isn't caught by the catch_unwind. And while i dont think that can happen, it's rather involved control flow and i havent been able to fully convince myself that it definitely wont happen.
Another note is that it's also not actually accessed anywhere. the only place the Poisoned variant is used it also just discards the circumvent value.
Another option than using new_unchecked() would be new().expect() which makes it not (library) UB to create the GlobalGuard, but does throw a panic. However if we are pretty certain that this wont happen, and this error case is fairly rare anyway, then maybe that's just fine?.
There was a problem hiding this comment.
Generally I'd tend to make it as expressive as possible (i.e. not Option), and if our assumptions end up being wrong, treat it as a bug to be fixed.
I think new().expect() would be the safe choice here, retrieving poisoned guards shouldn't be the hot path. If it turns out that we limit an actual use case without None, then we'll have to make a (breaking) API change.
It's still relatively easy, the changes needed now would be:
|
lilizoey
force-pushed
the
refactor/global-safety
branch
from
98b2e22 to
50831ab
Compare
| /// | ||
| /// The value must be `Initialized`. | ||
| pub(super) unsafe fn new_unchecked(mutex_guard: MutexGuard<'a, InitState<T>>) -> Self { | ||
| debug_assert!(matches!(*mutex_guard, InitState::Initialized(_))); |
There was a problem hiding this comment.
this isnt necessary as unwrap_unchecked panics in debug already.
There was a problem hiding this comment.
the error message is different, and this also more explicitly lays out what the reconditioned is
Mainly moves the burden of proving that the
InitStateisInitializedto when theGlobalGuardis constructed. This includes adebug_assertthat theInitStateis in the correct state, which meant some otherdebug_asserts that did the same thing could be removed. I think this could be made entirely safe once mapped mutex guards stabilize.Also removes
TransientInitializingentirely since it was just used to get theinit_fnout of theInitState::Pending. However function pointers areCopyso we dont need to replace it with anything, and we have theInitStatelocked so it would never be observable anyway.