Skip to content

Fix Match Whole Word skipping overlapping matches - #326447

Open
varshitha (dobbydobap) wants to merge 6 commits into
microsoft:mainfrom
dobbydobap:fix-291591-wholeword-overlap
Open

varshitha (dobbydobap) wants to merge 6 commits into
microsoft:mainfrom
dobbydobap:fix-291591-wholeword-overlap

Conversation

@dobbydobap

Copy link
Copy Markdown
Contributor

Fixes #291591. Match Whole Word is applied as a post-filter on each regex match; when a candidate fails the boundary check, the search resumed at the end of that candidate, skipping any valid whole-word match that starts inside it (e.g. the trailing aa-aa in xaa-aa-aa, which grep -w finds). Now it advances one code point on a failed check so overlapping matches are considered. Added a regression test.

Copilot AI review requested due to automatic review settings July 18, 2026 14:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes whole-word search skipping overlapping candidates after a boundary-check failure.

Changes:

  • Advances rejected matches by one Unicode code point.
  • Adds regression coverage for issue #291591.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/vs/editor/common/model/textModelSearch.ts Adjusts search progression after rejected matches.
src/vs/editor/test/common/model/textModelSearch.test.ts Adds overlapping-match regression test.

Comment thread src/vs/editor/common/model/textModelSearch.ts Outdated
Comment thread src/vs/editor/common/model/textModelSearch.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment on lines +269 to +272
lastMatchIndex = text.indexOf(searchString, lastMatchIndex + searchStringLen);
} else {
// Not a whole-word match; advance by one so overlapping whole-word matches aren't skipped.
lastMatchIndex = text.indexOf(searchString, lastMatchIndex + 1);
@dobbydobap

Copy link
Copy Markdown
Contributor Author

right on both counts. pieceTreeBase.ts had its own copy of that loop and it's the one model.findMatches ends up in. rather than patch the same loop twice I pulled it out into findSimpleMatchesInLine in textModelSearch.ts, and both TextModelSearch._findMatchesInLine and PieceTreeBase._findMatchesInLine call it now — that duplication is what let the first commit fix only half of it.

one thing worth flagging on the test: findMatchesLineByLine returns early through findMatchesInNode when the range starts and ends in the same node, and that path goes through Searcher, which was already fixed. so a plain createTextModel(text) passes with or without this change. the test builds the model from a chunked PieceTreeTextBufferBuilder so the search crosses a node boundary and actually reaches the line scanner — it fails without this commit and passes with it.

}

// Not a whole-word match; advance one code point so overlapping whole-word matches aren't skipped.
this._searchRegex.lastIndex = matchStartIndex + (strings.getNextCodePoint(text, textLength, matchStartIndex) > 0xFFFF ? 2 : 1);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Review: Retrying from every code point after a rejected whole-word candidate makes the new regex path quadratic. For example, whole-word /a+/ over x plus 40k as reruns the regex 40k times and took about 542 ms locally instead of one scan; the literal helper has the same repeated-suffix behavior. Because find runs synchronously, long/minified lines can freeze the renderer. Please advance to the next position that can satisfy the left-boundary rules (while honoring separators, CR/LF, and code points) and add long-line coverage for both paths.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, the one code point retry makes the regex path quadratic, and the literal helper has the same repeated-suffix problem. reworking it to advance to the next position that can actually satisfy the left boundary rule instead of stepping one code point at a time on your x + 40k as case no such position exists, so it scans once instead of 40k times, and the overlap fix still holds since a whole word match can only start right after a boundary. adding long line coverage for both paths.


// Not a whole-word match; advance one code point so overlapping whole-word matches aren't skipped.
this._searchRegex.lastIndex = matchStartIndex + (strings.getNextCodePoint(text, textLength, matchStartIndex) > 0xFFFF ? 2 : 1);
this._prevMatchStartIndex = -1;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Review: Searcher accepts any RegExp, and SearchData does not require g/y. For a non-global /abc/ on xabc, exec() ignores the assigned lastIndex and repeats index 1; clearing the previous-match state here defeats the duplicate-range guard, so next() loops forever. Current factories use global regexes, but an existing piece-tree test already constructs non-global SearchData. Please preserve duplicate detection for regexes that cannot advance via lastIndex, or explicitly normalize/reject them, with regression coverage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a real hang. clearing the prev-match state was only there to get past the end-of-line guard at the top of the loop, but it takes the duplicate-range check out with it, so a regex that can't advance via lastIndex never terminates. switching to a one-shot flag that skips just the end-of-line check and leaves _prevMatchStartIndex/_prevMatchLength intact. adding a non-global regression test pieceTreeTextBuffer.test.ts already constructs a non-global SearchData with separators, it just happens to run on an empty buffer today so it never spins.

This branch has not been deployed

No deployments
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.

Find with "Match Whole Word" misses valid occurrences

4 participants