Fix false error on extraction of for loop contents (#1415)

- fix ExtractMethodAnalyzer.canHandleBranches() to not flag a non-labelled break statement if the for loop is included - add new test to ExtractMethodTests - fixes #1291
eclipse-jdt · Jun 6, 2024 · cb81f1f · cb81f1f
1 parent 1cc6fff
commit cb81f1f
Show file tree

Hide file tree

Showing 4 changed files with 77 additions and 9 deletions.
diff --git a/...n/refactoring/org/eclipse/jdt/internal/corext/refactoring/code/ExtractMethodAnalyzer.java b/...n/refactoring/org/eclipse/jdt/internal/corext/refactoring/code/ExtractMethodAnalyzer.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
@@ -19,7 +19,9 @@
 import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 import org.eclipse.core.runtime.CoreException;
@@ -469,7 +471,7 @@ private String canHandleBranches() {
 				return RefactoringCoreMessages.ExtractMethodAnalyzer_branch_mismatch;
 		}
 
-		final String continueMatchesLoopProblem[]= { null };
+		final Map<ASTNode, String> pendingProblems= new LinkedHashMap<>();
 		for (ASTNode astNode : selectedNodes) {
 			astNode.accept(new ASTVisitor() {
 				ArrayList<String> fLocalLoopLabels= new ArrayList<>();
@@ -479,14 +481,14 @@ private String canHandleBranches() {
 				public boolean visit(BreakStatement node) {
 					SimpleName label= node.getLabel();
 					if (label != null && !fLocalLoopLabels.contains(label.getIdentifier())) {
-						continueMatchesLoopProblem[0]= Messages.format(
+						pendingProblems.put(label, Messages.format(
 							RefactoringCoreMessages.ExtractMethodAnalyzer_branch_break_mismatch,
-							new Object[] { ("break " + label.getIdentifier()) }); //$NON-NLS-1$
+							new Object[] { ("break " + label.getIdentifier()) })); //$NON-NLS-1$
 					} else if (label == null) {
 						ASTNode parentStatement= ASTNodes.getFirstAncestorOrNull(node, WhileStatement.class, ForStatement.class,
 								DoStatement.class, SwitchStatement.class, EnhancedForStatement.class);
 						if (parentStatement != null && !fBreakTargets.contains(parentStatement)) {
-							continueMatchesLoopProblem[0]= RefactoringCoreMessages.ExtractMethodAnalyzer_break_parent_missing;
+							pendingProblems.put(parentStatement, RefactoringCoreMessages.ExtractMethodAnalyzer_break_parent_missing);
 						}
 					}
 					return false;
@@ -495,33 +497,39 @@ public boolean visit(BreakStatement node) {
 				@Override
 				public boolean visit(LabeledStatement node) {
 					SimpleName label= node.getLabel();
-					if (label != null)
+					if (label != null) {
 						fLocalLoopLabels.add(label.getIdentifier());
+					}
 					return true;
 				}
 
 				@Override
 				public void endVisit(ForStatement node) {
+					pendingProblems.remove(node);
 					fBreakTargets.add(node);
 				}
 
 				@Override
 				public void endVisit(EnhancedForStatement node) {
+					pendingProblems.remove(node);
 					fBreakTargets.add(node);
 				}
 
 				@Override
 				public void endVisit(DoStatement node) {
+					pendingProblems.remove(node);
 					fBreakTargets.add(node);
 				}
 
 				@Override
 				public void endVisit(SwitchStatement node) {
+					pendingProblems.remove(node);
 					fBreakTargets.add(node);
 				}
 
 				@Override
 				public void endVisit(WhileStatement node) {
+					pendingProblems.remove(node);
 					fBreakTargets.add(node);
 				}
 
@@ -530,15 +538,18 @@ public void endVisit(ContinueStatement node) {
 					SimpleName label= node.getLabel();
 					if (label != null && !fLocalLoopLabels.contains(label.getIdentifier())) {
 						if (fEnclosingLoopLabel == null || ! label.getIdentifier().equals(fEnclosingLoopLabel.getIdentifier())) {
-							continueMatchesLoopProblem[0]= Messages.format(
+							pendingProblems.put(node, Messages.format(
 								RefactoringCoreMessages.ExtractMethodAnalyzer_branch_continue_mismatch,
-								new Object[] { "continue " + label.getIdentifier() }); //$NON-NLS-1$
+								new Object[] { "continue " + label.getIdentifier() })); //$NON-NLS-1$
 						}
 					}
 				}
 			});
 		}
-		return continueMatchesLoopProblem[0];
+		if (!pendingProblems.isEmpty()) {
+			return pendingProblems.values().iterator().next();
+		}
+		return null;
 	}
 
 	private Statement getParentLoopBody(ASTNode node) {

diff --git a/....refactoring/resources/ExtractMethodWorkSpace/ExtractMethodTests/branch_in/A_test770.java b/....refactoring/resources/ExtractMethodWorkSpace/ExtractMethodTests/branch_in/A_test770.java
@@ -0,0 +1,24 @@
+package branch_in;
+
+import java.util.List;
+
+public class A_test770 {
+
+	public static void foo(List<List<String>> defs) {
+		for (List<String> def : defs) {
+			/*]*/
+			boolean isLeftRecursive= false;
+			for (String rule : def) {
+				if (!rule.isEmpty()) {
+					break;
+				}
+			}
+
+			if (!isLeftRecursive) {
+				continue;
+			}
+			/*[*/
+		}
+	}
+}
+
diff --git a/...refactoring/resources/ExtractMethodWorkSpace/ExtractMethodTests/branch_out/A_test770.java b/...refactoring/resources/ExtractMethodWorkSpace/ExtractMethodTests/branch_out/A_test770.java
@@ -0,0 +1,28 @@
+package branch_out;
+
+import java.util.List;
+
+public class A_test770 {
+
+	public static void foo(List<List<String>> defs) {
+		for (List<String> def : defs) {
+			/*]*/
+			extracted(def);
+			/*[*/
+		}
+	}
+
+	protected static void extracted(List<String> def) {
+		boolean isLeftRecursive= false;
+		for (String rule : def) {
+			if (!rule.isEmpty()) {
+				break;
+			}
+		}
+
+		if (!isLeftRecursive) {
+			return;
+		}
+	}
+}
+
diff --git a/...tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ExtractMethodTests.java b/...tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ExtractMethodTests.java
@@ -2125,6 +2125,11 @@ public void test769() throws Exception {
 		branchTest();
 	}
 
+	@Test
+	public void test770() throws Exception {
+		branchTest();
+	}
+
 	//---- Test for CUs with compiler errors
 
 	@Test