From bee98f0f079af8bd8de1cbf46b372df2862d04b1 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Tue, 27 Sep 2016 17:47:29 +0700 Subject: [PATCH] IDEA-161259 Add Java-8 migration to use Collection.removeIf() where appropriate --- .../OptionalIsPresentInspection.java | 2 +- .../Java8CollectionsApiInspection.java | 167 +++++++++++++++++- .../afterIteratorRemoveFor.java | 8 + .../afterIteratorRemoveWhile.java | 9 + .../beforeIteratorRemoveFor.java | 13 ++ .../beforeIteratorRemoveWhile.java | 15 ++ .../beforeIteratorRemoveWhileReused.java | 16 ++ .../Java8CollectionsApi.html | 10 ++ .../src/messages/QuickFixBundle.properties | 2 + 9 files changed, 235 insertions(+), 7 deletions(-) create mode 100644 java/java-tests/testData/inspection/java8CollectionsApi/afterIteratorRemoveFor.java create mode 100644 java/java-tests/testData/inspection/java8CollectionsApi/afterIteratorRemoveWhile.java create mode 100644 java/java-tests/testData/inspection/java8CollectionsApi/beforeIteratorRemoveFor.java create mode 100644 java/java-tests/testData/inspection/java8CollectionsApi/beforeIteratorRemoveWhile.java create mode 100644 java/java-tests/testData/inspection/java8CollectionsApi/beforeIteratorRemoveWhileReused.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/OptionalIsPresentInspection.java b/java/java-analysis-impl/src/com/intellij/codeInspection/OptionalIsPresentInspection.java index 7f12f018cc99..d483d57ff252 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/OptionalIsPresentInspection.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/OptionalIsPresentInspection.java @@ -154,7 +154,7 @@ public class OptionalIsPresentInspection extends BaseJavaBatchLocalInspectionToo } @Contract("null -> false") - static boolean isVoidLambdaCandidate(PsiExpression lambdaCandidate) { + public static boolean isVoidLambdaCandidate(PsiExpression lambdaCandidate) { if(lambdaCandidate == null) return false; if(!ExceptionUtil.getThrownCheckedExceptions(new PsiElement[] {lambdaCandidate}).isEmpty()) return false; return PsiTreeUtil.processElements(lambdaCandidate, e -> { diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/java18api/Java8CollectionsApiInspection.java b/java/java-analysis-impl/src/com/intellij/codeInspection/java18api/Java8CollectionsApiInspection.java index f9a2e8ff5141..b23316268de0 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/java18api/Java8CollectionsApiInspection.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/java18api/Java8CollectionsApiInspection.java @@ -19,16 +19,16 @@ import com.intellij.codeInsight.ExceptionUtil; import com.intellij.codeInsight.FileModificationService; import com.intellij.codeInsight.daemon.QuickFixBundle; import com.intellij.codeInsight.daemon.impl.analysis.HighlightControlFlowUtil; -import com.intellij.codeInspection.BaseJavaBatchLocalInspectionTool; -import com.intellij.codeInspection.LocalQuickFix; -import com.intellij.codeInspection.ProblemDescriptor; -import com.intellij.codeInspection.ProblemsHolder; +import com.intellij.codeInspection.*; import com.intellij.codeInspection.ui.SingleCheckboxOptionsPanel; import com.intellij.openapi.diagnostic.Logger; 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.search.searches.ReferencesSearch; +import com.intellij.psi.util.InheritanceUtil; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.psi.util.PsiUtil; import com.intellij.util.containers.ContainerUtil; @@ -85,6 +85,51 @@ public class Java8CollectionsApiInspection extends BaseJavaBatchLocalInspectionT } } + void handleIteratorLoop(PsiLoopStatement statement, PsiJavaToken endToken, IteratorDeclaration declaration) { + if(endToken == null) return; + 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(!OptionalIsPresentInspection.isVoidLambdaCandidate(condition)) return; + //noinspection DialogTitleCapitalization + holder.registerProblem(statement, new TextRange(0, endToken.getTextOffset() - statement.getTextOffset() + 1), + QuickFixBundle.message("java.8.collections.api.inspection.remove.description"), + new ReplaceWithRemoveIfQuickFix()); + } + + @Override + public void visitForStatement(PsiForStatement statement) { + super.visitForStatement(statement); + PsiStatement initialization = statement.getInitialization(); + IteratorDeclaration declaration = IteratorDeclaration.extract(initialization); + if(declaration == null) return; + if(statement.getUpdate() != null) return; + if(!declaration.isHasNextCall(statement.getCondition())) return; + handleIteratorLoop(statement, statement.getRParenth(), declaration); + } + + @Override + public void visitWhileStatement(PsiWhileStatement statement) { + super.visitWhileStatement(statement); + PsiElement previous = PsiTreeUtil.skipSiblingsBackward(statement, PsiComment.class, PsiWhiteSpace.class); + if(!(previous instanceof PsiDeclarationStatement)) return; + IteratorDeclaration declaration = IteratorDeclaration.extract((PsiStatement)previous); + if(declaration == null || !declaration.isHasNextCall(statement.getCondition())) return; + if(!ReferencesSearch.search(declaration.myIterator, declaration.myIterator.getUseScope()).forEach(ref -> { + return PsiTreeUtil.isAncestor(statement, ref.getElement(), true); + })) return; + handleIteratorLoop(statement, statement.getRParenth(), declaration); + } + @Override public void visitConditionalExpression(PsiConditionalExpression expression) { final ConditionInfo conditionInfo = extractConditionInfo(expression.getCondition()); @@ -500,7 +545,7 @@ public class Java8CollectionsApiInspection extends BaseJavaBatchLocalInspectionT if (!(expression instanceof PsiAssignmentExpression)) return; PsiExpression defaultValue = ((PsiAssignmentExpression)expression).getRExpression(); if (!ExpressionUtils.isSimpleExpression(defaultValue)) return; - if (!FileModificationService.getInstance().preparePsiElementForWrite(element.getContainingFile())) return; + if (!FileModificationService.getInstance().preparePsiElementForWrite(element)) return; nameElement.replace(factory.createIdentifier("getOrDefault")); getCall.getArgumentList().add(defaultValue); } else if(thenBranch instanceof PsiBlockStatement) { @@ -510,7 +555,7 @@ public class Java8CollectionsApiInspection extends BaseJavaBatchLocalInspectionT if(assignment == null) return; PsiExpression lambdaCandidate = assignment.getRExpression(); if(lambdaCandidate == null) return; - if (!FileModificationService.getInstance().preparePsiElementForWrite(element.getContainingFile())) return; + if (!FileModificationService.getInstance().preparePsiElementForWrite(element)) return; nameElement.replace(factory.createIdentifier("computeIfAbsent")); String varName = JavaCodeStyleManager.getInstance(project).suggestUniqueVariableName("k", lambdaCandidate, true); PsiExpression lambda = factory.createExpressionFromText(varName + " -> " + lambdaCandidate.getText(), lambdaCandidate); @@ -521,4 +566,114 @@ public class Java8CollectionsApiInspection extends BaseJavaBatchLocalInspectionT comments.forEach(comment -> statement.getParent().addBefore(comment, statement)); } } + + private static class ReplaceWithRemoveIfQuickFix implements LocalQuickFix { + @Nls + @NotNull + @Override + public String getName() { + return QuickFixBundle.message("java.8.collections.api.inspection.remove.fix.name"); + } + + @Nls + @NotNull + @Override + public String getFamilyName() { + return getName(); + } + + @Override + public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) { + PsiElement element = descriptor.getStartElement(); + if(!(element instanceof PsiLoopStatement)) return; + PsiLoopStatement loop = (PsiLoopStatement)element; + IteratorDeclaration declaration; + PsiElement previous = null; + if(loop instanceof PsiForStatement) { + declaration = IteratorDeclaration.extract(((PsiForStatement)loop).getInitialization()); + } else if(loop instanceof PsiWhileStatement) { + previous = PsiTreeUtil.skipSiblingsBackward(loop, PsiComment.class, PsiWhiteSpace.class); + if(!(previous instanceof PsiDeclarationStatement)) return; + declaration = IteratorDeclaration.extract((PsiStatement)previous); + } else return; + if(declaration == null) return; + 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; + if (!FileModificationService.getInstance().preparePsiElementForWrite(element)) return; + String replacement = (declaration.myCollection == null ? "" : declaration.myCollection.getText() + ".") + + "removeIf(" + + LambdaUtil.createLambda(variable, condition) + + ");"; + 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(); + LambdaCanBeMethodReferenceInspection.replaceAllLambdasWithMethodReferences(result); + CodeStyleManager.getInstance(project).reformat(result); + comments.forEach(comment -> result.getParent().addBefore(comment, result)); + } + } + + static class IteratorDeclaration { + private final @NotNull PsiVariable myIterator; + private final @Nullable PsiExpression myCollection; + + private IteratorDeclaration(@NotNull PsiVariable iterator, @Nullable PsiExpression collection) { + myIterator = iterator; + myCollection = collection; + } + + public boolean isHasNextCall(PsiExpression condition) { + return isIteratorMethodCall(condition, "hasNext"); + } + + boolean isIteratorMethodCall(PsiExpression candidate, String method) { + if(!(candidate instanceof PsiMethodCallExpression)) return false; + PsiMethodCallExpression call = (PsiMethodCallExpression)candidate; + if(call.getArgumentList().getExpressions().length != 0) return false; + PsiReferenceExpression expression = call.getMethodExpression(); + if(!method.equals(expression.getReferenceName())) return false; + PsiExpression qualifier = expression.getQualifierExpression(); + if(!(qualifier instanceof PsiReferenceExpression)) return false; + return ((PsiReferenceExpression)qualifier).resolve() == myIterator; + } + + public PsiVariable getNextElementVariable(PsiStatement statement) { + if(!(statement instanceof PsiDeclarationStatement)) return null; + PsiDeclarationStatement declaration = (PsiDeclarationStatement)statement; + if(declaration.getDeclaredElements().length != 1) return null; + PsiElement element = declaration.getDeclaredElements()[0]; + if(!(element instanceof PsiVariable)) return null; + PsiVariable var = (PsiVariable)element; + if(!isIteratorMethodCall(var.getInitializer(), "next")) return null; + return var; + } + + @Contract("null -> null") + static IteratorDeclaration extract(PsiStatement statement) { + if(!(statement instanceof PsiDeclarationStatement)) return null; + PsiDeclarationStatement declaration = (PsiDeclarationStatement)statement; + if(declaration.getDeclaredElements().length != 1) return null; + PsiElement element = declaration.getDeclaredElements()[0]; + if(!(element instanceof PsiVariable)) return null; + PsiVariable variable = (PsiVariable)element; + PsiExpression initializer = variable.getInitializer(); + if(!(initializer instanceof PsiMethodCallExpression)) return null; + PsiMethodCallExpression call = (PsiMethodCallExpression)initializer; + if(call.getArgumentList().getExpressions().length != 0) return null; + PsiReferenceExpression methodExpression = call.getMethodExpression(); + if(!"iterator".equals(methodExpression.getReferenceName())) return null; + PsiMethod method = call.resolveMethod(); + if(method == null || !InheritanceUtil.isInheritor(method.getContainingClass(), CommonClassNames.JAVA_UTIL_COLLECTION)) return null; + PsiType type = variable.getType(); + if(!(type instanceof PsiClassType) || !((PsiClassType)type).rawType().equalsToText(CommonClassNames.JAVA_UTIL_ITERATOR)) return null; + return new IteratorDeclaration(variable, methodExpression.getQualifierExpression()); + } + } } \ No newline at end of file diff --git a/java/java-tests/testData/inspection/java8CollectionsApi/afterIteratorRemoveFor.java b/java/java-tests/testData/inspection/java8CollectionsApi/afterIteratorRemoveFor.java new file mode 100644 index 000000000000..7e4741a3599e --- /dev/null +++ b/java/java-tests/testData/inspection/java8CollectionsApi/afterIteratorRemoveFor.java @@ -0,0 +1,8 @@ +// "Replace the loop with Collection.removeIf" "true" +import java.util.*; + +public class Main { + public void removeEmpty(List list) throws Exception { + list.removeIf(String::isEmpty); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/java8CollectionsApi/afterIteratorRemoveWhile.java b/java/java-tests/testData/inspection/java8CollectionsApi/afterIteratorRemoveWhile.java new file mode 100644 index 000000000000..162cd84f3f7c --- /dev/null +++ b/java/java-tests/testData/inspection/java8CollectionsApi/afterIteratorRemoveWhile.java @@ -0,0 +1,9 @@ +// "Replace the loop with Collection.removeIf" "true" +import java.util.*; + +public class Main { + public void removeEmpty(List list) throws Exception { + // remove empty + list.removeIf(String::isEmpty); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/java8CollectionsApi/beforeIteratorRemoveFor.java b/java/java-tests/testData/inspection/java8CollectionsApi/beforeIteratorRemoveFor.java new file mode 100644 index 000000000000..5b2aed95e73c --- /dev/null +++ b/java/java-tests/testData/inspection/java8CollectionsApi/beforeIteratorRemoveFor.java @@ -0,0 +1,13 @@ +// "Replace the loop with Collection.removeIf" "true" +import java.util.*; + +public class Main { + public void removeEmpty(List list) throws Exception { + for(Iterator it = list.iterator(); it.hasNext();) { + String str = it.next(); + if(str.isEmpty()) { + it.remove(); + } + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/java8CollectionsApi/beforeIteratorRemoveWhile.java b/java/java-tests/testData/inspection/java8CollectionsApi/beforeIteratorRemoveWhile.java new file mode 100644 index 000000000000..ca9e10e7b6ba --- /dev/null +++ b/java/java-tests/testData/inspection/java8CollectionsApi/beforeIteratorRemoveWhile.java @@ -0,0 +1,15 @@ +// "Replace the loop with Collection.removeIf" "true" +import java.util.*; + +public class Main { + public void removeEmpty(List list) throws Exception { + Iterator it = list.iterator(); + while(it.hasNext()) { + String str = it.next(); + // remove empty + if(str.isEmpty()) { + it.remove(); + } + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/java8CollectionsApi/beforeIteratorRemoveWhileReused.java b/java/java-tests/testData/inspection/java8CollectionsApi/beforeIteratorRemoveWhileReused.java new file mode 100644 index 000000000000..87bf6b6d15a7 --- /dev/null +++ b/java/java-tests/testData/inspection/java8CollectionsApi/beforeIteratorRemoveWhileReused.java @@ -0,0 +1,16 @@ +// "Replace the loop with Collection.removeIf" "false" +import java.util.*; + +public class Main { + public void removeEmpty(List list) throws Exception { + Iterator it = list.iterator(); + while(it.hasNext()) { + String str = it.next(); + // remove empty + if(str.isEmpty()) { + it.remove(); + } + } + it = list.iterator(); + } +} \ No newline at end of file diff --git a/resources-en/src/inspectionDescriptions/Java8CollectionsApi.html b/resources-en/src/inspectionDescriptions/Java8CollectionsApi.html index 3e119cbbe772..f773fb5138e9 100644 --- a/resources-en/src/inspectionDescriptions/Java8CollectionsApi.html +++ b/resources-en/src/inspectionDescriptions/Java8CollectionsApi.html @@ -28,6 +28,16 @@ The following cases are covered by this inspection: } +
  • Collection.removeIf method could be used to replace the code like this: + 
    +      for (Iterator<String> it = collection.iterator(); it.hasNext(); ) {
+        String aValue = it.next();
+        if(shouldBeRemoved(aValue)) {
+          it.remove();
+        }
+      }
+
    +
  • List.sort instance method could be used to replace Collections.sort static method
    • diff --git a/resources-en/src/messages/QuickFixBundle.properties b/resources-en/src/messages/QuickFixBundle.properties index e03fe93efc09..82ffa1e9f480 100644 --- a/resources-en/src/messages/QuickFixBundle.properties +++ b/resources-en/src/messages/QuickFixBundle.properties @@ -289,6 +289,8 @@ java.8.collections.api.inspection.fix.text=Replace with ''{0}'' method call java.8.collections.api.inspection.sort.description=Collections.sort could be replaced with List.sort java.8.collections.api.inspection.sort.fix.name=Replace with List.sort java.8.collections.api.inspection.get.fix.family.name=Replace Map.get and condition with single method call +java.8.collections.api.inspection.remove.description=The loop could be replaced with Collection.removeIf +java.8.collections.api.inspection.remove.fix.name=Replace the loop with Collection.removeIf wrap.with.optional.parameter.text=Wrap {0, choice, 1#1st|2#2nd|3#3rd|4#{0,number}th} parameter using ''java.util.Optional'' wrap.with.optional.single.parameter.text=Wrap using 'java.util.Optional'