Bug 572048 - Exit loop early results in wrong logic

Given:
for (byte element : alphaData) {
	int alpha = element & 0xFF;
	if (!(alpha == 0 || alpha == 255)) {
		/* Full alpha channel transparency */
		return imageData;
	}
	if (!transparency && alpha == 0) {
		transparency = true;
	}
}

When:
Applying "Break loop" cleanup

Expected:
(same)

Actual:
for (byte element : alphaData) {
	int alpha = element & 0xFF;
	if (!(alpha == 0 || alpha == 255)) {
		/* Full alpha channel transparency */
		return imageData;
	}
	if (!transparency && alpha == 0) {
		transparency = true;
		break;
	}
}

Change-Id: If60af2d032751d77ce1c41143bc73fbc96affcf8
Signed-off-by: Fabrice Tiercelin <fabrice.tiercelin@yahoo.fr>

2 files changed, 85 insertions, 22 deletions

diff --git a/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest.java b/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest.java
index 02afd39ee4..519e05106c 100644
--- a/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest.java
+++ b/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest.java
@@ -14381,6 +14381,48 @@ public class CleanUpTest extends CleanUpTestCase {
 				+ "        return isFound ? \"The result has been found\" : (\"The result has not been found\");\n" //
 				+ "    }\n" //
 				+ "\n" //
+				+ "    public boolean doNotShortcutReturn(int number) {\n" //
+				+ "        boolean isFound = false;\n" //
+				+ "        for (int i = 0; i < number; i++) {\n" //
+				+ "            if (i == 43) {\n" //
+				+ "                return false;\n" //
+				+ "            }\n" //
+				+ "            if (i == 42) {\n" //
+				+ "                isFound = true;\n" //
+				+ "            }\n" //
+				+ "        }\n" //
+				+ "        return isFound;\n" //
+				+ "    }\n" //
+				+ "\n" //
+				+ "    public boolean doNotShortcutThrow(int number) {\n" //
+				+ "        boolean isFound = false;\n" //
+				+ "        for (int i = 0; i < number; i++) {\n" //
+				+ "            if (i == 43) {\n" //
+				+ "                throw null;\n" //
+				+ "            }\n" //
+				+ "            if (i == 42) {\n" //
+				+ "                isFound = true;\n" //
+				+ "            }\n" //
+				+ "        }\n" //
+				+ "        return isFound;\n" //
+				+ "    }\n" //
+				+ "\n" //
+				+ "    public boolean doNotShortcutLabelledBreak(int number) {\n" //
+				+ "        boolean isFound = false;\n" //
+				+ "        doNotForgetMe: for (int j = 0; j < 10; j++) {\n" //
+				+ "            for (int i = 0; i < number; i++) {\n" //
+				+ "                if (i == 43) {\n" //
+				+ "                    break doNotForgetMe;\n" //
+				+ "                }\n" //
+				+ "                if (i == 42) {\n" //
+				+ "                    isFound = true;\n" //
+				+ "                }\n" //
+				+ "            }\n" //
+				+ "            isFound = false;\n" //
+				+ "        }\n" //
+				+ "        return isFound;\n" //
+				+ "    }\n" //
+				+ "\n" //
 				+ "    public String doNotBreakWithExternalIterator(int number) {\n" //
 				+ "        boolean isFound = false;\n" //
 				+ "        int i;\n" //
diff --git a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/BreakLoopCleanUp.java b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/BreakLoopCleanUp.java
index 687aa77c03..7a977240d8 100644
--- a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/BreakLoopCleanUp.java
+++ b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/BreakLoopCleanUp.java
@@ -30,6 +30,7 @@ import org.eclipse.jdt.core.dom.ASTNode;
 import org.eclipse.jdt.core.dom.ASTVisitor;
 import org.eclipse.jdt.core.dom.Assignment;
 import org.eclipse.jdt.core.dom.Block;
+import org.eclipse.jdt.core.dom.BreakStatement;
 import org.eclipse.jdt.core.dom.ClassInstanceCreation;
 import org.eclipse.jdt.core.dom.CompilationUnit;
 import org.eclipse.jdt.core.dom.EnhancedForStatement;
@@ -40,6 +41,7 @@ import org.eclipse.jdt.core.dom.InfixExpression;
 import org.eclipse.jdt.core.dom.MethodInvocation;
 import org.eclipse.jdt.core.dom.PostfixExpression;
 import org.eclipse.jdt.core.dom.PrefixExpression;
+import org.eclipse.jdt.core.dom.ReturnStatement;
 import org.eclipse.jdt.core.dom.SimpleName;
 import org.eclipse.jdt.core.dom.Statement;
 import org.eclipse.jdt.core.dom.SuperMethodInvocation;
@@ -131,22 +133,22 @@ public class BreakLoopCleanUp extends AbstractMultiFix {
 		final List<CompilationUnitRewriteOperation> rewriteOperations= new ArrayList<>();
 
 		unit.accept(new ASTVisitor() {
-			final class SideEffectVisitor extends InterruptibleVisitor {
+			final class DisturbingEffectVisitor extends InterruptibleVisitor {
 				private final Set<SimpleName> localVariableNames;
-				private boolean hasSideEffect;
+				private boolean hasDisturbingEffect;
 
-				private SideEffectVisitor(final Set<SimpleName> localVariableNames) {
+				private DisturbingEffectVisitor(final Set<SimpleName> localVariableNames) {
 					this.localVariableNames= localVariableNames;
 				}
 
-				private boolean hasSideEffect() {
-					return hasSideEffect;
+				private boolean hasDisturbingEffect() {
+					return hasDisturbingEffect;
 				}
 
 				@Override
 				public boolean visit(final Assignment node) {
 					if (!ASTNodes.hasOperator(node, Assignment.Operator.ASSIGN)) {
-						hasSideEffect= true;
+						hasDisturbingEffect= true;
 						return interruptVisit();
 					}
 
@@ -155,7 +157,7 @@ public class BreakLoopCleanUp extends AbstractMultiFix {
 
 				private boolean visitVar(final Expression modifiedVar) {
 					if (!(modifiedVar instanceof SimpleName)) {
-						hasSideEffect= true;
+						hasDisturbingEffect= true;
 						return interruptVisit();
 					}
 
@@ -169,7 +171,7 @@ public class BreakLoopCleanUp extends AbstractMultiFix {
 					}
 
 					if (!isFound) {
-						hasSideEffect= true;
+						hasDisturbingEffect= true;
 						return interruptVisit();
 					}
 
@@ -196,7 +198,7 @@ public class BreakLoopCleanUp extends AbstractMultiFix {
 							&& (mayCallImplicitToString(node.getLeftOperand())
 									|| mayCallImplicitToString(node.getRightOperand())
 									|| mayCallImplicitToString(node.extendedOperands()))) {
-						hasSideEffect= true;
+						hasDisturbingEffect= true;
 						return interruptVisit();
 					}
 
@@ -224,45 +226,64 @@ public class BreakLoopCleanUp extends AbstractMultiFix {
 
 				@Override
 				public boolean visit(final SuperMethodInvocation node) {
-					hasSideEffect= true;
+					hasDisturbingEffect= true;
 					return interruptVisit();
 				}
 
 				@Override
 				public boolean visit(final MethodInvocation node) {
-					hasSideEffect= true;
+					hasDisturbingEffect= true;
 					return interruptVisit();
 				}
 
 				@Override
 				public boolean visit(final ClassInstanceCreation node) {
-					hasSideEffect= true;
+					hasDisturbingEffect= true;
 					return interruptVisit();
 				}
 
 				@Override
 				public boolean visit(final ThrowStatement node) {
-					hasSideEffect= true;
+					hasDisturbingEffect= true;
 					return interruptVisit();
 				}
+
+				@Override
+				public boolean visit(final ReturnStatement node) {
+					hasDisturbingEffect= true;
+					return interruptVisit();
+				}
+
+				@Override
+				public boolean visit(final BreakStatement node) {
+					if (node.getLabel() != null) {
+						hasDisturbingEffect= true;
+						return interruptVisit();
+					}
+
+					return true;
+				}
 			}
 
 			@Override
 			public boolean visit(final ForStatement node) {
 				Set<SimpleName> vars= new HashSet<>();
+				List<Expression> initializers= node.initializers();
 
-				for (Expression initializer : (List<Expression>) node.initializers()) {
+				for (Expression initializer : initializers) {
 					vars.addAll(ASTNodes.getLocalVariableIdentifiers(initializer, true));
 				}
 
 				if (node.getExpression() == null
-						|| hasSideEffect(node.getExpression(), vars)
+						|| hasDisturbingEffect(node.getExpression(), vars)
 						|| node.updaters().isEmpty()) {
 					return true;
 				}
 
-				for (Expression updater : (List<Expression>) node.updaters()) {
-					if (hasSideEffect(updater, vars)) {
+				List<Expression> updaters= node.updaters();
+
+				for (Expression updater : updaters) {
+					if (hasDisturbingEffect(updater, vars)) {
 						return true;
 					}
 				}
@@ -270,10 +291,10 @@ public class BreakLoopCleanUp extends AbstractMultiFix {
 				return visitLoopBody(node.getBody(), vars);
 			}
 
-			private boolean hasSideEffect(final ASTNode node, final Set<SimpleName> allowedVars) {
-				SideEffectVisitor variableUseVisitor= new SideEffectVisitor(allowedVars);
+			private boolean hasDisturbingEffect(final ASTNode node, final Set<SimpleName> allowedVars) {
+				DisturbingEffectVisitor variableUseVisitor= new DisturbingEffectVisitor(allowedVars);
 				variableUseVisitor.traverseNodeInterruptibly(node);
-				return variableUseVisitor.hasSideEffect();
+				return variableUseVisitor.hasDisturbingEffect();
 			}
 
 			@Override
@@ -298,14 +319,14 @@ public class BreakLoopCleanUp extends AbstractMultiFix {
 					Statement statement= statements.get(i);
 					allowedVars.addAll(ASTNodes.getLocalVariableIdentifiers(statement, true));
 
-					if (hasSideEffect(statement, allowedVars)) {
+					if (hasDisturbingEffect(statement, allowedVars)) {
 						return true;
 					}
 				}
 
 				IfStatement ifStatement= ASTNodes.as(statements.get(statements.size() - 1), IfStatement.class);
 
-				if (ifStatement != null && ifStatement.getElseStatement() == null && !hasSideEffect(ifStatement.getExpression(), allowedVars)) {
+				if (ifStatement != null && ifStatement.getElseStatement() == null && !hasDisturbingEffect(ifStatement.getExpression(), allowedVars)) {
 					List<Statement> assignments= ASTNodes.asList(ifStatement.getThenStatement());
 
 					if (areAssignmentsValid(allowedVars, assignments)) {