Remove internal PathBuf::as_mut_vec

[workingjubilee]

This function allows breaking the invariants of the OsString that PathBuf wraps, which, combined with the optimizations introduced in #96869 which bypassed the "scan the entire string" check, led to #126291 as a result. That was fixed in #126305 but the "fix" was the brutal hack of "simply remove the opt every time".

It has been there for a long time because it lends PathBuf, in general, a simpler implementation, and works fine for strings-are-byte-strings platforms.

We can afford to "complicate" things a little in order to actually preserve our invariants. A likely approach would be to remove PathBuf::as_mut_vec, and add a set of new fn to the Wtf8Buf / Buf / OsString types that directly implement the desired behaviors (e.g. truncating to a byte len), so that each such fn correctly updates is_known_utf8 as a result. Their implementation would be trivial on other platforms.

[mews6]

Hey there, i'm pretty new here and i'd like to take a look at this, but i have a few questions about the nature of the problem. For instance, as it stands, would this cause any problems for compatibility with older code? Besides, would the new fn set eventually need more implementations, or is it expected for just implementing the Wtf8Buf / Buf / OsString types to be sufficient? If i'm misinterpreting anything, please correct me.

[workingjubilee]

Hey there, i'm pretty new here and i'd like to take a look at this, but i have a few questions about the nature of the problem. For instance, as it stands, would this cause any problems for compatibility with older code?

This is purely internal code. No Rust programmer is allowed to call it beyond std.

[Borgerr]

Hi, first time contributor, so bear with me. I would love to look closer at this issue but I think I'm missing a few pieces of detail.

Before me is this single block of code for this internal method, in rust/library/std/src/path.rs:

#[inline]
fn as_mut_vec(&mut self) -> &mut Vec<u8> {
  self.inner.as_mut_vec_for_path_buf()
}

Where inner is an OsString instance. Its implementation of as_mut_vec_for_path_buf is another hack, it seems, which relates deeper into its own inner field, a Slice instance. And again, this Slice instance relates back to Wtf8, and at the end of all this we have a [u8] underlying field.

Going all the way down to Wtf8::as_mut_vec_for_path_buf has a FIXME comment articulating basically what you are: this function shouldn't exist, and we should assume the fix is about to happen. All fine and good.

What I would like to see is an example of the described "Wtf8Buf invariants", in both implementation, and what that set in stone policy really is. If you could help me find that, that would be wonderful. I'm not sure how Rust developers have been going about organizing these policies so far.

Here's the FIXME comment if it helps :)

[Borgerr]

@rustbot claim

[workingjubilee]

@Borgerr the invariants are implicit:

rust/library/std/src/sys_common/wtf8.rs

Lines 131 to 146 in d2ad293

	/// An owned, growable string of well-formed WTF-8 data.
	///
	/// Similar to `String`, but can additionally contain surrogate code points
	/// if they’re not in a surrogate pair.
	#[derive(Eq, PartialEq, Ord, PartialOrd, Clone)]
	pub struct Wtf8Buf {
	bytes: Vec<u8>,
	/// Do we know that `bytes` holds a valid UTF-8 encoding? We can easily
	/// know this if we're constructed from a `String` or `&str`.
	///
	/// It is possible for `bytes` to have valid UTF-8 without this being
	/// set, such as when we're concatenating `&Wtf8`'s and surrogates become
	/// paired, as we don't bother to rescan the entire string.
	is_known_utf8: bool,
	}

a boolean field should not be true if the answer to the question it represents is false, and direct access to the Wtf8Buf's Vec allows someone to push a set of bytes that make the result non-UTF-8 without updating the bool.

[workingjubilee]

this is also referenced in the safety comment here:

rust/library/std/src/sys_common/wtf8.rs

Lines 256 to 262 in d2ad293

	#[inline]
	pub fn as_mut_slice(&mut self) -> &mut Wtf8 {
	// Safety: `Wtf8` doesn't expose any way to mutate the bytes that would
	// cause them to change from well-formed UTF-8 to ill-formed UTF-8,
	// which would break the assumptions of the `is_known_utf8` field.
	unsafe { Wtf8::from_mut_bytes_unchecked(&mut self.bytes) }
	}

[Borgerr]

Correct me if I'm wrong, but files to touch would be library/std/src/path.rs for PathBuf::as_mut_vec, library/std/src/ffi/os_str.rs for OsString::as_mut_vec_for_path_buf.

We have a splintering off into both std/src/sys/os_str/bytes.rs and std/src/sys/os_str/wtf8.rs for their own respective as_mut_vec_for_path_buf; the latter having an inner Wtf8Buf instance, sourced at std/src/sys_common/wtf8.rs, which has the implicit invariants you describe. Understood.

However, it looks like there is indeed a fork in the road between types bytes and wtf8? Possible to see that here. To be completely honest I've never seen this kind of split include before in Rust, and I'm unsure of its purpose. It may be that I'm mistaken, too, and don't have the dependency correctly lined up. rust-analyzer gives the Buf type as coming from the bytes submodule, but given that os_str has two different spots where Buf is declared and defined, I wanted to ensure I was covering all bases.

Also possibly necessary to dumb this down a bit more for me, @workingjubilee, sorry!

[workingjubilee]

rust-analyzer is more limited in its ability to understand multi-target code, because it does so partly via running rustc for compilation, but cfg is defined as erasing code unless its requirements are met: thus compilation "never sees" the other branches. These are the atrocities the stdlib commits in order to allow Rust code to simply use std and have cross-platform abstractions that (mostly) don't leak.

[workingjubilee]

I don't think you are dumb. I don't think you need me to dumb it down, either. Based on what you have said, you seem to understand everything perfectly, except that this may be the first time you have seen a branching cfg, as here:

https://github.com/rust-lang/rust/blob/3fc81daffd18aa1fc00a1d67cfaca12a32a07e46/library/std/src/sys/os_str/mod.rs

Conditional compilation is explained in the reference, but is probably what you would expect if you've ever seen it in another language. Our CI will test multiple platforms, and that runs before merging every PR, so as long as you try to account for "both platforms exist", you needn't fear making a significant mistake here, and also code review exists.

[Borgerr]

I guess the question then would be what we replace it with since there's a few spots that rely on the function. Or, if we want to keep providing PathBuf::as_mut_vec just for the sake of existing architecture, there's certainly some motivation there. It also makes sense to me to have some support for representing a PathBuf as a mut vec, at least internally.

What's the general approach for something like this? Would I be stepping on anyone's toes to truly remove the function in its entirety and replace it with something else, or is the expectation just to ensure behaviors of public things remain the same?

[workingjubilee]

Would I be stepping on anyone's toes to truly remove the function in its entirety and replace it with something else

Nope!

or is the expectation just to ensure behaviors of public things remain the same?

Yep!

[Borgerr]

Progress has been admittedly slow since I'm working concurrently. Thank you for your quick replies, though!

first instance in _push can easily be removed by relying on OsStr::as_encoded_bytes fed into to_vec, or something possibly more simple that I'll continue to look into.

I see what you've been saying about internally truncating lengths. Once I've come up with a solid enough alternative for this first spot, I'll move on to those.

[Borgerr]

newest commit. I believe the next steps would be to:

• apply this same kind of plumbing to everything related to variable v
• and this simple extend method
• move on to implementation on other platforms

If I'm missing anything, please let me know, but I feel like this is finally getting rolling.

[workingjubilee]

Yep! That's basically the idea I had in mind.