Add checking so no invalid extract local variable proposals are offer…

…ed (#1177)

* Add checking so no invalid extract local variable proposals are offered

- fix ExtractTempRefactoring.checkFinalConditions() to recognize an
  empty edit and set refactoring status to info with failure message
- also fix the method to register it has been run and to save the
  refactoring status code
- fix QuickAssistProcessor.getExtractVariableProposal() to run the
  checkFinalConditions() method of the extract temp refactorings
  and to not offer a proposal if the status is not OK
- add new tests to ExtractTempTests and AssistQuickFixTest

org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/RefactoringCoreMessages.java

@@ -713,6 +713,8 @@ public final class RefactoringCoreMessages extends NLS {
 
 	public static String ExtractTempRefactoring_side_effcts_in_selected_expression;
 
+	public static String ExtractTempRefactoring_side_effects_possible;
+
 	public static String FlowAnalyzer_execution_flow;
 
 	public static String HierarchyRefactoring_add_member;

org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/refactoring.properties

@@ -306,6 +306,7 @@ ExtractTempRefactoring_resource_in_try_with_resources=Cannot extract a resource
 ExtractTempRefactoring_destination_pattern=Destination method: ''{0}''
 ExtractTempRefactoring_for_initializer_updater=Cannot extract \'for\' initializer or updater.
 ExtractTempRefactoring_side_effcts_in_selected_expression=Only one expression can be extracted due to side-effects.
+ExtractTempRefactoring_side_effects_possible=Expression will not be extracted as it may affect statement logic
 
 #-- extract class --------------------------------------------------
 ExtractClassRefactoring_change_comment_header=Extract class ''{0}'' from fields in ''{1}''

org.eclipse.jdt.core.manipulation/refactoring/org/eclipse/jdt/internal/corext/refactoring/code/ExtractTempRefactoring.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2000, 2023 IBM Corporation and others.
+ * Copyright (c) 2000, 2024 IBM Corporation and others.
  *
  * This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License 2.0
@@ -40,6 +40,7 @@
 import org.eclipse.core.runtime.IProgressMonitor;
 
 import org.eclipse.text.edits.CopySourceEdit;
+import org.eclipse.text.edits.MultiTextEdit;
 import org.eclipse.text.edits.TextEdit;
 import org.eclipse.text.edits.TextEditGroup;
 import org.eclipse.text.edits.TextEditVisitor;
@@ -526,6 +527,10 @@ private IASTFragment[] retainOnlyReplacableMatches(IASTFragment[] allMatches) {
 
 	private CompilationUnitChange fChange;
 
+	private boolean fCheckFinal;
+
+	private RefactoringStatus fCheckFinalResult;
+
 	private LinkedProposalModelCore fLinkedProposalModel;
 
 	private static final String KEY_NAME= "name"; //$NON-NLS-1$
@@ -709,52 +714,65 @@ public RefactoringStatus checkFinalConditions(IProgressMonitor pm) throws CoreEx
 		try {
 			pm.beginTask(RefactoringCoreMessages.ExtractTempRefactoring_checking_preconditions, 4);
 
-			fCURewrite= new CompilationUnitRewrite(null, fCu, fCompilationUnitNode, fFormatterOptions);
-			fCURewrite.getASTRewrite().setTargetSourceRangeComputer(new NoCommentSourceRangeComputer());
+			if (!fCheckFinal) {
 
-			RefactoringStatus result= new RefactoringStatus();
-			fStartPoint= -1;
-			fEndPoint= -1;
-			fSeen.clear();
-			boolean replaceAll= fReplaceAllOccurrences;
+				fCURewrite= new CompilationUnitRewrite(null, fCu, fCompilationUnitNode, fFormatterOptions);
+				fCURewrite.getASTRewrite().setTargetSourceRangeComputer(new NoCommentSourceRangeComputer());
+
+				RefactoringStatus result= new RefactoringStatus();
+				fStartPoint= -1;
+				fEndPoint= -1;
+				fSeen.clear();
+				boolean replaceAll= fReplaceAllOccurrences;
 
-			if (fReplaceAllOccurrences) {
-				RefactoringStatus checkSideEffectsInSelectedExpression= checkSideEffectsInSelectedExpression();
-				if (checkSideEffectsInSelectedExpression.hasInfo()) {
-					fReplaceAllOccurrences= false;
-					result.merge(checkSideEffectsInSelectedExpression);
+				if (fReplaceAllOccurrences) {
+					RefactoringStatus checkSideEffectsInSelectedExpression= checkSideEffectsInSelectedExpression();
+					if (checkSideEffectsInSelectedExpression.hasInfo()) {
+						fReplaceAllOccurrences= false;
+						result.merge(checkSideEffectsInSelectedExpression);
+					}
 				}
-			}
 
-			doCreateChange(Progress.subMonitor(pm, 2));
+				doCreateChange(Progress.subMonitor(pm, 2));
 
-			fChange= fCURewrite.createChange(RefactoringCoreMessages.ExtractTempRefactoring_change_name, true, Progress.subMonitor(pm, 1));
+				fChange= fCURewrite.createChange(RefactoringCoreMessages.ExtractTempRefactoring_change_name, true, Progress.subMonitor(pm, 1));
 
-			fChange.getEdit().accept(new TextEditVisitor() {
-				@Override
-				public void preVisit(TextEdit edit) {
-					TextEdit[] children= edit.getChildren();
-					for (TextEdit te : children)
-						if (te instanceof CopySourceEdit) {
-							CopySourceEdit cse= (CopySourceEdit) te;
-							if (cse.getTargetEdit() == null || cse.getTargetEdit().getOffset() == 0) {
-								edit.removeChild(te);
+				fChange.getEdit().accept(new TextEditVisitor() {
+					@Override
+					public void preVisit(TextEdit edit) {
+						TextEdit[] children= edit.getChildren();
+						for (TextEdit te : children)
+							if (te instanceof CopySourceEdit) {
+								CopySourceEdit cse= (CopySourceEdit) te;
+								if (cse.getTargetEdit() == null || cse.getTargetEdit().getOffset() == 0) {
+									edit.removeChild(te);
+								}
 							}
+						super.preVisit(edit);
+					}
+				});
+				if (fChange.getEdit() instanceof MultiTextEdit topEdit) {
+					TextEdit[] children= topEdit.getChildren();
+					if (children.length == 1 && children[0] instanceof MultiTextEdit childEdit) {
+						if (childEdit.hasChildren() == false) {
+							result.addInfo(RefactoringCoreMessages.ExtractTempRefactoring_side_effects_possible);
 						}
-					super.preVisit(edit);
+					}
 				}
-			});
-			if (Arrays.asList(getExcludedVariableNames()).contains(fTempName))
-				result.addWarning(Messages.format(RefactoringCoreMessages.ExtractTempRefactoring_another_variable, BasicElementLabels.getJavaElementName(fTempName)));
+				if (Arrays.asList(getExcludedVariableNames()).contains(fTempName))
+					result.addWarning(Messages.format(RefactoringCoreMessages.ExtractTempRefactoring_another_variable, BasicElementLabels.getJavaElementName(fTempName)));
 
-			result.merge(checkMatchingFragments());
-			fChange.setKeepPreviewEdits(false);
+				result.merge(checkMatchingFragments());
+				fChange.setKeepPreviewEdits(false);
 
-			if (fCheckResultForCompileProblems) {
-				result.merge(RefactoringAnalyzeUtil.checkNewSource(fChange, fCu, fCompilationUnitNode, pm));
+				if (fCheckResultForCompileProblems) {
+					result.merge(RefactoringAnalyzeUtil.checkNewSource(fChange, fCu, fCompilationUnitNode, pm));
+				}
+				fReplaceAllOccurrences= replaceAll;
+				fCheckFinal= true;
+				fCheckFinalResult= result;
 			}
-			fReplaceAllOccurrences= replaceAll;
-			return result;
+			return fCheckFinalResult;
 		} finally {
 			pm.done();
 		}

org.eclipse.jdt.core.manipulation/refactoring/org/eclipse/jdt/internal/corext/refactoring/util/ChangedValueChecker.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2000, 2023 IBM Corporation and others.
+ * Copyright (c) 2000, 2024 IBM Corporation and others.
  *
  * This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License 2.0
@@ -483,6 +483,9 @@ public boolean visit(SimpleName simpleName) {
 		@Override
 		public boolean visit(MethodInvocation methodInvocation) {
 			final IMethodBinding resolveMethodBinding= methodInvocation.resolveMethodBinding();
+			if (resolveMethodBinding == null) {
+				return super.visit(methodInvocation);
+			}
 			if (!this.visitMethodCall || resolveMethodBinding.getMethodDeclaration() != null && fEnclosingMethodSignature != null &&
 					fEnclosingMethodSignature.equals(resolveMethodBinding.getMethodDeclaration().getKey())) {
 				return super.visit(methodInvocation);

org.eclipse.jdt.ui.tests.refactoring/resources/ExtractTemp/cannotExtract/A_testFail43.java

@@ -0,0 +1,23 @@
+package p; //19, 9, 19, 88
+
+public class A {
+	private Object elementName;
+	private Object parent;
+
+	public Object getElementName() {
+		return elementName;
+	}
+
+    private class UtilClass {
+        public static int combineHashCodes(int a, int b) {
+            return a + b;
+        }
+    }
+    @Override
+    public int hashCode() {
+        int k = this.parent == null ? super.hashCode() :
+        UtilClass.combineHashCodes(getElementName().hashCode(), this.parent.hashCode());
+        return k;
+    }
+
+}

org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ExtractTempTests.java

@@ -866,12 +866,6 @@ public void test118() throws Exception {
 		helper1(8, 28, 8, 38, true, false, "length", "length");
 	}
 
-	@Test
-	public void test119() throws Exception {
-		//test for https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/39
-		helper1(5, 28, 5, 38, true, false, "length", "length");
-	}
-
 	@Test
 	public void test120() throws Exception {
 		//test for https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/39
@@ -884,12 +878,6 @@ public void test121() throws Exception {
 		helper1(8, 44, 8, 58, true, false, "length", "length");
 	}
 
-	@Test
-	public void test122() throws Exception {
-		//test for https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/39
-		helper1(5, 68, 5, 94, true, false, "intValue", "intValue");
-	}
-
 	@Test
 	public void test123() throws Exception {
 		//test for https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/39
@@ -1366,4 +1354,23 @@ public void testFail40() throws Exception {
 		//test for https://bugs.eclipse.org/bugs/show_bug.cgi?id=100430
 		failHelper1(7, 16, 7, 28, false, false, "bar", RefactoringStatus.WARNING);
 	}
+
+	@Test
+	public void testFail41() throws Exception {
+		//test for https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/39
+		failHelper1(5, 28, 5, 38, true, false, "length", RefactoringStatus.INFO);
+	}
+
+	@Test
+	public void testFail42() throws Exception {
+		//test for https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/39
+		failHelper1(5, 68, 5, 94, true, false, "intValue", RefactoringStatus.INFO);
+	}
+
+	@Test
+	public void testFail43() throws Exception {
+		//test for https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1176
+		failHelper1(19, 9, 19, 88, true, false, "intValue", RefactoringStatus.INFO);
+	}
+
 }

org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ExtractTempTests1d8.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2014, 2023 IBM Corporation and others.
+ * Copyright (c) 2014, 2024 IBM Corporation and others.
  *
  * This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License 2.0
@@ -95,7 +95,6 @@ public void test118() throws Exception {
 		helper1(6, 59, 6, 59, true, false, "string", "string");
 	}
 
-	@Override
 	@Test
 	public void test119() throws Exception {
 		helper1(7, 30, 7, 63, true, false, "supplier", "supplier");