Tracking Issue for utf8_chunks

[dylni]

Feature gate: #![feature(utf8_chunks)]

This is a tracking issue for an improved API for str::from_utf8.

Public API

// core::str

pub struct Utf8Chunks<'a> { ... }

impl<'a> Utf8Chunks<'a> {
    pub fn new(bytes: &'a [u8]) -> Self;
}

impl<'a> Iterator for Utf8Chunks<'a> {
    type Item = Utf8Chunk<'a>;
}

impl<'a> Clone for Utf8Chunks<'a>;
impl<'a> Debug for Utf8Chunks<'a>;
impl<'a> FusedIterator for Utf8Chunks<'a>;


pub struct Utf8Chunk<'a> { ... }

impl<'a> Utf8Chunk<'a> {
    pub fn valid(&self) -> &'a str;
    pub fn invalid(&self) -> &'a [u8];
}

impl<'a> Clone for Utf8Chunk<'a>;
impl<'a> Debug for Utf8Chunk<'a>;
impl<'a> PartialEq for Utf8Chunk<'a>;
impl<'a> Eq for Utf8Chunk<'a>;

Steps / History

• Proposal: Expose Utf8LossyChunksIter libs-team#54
• Implementation: Expose Utf8Lossy as Utf8Chunks #99544
• Final comment period (FCP)¹
• Stabilization PR

Unresolved Questions

• Should the constructor be Utf8Chunks::new or <[u8]>::utf8_chunks?
• Should Utf8Chunks::debug or a similar method be exposed?

Footnotes

1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html ↩

[dtolnay]

I'd be interested in using this to implement Display and Debug for CxxString in the cxx crate. Here is the current implementation without Utf8Chunks:

• https://github.com/dtolnay/cxx/blob/6432e9e5babd1207ecf1259262e023fd3a496bcc/src/cxx_string.rs#L210-L220
• https://github.com/dtolnay/cxx/blob/6432e9e5babd1207ecf1259262e023fd3a496bcc/src/lossy.rs

Here is what it looks like using Utf8Chunks as currently exists in nightly:

• https://github.com/dtolnay/cxx/pull/1289/files

Are there other known use cases so far that we could look at before an FCP? One thing I am interested in is how the current Utf8Chunks API compares with this alternative one, not based on Iterator, with just 1 type:

pub struct Utf8Chunks<'a>;

impl<'a> Utf8Chunks<'a> {
    pub fn next_valid(&mut self) -> &'a str;
    pub fn next_invalid(&mut self) -> &'a [u8];
}

• https://github.com/dtolnay/cxx/blob/5289ce332cde36d31c54faba42cd64a148c3f94f/src/lossy.rs

[dylni]

@dtolnay I am currently waiting on this ACP for stabilization.

Are there other known use cases so far that we could look at before an FCP?

I am aware of the following use cases.

• Lossy conversion (String::from_utf8_lossy)
• Debug formatting (as you mentioned)

I was originally going to use this feature in os_str_bytes for Debug formatting, but invalid returning individual "sequences" made this usage cumbersome. OsStr cannot be assumed to have the same invalid sequences. However, the individual sequences are required for lossy conversion to work with Utf8Chunks in its current form within libstd.

One thing I am interested in is how the current Utf8Chunks API compares with this alternative one, not based on Iterator, with just 1 type:

My concern is that the alternate API is easier to misuse (e.g., calling next_valid twice for two valid chunks). It also requires parsing each invalid sequence twice.

[Dylan-DPC]

@dylni generally, you don't need an ACP for this to stabilise (unless the team explicitly asked for it which I don't think happened in this case).
The next step is an FCP. In which case, you can submit a stabilisation pr for it linking this issue and preferably putting the report you shared here in that pr and then the team will run an fcp either in the pr or the issue.

[dylni]

@Dylan-DPC Right, but the problem is that the ACP would change the API. Stabilizing at this point would prevent the API change from landing.

[dtolnay]

@rust-lang/libs-api:
@rfcbot fcp merge

I propose stabilizing core::str::Utf8Chunks and core::str::Utf8Chunk ❗with the minor modification described in rust-lang/libs-team#190 ❗.

I recently wanted this API for C++ string Debug impls in cxx as described in #99543 (comment), and also in libproc_macro for synthesizing C-string literals in #123769.

[rfcbot]

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[BurntSushi]

Note that this same API, under the same name, is also in bstr. See also Utf8Chunks and Utf8Chunk. I can only identify two differences:

• bstr doesn't have a Utf8Chunks::new constructor. This seems okay.
• Utf8Chunk in std lacks bstr's Utf8Chunk::incomplete method. This seems okay and something we can add later if the use cases arise.

With that said, does this mean we also have an itertools situation here? bstr works by defining methods on [u8] via an extension trait.

[dtolnay]

Yeah there would be 2 "itertools situations":

Code that calls .utf8_chunks() on [u8] and then calls .incomplete()

