IDEA-167452 Reduce false-positives in "string concatenation in loop" inspection

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) {

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<PsiExpression> 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) {

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);

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 <warning descr="String concatenation '+=' in loop">+=</warning> 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<String> 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 <warning descr="String concatenation '+=' in loop">+=</warning> 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 <warning descr="String concatenation '+=' in loop">+=</warning> 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++) {