workingjubilee
commented
2 years ago One of the things I discovered while reading Postgres' source is that Postgres attempts to ereport before it passes a pointer out via palloc, so it will avoid null pointers except in truly disastrous situations (so, the ones that Rust should probably also start panicking in). This doesn't cover pointers from other cases, but it strongly suggests we can in fact tighten the ratchet and make PgBox resemble Box more, reducing impedance mismatches.
In order to handle the cases where it is in fact null, we could bubble that up to the programmer, changing
pub fn from_pg(*mut T) -> PgBox<T>into
pub fn from_pg(*mut T) -> Option<Box<T>>;
pub unsafe fn from_pg_unchecked(*mut T) -> PgBox<T>;In some cases, PgBox is handling genuinely-in-practice nullable pointers, and for those we may wish to introduce a PgPtr type to cover cases where pointers should maintain stronger invariants than the ones most raw pointers in Rust have, but much weaker invariants than the ones a Box typically maintains, and thus shouldn't offer quiiite the same nice, implicitly usable interface, but should still be easy to use. It may reduce ergonomics on some edge cases, but it's probably worth any correctness improvements we can pick up.
Doing this would also remove any potential complications from the current implementations of Deref and DerefMut on PgBox, as currently it is possible for either to fail, which is potentially bad, as Rust has various points where it will implicitly invoke deref or deref_mut, and if failure yields a panic then user code may suddenly panic due to a fairly hidden method call in a hard-to-debug way.
eeeebbbbrrrr
Much of the way Rust manages to be safe is by aggressively checking invariants at the first opportunity and then encoding the state of having performed these checks in the type, so as to guarantee that later usages are safe. While it gives Rust code an impression of being quite anxious, in practice this usually minimizes the overhead of panic code as it moves the panic to a single location.
This would improve user experience by revealing errors like pushing a null pointer into
PgBox<>, making them more obvious. It may not be completely clear we want to do this for pointers originated from Postgres, so maybe we should be more cautious with those. However, we should at least consider it, and we should definitely do this for any Rust pointers, as it will always be a user error to try to use a null pointer in a Box-like abstraction.