Make BorrowedCursor<'a, T> covariant in 'a and drop an indirection - #160563
Make BorrowedCursor<'a, T> covariant in 'a and drop an indirection#160563Ddystopia wants to merge 1 commit into
BorrowedCursor<'a, T> covariant in 'a and drop an indirection#160563Conversation
rustbot
added
the
S-waiting-on-review
rustbot
added
the
T-libs
|
r? @jhpratt rustbot has assigned @jhpratt. Use Why was this reviewer chosen?The reviewer was selected based on:
|
a23dd99 to
920ec31
Compare
|
Stepping away from reviews temporarily. @rustbot reroll |
|
I'm not sure if the mention from rustbot worked correctly, so I'll repeat it myself: @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 |
|
I think the same could be implemented without 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 I get that it may be harder to maintain, but a) I believe the standard library is expected to provide efficiency, and b) the Also the code will look a lot less scary if the |
This comment has been minimized.
This comment has been minimized.
This is a solution to rust-lang#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: ```rust 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>`. They were sound only as long as nobody ever assigned into `BorrowedCursor::buf`, which the cursor can no longer do at all, since it never holds a `BorrowedBuf` reference now. --- 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.
920ec31 to
a94449b
Compare
|
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. |
|
I added another version of this PR, which is more elegant and less error prone in my opinion, but touches |
|
@clarfonthey hi, could this be a good time? |
| } | ||
|
|
||
| impl<T> Debug for BorrowedBuf<'_, T> { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { |
There was a problem hiding this comment.
Is there a specific need for this?
There was a problem hiding this comment.
Oh, I see, you're also using it for BorrowedCursor. Carry on.
| // Safety invariant: this points to the start of the *whole* buffer of `*borrowed_buf` and is | ||
| // valid for reads and writes of `(*borrowed_buf).buf.len()` elements, so that | ||
| // `(*borrowed_buf).filled` indexes into it. | ||
| buf: NonNull<MaybeUninit<T>>, | ||
| /// The buffer this cursor was created from. | ||
| // Safety invariants: | ||
| // 1. `(*borrowed_buf).buf` is *never* accessed by the owner of the pointee while the `buf` | ||
| // field above is alive, because there is a `&mut` of the pointee while the cursor is alive. | ||
| // 2. We promise to only access the `filled` and `init` fields and the metadata of the `buf` | ||
| // field through the `borrowed_buf` pointer, never triggering any retag of `buf`'s pointer, | ||
| // as the `buf` field above holds a reborrow of it and reaching the parent again would be a | ||
| // foreign access for that reborrow. This includes not making a reference to the whole | ||
| // pointee out of `borrowed_buf`, but only accessing those fields directly through pointer | ||
| // manipulation. |
There was a problem hiding this comment.
Technically doing what the code was doing before, but in general, I would recommend just making the invariant documentation also doc comments, since they're useful if you document the private items with rustdoc.
|
Sorry I took so long to get to this. This all looks good; I have one small note about comments, and I think it would be nice if we explicitly ran some of this code through miri in the tests to make sure that there aren't any additional changes we need to add to avoid improper tagging. None of those are blocking however, so, I'll give you a bit of time to respond to my comments and can still merge later if you don't have time. (r=me, basically) |
4 participants
This is a solution to #117693 (comment), with some improvements.
Currently
'ainBorrowedCursoris 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'bufand'datalifetimes had the same flaw:'datawas invariant.A later PR landed that merged them and said that
BorrowedCursormanually ensures that'datawon'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:And the errors (also say that
BorrowedCursoris invariant over'a):This also removes the two
mem::transmutecalls thatunfilledandreborrowused to shorten&'this mut BorrowedBuf<'data, T>into&'this mut BorrowedBuf<'this, T>.Additionally I noticed that
BorrowedCursoris not really as efficient as it could be, for a standard library: it contained a reference to theBorrowedBuf, which in turn contains a slice to the data. Without this, the fix is just replacing&'a mut BorrowedBuf<'a, T>withNonNull<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.
filledandinitare still read from the pointer toBorrowedBuf, 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 inBorrowedCursor, making it disabled. It increased the size ofBorrowedCursorfrom oneusizeto two of them.It is stored as the pointer rather than
&mut [MaybeUninit<T>]to save ausizefrom theBorrowedCursorsize.