mruby-regexp: apply /i to character classes - #7049
Conversation
`RE_FLAG_IGNORECASE` was folded in only where `compile_atom()` emits a single
literal character. `compile_charclass()` never read the flag, so every `[...]`
ignored `/i`.
```ruby
/[abc]/i.match?("A") # CRuby: true, mruby: false
/[a-c]/i.match?("A") # CRuby: true, mruby: false
/[a-c]+/i.match?("AB") # CRuby: true, mruby: false
/(?i)[a-c]/.match?("A") # CRuby: true, mruby: false
/[[:lower:]]/i.match?("A") # CRuby: true, mruby: false
# A negated class is worse: it matches what it must reject.
/[^a-c]/i.match?("A") # CRuby: false, mruby: true
```
The negated form is a false positive, so a `/i` regexp used for validation
silently accepts input it should reject.
Fold the ASCII letters into the class bitmap at the end of
`compile_charclass()`, once the parse loop has merged POSIX brackets,
shorthands, ranges and single literals into it. One pass there covers all four
forms. Negation is applied at match time against that same bitmap via
`RE_NCLASS`, so folding the positive set fixes the negated form as well.
`class_get_bit()` is added next to `class_set_bit()` because `re_compile.c` had
no bit-level reader for `re_charclass::bitmap`; the equivalent test in
`class_match()` is `static` in another translation unit and takes a codepoint
rather than a bit index.
Folding at compile time also keeps `compute_first_set()` in step: it derives
`pat->first_bytes` from the same bitmap to skip input, so a match time fold
would leave that skip narrower than the class and `/[a-c]/i` would still fail
on `"A"`. Reading `c->flags` rather than `pat->flags` is what makes the
inline-scoped `(?i)` and `(?i:...)` forms work.
Non-ASCII case folding stays out of scope, so the codepoint range list is
untouched.
📝 WalkthroughWalkthroughThe regexp compiler now folds ASCII letter pairs in case-insensitive character classes. The change covers literals, ranges, shorthands, POSIX classes, and negated classes. Tests cover ASCII boundaries and inline option scope. ChangesASCII character-class case folding
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@mrbgems/mruby-regexp/src/re_compile.c`:
- Around line 476-488: Extend the IGNORECASE handling around the class-building
logic to case-fold Unicode codepoints in the inclusive range list, not just
ASCII bitmap entries. Merge the folded Unicode set into the class representation
so ranges such as [a-\u0100] include both case variants, including under
RE_NCLASS negation. Add positive and negated mixed-range tests covering these
cases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a4f2271-2878-43ee-b820-4551e25eade3
📒 Files selected for processing (2)
mrbgems/mruby-regexp/src/re_compile.cmrbgems/mruby-regexp/test/regexp.rb
RE_FLAG_IGNORECASEis folded in only wherecompile_atom()emits a singleliteral character.
compile_charclass()never reads the flag, so every[...]ignores
/i.The inline and constructor forms of the flag reach the same code, so they fail
the same way.
POSIX bracket classes are affected too, since they are merged into the same
bitmap.
A negated class is worse: it matches what it must reject, so a
/iregexpused for validation accepts input it should refuse. Nothing raises.
Fix
Fold the ASCII letters into the class bitmap at the end of
compile_charclass(), after the parse loop and before the class is committedto an
RE_CLASSorRE_NCLASS. That is the only point at which a[...]iscomplete: the loop merges POSIX bracket bits, shorthands, ASCII ranges and
single literals into one bitmap, so a single pass there covers all four forms
and
[[:lower:]]under/ipicks up uppercase.Negation is applied at match time against that same bitmap, so folding the
positive set fixes
[^a-c]at the same time.class_add_shorthand()andposix_class_bits()both run before the class is complete and would missplain literals and
a-cranges, so neither is the right place.class_get_bit()is added alongsideclass_set_bit().re_compile.chad nobit-level reader for
re_charclass::bitmap:first_set_walk()ORs wholebytes, and the one bit test in the gem lives in
class_match(), which isstaticin another translation unit and takes a codepoint rather than a bitindex.
The fold has to happen at compile time rather than in the matcher for three
reasons.
class_match()receives only are_charclassand has no access toany flags.
pat->flagsis the whole-pattern option set and so cannot seeinline
(?i:...)scoping. Andcompute_first_set()derivespat->first_bytesfrom the bitmap at compile time, which the matcher then uses to skip input; a
match time fold would leave that skip narrower than the class, and
/[a-c]/iwould still fail on
"A".Reading
c->flagsis what makes the inline forms work:compile_atom()already saves, replaces and restores it around
(?i)and(?i:...), so theclass sees the scoped value with no further change.
The existing
IGNORECASEblocks incompile_atom()are already correct for asingle literal and are left alone.
Scope
Non-ASCII case folding is not addressed. The codepoint range list and
cc->utf8_anyare untouched, andclass_add_range()andclass_add_codepoint()are unchanged.Tests
mrbgems/mruby-regexp/test/regexp.rb:Regexp - case insensitive character class, a new block next toRegexp - case insensitive: the four positive forms, theRegexp::IGNORECASEconstructor form, and both negated forms. It alsoguards the fold against widening the class past the ASCII letters, since
[and{are 32 apart but are not a case pair.Regexp - POSIX bracket classes:[[:upper:]]and[[:lower:]]under/i, next to the existing[[:upper:]]assertion.Regexp - inline options (?i) / (?i:...):(?i)[a-c],(?i:[a-c]), andthat the option does not leak past the closing paren.
Verified on
x86_64-linux:rake test: 1968 total, 1950 OK, 0 KO, 0 crash, and bintest 105 OK.MRB_INT32build with clang and-Wall -Wextra: 1870 total, 1852 OK,0 KO, 0 crash, and no new warning from any
mruby-regexpfile.Summary by CodeRabbit
Bug Fixes
Tests