BufWriter: Make the soundness of BorrowedBuf usage clearer.

[briansmith]

The previous safety comment was outdated as it was written before BorrowedBuf::set_init was changed to be a boolean. It also failed to address the possibility that flush_buf invalidated the assumption.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ChrisDenton, libs
• @ChrisDenton, libs expanded to 8 candidates
• Random selection from Mark-Simulacrum, jhpratt

[Mark-Simulacrum]

I think this seems like a good clarification with the comment addressed. Could you also improve https://doc.rust-lang.org/nightly/std/io/struct.BorrowedBuf.html#method.is_init to note confusingly say it returns the length when it's a boolean?

View changes since this review

[Mark-Simulacrum]

Hm, I think that guarantee is almost upheld today. Vec::drain(..written) probably is allowed to semantically de-initialize the tail by moving the memory to the front, I'm not quite sure what our impl under the hood looks like with respect to that property.

I think as-is this will cause re-initialization of the buffer in many cases on every read, though. Can you look into whether flush_buf can be made to uphold that stronger guarantee? I think we would largely just need to ensure that we can guarantee no uninit bytes are written during the drain (maybe replacing it with a lower-level operation to do so)?

[briansmith]

Good idea. I updated the PR to replace the use of Vec::drain with two simpler operations like you suggested.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[briansmith]

Could you also improve https://doc.rust-lang.org/nightly/std/io/struct.BorrowedBuf.html#method.is_init to note confusingly say it returns the length when it's a boolean?

Done. I copied the documentation from BorrowedCursor::is_init.

[briansmith]

@rustbot ready.

[Mark-Simulacrum]

Wording nit, but r=me with that resolved.

View changes since this review

[Mark-Simulacrum]

I think it's safe to assume that the copy_within code can't create uninitialized bytes -- it's a safe method on &mut [u8] here, after all.

The trickier, unmentioned, safety assumption is that as_mut_slice and truncate don't de-initialize any bytes outside the Vec's len. I think the current wording (especially out of context in the SAFETY comments you added) can be read as preserving the allocation, but we need a stronger condition of not de-initializing the tail IIUC. Can we update to say something like "preserve spare capacity, potentially newly created (for truncate), as initialized if it was before"?

I'm not actually sure if we need the capacity to still exist -- a reallocating truncate might actually be OK? -- but that doesn't matter much since we have a public guarantee that truncate doesn't change the allocation:

Note that this method has no effect on the allocated capacity of the vector.

as_mut_slice has no such guarantee, but I think it's strongly implied since &mut s[..] calling into an allocator would be deeply weird.

[briansmith]

The trickier, unmentioned, safety assumption is that as_mut_slice and truncate don't de-initialize any bytes outside the Vec's len. I think the current wording (especially out of context in the SAFETY comments you added) can be read as preserving the allocation, but we need a stronger condition of not de-initializing the tail IIUC. Can we update to say something like "preserve spare capacity, potentially newly created (for truncate), as initialized if it was before"?

Thanks. I updated the comments to be more in line with your suggestion to mention that the concern is about de-initialization. I think that if/when truncate/as_mut_slice are changed in the future where an even more specific policy is needed, then the comment will get updated then with precisely what that change requires.

I'm not actually sure if we need the capacity to still exist -- a reallocating truncate might actually be OK? but that doesn't matter much since we have a public guarantee that truncate doesn't change the allocation:

The guarantee isn't that truncate won't change the allocation. The guarantee is that truncate won't change the capacity. There is probably a lot of code that assumes that the allocation won't be moved (i.e. as_ptr() and friends don't change in surprising ways) and moving wouldn't be allowed by implicit performance requirements, but there isn't a guarantee about that yet, AFAICT.

Regardless, I think the updated comments address the main risk; Vec reserves the right to do whatever it wants with the spare capacity (except shrink its size) but these methods are constrained by BufWriters usage to guarantee slightly more.

[briansmith]

@rustbot ready.

Just to make sure we're on the same page regarding the precise wording, I won't merge this without another review.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.