Make ByteStr/ByteString a more-opaque wrapper like OsStr/OsString

[clarfonthey]

This attempts to resolve the issues I brought up in the tracking issue (#134915) a few weeks ago by making ByteStr and ByteString into "more opaque" wrapper types like OsStr and OsString. I decided to put together a set of methods that are mostly drawn from OsStr and OsString, but include a few extras. I originally asked if an ACP would be required to do this, but without a response from libs-api and reasonable support from passers-by, I'm just making a PR instead.

The main issue with ByteStr is that it's not a "real" string type: they're just wrappers over [u8] and Vec<u8> with barely any methods of their own, relying instead on Deref impls. The inability to move out of deref here prevents methods like Vec::leak from being used without much effort, and the fact that the two types aren't linked via Deref means that any extra methods added to ByteStr would have to be added to ByteString too.

The new wrapper types act more in line with a "real" string type while still allowing easy access to the underlying [u8] and Vec<u8> without any complaining. In particular, this means that ByteString has an as_mut_vec method that can be executed safely (unlike String::as_mut_vec) and ByteStr has an as_bytes_mut method that can be executed safely. Any methods added to ByteStr can be accessed directly by ByteString due to the deref implementation, avoiding the need to add new methods to both types.

Note that these changes parallel OsStr and OsString for a reason: I was hoping to update OsStr and OsString to internally use these types on non-Windows targets. This feels like an obvious case where these types would be useful, and considering how I totally failed due to the issues I mentioned, I decided to at least start with what would be necessary to achieve that and we can build from there.

While the trait implementations remain untouched (except Deref, which now follows ByteString -> ByteStr), here is the new set of inherent methods added:

impl ByteStr {
    // constructors based on `OsStr`, but with new mutable version
    pub const fn new<S: ?Sized + [const] AsRef<ByteStr>>(bytes: &S) -> &ByteStr;
    pub const fn new_mut<S: ?Sized + [const] AsMut<ByteStr>>(bytes: &mut S) -> &mut ByteStr;

    // mirrors `OsStr` methods
    pub const fn is_empty(&self) -> bool;
    pub const fn len(&self) -> usize;
    pub const fn is_ascii(&self) -> bool;
    pub const fn make_ascii_lowercase(&mut self);
    pub const fn make_ascii_uppercase(&mut self);
    pub const fn eq_ignore_ascii_case<S: [const] AsRef<ByteStr> + [const] Destruct>(&self, other: S) -> bool;
    pub fn to_string_lossy(&self) -> Cow<'_, str>;
    pub fn to_byte_string(&self) -> ByteString;
    pub fn to_ascii_lowercase(&self) -> ByteString;
    pub fn to_ascii_uppercase(&self) -> ByteString;
    pub fn into_byte_string(self: Box<ByteStr>) -> ByteString;

    // mirrors `str` methods, but more safe due to invariant differences
    pub const fn as_bytes(&self) -> &[u8];
    pub const fn as_bytes_mut(&mut self) -> &mut [u8];
    pub fn into_boxed_bytes(self: Box<ByteStr>) -> Box<[u8]>;
}

impl ByteString {
    // mirrors `Vec` methods on `OsString`
    pub const fn new() -> ByteString;
    pub fn with_capacity(capacity: usize) -> ByteString;
    pub fn clear(&mut self);
    pub fn capacity(&self) -> usize;
    pub fn reserve(&mut self, additional: usize);
    pub fn try_reserve(&mut self, additional: usize) -> Result<(), TryReserveError>;
    pub fn reserve_exact(&mut self, additional: usize);
    pub fn try_reserve_exact(&mut self, additional: usize) -> Result<(), TryReserveError>;
    pub fn shrink_to_fit(&mut self);
    pub fn shrink_to(&mut self, min_capacity: usize);

    // mirrors `OsString` methods
    pub fn as_byte_str(&self) -> &ByteStr; 
    pub fn as_mut_byte_str(&mut self) -> &mut ByteStr;
    pub fn leak<'a>(self) -> &'a mut ByteStr;

    // mirrors `String` methods
    pub fn as_mut_vec(&mut self) -> &mut Vec<u8>;
    pub fn truncate(&mut self, len: usize);
    pub fn into_bytes(self) -> Vec<u8>;
    pub fn into_boxed_byte_str(self) -> Box<ByteStr>;

    // this parallels `OsString`, but is opposite to `String`, which has `push` and `push_str`
    pub fn push<S: AsRef<ByteStr>>(&mut self, s: S);
    pub fn push_byte(&mut self, b: u8);

    // not implemented on str, but implemented on Box<str> and required for conversion to Box<ByteStr>, so,
    // decided to make public
    pub fn into_boxed_bytes(self) -> Box<[u8]>;

    // this one's dubious, but I figured it is okay to add for completeness; not on `String`
    pub fn as_vec(&self) -> &Vec<u8>;
}

You may have also noticed these omitted, which I'm pointing out separately due to a bit of an API complication. CStr and CString decide to provide full access to the UTF-8 error here, but OsStr and OsString do not. I decided to go with OsStr and OsString's API because they were simpler, but I personally think that the decision on what to do here should also be paired with one on what to do with the existing API discrepancy as well:

impl CStr {
    pub fn to_str(&self) -> Result<&str, Utf8Error>;
}
impl CString {
    pub fn into_string(self) -> Result<String, IntoStringError>;
}
impl OsStr {
    pub fn to_str(&self) -> Option<&str>;
}
impl OsString {
    pub fn into_string(self) -> Result<String, OsString>;
}
impl ByteStr {
    pub const fn to_str(&self) -> Option<&str>;
}
impl ByteString
    pub fn into_string(self) -> Result<String, ByteString>;
}

