neon icon indicating copy to clipboard operation
neon copied to clipboard

compile-time check to ensure `Results` from owned-buffers write path are handled

Open problame opened this issue 1 year ago • 4 comments

When I refactored the pageserver write path to used owned buffers, I used the style of api

fn write_...(buf: B) -> (B, Result<...>)

The problem is that this masks the #[must_use] on the Result: https://github.com/rust-lang/rust/issues/39524

We should figure out a way to preserve the #[must_use] warning somehow.

One option would be to move the B into the Result, i.e..

fn write_...(buf: B) -> Result<(B, ...), (B, ...)>

problame avatar Feb 14 '24 13:02 problame

One can make a custom tuple struct #[must_use] pub struct BufWithResult<B, T, E>(pub B, pub Result<T, E>).

arpad-m avatar Feb 14 '24 13:02 arpad-m

I don't understand how this is a problem, does it happen because we are crossing the crate boundaries? https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4809433abb7443bc9c7d7f5bde8b4756

koivunej avatar Feb 16 '24 09:02 koivunej

My understanding from the linked Rust issue is that, if you return a #[must_use] type inside a tuple, you lose the #[must_use] check in the caller, i.e., it's not infectious, and it doesn't survive destructuring.

problame avatar Feb 16 '24 10:02 problame

Discussed about this, the issue is about that the fact that (B, Result<T, E>) return type allows getting the buffer while not getting the warning on the result, like:

  1. let (buf, _res) = op();
  2. let buf = op().0;

But it one will still get must_use warning if the return type is unbound, which I think is working in all examples as intended and why the linked issue was closed. For me this seems like "it hurts when I do X -stop doing X" situation which doesn't necessarily need any complications.

koivunej avatar Feb 16 '24 11:02 koivunej

So, let's just close this?

problame avatar Apr 12 '24 10:04 problame