std::io: migrate ReadBuf to BorrowBuf/BorrowCursor

[nrc]

WIP

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 https://github.com/rust-lang/rust/issues/78485, https://github.com/rust-lang/rust/issues/94741 supersedes: https://github.com/rust-lang/rust/pull/95770, https://github.com/rust-lang/rust/pull/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)

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
    Checking addr2line v0.16.0
error[E0432]: unresolved import `crate::io::ReadBuf`
 --> library/std/src/sys/windows/fs.rs:5:51
  |
5 | use crate::io::{self, Error, IoSlice, IoSliceMut, ReadBuf, SeekFrom};
  |                                                   |
  |                                                   |
  |                                                   no `ReadBuf` in `io`
  |                                                   help: a similar name exists in the module: `readbuf`
error[E0432]: unresolved import `crate::io::ReadBuf`
 --> library/std/src/sys/windows/handle.rs:4:61
  |
  |
4 | use crate::io::{self, ErrorKind, IoSlice, IoSliceMut, Read, ReadBuf};
  |                                                             |
  |                                                             |
  |                                                             no `ReadBuf` in `io`
  |                                                             help: a similar name exists in the module: `readbuf`
For more information about this error, try `rustc --explain E0432`.
error: could not compile `std` due to 2 previous errors
Build completed unsuccessfully in 0:00:15

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
    Checking addr2line v0.16.0
error[E0432]: unresolved import `crate::io::ReadBuf`
 --> library/std/src/sys/windows/fs.rs:5:51
  |
5 | use crate::io::{self, Error, IoSlice, IoSliceMut, ReadBuf, SeekFrom};
  |                                                   |
  |                                                   |
  |                                                   no `ReadBuf` in `io`
  |                                                   help: a similar name exists in the module: `readbuf`
error[E0432]: unresolved import `crate::io::ReadBuf`
 --> library/std/src/sys/windows/handle.rs:4:61
  |
  |
4 | use crate::io::{self, ErrorKind, IoSlice, IoSliceMut, Read, ReadBuf};
  |                                                             |
  |                                                             |
  |                                                             no `ReadBuf` in `io`
  |                                                             help: a similar name exists in the module: `readbuf`
For more information about this error, try `rustc --explain E0432`.
error: could not compile `std` due to 2 previous errors
Build completed unsuccessfully in 0:00:15

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
    Checking miniz_oxide v0.4.0
    Checking std_detect v0.1.5 (/checkout/library/stdarch/crates/std_detect)
    Checking object v0.26.2
    Checking addr2line v0.16.0
error[E0596]: cannot borrow `cursor` as mutable, as it is not declared as mutable
   --> library/std/src/sys/windows/handle.rs:114:44
    |
