Implement `to_string()` on `ByteStr` and `ByteString` by DanielEScherzer · Pull Request #159300 · rust-lang/rust

DanielEScherzer · 2026-07-14T18:27:24Z

View all comments

workingjubilee · 2026-07-14T20:27:22Z

but why?

DanielEScherzer · 2026-07-14T20:28:30Z

but why?

For #134915 but I wasn't going to link it until I got CI to pass

workingjubilee · 2026-07-14T20:29:55Z

hmm.

see also #134915 (comment)

workingjubilee · 2026-07-15T01:54:28Z

but why?

For #134915 but I wasn't going to link it until I got CI to pass

also for note this is a totally reasonable thing to do, sorry to bother, I only asked because the PR kind of made me do a double-take ("but ByteStr isn't even stable...?")

rustbot · 2026-07-17T21:56:17Z

r? @clarfonthey

rustbot has assigned @clarfonthey.
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: libs
libs expanded to 12 candidates
Random selection from 6 candidates

DanielEScherzer · 2026-07-17T21:56:30Z

If this looks good I'll squash it before merging

clarfonthey · 2026-07-24T02:08:52Z

+    /// Try to get a `String` representation of the `&ByteString`, if it is
+    /// valid UTF-8.
+    ///
+    /// This method is named `to_string()` because we want `ByteString` to
+    /// implement `Display`, but the `ToString` trait has a blanket
+    /// implementation for types that implement `Display`, and the trait version
+    /// will use the Unicode replacement character rather than returning a
+    /// `Result` and allowing for the possibility of the content not being UTF-8.
+    #[unstable(feature = "bstr", issue = "134915")]
+    #[rustc_allow_incoherent_impl]
+    pub fn to_string(&self) -> Result<String, FromUtf8Error> {
+        String::from_utf8(self.0.clone())
+    }


Based upon the precedent set by other APIs like CString and OsString, this should be into_string. That is a bit annoying, however, since it appears that OsString and PathBuf just return the old string on error, whereas CString has its own custom IntoStringError type. I don't like just copying that from std::ffi into std::bstr, but that seems like the best option.

View changes since the review

The idea was that this would ensure that people don't use the ToString blanket implementation for types implementing Display - calling this into_string() would leave the to_string() method being the problematic display version

We could switch this to be into_string() and then add a version of to_string() that emits deprecation warnings, but my understanding of the discussion on #134915 was that it was preferrable to just have to_string() do the conversion, but returning a result rather than being lossy

Note: I think it's fair to have both in this case, although at least with this method there is one key problem: the Vec is allocated before it's validated, and so, you end up throwing away the allocation if it's invalid UTF-8. I would expect this to at least run str::from_utf8 first and then do String::from_utf8_unchecked on the result if it passes.

