mruby-regexp: pass the block through String#match - #6991
Conversation
Regexp#match yields the MatchData when a block is given, but String#match dropped the block and returned the MatchData instead, so the block result was silently discarded. Forward the block to Regexp#match rather than yielding in Ruby, which keeps the "yield only on a successful match" rule in one place. The no-match tests flip a flag inside the block rather than only checking that nil came back, since nil also is what you get when the block runs and happens to return nil. A separate test pins down that $~ and Regexp.last_match are already set while the block runs, since that ordering is what makes the delegation useful. Because the block now travels from a Ruby method through &block into a C mrb_yield, a test covers break as well: it is the part of the delegation most likely to regress without anyone noticing.
The API listing showed only the MatchData-returning call, so the block form was undiscoverable from the README even though both Regexp#match and String#match support it.
|
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)
📝 WalkthroughWalkthrough
ChangesRegexp match blocks
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant StringMatch
participant RegexpMatch
Caller->>StringMatch: call match with pattern and block
StringMatch->>RegexpMatch: forward pattern, position, and block
RegexpMatch-->>Caller: return block result or nil
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/test/regexp.rb`:
- Around line 524-535: Extend the “String#match - block” test after the existing
failed-match cases with a successful match whose block returns nil. Verify the
block executes and assert that String#match returns nil despite the successful
match, using the existing called flag pattern.
🪄 Autofix (Beta)
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: 5f03d263-889b-4a86-a61b-d0a3fc43ddc4
📒 Files selected for processing (3)
mrbgems/mruby-regexp/README.mdmrbgems/mruby-regexp/mrblib/string_regexp.rbmrbgems/mruby-regexp/test/regexp.rb
The existing cases only produce nil when the match fails, so they cannot tell a skipped block apart from a block that ran and returned nil.
CRuby defines `match`, `match?` and `=~` on Symbol so that a symbol can be matched against a regexp without spelling out the `to_s`; mruby-regexp provided none of them, so any regexp use on a symbol raised NoMethodError. ```ruby :abc.match?(/b/) # CRuby: true, mruby: NoMethodError :abc =~ /b/ # CRuby: 1, mruby: NoMethodError ``` CRuby implements all three as the String method applied to the symbol's name (`rb_sym2str` then `rb_str_match_m` / `rb_str_match`), so they delegate to `to_s` rather than repeat the pattern handling. That inherits the String pattern compilation, the `pos` argument, the block form, and the `TypeError` for a String argument to `=~`. `$~` and `$1`-`$9` are set by the engine itself, so delegating does not lose them. `Symbol#match` needs the block to survive the delegation, which #6991 made `String#match` do. Delegating also inherits one difference from CRuby that is worth naming: for an argument that is neither a Regexp nor a String, `String#=~` dispatches `re =~ self`, so `:a =~ nil` raises NoMethodError where CRuby returns nil. Fixing that belongs to `String#=~`, not to the Symbol wrapper. This covers the symbol-on-the-left direction only. The Regexp side still takes strings only, and rejects symbols in every entry point rather than just in `#===`: ```ruby /l/ =~ :hello # TypeError (CRuby: 2) /l/.match(:hello) # TypeError (CRuby: MatchData) /l/.match?(:hello) # TypeError (CRuby: true) /l/ === :hello # false (CRuby: true) [:to_s, :abc].grep(/^to_/) # [] -- Enumerable#grep goes through Regexp#=== ``` `sym[/re/]` is a third gap: `mruby-symbol-ext` already delegates `Symbol#[]` to `String#slice`, but this gem does not implement the regexp form of `String#[]` / `#slice`, so `"hello"[/l+/]` does not work either. Both are fixes on the String and Regexp side, so they are left alone here; the README Limitations section and the comment in symbol_regexp.rb now spell them out.
Regexp#matchyields theMatchDataand returns the block result when a blockis given, but
String#matchdropped the block: it always returned theMatchData, so the block was never called.This forwards the block to
Regexp#matchinstead of yielding in Ruby, whichkeeps the "yield only on a successful match" rule in a single place.
Tests cover the successful and failing match,
breakout of the block (theblock now travels from a Ruby method through
&blockinto a Cmrb_yield),and that
$~/Regexp.last_matchare already set while the block runs.The no-match tests flip a flag inside the block rather than only asserting
nil, sincenilis also what you get when the block does run and returnsnil.The README API listing is updated as well: it showed only the
MatchData-returning call, so the block form was undiscoverable from the docseven though both
Regexp#matchandString#matchare meant to support it.rake testpasses.Summary by CodeRabbit
New Features
String#matchnow accepts blocks and provides match results within them.Bug Fixes
Documentation
Regexp#matchandString#match.