Skip to content

Abort failing FindReplaceDocumentAdapter operations consistently #2657 #2664

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 6, 2025
Merged

Abort failing FindReplaceDocumentAdapter operations consistently #2657 #2664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f020fbe
Abort failing FindReplaceDocumentAdapter operations consistently #2657
HeikoKlare Jan 6, 2025
5cd7b18
Version bump(s) for 4.35 stream
eclipse-platform-bot Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions bundles/org.eclipse.text/src/org/eclipse/jface/text/FindReplaceDocumentAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,9 +188,6 @@ private IRegion findReplace(final FindReplaceOperationCode operationCode, int st
}
}

// Set state
fFindReplaceState= operationCode;

if (operationCode == REPLACE || operationCode == REPLACE_FIND_NEXT) {
if (regExSearch) {
Pattern pattern= fFindReplaceMatcher.pattern();
Expand All @@ -199,7 +196,10 @@ private IRegion findReplace(final FindReplaceOperationCode operationCode, int st
replaceText= interpretReplaceEscapes(replaceText, prevMatch);
Matcher replaceTextMatcher= pattern.matcher(prevMatch);
replaceText= replaceTextMatcher.replaceFirst(replaceText);
} catch (IndexOutOfBoundsException ex) {
} catch (IndexOutOfBoundsException | IllegalArgumentException ex) {
Copy link
Contributor Author

@HeikoKlare HeikoKlare Jan 6, 2025

Choose a reason for hiding this comment

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

This is necessary to fulfill the intention of interpretReplaceEscapes() to leave an unfinished character escape at the end of the pattern as is (such as in the pattern demo\) in order to produce a proper error message (which is delivered through an IllegalArgumentException from Matcher#replaceFirst()).

Copy link
Contributor

@jukzi jukzi Jan 6, 2025

Choose a reason for hiding this comment

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

such comment should be put in the code

Copy link
Contributor Author

@HeikoKlare HeikoKlare Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed, I have added a comment to the code explaining why these exceptions are caught in f020fbe.

// These exceptions are thrown by Matcher#replaceFirst(), capturing information about
// invalid regular expression patterns, such as unfinished character escape sequences
// at the end of the pattern
throw new PatternSyntaxException(ex.getLocalizedMessage(), replaceText, -1);
}
}
Expand All @@ -214,12 +214,13 @@ private IRegion findReplace(final FindReplaceOperationCode operationCode, int st
}

fDocument.replace(offset, length, replaceText);

fFindReplaceState= operationCode;

if (operationCode == REPLACE) {
return new Region(offset, replaceText.length());
}
}

if (operationCode != REPLACE) {
try {
if (forwardSearch) {
Expand All @@ -230,8 +231,11 @@ private IRegion findReplace(final FindReplaceOperationCode operationCode, int st
else
found= fFindReplaceMatcher.find();

if (operationCode == REPLACE_FIND_NEXT)
if (operationCode == REPLACE_FIND_NEXT) {
fFindReplaceState= FIND_NEXT;
} else {
fFindReplaceState= operationCode;
}

if (found && !fFindReplaceMatcher.group().isEmpty())
return new Region(fFindReplaceMatcher.start(), fFindReplaceMatcher.group().length());
Expand All @@ -247,6 +251,7 @@ private IRegion findReplace(final FindReplaceOperationCode operationCode, int st
found= fFindReplaceMatcher.find(index + 1);
}
fFindReplaceMatchOffset= index;
fFindReplaceState= operationCode;
if (index > -1) {
// must set matcher to correct position
fFindReplaceMatcher.find(index);
Expand Down
19 changes: 18 additions & 1 deletion tests/org.eclipse.text.tests/src/org/eclipse/text/tests/FindReplaceDocumentAdapterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@
*******************************************************************************/
package org.eclipse.text.tests;

import static org.junit.Assert.*;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import java.util.Arrays;
import java.util.Locale;
Expand Down Expand Up @@ -290,6 +295,18 @@ public void testRegexReplace3() throws Exception {
assertEquals("f0", fDocument.get());
}

@Test
public void testRegexReplace_invalidRegex() throws Exception {
FindReplaceDocumentAdapter findReplaceDocumentAdapter = new FindReplaceDocumentAdapter(fDocument);

fDocument.set("foo");
assertThrows(PatternSyntaxException.class, () -> regexReplace("foo", "foo\\", findReplaceDocumentAdapter));
assertEquals("foo", fDocument.get());

findReplaceDocumentAdapter.replace("foo" + System.lineSeparator(), true);
assertEquals("foo" + System.lineSeparator(), fDocument.get());
}

/*
* @since 3.4
*/
Expand Down
2 changes: 1 addition & 1 deletion tests/org.eclipse.ui.workbench.texteditor.tests/META-INF/MANIFEST.MF
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ Manifest-Version: 1.0
Bundle-ManifestVersion: 2
Bundle-Name: %Plugin.name
Bundle-SymbolicName: org.eclipse.ui.workbench.texteditor.tests
Bundle-Version: 3.14.600.qualifier
Bundle-Version: 3.14.700.qualifier
Bundle-Vendor: %Plugin.providerName
Bundle-Localization: plugin
Export-Package:
Expand Down
23 changes: 23 additions & 0 deletions ...nch.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogicTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,29 @@ public void testPerformReplaceAndFindRegEx_incrementalActive() {
executeReplaceAndFindRegExTest(textViewer, findReplaceLogic);
}

@Test
public void testPerformReplaceAndFindRegEx_withInvalidEscapeInReplace() {
TextViewer textViewer= setupTextViewer("Hello");
IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(textViewer);
findReplaceLogic.activate(SearchOptions.FORWARD);
findReplaceLogic.activate(SearchOptions.REGEX);

setFindAndReplaceString(findReplaceLogic, "Hello", "Hello\\");
boolean status= findReplaceLogic.performReplaceAndFind();
assertFalse(status);
assertThat(textViewer.getDocument().get(), equalTo("Hello"));
assertThat(findReplaceLogic.getTarget().getSelectionText(), equalTo("Hello"));
assertThat(findReplaceLogic.getStatus(), instanceOf(InvalidRegExStatus.class));

setFindAndReplaceString(findReplaceLogic, "Hello", "Hello" + System.lineSeparator());

status= findReplaceLogic.performReplaceAndFind();
assertTrue(status);
assertThat(textViewer.getDocument().get(), equalTo("Hello" + System.lineSeparator()));
assertThat(findReplaceLogic.getTarget().getSelectionText(), equalTo("Hello" + System.lineSeparator()));
expectStatusIsCode(findReplaceLogic, FindStatus.StatusCode.NO_MATCH);
}

private void executeReplaceAndFindRegExTest(TextViewer textViewer, IFindReplaceLogic findReplaceLogic) {
setFindAndReplaceString(findReplaceLogic, "<(\\w*)>", " ");

Expand Down
Loading