mruby-regexp: close the pattern dispatch in the String overrides - #7079
Merged
Conversation
The String overrides in string_regexp.rb reached their search through `pattern.match(self)`, an ordinary method call on the pattern, so a method rewritten on a Regexp instance replaced the search itself: ```ruby r = /l+/ def r.match(*args); "PWNED"; end "hello"[r] # CRuby: "ll", mruby: "P" ``` CRuby dispatches `match` to the pattern only in `rb_str_match_m()`, on purpose; everywhere else (`rb_str_sub_bang()`, `rb_str_subpat()`, `rb_str_index_m()` and the rest) it reaches the engine through C internals that never consult the pattern's method table. This commit gives the gem the same boundary: `Regexp.__search` is `Regexp#match` with the pattern as an argument and no block form, a thin wrapper over the same `exec_match()`. A class method leaves no instance for a singleton method to ride on, so the mrblib callers keep their loops and blocks in Ruby while the search itself stops being redefinable. The call sites moved over are the block form of `sub`, the guards of `sub!` and `gsub!`, `[]`, `slice`, `[]=`, `slice!`, `index`, `rindex`, the miss paths of `byteindex` and `byterindex`, `partition`, `rpartition` (through `__regexp_rsearch`) and `start_with?`. `String#match` keeps dispatching, as CRuby does. The `match?`, `=~`, `__byte_match`, `__sub_str`, `__gsub_str` and `__scan` call sites are the same shape and move in the following commits. The MatchData these call sites read (`pre_match`, `begin`, `[]` and the rest) now always comes from `create_matchdata()`, so no argument can have planted a singleton method on it either. What remains reachable is a class-wide redefinition of a MatchData method, which is the same category as redefining `String#sub` itself and is not steered by the argument.
The byte-space loops in `gsub` (block form), `split` and `byteindex`
drove their search through `pattern.__byte_match(self, pos)`, an
ordinary method call on the pattern, so a method rewritten on a Regexp
instance replaced the search under the loop:
```ruby
r = /l+/
def r.__byte_match(*args); "PWNED"; end
"hello".gsub(r) { |m| m.upcase }
# CRuby: "heLLo", mruby: NoMethodError for `__byte_begin` on String
```
`Regexp.__byte_search` is the same thin wrapper over `exec_match()`
with the pattern as an argument, so the search stops being redefinable
while both loops stay in Ruby, where the block call and the field list
belong. As with `Regexp.__search`, there is no position normalization,
because the callers already work in byte space, and no operand
conversion, because they always pass a String.
The instance form had no caller left outside these three sites, so it
is removed rather than kept alongside: `__byte_match` was an internal
of this gem, and its replacement takes over its method table slot
rather than adding one.
`__sub_str`, `__gsub_str` and `__scan` are the whole-operation entry points of `sub`, `gsub` and `scan` with a replacement string: each runs an entire operation in C and hands back its result. They took the pattern from `self`, so the mrblib callers reached them through an ordinary method call on the pattern, and a method rewritten on a Regexp instance replaced the whole operation. This was the quietest part of the surface, because the caller got the redefinition's return value verbatim and nothing raised on the way: ```ruby r = /l+/ def r.__sub_str(*args); "PWNED"; end "hello".sub(r, "X") # CRuby: "heXo", mruby: "PWNED" "hello".dup.sub!(r, "X") # worse: the receiver became "PWNED" too ``` CRuby closes all of these: its `sub`, `gsub` and `scan` reach the pattern through `rb_pat_search()`, which searches a real Regexp without asking it anything. Each core becomes a class method taking the pattern as its first argument, the boundary `Regexp.__search` already draws for the plain searches. The instance forms had no caller left outside the gem, so the class forms take over their method table slots rather than adding new ones. The bodies themselves are unchanged: they already ran without calling back into Ruby, which is why they are in C at all. `sub!` and `gsub!` reach the same two cores by calling `sub` and `gsub` with a pattern they already resolved, so they are closed by the same change, receiver included.
These two are the overrides whose CRuby counterparts do not dispatch to the pattern at all: `rb_str_match_m_p()` resolves the argument and searches it, and `rb_str_match()` sends `=~` to the argument only when it is neither a String nor a Regexp. The overrides here asked the pattern on every path, so for a real Regexp they answered whatever a rewritten method answered, and nothing raised on the way: ```ruby r = /l+/ def r.match?(*args); "PWNED"; end def r.=~(*args); 99; end "hello".match?(r) # CRuby: true, mruby: "PWNED" "hello" =~ r # CRuby: 2, mruby: 99 ``` `String#match?` now hands the pattern to `Regexp.__search_p`, which is `Regexp#match?` with the pattern as an argument: the same search with a NULL capture buffer, so it allocates no MatchData and leaves the match globals alone. `String#=~` searches a real Regexp through the existing `Regexp.__search` and reads `begin(0)` from the MatchData, so no second entry point is needed; the globals it publishes and the character offset it answers are the ones `Regexp#=~` produced before. The forward for everything that is not a Regexp stays, because CRuby forwards there too: `"hello" =~ obj` still reaches the argument's own `=~`. Only the branch in front of it changed, from every argument to the non-Regexp ones.
The comment at the top of string_regexp.rb described a file where every override reached its pattern through redefinable methods, and argued that the gem accepts that surface rather than closing it. The previous commits closed it, so the description no longer holds; what is left to say is smaller. The type-check paragraph survives unchanged, because nothing about the argument checks moved. The surface paragraphs give way to what the file does now: the searches go through class methods that take the pattern as an argument, the MatchData comes from C, and the reachable remainder is a class-wide redefinition of a MatchData method, which is the same category as redefining `String#sub` itself. The CRuby comparison stays, inverted: `String#match` dispatching `match` on purpose (`rb_str_match_m()`) and `=~` forwarding a non-Regexp argument (`rb_str_match()`) are now the two deliberate exceptions rather than the two ends of an open range.
|
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 (3)
📝 WalkthroughWalkthroughRegexp-aware ChangesRegexp dispatch refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
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 |
This was referenced Aug 10, 2026
Closed
Closed
Search a real pattern in
String#match? and String#=~ instead of asking the argument
takumin/mruby#54
Closed
This was referenced Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows #7067, which documented that the String overrides in
mrbgems/mruby-regexp/mrblib/string_regexp.rbreach their pattern throughordinary Ruby-visible methods, and accepted that surface in the file comment.
This PR proposes the other side of that question: close the surface, because
working out the fix showed it is smaller than the comment assumed, and the
boundary it draws is the one CRuby already has.
What was open
Every override reached the engine by calling methods on the pattern
(
match,match?,=~,__byte_match,__sub_str,__gsub_str,__scan), so a method rewritten on a Regexp instance replaced the searchitself, and every String method in this file followed the replacement:
The quietest paths handed back the redefinition's own value with nothing
raising, so the caller had no way to tell:
String#match?andString#=~diverged even one level earlier: their CRubycounterparts
rb_str_match_m_p()andrb_str_match()search a real Regexpwithout asking it anything, so for these two the dispatch itself was the
divergence, before any redefinition enters.
The shape of the fix
The overrides now reach the engine through class methods that take the
pattern as an argument:
Regexp.__search,Regexp.__byte_searchandRegexp.__search_pare thin wrappers over the existingexec_match(), and__sub_str,__gsub_strand__scanbecome class forms of the operationcores they already were. A class method leaves no instance for a singleton
method to ride on, so the search stops being redefinable while the loops,
blocks and field lists stay in mrblib, which keeps the C side free of
callbacks into the VM.
This is architecture parity with CRuby rather than case-by-case behaviour
parity: CRuby's
rb_str_sub_bang(),rb_str_subpat(),rb_str_index_m()and the rest reach the engine through C internals that never consult the
pattern's method table, and now this gem does the same. Two consequences
fall out rather than being patched individually. A rewritten pattern method
no longer steers any String method, and the MatchData the overrides read
always comes from
create_matchdata(), so no argument can have planted asingleton method on it either; what remains reachable is a class-wide
redefinition of a MatchData method, the same category as redefining
String#subitself.Two dispatches stay, both deliberate and both CRuby's own shape:
String#matchsendsmatchto the pattern becauserb_str_match_m()doesso on purpose, and
String#=~still forwards an argument that is not aRegexp to the argument's own
=~, asrb_str_match()does. The filecomment from #7067 is rewritten in the last commit to describe exactly these
two exceptions instead of the open surface.
Cost
Four internal instance methods are removed (
__byte_match,__sub_str,__gsub_str,__scan) and six class methods added, so the method tablegrows by two entries net. The operation core bodies are unchanged apart from
taking the pattern as an argument; the new code is the three thin search
wrappers and a shared type-check backstop.
Commits
Each commit closes one dispatch and carries a test that rewrites the method
it closes on a pattern instance and asserts the CRuby answer, receivers of
the bang forms included. Every CRuby answer above was checked against
CRuby 4.0.6.
Summary by CodeRabbit