Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make backtracking modifiers on alternations work correctly #368

Merged
merged 2 commits into from Sep 20, 2017

Conversation

smls
Copy link
Contributor

@smls smls commented Aug 27, 2017

Summary

Current Rakudo behavior:

Backtrackable atom: ? * + ** | ||
Respects default (eager backtracking) mode: yes yes yes yes yes yes
Respects ratchet mode: yes yes yes yes yes no
Respects atom-specific backtracking modifier: yes yes yes yes yes no
Atom-specific backmod overrides ratchet mode: yes yes yes yes no n/a

With this pull request, the behavior becomes "yes" for all 24 cases.

Implementation

Please review carefully, because these are my first commits to nqp and my understanding of this code base is limited... 馃槂
I basically just copied the existing no-backtrack handling code from method alt to method altseq in both the MoarVM and JVM backend (263257a), and fixed a thinko in the quantified_atom action method where the "backtrack" flag for alternation atoms is set in the first place (ba165c8).

RT & SYN

This fixes:

It also matches the behavior intended for these backtracking modifiers by S05, as I understand it.

Roast

This PR passed make spectest on my machine (Linux/MoarVM) .

One it's merged, I'd suggest adding the following additional tests to roast:

use Test;
plan 24;

is "ab" ~~ / a .? b /,            "ab", "backtrack into ?";
is "ab" ~~ / a .?: b /,           Nil,  "don't backtrack into ?:";
is "ab" ~~ / :r a .? b /,         Nil,  "don't backtrack into ? after :r";
is "ab" ~~ / :r a .?! b /,        "ab", "backtrack into ?! (overrides :r)";

is "ab" ~~ / a .* b /,            "ab", "backtrack into *";
is "ab" ~~ / a .*: b /,           Nil,  "don't backtrack into *:";
is "ab" ~~ / :r a .* b /,         Nil,  "don't backtrack into * after :r";
is "ab" ~~ / :r a .*! b /,        "ab", "backtrack into *! (overrides :r)";

is "ab" ~~ / .+ b /,              "ab", "backtrack into +";
is "ab" ~~ / .+: b /,             Nil,  "don't backtrack into +:";
is "ab" ~~ / :r .+ b /,           Nil,  "don't backtrack into + after :r";
is "ab" ~~ / :r .+! b /,          "ab", "backtrack into +! (overrides :r)";

is "ab" ~~ / . ** 1..3 b /,       "ab", "backtrack into **";
is "ab" ~~ / . **: 1..3 b /,      Nil,  "don't backtrack into **:";
is "ab" ~~ / :r . ** 1..3 b /,    Nil,  "don't backtrack into ** after :r";
is "ab" ~~ / :r . **! 1..3 b /,   "ab", "backtrack into **! (overrides :r)";

is "ab" ~~ / [ab | a ] b /,       "ab", "backtrack into |";
is "ab" ~~ / [ab | a ]: b /,      Nil,  "don't backtrack into | with :";
is "ab" ~~ / :r [ab | a ] b /,    Nil,  "don't backtrack into | after :r";
is "ab" ~~ / :r [ab | a ]:! b /,  "ab", "backtrack into | with :! (overrides :r)";

is "ab" ~~ / [ab || a ] b /,      "ab", "backtrack into ||";
is "ab" ~~ / [ab || a ]: b /,     Nil,  "don't backtrack into || with :";
is "ab" ~~ / :r [ab || a ] b /,   Nil,  "don't backtrack into || after :r";
is "ab" ~~ / :r [ab || a ]:! b /, "ab", "backtrack into || with :! (overrides :r)";

Of these tests, the following three fail without this PR:

not ok 20 - backtrack into | with :! (overrides :r)
not ok 22 - don't backtrack into || with :
not ok 23 - don't backtrack into || after :r

With this PR applied, all 24 tests succeed.

:ratchet mode already stopped the regex engine from backtracking
into quantifiers and `|` (LTM) alternations.

Now it also stops it from backtracking into `||` (sequential)
alternations.

(Fixes RT #130117)
@smls smls changed the title Make backtracking modifiers on alternations more S05-compliant Make backtracking modifiers on alternations work correctly in different edge-cases Aug 28, 2017
@smls smls changed the title Make backtracking modifiers on alternations work correctly in different edge-cases Make backtracking modifiers on alternations work correctly Aug 28, 2017
@jnthn
Copy link
Contributor

jnthn commented Sep 20, 2017

Seems reasonable to me; thanks!

@jnthn jnthn merged commit a9bea1b into Raku:master Sep 20, 2017
smls added a commit to smls/roast that referenced this pull request Oct 6, 2017
They're the bottom eight tests listed at
Raku/nqp#368
(The other ones listed there, for quantifiers,
seem to be already covered in S05-mass/rx.t)

This closes RT #130117 and #131973.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants