mruby-regexp: read the \u escape - #7074
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe regexp gem adds fixed-width and braced Unicode escapes. It validates and encodes codepoints as UTF-8, supports lists in character classes, updates quantifier and case-folding behavior, and adds tests and documentation. ChangesUnicode regexp escape support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Pattern
participant RegexpCompiler
participant UTF8Encoder
participant Bytecode
Pattern->>RegexpCompiler: provide Unicode escape
RegexpCompiler->>RegexpCompiler: parse and validate codepoint
RegexpCompiler->>UTF8Encoder: encode codepoint
UTF8Encoder-->>RegexpCompiler: return UTF-8 bytes
RegexpCompiler->>Bytecode: emit atom instructions
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This needs a rebase: #7058 landed as 3bc4b88 and it adds The textual part is mechanical, both sides being new functions side by side. The part I would rather you decide than guess at is what On your branch as it stands: Regexp.new("\\u00c0", Regexp::IGNORECASE) =~ "à" # nilOn master, the same character written literally: Regexp.new("À", Regexp::IGNORECASE) =~ "à" # RegexpErrorA naive merge keeps both, and then one character answers "silently no match" when it is named by codepoint and "refused" when it is spelled, in the same build. That is the split #7058 exists to remove, so whichever way it goes it should go the same way for both spellings. I have no view on where The rest I checked before running into this. All seven rows in your table reproduce on master, |
`compile_atom()` spells the /i literal out twice, once for the escape path and once for the plain one, and each spelling branches on the letter's case to pick the byte to fold to. `compile_charclass()` likewise decides inline which side of 128 a class member belongs to. A `\u` escape is about to want all of it: it emits a literal, it adds class members, and it reads hex digits the way `\xHH` already does. Lift the three out. `emit_char()` emits one byte, folding an ASCII letter to a two-member class under /i, and reaches the other case with `ch ^ 0x20` rather than a branch per case. `class_add_member()` routes a codepoint to the ASCII bitmap or to the codepoint range list. `hex_value()` reads one hex digit, returning -1 for anything else, which covers the -1 that `peek()` gives at the end of the pattern. `emit_char()` sits beside `emit_char_bytes()` and names the same job for a character that is a single byte, so the escape path and the plain path reach one of the two by the width of what they read. No behaviour change. The folding condition already implied a byte below 128, so the plain path's separate `ch < 128` test drops with it.
`parse_escape()` knows `\n`, `\t`, `\xHH` and the octal form, and returns every
other escaped character as itself. `\u` is not among them, so the backslash is
dropped and the `u` becomes an ordinary literal, along with whatever follows
it.
```ruby
"µ" =~ /\u00b5/ # CRuby: 0, mruby: nil
"u00b5" =~ /\u00b5/ # CRuby: nil, mruby: 0
"µ" =~ /[\u00b5]/ # CRuby: 0, mruby: nil
"あ" =~ /\u{3042}/ # CRuby: 0, mruby: nil
"ab" =~ /\u{61 62}/ # CRuby: 0, mruby: nil
```
The parser is not involved. `/\u00b5/.source` is `"\\u00b5"` in both, so the
escape reaches the engine intact and the engine is where it is lost.
What the leftover text then means depends on the spelling, and neither reading
is the one that was written. `\uXXXX` leaves five ASCII characters, so a
pattern that names one codepoint quietly names the text `u00b5`, and a subject
that happens to carry that text matches. `\u{...}` leaves a `u` followed by a
brace group, so a body that reads as a repetition count becomes a quantifier on
the `u`: `/\u{3042}/` matches 3042 `u`s and nothing else, and `/\u{110000}/`
raises `RegexpError: quantifier too large`, naming something the pattern never
said.
```ruby
("u" * 3042) =~ /\u{3042}/ # CRuby: nil, mruby: 0
```
Read the escape instead. `\uXXXX` takes exactly four hex digits, `\u{...}` one
to six per codepoint and several codepoints separated by whitespace. A
codepoint above 127 becomes its UTF-8 byte sequence, emitted as the run of
`RE_CHAR` that `emit_char_bytes()` already produces for a multibyte literal, so
the run is one atom and a following quantifier repeats the whole character
rather than its last byte. The bytes come from `mrb_re_utf8_encode()`, the
encoding counterpart of the `mrb_re_utf8_decode()` the gem already has, since
a named codepoint is not spelled out in the pattern to copy from.
Under `/i` a codepoint above 127 takes the route a spelled out literal takes
since `emit_char_folded()` arrived: the class of its case counterparts, or a
refusal when the build has no folding for the character. Naming a character
rather than spelling it does not change what `/i` means by it, so `/\u{212a}/i`
and `/K/i` compile alike.
The branch has to sit ahead of the `ch >= 0xC0` one that routes a backslash
before a multibyte character away from `parse_escape()`, and ahead of the
matching `peek(c) < 0xC0` guard in `read_class_atom()`. `u` is below `0xC0`,
so either would otherwise take `\u` back to `parse_escape()` as a literal.
The list form is where the two contexts part. Outside a character class
`/\u{61 62}/` is a sequence of atoms rather than one, and a quantifier after it
repeats the last codepoint alone, so `compile_quantified()` now takes the
atom's start from the compiler state rather than from the position it saved:
the `\u` case moves it past every codepoint but the last. Inside a class each
codepoint is a member of its own, and the last one is handed back to
`compile_charclass()` so it can still open a range, as in `/[\u{61 62}-z]/`.
The malformed spellings raise instead of falling back on a shorter codepoint or
on literal text, with CRuby's messages. A short `\uXX`, an overlong
`\u{0000061}`, an out of range `\u{110000}`, a surrogate and an empty `\u{}`
each used to be a quiet wrong answer or an error about a quantifier.
Whitespace separates the codepoints of a list, so `preprocess_pattern()` copies
a `\u{...}` group whole. The free-spacing pass runs before the parser and would
otherwise join `/\u{61 62}/x` into the single codepoint `\u{6162}`.
The README lists the escape alongside the character escapes that already
worked, which it never mentioned either.
a259474 to
048e5da
Compare
|
Rebased, onto On the question you left to me: the escape reaches the folding the way a literal does, as you guessed. Both spellings answer alike now, and both spellings of the escape do too: # Build without MRB_REGEXP_UNICODE_CASE
Regexp.new("À", Regexp::IGNORECASE) # RegexpError
Regexp.new("\\u00c0", Regexp::IGNORECASE) # RegexpError
Regexp.new("\\u{c0}", Regexp::IGNORECASE) # RegexpError
Regexp.new("[\\u{c0}]", Regexp::IGNORECASE) # RegexpError
# Build with MRB_REGEXP_UNICODE_CASE
Regexp.new("À", Regexp::IGNORECASE) =~ "à" # 0
Regexp.new("\\u00c0", Regexp::IGNORECASE) =~ "à" # 0A character class was already consistent before this, since a class member is a codepoint whichever way it was written and the closure runs over the finished class. The literal path is what needed the change. The tests for it sit with the rest of the case folding tests rather than with the Checked both builds, and checked that the tests fail without the change: without it the option build answers |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mrbgems/mruby-regexp/test/ascii_case.rb (1)
29-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the option-specific message for Unicode escape paths.
Lines 29-33 check only
RegexpError. Lines 34-39 checkMRB_REGEXP_UNICODE_CASEonly for the literal path. A regression can return a genericRegexpErrorfor\uXXXX,\u{...}, or character-class escapes and still pass. Check that each escape parser path reports the option-specific message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mrbgems/mruby-regexp/test/ascii_case.rb` around lines 29 - 39, Update the Unicode escape assertions in the RegexpError tests for \uXXXX, \u{...}, and character-class escape paths to also verify that the exception message includes “MRB_REGEXP_UNICODE_CASE”. Preserve coverage for the existing escape variants and ensure each assertion validates both the error type and option-specific message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@mrbgems/mruby-regexp/test/ascii_case.rb`:
- Around line 29-39: Update the Unicode escape assertions in the RegexpError
tests for \uXXXX, \u{...}, and character-class escape paths to also verify that
the exception message includes “MRB_REGEXP_UNICODE_CASE”. Preserve coverage for
the existing escape variants and ensure each assertion validates both the error
type and option-specific message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e75e4d22-3937-41ab-91fe-f2d81bb9705e
📒 Files selected for processing (7)
mrbgems/mruby-regexp/README.mdmrbgems/mruby-regexp/include/re_internal.hmrbgems/mruby-regexp/src/re_compile.cmrbgems/mruby-regexp/src/re_utf8.cmrbgems/mruby-regexp/test/ascii_case.rbmrbgems/mruby-regexp/test/regexp.rbmrbgems/mruby-regexp/test/unicode_case.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- mrbgems/mruby-regexp/src/re_utf8.c
- mrbgems/mruby-regexp/README.md
- mrbgems/mruby-regexp/include/re_internal.h
- mrbgems/mruby-regexp/src/re_compile.c
The `\u` cases asserted `RegexpError` and nothing else, and a `\u`
pattern has reasons of its own to raise that class: a regression in the
escape parser could report a malformed escape for `\u{100}` and the test
would still pass. Assert the message instead. It names the option, and
its tail says whether the character path or the character class path
refused, so each spelling is pinned to the refusal it is there for.
`assert_raise_with_message` also replaces the `begin`/`rescue` that
checked the literal path's message, which asserted nothing at all when
the pattern compiled.
|
The CodeRabbit nitpick on The point was fair. The The
|
2 participants
The regexp engine has no
\uescape.parse_escape()(
re_compile.c:339) knows\n,\t,\r,\f,\v,\a,\e,\b, theoctal form
\NNNand the hex form\xHH, and returns every other escapedcharacter as itself (
re_compile.c:389).\uis not among them, so thebackslash is dropped and the
ubecomes an ordinary literal, along withwhatever follows it.
The parser is not involved.
/\u00b5/.sourceis"\\u00b5"in both, so theescape reaches the engine intact and the engine is where it is lost.
Cause
What the leftover text means depends on the spelling, and neither reading is
the one that was written.
\uXXXXleaves five ASCII characters. A pattern that means one codepointquietly means
u00b5, and a subject that happens to carry that text matches. Ashort
\uXX, which is aRegexpErrorin CRuby, likewise matches the textuXX.\u{...}leaves aufollowed by a brace group, so a body that reads as arepetition count becomes a quantifier on the
u./\u{3042}/isu{3042},which matches 3042
us and nothing else, and/\u{110000}/raisesRegexpError: quantifier too large, naming something the pattern never said. Abody that is not a count stays literal, so
/\u{b5}/matchesu{b5}, and/\u{}/matchesu{}where CRuby raisesinvalid Unicode list.So the quiet wrong answer is the common case, but it is not the only one: the
\u{...}spelling can also fail loudly under a message about a quantifier theauthor never wrote.
Neither the README's
Pattern Syntaxlist nor itsLimitationssection saidanything about
\u, and the character escapes that do work (\n,\t,\xHH,\NNN) were not listed either, so the gap was not written downanywhere.
Fix
Read the escape.
unicode_escape_first()takes\uXXXXas exactly four hexdigits, or opens a
\u{...}list whose codepointsunicode_escape_next()yields one at a time, one to six hex digits each and whitespace between them.
A codepoint above 127 has to become its UTF-8 byte sequence, which is a run of
RE_CHAR, the same shapeemit_char_bytes()already emits for a multibytecharacter written literally in the pattern (
re_compile.c:681), and for thesame reason: the run has to be one atom, or a following quantifier binds to the
last byte alone.
emit_codepoint()is that function for a codepoint thepattern names rather than spells, so the bytes come from
mrb_re_utf8_encode(), the encoding counterpart of themrb_re_utf8_decode()the gem already has.The
\ubranch sits ahead of thech >= 0xC0branch0e96b2c2dadded, whichroutes a backslash before a multibyte character away from
parse_escape().uis below0xC0, so the order is what keeps\ufrom reachingparse_escape()as a literalu; the same holds for thepeek(c) < 0xC0guard in
read_class_atom().The list form is where the two contexts part.
Outside a character class
/\u{61 62}/is a sequence of atoms rather than one,and a quantifier after it repeats the last codepoint only, so
/\u{61 62}+/isafollowed byb+.compile_quantified()used to take the atom's start fromthe position it saved before calling
compile_atom(); it now takes it fromc->atom_start, which the\ucase moves past every codepoint but the last.The field is saved and restored around the call, so a nested
compile_quantified()inside a group leaves nothing behind for the outer one.Inside a class each codepoint is a member of its own, and
read_class_atom()adds all but the last to the class itself and returns the last, so it can still
open a range:
/[\u{61 62}-z]/isaplusb-z, which is how CRuby reads it.read_class_atom()takes the class for that, andclass_add_member()routes amember to the ASCII bitmap or the codepoint range list.
The malformed spellings raise rather than fall back on a shorter codepoint or
on literal text, with CRuby's messages:
invalid Unicode escapefor a\uXXXXwith fewer than four hex digits,
too short escape sequencefor a bare\uatthe end of the pattern,
invalid Unicode listfor\u{}, for a separatorCRuby does not take and for an unterminated group, and
invalid Unicode rangefor more than six digits, for a surrogate and for anything above U+10FFFF. A
surrogate and an out of range value have no UTF-8 encoding, so rejecting them
is also what keeps
mrb_re_utf8_encode()total.One more pass had to learn about the escape. Whitespace separates the
codepoints of a list, and
preprocess_pattern()strips whitespace for/xbefore the parser runs, so it now copies a
\u{...}group whole; without that/\u{61 62}/xwould join into the single codepoint\u{6162}.The executors are not touched. They see the
RE_CHARrun and the classmembers any other pattern produces.
The change comes in two commits: the first lifts out the three pieces the
escape reuses (
emit_char(),class_add_member(),hex_value()) with nobehaviour change, the second reads the escape.
emit_char()sits beside theemit_char_bytes()that0e96b2c2dextracted and names the same job for acharacter that is a single byte.
Tests
Three cases in
mrbgems/mruby-regexp/test/regexp.rb:Regexp - \u escapes: both spellings against the character and against thetext they used to match, a codepoint in each UTF-8 length, the quantifier
binding for a single codepoint and for a list, the
/xcase that thefree-spacing pass would otherwise join, and the
/ifolding of an ASCIIletter reached through
\u.Regexp - \u escapes in a character class: single members, a codepointrange, a list, a list that opens a range, and a negated class.
Regexp - malformed \u escapes: the eleven spellings CRuby refuses, with themessage asserted for three of them,
\u{110000}among them, since that isthe one that used to raise about a quantifier instead.
Patterns are always parsed as UTF-8, so the subjects are written as explicit
bytes and the assertions hold in both
MRB_UTF8_STRINGand byte-string builds.Verified on x86_64-linux
combinations and 4700 answers: 57 patterns under no flag,
/iand/xagainst 20 subjects; 26 patterns exercising the interaction with groups,
repetition counts, alternation and backreferences against the same subjects;
27 malformed or boundary patterns under no flag and
/xagainst 10 subjects;and 22 patterns crossing the escape with the escaped multibyte literal
0e96b2c2dadded, with the surrogate boundary and the astral plane, against10 subjects. Every answer and every error class and message agrees with
CRuby. The one difference left is that mruby's
RegexpErrormessage does notcarry the flag suffix (
/\uXX/where CRuby writes/\uXX/i), which is howcompile_error()has always formatted a pattern and is unrelated to thischange.
rake test: 2020 total, 2002 OK, 0 KO, 0 crash, and bintest 105 OK.MRUBY_CONFIG=build_config/asan.rb rake test(clang, ASAN and UBSan): 2197total, 2195 OK, 0 KO, 0 crash, no sanitizer report, and bintest 78 OK.
full-corebuild withMRB_UTF8_STRING: 2197 total, 2195 OK, 0 KO,0 crash, and bintest 78 OK. The sweep above runs there.
clang -Wall -Wextraoverre_compile.candre_utf8.c: no new warning.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests