mruby-regexp: turn \1 through \9 off in a replacement that names a group - #7283
Merged
Merged
Conversation
`__sub_str` and `__gsub_str` keep their capture offsets in an `mrb_malloc` buffer that is freed where the call ends, so a path out of the walk that does not reach that free leaves it behind. The block core beside them already holds the same offsets in `int captures[RE_MAX_CAPTURES * 2]`, and so does the `last_captures` copy inside `__gsub_str` itself: compiling a pattern with more groups than `RE_MAX_CAPTURES` fails, so the buffer is 256 bytes at most whatever the pattern. Standing it there leaves nothing owned across `apply_replacement()`, which is where the next commit raises for a replacement naming a group the pattern does not have.
`\0` to `\9`, `\&` and `\+` each settle on a group and then copy its bytes out, three times over with the same three lines between them. Saying which group the escape stands for and appending that one group are two things, and splitting them leaves each escape with only the first: `\&` is `\0` by another spelling, so it says group 0 rather than reading the whole match's offsets its own way, and `\+` says the last group that took part rather than appending from inside its search. A group the escape reaches past, and one that took no part in the match, then stand for nothing in one place instead of in three.
A replacement string reads `\1` through `\9` for a group the pattern numbered, but nothing read `\k<name>` for one the pattern named: the reference was copied out as the text it was spelled with, a wrong answer that nothing reported. ```ruby "ab".sub(/(?<x>b)/, '\k<x>!') # CRuby: "ab!", mruby: "a\\k<x>!" ``` The name is the bytes between the angles, with no escape among them: CRuby ends the name at the first `>` whatever stands before it, and only this spelling opens a reference, so `\k'name'`, which the pattern side does read as a backreference, stays the literal it was here. What the name is asked of is the pattern the match was made with, not the offsets it left: a name no group carries is a mistake in the replacement, and raises `IndexError` even where a group of that name would have taken no part in the match and stood for nothing. A pattern with no named group at all is asked the same question, and so is a literal String pattern, which has no group to name. An unclosed `\k<` is a mistake of a different kind and raises `RuntimeError`, as CRuby has it. Nothing is asked when nothing matched, since the replacement is expanded once per match. The lookup itself is the one `MatchData#[]` makes for a String or Symbol name, extracted so both sides read a name the same way.
…a group CRuby stops a digit escape in a replacement string from reaching a group as soon as the pattern declares a named one, which is the same `ONIG_OPTION_DONT_CAPTURE_GROUP` rule that stops a plain `(...)` from taking a number: `rb_reg_regsub()` reads the escape as a group only where `onig_noname_group_capture_is_active()` holds, and drops the reference where it does not, so the escape stands for nothing. mruby-regexp read every digit escape as whatever group the number reached, so a replacement that named a group by number answered with the group's text. ```ruby "ab".sub(/(?<x>b)/, '[\1]') # CRuby: "a[]", mruby: "a[b]" "abab".gsub(/(?<x>b)/, '[\1]') # CRuby: "a[]a[]", mruby: "a[b]a[b]" "ab".sub(/(?<a>a)(?<b>b)/, '[\1\2]') # CRuby: "[]", mruby: "[ab]" ``` `apply_replacement()` is handed the pattern the match was made with, so the question is one the pattern answers: a named group registers its name, and `pat->num_named > 0` is what the compiler asked `has_named_group()` for when it demoted the plain groups. A literal String pattern hands in no pattern and has no group for a number to reach either way. `\0` still stands for the whole match, and so do `\&` and `\+`, which CRuby keeps outside the guard. The number a named group answers to is untouched where it is read, `md[1]` and `$1` alike, and `\k<name>` is what reaches it from a replacement.
|
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 (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe regexp implementation adds named replacement backreferences, shared capture-name lookup, validation errors, and named-group numeric-reference rules. Compiled substitutions pass patterns to replacement expansion, while literal substitutions retain literal behavior. Regression tests cover these cases. ChangesNamed replacement backreferences
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR changes numbered replacement behavior for named regexp groups and reports matching CRuby behavior with all listed tests passing; no actionable merge-blocking risk remains. 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 |
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
2 participants
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.
Stacked on #7282, which is what hands
apply_replacement()the pattern. Thelast commit is the whole of this change.
A replacement string reads
\1through\9as whatever group the numberreaches, a group the pattern gave a name to among them. CRuby turns those
escapes off as soon as the pattern declares a named group, so mruby answers
with the group's text where CRuby answers with nothing:
It is the
ONIG_OPTION_DONT_CAPTURE_GROUPrule that stops a plain(...)fromtaking a number in a pattern that names a group, reaching the replacement:
rb_reg_regsub()reads a digit escape as a group only whereonig_noname_group_capture_is_active()holds, and drops the reference where itdoes not, so the escape stands for nothing.
\0,\&and\+sit in aseparate case of the same switch and are not guarded, so the whole match and
the last participating group keep working there.
The replacement string is the one place left that differs. Every other numbered
accessor already agrees, ea0b2f9 having demoted the plain groups:
The
\2row is what that demotion fixed: a plain group in a named pattern hasno number for
\2to reach. What remains is the named group's own number,which the same rule governs and which no demotion can take away, the group
being still numbered for
md[1]and$1.The fix
apply_replacement()is handed the pattern the match was made with, so thequestion is one the pattern answers: a named group registers its name, and
pat->num_named > 0is what the compiler askedhas_named_group()for when itset
dont_capture. A digit above0stands for nothing there.\0is thewhole match, which naming a group does not touch, and neither do
\&and\+,which CRuby keeps outside the guard. A literal String pattern hands in no
pattern and has no group for a number to reach either way.
Size
.textofbin/mruby,build_config/ci/gcc-clang.rb, each side from a cleanbuild directory at the same path; the
#7282column is the base this stackson.
regexp.ois the only object that changes.bintestascii-ctypebyte-stringcxx_abifull-debug(-O0)The condition weighs 48 bytes, which
full-debugpays once and the optimisedbuilds twice:
apply_replacement()is inlined into a second,constant-propagated copy for the two literal cores, and both copies carry it.
Verification
The tests go in
string_regexp.rb, after the\k<name>blocks the base adds:the four cases above, in
sub,sub!andgsub, over one named group andtwo; the number read where it still reaches,
md[1]and\k<name>; a plaingroup beside a named one, which has no number of its own left either; the
escapes the rule leaves alone,
\0,\&,\+,\`and\'; a patternthat names nothing, which keeps every number it hands out; and a literal String
pattern, which has no group for a number to reach. 5 of the 17 assertions fail
on #7282, 6 on master.
Differential against CRuby 4.0.6, master, #7282 and this branch against the
same cases. 16 patterns (one and two plain groups, one and two named ones, a
named group beside a plain one and beside a non-capturing one, a named group
nested either way, optional and repeated named groups, alternation, a pattern
with no group at all, a backreference) crossed with 11 replacements (
\0through
\9,\&,\+,\`and\', two digits at once, an escapedbackslash,
\k<name>), 4 subjects andsubandgsub: 1,408 cases. Thisbranch answers all 1,408 as CRuby does; #7282 differs on 112 of them, master on
202, the other 90 being the
\k<name>the base adds.rake test,build_config/ci/gcc-clang.rb, no compiler warning:full-debugbintestbintest(bintest suite)cxx_abibyte-stringascii-ctypeThe default configuration: 2,158 total, 0 KO, 0 crash.
Environment
Details
Compile lines for
mrbgems/mruby-regexp/src/regexp.cin the builds quotedabove, paths shortened:
Summary by CodeRabbit
New Features
\k<name>.Bug Fixes