cut: char-aware multibyte field, byte and delimiter handling - #13927
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR makes cut and paste locale-aware for multibyte behavior: treating C/POSIX as single-byte, ensuring delimiters are matched on character boundaries (not inside multibyte characters), and expanding -w whitespace handling to recognize Unicode “blank” characters in UTF-8 locales.
Changes:
- Update i18n charmap logic to treat
C/POSIXas strictly single-byte and add Unicode “blank” detection for-w. - Update
cutdelimiter matching to avoid splitting multibyte characters and add corresponding regression tests. - Update
pastedelimiter parsing and tests to run under a UTF-8 locale when multibyte delimiters are involved.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/by-util/test_paste.rs | Sets a UTF-8 locale for multibyte-delimiter paste tests. |
| tests/by-util/test_cut.rs | Adds UTF-8-locale regression tests for cut byte/field behaviors with multibyte characters/delimiters and Unicode blanks. |
| src/uucore/src/lib/features/i18n/charmap.rs | Makes C/POSIX strictly single-byte and introduces mb_blank_len for Unicode blank detection in UTF-8 locales. |
| src/uu/paste/src/paste.rs | Uses mb_char_len directly for delimiter tokenization (character-stepped). |
| src/uu/cut/src/matcher.rs | Adds a multibyte-aware exact matcher and updates whitespace matching to use locale-aware blank detection. |
| src/uu/cut/src/cut.rs | Uses MbExactMatcher for non-ASCII delimiters and refactors field-cutting to share matcher/out-delim handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let mut pos = 0; | ||
| while pos < haystack.len() { | ||
| if let Some(blank_len) = mb_blank_len(&haystack[pos..]) { | ||
| let start = pos; | ||
| pos += blank_len; | ||
| while pos < haystack.len() { | ||
| match mb_blank_len(&haystack[pos..]) { | ||
| Some(len) => pos += len, | ||
| None => break, | ||
| } | ||
| } | ||
| return Some((start, pos)); | ||
| } | ||
| pos += mb_char_len(&haystack[pos..]); | ||
| } | ||
|
|
||
| Some((match_idx, skip)) | ||
| None |
| pub fn mb_blank_len(bytes: &[u8]) -> Option<usize> { | ||
| let len = mb_char_len(bytes); | ||
| if len == 1 { | ||
| return (bytes[0] == b' ' || bytes[0] == b'\t').then_some(1); | ||
| } | ||
| // `char_len` only looks at the shape of the sequence, so decode it for real | ||
| // here: an overlong or surrogate encoding is not a character at all, and | ||
| // must not pass for one of the blanks it decodes to. | ||
| if locale_encoding() == Encoding::Utf8 | ||
| && std::str::from_utf8(&bytes[..len]).is_ok_and(|s| s.starts_with(is_unicode_blank)) | ||
| { | ||
| return Some(len); | ||
| } | ||
| None | ||
| } |
6048734 to
3ab2ecd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/uu/cut/src/cut.rs:970
- This validation now only checks “UTF-8 shape” via
mb_char_lenand may accept ill-formed UTF-8 byte sequences (e.g., surrogate-encoded triples) as a “single character” in a UTF-8 locale. In UTF-8 locales, delimiter validation should additionally require the bytes decode as UTF-8 and contain exactly one Unicode scalar value (while keeping the current byte-length logic for non-UTF-8 encodings).
// The delimiter must be a single character in the *current locale's*
// encoding: a multi-byte UTF-8 character in a UTF-8 locale, a 2-byte
// GB18030 character, or any single byte like `b"\xAD"`. In a C/POSIX
// (single-byte) locale even a valid UTF-8 multibyte sequence counts
// as several characters, matching GNU.
let bytes = os_str_as_bytes(os_string)?;
let is_single_char = mb_char_len(bytes) == bytes.len();
if !is_single_char {
src/uu/cut/src/matcher.rs:73
MbExactMatcher::next_matchis O(n) over the full input slice and resolves locale encoding on every call; if the surroundingSearcherinvokesnext_matchrepeatedly while scanning, this can easily become O(n²) per line. Consider resolvingEncodingonce per line (or storing it in the matcher) and designing the search to advance incrementally over character boundaries (stateful searcher/iterator) rather than restarting frompos = 0each call.
fn next_match(&self, haystack: &[u8]) -> Option<(usize, usize)> {
// Resolved once per line: character boundaries have to be walked from
// the start, so there is no way to skip ahead with `memchr` here.
let encoding = locale_encoding();
let mut pos = 0;
while pos < haystack.len() {
let clen = encoding.char_len(&haystack[pos..]);
if clen == self.needle.len() && &haystack[pos..pos + clen] == self.needle {
return Some((pos, pos + clen));
}
pos += clen;
}
None
}
tests/by-util/test_paste.rs:434
- This sets
LC_ALL=C.UTF-8for all paste data examples, although the comment says only “some” require it. It would be more precise (and better at catching regressions in the C locale) to set the locale only for examples that actually contain multibyte delimiters (e.g., by tagging such cases in the test vector or checking the delimiter bytes).
// Some examples use multibyte delimiters, which need a UTF-8 locale.
ucmd.env("LC_ALL", "C.UTF-8")
.args(example.args)
| if self == Self::Utf8 | ||
| && std::str::from_utf8(&bytes[..len]).is_ok_and(|s| s.starts_with(is_unicode_blank)) | ||
| { | ||
| return Some(len); | ||
| } | ||
| None |
3ab2ecd to
69568dd
Compare
Merging this PR will degrade performance by 10.2%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
|
GNU testsuite comparison: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/uu/cut/src/matcher.rs:92
- The fast
memchr2path is currently limited toEncoding::SingleByte, but for the other non-UTF-8 multibyte encodings supported here (GB18030/EUC/Big5),blank_len()will only ever match ASCII space/tab anyway. You can likely extend the fast path to allencoding != Encoding::Utf8to avoid per-character decoding on large inputs in those locales, while preserving correctness.
// In a single-byte locale SPACE and TAB are the only blanks, so the run
// can be found with a SIMD scan instead of decoding every character.
let encoding = locale_encoding();
if encoding == Encoding::SingleByte {
let start = memchr2(b' ', b'\t', haystack)?;
let mut end = start + 1;
while end < haystack.len() && matches!(haystack[end], b' ' | b'\t') {
end += 1;
}
return Some((start, end));
}
src/uu/cut/src/cut.rs:860
- With this PR, whitespace delimiter matching under
-wrecognizes Unicode blanks in a UTF-8 locale, but the--whitespace-delimited=trimmedcode path (viacut_fields_whitespace_trimmed) still trims using only ASCII space/tab (per the current implementation in context). This creates inconsistent behavior where leading/trailing Unicode blanks won’t be trimmed even though they’re treated as delimiters. Consider updating the trimming logic to use the same blank definition (e.g.,Encoding::blank_len/ the whitespace matcher) sotrimmedremains consistent with delimiter detection.
Delimiter::Whitespace => {
let out_delim = opts.out_delimiter.unwrap_or(b"\t");
if field_opts.whitespace_trimmed {
src/uu/cut/src/matcher.rs:66
- The comment says the encoding is “Resolved once per line”, but
locale_encoding()is resolved pernext_match()call. Consider either (a) adjusting the comment, or (b) storingEncodinginsideMbExactMatcher(or passing it in) to align implementation with the comment and avoid repeated lookups.
impl Matcher for MbExactMatcher<'_> {
fn next_match(&self, haystack: &[u8]) -> Option<(usize, usize)> {
// Resolved once per line: character boundaries have to be walked from
// the start, so there is no way to skip ahead with `memchr` here.
let encoding = locale_encoding();
let mut pos = 0;
while pos < haystack.len() {
let clen = encoding.char_len(&haystack[pos..]);
|
Binary size comparison: |
69568dd to
cd30e8e
Compare
cd30e8e to
8028d90
Compare
|
Is it intentional that this PR also contains changes of |
8028d90 to
15eabab
Compare
15eabab to
8028d90
Compare
Treat the C/POSIX locale as single-byte in the charmap, match field delimiters on character boundaries (never inside a multibyte character), and recognize Unicode whitespace for -w. Should make test tests/cut/cut.pl pass
… in tests mb_char_len now guarantees the returned length never exceeds the slice size, making .min(remaining.len()) redundant. Set LC_ALL=C.UTF-8 in multibyte-delimiter tests and skip them under wasi_runner.
8028d90 to
96dbf26
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/uu/cut/src/matcher.rs:80
WhitespaceMatchernow treats additional Unicode “blank” characters as delimiters in UTF-8 locales, but the--whitespace-delimited=trimmedpath incut.rstrims only ASCII space/tab before splitting. This means leading/trailing Unicode blanks can survive “trimmed” preprocessing and yield empty/phantom fields even though they’re treated as delimiters later; trimming should use the same blank definition as matching (e.g.,Encoding::blank_len).
// Matches any number of whitespace characters. ASCII space and tab are always
// recognized; in a UTF-8 locale Unicode space separators are too.
pub struct WhitespaceMatcher {}
impl Matcher for WhitespaceMatcher {
src/uu/cut/src/cut.rs:968
- In
get_delimiters, the--output-delimiterconversion later in this function usesos_str_as_bytes(os_string).unwrap(), which can panic on non-unix platforms if the OS string can’t be coerced to UTF-8 (seeuucore::os_str_as_bytes). Since this function already returnsUResult, it should propagate or map that error instead of unwrapping so invalid input is reported gracefully.
// The delimiter must be a single character in the *current locale's*
// encoding: a multi-byte UTF-8 character in a UTF-8 locale, a 2-byte
// GB18030 character, or any single byte like `b"\xAD"`. In a C/POSIX
// (single-byte) locale even a valid UTF-8 multibyte sequence counts
// as several characters, matching GNU.
mb_char_len was introduced/fixed in this very PR to always return a length ≤ slice size, making .min(remaining.len()) redundant - so paste had to be updated to use the new guarantee. |
Treat the C/POSIX locale as single-byte in the charmap, match field
delimiters on character boundaries (never inside a multibyte character),
and recognize Unicode whitespace for -w. paste tests that exercise
multibyte delimiters now declare a UTF-8 locale.
Should make test tests/cut/cut.pl pass
Stack created with GitHub Stacks CLI • Give Feedback 💬