mruby-regexp: number atomic groups so a possessive repeat cuts as its own - #7276
Merged
Conversation
… own A possessive repeat wraps an atomic group around the code it repeats, after that code is emitted, and it took the group's depth from the count of groups open around it, the same count the groups inside that code took theirs from. So the wrapper and a group inside it shared a depth, and a failure after the wrapper, meant to fail the wrapper without opening the repeat to being skipped, was read by the inner group as its own cut: the inner group failed, the repeat was skipped, and the text after the wrapper was tried again where CRuby has already failed: ```ruby /(?>a)?+a/ =~ "a" # CRuby: nil, mruby: 0 /(?>a)*+a/ =~ "aa" # CRuby: nil, mruby: 0 /(?:(?>a)b?)?+a/ =~ "a" # CRuby: nil, mruby: 0 /(?>(?:(?>a))?)a/ =~ "a" # nil in both: the group form nests its depths ``` The two instructions of an atomic group carry the count of groups numbered before it now, one more for each group the pattern opens and for each possessive repeat, instead of the nesting depth. A cut is still keyed by that number, and it is unique in the pattern, so a wrapper and any group inside it never share one; every numbered group emits two instructions, so the number fits the field as the depth did.
|
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 (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe regexp compiler now assigns unique numbers to atomic groups and possessive repeats. The bytecode documentation and executor terminology use cut identifiers. Regression tests cover possessive-repeat backtracking and nested atomic-group failures. ChangesRegexp atomic cut numbering
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This focused regexp change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant RegexpCompiler
participant RegexpBytecode
participant RegexpExecutor
participant SyntaxTests
RegexpCompiler->>RegexpBytecode: Emit atomic boundaries with unique cut numbers
RegexpCompiler->>RegexpBytecode: Emit possessive-repeat boundaries with unique cut numbers
RegexpBytecode->>RegexpExecutor: Provide numbered atomic boundaries
SyntaxTests->>RegexpExecutor: Validate possessive atomicity and nested failures
🚥 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.
A possessive repeat (
a*+,a?+,a++) wraps an atomic group around thecode it repeats after that code is emitted, and it takes the group's depth
from the count of groups open around it, which is the count the groups inside
that code took theirs from. So the wrapper and a group inside it share a
depth, and a failure after the wrapper, which is meant to fail the wrapper
without opening the repeat to being skipped, is read by the inner group as
its own cut: the inner group fails, the
?or*skips the repeat, and thetext after the wrapper is tried once more where CRuby has already failed:
The written-out group nests because its depth is taken before its body is
compiled; the wrapper is the one construct that is put around code already
emitted, so a depth is not what tells it apart from what it holds.
The fix
The two instructions of an atomic group carry the count of groups numbered
before it, one more for each
(?>...)the pattern opens and for eachpossessive repeat, instead of the nesting depth. A cut is still keyed by that
number (
BT_CUT), and it is unique in the pattern, so a wrapper and any groupinside it never share one. Every numbered group emits two instructions, so the
number fits the field as the depth did.
emit_atom_copy()copies a group'snumber with the group, and the copies are sequential, never one inside the
other, so they cut independently as before.
Size
.textofbin/mruby,build_config/ci/gcc-clang.rb, each side from a cleanbuild directory.
re_compile.ois the only object that changes;re_exec.o, whosechange is a comment and the name of a macro parameter, keeps its size.
bintestascii-ctypebyte-stringcxx_abifull-debug(-O0)Verification
The tests go in
regexp_syntax.rb, in the atomic group block: the examplesabove, a possessive repeat around a possessive repeat, a repeat that matches
with the text after it, and a failure inside the repeat before its end, which
still fails only the inner group and lets the
?skip the repeat(
/(?:(?>a)b)?+c/ =~ "ac"is 1 in both). Four of the assertions fail onmaster.
Differential against CRuby 4.0.6, the harness of #7269 with a possessive
form added to the quantifiers it draws, master and this PR against the same
cases, compared as
MatchData#to_a. 10,000 random patterns over the defaultfeatures (seed 2), 3,081 of them with a possessive quantifier: 8,406 compared
(master and this PR both refuse 1,592, an empty group under a quantifier,
which is #7275), 8,294 the same everywhere, 111 differing from CRuby on both
sides, 1 that master answers differently and this PR as CRuby does
(
/(?:(?<!aa)(?:b|)??(?>a{2}?))?+\B/on"baba"), 0 that only this PRanswers differently. A second 10,000 (seed 3) drawn from atomic groups,
possessive and lazy quantifiers, intervals, empty-matching atoms, classes and
backreferences: 8,247 compared, 5 that master answers differently and this PR
as CRuby does, every one a possessive repeat around an atomic group, 11 that
both differ on (none with a possessive repeat around a group), 0 only this PR.
rake test,build_config/ci/gcc-clang.rb, no compiler warning:full-debugbintestbintest(bintest suite)cxx_abibyte-stringascii-ctypeThe default configuration: 2,140 total, 0 KO, 0 crash, plus its 112 bintests.
Environment
Details
Compile lines for
mrbgems/mruby-regexp/src/re_compile.cin the buildsquoted above, paths shortened:
Summary by CodeRabbit
Bug Fixes
Tests