From 326b6c5ba0fb3f15e7d1dac000fb5d26232e4b7a Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 13 Oct 2017 12:53:40 +0700 Subject: [PATCH] Idempotent loop body inspection Fixes IDEA-180227 Better infinite loop detection --- .../IdempotentLoopBodyInspection.java | 172 ++++++++++++++++++ .../IdempotentLoopBody.java | 57 ++++++ .../IdempotentLoopBodyInspectionTest.java | 21 +++ .../src/messages/InspectionsBundle.properties | 4 +- .../IdempotentLoopBody.html | 17 ++ resources/src/META-INF/IdeaPlugin.xml | 4 + 6 files changed, 274 insertions(+), 1 deletion(-) create mode 100644 java/java-impl/src/com/intellij/codeInspection/IdempotentLoopBodyInspection.java create mode 100644 java/java-tests/testData/inspection/idempotentLoopBody/IdempotentLoopBody.java create mode 100644 java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/quickFix/IdempotentLoopBodyInspectionTest.java create mode 100644 resources-en/src/inspectionDescriptions/IdempotentLoopBody.html diff --git a/java/java-impl/src/com/intellij/codeInspection/IdempotentLoopBodyInspection.java b/java/java-impl/src/com/intellij/codeInspection/IdempotentLoopBodyInspection.java new file mode 100644 index 000000000000..756426b34868 --- /dev/null +++ b/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 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 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 thenVars = new HashSet<>(variables); + if (usesInputVariable(ifStatement.getThenBranch(), thenVars)) return true; + Set 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 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 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 thenResult = extractWrites(ifStatement.getThenBranch()); + if (thenResult == null) return null; + Set 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 extractWrites(PsiStatement... statements) { + Set result = new HashSet<>(); + for (PsiStatement subStatement : statements) { + Set subResult = extractWrites(subStatement); + if (subResult == null) return null; + result.addAll(subResult); + } + return result; + } + }; + } +} diff --git a/java/java-tests/testData/inspection/idempotentLoopBody/IdempotentLoopBody.java b/java/java-tests/testData/inspection/idempotentLoopBody/IdempotentLoopBody.java new file mode 100644 index 000000000000..cf5553a100bb --- /dev/null +++ b/java/java-tests/testData/inspection/idempotentLoopBody/IdempotentLoopBody.java @@ -0,0 +1,57 @@ +import java.util.*; + +class IdempotentLoopBody { + String getUniqueName(String baseName, Set names) { + int index = 1; + String name = baseName; + while(names.contains(name)) { + name = baseName + index; + } + return name; + } + + String getUniqueNameComplex(String baseName, Set names) { + int index = 1; + String name = baseName; + while (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 names) { + int index = 1; + String name = baseName; + while(names.contains(name)) { + name = baseName + (index++); + } + return name; + } + + String getUniqueNameFor(String baseName, Set names) { + int index = 1; + String name; + for(name = baseName; names.contains(name); name = baseName + index); + return name; + } + + String leftPad(String val, int desiredLength) { + String result = val; + while(result.length() < desiredLength) { + result = " " + val; + } + return result; + } + + String leftPadCorrect(String val, int desiredLength) { + String result = val; + while(result.length() < desiredLength) { + result = " " + result; + } + return result; + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/quickFix/IdempotentLoopBodyInspectionTest.java b/java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/quickFix/IdempotentLoopBodyInspectionTest.java new file mode 100644 index 000000000000..a1d1b5dee8de --- /dev/null +++ b/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"; + } +} \ No newline at end of file diff --git a/platform/platform-resources-en/src/messages/InspectionsBundle.properties b/platform/platform-resources-en/src/messages/InspectionsBundle.properties index f569088a848d..c6a1efe3f3b8 100644 --- a/platform/platform-resources-en/src/messages/InspectionsBundle.properties +++ b/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 \ No newline at end of file +navigate.to.duplicate.fix=Navigate to duplicate + +inspection.idempotent.loop.body=Idempotent loop body \ No newline at end of file diff --git a/resources-en/src/inspectionDescriptions/IdempotentLoopBody.html b/resources-en/src/inspectionDescriptions/IdempotentLoopBody.html new file mode 100644 index 000000000000..68c62c3bd125 --- /dev/null +++ b/resources-en/src/inspectionDescriptions/IdempotentLoopBody.html @@ -0,0 +1,17 @@ + + +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 if statement. Otherwise there's a danger that +the program could stuck. Example: +
+  int suffix = 1;
+  String name = baseName;
+  while(names.contains(name)) {
+    name = baseName + suffix; // error: suffix is not updated making loop body idempotent
+  }
+
+ +New in 2018.1 + + \ No newline at end of file diff --git a/resources/src/META-INF/IdeaPlugin.xml b/resources/src/META-INF/IdeaPlugin.xml index ef1066f8b4f4..15bc4fa0ccad 100644 --- a/resources/src/META-INF/IdeaPlugin.xml +++ b/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"/> +