Skip to content

mruby-regexp: pass the block through String#match - #6991

Merged
matz merged 3 commits into
mruby:masterfrom
takumin:string-match-block
Aug 2, 2026
Merged

mruby-regexp: pass the block through String#match#6991
matz merged 3 commits into
mruby:masterfrom
takumin:string-match-block

Conversation

@takumin

@takumin takumin commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Regexp#match yields the MatchData and returns the block result when a block
is given, but String#match dropped the block: it always returned the
MatchData, so the block was never called.

"hello".match("l+") { |md| md[0].upcase }  # => #<MatchData ...> instead of "LL"

This forwards the block to Regexp#match instead of yielding in Ruby, which
keeps the "yield only on a successful match" rule in a single place.

Tests cover the successful and failing match, break out of the block (the
block now travels from a Ruby method through &block into a C mrb_yield),
and that $~ / Regexp.last_match are already set while the block runs.
The no-match tests flip a flag inside the block rather than only asserting
nil, since nil is also what you get when the block does run and returns
nil.

The README API listing is updated as well: it showed only the
MatchData-returning call, so the block form was undiscoverable from the docs
even though both Regexp#match and String#match are meant to support it.

rake test passes.

Summary by CodeRabbit

  • New Features

    • String#match now accepts blocks and provides match results within them.
    • Block return values and control flow are supported.
    • String patterns continue to be treated as regular expressions.
  • Bug Fixes

    • Improved handling when matches fail or the starting position is out of range.
  • Documentation

    • Documented block usage for Regexp#match and String#match.

takumin added 2 commits August 2, 2026 21:39
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.
@takumin
takumin requested a review from matz as a code owner August 2, 2026 12:58
@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 376ea10b-2e09-40c2-a166-14b049f178e3

📥 Commits

Reviewing files that changed from the base of the PR and between da98488 and a6a27c5.

📒 Files selected for processing (1)
  • mrbgems/mruby-regexp/test/regexp.rb
🚧 Files skipped from review as they are similar to previous changes (1)
  • mrbgems/mruby-regexp/test/regexp.rb

📝 Walkthrough

Walkthrough

String#match now accepts an optional block and forwards it to Regexp#match. Tests cover block results, failed matches, break, and match globals. README documents both block forms.

Changes

Regexp match blocks

Layer / File(s) Summary
String#match block forwarding
mrbgems/mruby-regexp/mrblib/string_regexp.rb
String#match accepts a block and forwards it to Regexp#match. String pattern conversion remains unchanged.
Block behavior validation and documentation
mrbgems/mruby-regexp/test/regexp.rb, mrbgems/mruby-regexp/README.md
Tests cover block results, failed matches, break, and $~ and Regexp.last_match. README documents block return behavior.

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
Loading

Possibly related PRs

  • mruby/mruby#6992: Both PRs test block behavior for Regexp#match and String#match, including break value propagation.

Suggested reviewers: matz

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: forwarding blocks through String#match.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 11ff450 and da98488.

📒 Files selected for processing (3)
  • mrbgems/mruby-regexp/README.md
  • mrbgems/mruby-regexp/mrblib/string_regexp.rb
  • mrbgems/mruby-regexp/test/regexp.rb

Comment thread mrbgems/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.
@matz
matz merged commit 3cf26f4 into mruby:master Aug 2, 2026
21 checks passed
@takumin
takumin deleted the string-match-block branch August 2, 2026 13:55
matz pushed a commit that referenced this pull request Aug 2, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants