Idempotent loop body inspection

Fixes IDEA-180227 Better infinite loop detection

java/java-impl/src/com/intellij/codeInspection/IdempotentLoopBodyInspection.java

@@ -0,0 +1,172 @@
+// 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.psi.*;
+import com.intellij.psi.util.PsiUtil;
+import com.siyeh.ig.psiutils.SideEffectChecker;
+import com.siyeh.ig.psiutils.VariableAccessUtils;
+import one.util.streamex.StreamEx;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.function.Function;
+
+import static com.intellij.util.ObjectUtils.tryCast;
+
+public class IdempotentLoopBodyInspection extends AbstractBaseJavaLocalInspectionTool {
+  @NotNull
+  @Override
+  public PsiElementVisitor buildVisitor(@NotNull ProblemsHolder holder, boolean isOnTheFly) {
+    return new JavaElementVisitor() {
+      @Override
+      public void visitWhileStatement(PsiWhileStatement loop) {
+        PsiExpression condition = loop.getCondition();
+        if (condition == null || SideEffectChecker.mayHaveSideEffects(condition)) return;
+        if (isIdempotent(loop.getBody())) {
+          holder.registerProblem(loop.getFirstChild(), InspectionsBundle.message("inspection.idempotent.loop.body"));
+        }
+      }
+
+      @Override
+      public void visitForStatement(PsiForStatement loop) {
+        PsiExpression condition = loop.getCondition();
+        if (condition == null || SideEffectChecker.mayHaveSideEffects(condition)) return;
+        if (isIdempotent(loop.getBody(), loop.getUpdate())) {
+          holder.registerProblem(loop.getFirstChild(), InspectionsBundle.message("inspection.idempotent.loop.body"));
+        }
+      }
+
+      private boolean isIdempotent(PsiStatement... statements) {
+        Set<PsiVariable> variables = extractWrites(statements);
+        if (variables == null || variables.isEmpty()) return false;
+        if (!(variables instanceof HashSet)) {
+          variables = new HashSet<>(variables);
+        }
+        for (PsiStatement statement : statements) {
+          if (usesInputVariable(statement, variables)) {
+            return false;
+          }
+        }
+        return true;
+      }
+
+      private boolean usesInputVariable(PsiStatement statement, Set<PsiVariable> variables) {
+        if (statement == null) return false;
+        if (statement instanceof PsiBlockStatement) {
+          for (PsiStatement st : ((PsiBlockStatement)statement).getCodeBlock().getStatements()) {
+            if (usesInputVariable(st, variables)) {
+              return true;
+            }
+          }
+          return false;
+        }
+        if (statement instanceof PsiExpressionStatement) {
+          PsiAssignmentExpression assignment = tryCast(((PsiExpressionStatement)statement).getExpression(), PsiAssignmentExpression.class);
+          if (assignment != null) {
+            if (anyVariableIsUsed(assignment.getRExpression(), variables)) return true;
+            PsiReferenceExpression ref =
+              tryCast(PsiUtil.skipParenthesizedExprDown(assignment.getLExpression()), PsiReferenceExpression.class);
+            if (ref != null) {
+              PsiElement var = ref.resolve();
+              if (var instanceof PsiVariable) {
+                variables.remove(var);
+              }
+            }
+            return false;
+          }
+        }
+        if (statement instanceof PsiIfStatement) {
+          PsiIfStatement ifStatement = (PsiIfStatement)statement;
+          if (anyVariableIsUsed(ifStatement.getCondition(), variables)) return true;
+          Set<PsiVariable> thenVars = new HashSet<>(variables);
+          if (usesInputVariable(ifStatement.getThenBranch(), thenVars)) return true;
+          Set<PsiVariable> elseVars = new HashSet<>(variables);
+          if (usesInputVariable(ifStatement.getElseBranch(), elseVars)) return true;
+          thenVars.addAll(elseVars);
+          variables.retainAll(thenVars);
+          return false;
+        }
+        if (statement instanceof PsiDeclarationStatement) {
+          StreamEx.of(((PsiDeclarationStatement)statement).getDeclaredElements()).select(PsiVariable.class)
+            .forEach(variables::remove);
+        }
+        return anyVariableIsUsed(statement, variables);
+      }
+
+      private boolean anyVariableIsUsed(@Nullable PsiElement statement, @NotNull Set<PsiVariable> variables) {
+        return VariableAccessUtils.collectUsedVariables(statement).stream().anyMatch(variables::contains);
+      }
+
+      /**
+       * Extract written variables from statement which may affect the next iteration
+       * @param statement
+       * @return list of written variables or null if the statement may have unknown side effects, thus further analysis is impossible.
+       */
+      @Nullable
+      private Set<PsiVariable> extractWrites(@Nullable PsiStatement statement) {
+        if (statement == null ||
+            statement instanceof PsiEmptyStatement ||
+            (statement instanceof PsiContinueStatement && ((PsiContinueStatement)statement).getLabelIdentifier() == null)) {
+          return Collections.emptySet();
+        }
+        if (statement instanceof PsiBlockStatement) {
+          PsiStatement[] statements = ((PsiBlockStatement)statement).getCodeBlock().getStatements();
+          return extractWrites(statements);
+        }
+        if (statement instanceof PsiDeclarationStatement) {
+          PsiElement[] elements = ((PsiDeclarationStatement)statement).getDeclaredElements();
+          for (PsiElement element : elements) {
+            if (!(element instanceof PsiLocalVariable)) return null;
+            PsiLocalVariable var = (PsiLocalVariable)element;
+            PsiExpression initializer = var.getInitializer();
+            if (initializer != null && SideEffectChecker.mayHaveSideEffects(initializer)) return null;
+          }
+          return Collections.emptySet();
+        }
+        if (statement instanceof PsiExpressionStatement) {
+          PsiAssignmentExpression assignment = tryCast(((PsiExpressionStatement)statement).getExpression(), PsiAssignmentExpression.class);
+          if (assignment == null ||
+              assignment.getOperationTokenType() != JavaTokenType.EQ ||
+              assignment.getRExpression() == null ||
+              SideEffectChecker.mayHaveSideEffects(assignment.getRExpression())) {
+            return null;
+          }
+          PsiReferenceExpression ref =
+            tryCast(PsiUtil.skipParenthesizedExprDown(assignment.getLExpression()), PsiReferenceExpression.class);
+          if (ref == null) return null;
+          PsiElement var = ref.resolve();
+          if (var instanceof PsiLocalVariable || var instanceof PsiParameter) {
+            return Collections.singleton((PsiVariable)var);
+          }
+        }
+        if (statement instanceof PsiIfStatement) {
+          PsiIfStatement ifStatement = (PsiIfStatement)statement;
+          PsiExpression condition = ifStatement.getCondition();
+          if (condition == null || SideEffectChecker.mayHaveSideEffects(condition)) return null;
+          Set<PsiVariable> thenResult = extractWrites(ifStatement.getThenBranch());
+          if (thenResult == null) return null;
+          Set<PsiVariable> elseResult = extractWrites(ifStatement.getElseBranch());
+          if (elseResult == null) return null;
+          if (thenResult.isEmpty()) return elseResult;
+          if (elseResult.isEmpty()) return thenResult;
+          return StreamEx.of(thenResult, elseResult).toFlatCollection(Function.identity(), HashSet::new);
+        }
+        return null;
+      }
+
+      @Nullable
+      private Set<PsiVariable> extractWrites(PsiStatement... statements) {
+        Set<PsiVariable> result = new HashSet<>();
+        for (PsiStatement subStatement : statements) {
+          Set<PsiVariable> subResult = extractWrites(subStatement);
+          if (subResult == null) return null;
+          result.addAll(subResult);
+        }
+        return result;
+      }
+    };
+  }
+}

java/java-tests/testData/inspection/idempotentLoopBody/IdempotentLoopBody.java

@@ -0,0 +1,57 @@
+import java.util.*;
+
+class IdempotentLoopBody {
+  String getUniqueName(String baseName, Set<String> names) {
+    int index = 1;
+    String name = baseName;
+    <warning descr="Idempotent loop body">while</warning>(names.contains(name)) {
+      name = baseName + index;
+    }
+    return name;
+  }
+
+  String getUniqueNameComplex(String baseName, Set<String> names) {
+    int index = 1;
+    String name = baseName;
+    <warning descr="Idempotent loop body">while</warning> (names.contains(name)) {
+      String suffix = String.valueOf(index);
+      if (suffix.length() == 1) {
+        suffix = "0" + suffix;
+      }
+      name = baseName + suffix;
+    }
+    return name;
+  }
+
+  String getUniqueNameCorrect(String baseName, Set<String> names) {
+    int index = 1;
+    String name = baseName;
+    while(names.contains(name)) {
+      name = baseName + (index++);
+    }
+    return name;
+  }
+
+  String getUniqueNameFor(String baseName, Set<String> names) {
+    int index = 1;
+    String name;
+    <warning descr="Idempotent loop body">for</warning>(name = baseName; names.contains(name); name = baseName + index);
+    return name;
+  }
+
+  String leftPad(String val, int desiredLength) {
+    String result = val;
+    <warning descr="Idempotent loop body">while</warning>(result.length() < desiredLength) {
+      result = " " + val;
+    }
+    return result;
+  }
+
+  String leftPadCorrect(String val, int desiredLength) {
+    String result = val;
+    while(result.length() < desiredLength) {
+      result = " " + result;
+    }
+    return result;
+  }
+}

java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/quickFix/IdempotentLoopBodyInspectionTest.java

@@ -0,0 +1,21 @@
+// 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.java.codeInsight.daemon.quickFix;
+
+import com.intellij.JavaTestUtil;
+import com.intellij.codeInspection.IdempotentLoopBodyInspection;
+import com.intellij.testFramework.fixtures.LightCodeInsightFixtureTestCase;
+
+
+public class IdempotentLoopBodyInspectionTest extends LightCodeInsightFixtureTestCase {
+  public void testIdempotentLoopBody() { doTest(); }
+
+  private void doTest() {
+    myFixture.enableInspections(new IdempotentLoopBodyInspection());
+    myFixture.testHighlighting(getTestName(false) + ".java");
+  }
+
+  @Override
+  protected String getBasePath() {
+    return JavaTestUtil.getRelativeJavaTestDataPath()+"/inspection/idempotentLoopBody";
+  }
+}

platform/platform-resources-en/src/messages/InspectionsBundle.properties

@@ -902,4 +902,6 @@ inspection.fuse.stream.operations.message=Stream may be extended replacing {0}
 inspection.fuse.stream.operations.display.name=Subsequent steps can be fused into Stream API chain
 inspection.overwritten.key.set.message=Duplicating Set element
 inspection.overwritten.key.map.message=Duplicating Map key
-navigate.to.duplicate.fix=Navigate to duplicate
+navigate.to.duplicate.fix=Navigate to duplicate
+
+inspection.idempotent.loop.body=Idempotent loop body

resources-en/src/inspectionDescriptions/IdempotentLoopBody.html

@@ -0,0 +1,17 @@
+<html>
+<body>
+Detects loops which second and following iterations do not produce any additional side effects other than produced by the first iteration,
+which could indicate a programming error. Such loops may iterate only zero, one or infinite number of times.
+If infinite number of times case is unreachable, such loop could be replaced with <b>if</b> statement. Otherwise there's a danger that
+the program could stuck. Example:
+<pre>
+  int suffix = 1;
+  String name = baseName;
+  while(names.contains(name)) {
+    name = baseName + suffix; // error: suffix is not updated making loop body idempotent
+  }
+</pre>
+<!-- tooltip end -->
+<small>New in 2018.1</small>
+</body>
+</html>

resources/src/META-INF/IdeaPlugin.xml

@@ -1014,6 +1014,10 @@
                      groupPath="Java" groupBundle="messages.InspectionsBundle" groupKey="group.names.code.style.issues"
                      displayName="Field assignment can be moved to initializer"
                      implementationClass="com.intellij.codeInspection.MoveFieldAssignmentToInitializerInspection"/>
+    <localInspection language="JAVA" shortName="IdempotentLoopBody" enabledByDefault="true" level="WARNING"
+                     groupPath="Java" groupBundle="messages.InspectionsBundle" groupKey="group.names.control.flow.issues"
+                     bundle="messages.InspectionsBundle" key="inspection.idempotent.loop.body"
+                     implementationClass="com.intellij.codeInspection.IdempotentLoopBodyInspection"/>
     <localInspection language="JAVA" shortName="JavaModuleNaming" enabledByDefault="true" level="WARNING"
                      groupPath="Java" groupBundle="messages.InspectionsBundle" groupKey="group.names.naming.conventions"
                      bundle="messages.InspectionsBundle" key="inspection.java.module.naming"