I also included these methods on [u8] and Vec<u8> because, currently, a lot of conversions from these types to ByteStr and ByteString are not available due to inference issues. Previously, these were resolved by making the actual type constructors public, but that forbids modifying the wrappers in the future, for example, to cache whether the string is valid UTF-8 like OsString currently does. So:

impl [u8] {
    pub fn as_byte_str(&self) -> &ByteStr;
    pub fn as_mut_byte_str(&mut self) -> &mut ByteStr;
    pub fn into_boxed_byte_str(self: Box<[u8]>) -> Box<ByteStr>;
}
impl Vec<u8> {
    pub fn into_byte_string(self) -> ByteString;
}

I think that the inference issues blocking all the relevant AsRef, AsMut, and From impls for these types should be seriously considered in terms of the future of this API. I'm avoiding this question for now since I think the changes to Deref will be needed regardless, but I added these methods to get around some of the issues in the meantime. If you'd like me to change the feature gate to put them under a different one, I can do that too, just to point out the issues here.

I would like to also apologise because I was mostly copying examples for half of them, then just got tired of it and copied the documentation for the methods only. We can add examples before stabilisation, and tests are unnecessary due to the fact that these are all shallow methods.

r? libs-api

[clarfonthey]

Okay, I truly don't understand this rustdoc error but ByteStr is not present at all in rustdoc's code so I'm going to assume it's unrelated for now.

[fmease]

What an odd one. However, I don't see how this test can possibly be spurious and it does seem related to your changes in a roundabout way. You might've exposed a rustdoc bug, but I haven't really investigated anything deeply.

The test fails because apparently we no longer display a "notable impl" tooltip advertising Iterator for sth. returning Vec<u8> and your patch touches lines relating to Vec<u8> (duh). So idk what the underlying issue is but it seems like rustdoc no longer "believes" impl Iterator for Vec<u8> in such cases.

[clarfonthey]

That is a very strange bug. I will definitely investigate later, but I definitely think that this PR is good enough to review for now.

[joshtriplett]

So, these types were modeled after the equivalents in bstr, which switched from an opaque-type model to a model that largely just has Deref, trait impls, and adds all the relevant methods to extension traits. I'd like to understand better why this seems to be doing the reverse.

In particular, why shouldn't most of these inherent methods live on Vec<u8> or [u8] instead?

[clarfonthey]

Not to be rude, but did you read the PR description at all? Because I feel like I went over my justification decently well and it's harder to respond to "I'd like to understand better" when you don't acknowledge any of the things I said to help people understand and why they didn't actually help.

[joshtriplett]

I did, in fact, read the entire PR description before asking that question, and I wouldn't have asked the question if the PR description had answered it.

For instance:

The main issue with ByteStr is that it's not a "real" string type: they're just wrappers over [u8] and Vec with barely any methods of their own, relying instead on Deref impls.

That was the intended design of ByteStr/ByteString. Any additional methods can go on [u8] and Vec<u8>, rather than the wrappers. The main reason the wrappers exist is to have different impls for things like Debug/Display, and to let people write more semantically clear type signatures.

