Skip to content

Commit

Permalink
regex: fix fast path for -w/--word-regexp flag (#2576)
Browse files Browse the repository at this point in the history
It turns out our fast path for -w/--word-regexp wasn't quite correct in
some cases. Namely, we use `(?m:^|\W)(<original-regex>)(?m:\W|$)` as the
implementation of -w/--word-regexp since `\b(<original-regex>)\b` has
some unintuitive results in certain cases, specifically when
<original-regex> matches non-word characters at match boundaries.

The problem is that using this formulation means that you need to
extract the capture group around <original-regex> to find the "real"
match, since the surrounding (^|\W) and (\W|$) aren't part of the match.
This is fine, but the capture group engine is usually slow, so we have a
fast path where we try to deduce the correct match boundary after an
initial match (before running capture groups). The problem is that doing
this is rather tricky because it's hard to know, in general, whether the
`^` or the `\W` matched.

This still doesn't seem quite right overall, but we at least fix one
more case.

Fixes #2574
  • Loading branch information
BurntSushi committed Jul 31, 2023
1 parent fed4fea commit 341a19e
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 2 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ Bug fixes:
Fix bug when using inline regex flags with `-e/--regexp`.
* [BUG #2523](https://github.com/BurntSushi/ripgrep/issues/2523):
Make executable searching take `.com` into account on Windows.
* [BUG #2574](https://github.com/BurntSushi/ripgrep/issues/2574):
Fix bug in `-w/--word-regexp` that would result in incorrect match offsets.


13.0.0 (2021-06-12)
Expand Down
16 changes: 14 additions & 2 deletions crates/regex/src/word.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,9 @@ impl WordMatcher {
// The reason why we cannot handle the ^/$ cases here is because we
// can't assume anything about the original pattern. (Try commenting
// out the checks for ^/$ below and run the tests to see examples.)
//
// NOTE(2023-07-31): After fixing #2574, this logic honestly still
// doesn't seem correct. Regex composition is hard.
let input = Input::new(haystack).span(at..haystack.len());
let mut cand = match self.regex.find(input) {
None => return Ok(None),
Expand All @@ -136,8 +139,17 @@ impl WordMatcher {
if cand.start() == 0 || cand.end() == haystack.len() {
return Err(());
}
let (_, slen) = bstr::decode_utf8(&haystack[cand]);
let (_, elen) = bstr::decode_last_utf8(&haystack[cand]);
// We decode the chars on either side of the match. If either char is
// a word character, then that means the ^/$ matched and not \W. In
// that case, we defer to the slower engine.
let (ch, slen) = bstr::decode_utf8(&haystack[cand]);
if ch.map_or(true, regex_syntax::is_word_character) {
return Err(());
}
let (ch, elen) = bstr::decode_last_utf8(&haystack[cand]);
if ch.map_or(true, regex_syntax::is_word_character) {
return Err(());
}
let new_start = cand.start() + slen;
let new_end = cand.end() - elen;
// This occurs the original regex can match the empty string. In this
Expand Down
15 changes: 15 additions & 0 deletions tests/regression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1173,3 +1173,18 @@ rgtest!(r2480, |dir: Dir, mut cmd: TestCommand| {
cmd.args(&["--only-matching", "-e", "(?i)notfoo", "-e", "bar", "file"]);
cmd.assert_err();
});

// See: https://github.com/BurntSushi/ripgrep/issues/2574
rgtest!(r2574, |dir: Dir, mut cmd: TestCommand| {
dir.create("haystack", "some.domain.com\nsome.domain.com/x\n");
let got = cmd
.args(&[
"--no-filename",
"--no-unicode",
"-w",
"-o",
r"(\w+\.)*domain\.(\w+)",
])
.stdout();
eqnice!("some.domain.com\nsome.domain.com\n", got);
});

0 comments on commit 341a19e

Please sign in to comment.