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 ##2657

Author: HeikoKlare

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);

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
 	 */

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*)>", " ");