Add back BorrowedBuf::filled_mut

[SUPERCILEX]

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

The API was removed in #97015 with no explanation. The RFC also has it as part of its API, so this just seems like a mistake: RFC

ACP: rust-lang/libs-team#139

[rustbot]

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[SUPERCILEX]

@rustbot label +T-libs-api -T-libs

r? @scottmcm

[scottmcm]

I don't think I have any particular expertise here, so I'll leave it to highfive's original choice.

r? joshtriplett

Also, has this been discussed at all in #78485? Given that this type had an RFC (https://rust-lang.github.io/rfcs/2930-read-buf.html), it seems odd that this didn't come up back then if it's needed?

[SUPERCILEX]

Sounds good.

The original RFC has filled_mut and I can't find any explanation for its disappearance, so yeah just seems like an oversight.

[SUPERCILEX]

r? @thomcc

[SUPERCILEX]

@nrc can you confirm that this change wasn't intentional?

[thomcc]

Needs an ACP?

[thomcc]

Going to mark as blocked on ACP, but really it's just that while the impl seems fine, I'm not an API reviewer so I don't really feel qualified to say.

[SUPERCILEX]

Dang, yeah that's fair. And no, this shouldn't need an ACP since the API was removed without explanation and is already part of the RFC. I'll keep doing the rounds—I've got a crate that needs this feature and my past experience has suggested it'll take 1-3 months before anyone from the libs team looks at this which is a bummer. r? @the8472

PS: you can probably make another contributor happy by taking a look at this easy PR #103901

[SUPERCILEX]

Sweet, thanks for replying! The use-case is that I'm continuously filling and clearing the same buffer and doing in-place processing of the data. Currently I have to copy it over to a Vec which is really sad. It'd be a huge pain to use the original buffer since a) its a bunch of MaybeUninits and I'd have to manually assume_init them, but the whole point of this API is to avoid that and b) I would need to create and drop the borrowedbuf in the state machine of my loop and dealing with the lifetimes of passing stuff across some but not all iterations of the loop will be a huge pain.

BorrowedBuf which is 'read only'

Not super true since you have clear.

So, filled_mut was removed because it allows writing to the BorrowedBuf.

I don't think this read-only split is the right approach. I think of BorrowedBuf as having full access to the filled part of the buffer and BorrowedCursor having full access to the unfilled part.

if you want to mutate the filled part, then you should do so via the underlying buffer.

But then you have to deal with MaybeUninits. Or am I missing something?

[the8472]

Note that I'm on the libs team, not libs-api, so approving this is not in my purview.

r? libs-api

[SUPERCILEX]

Oops, you're listed in the first group of people on the libs team website so I got confused. Thanks for the heads up.

[nrc]

Not super true since you have clear.

Yeah, not only clear there's also set_init (I regard these two methods as about controlling the buffer rather than writing into it) and unfilled for writing. So this is a design choice to try and make the API easier to understand rather than a safety property or something.

I don't think this read-only split is the right approach. I think of BorrowedBuf as having full access to the filled part of the buffer and BorrowedCursor having full access to the unfilled part.

That's fine. If you regard the filled part as immutable once writing is complete, then these two points of view are equivalent.

But then you have to deal with MaybeUninits. Or am I missing something?

You do if the undlerlying buffer uses MaybeUninits. I expect the common case to be something like a Vec where the filled part is always initialised and the unfilled part if MaybeUninit, in which case you don't have to deal with this.

I would need to create and drop the borrowedbuf in the state machine of my loop and dealing with the lifetimes of passing stuff across some but not all iterations of the loop will be a huge pain.

Creating and destroying a BorrowedBuf is cheap, so I would expect you to do this on every iteration.

As a strawman solution, I suggest you have a Vec initialised with a capacity outside the loop, and create a BorrowedBuf on each iteration, using the underlying Vec for accessing the data. I'd be curious if that works out as a good solution or if it is not ergonomic in some way.

[SUPERCILEX]

Creating and destroying a BorrowedBuf is cheap

Sure, but again I then have to keep track of which bytes were initialized myself.

As a strawman solution, I suggest you have a Vec

This doesn't make sense given that the whole point of the API is to use a borrowed buf: it should be able to come from anywhere including people passing me arbitrary buffers that I can't control. The way I see it, the API is forcing its users to bend over backwards for theoretical purity that doesn't offer material benefits.

Anyway, since this is an intentional change, I'll create an ACP later today to fix this.

[nrc]

This is clearly ridiculous given that the whole point of the API is to use a borrowed buf: it should be able to come from anywhere including people passing me arbitrary buffers that I can't control. The way I see it, the API is forcing its users to bend over backwards for useless theoretical purity.

I'm trying to understand your use case here. It sounded like you were providing the underlying buffer and then operating on it, but here it sounds like you want to take a buffer and both read into it and operate on it? That is not a situation I imagined, so I'm keen to understand what you need.

the whole point of the API is to use a borrowed buf

The point of the API is to have an abstraction over a buffer to read into. It sounds like you want to do more than that, so we need to understand what exactly.

useless theoretical purity

One person's useless theoretical purity is another person's clean API design. It would be more useful to fully explain your requirements rather than name calling.

[SUPERCILEX]

here it sounds like you want to take a buffer and both read into it and operate on it

Yup, it's read + preprocessing.

buffer to read into. It sounds like you want to do more than that, so we need to understand what exactly.

But don't you need to actually use the buffer after reading it in? I don't why the API is being constrained to only allow reading. For example, I'm pretty sure the lifetimes would allow a neat API like fn load(buf: &mut [MaybeUninit<u8>]) -> &mut [u8] where no unsafe needs to be used at all assuming filled_mut exists.

One person's useless theoretical purity is another person's clean API design.

I'm missing something then. BorrowedBuf takes in a mut reference so you could always combine a clear -> unfilled -> modify -> set_init to emulate filled_mut.

Going back to this:

More precisely, if you have a BorrowedBuf, you can assume that the filled part of the buffer will never be modified.

Not true since you can do the above I mentioned.

The "useless theoretical purity" is not a dis — I legitimately don't see any benefit to preventing buffer modification. What bug would it be preventing? And if such a bug exists, how is it more scary than forcing people to use unsafe to modify the buffer?

Sidenote: I haven't actually seen the discussion that led to this change so maybe there's more context I'm missing?

[SUPERCILEX]

The point of the API is to have an abstraction over a buffer to read into.

Actually, setting aside the preprocessing stuff, the load API I showed seems like a pretty important use case: BorrowedBuf currently doesn't allow building an API that takes in uninitialized memory and returns the initialized portion of the buffer with mutation allowed.

[SUPERCILEX]

Alrighty, created the ACP: rust-lang/libs-team#139

[dtolnay]

Reassigning yaahc's reviews as she has stepped down from the review rotation.

r? rust-lang/libs-api

[m-ou-se]

@bors r+

[bors]

📌 Commit 050d5f6 has been approved by m-ou-se

It is now in the queue for this repository.

[bors]

⌛ Testing commit 050d5f6 with merge 48a814d...

[bors]

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 48a814d to master...

[rust-timer]

Finished benchmarking commit (48a814d): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.0%	[1.0%, 1.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.3%	[3.3%, 3.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 657.118s -> 656.572s (-0.08%)