Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Tracked by #6663
problame opened this issue Feb 14, 2024 · 5 comments
Closed
Tracked by #6663
Assignees

Comments

@problame
Copy link
Contributor

problame commented Feb 14, 2024

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: rust-lang/rust#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, ...)>
@arpad-m
Copy link
Member

arpad-m commented Feb 14, 2024

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

@koivunej
Copy link
Member

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

@problame
Copy link
Contributor Author

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.

@koivunej
Copy link
Member

koivunej commented Feb 16, 2024

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.

@problame
Copy link
Contributor Author

So, let's just close this?

@jcsp jcsp closed this as completed Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants