[IDEA-295460] ConditionalBreak: Handle else & finite loop

GitOrigin-RevId: 51df8f08a54c35043d45f728e80f10f68543feee
This commit is contained in:
Fabrice TIERCELIN
2022-06-28 17:09:51 +02:00
committed by intellij-monorepo-bot
parent a124d59a71
commit d5963c01d2
30 changed files with 470 additions and 63 deletions

View File

@@ -1,16 +1,12 @@
// Copyright 2000-2017 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
package com.intellij.codeInspection;
import com.intellij.codeInspection.ui.MultipleCheckboxOptionsPanel;
import com.intellij.java.JavaBundle;
import com.intellij.openapi.project.Project;
import com.intellij.psi.*;
import com.intellij.psi.util.PsiTreeUtil;
import com.siyeh.ig.psiutils.BoolUtils;
import com.siyeh.ig.psiutils.CommentTracker;
import com.siyeh.ig.psiutils.ControlFlowUtils;
import com.siyeh.ig.psiutils.VariableAccessUtils;
import com.siyeh.ig.psiutils.*;
import one.util.streamex.StreamEx;
import org.jetbrains.annotations.Nls;
import org.jetbrains.annotations.NotNull;
@@ -18,6 +14,10 @@ import org.jetbrains.annotations.Nullable;
import javax.swing.*;
import java.util.Collections;
import java.util.Set;
import java.util.stream.Collectors;
import static com.intellij.util.ObjectUtils.tryCast;
public class ConditionalBreakInInfiniteLoopInspection extends AbstractBaseJavaLocalInspectionTool {
@@ -35,24 +35,23 @@ public class ConditionalBreakInInfiniteLoopInspection extends AbstractBaseJavaLo
@Override
public PsiElementVisitor buildVisitor(@NotNull ProblemsHolder holder, boolean isOnTheFly) {
return new JavaElementVisitor() {
@Override
public void visitForStatement(@NotNull PsiForStatement statement) {
visitLoop(statement, statement.getFirstChild());
}
@Override
public void visitWhileStatement(@NotNull PsiWhileStatement statement) {
visitLoop(statement);
visitLoop(statement, statement.getFirstChild());
}
@Override
public void visitDoWhileStatement(@NotNull PsiDoWhileStatement statement) {
visitLoop(statement);
visitLoop(statement, statement.getWhileKeyword());
}
@Override
public void visitForStatement(@NotNull PsiForStatement statement) {
visitLoop(statement);
}
private void visitLoop(@NotNull PsiConditionalLoopStatement loopStatement) {
PsiElement keyword = getKeyword(loopStatement);
if(keyword == null) return;
private void visitLoop(@NotNull PsiConditionalLoopStatement loopStatement, @Nullable PsiElement keyword) {
if (keyword == null) return;
Context context = Context.from(loopStatement, noConversionToDoWhile);
if (context == null) return;
LocalQuickFix[] fixes;
@@ -73,78 +72,107 @@ public class ConditionalBreakInInfiniteLoopInspection extends AbstractBaseJavaLo
};
}
@Nullable
private static PsiElement getKeyword(@NotNull PsiLoopStatement loopStatement) {
if(loopStatement instanceof PsiWhileStatement || loopStatement instanceof PsiForStatement) {
return loopStatement.getFirstChild();
}
return ((PsiDoWhileStatement)loopStatement).getWhileKeyword();
}
private static class Context {
final @NotNull PsiLoopStatement myLoopStatement;
final @NotNull PsiStatement myLoopBody;
final @NotNull PsiExpression myCondition;
final @NotNull PsiStatement myConditionStatement;
final @NotNull PsiIfStatement myConditionStatement;
final boolean myConditionInTheBeginning;
final boolean myConditionInThen;
Context(@NotNull PsiLoopStatement loopStatement,
@NotNull PsiStatement loopBody,
@NotNull PsiExpression condition,
@NotNull PsiStatement statement,
boolean conditionInTheBeginning) {
@NotNull PsiStatement loopBody,
@NotNull PsiExpression condition,
@NotNull PsiIfStatement statement,
boolean conditionInTheBeginning,
boolean conditionInThen) {
myLoopStatement = loopStatement;
myLoopBody = loopBody;
myCondition = condition;
myConditionStatement = statement;
myConditionInTheBeginning = conditionInTheBeginning;
myConditionInThen = conditionInThen;
}
@Nullable
static Context from(@NotNull PsiConditionalLoopStatement loopStatement, boolean noConversionToDoWhile) {
if (!ControlFlowUtils.isEndlessLoop(loopStatement)) return null;
boolean isEndlessLoop = ControlFlowUtils.isEndlessLoop(loopStatement);
if (!isEndlessLoop) {
if (loopStatement instanceof PsiForStatement) {
PsiForStatement forStatement = (PsiForStatement)loopStatement;
if ((forStatement.getInitialization() != null && !(forStatement.getInitialization() instanceof PsiEmptyStatement))
|| (forStatement.getUpdate() != null && !(forStatement.getUpdate() instanceof PsiEmptyStatement))) {
return null;
}
}
}
PsiStatement body = loopStatement.getBody();
if (body == null) return null;
PsiStatement[] statements = ControlFlowUtils.unwrapBlock(body);
if (statements.length < 2) return null;
if (statements.length < 1) return null;
if (StreamEx.ofTree((PsiElement)body, el -> StreamEx.of(el.getChildren()))
.select(PsiBreakStatement.class)
.filter(stmt -> ControlFlowUtils.statementBreaksLoop(stmt, loopStatement))
.count() != 1) {
return null;
}
PsiStatement first = statements[0];
PsiExpression firstBreakCondition = extractBreakCondition(first, loopStatement);
if (firstBreakCondition != null) {
return new Context(loopStatement, body, firstBreakCondition, first, true);
PsiIfStatement first = tryCast(statements[0], PsiIfStatement.class);
boolean[] isBreakInThen = new boolean[1];
PsiExpression firstBreakCondition = extractBreakCondition(first, loopStatement, isBreakInThen);
PsiExpression loopCondition = loopStatement.getCondition();
boolean isLoopConditionAtStart = !(loopStatement instanceof PsiDoWhileStatement);
if (first != null
&& firstBreakCondition != null
&& (isEndlessLoop || (isLoopConditionAtStart && (BoolUtils.getLogicalOperandCount(loopCondition) + BoolUtils.getLogicalOperandCount(firstBreakCondition)) < 4))) {
return new Context(loopStatement, body, firstBreakCondition, first, true, isBreakInThen[0]);
}
if (noConversionToDoWhile) return null;
PsiStatement last = statements[statements.length - 1];
PsiExpression lastBreakCondition = extractBreakCondition(last, loopStatement);
if (lastBreakCondition != null) {
if (StreamEx.of(statements)
.flatMap(statement -> StreamEx.ofTree((PsiElement)statement, el -> StreamEx.of(el.getChildren())))
.anyMatch(e -> e instanceof PsiContinueStatement &&
((PsiContinueStatement)e).findContinuedStatement() == loopStatement)) {
return null;
}
boolean variablesInLoop = VariableAccessUtils.collectUsedVariables(lastBreakCondition).stream()
.anyMatch(var -> PsiTreeUtil.isAncestor(loopStatement, var, false));
if (variablesInLoop) return null;
return new Context(loopStatement, body, lastBreakCondition, last, false);
PsiIfStatement last = tryCast(statements[statements.length - 1], PsiIfStatement.class);
PsiExpression lastBreakCondition = extractBreakCondition(last, loopStatement, isBreakInThen);
if (lastBreakCondition == null || !isBreakInThen[0]) return null;
if (!isEndlessLoop && (isLoopConditionAtStart || (3 < BoolUtils.getLogicalOperandCount(loopCondition) + BoolUtils.getLogicalOperandCount(lastBreakCondition)))) return null;
if (StreamEx.of(statements)
.flatMap(statement -> StreamEx.ofTree((PsiElement)statement, el -> StreamEx.of(el.getChildren())))
.anyMatch(e -> e instanceof PsiContinueStatement &&
((PsiContinueStatement)e).findContinuedStatement() == loopStatement)) {
return null;
}
boolean variablesInLoop = VariableAccessUtils.collectUsedVariables(lastBreakCondition).stream()
.anyMatch(variable -> PsiTreeUtil.isAncestor(loopStatement, variable, false));
if (last == null || variablesInLoop) return null;
return new Context(loopStatement, body, lastBreakCondition, last, false, isBreakInThen[0]);
}
@Nullable
private static PsiExpression extractBreakCondition(@Nullable PsiIfStatement ifStatement,
@NotNull PsiLoopStatement loopStatement,
@NotNull boolean[] isBreakInThen) {
if (ifStatement == null) return null;
if (ControlFlowUtils.statementBreaksLoop(ControlFlowUtils.stripBraces(ifStatement.getThenBranch()), loopStatement)) {
if (hasVariableNameConflict(loopStatement, ifStatement, ifStatement.getElseBranch())) return null;
isBreakInThen[0] = true;
return ifStatement.getCondition();
}
if (ifStatement.getElseBranch() != null && ControlFlowUtils.statementBreaksLoop(ControlFlowUtils.stripBraces(ifStatement.getElseBranch()), loopStatement)) {
if (hasVariableNameConflict(loopStatement, ifStatement, ifStatement.getThenBranch())) return null;
isBreakInThen[0] = false;
return ifStatement.getCondition();
}
return null;
}
@Nullable
private static PsiExpression extractBreakCondition(@NotNull PsiStatement statement, @NotNull PsiLoopStatement loopStatement) {
PsiIfStatement ifStatement = tryCast(statement, PsiIfStatement.class);
if (ifStatement == null) return null;
if (ifStatement.getElseBranch() != null) return null;
PsiStatement thenBranch = ifStatement.getThenBranch();
if (!ControlFlowUtils.statementBreaksLoop(ControlFlowUtils.stripBraces(thenBranch), loopStatement)) return null;
return ifStatement.getCondition();
private static boolean hasVariableNameConflict(@NotNull PsiLoopStatement loopStatement,
@NotNull PsiIfStatement ifStatement,
@Nullable PsiStatement branch) {
if (branch == null) return false;
Set<String> variablesCreatedInBranch = VariableAccessUtils.findDeclaredVariables(branch).stream()
.map(PsiVariable::getName)
.collect(Collectors.toSet());
Set<String> otherVariablesUsedInLoop = VariableAccessUtils.collectUsedVariables(loopStatement.getBody()).stream()
.filter(variable -> !PsiTreeUtil.isAncestor(ifStatement, variable, false))
.map(PsiVariable::getName)
.collect(Collectors.toSet());
return !Collections.disjoint(variablesCreatedInBranch, otherVariablesUsedInLoop);
}
}
@@ -163,12 +191,34 @@ public class ConditionalBreakInInfiniteLoopInspection extends AbstractBaseJavaLo
Context context = Context.from(loop, false);
if (context == null) return;
CommentTracker ct = new CommentTracker();
String negated = BoolUtils.getNegatedExpressionText(context.myCondition, ct);
ct.delete(context.myConditionStatement);
String loopText = context.myConditionInTheBeginning
? "while(" + negated + ")" + ct.text(context.myLoopBody)
: "do" + ct.text(context.myLoopBody) + "while(" + negated + ");";
String loopText;
if (ControlFlowUtils.isEndlessLoop(loop)) {
String conditionForWhile = context.myConditionInThen ? BoolUtils.getNegatedExpressionText(context.myCondition, ct) : ct.text(context.myCondition);
pullDownStatements(context.myConditionStatement, loop, context.myConditionInThen ? context.myConditionStatement.getElseBranch() : context.myConditionStatement.getThenBranch());
ct.delete(context.myConditionStatement);
loopText = context.myConditionInTheBeginning
? "while(" + conditionForWhile + ")" + ct.text(context.myLoopBody)
: "do" + ct.text(context.myLoopBody) + "while(" + conditionForWhile + ");";
} else {
String conditionForWhile = context.myConditionInThen ? BoolUtils.getNegatedExpressionText(context.myCondition, ParenthesesUtils.AND_PRECEDENCE, ct) : ct.text(context.myCondition, ParenthesesUtils.AND_PRECEDENCE);
ct.delete(context.myConditionStatement);
PsiExpression loopCondition = loop.getCondition();
loopText = context.myConditionInTheBeginning
? "while(" + ct.text(loopCondition, ParenthesesUtils.AND_PRECEDENCE) + " && " + conditionForWhile + ")" + ct.text(context.myLoopBody)
: "do" + ct.text(context.myLoopBody) + "while(" + conditionForWhile + " && " + ct.text(loopCondition, ParenthesesUtils.AND_PRECEDENCE) + ");";
}
ct.replaceAndRestoreComments(context.myLoopStatement, loopText);
}
private void pullDownStatements(@NotNull PsiIfStatement conditionStatement, @NotNull PsiConditionalLoopStatement loop, @Nullable PsiStatement branch) {
if (branch != null) {
PsiElement parent = conditionStatement.getParent();
PsiStatement[] branchStatements = ControlFlowUtils.unwrapBlock(branch);
for (int statementIndex = branchStatements.length - 1; 0 <= statementIndex; statementIndex--) {
PsiStatement statement = branchStatements[statementIndex];
parent.addAfter(statement, conditionStatement);
}
}
}
}
}

View File

@@ -1,6 +1,6 @@
<html>
<body>
Reports conditional breaks at the beginning or at the end of a loop and suggests using a loop condition instead to shorten the code.
Reports conditional breaks at the beginning or at the end of a loop and suggests adding a loop condition instead to shorten the code.
<p>Example:</p>
<pre><code>
<b>while</b> (true) {

View File

@@ -0,0 +1,15 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
int i = 1;
/*1*/
/*2*/
/*3*/
/*7*/
/*8*/
while (i % 10 != 0 && i < 12) {
/*4*/
i =/*5*/ i * 2;/*6*/
}
}
}

View File

@@ -0,0 +1,15 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
int i = 1;
/*1*/
/*2*/
/*3*/
/*7*/
/*8*/
do {
i =/*5*/ i * 2;/*6*/
/*4*/
} while (i < 12 && i % 10 != 0);
}
}

View File

@@ -0,0 +1,15 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
int i = 1;
/*1*/
/*2*/
/*3*/
/*7*/
/*8*/
while (i % 10 != 0 && i < 12) {
/*4*/
i =/*5*/ i * 2;/*6*/
}
}
}

View File

@@ -0,0 +1,9 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
int i = 0;
while (i < 12) {
i++;
}
}
}

View File

@@ -0,0 +1,11 @@
// "Move condition to loop" "true"
class Main {
private int variableWithSameName = 1;
public static void main(String[] args) {
int i = 1;
while (i < 100) {
int variableWithAnotherName = -100;
i = i + variableWithSameName;
}
}
}

View File

@@ -0,0 +1,10 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
int i = 0;
while (i < 14) {
i++;
System.out.println("I'm doing two things in this loop");
}
}
}

View File

@@ -0,0 +1,9 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
int i = 0;
while (i < 13) {
i++;
}
}
}

View File

@@ -0,0 +1,17 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
boolean isValid = true;
boolean isEnabled = true;
int i = 1;
/*1*/
/*2*/
/*3*/
/*7*/
/*8*/
while (i % 10 != 0 && (!isValid/*9*/ ||/*10*/ !isEnabled)) {
/*4*/
i =/*5*/ i * 2;/*6*/
}
}
}

View File

@@ -0,0 +1,17 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
boolean isValid = true;
boolean isEnabled = true;
int i = 1;
/*1*/
/*2*/
/*3*/
/*7*/
/*8*/
while ((isValid || isEnabled) && i < 12) {
/*4*/
i =/*5*/ i * 2;/*6*/
}
}
}

View File

@@ -0,0 +1,9 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
int i = 0;
while (i < 14) {
i++;
}
}
}

View File

@@ -0,0 +1,13 @@
// "Move condition to loop" "false"
class Main {
public static void main(String[] args) {
int i = 1;
do<caret> {
if (i < 100) {
break;
} else {
i = i * 3;
}
} while(true);
}
}

View File

@@ -0,0 +1,15 @@
// "Move condition to loop" "false"
class Main {
public static void main(String[] args) {
int i = 0;
int j = 0;
annoyingLabel: while(j++ < 100) {
while<caret>(true) {
i++;
if(i < 0) {
break annoyingLabel;
}
}
}
}
}

View File

@@ -0,0 +1,11 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
int i = 1;
for<caret>(;i % 10 != 0/*7*/;) /*8*/ {
if(i >= 12/*1*/)/*2*/ break;/*3*/
/*4*/
i =/*5*/ i * 2;/*6*/
}
}
}

View File

@@ -0,0 +1,11 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
int i = 1;
do {
i =/*5*/ i * 2;/*6*/
/*4*/
if(i >= 12/*1*/)/*2*/ break;/*3*/
} while<caret>(i % 10 != 0/*7*/) /*8*/;
}
}

View File

@@ -0,0 +1,11 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
int i = 1;
while<caret>(i % 10 != 0/*7*/) /*8*/ {
if(i >= 12/*1*/)/*2*/ break;/*3*/
/*4*/
i =/*5*/ i * 2;/*6*/
}
}
}

View File

@@ -0,0 +1,10 @@
// "Move condition to loop" "false"
class Main {
public static void main(String[] args) {
int i = 1;
do {
if(i > 100) break;
i = i * 2;
} while<caret>(i % 10 != 0);
}
}

View File

@@ -0,0 +1,13 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
int i = 0;
for<caret>(;;) {
if (i >= 12) {
break;
} else {
i++;
}
}
}
}

View File

@@ -0,0 +1,15 @@
// "Move condition to loop" "true"
class Main {
private int variableWithSameName = 1;
public static void main(String[] args) {
int i = 1;
while<caret>(true) {
if (i >= 100) {
break;
} else {
int variableWithAnotherName = -100;
}
i = i + variableWithSameName;
}
}
}

View File

@@ -0,0 +1,14 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
int i = 0;
while<caret>(true) {
if (i < 14) {
i++;
} else {
break;
}
System.out.println("I'm doing two things in this loop");
}
}
}

View File

@@ -0,0 +1,13 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
int i = 0;
while<caret> (true) {
if (i < 13) {
i++;
} else {
break;
}
}
}
}

View File

@@ -0,0 +1,13 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
boolean isValid = true;
boolean isEnabled = true;
int i = 1;
while<caret>(i % 10 != 0/*7*/) /*8*/ {
if(isValid/*9*/ &&/*10*/ isEnabled/*1*/)/*2*/ break;/*3*/
/*4*/
i =/*5*/ i * 2;/*6*/
}
}
}

View File

@@ -0,0 +1,13 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
boolean isValid = true;
boolean isEnabled = true;
int i = 1;
while<caret>(isValid || isEnabled/*7*/) /*8*/ {
if(i >= 12/*1*/)/*2*/ break;/*3*/
/*4*/
i =/*5*/ i * 2;/*6*/
}
}
}

View File

@@ -0,0 +1,13 @@
// "Move condition to loop" "false"
class Main {
public static void main(String[] args) {
boolean isValid = true;
boolean isEnabled = true;
int i = 1;
while<caret>(i % 10 != 0 && i < 100/*7*/) /*8*/ {
if(isValid/*9*/ ||/*10*/ isEnabled/*1*/)/*2*/ break;/*3*/
/*4*/
i =/*5*/ i * 2;/*6*/
}
}
}

