Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Write for Cursor<[u8; N]>, plus A: Allocator cursor support #92663

Merged
merged 4 commits into from
Mar 19, 2022

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Jan 8, 2022

This implements Write for Cursor<[u8; N]>, and also adds support for generic A: Allocator in Box and Vec cursors.

This was inspired by a user questioning why they couldn't write a Cursor<[u8; N]>:
https://users.rust-lang.org/t/why-vec-and-not-u8-makes-cursor-have-write/68210

Related history:

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 8, 2022
@cuviper
Copy link
Member Author

cuviper commented Jan 8, 2022

r? @dtolnay

@rust-highfive rust-highfive assigned dtolnay and unassigned kennytm Jan 8, 2022
@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 8, 2022
@dtolnay
Copy link
Member

dtolnay commented Jan 8, 2022

@rust-lang/libs-api:
@rfcbot fcp merge

Previously there were only the following set of 4 concrete Write impls on Cursor<T>:

  • impl Write for Cursor<&mut [u8]>
  • impl Write for Cursor<&mut Vec<u8>>
  • impl Write for Cursor<Vec<u8>>
  • impl Write for Cursor<Box<[u8]>>

This PR generalizes it to:

  • impl<T> Write for Cursor<T> where T: AsMut<[u8]>

Example of code that this makes work:

use std::io::{Cursor, Write};

fn main() {
    let mut writer = Cursor::new([0u8; 128]);
    write!(writer, "...").unwrap();
    let n = writer.position();
    println!("{:?}", &writer.into_inner()[..n as usize]);
}