the fact that the two types aren't linked via Deref means that any extra methods added to ByteStr would have to be added to ByteString too.

If you add methods to [u8] rather than ByteStr, then they're available on ByteString via the chain of derefs to Vec<u8> then &[u8].

Hence my question: why shouldn't most of these inherent methods live on Vec<u8> or [u8] instead?

[clarfonthey]

Pretty much all of these methods are on Vec or [u8] already. The biggest point, which you did not respond to, was this:

The inability to move out of deref here prevents methods like Vec::leak from being used without much effort,

This is just an aside:

the fact that the two types aren't linked via Deref means that any extra methods added to ByteStr would have to be added to ByteString too.

I also mention this a bit more on my comment on the tracking issue:

One massive issue with the current behaviour is the lack of any form of DerefMove, which means that any methods on Vec which consume the vector entirely do not work via ByteString. For example, calling into_boxed_slice on a ByteString to convert it into a Box<[u8]> will just throw an error. rustdoc is smart enough to not show these methods under the Deref section on ByteString, but users may still want to call them.

Also, while I admit that asking whether you actually read the description was mostly said in frustration, I was not trying to accuse you of actually not reading the description: the point was that it's not useful to simply ask for more explanation when you don't acknowledge the explanation I did give. Here, it's understandable that you honed in on the (understandably large) part of the explanation that focused on Deref, when the biggest dealbreaker for me was the lack of by-value conversion methods, which was comparably small. It was larger in my tracking issue comment, but I did not do that here.

Basically, I see this as a comparison of costs: on one hand, this solution I propose makes user actions on these types generally simpler while requiring additional methods to call more complicated methods directly. On the other, the existing solution makes the implementation simpler while requiring additional methods to call any by-value methods on Vec directly. It also relies heavily on trait implementations of From and AsRef to do most of these conversions, despite the fact that a lot of these trait implementations are missing due to coherence issues.

In my mind, increasing the cost of implementation is justified because we already bear the cost of implementation elsewhere. As mentioned, we basically implement our own versions of ByteStr and ByteString (called Slice and Buf internally) for the implementation of OsStr and OsString on many platforms, just, we don't expose this implementation directly. (While this PR does strictly increase the implementation burden, it's only temporary, and the plan is to get rid of the OsStr code later.)

I think that it's very reasonable to consider this a compromise over the current state of the Rust compiler and ask whether we really need a stable ByteString before something like DerefMove exists, or whether we can apply the same magic that allows moving out of boxes to ByteString specifically.

In that case, these problems would not exist and the implementation would remain simple, although the "issue" would still remain that Deref doesn't connect the types, and it's questionable whether anyone would need to rely on this for trait bounds. To me, ToOwned not matching Deref is very weird, and while it's mostly hypothetical at this stage, I worry that code which generalises over string types like CStr and OsStr would have to compromise itself for this quirk of ByteStr for not a very good reason. It's a very minor papercut, but one that still has to be considered.

And ultimately, the final decision to consider is whether we think that byte_str.obscure_slice_method() should be allowed over byte_str.as_bytes().obscure_slice_method() and whether we truly, never, ever would want to add a method to ByteStr that would not be present on [u8], and I don't know if that's a good API decision given the other problems, which range from breaking now + fixed in the future (no DerefMove) and free to implement now + annoying in the future (adding wrapper methods which could be replaced by derefs).

[bors]

☔ The latest upstream changes (presumably #135634) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

Some changes occurred in src/tools/cargo

cc @ehuss

[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.

[rust-log-analyzer]

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[WARNING] `tests/rustdoc-gui/search-result-display.goml` line 39: Delta is 0 for "x", maybe try to use `compare-elements-position` instead?

======== tests/rustdoc-gui/tooltips.goml ========

[ERROR] `tests/rustdoc-gui/tooltips.goml` line 8
    from `tests/rustdoc-gui/tooltips.goml` line 14: `a.tooltip` not found: for command `assert-css: (
            "a.tooltip", {"font-family": '"Source Serif 4", NanumBarunGothic, serif'}, ALL,
        )`
[ERROR] `tests/rustdoc-gui/tooltips.goml` line 8
    from `tests/rustdoc-gui/tooltips.goml` line 15: `a.tooltip` not found: for command `assert-css: (
            "a.tooltip", {"font-family": '"Source Serif 4", NanumBarunGothic, serif'}, ALL,
        )`


<= doc-ui tests done: 142 succeeded, 1 failed, 0 filtered out