Add {floor,ceil}_char_boundary methods to str

[clarfonthey]

This is technically already used internally by the standard library in the form of truncate_to_char_boundary.

Essentially these are two building blocks to allow for approximate string truncation, where you want to cut off the string at "approximately" a given length in bytes but don't know exactly where the character boundaries lie. It's also a good candidate for the standard library as it can easily be done naively, but would be difficult to properly optimise. Although the existing code that's done in error messages is done naively, this code will explicitly only check a window of 4 bytes since we know that a boundary must lie in that range, and because it will make it possible to vectorise.

Although this method doesn't take into account graphemes or other properties, this would still be a required building block for splitting that takes those into account. For example, if you wanted to split at a grapheme boundary, you could take your approximate splitting point and then determine the graphemes immediately following and preceeding the split. If you then notice that these two graphemes could be merged, you can decide to either include the whole grapheme or exclude it depending on whether you decide splitting should shrink or expand the string.

This takes the most conservative approach and just offers the raw indices to the user, and they can decide how to use them. That way, the methods are as useful as possible despite having as few methods as possible.

(Note: I'll add some tests and a tracking issue if it's decided that this is worth including.)

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[clarfonthey]

(I know how to fix the error but my desktop is in literal pieces at the moment, so, I'll fix it this weekend.)

[scottmcm]

The functionality of these seems like a good thing to provide 👍

For the bikeshed, mixing "nearest" and "min/max" in the name is a bit confusing to me. Maybe char_boundary_begin and char_boundary_end or something? (And thus is_char_boundary == (char_boundary_begin == char_boundary_end).)

Or maybe, with subtly different semantics char_{begin|end}? So that s.char_begin(i) .. s.char_end(i) is the Range<usize> representing the USV that includes s.as_bytes()[i]?

That raises the question of endpoints, I guess. What's .nearest_char_boundary_min(usize::MAX) supposed to do?

[clarfonthey]

That raises the question of endpoints, I guess. What's .nearest_char_boundary_min(usize::MAX) supposed to do?

Forgetting the names (and I agree, it's hard to come up with something reasonable) the current behaviour here is to panic.

The one which is upper-bounded will currently not panic, which I think is good for truncating behaviour -- if you want your string to be under, say, 20 bytes, and it's empty, that's fine.

Lower-bounded ones must panic if the index is above len, since they can't go any lower.

If we were to use the concept of bounding the results to a specific character, then our issue is that we treat past the end as an "end of string" character, and this isn't C.

My initial terms for the methods were "largest char boundary not exceeding" and "smallest char boundary not below," and I figured those names sucked, but that's the intent that I wanted to preserve in the methods themselves.

[Mark-Simulacrum]

r? @dtolnay as T-libs-api to evaluate the addition of unstable API surface area. Implementations look OK to me modulo comments but I didn't evaluate the correctness of the search criteria (i.e., as casts and equality to -0x40).

[clarfonthey]

Note that the -0x40 was taken directly from is_char_boundary. I could factor out that bit into a separate private method (like byte_is_char_boundary) to avoid potential drift.

(Edit: I did that)

[clarfonthey]

That test passed for me locally, and I'm not 100% sure on how to fix it. I also know that u8::is_utf8_char_boundary should probably be inlined. Mostly want to wait for a more affirmative "yes, I like this API ± name bikeshedding" before I put more work into it.

[clarfonthey]

Rebased, would be nice if someone could take a look before I have to rebase again. 😉

[falk-hueffner]

This comment should probably move to is_utf8_char_boundary.

[dtolnay]

These method names and signatures look good to me. I was skeptical about the previous iterations but these are nice. Thanks for all the discussion above.

Please open a tracking issue and update the unstable attributes in the PR.

This is going to require more extensive test coverage prior to landing. See if you can find where the unit tests are for the other methods in this impl block.

[crlf0710]

@clarfonthey Ping from triage, would you mind addressing the comments above? Thanks!

[clarfonthey]

Hey, sorry that I took so long getting to this. Will add some more tests and see what I can do about fixing the implementation.

[clarfonthey]

Okay, this is finally ready for a proper review. I added a (hopefully) thorough set of tests and fixed the issue with ceil_char_boundary, which also did result in the code being more readable (IMHO) anyway.

@rustbot ready

[bors]

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

[scottmcm]

This is looking pretty good to me, code-wise, now.

Can you please add the tracking issue (as mentioned in #86497 (review))? With that I think it'll be good to go.

[clarfonthey]

Tracking issue made, also fixed the test being called ceil_char_bonudary instead of ceil_char_boundary 😳

Should be ready for final CR

[clarfonthey]

(I had accidentally committed the wrong version of a submodule when rebasing, so, hopefully it doesn't fail this time.)

[scottmcm]

Thanks for your persistence here! These look good to go for nightly.

@bors r+

If anyone has further thoughts, please use the tracking issue #93743 or a new bug cc'ing it.

[bors]

📌 Commit edd318c has been approved by scottmcm