The previous set of impls required you to either heap-allocate your buffer (T=Vec<u8>, T=Box<[u8]>) or deal with a lifetime parameter (T=&'a mut [u8]) — you couldn't store a stack-allocated buffer in a struct and write into it using Cursor.

@rfcbot
Copy link

rfcbot commented Jan 8, 2022

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

Concerns:

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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 8, 2022
@dtolnay
Copy link
Member

dtolnay commented Jan 8, 2022

@rfcbot concern specialization

I think we'll likely just want impl<const N: usize> Write for Cursor<[u8; N]> instead of this. The way this PR specializes for Vec<u8> to make the buffer growable on write is not an API that we could ever emulate if specialization goes away. So I think this is outside the scope of what we've been willing to use specialization for in standard library impls, which is basically performance improvements where the observable behavior difference is not a meaningful aspect of the API.

@the8472
Copy link
Member

the8472 commented Jan 8, 2022

It could also be confusing for users because AsMut<[u8]> implies it would write to fixed-sized buffers.

@cuviper
Copy link
Member Author

cuviper commented Jan 8, 2022

If we back off to [u8; N], is it worth adding any variations like Box<[u8; N]>? We can add/keep the A: Allocator extensions too, I think.

@inquisitivecrystal inquisitivecrystal added the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 8, 2022
@joshtriplett
Copy link
Member

I agree that we should use a more specific impl.

As an example: BString has AsMut<[u8]>, so this Write impl would affect it. However, this Write impl wouldn't allow growing it; it'd be preferable to use Write via the DerefMut to Vec<u8>, which would allow growing it. That seems confusing. (And if BString had implemented Write directly, which I think it reasonably could have, this would break that implementation in the absence of specialization.)

If we back off to [u8; N], is it worth adding any variations like Box<[u8; N]>?

Do we need to, given that a Box can DerefMut to its contents? If you pass &mut thebox you can use Write.

@cuviper
Copy link
Member Author

cuviper commented Jan 8, 2022

Note that this is only about Write for Cursor<T> -- a third party can't implement for their own T there at all.

As an example: BString has AsMut<[u8]>, so this Write impl would affect it. However, this Write impl wouldn't allow growing it; it'd be preferable to use Write via the DerefMut to Vec<u8>, which would allow growing it.

Ideally we might be able to specialize that deref, but right now it's "error: cannot specialize on trait DerefMut". (Whereas the specialization I wrote in this PR is only on the direct type Vec.)

(And if BString had implemented Write directly, which I think it reasonably could have, this would break that implementation in the absence of specialization.)

Sure, they can implement that, and it's not affected by whatever we do for Cursor<T>, even if that includes T = BString.

Currently Cursor<BString> will implement Read and Seek because of AsRef<[u8]>, but not Write yet. That's a foreign trait for a foreign type, so bstr can't do anything to make that happen. T: AsMut<[u8]> lets it act like a fixed buffer, at least.

If we back off to [u8; N], is it worth adding any variations like Box<[u8; N]>?

Do we need to, given that a Box can DerefMut to its contents? If you pass &mut thebox you can use Write.

The point would be an owned but indirect cursor, perhaps for a large array. Its use would be the same as Cursor<Box<[u8]>>, just with a constant length. This is not as compelling as Cursor<[u8; N]> though, which is a new case of owned + heap-free.


I'm okay with backing off without specialization, just doing arrays. I especially accept the point that we can't fake the API semantics for Vec if specialization had to go away. I'm just trying to make sure we're clear about the other aspects above.

Should I update that in this PR, or open a replacement?

@joshtriplett
Copy link
Member

joshtriplett commented Jan 8, 2022

@cuviper Thanks for the clarifications; I had forgotten that Cursor, unlike Box, was not #[fundamental], so impl Write for Cursor<T> isn't possible outside std.

I do still think that this should just handle Cursor<[u8; N]> and perhaps Cursor<Box<[u8; N]>> but not arbitrary AsMut<[u8]>.

I would suggest updating this PR.

@dtolnay dtolnay added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2022
@cuviper cuviper changed the title Implement generic Write for Cursor<T> Implement Write for Cursor<[u8; N]> Jan 20, 2022
@cuviper cuviper changed the title Implement Write for Cursor<[u8; N]> Implement Write for Cursor<[u8; N]>, plus A: Allocator cursor support Jan 20, 2022
@cuviper cuviper added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 20, 2022
@cuviper
Copy link
Member Author

cuviper commented Jan 20, 2022

Ok, I've rewritten this to just the Cursor<[u8; N]> implementation and the A: Allocator extension.

I considered Write for Cursor<Box<[u8; N], A>>, but I decided it would be awkward without Read and Seek too. The right way to accomplish those is to have AsRef<[T]> for Box<[T; N], A>, as unboxed arrays do, but this seems like a larger expansion that I didn't want to bundle in this PR. That can be a separate followup to expand Box<[T; N], A> capabilities.

@dtolnay
Copy link
Member

dtolnay commented Jan 28, 2022

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented Jan 28, 2022

@dtolnay proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 28, 2022
@dtolnay
Copy link
Member

dtolnay commented Jan 28, 2022

@rfcbot fcp merge

This PR generalizes the following existing stable impls:

- impl               Write for Cursor<&mut Vec<u8>>
+ impl<A: Allocator> Write for Cursor<&mut Vec<u8, A>>

- impl               Write for Cursor<Vec<u8>>
+ impl<A: Allocator> Write for Cursor<Vec<u8, A>>

- impl               Write for Cursor<Box<[u8]>>
+ impl<A: Allocator> Write for Cursor<Box<[u8], A>>

and adds this new stable impl:

impl<const N: usize> Write for Cursor<[u8; N]>

Example of code that this makes work:

use std::io::{Cursor, Write};

fn main() {
    let mut writer = Cursor::new([0u8; 128]);
    write!(writer, "...").unwrap();
    let n = writer.position();
    println!("{:?}", &writer.into_inner()[..n as usize]);
}

The previous set of impls required you to either heap-allocate your buffer (T=Vec<u8>, T=Box<[u8]>) or deal with a lifetime parameter (T=&'a mut [u8]) — you couldn't store a stack-allocated buffer in a struct and write into it using Cursor.

@rfcbot
Copy link

rfcbot commented Jan 28, 2022

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

No concerns currently listed.

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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 28, 2022
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Feb 23, 2022
@rfcbot
Copy link

rfcbot commented Feb 23, 2022

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Feb 23, 2022
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 5, 2022
@rfcbot
Copy link

rfcbot commented Mar 5, 2022

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.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Mar 5, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Mar 17, 2022
@dtolnay
Copy link
Member

dtolnay commented Mar 18, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Mar 18, 2022

📌 Commit 7d44316 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 19, 2022
…lnay

Implement `Write for Cursor<[u8; N]>`, plus `A: Allocator` cursor support

This implements `Write for Cursor<[u8; N]>`, and also adds support for generic `A: Allocator` in `Box` and `Vec` cursors.

This was inspired by a user questioning why they couldn't write a `Cursor<[u8; N]>`:
https://users.rust-lang.org/t/why-vec-and-not-u8-makes-cursor-have-write/68210

Related history:
- rust-lang#27197 switched `AsRef<[u8]>` for reading and seeking
- rust-lang#67415 tried to use `AsMut<[u8]>` for writing, but did not specialize `Vec`.
This was referenced Mar 19, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 19, 2022
Rollup of 6 pull requests

Successful merges:

 - rust-lang#92519 (Use verbatim paths for `process::Command` if necessary)
 - rust-lang#92612 (Update stdlib for the l4re target)
 - rust-lang#92663 (Implement `Write for Cursor<[u8; N]>`, plus `A: Allocator` cursor support)
 - rust-lang#93263 (Consistently present absent stdio handles on Windows as NULL handles.)
 - rust-lang#93692 (keyword_docs: document use of `in` with `pub` keyword)
 - rust-lang#94984 (add `CStr` method that accepts any slice containing a nul-terminated string)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e9f63fd into rust-lang:master Mar 19, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 19, 2022
@cuviper cuviper deleted the generic-write-cursor branch April 12, 2022 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.