Conversation
Collect all matches first, then replace from last to first so patterns like "^ " with an empty replacement remove only one leading space per line instead of all leading spaces. Fixes eclipse-platform#2820 Signed-off-by: Edward Lo <ed700a@gmail.com>
FindReplaceDocumentAdapter returns no match when group().isEmpty(), so bare "^" never finds. Keep the ^[space] regression for issue 2820.
HeikoKlare
left a comment
There was a problem hiding this comment.
Thank you for this contribution.
In general, this seems to be much more consistent of the "replace all" functionality, solving multiple issue. One is the repeated matching of line starts and the removal of repeated trailing characters. More generally, this addresses the issue of matching re-execution after a single replace was performed, leading to matches only occurring after one replace was done to be processed unexpectedly. In my opinion, that should be reflected with test cases as well. It would be a good chance to better cover the replace all functionality with a representative set of test cases in general, which might be easily done with the help of an AI agent. If not available, I could also assist in adding such test cases.
Regarding the actual change, I wonder if we could reuse / unify the "replace all" and "select all" functionalities. The implementation for finding all matches seems to be duplicates now (between findAllMatches() and selectAll()).
| for (int i = matches.size() - 1; i >= 0; i--) { | ||
| Point match = matches.get(i); | ||
| // Re-select to restore find/replace state (including regex groups) | ||
| if (findAndSelect(match.x) != match.x) { |
There was a problem hiding this comment.
Why is the comparison and the resulting continue necessary? Seems like it is related to regexes, but I do not fully understand what it is supposed to do. In particular, it does not seem to be related to the issue addressed by this PR (when I remove the continue, the issue is still resolved). So in addition to an explanation/documentation, a test case covering the necessity for it would be beneficial.
There was a problem hiding this comment.
Thanks for looking at this so closely. You're right, I should have explained it properly, or not written it that way at all.
The comparison and the continue were there out of caution, without a concrete case behind them. I wasn't confident that re-finding at a stored offset would always land on the same match, so I preferred to skip rather than risk replacing the wrong region. That was speculative, which is exactly the problem you're pointing at.
So I went back and tried to find such a case. I tried line-start and end anchors, lookarounds, word boundaries, overlapping matches, groups, newlines, across a few thousand combinations, and the comparison never came out true once. That means the test you asked for can't be written, because no input reaches that branch. Which I think confirms your reading that it isn't related to #2820.
One half of that line does need to stay though, and I've documented why, so it doesn't read as an unexplained leftover. findAndSelect(match.x) itself is required. replaceSelection() ends up in FindReplaceDocumentAdapter#replace(...), which takes no offset. It uses the matcher's current match and expands $n from it. Collecting all matches first leaves that matcher exhausted, so without the re-selection every replacement throws IllegalStateException:, and replace-all becomes a no-op, plain finds included.
If you'd rather keep the guard as protection against third-party IFindReplaceTarget implementations that might not return the offset they were given, I'm happy to put it back. I'd just say so explicitly in the comment instead of leaving it implied. Either way works for me.
FindReplaceDocumentAdapter#replace() does not take an offset: it uses the matcher's current match and expands $n from it. Collecting all matches before replacing leaves that matcher exhausted, so each match has to be re-established before it is replaced - without the re-selection every replacement fails. The additional comparison of the re-selected offset against the stored one was defensive only. It never triggered: matches are replaced last to first, so the text of every pending match is unchanged and a forward search from its offset necessarily returns the same offset. Being unreachable, it also cannot be covered by a test. Drop it and document why the re-selection itself is required instead. Also let selectAll() reuse findAllMatches() rather than duplicating the same match walk, as raised in review. Add regression tests for re-matching during replace all: the anchor cases fail on the previous implementation, while the group-expansion case pins the re-selection requirement (it fails if the re-selection is removed).
replaceSelection() does not take an offset: it reads the adapter's current match and expands $n from it. Document this on the findAndSelect(match.x) call so the necessity of the re-selection is clear, following up on the review discussion where the surrounding guard was removed. Signed-off-by: Edward Lo <ed700a@gmail.com>
I've extended the
Five of these fail on the old implementation and pass now, so they are real regression tests, the four line-start cases plus the word-boundary case. The other three are guards, including the group expansion case, which covers the re-selection I mentioned in the other comment.
One thing I couldn't reproduce is the repeated trailing characters you mentioned. |
Summary
Fixes #2820
Regex Replace All with a line-start pattern such as
^→ `` removed all leading spaces on each line.replaceAll()looped find+replace until no match remained; after deleting one leading space, `^ ` matched again at the same line start.@HeikoKlare suggested collecting matches first, then replacing them. This change does that:
findAllMatches()walks forward once, advancing past each match (and by at least one character for zero-length matches).findAndSelectbeforereplaceSelection()so regex group replacements keep working.This matches the expected “one match per line start” behavior for
^and preserves existing replace-all semantics for ordinary patterns (e.g.b→aaonbbbbstill yields four replacements).Note: Open PR #3028 addresses the same issue with position-tracking in the old loop; this PR follows the find-all-then-replace approach discussed on the issue.
Test plan
testPerformReplaceAllRegExLineStartAnchor—^→ `` on multi-space indented lines removes one space per line.testPerformReplaceAllRegExZeroLengthLineStart—^→>inserts once per line without looping.FindReplaceLogicTestreplace-all cases (run in Eclipse PDE / aggregator build; not executed in this sparse checkout).Manual verification
hello/world/three.^; Replace with empty.hello/world/three(one leading space removed per line).