Skip to content

Fix cursor bookkeeping in auto-possessification - #908

Open
maxches99 wants to merge 1 commit into
swiftlang:mainfrom
maxches99:fix/auto-possessify-cursor
Open

maxches99 wants to merge 1 commit into
swiftlang:mainfrom
maxches99:fix/auto-possessify-cursor

Conversation

@maxches99

Copy link
Copy Markdown

Fixes #898.

The bug

print("aA".matches(of: try Regex(#"(?:a*\+*)A{2}"#).ignoresCase()).count)  // 0, expected 1

AutoPossessification.swift walks the flat DSLList, and its two
traversals disagreed about where they left position: on the node
itself, on the first node of a child's subtree, or on the subtree's last
node, depending on which case ran. An optional quantification, for one,
answered "no required first atom" without stepping over its child, so
the enclosing concatenation went on to read a node from inside that
quantification instead of the next sibling. Here a* got compared
against the + inside \+* rather than against A, was made
possessive on the strength of that, swallowed all of "aA", and A{2}
had nothing left to match.

This is the same class of defect as the forward-progress walk in #907,
in a neighbouring traversal; that PR does not cover this one.

The fix

Both functions now use the invariant from that fix: find the end of the
subtree up front with skipNode and restore the cursor there in a
defer, so a case is free to look at as many or as few children as it
needs and callers always read the next sibling.

The concatenation case of autoPossessifyNextQuantification becomes a
single left-to-right pass carrying the quantification it is trying to
possessify. Two behaviour changes fall out of that:

  • A component with no determinable required first atom (.some(nil),
    e.g. a custom character class) now drops the pending quantification
    instead of handing it to the enclosing concatenation, which was
    possessifying it against an unrelated atom. Found while fixing the
    above, not in the issue: (?:a*[ab])b missed "aab".
  • Every quantification of a concatenation is considered, not just the
    first one, so the optimization fires more than before — \w+\s\d+a
    possessifies both of its quantifiers where it used to possessify one.

Testing

  • MatchTests.testMatchQuantification gets the three reproducers from
    the issue plus the [ab] one above; all six new cases fail on main
    and pass here.
  • OptimizationTests gets partialAutoPossessify, which pins the exact
    number of quantifications possessified per pattern, so that neither
    over- nor under-possessification can slip through, and
    (?:a*[ab])b joins noAutoPossessify. These are gated on
    macOS 9999 like the rest of the suite; I ran them with the gate
    lowered to confirm they fail before the change (4 of 7 expectations,
    plus the noAutoPossessify case) and pass after it.
  • Full suite: 232 XCTest tests, 0 failures.
  • Differential run against the OS engine (which predates the flat
    DSLList and behaves correctly) over ~590k generated pattern/input
    pairs weighted toward quantifiers with optional neighbours, groups,
    lookarounds and alternations: 40 mismatches before the change — lost
    matches and wrong match extents — and none after, with nothing newly
    broken. Compile+match time over a 120k-pair corpus is unchanged within
    measurement noise.

The two traversals in AutoPossessification.swift disagreed about where
they left `position` in the flat node list: on the node itself, on the
first node of a child's subtree, or on the subtree's last node. An
optional quantification, for one, answered "no required first atom"
without stepping over its child, so the enclosing concatenation went on
to read a node from inside that quantification instead of the next
sibling. In `(?i)(?:a*\+*)A{2}`, `a*` was compared against the `+` inside
`\+*` rather than against `A`, made possessive on the strength of that,
and the pattern stopped matching "aA".

Give both functions the same invariant as the forward-progress walk:
find the end of the subtree up front with `skipNode` and restore the
cursor there on the way out, so that a case is free to look at as many
or as few children as it needs.

The concatenation case of `autoPossessifyNextQuantification` is now a
single left-to-right pass carrying the quantification it is trying to
possessify. A component with no determinable required first atom drops
that quantification instead of handing it to the enclosing
concatenation, which was possessifying it against an unrelated atom:
`(?:a*[ab])b` no longer loses "aab". Every quantification of a
concatenation is considered, not just the first one, so `\w+\s\d+a`
possessifies both of its quantifiers.

Fixes swiftlang#898.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Auto-possessification loses its place after an optional sibling: (?i)(?:a*\+*)A{2} misses "aA"

1 participant