Add ReadBufRef #93359

beepster4096 · 2022-01-27T03:53:29Z

Adds ReadBufRef, a wrapper around &mut ReadBuf that prevents its buffer from being swapped with a different buffer and makes Read::read_buf use it.

Fixes #93305

beepster4096 · 2022-01-27T04:03:23Z

I'm not sure if this is the best way to do this, it might be better to use a macro for the methods or even have the methods solely on ReadBuf.

Aaron1011 · 2022-01-29T02:55:15Z

r? @rust-lang/libs

joshtriplett · 2022-01-30T01:18:38Z

Would it be possible for all the &self methods to be replaced with a Deref impl?

Also, as a nit, things ending in Ref typically correspond to &; I would expect this to end in Mut.

beepster4096 · 2022-01-31T07:35:34Z

I don't think ReadBufMut or ReadBufRefMut are very good sounding names, maybe something around GuardedReadBuf, LockedReadBuf, WrappedReadBuf?

bors · 2022-02-07T23:42:37Z

☔ The latest upstream changes (presumably #87869) made this pull request unmergeable. Please resolve the merge conflicts.

nrc

Thanks for the PR! I think it is probably worth discussing the overall design here, as well as the naming. I'd really like to find a way to make this more ergonomic. At the moment, it seems like it is increasing friction for using the better API on Read significantly.

nrc · 2022-03-09T07:52:16Z

library/std/src/io/readbuf.rs

+
+/// A wrapper around [`&mut ReadBuf`](ReadBuf) which prevents the buffer that the `ReadBuf` points to from being replaced.
+#[derive(Debug)]
+pub struct ReadBufRef<'a, 'b> {


How necessary is it to have two lifetimes rather than one?

nrc · 2022-03-09T07:57:19Z

library/std/src/io/readbuf.rs

+
+impl<'a, 'b> ReadBufRef<'a, 'b> {
+    /// Creates a new `ReadBufRef` referencing the same `ReadBuf` as this one.
+    pub fn reborrow(&mut self) -> ReadBufRef<'_, 'b> {


This should be a Clone impl

This can't be a Clone impl.

Clone is &'r ReadBufRef<'a, 'b> -> ReadBufRef<'a, 'b>

this is &'r mut ReadBufRef<'a, 'b> -> ReadBufRef<'r, 'b>

nrc · 2022-03-09T08:12:20Z

library/std/src/io/readbuf.rs

+
+    /// Returns a mutable reference to the filled portion of the buffer.
+    #[inline]
+    pub fn filled_mut(&mut self) -> &mut [u8] {


I think rather than duplicating the API of ReadBuf, it would be better to Deref to it

Since the point is to avoid allowing use of mem::replace, that would defeat the purpose, no?

We can do things the other way around: keep only APIs on ReadBufRef and remove them from ReadBuf

Cons: probably a little less ergonomic? (need to call .borrow() all the time)

Pros: no API duplication

nrc · 2022-03-09T08:17:55Z

cc #78485

yoshuawuyts · 2022-03-09T10:25:53Z

library/std/src/io/readbuf.rs

+    /// Creates a new [`ReadBufRef`] referencing this `ReadBuf`.
+    #[inline]
+    pub fn borrow(&mut self) -> ReadBufRef<'_, 'a> {
+        ReadBufRef { read_buf: self }
+    }
+


Should this implement the Borrow trait instead?

I don't think it can. The borrow trait returns &Borrowed which not work for this API

If the name was changed to ReadBufGuard or similar as suggested above, both borrow() and reborrow() methods could be renamed to guard()?

I think borrow is the right name, following RefCell. We only use guard in the context of an RAII guard and this isn't following that pattern.

bors · 2022-04-06T04:02:35Z

☔ The latest upstream changes (presumably #95469) made this pull request unmergeable. Please resolve the merge conflicts.

WaffleLapkin · 2022-05-03T16:56:06Z

What's the state of this PR?

beepster4096 · 2022-05-04T18:33:50Z

There's unresolved discussion on what the ReadBuf api should look like. This PR's changes, while fixing the issue, are unergonomic to use

JohnCSimon · 2022-06-20T04:30:26Z

Looks like this is still waiting on more work from the author
@rustbot author

beepster4096 · 2022-06-20T06:20:53Z

Actually, I'm just going to close this in favor of #97015

std::io: migrate ReadBuf to BorrowBuf/BorrowCursor This PR replaces `ReadBuf` (used by the `Read::read_buf` family of methods) with `BorrowBuf` and `BorrowCursor`. The general idea is to split `ReadBuf` because its API is large and confusing. `BorrowBuf` represents a borrowed buffer which is mostly read-only and (other than for construction) deals only with filled vs unfilled segments. a `BorrowCursor` is a mostly write-only view of the unfilled part of a `BorrowBuf` which distinguishes between initialized and uninitialized segments. For `Read::read_buf`, the caller would create a `BorrowBuf`, then pass a `BorrowCursor` to `read_buf`. In addition to the major API split, I've made the following smaller changes: * Removed some methods entirely from the API (mostly the functionality can be replicated with two calls rather than a single one) * Unified naming, e.g., by replacing initialized with init and assume_init with set_init * Added an easy way to get the number of bytes written to a cursor (`written` method) As well as simplifying the API (IMO), this approach has the following advantages: * Since we pass the cursor by value, we remove the 'unsoundness footgun' where a malicious `read_buf` could swap out the `ReadBuf`. * Since `read_buf` cannot write into the filled part of the buffer, we prevent the filled part shrinking or changing which could cause underflow for the caller or unexpected behaviour. ## Outline ```rust pub struct BorrowBuf<'a> impl Debug for BorrowBuf<'_> impl<'a> From<&'a mut [u8]> for BorrowBuf<'a> impl<'a> From<&'a mut [MaybeUninit<u8>]> for BorrowBuf<'a> impl<'a> BorrowBuf<'a> { pub fn capacity(&self) -> usize pub fn len(&self) -> usize pub fn init_len(&self) -> usize pub fn filled(&self) -> &[u8] pub fn unfilled<'this>(&'this mut self) -> BorrowCursor<'this, 'a> pub fn clear(&mut self) -> &mut Self pub unsafe fn set_init(&mut self, n: usize) -> &mut Self } pub struct BorrowCursor<'buf, 'data> impl<'buf, 'data> BorrowCursor<'buf, 'data> { pub fn clone<'this>(&'this mut self) -> BorrowCursor<'this, 'data> pub fn capacity(&self) -> usize pub fn written(&self) -> usize pub fn init_ref(&self) -> &[u8] pub fn init_mut(&mut self) -> &mut [u8] pub fn uninit_mut(&mut self) -> &mut [MaybeUninit<u8>] pub unsafe fn as_mut(&mut self) -> &mut [MaybeUninit<u8>] pub unsafe fn advance(&mut self, n: usize) -> &mut Self pub fn ensure_init(&mut self) -> &mut Self pub unsafe fn set_init(&mut self, n: usize) -> &mut Self pub fn append(&mut self, buf: &[u8]) } ``` ## TODO * ~~Migrate non-unix libs and tests~~ * ~~Naming~~ * ~~`BorrowBuf` or `BorrowedBuf` or `SliceBuf`? (We might want an owned equivalent for the async IO traits)~~ * ~~Should we rename the `readbuf` module? We might keep the name indicate it includes both the buf and cursor variations and someday the owned version too. Or we could change it. It is not publicly exposed, so it is not that important~~. * ~~`read_buf` method: we read into the cursor now, so the `_buf` suffix is a bit weird.~~ * ~~Documentation~~ * Tests are incomplete (I adjusted existing tests, but did not add new ones). cc rust-lang#78485, rust-lang#94741 supersedes: rust-lang#95770, rust-lang#93359 fixes rust-lang#93305

std::io: migrate ReadBuf to BorrowBuf/BorrowCursor This PR replaces `ReadBuf` (used by the `Read::read_buf` family of methods) with `BorrowBuf` and `BorrowCursor`. The general idea is to split `ReadBuf` because its API is large and confusing. `BorrowBuf` represents a borrowed buffer which is mostly read-only and (other than for construction) deals only with filled vs unfilled segments. a `BorrowCursor` is a mostly write-only view of the unfilled part of a `BorrowBuf` which distinguishes between initialized and uninitialized segments. For `Read::read_buf`, the caller would create a `BorrowBuf`, then pass a `BorrowCursor` to `read_buf`. In addition to the major API split, I've made the following smaller changes: * Removed some methods entirely from the API (mostly the functionality can be replicated with two calls rather than a single one) * Unified naming, e.g., by replacing initialized with init and assume_init with set_init * Added an easy way to get the number of bytes written to a cursor (`written` method) As well as simplifying the API (IMO), this approach has the following advantages: * Since we pass the cursor by value, we remove the 'unsoundness footgun' where a malicious `read_buf` could swap out the `ReadBuf`. * Since `read_buf` cannot write into the filled part of the buffer, we prevent the filled part shrinking or changing which could cause underflow for the caller or unexpected behaviour. ## Outline ```rust pub struct BorrowBuf<'a> impl Debug for BorrowBuf<'_> impl<'a> From<&'a mut [u8]> for BorrowBuf<'a> impl<'a> From<&'a mut [MaybeUninit<u8>]> for BorrowBuf<'a> impl<'a> BorrowBuf<'a> { pub fn capacity(&self) -> usize pub fn len(&self) -> usize pub fn init_len(&self) -> usize pub fn filled(&self) -> &[u8] pub fn unfilled<'this>(&'this mut self) -> BorrowCursor<'this, 'a> pub fn clear(&mut self) -> &mut Self pub unsafe fn set_init(&mut self, n: usize) -> &mut Self } pub struct BorrowCursor<'buf, 'data> impl<'buf, 'data> BorrowCursor<'buf, 'data> { pub fn clone<'this>(&'this mut self) -> BorrowCursor<'this, 'data> pub fn capacity(&self) -> usize pub fn written(&self) -> usize pub fn init_ref(&self) -> &[u8] pub fn init_mut(&mut self) -> &mut [u8] pub fn uninit_mut(&mut self) -> &mut [MaybeUninit<u8>] pub unsafe fn as_mut(&mut self) -> &mut [MaybeUninit<u8>] pub unsafe fn advance(&mut self, n: usize) -> &mut Self pub fn ensure_init(&mut self) -> &mut Self pub unsafe fn set_init(&mut self, n: usize) -> &mut Self pub fn append(&mut self, buf: &[u8]) } ``` ## TODO * ~~Migrate non-unix libs and tests~~ * ~~Naming~~ * ~~`BorrowBuf` or `BorrowedBuf` or `SliceBuf`? (We might want an owned equivalent for the async IO traits)~~ * ~~Should we rename the `readbuf` module? We might keep the name indicate it includes both the buf and cursor variations and someday the owned version too. Or we could change it. It is not publicly exposed, so it is not that important~~. * ~~`read_buf` method: we read into the cursor now, so the `_buf` suffix is a bit weird.~~ * ~~Documentation~~ * Tests are incomplete (I adjusted existing tests, but did not add new ones). cc rust-lang/rust#78485, rust-lang/rust#94741 supersedes: rust-lang/rust#95770, rust-lang/rust#93359 fixes #93305

This comment has been minimized.

Sign in to view

beepster4096 force-pushed the read_buf_ref branch from d147fd4 to 2d7a415 Compare January 27, 2022 04:43

rust-highfive assigned joshtriplett Jan 29, 2022

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 29, 2022

beepster4096 added 3 commits February 13, 2022 16:02

add readbufref

b1c0672

formatting

325455f

impl deref

f168e97

beepster4096 force-pushed the read_buf_ref branch from 321ed20 to f168e97 Compare February 14, 2022 00:02

beepster4096 mentioned this pull request Mar 8, 2022

Read::read_buf mutable reference to ReadBuf #94742

Closed

nrc reviewed Mar 9, 2022

View reviewed changes

nrc added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 9, 2022

yoshuawuyts reviewed Mar 9, 2022

View reviewed changes

nrc mentioned this pull request May 13, 2022

std::io: migrate ReadBuf to BorrowedBuf/BorrowedCursor #97015

Merged

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2022

beepster4096 closed this Jun 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ReadBufRef #93359

Add ReadBufRef #93359

beepster4096 commented Jan 27, 2022

beepster4096 commented Jan 27, 2022 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

Aaron1011 commented Jan 29, 2022

joshtriplett commented Jan 30, 2022

beepster4096 commented Jan 31, 2022

bors commented Feb 7, 2022

nrc left a comment

nrc Mar 9, 2022

nrc Mar 9, 2022

WaffleLapkin Mar 9, 2022

nrc Mar 9, 2022

conradludgate Mar 9, 2022

nrc Mar 9, 2022

WaffleLapkin Mar 9, 2022

nrc commented Mar 9, 2022

yoshuawuyts Mar 9, 2022

conradludgate Mar 9, 2022

conradludgate Mar 9, 2022

nrc Mar 9, 2022

bors commented Apr 6, 2022

WaffleLapkin commented May 3, 2022

beepster4096 commented May 4, 2022

JohnCSimon commented Jun 20, 2022

beepster4096 commented Jun 20, 2022

Add ReadBufRef #93359

Add ReadBufRef #93359

Conversation

beepster4096 commented Jan 27, 2022

beepster4096 commented Jan 27, 2022 • edited Loading

This comment has been minimized.

This comment has been minimized.

Aaron1011 commented Jan 29, 2022

joshtriplett commented Jan 30, 2022

beepster4096 commented Jan 31, 2022

bors commented Feb 7, 2022

nrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Mar 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 6, 2022

WaffleLapkin commented May 3, 2022

beepster4096 commented May 4, 2022

JohnCSimon commented Jun 20, 2022

beepster4096 commented Jun 20, 2022

beepster4096 commented Jan 27, 2022 •

edited

Loading