Implement Write for Cursor<W: AsMut<[u8]>> - #160960
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
Change `WriteThroughCursor` into a specialization which allows `Vec` and `&mut Vec` to extend their allocation for writing. This makes discoverability of the `Write` implementation better.
517f9a5 to
8142083
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 haven't looked at the implementation yet, but I'm dubious on this being a good idea. Specialization used for performance seems OK, but exposing additional properties in the implementation seems pretty unfortunate. Is the only reason to use specialization to avoid a publicly visible additional bound? Can we directly implement with the coherence bypassing attribute? |
Yeah the goal is to avoid exposing the current
I wasn't aware of any attribute that could allow an incoherent trait implementation. I'd be happy to use it if you could point me in the right direction? |
|
Hm, maybe we don't have that capability today. I guess WriteThroughCursor is not public, so users actually have no way to find these impls whatsoever today? That does seem unfortunate... maybe making it public (but unstable) for now would help mitigate that? Do I understand correctly that with this PR, the rustdoc on Cursor and Write will show |
Yeah that's an issue introduced early in the
Correct.
I'm not against that. There'd still need to be some changes to how the trait is implemented since, today, it's only actually implemented for 5 types (slices, boxed slices, and |
|
Hm, that seems right. I guess we would still need specialization even with a custom trait (rather than AsMut)? Part of why I'm worried about that approach is it probably locks us out of nicely exposing the additional feature set to ecosystem types that support adding capacity (e.g., Bytes) - right? I think there's no way to have impls for both Everything here also needs to be insta-stable, right? That also seems unfortunate. |
Another option, that I also see as having drawbacks, is marking
Actually to add insult to injury,
I believe so, since this is effectively a restructuring of existing stable implementations. |
Change
WriteThroughCursorinto a specialization which allowsVecand&mut Vecto extend their allocation for writing. This makes discoverability of theWriteimplementation better.ACP: rust-lang/libs-team#853
Tracking Issue: #154046
Follow Up To: #160952 & #158537
Description
Currently,
Writeis implemented forCursor<W>, whereWis one of:&mut [u8]for<const N: usize> [u8; N]for<A: Allocator> Box<[u8], A>for<A: Allocator> Vec<u8, A>for<A: Allocator> &mut Vec<u8, A>When moving
Writeintocore::io, it was noted here that the documentation ofCursorwould be degraded due to the introduction ofWriteThroughCursor, an indirection trait to allowCursorandWriteincore, whileBoxandVecexist inalloc.To resolve this documentation regression, and add additional functionality as well, I'm proposing we instead implement
Write for Cursor<W>whereW: AsMut<[u8]>. All 5 of the original types above implementAsMut<[u8]>, so we'll correctly document the breadth of support.However, for
Vecand&mut Vec, I use specialization to allow their implementation to extend, rather than only writing to the existing slice. While this still uses an indirection trait (SpecCursorWrite), but now this is purely for specialization, and therefore doesn't need to be documented publicly.I'm opening a PR directly as a reference point, but I suspect this will need an ACP since this changes the public API.
Notes