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!