mruby-regexp: add the regexp form of String#[]= and String#slice! - #7063
Conversation
The read side landed as `String#[]` and `String#slice`, but the write side still funnels a Regexp through an Integer conversion and fails with a type error that names Integer: ```ruby s = +"hello" s[/l+/] = "X" # CRuby: "heXo", mruby: TypeError s[/(?<x>l+)/, :x] = "Y" # CRuby: "heYo", mruby: TypeError s.slice!(/l+/) # CRuby: "ll", mruby: TypeError s.slice!(/(l)(o)/, 1) # CRuby: "l", mruby: TypeError ``` `mrb_str_aset()` shares `str_convert_range()` with `mrb_str_aref()`, so it has the same three accepted index types and the same fallback to `mrb_ensure_int_type()`. `mrb_str_slice_bang()` does its own conversion and reaches `mrb_as_int()` for anything that is neither a String nor a Range. It is easy to miss when grepping: it is registered as `MRB_SYM_B(slice)`, so the literal `slice!` appears nowhere in the tree outside its own tests. Override both in `mrblib/string_regexp.rb`, in the shape `[]` uses there: capture the C-defined method under a `__` name, hand every argument list that does not start with a Regexp straight back to it, and read the argument's type with `Regexp ===` rather than `is_a?`. No new C is needed. `MatchData#begin` and `#end` resolve a group name and report character offsets, which is the space the two-integer form of `[]=` works in, so a named group's span is reachable from Ruby and a multibyte subject needs no further conversion. The two halves disagree in three places, and CRuby is followed in each: - An unusable capture argument. `[]=` raises `IndexError` both for an index that reaches no group and for a group that exists but did not take part in the match, where `slice!` answers nil for the first and `""` for the second. The `""` falls out of `rb_str_slice_bang()` building its result from the group's `-1` offset rather than out of a decision, but it is what CRuby answers. A negative index is normalized the same way on both sides, which puts group 0 out of its reach; `MatchData#begin` rejects every negative index with a wording of its own, so neither the normalization nor the range check can be left to it. - A name that resolves to no group is the one case that raises for both, and `MatchData#begin` already raises it with CRuby's message. A name that resolves to a group which did not take part in the match is reported by CRuby with the group's number, which is not reachable from Ruby, so the message repeats the name as it was given. - The frozen check. CRuby searches before it modifies for `[]=`, so a frozen receiver raises only once the match has been published, and a pattern that does not match raises `IndexError` rather than `FrozenError`; letting the mutation be what raises reproduces both. For `slice!` CRuby checks first, as `mrb_str_slice_bang()` already does, so the override asks `frozen?` before it searches and leaves `$~` alone. A Regexp goes through `Regexp#match` rather than `#match?`, because the match globals have to be published here, including the clearing a failed match does before `[]=` raises and before `slice!` returns nil. The MatchData left behind describes the subject as it was before the replacement, since `create_matchdata()` snapshots it. The replacement reaches the core method's type check unconverted, the way the `sub` override refuses to invent an implicit conversion. Its message reads `Symbol cannot be converted to String` where CRuby says `no implicit conversion of Symbol into String`; that wording is the core's throughout and is not this change's to fix. Unlike the read side, this override is not free. `vm_op_getidx()` answers `str[Integer]`, `str[String]` and `str[Range]` from C, which is what keeps `[]` off the common paths, but `vm_op_setidx()` optimizes Array and Hash only and sends `[]=` for everything else, so every `str[...] = repl` pays a Ruby frame on its way to `__aset` once this gem is in the build. The delegation guard is a single `Regexp ===` before any other work.
📝 WalkthroughWalkthrough
ChangesRegexp String mutation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant String
participant Regexp
participant MatchGlobals
Caller->>String: []= or slice! with regexp
String->>Regexp: search subject and resolve capture
Regexp-->>String: match range or failure
String->>MatchGlobals: publish or preserve match state
String-->>Caller: replace, remove, or return result
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 |
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/mrblib/string_regexp.rb`:
- Line 288: Replace the overridable args[0].match(self) dispatch in both
String#[]= at mrbgems/mruby-regexp/mrblib/string_regexp.rb:288-288 and
String#slice! at mrbgems/mruby-regexp/mrblib/string_regexp.rb:333-333 with the
resolved non-overridable regexp helper already used by String#match and
String#[].
🪄 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: a711a366-ec96-4f55-8754-4816bbf841e6
📒 Files selected for processing (3)
mrbgems/mruby-regexp/README.mdmrbgems/mruby-regexp/mrblib/string_regexp.rbmrbgems/mruby-regexp/test/regexp.rb
String#[]andString#slicelearned their regexp form, but the writeside was left out and still funnels a Regexp through an Integer
conversion:
mrb_str_aset()sharesstr_convert_range()withmrb_str_aref(), andmrb_str_slice_bang()reachesmrb_as_int()for anything that isneither a String nor a Range.
String#slice!is registered asMRB_SYM_B(slice), so the literal name appears nowhere in the treeoutside its own tests.
What this adds
Both are overridden in
mrblib/string_regexp.rbin the shape[]usesthere: the C-defined method is captured under a
__name, every argumentlist that does not start with a Regexp is handed straight back to it, and
the argument's type is read with
Regexp ===rather thanis_a?. Nonew C is needed, because
MatchData#beginand#endresolve a groupname and report character offsets, which is the space the two-integer
form of
[]=works in.Behaviour
Checked against CRuby 4.0.6; every row below is covered by a test.
s[/l+/] = "X"sbecomes"heXo", evaluates to"X"s[/z/] = "X"IndexError: regexp not matcheds[/(?<x>l+)/, :x] = "Y"sbecomes"heYo"s[/(l+)(o)/, -1] = "X"sbecomes"hellX"s[/l+/, -1] = "X"IndexError: index -1 out of regexps[/l+/, 5] = "X"IndexError: index 5 out of regexps[/(h)|(z)/, 2] = "X"IndexError: regexp group 2 not matcheds[/(l+)/, "zz"] = "Y"IndexError: undefined group name reference: zzs.slice!(/l+/)"ll",sbecomes"heo"s.slice!(/(l+)(o)/, -1)"o",sbecomes"hell"s.slice!(/l+/, -1)nil,sunchangeds.slice!(/l+/, 5)nil,sunchangeds.slice!(/(h)|(z)/, 2)"",sunchangeds.slice!(/z/)nil,sunchanged,$~clearedThe three places the two halves disagree are followed as CRuby has them.
An unusable capture argument raises for
[]=and answers nil or""forslice!; the""is whatrb_str_slice_bang()produces from anon-participating group's
-1offset. A negative index is normalizedthe same way on both sides, which puts group 0 out of its reach. And the
frozen check lands on opposite sides of the search, which
$~makesobservable:
[]=gets its ordering for free by letting the mutation be what raises,which also makes a pattern that does not match raise
IndexErroron afrozen receiver rather than
FrozenError.slice!asksfrozen?before it searches, matching both CRuby and
mrb_str_slice_bang(), whoseown check is its first statement.
One divergence: for a name that resolves to a group which did not take
part in the match, CRuby reports the group's number and the override
repeats the name, because
matchdata_name_to_group()is not reachablefrom Ruby. The exception class and the position are the same.
Notes
Unlike the read side, this override is not free.
vm_op_getidx()answers
str[Integer],str[String]andstr[Range]from C, butvm_op_setidx()optimizes Array and Hash only and sends[]=foreverything else, so every
str[...] = replpays a Ruby frame on its wayto
__asetonce this gem is in the build. The delegation guard is asingle
Regexp ===before any other work.String#sub!andString#gsub!are the other half of the destructivework and are not touched here.
Testing
rake testpasses, and so does aMRB_UTF8_STRINGbuild, which themultibyte cases are there for.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation