tokio-rs / bytes

Utilities for working with bytes
MIT License
1.91k stars 286 forks source link

Feature Request: Default implementations for pinned types #654

Closed dylanplecki closed 9 months ago

dylanplecki commented 9 months ago

As a small feature request, it would be nice to have default implementations of Buf and BufMut for types that are pinned (that is, wrapped in a Pin<T> struct) and that also implement the Unpin trait.

This would make working with pinned buffers easier for the end-user since they will no longer need to add implementations for Pin<T> on their own data types to implement Buf and BufMut. These types of default implementations usually just call back to the original type, so are needlessly repetitive. Pinned data types are especially prevalent in async code when using custom futures.

Pins are a somewhat complicated feature of rust, but the idea behind this default implementation is that any Buf or BufMut that is marked as Unpin (a default / opt-out trait impl), can be wrapped in a Pin<T> struct and accessed like normal. It's similar to the default implementations of Buf and BufMut on other container types, like Box.

Example:

// Note that Buffer has a default impl for the Unpin trait.
pub struct Buffer {
  data: Vec<u8>,
}

impl Buf for Buffer {
  // implementation...
}

pub type PinnedBuffer = Pin<Buffer>;

// Now PinnedBuffer does not implement `Buf`.

I've prepared a PR to address this feature request and will attach it to this issue.

Darksonn commented 9 months ago

Is this a breaking change? Can people implement Buf for Pin<T> when T is a type they own?

I know that this would normally not be the case, but Pin is #[fundamental] and I'm unsure how that interacts here.

dylanplecki commented 9 months ago

Great point!

After throwing this into the Rust playground (first error is relevant, second is due to my hack reproduction in the playground), I do see that any crate can impl bytes::Buf for any type P due to Pin, and that this change would generate conflicts with any existing impl, so you're right that this is a breaking change (likely negating any positive effect it could have). I think that makes this change dead in the water, and I appreciate the accurate feedback @Darksonn!

If there's any consideration for marking changes for future breaking iterations of the crate, this one would be fairly unremarkable if done early enough, and becomes a useful convenience for async code (though not a requirement as users can manually impl for their own types). See a similar implementation for AsyncRead in futures-io.