From f6fd101c3115bc1e640d9df5169ded43225a39fd Mon Sep 17 00:00:00 2001 From: Jeff Johnston Date: Tue, 21 May 2024 18:07:52 -0400 Subject: [PATCH] Fix false error on extraction of for loop contents - fix ExtractMethodAnalyzer.canHandleBranches() to not flag a non-labelled break statement if the for loop is included - add new test to ExtractMethodTests - fixes #1291 --- .../code/ExtractMethodAnalyzer.java | 29 +++++++++++++------ .../branch_in/A_test770.java | 24 +++++++++++++++ .../branch_out/A_test770.java | 28 ++++++++++++++++++ .../tests/refactoring/ExtractMethodTests.java | 5 ++++ 4 files changed, 77 insertions(+), 9 deletions(-) create mode 100644 org.eclipse.jdt.ui.tests.refactoring/resources/ExtractMethodWorkSpace/ExtractMethodTests/branch_in/A_test770.java create mode 100644 org.eclipse.jdt.ui.tests.refactoring/resources/ExtractMethodWorkSpace/ExtractMethodTests/branch_out/A_test770.java diff --git a/org.eclipse.jdt.core.manipulation/refactoring/org/eclipse/jdt/internal/corext/refactoring/code/ExtractMethodAnalyzer.java b/org.eclipse.jdt.core.manipulation/refactoring/org/eclipse/jdt/internal/corext/refactoring/code/ExtractMethodAnalyzer.java index bf96a9edd31..ac36e28c2b1 100644 --- a/org.eclipse.jdt.core.manipulation/refactoring/org/eclipse/jdt/internal/corext/refactoring/code/ExtractMethodAnalyzer.java +++ b/org.eclipse.jdt.core.manipulation/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 pendingProblems= new LinkedHashMap<>(); for (ASTNode astNode : selectedNodes) { astNode.accept(new ASTVisitor() { ArrayList 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/org.eclipse.jdt.ui.tests.refactoring/resources/ExtractMethodWorkSpace/ExtractMethodTests/branch_in/A_test770.java b/org.eclipse.jdt.ui.tests.refactoring/resources/ExtractMethodWorkSpace/ExtractMethodTests/branch_in/A_test770.java new file mode 100644 index 00000000000..c1d2798a400 --- /dev/null +++ b/org.eclipse.jdt.ui.tests.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> defs) { + for (List def : defs) { + /*]*/ + boolean isLeftRecursive= false; + for (String rule : def) { + if (!rule.isEmpty()) { + break; + } + } + + if (!isLeftRecursive) { + continue; + } + /*[*/ + } + } +} + diff --git a/org.eclipse.jdt.ui.tests.refactoring/resources/ExtractMethodWorkSpace/ExtractMethodTests/branch_out/A_test770.java b/org.eclipse.jdt.ui.tests.refactoring/resources/ExtractMethodWorkSpace/ExtractMethodTests/branch_out/A_test770.java new file mode 100644 index 00000000000..ab66d1c17f6 --- /dev/null +++ b/org.eclipse.jdt.ui.tests.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> defs) { + for (List def : defs) { + /*]*/ + extracted(def); + /*[*/ + } + } + + protected static void extracted(List def) { + boolean isLeftRecursive= false; + for (String rule : def) { + if (!rule.isEmpty()) { + break; + } + } + + if (!isLeftRecursive) { + return; + } + } +} + diff --git a/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ExtractMethodTests.java b/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ExtractMethodTests.java index 33f0dfaab27..2ff37a87298 100644 --- a/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ExtractMethodTests.java +++ b/org.eclipse.jdt.ui.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