View File

@@ -0,0 +1,12 @@
// "Move condition to loop" "true"
class Main {
public static void main(String[] args) {
int i = 0;
for<caret>(;;) {
if (i >= 14)
break;
else
i++;
}
}
}

View File

@@ -0,0 +1,15 @@
// "Move condition to loop" "false"
class Main {
private int variableWithSameName = 1;
public static void main(String[] args) {
int i = 1;
while<caret>(true) {
if (i < 100) {
break;
} else {
int variableWithSameName = -100;
}
i = i + variableWithSameName;
}
}
}

View File

@@ -0,0 +1,10 @@
// "Move condition to loop" "false"
class Main {
public static void main(String[] args) {
int i = 1;
while<caret>(i % 10 != 0) {
i = i * 2;
if(i > 100) break;
}
}
}

View File

@@ -369,7 +369,7 @@ inspection.comparator.result.comparison.display.name=Suspicious usage of compare
inspection.comparator.result.comparison.fix.family.name=Fix comparator result comparison
inspection.comparator.result.comparison.problem.display.name=Comparison of compare method result with specific constant
inspection.conditional.break.in.infinite.loop=Move condition to loop
inspection.conditional.break.in.infinite.loop.description=Conditional break inside infinite loop
inspection.conditional.break.in.infinite.loop.description=Conditional break inside loop
inspection.conditional.break.in.infinite.loop.no.conversion.with.do.while=Don't suggest to replace with 'do while'
inspection.convert.to.local.quickfix=Convert to local
inspection.data.flow.display.name=Constant conditions \\& exceptions

View File

@@ -30,11 +30,13 @@ import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.function.Predicate;
import static com.intellij.codeInspection.util.OptionalUtil.*;
import static com.intellij.psi.CommonClassNames.JAVA_UTIL_OPTIONAL;
import static com.intellij.psi.JavaTokenType.*;
public final class BoolUtils {
@@ -84,6 +86,32 @@ public final class BoolUtils {
return getNegatedExpressionText(condition, ParenthesesUtils.NUM_PRECEDENCES, tracker);
}
/**
* Returns the number of logical operands in the expression.
*
* @param condition The expression
* @return the number of logical operands in the expression
*/
public static int getLogicalOperandCount(@Nullable PsiExpression condition) {
PsiExpression unparenthesizedExpression = PsiUtil.skipParenthesizedExprDown(condition);
if (!(unparenthesizedExpression instanceof PsiPolyadicExpression)) {
return 1;
}
PsiPolyadicExpression infixExpression= (PsiPolyadicExpression) unparenthesizedExpression;
if (!ANDAND.equals(infixExpression.getOperationTokenType())
&& !OROR.equals(infixExpression.getOperationTokenType())
&& (PsiType.BOOLEAN.equals(infixExpression.getOperands()[0].getType())
|| PsiType.BOOLEAN.equals(PsiPrimitiveType.getUnboxedType(infixExpression.getOperands()[0].getType()))
|| !Arrays.asList(AND, OR).contains(infixExpression.getOperationTokenType()))) {
return 1;
}
int nbOperands= 0;
for (PsiExpression operand : infixExpression.getOperands()) {
nbOperands+= getLogicalOperandCount(operand);
}
return nbOperands;
}
private static final CallMatcher STREAM_ANY_MATCH = CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_STREAM_STREAM, "anyMatch");
private static final CallMatcher STREAM_NONE_MATCH = CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_STREAM_STREAM, "noneMatch");