Make BorrowedCursor<'a, T> covariant in 'a and drop an indirection

[Ddystopia]

This is a solution to #117693 (comment), with some improvements.

Currently 'a in BorrowedCursor is invariant, though people seem to talk about it as if it were covariant, and the feature is in FCP right now. The previous version with 'buf and 'data lifetimes had the same flaw: 'data was invariant.

A later PR landed that merged them and said that BorrowedCursor manually ensures that 'data won't be ever overwritten thus invariance should not be needed. But unfortunately the lifetime is still left invariant. You can see it here, and the error spells it out exactly:

#![feature(core_io_borrowed_buf)]

use std::io::{BorrowedBuf, BorrowedCursor};

// Accepted.
fn buf_covariant<'short, 'long: 'short>(buf: BorrowedBuf<'long, u8>) -> BorrowedBuf<'short, u8> {
    buf
}

// Rejected.
fn cursor_covariant<'short, 'long: 'short>(
    cursor: BorrowedCursor<'long, u8>,
) -> BorrowedCursor<'short, u8> {
    cursor
}

// Rejected.
fn cursor_contravariant<'short, 'long: 'short>(
    cursor: BorrowedCursor<'short, u8>,
) -> BorrowedCursor<'long, u8> {
    cursor
}

fn main() {}

And the errors (also say that BorrowedCursor is invariant over 'a):

error: lifetime may not live long enough
  --> src/main.rs:14:5
   |
11 | fn cursor_covariant<'short, 'long: 'short>(
   |                     ------  ----- lifetime `'long` defined here
   |                     |
   |                     lifetime `'short` defined here
...
14 |     cursor
   |     ^^^^^^ function was supposed to return data with lifetime `'long` but it is returning data with lifetime `'short`
   |
   = help: consider adding the following bound: `'short: 'long`
   = note: requirement occurs because of the type `BorrowedCursor<'_, u8>`, which makes the generic argument `'_` invariant
   = note: the struct `BorrowedCursor<'a, T>` is invariant over the parameter `'a`
   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance

error: lifetime may not live long enough
  --> src/main.rs:21:5
   |
18 | fn cursor_contravariant<'short, 'long: 'short>(
   |                         ------  ----- lifetime `'long` defined here
   |                         |
   |                         lifetime `'short` defined here
...
21 |     cursor
   |     ^^^^^^ function was supposed to return data with lifetime `'long` but it is returning data with lifetime `'short`
   |
   = help: consider adding the following bound: `'short: 'long`
   = note: requirement occurs because of the type `BorrowedCursor<'_, u8>`, which makes the generic argument `'_` invariant
   = note: the struct `BorrowedCursor<'a, T>` is invariant over the parameter `'a`
   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance

error: could not compile `play` (bin "play") due to 2 previous errors

This also removes the two mem::transmute calls that unfilled and reborrow used to shorten &'this mut BorrowedBuf<'data, T> into &'this mut BorrowedBuf<'this, T>.

---

Additionally I noticed that BorrowedCursor is not really as efficient as it could be, for a standard library: it contained a reference to the BorrowedBuf, which in turn contains a slice to the data. Without this, the fix is just replacing &'a mut BorrowedBuf<'a, T> with NonNull<BorrowedBuf<'a, T>>, plus some convenience helpers.

To fix this, I also stored a reborrowed pointer to the first element of the array, with the provenance to access the whole array. filled and init are still read from the pointer to BorrowedBuf, the buffer length is also read from it but carefully, in order to not create a retag which will trigger a foreign access to the pointer stored in BorrowedCursor, making it disabled. It increased the size of BorrowedCursor from one usize to two of them.

It is stored as the pointer rather than &mut [MaybeUninit<T>] to save a usize from the BorrowedCursor size.

[rustbot]

r? @jhpratt

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from JohnTitor, Mark-Simulacrum, clarfonthey, jhpratt, nia-e

[jhpratt]

Stepping away from reviews temporarily.

@rustbot reroll

[Ddystopia]

I'm not sure if the mention from rustbot worked correctly, so I'll repeat it myself: @clarfonthey

[clarfonthey]

It did, I've just been slow getting to this change. Feel free to @ me whenever I'm slow getting to reviews.

I'll take a look at this later. The main thing that caused me to pause a bit when I initially looked over this was if there's a way to ensure covariance without using NonNull (probably would require something ridiculous like &[Cell<T>]) but I couldn't figure out if there was actually anything reasonable for it.

[Ddystopia]

I think the same could be implemented without unsafe (modulo touching BorrowedBuf's invariants, but that's beyond the scope) like this:

struct BorrowedCursor<'a, T> {
    data: &'a mut [MaybeUninit<T>],
    filled: &'a mut usize,
    init: &'a mut usize,
}

In other words, cursor will fully reborrow the buffer and other things, without the indirection that causes invariance.

But it will blow up the size of BorrowedCursor to 4 * size_of::<usize>(). As well as any other approach icluding the slice, because the slice is already 2 words while filled and init still have to be referenced. Well, init could be stored as the highest bit inside filled to reduce the size by one word.

I get that it may be harder to maintain, but a) I believe the standard library is expected to provide efficiency, and b) the unsafe code touching the invariants is kind of localized to a single small impl block, so it is not spread through the whole module. Well, it was my intention at least.

---

Also the code will look a lot less scary if the BorrowedBuf will store the buffer as the pointer and the length, with a simple method that will return the slice on demand (so basically a couple of sites get additional ()). That way there is no need for this game to read the length out of &mut [] without triggering retagging.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Ddystopia]

I added another version of this PR, which is more elegant and less error prone in my opinion, but touches BorrowedBuf too. improve-borrowed-cursor-v2 branch.