Repository navigation
Fix --posix to disable the GNU regular expression escapes - #550
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.93% +0.08%
==========================================
Files 14 14
Lines 7230 7292 +62
Branches 432 434 +2
==========================================
+ Hits 6858 6923 +65
+ Misses 371 368 -3
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
|
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.
- 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: |
GNU sed drops the backslash of an escape it does not recognize, so do the same in the s replacement and in the y strings. The text of a, i and c already did. This also makes `y/x/\b/` compile again. Now that \b is not a character escape it produced two characters instead of one, and the two y strings must have equal lengths.
There was a problem hiding this comment.
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.
Copilot review overview
2 open issues
Review effort: Lite
|
GNU sed testsuite comparison: |
parse_regex is only called by the unit tests, so gate it behind cfg(test). The compiler calls parse_regex_for_mode, which is given the character and --posix modes of the run, and a --posix path can no longer reach a function that hardcodes posix = false. No backslash escape at all is recognized inside a bracket expression under --posix, not only the GNU ones, so say that instead.
|
GNU sed testsuite comparison: |
|
Thanks for your PR |
|
@sylvestre Thanks for the help! But I didn't squash. Do we want to? |
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(), so a GNU extension is never mistaken for a character escape.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.
Bracket expressions
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 what follows, so
[\w]holds a backslash and aw, and[a\]bc]ends at the first]. A[that does not open a POSIX class, collating symbol or equivalence is an ordinary member too, which also fixed[[x]and[x[].Under
--posixno backslash escape is recognized inside a bracket expression, so[\t]matches a backslash or atwhile\tstill matches a tab. The manual documents this forPOSIXLY_CORRECTand basic regular expressions, but GNU applies it to--posixand to extended regular expressions as well.The
\bescapeparse_char_escape()no longer decodes\bas a backspace, because sed has no such escape. The manual omits it on purpose:Removing it corrected four places at once:
\bis a word boundary in a regular expression, and a barebin thesreplacement, in theystrings and in the text ofa,iandc.That exposed a further divergence in the
sreplacement and theystrings, which emitted the backslash along with the character. An escape sed does not recognize now stands for the bare character, as GNU sed does, sos/x/a\qc/givesaqcandy/x/\b/givesbinstead of failing to compile.