expr: replace onig with fancy-regex - #14329
Conversation
| if is_start_of_expression { | ||
| re_string.push(curr); | ||
| } else { | ||
| match curr { |
There was a problem hiding this comment.
maybe move this into a new function?
d49f620 to
4bdf740
Compare
|
Binary size comparison: |
https://docs.rs/fancy-regex/latest/fancy_regex/#features maybe we can disable some features |
ccb43d7 to
734f140
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
| && !matches!( | ||
| curr, | ||
| '1'..='9' | ||
| | '.' | ||
| | '*' | ||
| | '^' | ||
| | '$' | ||
| | '[' | ||
| | ']' | ||
| | '\\' | ||
| | 'w' | ||
| | 'W' | ||
| | 's' | ||
| | 'S' | ||
| | 'b' | ||
| | 'B' | ||
| ) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
734f140 to
4c8805d
Compare
|
GNU testsuite comparison: |
This comment was marked as resolved.
This comment was marked as resolved.
4c8805d to
dab85cb
Compare
dab85cb to
ff7641e
Compare
There was a problem hiding this comment.
Pull request overview
This PR replaces expr’s onig (Oniguruma) dependency with the pure-Rust fancy-regex crate, updating the BRE-to-ERE transpilation and locale-aware matching logic to avoid a C library dependency and simplify cross-compilation (notably for WASI).
Changes:
- Switch
exprregex engine fromonigtofancy-regex, including a new BRE→ERE transpiler and updated match evaluation logic. - Update workspace and crate dependencies/locks to remove
onigand addfancy-regex. - Remove now-unneeded WASI sysroot installation steps from CI workflows and update documentation references.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/uu/expr/src/syntax_tree.rs | Replaces Oniguruma usage with fancy-regex, rewrites BRE→ERE transpilation, updates locale-aware match behavior, and adds unit tests. |
| src/uu/expr/Cargo.toml | Swaps onig dependency for fancy-regex. |
| Cargo.toml | Removes workspace onig dependency and adds fancy-regex workspace dependency configuration. |
| Cargo.lock | Lockfile updates reflecting removal of onig and addition of fancy-regex and its deps. |
| fuzz/Cargo.lock | Same lockfile updates for fuzz workspace. |
| README.md | Removes outdated mention of expr linking Oniguruma. |
| .github/workflows/wasi.yml | Removes WASI sysroot install step previously needed for C dependency. |
| .github/workflows/code-quality.yml | Removes WASI prerequisites step tied to Oniguruma. |
| .github/workflows/CICD.yml | Removes WASI sysroot setup tied to Oniguruma. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let re_string = transpile_bre_to_ere(&pattern_str)?; | ||
|
|
||
| RegexBuilder::new(&format!("(?s){re_string}")) | ||
| .oniguruma_mode(true) | ||
| .build() | ||
| .map_err(|_| ExprError::InvalidRegexExpression) |
|
Did you run benchmarks on it ? (hyperfine) |
|
ff7641e to
17f1407
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new invalid-UTF-8 matching path can compute incorrect match byte ranges (and potentially panic) due to mixing String byte offsets with original byte indices.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/9 changed files
- Comments generated: 1
- Review effort level: Lite
17f1407 to
b0c5c1c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new regex implementation appears to miss prior multiline matching behavior and has a bracket-mode parsing path that can skip the intended TrailingBackslash error handling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/uu/expr/src/syntax_tree.rs:431
- The previous onig-based implementation enabled both SINGLELINE and MULTILINE; the new fancy-regex pattern only sets dot-all via
(?s). Without multiline mode,$/^behavior can change (e.g.,$won’t match before a trailing newline), which risks breaking regex compatibility.
RegexBuilder::new(&format!("(?s){re_string}"))
.oniguruma_mode(true)
.build()
.map_err(|_| ExprError::InvalidRegexExpression)
- Files reviewed: 7/9 changed files
- Comments generated: 1
- Review effort level: Lite
| if in_bracket { | ||
| let is_first = re_string.len() == bracket_start_idx + 1 | ||
| || (re_string.ends_with("[^") && re_string.len() == bracket_start_idx + 2); | ||
| if curr == ']' && !curr_is_escaped && !is_first { | ||
| in_bracket = false; |
Replace the onig C-dependency in expr with the pure-Rust fancy-regex crate. This removes the onig_sys C library dependency while preserving full POSIX BRE, backreference, multibyte UTF-8, and C locale compatibility.
b0c5c1c to
92e9cad
Compare
|
I think |
Replace the onig C-dependency in expr with the pure-Rust fancy-regex crate.
This removes the onig_sys C library dependency while preserving full POSIX BRE,
backreference, multibyte UTF-8, and C locale compatibility.
Closes #1145