Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #550 +/- ##
==========================================
+ Coverage 94.85% 94.89% +0.04%
==========================================
Files 14 14
Lines 7230 7294 +64
Branches 432 434 +2
==========================================
+ Hits 6858 6922 +64
Misses 371 371
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
The new integration tests don’t yet cover all GNU escapes called out by #542 (notably \+ and \|), reducing end-to-end regression protection for the claimed fix.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the sed script regex parser so that when --posix is enabled, GNU regex escape extensions (e.g., \?, \+, \|, \w, \s, etc.) are treated as literals rather than GNU-specific operators/classes/anchors, aligning behavior with GNU sed and fixing #542.
Changes:
- Thread
context.posixinto regex parsing (parse_regex_for_mode) and handle GNU escape extensions as literals under POSIX mode (with special handling for ERE metacharacters? + |). - Add end-to-end regression tests validating
--posixdisables key GNU regex escapes, plus unit tests covering BRE vs ERE emission rules.
File summaries
| File | Description |
|---|---|
| tests/by-util/test_sed.rs | Adds integration tests for --posix behavior around GNU regex escape extensions. |
| src/sed/delimited_parser.rs | Implements --posix parsing behavior for GNU regex escapes and adds targeted unit tests. |
| src/sed/compiler.rs | Passes context.posix into regex parsing for addresses and substitution patterns. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@sylvestre While working on this I noticed that $ printf 'cat catalog\n' | sed 's/\<cat\>/X/g'
X catalog
$ printf 'cat catalog\n' | ./target/debug/sed 's/\<cat\>/X/g'
cat catalogThe fix looks small, and differs per escape. All three engines support
This is the separate problem from this PR, so I left it out. Do you want to add it in this PR or open a separate issue for it? |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, threads --posix consistently through the compiler, and includes both unit and integration tests covering the corrected behavior.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
| continue; | ||
| } | ||
| if posix && is_gnu_regex_escape(line.current()) { | ||
| // ? + | must keep their backslash to remain literals. |
There was a problem hiding this comment.
in ERE the fallback below already emits \? as is, could you just skip this branch for ? + | there instead of pushing the backslash again?
|
|
||
| #[test] | ||
| fn test_posix_keeps_posix_regex_constructs() { | ||
| // Groups, quantifiers, operators and character escapes are POSIX, |
There was a problem hiding this comment.
\x61 isn't POSIX, does GNU sed really keep it under --posix? please double check :)
There was a problem hiding this comment.
Verified, sed keeps it under --posix. But the test name doesn't capture the case so I'll move it in a new one including \o141 and \d097.
|
GNU sed testsuite comparison: |
Merging this PR will not alter performance
|
|
GNU sed testsuite comparison: |
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved review issues were identified.
1 open finding
1 resolved since last review
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| break; | ||
| } | ||
| if let Some(decoded) = parse_char_escape(line) { | ||
| let decoded = if posix { None } else { parse_char_escape(line) }; |
There was a problem hiding this comment.
under --posix [\n] now holds \ and n, are you sure GNU sed doesn't still turn it into a newline there?
printf 'a\nb\n' | sed --posix 'N;s/[\n]/X/' please check and add a test :)
There was a problem hiding this comment.
Just verified it holds \ and n under --posix so the newline survives. I'll include a test.
| // Outside the character escapes a \ is an ordinary member of the set, | ||
| // so the next character is left for the loop to handle. This is what | ||
| // makes `[a\]bc]` end at the first `]`, as GNU sed does. | ||
| result.extend_from_slice(br"\\"); |
There was a problem hiding this comment.
this also changes [\/] (escaped delimiter in a class): it now holds \ and /, GNU keeps only /
echo 'a\b/c' | sed 's/[\/]/X/g' should give a\bXc
could you please handle the delimiter before this and add a test? common idiom for paths
There was a problem hiding this comment.
GNU keeps both members here. Inside a bracket expression the backslash doesn’t quote the delimiter, it’s an ordinary member of the set:
$ echo 'a\b/c' | sed 's/[\/]/X/g'
aXbXc
$ echo 'a\b/c' | ./target/debug/sed 's/[\/]/X/g'
aXbXc- Add a POSIX parameter to `parse_regex_for_mode()` that emits the literal character for `\?`, `\+`, `\|`, `\w`, `\W`, `\s`, `\S`, `\b`, `\B`, `\<`, `\>`, `` \` `` and `\'` - Emit the character bare in BRE, where `bre_to_ere()` escapes it afterwards, and keep the backslash on `\?`, `\+` and `\|` in ERE - Add unit and integration tests
A backslash is only special inside a bracket expression when it starts a character escape. Elsewhere it is an ordinary member of the set and does not quote the character that follows, so `[\w]` matches a backslash or a w, and `[a\]bc]` ends at the first `]`. This matches GNU sed in both basic and extended mode.
A '[' inside a bracket expression that does not open a POSIX class, collating symbol or equivalence is an ordinary member of the set. It was emitted together with the character that followed it, so the ']' in `[\[]` was swallowed and the expression was reported as unterminated. sed has no \b character escape, so remove it from parse_char_escape. A backslash in `[\b]` is an ordinary member of the set, '\b' in a regular expression is a word boundary, and in text and transliteration strings it stands for a bare 'b'. Disable the remaining GNU character escapes inside bracket expressions under --posix. They stay available outside them, as GNU sed does, so `[\t]` matches a backslash or a t while `\t` still matches a tab.
|
GNU sed testsuite comparison: |

Fixes #542
Under
--posix, GNU sed treats the GNU regular expression escapes as the literal character that follows the backslash. These are the operators\?,\+and\|, the character classes\w,\W,\sand\S, and the empty-width matches\b,\B,\<,\>,\`and\'.parse_regex_for_mode()now takes a POSIX parameter and emits the literal character for these escapes. The check comes beforeparse_char_escape(), otherwise\bis decoded as a backspace.BRE and ERE
The two modes need the opposite spelling of the same literal:
bre_to_ere()escapes the ERE metacharacters afterwards. Emitting\?would makebre_to_ere()turn it back into the operator.\?,\+and\|keep their backslash, since a backslash is already what makes them literals there. Emitting them bare would make them operators.The remaining escapes are letters and punctuation with no meaning of their own, so they are emitted bare in both modes.