From f020fbedbf51e2cea6958aacae7cdf03db9c7f43 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Mon, 6 Jan 2025 11:27:51 +0100 Subject: [PATCH 1/2] Abort failing FindReplaceDocumentAdapter operations consistently #2657 The implementation of FindReplaceDocumentAdapter#findReplace(), used by its API methods #find() and #replace(), currently leaves an inconsistent state when failing because of an invalid input or state. This inconsistent state affects the last executed operation type, which the adapter stores in order to identify whether a replace operation is preceded by an according find operation. In case the #findReplace() method is called with an invalid position or to execute a replace without a preceding find, the method aborts (throwing an exception) without storing the operation to be executed as the last executed operation, i.e., it leaves the adapter as if that method was never called. In case the method is called in regular expression mode and the expression used as find or replace input is invalid, the operation aborts (throwing an exception) but still stores the operation to be executed as the last executed operation, i.e., it leaves the adapter as if that method was called successfully. This behavior is unexpected as is handles invalid inputs inconsistently. This also becomes visible in the existing consumers, such as FindReplaceTarget#replaceSelection() used by the FindReplaceDialog and FindReplaceOvleray. They assume that in case an exception occurs while trying to perform a replace operation, a subsequent replace should succeed without an additional find operation being required. This assumption does currently not hold. This change corrects the behavior of the FindReplaceDocumentAdapter#findReplace() method to always leave the adapter in an unmodified state when the method fails because of being called with invalid input or in an inconsistent state. In addition, regular expressions with an unfinished character escape at the end now lead to a proper exception. Fixes #https://github.com/eclipse-platform/eclipse.platform.ui/issues/2657 --- .../text/FindReplaceDocumentAdapter.java | 19 +++++++++------ .../tests/FindReplaceDocumentAdapterTest.java | 19 ++++++++++++++- .../findandreplace/FindReplaceLogicTest.java | 23 +++++++++++++++++++ 3 files changed, 53 insertions(+), 8 deletions(-) diff --git a/bundles/org.eclipse.text/src/org/eclipse/jface/text/FindReplaceDocumentAdapter.java b/bundles/org.eclipse.text/src/org/eclipse/jface/text/FindReplaceDocumentAdapter.java index 6cbe87da3dc..984800a4f64 100644 --- a/bundles/org.eclipse.text/src/org/eclipse/jface/text/FindReplaceDocumentAdapter.java +++ b/bundles/org.eclipse.text/src/org/eclipse/jface/text/FindReplaceDocumentAdapter.java @@ -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(); @@ -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) { + // 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); } } @@ -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) { @@ -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()); @@ -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); diff --git a/tests/org.eclipse.text.tests/src/org/eclipse/text/tests/FindReplaceDocumentAdapterTest.java b/tests/org.eclipse.text.tests/src/org/eclipse/text/tests/FindReplaceDocumentAdapterTest.java index 517939d277d..98de7eb46d7 100644 --- a/tests/org.eclipse.text.tests/src/org/eclipse/text/tests/FindReplaceDocumentAdapterTest.java +++ b/tests/org.eclipse.text.tests/src/org/eclipse/text/tests/FindReplaceDocumentAdapterTest.java @@ -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; @@ -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 */ diff --git a/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogicTest.java b/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogicTest.java index df6d786d9a8..992b4aefb8a 100644 --- a/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogicTest.java +++ b/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogicTest.java @@ -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*)>", " "); From 5cd7b1824234e3b16c5b9c75b784980ee5632d91 Mon Sep 17 00:00:00 2001 From: Eclipse Platform Bot Date: Mon, 6 Jan 2025 13:17:02 +0000 Subject: [PATCH 2/2] Version bump(s) for 4.35 stream --- .../META-INF/MANIFEST.MF | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/org.eclipse.ui.workbench.texteditor.tests/META-INF/MANIFEST.MF b/tests/org.eclipse.ui.workbench.texteditor.tests/META-INF/MANIFEST.MF index 1a6ef95d92a..05ec3b30864 100644 --- a/tests/org.eclipse.ui.workbench.texteditor.tests/META-INF/MANIFEST.MF +++ b/tests/org.eclipse.ui.workbench.texteditor.tests/META-INF/MANIFEST.MF @@ -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: