diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/performance/StringConcatenationInLoopsInspection.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/performance/StringConcatenationInLoopsInspection.java index 0bea982ec3a0..57ad12b078fd 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/performance/StringConcatenationInLoopsInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/performance/StringConcatenationInLoopsInspection.java @@ -113,7 +113,7 @@ public class StringConcatenationInLoopsInspection extends BaseInspection { registerError(sign, getAppendedVariable(expression)); } - private boolean checkExpression(PsiExpression expression) { + private static boolean checkExpression(PsiExpression expression) { if (!TypeUtils.isJavaLangString(expression.getType()) || ControlFlowUtils.isInExitStatement(expression) || !ControlFlowUtils.isInLoop(expression)) return false; @@ -133,14 +133,34 @@ public class StringConcatenationInLoopsInspection extends BaseInspection { if (variable != null) { PsiLoopStatement commonLoop = getOutermostCommonLoop(expression, variable); - return commonLoop != null && !ControlFlowUtils - .flowBreaksLoop(PsiTreeUtil.getParentOfType(expression, PsiStatement.class), commonLoop); + return commonLoop != null && + !ControlFlowUtils.isExecutedOnceInLoop(PsiTreeUtil.getParentOfType(expression, PsiStatement.class), commonLoop) && + !isUsedCompletely(variable, commonLoop); } } return false; } - private PsiLoopStatement getOutermostCommonLoop(PsiExpression expression, PsiVariable variable) { + private static boolean isUsedCompletely(PsiVariable variable, PsiLoopStatement loop) { + boolean notUsedCompletely = ReferencesSearch.search(variable, new LocalSearchScope(loop)).forEach(ref -> { + PsiExpression expression = ObjectUtils.tryCast(ref.getElement(), PsiExpression.class); + if (expression == null) return true; + PsiElement parent = PsiUtil.skipParenthesizedExprUp(expression.getParent()); + while (parent instanceof PsiTypeCastExpression || parent instanceof PsiConditionalExpression) { + parent = PsiUtil.skipParenthesizedExprUp(expression.getParent()); + } + if (parent instanceof PsiExpressionList || + (parent instanceof PsiAssignmentExpression && + PsiTreeUtil.isAncestor(((PsiAssignmentExpression)parent).getRExpression(), expression, false))) { + PsiStatement statement = PsiTreeUtil.getParentOfType(parent, PsiStatement.class); + return ControlFlowUtils.isExecutedOnceInLoop(statement, loop) || ControlFlowUtils.isVariableReassigned(statement, variable); + } + return true; + }); + return !notUsedCompletely; + } + + private static PsiLoopStatement getOutermostCommonLoop(PsiExpression expression, PsiVariable variable) { PsiElement stopAt = null; PsiCodeBlock block = getSurroundingBlock(expression); if(block != null) { diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ControlFlowUtils.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ControlFlowUtils.java index 26fc116ab58d..3b16c7056785 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ControlFlowUtils.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ControlFlowUtils.java @@ -16,7 +16,11 @@ package com.siyeh.ig.psiutils; import com.intellij.psi.*; +import com.intellij.psi.search.searches.ReferencesSearch; import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.psi.util.PsiUtil; +import com.intellij.util.ObjectUtils; +import one.util.streamex.StreamEx; import org.jetbrains.annotations.Contract; import org.jetbrains.annotations.NonNls; import org.jetbrains.annotations.NotNull; @@ -600,6 +604,67 @@ public class ControlFlowUtils { return false; } + private static StreamEx conditions(PsiElement element) { + return StreamEx.iterate(element, e -> e != null && + !(e instanceof PsiLambdaExpression) && !(e instanceof PsiMethod), PsiElement::getParent) + .pairMap((child, parent) -> parent instanceof PsiIfStatement && ((PsiIfStatement)parent).getThenBranch() == child ? parent : null) + .select(PsiIfStatement.class) + .map(PsiIfStatement::getCondition) + .flatMap(cond -> cond instanceof PsiPolyadicExpression && ((PsiPolyadicExpression)cond).getOperationTokenType().equals( + JavaTokenType.ANDAND) ? StreamEx.of(((PsiPolyadicExpression)cond).getOperands()) : StreamEx.of(cond)); + } + + /** + * @param statement statement to check + * @param loop surrounding loop + * @return true if it could be statically determined that given statement is executed at most once + */ + public static boolean isExecutedOnceInLoop(PsiStatement statement, PsiLoopStatement loop) { + if (flowBreaksLoop(statement, loop)) return true; + if (loop instanceof PsiForStatement) { + // Check that we're inside counted loop which increments some loop variable and + // the code is executed under condition like if(var == something) + PsiDeclarationStatement initialization = + ObjectUtils.tryCast(((PsiForStatement)loop).getInitialization(), PsiDeclarationStatement.class); + PsiStatement update = ((PsiForStatement)loop).getUpdate(); + if (initialization != null && update != null) { + PsiLocalVariable variable = StreamEx.of(initialization.getDeclaredElements()).select(PsiLocalVariable.class) + .findFirst(var -> VariableAccessUtils.variableIsIncremented(var, update) || + VariableAccessUtils.variableIsDecremented(var, update)).orElse(null); + if (variable != null) { + boolean hasLoopVarCheck = conditions(statement).select(PsiBinaryExpression.class) + .filter(binOp -> binOp.getOperationTokenType().equals(JavaTokenType.EQEQ)) + .anyMatch(binOp -> ExpressionUtils.getOtherOperand(binOp, variable) != null); + if (hasLoopVarCheck) { + boolean notWritten = ReferencesSearch.search(variable).forEach(ref -> { + PsiExpression expression = ObjectUtils.tryCast(ref.getElement(), PsiExpression.class); + return expression == null || PsiTreeUtil.isAncestor(update, expression, false) || !PsiUtil.isAccessedForWriting(expression); + }); + if (notWritten) return true; + } + } + } + } + return false; + } + + /** + * Returns true if the variable is definitely reassigned to fresh value after executing given statement + * without intermediate usages (ignoring possible exceptions in-between) + * + * @param statement statement to start checking from + * @param variable variable to check + * @return true if variable is reassigned + */ + public static boolean isVariableReassigned(PsiStatement statement, PsiVariable variable) { + for (PsiStatement sibling = nextExecutedStatement(statement); sibling != null; sibling = nextExecutedStatement(sibling)) { + PsiExpression rValue = ExpressionUtils.getAssignmentTo(sibling, variable); + if (rValue != null && !VariableAccessUtils.variableIsUsed(variable, rValue)) return true; + if (VariableAccessUtils.variableIsUsed(variable, sibling)) return false; + } + return false; + } + /** * Checks whether control flow after executing given statement will definitely not go into the next iteration of given loop. * @@ -610,27 +675,41 @@ public class ControlFlowUtils { @Contract("null, _ -> false") public static boolean flowBreaksLoop(PsiStatement statement, PsiLoopStatement loop) { if(statement == null || statement == loop) return false; - for (PsiStatement sibling = nextExecutedStatement(statement); sibling != null; sibling = nextExecutedStatement(sibling)) { + for (PsiStatement sibling = statement; sibling != null; sibling = nextExecutedStatement(sibling)) { if(sibling instanceof PsiContinueStatement) return false; if(sibling instanceof PsiThrowStatement || sibling instanceof PsiReturnStatement) return true; if(sibling instanceof PsiBreakStatement) { PsiBreakStatement breakStatement = (PsiBreakStatement)sibling; PsiStatement exitedStatement = breakStatement.findExitedStatement(); if(exitedStatement == loop) return true; - return flowBreaksLoop(exitedStatement, loop); + return flowBreaksLoop(nextExecutedStatement(exitedStatement), loop); + } + if (sibling instanceof PsiIfStatement || sibling instanceof PsiSwitchStatement) { + if (!PsiTreeUtil.collectElementsOfType(sibling, PsiContinueStatement.class).isEmpty()) return false; + } + if (sibling instanceof PsiLoopStatement) { + if (PsiTreeUtil.collectElements(sibling, e -> e instanceof PsiContinueStatement && + ((PsiContinueStatement)e).getLabelIdentifier() != null).length > 0) { + return false; + } } } return false; } @Nullable - private static PsiStatement nextExecutedStatement(PsiStatement statement) { - PsiStatement next = PsiTreeUtil.getNextSiblingOfType(statement, PsiStatement.class); - while (next instanceof PsiBlockStatement) { - PsiStatement[] statements = ((PsiBlockStatement)next).getCodeBlock().getStatements(); + private static PsiStatement firstStatement(@Nullable PsiStatement statement) { + while (statement instanceof PsiBlockStatement) { + PsiStatement[] statements = ((PsiBlockStatement)statement).getCodeBlock().getStatements(); if (statements.length == 0) break; - next = statements[0]; + statement = statements[0]; } + return statement; + } + + @Nullable + private static PsiStatement nextExecutedStatement(PsiStatement statement) { + PsiStatement next = firstStatement(PsiTreeUtil.getNextSiblingOfType(statement, PsiStatement.class)); if (next == null) { PsiElement parent = statement.getParent(); if (parent instanceof PsiCodeBlock) { diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ExpressionUtils.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ExpressionUtils.java index cece226d29dc..ebb23718804e 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ExpressionUtils.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ExpressionUtils.java @@ -839,6 +839,22 @@ public class ExpressionUtils { return true; } + /** + * If any operand of supplied binary expression refers to the supplied variable, returns other operand; + * otherwise returns null. + * + * @param binOp {@link PsiBinaryExpression} to extract the operand from + * @param variable variable to check against + * @return operand or null + */ + @Contract("null, _ -> null; !null, null -> null") + public static PsiExpression getOtherOperand(@Nullable PsiBinaryExpression binOp, @Nullable PsiVariable variable) { + if(binOp == null || variable == null) return null; + if(isReferenceTo(binOp.getLOperand(), variable)) return binOp.getROperand(); + if(isReferenceTo(binOp.getROperand(), variable)) return binOp.getLOperand(); + return null; + } + @Contract("null, _ -> false; _, null -> false") public static boolean isReferenceTo(PsiExpression expression, PsiVariable variable) { expression = PsiUtil.skipParenthesizedExprDown(expression); diff --git a/plugins/InspectionGadgets/test/com/siyeh/igtest/performance/string_concatenation_in_loops/StringConcatenationInLoop.java b/plugins/InspectionGadgets/test/com/siyeh/igtest/performance/string_concatenation_in_loops/StringConcatenationInLoop.java index 6137cb3d14db..f3e479a5aee7 100644 --- a/plugins/InspectionGadgets/test/com/siyeh/igtest/performance/string_concatenation_in_loops/StringConcatenationInLoop.java +++ b/plugins/InspectionGadgets/test/com/siyeh/igtest/performance/string_concatenation_in_loops/StringConcatenationInLoop.java @@ -79,7 +79,7 @@ public class StringConcatenationInLoop s += i; { { - System.out.println(s); + System.out.println(i); } break; } @@ -89,9 +89,36 @@ public class StringConcatenationInLoop if(i > 2) { s += i; { - System.out.println(s); + System.out.println(i); } - System.out.println(s); + System.out.println(i); + } + } + for (int i = 0; i < 10; i++) { + if (i == 5) { + // concatenated only on single iteration + s += i; + } + System.out.println(i); + } + List strings = new ArrayList<>(); + for (int i = 0; i < 10; i++) { + s += i; + if (s.length() > 0) { + strings.add(s); // the whole string is used on every iteration anyways: it's useless to create a StringBuilder here + } + } + for (int i = 0; i < 10; i++) { + s += i; + if (i == 5 && s.length() > 0) { + strings.add(s); // the whole string is used, but only once: it's still reasonable to migrate to StringBuilder + } + } + for (int i = 0; i < 10; i++) { + s += i; + if (s.length() > 50) { + strings.add(s); // the whole string is used, but it's recreated after the usage: it's still reasonable to migrate to StringBuilder + s = ""; } } for (int i = 0; i < 10; i++) {