112 |     pub fn read_buf(&self, cursor: BorrowCursor<'_, '_>) -> io::Result<()> {
    |                            ------ help: consider changing this to be mutable: `mut cursor`
113 |         let res =
114 |             unsafe { self.synchronous_read(cursor.as_mut().as_mut_ptr(), cursor.capacity(), None) };
    |                                            ^^^^^^^^^^^^^^^ cannot borrow as mutable

error[E0596]: cannot borrow `cursor` as mutable, as it is not declared as mutable
   --> library/std/src/sys/windows/handle.rs:120:21
    |
112 |     pub fn read_buf(&self, cursor: BorrowCursor<'_, '_>) -> io::Result<()> {
    |                            ------ help: consider changing this to be mutable: `mut cursor`
...
120 |                     cursor.advance(read as usize);
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot borrow as mutable
For more information about this error, try `rustc --explain E0596`.
error: could not compile `std` due to 2 previous errors
Build completed unsuccessfully in 0:00:18

[bors]

:umbrella: 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 https://github.com/rust-lang/reference/issues/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:

• https://github.com/rust-lang/rust/issues/65409
• https://github.com/rust-lang/rust/pull/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 https://github.com/rust-lang/libs-team/issues/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 https://github.com/rust-lang/libs-team/issues/65

[nrc]

I think with this API it is fine just to use the original reference because that buffer in the BorrowBuf cannot be swapped out

[programmerjake]

I think with this API it is fine just to use the original reference because that buffer in the BorrowBuf cannot be swapped out

yes, but what if you don't have the original reference for some reason?

[nrc]

yes, but what if you don't have the original reference for some reason?

Do you have a use case in mind? In general, the creator of the buffer should always have such a reference (it needs one to create the buffer). The buffer is a borrowed and temporary one for writing in to, so I don't think there is a good use case for storing it, etc.

[programmerjake]

yes, but what if you don't have the original reference for some reason?

Do you have a use case in mind?

yes, 2 use cases:

1. it may be easier to use only one variable like so:

   let mut buf = BorrowBuf::from(&mut MaybeUninit::uninit_array::<256>());
   reader.read_buf(buf.unfilled())?;
   let len = unsafe { process_in_place_ffi(buf.buf_mut().as_mut_ptr() as *mut u8, buf.capacity(), buf.len()) };
   buf.clear();
   unsafe { buf.unfilled().advance(len); }
   println!("{:?}", buf.filled());
   

2. passing a BorrowBuf or BorrowCursor through FFI:

   pub unsafe trait FromToFFISafe: Sized {
       type Type;
       unsafe fn to_ffi(self) -> Self::Type;
       unsafe fn from_ffi(ffi: Self::Type) -> Self;
   }
   
   #[repr(C)]
   pub struct FFISafeBorrowBuf<'a> {
       pub buf: *mut u8,
       pub capacity: usize,
       pub filled: usize,
       pub initialized: usize,
       _phantom: PhantomData<&'a mut [u8]>,
   }
   
   impl<'a> FromToFFISafe for BorrowBuf<'a> {
       type Type = FFISafeBorrowBuf<'a>;
       unsafe fn to_ffi(self) -> Self::Type {
           FFISafeBorrowBuf {
               capacity: self.capacity(),
               filled: self.len(),
               initialized: self.init_len(),
               buf: self.buf_mut().as_mut_ptr(),
           }
       }
       unsafe fn from_ffi(ffi: Self::Type) -> Self {
           let mut retval = Self::from(slice::from_raw_parts_mut(ffi.buf as *mut MaybeUninit<u8>, buf.capacity));
           retval.set_init(ffi.initialized);
           retval.unfilled().advance(ffi.filled);
           retval
       }
   }
   

[nrc]

it may be easier to use only one variable like so

I don't think this works because of the borrowing rules around temporaries

passing a BorrowBuf or BorrowCursor through FFI:

This is interesting! Why do you want to pass it through FFI? And why would you want to do that rather than pass one of the views (e.g., to the unfilled part)?

[bors]

:umbrella: The latest upstream changes (presumably #98748) made this pull request unmergeable. Please resolve the merge conflicts.

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
Attempting with retry: make prepare
---
* highest error code: E0790
Found 506 error codes
Found 0 error(s) in error codes
Done!
tidy error: /checkout/library/std/src/io/mod.rs:808: TODO is deprecated; use FIXME
some tidy checks failed
Build completed unsuccessfully in 0:00:09

[rustbot]

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 @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. 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

[nrc]

@rustbot label +T-libs-api -T-libs

[nrc]

@rustbot label +S-waiting-on-review -S-waiting-on-author

[nrc]

r? @Mark-Simulacrum or @joshtriplett or @sfackler or @BurntSushi (pinging some folk who I know have reviewed this area somewhat recently)

[SabrinaJewson]

From looking at the API, I don’t see the purpose in having that 'data lifetime parameter — it doesn’t actually do anything, since it doesn’t feature in the public API and the type also has 'buf which is always shorter than 'data. Could we just remove it? I think it would need some extra unsafe trickery to maintain covariance of buf, but it seems worth it to avoid forcing users to type out '_ so much.

[Mark-Simulacrum]

I believe this would need an ACP (per https://github.com/rust-lang/rust/pull/97015#issuecomment-1206628480, "Introducing new or changing existing unstable library APIs"), but otherwise happy to review the actual implementation. I'll mark as waiting-on-author for now though.

The PR description looks like basically what I'd expect to see in an ACP, so hopefully this is relatively easy to file.

[nrc]

I believe this would need an ACP (per https://github.com/rust-lang/rust/pull/97015#issuecomment-1206628480, "Introducing new or changing existing unstable library APIs"), but otherwise happy to review the actual implementation. I'll mark as waiting-on-author for now though.

The PR is older than the ACP (so, legally, should be grandfathered in) and has already been discussed with the team on Zulip and as a draft PR, so I feel like we've already accomplished the spirit of the ACP and it would just be pointless bureaucracy to file an issue. I can do so if it will help with tracking or something, otherwise it seems better to keep all discussion in one place (here).

@rustbot label +S-waiting-on-review -S-waiting-on-author

[Mark-Simulacrum]

I'll nominate for discussion on naming https://github.com/rust-lang/rust/pull/97015#discussion_r941415367.

[nrc]

I've addressed the reviewer comments (except for the naming of clone where I'm waiting for more feedback). BorrowedCursor now only has a single lifetime, which is obviously nicer and I believe is safe, but does make me feel a little less comfy.

[nrc]

Renamed clone -> reborrow. All comments are addressed. Could I get a re-review please?

[Mark-Simulacrum]

@bors r+ rollup=iffy

OK, I think this is good to go, at least initially. The new APIs are all unstable, so we can continue iterating in the future if there's thoughts, but I think getting this merged is a good first step.

[bors]

:pushpin: Commit ac70aea98509c33ec75208f7b42c8d905c74ebaf has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[RalfJung]

This is now r+'d but the PR comment still says "WIP"?

[Mark-Simulacrum]

I think that was just leftover from (much) earlier -- @nrc, I dropped it from the PR description, but would be good to take another look on your end and confirm.

[nrc]

Whoops, yeah my mistake, should have removed that