std::io: migrate ReadBuf to BorrowedBuf/BorrowedCursor

[nrc]

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

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 #78485, #94741
supersedes: #95770, #93359
fixes #93305

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[bors]

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

[danielhenrymantilla]

Could you provide, in the PR description, a summary of the new resulting API, without the implementation details? That would be quite useful to skim through the changes 🙏

Here is my own take with a basic grep:

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<'b>(&'b mut self) -> BorrowCursor<'a, 'b> {
    pub fn clear(&mut self) -> &mut Self {
    pub unsafe fn set_init(&mut self, n: usize) -> &mut Self {
impl<'a, 'b> BorrowCursor<'a, 'b> {
    pub fn clone<'c>(&'c mut self) -> BorrowCursor<'a, 'c> {
    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]) {

• From there; could you rename 'a to 'buf, and 'b to 'cursor ?
  • and change the order between 'a and 'b (smaller lifetime goes first to mimic the &'outer &'inner … syntax of references)
  • and clone/'c to {,'}reborrow ?
  • Ideally, since we are dealing with a cursor / ref type, the methods would be self-based, and the reborrowing method would be used to only borrow the original one, much like with .as_mut() and Pin<&mut >. This leads to the ".r()-shorthand-for-.reborrow()ing idiom". All this could avoid issues with temporaries in some cases:

    // without `self`-based APIs, this would lead to a "`.unfilled()`-temporary dropped" issue
    let buf = borrow_buf.unfilled().ensure_init().init_mut(); // this ought to be typical

---

Finally, it could be envisioned to add impl<'cursor, 'buf> Deref for BorrowCursor<'cursor, 'buf> { type Target = BorrowBuf<'buf>; so as to move some of the _ref methods (such as init_ref()) to BorrowBuf (even though this would go against the aforementioned self-receiver guideline);

• in any case, it looks like .init_{ref,mut}() could be added to BorrowBuf?

[nrc]

@danielhenrymantilla

From there; could you rename 'a to 'buf, and 'b to 'cursor ?

I like the idea of renaming the lifetimes to make them more explanatory, but I'm not sure about this scheme. IMO 'a is the lifetime of the underlying data and 'b is the lifetime of the buffer, but since buffer is such an overloaded term it could be interpreted to mean the underlying data or the BorrowBuf.

and change the order between 'a and 'b

This seems reasonable though I'm not sure I've come across it explicitly, is it a documented pattern?

and clone/'c to {,'}reborrow ?
Ideally, since we are dealing with a cursor / ref type, the methods would be self-based, and the reborrowing method would be used to only borrow the original one, much like with .as_mut() and Pin<&mut >. This leads to the ".r()-shorthand-for-.reborrow()ing idiom". All this could avoid issues with temporaries in some cases:

I personally find the reborrow terminology confusing - it seems very jargon-y and we don't use it anywhere in std. I was pretty unhappy with clone because the signature is not the same as a regular clone, but when using the method the reason is nearly always that you want to clone the cursor (i.e., you just want two cursors) rather than because you want to alter the lifetime (that is just a side-effect due to the borrow checker). Again, the r for reborrow idiom is not currently used in std. Is it common? Is it a convention we should encourage in std?

Finally, it could be envisioned to add impl<'cursor, 'buf> Deref for BorrowCursor<'cursor, 'buf> { type Target = BorrowBuf<'buf>;

It is not currently possible to get access to the BorrowBuf from the cursor and I that is a feature not a bug. It constrains callees to having write-only access to the buffer.

in any case, it looks like .init_{ref,mut}() could be added to BorrowBuf?

Likewise, the buffer has read-only access to the buffer (and API for initialisation), the way to get access to the initialised part of the buffer is to get a buffer. This is key to having smaller (hopefully clearer) APIs.

[nrc]

Ideally, since we are dealing with a cursor / ref type, the methods would be self-based,

So the drawback of this approach is that it prevents using the style of:

fn foo(cursor: BorrowCursor<'_, '_>) {
    cursor.ensure_init();
   // use the cursor
}

I'm not sure of the relative value of being able to do this vs the temporaries issues when using the builder pattern. Are there guidelines we follow for this self -> Self vs &mut self -> &mut self trade-off?

[Yurihaia]

Making BorrowCursor take self would force pervasive use of a reborrowing function, which would significantly decrease ergonomics. The only gain is the ability to omit a let binding, but I'm willing to say that the cursor is already bound through a function parameter in 90%+ of use cases. In the cases where you would need a let binding, you already have a BorrowBuf which means that buffer initialization functions can just be added directly to that.

[danielhenrymantilla]

Sorry for the delay in my answer, @nrc, and thank you for taking my input into consideration 🙏

lifetime names (and position)

buffer is such an overloaded term

Ah interesting; I was seeing buf as referring to the byte slice, but you use it to refer to the borrow of that byte slice, which is indeed just as legitimate. So I don't mind 'b then being the 'buf part, with 'a becoming 'data:

pub fn unfilled<'buf>(self: &'buf mut BorrowBuf<'data>) -> BorrowCursor<'buf, 'data>

• But the current 'this approach in your OP seems fine too, thanks for the rename!

---

[regarding the ordering of lifetime params]

I'm not sure I've come across it explicitly, is it a documented pattern?

Not that I know of, but having a choice of pattern be made consistently is useful, and this ordering is the most suitable one, since:

• Thing<&'lt T> can easily be refactored from/into ThingRef<'lt, T>
• and thus, more generally, Thing<'a, &'b T> from/into ThingRef<'a, 'b, T>

But just a very tiny nit, of course 🙂

---

clone or reborrow?

Is it common? Is it a convention we should encourage in std?

It's very uncommon indeed. But I think it being that uncommon plays a non-negligible role in its own demise, sort to speak: something is jargony if used rarely / only between the savy people. Things such as rust-lang/reference#788 or even this very recent post https://haibane-tenshi.github.io/rust-reborrowing hint at somewhere, in Rust, missing clearer documentation of reborrowing:

• its formal definition (I have to thank that post for bringing https://docs.rs/reborrow/0.2.0/reborrow/trait.ReborrowMut.html to my attention, which perfectly matches the signature here in question);
• how it implicitly occurs with &muts;
• how one would write lifetime-infected data types that would mimic this behavior.

In the stdlib, there is one instance where this explicit reborrowing comes in play:

impl<'orig> Pin<&'orig mut T> {
    fn as_mut<'r>(self: &'r mut Pin<&'orig mut T>) -> Pin<&'r mut T>

• the real impl is more generic and takes any P : DerefMut, so this reborrowing signature is even hidden behind generics 😅 (it's the P = &'orig mut T case).

and is indeed the reason for needing these pesky .as_mut()s calls when dealing with Pins.

See also:

• Suggest as_mut to reborrow Pin when calling method #65409
• Suggest as_mut when Pin<T> is used after move #93181

So what I think would benefit the language would be for the Rust book, or some other documentation to be more forthcoming about the reborrowing of &muts, which is deeply related to the common beginner-ish surprise of: "implementing Iter is easy, but IterMut is not" (&'short mut &'long mut [T] only yields &'short mut [T] through reborrowing, whereas &'short &'long [T] can yield &'long [T] through Copy), which comes often on URLO / Discord.

With all that being said, if the word reborrow in and of itself, may be too scary for the stdlib to use, then I guess re-using Pin's as_mut() would at least lead to API consistency 🤷 [EDIT: duh, it's already being used, my bad]. The one word that worries me is clone, since people could end up surprised by the &mut receiver of it

• Aside, btw: how often would beginners be using BorrowBuf? That is, taking https://rust-lang.github.io/wg-async/vision/characters.html, I'd say BorrowBuf does not target Alan nor Niklaus, but Grace and Barbara, so our concerns ought to think of Grace, in this case (since Barbara gets it).

---

owned or borrowed receivers

In a world with enough self-based methods in both APIs (and the then necessary .reborrow()ing methods), it would be possible to write the following helper function:

fn get_full_buffer<'data>(mut buf: BorrowBuf<'data>) -> &'data mut [u8] {
    buf.clear();
    let mut cursor = buf.unfilled_mut(); // `self`-based / consumes `buf`
    cursor.ensure_init();
    cursor.init_mut() //  `self`-based
}

With the current API, this function is impossible to write. To make it possible, some of the functions would have to become self-based. Hence my raising awareness about this point, and interest in trying to fix this, if possible, with some self-based methods.

That being said, I've been looking more in detail into how to adapt your API exactly for this purpose, and the only way to achieve it would be with a revamp changing the very point of BorrowCursor (that of not allowing it to modify some data of the BorrowBuf).

• That's why I recant my suggestions for self-based methods: let's keep the API as you have it 🙂

---

It constrains callees to having write-only access to the buffer.

I see 👍 in that case, we could consider exposing an extra getter to get that split: -> (&[u8], BorrowCursor…), à la https://doc.rust-lang.org/1.61.0/std/vec/struct.Vec.html#method.split_at_spare_mut:

fn split_at_unfilled<'buf>(
    self: &'buf mut BorrowBuf<'data>,
) -> (&'buf [u8], BorrowCursor<'buf, 'data>)

• And I guess ditto for a .split_at_uninit() for BorrowCursor, although both of these could be added in follow-up PRs, if needed.

---

TL,DR

The API as it is now, seems fine to me ✅, although I'm still not 100% fond of the clone name for that reborrowing function.

[nrc]

@danielhenrymantilla

in that case, we could consider exposing an extra getter to get that split: -> (&[u8], BorrowCursor…)

That's an interesting API idea I hadn't considered! Do you have use cases in mind for when one would need to access the filled and unfilled parts at the same time?

[nrc]

And @rust-lang/libs-api I'd love some feedback for the naming of clone (possibly, reborrow, see @danielhenrymantilla's comments). Data-wise (i.e., what happens at runtime), it is doing a clone, thus the name, but the lifetimes are changing, so the signature is significantly different to the usual clone signature. I'm not sure what's best here.

[programmerjake]

imho this api has the issue addressed in rust-lang/libs-team#65 -- it's missing a way to retrieve the original buffer reference -- admittedly that is less necessary with this API but imho is still quite useful. I suggest adding BorrowBuf::buf* methods like in rust-lang/libs-team#65

[RalfJung]

@nrc This creates a slice to uninitialized memory (set_init is called without actually initializing things). Under the rules as written in the reference, this is UB, albeit there are discussions going on whether we really want this to be UB.

Is that deliberate?