From afd1d1b470da2e209b8efd8d9b4a33b7c4992651 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Wed, 5 Oct 2016 15:23:51 +0700 Subject: [PATCH] IDEA-161259 follow-up: support scenarios when iterator.next() call is inlined into condition --- .../Java8CollectionRemoveIfInspection.java | 120 +++++++++++++++--- .../afterIteratorRemoveInline.java | 9 ++ .../beforeIteratorRemoveInline.java | 13 ++ ...eforeIteratorRemoveInlineShortCircuit.java | 13 ++ .../beforeIteratorRemoveInlineTwice.java | 13 ++ 5 files changed, 148 insertions(+), 20 deletions(-) create mode 100644 java/java-tests/testData/inspection/java8CollectionRemoveIf/afterIteratorRemoveInline.java create mode 100644 java/java-tests/testData/inspection/java8CollectionRemoveIf/beforeIteratorRemoveInline.java create mode 100644 java/java-tests/testData/inspection/java8CollectionRemoveIf/beforeIteratorRemoveInlineShortCircuit.java create mode 100644 java/java-tests/testData/inspection/java8CollectionRemoveIf/beforeIteratorRemoveInlineTwice.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/java18api/Java8CollectionRemoveIfInspection.java b/java/java-analysis-impl/src/com/intellij/codeInspection/java18api/Java8CollectionRemoveIfInspection.java index 64dcd17059b8..1d26b61ff4c0 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/java18api/Java8CollectionRemoveIfInspection.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/java18api/Java8CollectionRemoveIfInspection.java @@ -23,18 +23,26 @@ import com.intellij.openapi.project.Project; import com.intellij.openapi.util.TextRange; import com.intellij.psi.*; import com.intellij.psi.codeStyle.CodeStyleManager; +import com.intellij.psi.codeStyle.JavaCodeStyleManager; +import com.intellij.psi.codeStyle.SuggestedNameInfo; +import com.intellij.psi.codeStyle.VariableKind; +import com.intellij.psi.controlFlow.DefUseUtil; import com.intellij.psi.search.searches.ReferencesSearch; +import com.intellij.psi.tree.IElementType; import com.intellij.psi.util.InheritanceUtil; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.psi.util.PsiUtil; import com.intellij.util.containers.ContainerUtil; import com.siyeh.ig.psiutils.ControlFlowUtils; +import one.util.streamex.MoreCollectors; +import one.util.streamex.StreamEx; import org.jetbrains.annotations.Contract; import org.jetbrains.annotations.Nls; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import java.util.Collection; +import java.util.Optional; /** * @author Tagir Valeev @@ -52,22 +60,62 @@ public class Java8CollectionRemoveIfInspection extends BaseJavaBatchLocalInspect PsiStatement body = statement.getBody(); if(!(body instanceof PsiBlockStatement)) return; PsiStatement[] statements = ((PsiBlockStatement)body).getCodeBlock().getStatements(); - if(statements.length != 2 || !(statements[1] instanceof PsiIfStatement)) return; - PsiVariable element = declaration.getNextElementVariable(statements[0]); - if(element == null) return; - PsiIfStatement ifStatement = (PsiIfStatement)statements[1]; - PsiExpression condition = ifStatement.getCondition(); - if(condition == null || ifStatement.getElseBranch() != null) return; - PsiStatement thenStatement = ControlFlowUtils.stripBraces(ifStatement.getThenBranch()); - if(!(thenStatement instanceof PsiExpressionStatement)) return; - if(!declaration.isIteratorMethodCall(((PsiExpressionStatement)thenStatement).getExpression(), "remove")) return; - if(!LambdaGenerationUtil.canBeUncheckedLambda(condition)) return; + if (statements.length == 2 && statements[1] instanceof PsiIfStatement) { + PsiVariable element = declaration.getNextElementVariable(statements[0]); + if (element == null) return; + PsiIfStatement ifStatement = (PsiIfStatement)statements[1]; + if(checkAndExtractCondition(declaration, ifStatement) == null) return; + registerProblem(statement, endToken); + } + else if (statements.length == 1 && statements[0] instanceof PsiIfStatement){ + PsiIfStatement ifStatement = (PsiIfStatement)statements[0]; + PsiExpression condition = checkAndExtractCondition(declaration, ifStatement); + if (condition == null) return; + declaration.iteratorRefs(condition).collect(MoreCollectors.onlyOne()).ifPresent(ref -> { + if(declaration.isIteratorMethodCall(ref.getParent().getParent(), "next") && isAlwaysExecuted(condition, ref)) { + registerProblem(statement, endToken); + } + }); + } + } + + private boolean isAlwaysExecuted(PsiExpression condition, PsiElement ref) { + while(ref != condition) { + PsiElement parent = ref.getParent(); + if(parent instanceof PsiPolyadicExpression) { + PsiPolyadicExpression polyadicExpression = (PsiPolyadicExpression)parent; + IElementType type = polyadicExpression.getOperationTokenType(); + if ((type.equals(JavaTokenType.ANDAND) || type.equals(JavaTokenType.OROR)) && polyadicExpression.getOperands()[0] != ref) { + return false; + } + } + if(parent instanceof PsiConditionalExpression && ((PsiConditionalExpression)parent).getCondition() != ref) { + return false; + } + ref = parent; + } + return true; + } + + private void registerProblem(PsiLoopStatement statement, PsiJavaToken endToken) { //noinspection DialogTitleCapitalization holder.registerProblem(statement, new TextRange(0, endToken.getTextOffset() - statement.getTextOffset() + 1), QuickFixBundle.message("java.8.collection.removeif.inspection.description"), new ReplaceWithRemoveIfQuickFix()); } + @Nullable + private PsiExpression checkAndExtractCondition(IteratorDeclaration declaration, + PsiIfStatement ifStatement) { + PsiExpression condition = ifStatement.getCondition(); + if (condition == null || ifStatement.getElseBranch() != null) return null; + PsiStatement thenStatement = ControlFlowUtils.stripBraces(ifStatement.getThenBranch()); + if (!(thenStatement instanceof PsiExpressionStatement)) return null; + if (!declaration.isIteratorMethodCall(((PsiExpressionStatement)thenStatement).getExpression(), "remove")) return null; + if (!LambdaGenerationUtil.canBeUncheckedLambda(condition)) return null; + return condition; + } + @Override public void visitForStatement(PsiForStatement statement) { super.visitForStatement(statement); @@ -120,18 +168,41 @@ public class Java8CollectionRemoveIfInspection extends BaseJavaBatchLocalInspect PsiStatement body = loop.getBody(); if(!(body instanceof PsiBlockStatement)) return; PsiStatement[] statements = ((PsiBlockStatement)body).getCodeBlock().getStatements(); - if(statements.length != 2 || !(statements[1] instanceof PsiIfStatement)) return; - PsiVariable variable = declaration.getNextElementVariable(statements[0]); - if(variable == null) return; - PsiExpression condition = ((PsiIfStatement)statements[1]).getCondition(); - if(condition == null) return; + PsiElementFactory factory = JavaPsiFacade.getElementFactory(project); + String replacement = null; if (!FileModificationService.getInstance().preparePsiElementForWrite(element)) return; - String replacement = (declaration.myCollection == null ? "" : declaration.myCollection.getText() + ".") + - "removeIf(" + LambdaUtil.createLambda(variable, condition) + ");"; + if (statements.length == 2 && statements[1] instanceof PsiIfStatement) { + PsiVariable variable = declaration.getNextElementVariable(statements[0]); + if (variable == null) return; + PsiExpression condition = ((PsiIfStatement)statements[1]).getCondition(); + if (condition == null) return; + replacement = (declaration.myCollection == null ? "" : declaration.myCollection.getText() + ".") + + "removeIf(" + LambdaUtil.createLambda(variable, condition) + ");"; + } + else if (statements.length == 1 && statements[0] instanceof PsiIfStatement){ + PsiExpression condition = ((PsiIfStatement)statements[0]).getCondition(); + if (condition == null) return; + Optional iteratorRef = declaration.iteratorRefs(condition).collect(MoreCollectors.onlyOne()); + if(iteratorRef.isPresent()) { + PsiElement call = iteratorRef.get().getParent().getParent(); + if(!declaration.isIteratorMethodCall(call, "next")) return; + PsiType type = ((PsiExpression)call).getType(); + JavaCodeStyleManager javaCodeStyleManager = JavaCodeStyleManager.getInstance(project); + SuggestedNameInfo info = javaCodeStyleManager.suggestVariableName(VariableKind.PARAMETER, null, null, type); + if(info.names.length == 0) { + info = javaCodeStyleManager.suggestVariableName(VariableKind.PARAMETER, "value", null, type); + } + String paramName = javaCodeStyleManager.suggestUniqueVariableName(info, condition, true).names[0]; + call.replace(factory.createIdentifier(paramName)); + replacement = (declaration.myCollection == null ? "" : declaration.myCollection.getText() + ".") + + "removeIf(" + paramName + "->"+condition.getText() + ");"; + } + } + if(replacement == null) return; Collection comments = ContainerUtil.map(PsiTreeUtil.findChildrenOfType(loop, PsiComment.class), comment -> (PsiComment)comment.copy()); - PsiElement result = loop.replace(JavaPsiFacade.getElementFactory(project).createStatementFromText(replacement, loop)); - if(previous != null) previous.delete(); + PsiElement result = loop.replace(factory.createStatementFromText(replacement, loop)); + if (previous != null) previous.delete(); LambdaCanBeMethodReferenceInspection.replaceAllLambdasWithMethodReferences(result); CodeStyleManager.getInstance(project).reformat(result); comments.forEach(comment -> result.getParent().addBefore(comment, result)); @@ -151,7 +222,16 @@ public class Java8CollectionRemoveIfInspection extends BaseJavaBatchLocalInspect return isIteratorMethodCall(condition, "hasNext"); } - boolean isIteratorMethodCall(PsiExpression candidate, String method) { + public StreamEx iteratorRefs(PsiExpression parent) { + PsiElement element = PsiUtil.getVariableCodeBlock(myIterator, null); + PsiCodeBlock block = + element instanceof PsiCodeBlock ? (PsiCodeBlock)element : PsiTreeUtil.getParentOfType(element, PsiCodeBlock.class); + if(block == null) return StreamEx.empty(); + return StreamEx.of(DefUseUtil.getRefs(block, myIterator, myIterator.getInitializer())) + .filter(e -> PsiTreeUtil.isAncestor(parent, e, false)); + } + + boolean isIteratorMethodCall(PsiElement candidate, String method) { if(!(candidate instanceof PsiMethodCallExpression)) return false; PsiMethodCallExpression call = (PsiMethodCallExpression)candidate; if(call.getArgumentList().getExpressions().length != 0) return false; diff --git a/java/java-tests/testData/inspection/java8CollectionRemoveIf/afterIteratorRemoveInline.java b/java/java-tests/testData/inspection/java8CollectionRemoveIf/afterIteratorRemoveInline.java new file mode 100644 index 000000000000..fe9b9e02ff3d --- /dev/null +++ b/java/java-tests/testData/inspection/java8CollectionRemoveIf/afterIteratorRemoveInline.java @@ -0,0 +1,9 @@ +// "Replace the loop with Collection.removeIf" "true" +import java.util.Iterator; +import java.util.List; + +public class Main { + public void testIterator(List> data, boolean b) { + data.removeIf(strings -> strings.isEmpty() && b); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/java8CollectionRemoveIf/beforeIteratorRemoveInline.java b/java/java-tests/testData/inspection/java8CollectionRemoveIf/beforeIteratorRemoveInline.java new file mode 100644 index 000000000000..ccf66bbc4a01 --- /dev/null +++ b/java/java-tests/testData/inspection/java8CollectionRemoveIf/beforeIteratorRemoveInline.java @@ -0,0 +1,13 @@ +// "Replace the loop with Collection.removeIf" "true" +import java.util.Iterator; +import java.util.List; + +public class Main { + public void testIterator(List> data, boolean b) { + for(Iterator> iter = data.iterator(); iter.hasNext();) { + if(iter.next().isEmpty() && b) { + iter.remove(); + } + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/java8CollectionRemoveIf/beforeIteratorRemoveInlineShortCircuit.java b/java/java-tests/testData/inspection/java8CollectionRemoveIf/beforeIteratorRemoveInlineShortCircuit.java new file mode 100644 index 000000000000..f907b8c1cb45 --- /dev/null +++ b/java/java-tests/testData/inspection/java8CollectionRemoveIf/beforeIteratorRemoveInlineShortCircuit.java @@ -0,0 +1,13 @@ +// "Replace the loop with Collection.removeIf" "false" +import java.util.Iterator; +import java.util.List; + +public class Main { + public void testIterator(List> data, boolean b) { + for(Iterator> iter = data.iterator(); iter.hasNext();) { + if(b && iter.next().isEmpty()) { + iter.remove(); + } + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/java8CollectionRemoveIf/beforeIteratorRemoveInlineTwice.java b/java/java-tests/testData/inspection/java8CollectionRemoveIf/beforeIteratorRemoveInlineTwice.java new file mode 100644 index 000000000000..eed1ec2d9044 --- /dev/null +++ b/java/java-tests/testData/inspection/java8CollectionRemoveIf/beforeIteratorRemoveInlineTwice.java @@ -0,0 +1,13 @@ +// "Replace the loop with Collection.removeIf" "false" +import java.util.Iterator; +import java.util.List; + +public class Main { + public void testIterator(List> data, boolean b) { + for(Iterator> iter = data.iterator(); iter.hasNext();) { + if(iter.next().isEmpty() && iter.next().isEmpty()) { + iter.remove(); + } + } + } +} \ No newline at end of file