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

Add back BorrowedBuf::filled_mut #139

Closed
SUPERCILEX opened this issue Nov 28, 2022 · 2 comments
Closed

Add back BorrowedBuf::filled_mut #139

SUPERCILEX opened this issue Nov 28, 2022 · 2 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@SUPERCILEX
Copy link

SUPERCILEX commented Nov 28, 2022

Proposal

Problem statement

The current BorrowedBuf API has no way to modify the filled data without using unsafe. This blocks several use cases:

  • Preprocessing data before giving someone else the filled buf.
  • Taking in an uninitialized buffer and returning an initialized buffer with the same lifetime that allows mutation.
  • Any kind of in place mutation using the initialized data.

Motivation, use-cases

Need to mutate the initialized buffer for various reasons.

Solution sketches

See rust-lang/rust#103754. TL;DR: add

fn filled_mut(&mut self) -> &mut [u8]

Links and related work

filled_mut was part of the original RFC, but was removed during the split into BorrowedBuf and BorrowedCursor. The cited rationale is to make BorrowedBuf read-only and BorrowedCursor write-only. I don't believe this logic holds up for a number of reasons:

  • The original uninitialized buffer that's passed in is mutable, therefore that buffer's owner cannot make any guarantees about their buffer, regardless of the presence of filled_mut.
  • The BorrowedBuf has access to the mutable buffer which means you can always mess around with filled part by finagling around with clear, unfilled, and set_init. Again, this means adding filled_mut does not weaken the guarantees made by BorrowedBuf in any way.
  • Without mutable access to the filled buffer, the read-buf family of APIs becomes limited to read-only views of loaded data when using only safe Rust. But the purpose of this API is to avoid the need for unsafe.
    • The alternative is to drop the BorrowedBuf (which can be annoying for a number of reasons) and manually convert the underlying MaybeUninit buffer to initialized u8s using unsafe.
@SUPERCILEX SUPERCILEX added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Nov 28, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Jul 11, 2023

We briefly discussed this in today's meeting. Adding filled_mut back is fine. :)

@m-ou-se m-ou-se closed this as completed Jul 11, 2023
@Amanieu Amanieu added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Jul 11, 2023
@SUPERCILEX
Copy link
Author

Thank you so much! 🙏

bors added a commit to rust-lang-ci/rust that referenced this issue Jul 11, 2023
Add back BorrowedBuf::filled_mut

This is useful if you want to do some processing on the bytes while still using the BorrowedBuf.

The API was removed in rust-lang#97015 with no explanation. The RFC also has it as part of its API, so this just seems like a mistake: [RFC](https://rust-lang.github.io/rfcs/2930-read-buf.html#:~:text=inline%5D%0A%20%20%20%20pub%20fn-,filled_mut,-(%26mut%20self))

ACP: rust-lang/libs-team#139
RalfJung pushed a commit to RalfJung/miri that referenced this issue Jul 12, 2023
Add back BorrowedBuf::filled_mut

This is useful if you want to do some processing on the bytes while still using the BorrowedBuf.

The API was removed in rust-lang/rust#97015 with no explanation. The RFC also has it as part of its API, so this just seems like a mistake: [RFC](https://rust-lang.github.io/rfcs/2930-read-buf.html#:~:text=inline%5D%0A%20%20%20%20pub%20fn-,filled_mut,-(%26mut%20self))

ACP: rust-lang/libs-team#139
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Oct 17, 2023
Add back BorrowedBuf::filled_mut

This is useful if you want to do some processing on the bytes while still using the BorrowedBuf.

The API was removed in rust-lang/rust#97015 with no explanation. The RFC also has it as part of its API, so this just seems like a mistake: [RFC](https://rust-lang.github.io/rfcs/2930-read-buf.html#:~:text=inline%5D%0A%20%20%20%20pub%20fn-,filled_mut,-(%26mut%20self))

ACP: rust-lang/libs-team#139
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
Add back BorrowedBuf::filled_mut

This is useful if you want to do some processing on the bytes while still using the BorrowedBuf.

The API was removed in rust-lang/rust#97015 with no explanation. The RFC also has it as part of its API, so this just seems like a mistake: [RFC](https://rust-lang.github.io/rfcs/2930-read-buf.html#:~:text=inline%5D%0A%20%20%20%20pub%20fn-,filled_mut,-(%26mut%20self))

ACP: rust-lang/libs-team#139
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Add back BorrowedBuf::filled_mut

This is useful if you want to do some processing on the bytes while still using the BorrowedBuf.

The API was removed in rust-lang/rust#97015 with no explanation. The RFC also has it as part of its API, so this just seems like a mistake: [RFC](https://rust-lang.github.io/rfcs/2930-read-buf.html#:~:text=inline%5D%0A%20%20%20%20pub%20fn-,filled_mut,-(%26mut%20self))

ACP: rust-lang/libs-team#139
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

3 participants