Fix extract method false negative on local variable usage (#1418)

- fix ExtractMethodAnalyzer.computeOutput() method to add a check
  for local variables that have been added to the returnValues list
  but the declaration and all references are within the selected
  region
- add new test to ExtractMethodTests
- fixes #1292

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

@@ -18,6 +18,7 @@
 import java.lang.reflect.Modifier;
 import java.text.MessageFormat;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -678,6 +679,55 @@ private ITypeBinding[] computeTypeVariables(ITypeBinding[] bindings) {
 		return result.toArray(new ITypeBinding[result.size()]);
 	}
 
+	private class LocalWriteVisitor extends ASTVisitor {
+		final List<IVariableBinding> fRetValues;
+		final List<IVariableBinding> fOriginalRetValues;
+		final int fMinPosition;
+		final int fMaxPosition;
+		public LocalWriteVisitor(IRegion selectionRegion, List<IVariableBinding> returnValues) {
+			fRetValues= returnValues;
+			fOriginalRetValues= new ArrayList<>(returnValues);
+			fMinPosition= selectionRegion.getOffset();
+			fMaxPosition= fMinPosition + selectionRegion.getLength();
+		}
+
+		@Override
+		public boolean visit(SimpleName node) {
+			if (node.getStartPosition() > fMinPosition && node.getStartPosition() < fMaxPosition) {
+				if (node.getParent() instanceof VariableDeclarationFragment) {
+					IBinding binding= node.resolveBinding();
+					IVariableBinding foundValue= null;
+					if (binding instanceof IVariableBinding varBinding && !varBinding.isField()) {
+						for (IVariableBinding retValue : fRetValues) {
+							if (retValue.isEqualTo(binding)) {
+								foundValue= retValue;
+								break;
+							}
+						}
+						if (foundValue != null) {
+							fRetValues.remove(foundValue);
+						}
+					}
+				}
+			} else {
+				IBinding binding= node.resolveBinding();
+				IVariableBinding foundValue= null;
+				if (binding instanceof IVariableBinding varBinding && !varBinding.isField()) {
+					for (IVariableBinding origRetValue : fOriginalRetValues) {
+						if (origRetValue.isEqualTo(binding)) {
+							foundValue= origRetValue;
+							break;
+						}
+					}
+					if (foundValue != null && !fRetValues.contains(foundValue)) {
+						fRetValues.add(foundValue);
+					}
+				}
+			}
+			return super.visit(node);
+		}
+	}
+
 	private void computeOutput(RefactoringStatus status) {
 		// First find all writes inside the selection.
 		FlowContext flowContext= new FlowContext(0, fMaxVariableId + 1);
@@ -686,6 +736,13 @@ private void computeOutput(RefactoringStatus status) {
 		FlowInfo returnInfo= new InOutFlowAnalyzer(flowContext).perform(getSelectedNodes());
 		IVariableBinding[] returnValues= returnInfo.get(flowContext, FlowInfo.WRITE | FlowInfo.WRITE_POTENTIAL | FlowInfo.UNKNOWN);
 
+		// Remove all local variables declared in the selected region from potential return values
+		List<IVariableBinding> returnValueList= new ArrayList<>(Arrays.asList(returnValues));
+		LocalWriteVisitor visitor= new LocalWriteVisitor(getSelectedNodeRange(), returnValueList);
+		if (getLastCoveringNode() != null) {
+			getLastCoveringNode().accept(visitor);
+			returnValues= returnValueList.toArray(new IVariableBinding[0]);
+		}
 		// Compute a selection that exactly covers the selected nodes
 		IRegion region= getSelectedNodeRange();
 		Selection selection= Selection.createFromStartLength(region.getOffset(), region.getLength());

org.eclipse.jdt.ui.tests.refactoring/resources/ExtractMethodWorkSpace/ExtractMethodTests/locals_in/A_test579.java

@@ -0,0 +1,23 @@
+package locals_in;
+
+import java.util.List;
+import java.util.ArrayList;
+
+public class A_test579 {
+	public static void foo(List<String> defs) {
+		List<String> newDefs= new ArrayList<>();
+
+		for (String rule : defs) {
+			/*]*/boolean isLeftRecursive= false;
+			if (!isLeftRecursive) {
+				continue;
+			}
+
+			List<String> oldRules= new ArrayList<String>();
+			List<String> newSuffixes= new ArrayList<String>();
+
+			newDefs.addAll(oldRules);
+			newDefs.addAll(newSuffixes);/*[*/
+		}
+	}
+}

org.eclipse.jdt.ui.tests.refactoring/resources/ExtractMethodWorkSpace/ExtractMethodTests/locals_out/A_test579.java

@@ -0,0 +1,27 @@
+package locals_out;
+
+import java.util.List;
+import java.util.ArrayList;
+
+public class A_test579 {
+	public static void foo(List<String> defs) {
+		List<String> newDefs= new ArrayList<>();
+
+		for (String rule : defs) {
+			/*]*/extracted(newDefs);/*[*/
+		}
+	}
+
+	protected static void extracted(List<String> newDefs) {
+		boolean isLeftRecursive= false;
+		if (!isLeftRecursive) {
+			return;
+		}
+
+		List<String> oldRules= new ArrayList<String>();
+		List<String> newSuffixes= new ArrayList<String>();
+
+		newDefs.addAll(oldRules);
+		newDefs.addAll(newSuffixes);
+	}
+}

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

@@ -1655,6 +1655,11 @@ public void test578() throws Exception {
 		localsTest();
 	}
 
+	@Test
+	public void test579() throws Exception {
+		localsTest();
+	}
+
 	//---- Test expressions
 
 	@Test