impl Read and Write for VecDeque<u8>

[evanrichter]

Implementing Read and Write for VecDeque<u8> fills in the VecDeque api surface where Vec<u8> and Cursor<Vec<u8>> already impl Read and Write. Not only for completeness, but VecDeque in particular is a very handy mock interface for a TCP echo service, if only it supported Read/Write.

Since this PR is just an impl trait, I don't think there is a way to limit it behind a feature flag, so it's "insta-stable". Please correct me if I'm wrong here, not trying to rush stability.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

Please see the contribution instructions for more information.

[paolobarbolini]

Shouldn't this also have a Read::read_buf implementation?

[evanrichter]

Shouldn't this also have a Read::read_buf implementation?

Just pushed up another commit to include this, thanks! The codegen looks decent at least to me, still compiling down to a number of slice checks and a single memcpy

[evanrichter]

Just squashed the commits together and rewrote the commit message.

And I think I should have highfived the libs team?

r? rust-lang/libs

[thomcc]

r? rust-lang/libs-api @rustbot label +T-libs-api

[paolobarbolini]

Now that I think of it shouldn't there also be a write_vectored implementation?

This line shouldn't be necessary anymore after #95904 lands

nice! I suppose whichever PR gets merged second can update this line?

If nothing goes wrong with CI #95904 should be merged today 🥳

[m-ou-se]

@rfcbot merge

[rfcbot]

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

Concerns:

• write-vs-write_all resolved by impl Read and Write for VecDeque<u8> #95632 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[joshtriplett]

I love this idea, and I really look forward to using it! I also really look forward to when we have async read/write traits in std, and we can add async versions of this, to get a byte-oriented pipe in std.

I do have one concern:

@rfcbot concern write-vs-write_all

I'd like to ask if we should really make write do partial writes up to capacity, or if we should make write and write_all behave identically. Currently, the Write impl for Vec always writes all the data. Is there an advantage and use case for doing partial writes to VecDeque?

I am extremely open to hearing about such a use case, and I'd be happy to drop this concern if so.

[evanrichter]

@rfcbot concern trait-impl-behavior

This is my first code contribution and I'm a little nervous about getting this wrong 😅

Am I right that how a type implements a trait is guaranteed for the future? i.e. if rust 1.62.0 ships and VecDeque does partial reads, then it's decided it would be better to make read behave the same as read_exact, can we even do that? It's possible crates would have failing tests due to that change, even though VecDeque still implements Read either way.

Also: I will soon push an update making write behave the same as write_all

[evanrichter]

Oh no I can't raise a concern for my PR 🙃

@joshtriplett @m-ou-se Am I correct above?

[m-ou-se]

This is my first code contribution and I'm a little nervous about getting this wrong

No worries, there are many eyes on this and we won't merge this if we think there are any issues, nor will we blame you if we miss something during review. :)

Am I right that how a type implements a trait is guaranteed for the future?

That's a complicated question that has no general answer. We've regularly changed (trait) implementations of all kind of types to do something better or more efficient, as long as we think it doesn't break any code that was making reasonable assumptions. For read and write specifically, I think it's perfectly fine for the exact details of how many bytes they can produce/consume at once to change over time, as those are designed to be used in a loop (like write_all) anyway.

Also: I will soon push an update making write behave the same as write_all

👍

No need to block the FCP on that with an rfcbot concern. Rfcbot is just counting down to the decision for 'do we want VecDeque to implement Read and Write?', not for 'is this the exact code we should merge?'. Reviewing and merging this PR is a separate (manual) step, after the FCP finishes.

(If you want to make clear a PR not ready for merging yet, you can always mark a PR as a draft. (Top right in the GitHub web interface, right under the reviewers.))

[evanrichter]

Thanks a bunch for clarifying 🙂

I'm happy with the PR as it is now, knowing those details are considered somewhat flexible.

[evanrichter]

bumped the version on the "stable since" attribute to 1.63.0 after today's stable release of 1.61.0

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[joshtriplett]

@bors r+

[bors]

📌 Commit 8b7a3f4 has been approved by joshtriplett