Skip to content

fix(syntax): don't lift common prefix when it can match multiple ways - #1408

Closed
VishnuSrivatsava wants to merge 1 commit into
rust-lang:masterfrom
VishnuSrivatsava:fix/lift-common-prefix-preference
Closed

VishnuSrivatsava wants to merge 1 commit into
rust-lang:masterfrom
VishnuSrivatsava:fix/lift-common-prefix-preference

Conversation

@VishnuSrivatsava

Copy link
Copy Markdown

Problem

Hir::alternation calls lift_common_prefix, which rewrites XA|XB into X(?:A|B). When the common prefix X can match in more than one way, this rewrite is not leftmost-first equivalent. In XA|XB, every way of matching X followed by A is preferred over every way of matching X followed by B. But in X(?:A|B), X's most preferred way followed by B completes before X's less preferred way followed by A, since the lifted X never backtracks into its less preferred ways once the continuation matches.

Minimal example: a+a|a+b against aab.

Regex::new("a+a|a+b").unwrap().find("aab")
// expected: Some(0..2), matching "aa" (first branch, Perl style)
// actual:   Some(0..3), matching "aab"

This is #1400 (diagnosis credit to @ranflarion, who connected the lift_common_prefix rewrite to this behavior by differential testing against java.util.regex). It also manifests through ripgrep: BurntSushi/ripgrep#3568. The behavior has been present since regex-syntax 0.7.0 (regex 1.8.0), where the lift was introduced.

Solution

Skip the lift when the common prefix can match in more than one way. A new conservative analysis is_multi_way reports whether an expression has more than one distinct way to match; the lift now only applies to single way prefixes (literals, classes, exact count repetitions of single way expressions and concatenations of those). Some safe lifts may now be skipped, but no unsafe lift is performed.

Test

A unit test in regex-syntax asserts that a+a|a+b remains an alternation after translation while a single way prefix case still lifts. An integration test in tests/regression.rs asserts the correct match spans.

Verified: full workspace test suites pass (about 1100 tests), and the match span is correct across engine configurations (default and minimal size limit).

Fixes #1400

Hir::alternation calls lift_common_prefix, which rewrites XA|XB into
X(?:A|B). When the common prefix X can match in more than one way, this
rewrite is not leftmost-first equivalent: in XA|XB, every way of
matching X followed by A is preferred over every way of matching X
followed by B, but in X(?:A|B), X's first way followed by B completes
before X's second way followed by A, since the lifted X never
backtracks into its less preferred ways once the continuation matches.

Minimal example: 'a+a|a+b' against 'aab' matches 'aab' instead of 'aa'.

Skip the lift when the common prefix can match in more than one way.
This is conservative: some safe lifts may now be skipped, but no unsafe
lift is performed. Single-way prefixes (literals, classes and
exact-count repetitions of single-way expressions) are unaffected.

Fixes rust-lang#1400
@VishnuSrivatsava

Copy link
Copy Markdown
Author

Closing this as a duplicate of #1401, which implements the same fix with a better shape: rather than skipping the lift entirely, the prefix walk stops at the first element that can match in more than one way, so partial single way prefixes are still lifted. Sorry for the noise — I checked this issue for existing discussion but missed the linked pull request in the timeline.

Before closing I verified #1401 independently: with its branch, Regex::new("a+a|a+b").find("aab") returns Some(0..2) across engine configurations (default and minimal size limit), and the full regex and regex-syntax test suites pass on the branch. The verification table in this PR's description (all working output modes unchanged, both failure-shaped inputs corrected) applies to #1401 as well. @ranflarion the HIR equivalence test style in your PR is nice; an end-to-end span assertion like the one in this PR's tests/regression.rs commit could be a useful addition if you want it, but it is not required.

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.

wrong leftmost-first match when alternation branches start with the same repetition

1 participant