Repository navigation
Conversation
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.
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
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.
Fixes #898.
The bug
AutoPossessification.swiftwalks the flatDSLList, and its twotraversals disagreed about where they left
position: on the nodeitself, 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 comparedagainst the
+inside\+*rather than againstA, was madepossessive on the strength of that, swallowed all of
"aA", andA{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
skipNodeand restore the cursor there in adefer, so a case is free to look at as many or as few children as itneeds and callers always read the next sibling.
The concatenation case of
autoPossessifyNextQuantificationbecomes asingle left-to-right pass carrying the quantification it is trying to
possessify. Two behaviour changes fall out of that:
.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])bmissed"aab".first one, so the optimization fires more than before —
\w+\s\d+apossessifies both of its quantifiers where it used to possessify one.
Testing
MatchTests.testMatchQuantificationgets the three reproducers fromthe issue plus the
[ab]one above; all six new cases fail onmainand pass here.
OptimizationTestsgetspartialAutoPossessify, which pins the exactnumber of quantifications possessified per pattern, so that neither
over- nor under-possessification can slip through, and
(?:a*[ab])bjoinsnoAutoPossessify. These are gated onmacOS 9999like the rest of the suite; I ran them with the gatelowered to confirm they fail before the change (4 of 7 expectations,
plus the
noAutoPossessifycase) and pass after it.DSLListand behaves correctly) over ~590k generated pattern/inputpairs 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.