So I started working on switching to that, but that would change the error type from alloc::string::FromUtf8Error (which has a copy of the bytes) to core::str::Utf8Error (which doesn't)

The allocation of the vec isn't thrown away if it is invalid UTF-8, it is just returned in the FromUtf8Error rather than in a String. I'm fine with switching, but wanted to make sure that the change in error type was an intentional part of the suggestion

Yes, it was.

clarfonthey · 2026-07-24T02:13:59Z

+impl ByteStr {
+    /// Try to get a `String` representation of the `&ByteStr`, if it is valid
+    /// UTF-8.
+    ///
+    /// This method is named `to_string()` because we want `ByteStr` to
+    /// implement `Display`, but the `ToString` trait has a blanket
+    /// implementation for types that implement `Display`, and the trait version
+    /// will use the Unicode replacement character rather than returning a
+    /// `Result` and allowing for the possibility of the content not being UTF-8.
+    #[unstable(feature = "bstr", issue = "134915")]
+    #[rustc_allow_incoherent_impl]
+    pub fn to_string(&self) -> Result<String, FromUtf8Error> {
+        String::from_utf8(self.0.to_vec())
+    }
+}


So, just seeing this code makes me a little sceptical about this due to the presence of the extra allocation also being a bit of a weird footgun.

Sure, to_string is automatically added by Display, but I would imagine most people would want this to be a shorthand for str::from_utf8, for which simply a to_str method would be fine. The main benefit of that is that if you want to pass &str to a method when you have the correct lifetime for the &ByteStr, it will Just Work tm, whereas if you do &s.to_string() it won't because you have a reference to a temporary. There may be APIs using AsRef<str> that get around this, but then you're just allocating a new string for no reason.

So, I think perhaps we could allow this, if we also have a to_str method that just calls str::from_utf8. But I'm still feeling a bit weird about it. Either way, no matter what we do, it should have a separate feature flag so it can be dealt with separately.

View changes since the review

clarfonthey · 2026-07-24T02:17:18Z

(Also apologies for taking so long to get to this; I'm going to try to get to my ByteStr refactor PR in a few days and will probably want to figure if this PR should happen before that or not.)

clarfonthey · 2026-08-01T02:38:37Z

@@ -65,3 +66,16 @@ fn test_display() {
    assert_eq!(&format!("{b2:-^6.3}!"), "-�(�--!");
    assert_eq!(&format!("{b2:->6.3}!"), "---�(�!");
 }
+
+#[test]
+fn test_to_string() {


Since we generally just do all string tests in alloctests, and this is redundant anyway, I'd remove it.

View changes since the review

clarfonthey · 2026-08-01T02:39:57Z

So, looking at this, final notes:

Mentioned the redundant test in coretests.
While not strictly required, it would be nice to name the feature bstr_to_string so that it's explicitly separate from the bstr feature, so we could potentially decide to stabilise it separately.

Otherwise, I'm fine merging this. Specifically, r=me with those changes.

Thank you for working on this!

clarfonthey · 2026-08-01T02:40:10Z

@rustbot author

rustbot · 2026-08-01T02:40:15Z

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

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 14, 2026

This comment has been minimized.

Sign in to view

DanielEScherzer mentioned this pull request Jul 14, 2026

Empty log snippets should not be guessed rust-lang/rust-log-analyzer#100

Open

This comment has been minimized.

Sign in to view

DanielEScherzer force-pushed the bytestr-to-string branch from c9b6c8c to 91ae0c4 Compare July 14, 2026 21:32

Implement to_string() on ByteStr and ByteString

06765dd

DanielEScherzer force-pushed the bytestr-to-string branch from 91ae0c4 to 06765dd Compare July 16, 2026 21:10

DanielEScherzer changed the title ~~Deprecate to_string() on ByteStr and ByteString~~ Implement to_string() on ByteStr and ByteString Jul 16, 2026

This comment has been minimized.

Sign in to view

Fixes

5c2ab60

This comment has been minimized.

Sign in to view

assert_matches

8ec3715

This comment has been minimized.

Sign in to view

No clippy lints

28c3c48

DanielEScherzer marked this pull request as ready for review July 17, 2026 21:56

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 17, 2026

rustbot assigned clarfonthey Jul 17, 2026

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 17, 2026

clarfonthey reviewed Jul 24, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

Validate UTF8 before allocation

8a12241

DanielEScherzer force-pushed the bytestr-to-string branch from 4115019 to 8a12241 Compare July 26, 2026 19:41

clarfonthey reviewed Aug 1, 2026

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 1, 2026

Uh oh!

Uh oh!

Conversation

DanielEScherzer commented Jul 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

workingjubilee commented Jul 14, 2026

Uh oh!

DanielEScherzer commented Jul 14, 2026

Uh oh!

workingjubilee commented Jul 14, 2026

Uh oh!

workingjubilee commented Jul 15, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented Jul 17, 2026

Uh oh!

DanielEScherzer commented Jul 17, 2026

Uh oh!

clarfonthey Jul 24, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielEScherzer Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

clarfonthey Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

DanielEScherzer Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfonthey Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

clarfonthey Jul 24, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfonthey commented Jul 24, 2026

Uh oh!

This comment has been minimized.

clarfonthey Aug 1, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfonthey commented Aug 1, 2026

Uh oh!

clarfonthey commented Aug 1, 2026

Uh oh!

rustbot commented Aug 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DanielEScherzer commented Jul 14, 2026 •

edited by rustbot

Loading

clarfonthey Jul 24, 2026 •

edited by rustbot

Loading

DanielEScherzer Jul 26, 2026 •

edited

Loading

clarfonthey Jul 24, 2026 •

edited by rustbot

Loading

clarfonthey Aug 1, 2026 •

edited by rustbot

Loading