This can be mitigated by adding support for incomplete in libcore's Utf8Chunk.

use bstr::ByteSlice as _;

fn main() {
    for chunk in b"..."[..].utf8_chunks() {
        if chunk.incomplete() {
            /* ... */
        }
    }
}

error[E0599]: no method named `incomplete` found for struct `std::str::Utf8Chunk` in the current scope
 --> src/main.rs:5:18
  |
5 |         if chunk.incomplete() {
  |                  ^^^^^^^^^^ method not found in `Utf8Chunk<'_>`

warning: unused import: `bstr::ByteSlice`
 --> src/main.rs:1:5
  |
1 | use bstr::ByteSlice as _;
  |     ^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

Code that calls .utf8_chunks() on [u8] and expects the return type to be bstr::Utf8Chunks, as opposed to core::str::Utf8Chunks

This can be mitigated by having bstr check the compiler version and re-export libcore's Utf8Chunks and Utf8Chunk, instead of defining its own types.

use bstr::ByteSlice as _;

fn main() {
    let mut chunks: bstr::Utf8Chunks = b"..."[..].utf8_chunks();
    let chunk = chunks.next().unwrap();
    let _valid = bstr::Utf8Chunk::valid(&chunk);
}

error[E0308]: mismatched types
   --> src/main.rs:4:40
    |
4   |     let mut chunks: bstr::Utf8Chunks = b"..."[..].utf8_chunks();
    |                     ----------------   ^^^^^^^^^^^^^^^^^^^^^^^^ expected `bstr::Utf8Chunks<'_>`, found `std::str::Utf8Chunks<'_>`
    |                     |
    |                     expected due to this
    |
    = note: `std::str::Utf8Chunks<'_>` and `bstr::Utf8Chunks<'_>` have similar names, but are actually distinct types
note: `std::str::Utf8Chunks<'_>` is defined in crate `core`
   --> /rust/library/core/src/str/lossy.rs:175:1
    |
175 | pub struct Utf8Chunks<'a> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^
note: `bstr::Utf8Chunks<'_>` is defined in crate `bstr`
   --> ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bstr-1.9.1/src/utf8.rs:227:1
    |
227 | pub struct Utf8Chunks<'a> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^

[BurntSushi]

Thanks @dtolnay! I filed an issue on bstr: BurntSushi/bstr#181

I think it would be nice to have Utf8Chunk::incomplete included here, as that would make it easier to mitigate potential breakage. But I'm not sure it's worth blocking stabilization on that.

[dtolnay]

Do you happen to know any real-world usages of incomplete? If not, I can try crater on the stabilization PR to find them.

I would prefer not to blindly copy incomplete from bstr without having a sense of whether it's independently justifiable to include in core.

[BurntSushi]

I don't have a specific example to point to unfortunately. But it's similar in concept at least to Utf8Error::error_len. My guess is that if you can find a real world use case for Utf8Error::error_len, then it would also serve as a use case for Utf8Chunk::incomplete. That is, it's useful in a streaming context where a single codepoint might straddle chunk boundaries. @m-ou-se actually added it to bstr long ago: BurntSushi/bstr#68

[m-ou-se]

I don't remember exactly what I needed that function for, but I'm sure I sent that PR because I actually needed it. I can't find any uses of it in any of the code I currently have checked out, but it looks like at that time I was working on two relevant (never finished) projects where I might have needed this: 1. a terminal emulator, and 2. some embedded project with serial communication in chunks.

There is some separate discussion on use cases in this issue: BurntSushi/bstr#42

[coolreader18]

Unless Utf8Chunks::new is intended as a temporary solution until there can be a utf8_chunks method on [u8] (à la array::IntoIter::new) I feel like I would expect the construction function to be core::str::utf8_chunks(&[u8]) -> Utf8Chunks<'_>. This is a different type of operation, but for example, std::fs::read_dir() returns std::fs::ReadDir, you don't call std::fs::ReadDir::new(). new implies an operation, just like any other function, and it semantically makes sense to me to create a new Vec or Cell or io::Error, but not to create a new Utf8Chunks. It's, like... a behavior type, not a data type.

[dtolnay]

GitHub search for "chunk.incomplete()" (guessing a likely variable name) shows 0 hits. GitHub search for ".incomplete()" shows 123 hits, of which I looked through all of them and 0 are related to utf8 chunks. In #123909, crater uncovered 0 uses of incomplete across crates.io and GitHub.

As such, I recommend that we do not import bstr's incomplete into core until such time as it is better justified.

Crater showed a single occurrence of the second type of expected break in #99543 (comment). The affected crate is actively developed (within the past 5 days) and is not depended on by any other crate on crates.io. This breakage is allowed by the standard library's API evolution policy (https://rust-lang.github.io/rfcs/1105-api-evolution.html#minor-change-adding-any-inherent-items).

I recommend that we move forward with #123909 as written (when the FCP completes in 2 days, of course).

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.