From 59923c803f869078251d580374fddbb197d3a4d7 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Wed, 18 Jan 2017 11:04:33 +0700 Subject: [PATCH] IDEA-160448 Simplify groupingBy-collectingAndThen-Optional.get Collectors chain --- .../SimplifyCollectorInspection.java | 183 ++++++++++++++++++ .../simplifyCollector/afterMaxBy.java | 16 ++ .../simplifyCollector/afterReducing.java | 19 ++ .../afterReducingMapping.java | 14 ++ .../simplifyCollector/beforeMaxBy.java | 15 ++ .../simplifyCollector/beforeReducing.java | 18 ++ .../beforeReducingMapping.java | 17 ++ .../SimplifyCollectorInspectionTest.java | 40 ++++ .../src/messages/InspectionsBundle.properties | 4 + .../siyeh/ig/psiutils/MethodCallUtils.java | 42 ++++ .../SimplifyCollector.html | 9 + resources/src/META-INF/IdeaPlugin.xml | 5 + 12 files changed, 382 insertions(+) create mode 100644 java/java-impl/src/com/intellij/codeInspection/SimplifyCollectorInspection.java create mode 100644 java/java-tests/testData/inspection/simplifyCollector/afterMaxBy.java create mode 100644 java/java-tests/testData/inspection/simplifyCollector/afterReducing.java create mode 100644 java/java-tests/testData/inspection/simplifyCollector/afterReducingMapping.java create mode 100644 java/java-tests/testData/inspection/simplifyCollector/beforeMaxBy.java create mode 100644 java/java-tests/testData/inspection/simplifyCollector/beforeReducing.java create mode 100644 java/java-tests/testData/inspection/simplifyCollector/beforeReducingMapping.java create mode 100644 java/java-tests/testSrc/com/intellij/codeInspection/SimplifyCollectorInspectionTest.java create mode 100644 resources-en/src/inspectionDescriptions/SimplifyCollector.html diff --git a/java/java-impl/src/com/intellij/codeInspection/SimplifyCollectorInspection.java b/java/java-impl/src/com/intellij/codeInspection/SimplifyCollectorInspection.java new file mode 100644 index 000000000000..93ba4f314a4c --- /dev/null +++ b/java/java-impl/src/com/intellij/codeInspection/SimplifyCollectorInspection.java @@ -0,0 +1,183 @@ +/* + * Copyright 2000-2017 JetBrains s.r.o. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.intellij.codeInspection; + +import com.intellij.openapi.project.Project; +import com.intellij.psi.*; +import com.intellij.psi.codeStyle.CodeStyleManager; +import com.intellij.psi.codeStyle.JavaCodeStyleManager; +import com.intellij.psi.impl.PsiDiamondTypeUtil; +import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.psi.util.PsiUtil; +import com.intellij.util.ArrayUtil; +import com.intellij.util.ObjectUtils; +import com.siyeh.ig.psiutils.CommentTracker; +import com.siyeh.ig.psiutils.MethodCallUtils; +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; + +/** + * @author Tagir Valeev + */ +public class SimplifyCollectorInspection extends BaseJavaBatchLocalInspectionTool { + @NotNull + @Override + public PsiElementVisitor buildVisitor(@NotNull ProblemsHolder holder, boolean isOnTheFly) { + if (!PsiUtil.isLanguageLevel8OrHigher(holder.getFile())) { + return PsiElementVisitor.EMPTY_VISITOR; + } + + return new JavaElementVisitor() { + @Override + public void visitMethodCallExpression(PsiMethodCallExpression call) { + super.visitMethodCallExpression(call); + PsiElement nameElement = call.getMethodExpression().getReferenceNameElement(); + if (nameElement == null || !isCollectorMethod(call, "groupingBy", "groupingByConcurrent")) return; + PsiExpression[] args = call.getArgumentList().getExpressions(); + if (args.length != 2 && args.length != 3) return; + CombinedCollector combinedCollector = new CombinedCollector(ArrayUtil.getLastElement(args), null, null); + // Unwrap at most twice to gather collectingAndThen and/or mapping + combinedCollector = combinedCollector.tryUnwrap().tryUnwrap(); + PsiMethodCallExpression downstream = ObjectUtils.tryCast(combinedCollector.myDownstream, PsiMethodCallExpression.class); + if (downstream == null || + !MethodCallUtils.isFunctionalReferenceTo(combinedCollector.myFinisher, CommonClassNames.JAVA_UTIL_OPTIONAL, null, "get")) { + return; + } + if (isCollectorMethod(downstream, "maxBy", "minBy", "reducing") && + downstream.getArgumentList().getExpressions().length == 1) { + String replacement = nameElement.getText().equals("groupingBy") ? "toMap" : "toConcurrentMap"; + holder.registerProblem(nameElement, InspectionsBundle.message("inspection.simplify.collector.message", replacement), + new SimplifyCollectorFix(replacement)); + } + } + }; + } + + @Contract("null, _ -> false") + private static boolean isCollectorMethod(PsiMethodCallExpression call, String... methodNames) { + if (call == null) return false; + String name = call.getMethodExpression().getReferenceName(); + if (ArrayUtil.contains(name, methodNames)) { + PsiMethod method = call.resolveMethod(); + if (method != null && method.hasModifierProperty(PsiModifier.STATIC)) { + PsiClass aClass = method.getContainingClass(); + return aClass != null && CommonClassNames.JAVA_UTIL_STREAM_COLLECTORS.equals(aClass.getQualifiedName()) + && method.getParameterList().getParametersCount() == call.getArgumentList().getExpressions().length; + } + } + return false; + } + + static class CombinedCollector { + final PsiExpression myDownstream; + final @Nullable PsiExpression myFinisher; + final @Nullable PsiExpression myMapper; + + CombinedCollector(PsiExpression downstream, @Nullable PsiExpression finisher, @Nullable PsiExpression mapper) { + myDownstream = PsiUtil.skipParenthesizedExprDown(downstream); + myFinisher = PsiUtil.skipParenthesizedExprDown(finisher); + myMapper = PsiUtil.skipParenthesizedExprDown(mapper); + } + + CombinedCollector tryUnwrap() { + if (myDownstream instanceof PsiMethodCallExpression) { + PsiMethodCallExpression call = (PsiMethodCallExpression)myDownstream; + PsiExpression[] args = call.getArgumentList().getExpressions(); + if (myFinisher == null && isCollectorMethod(call, "collectingAndThen")) { + return new CombinedCollector(args[0], args[1], myMapper); + } + if (myMapper == null && isCollectorMethod(call, "mapping")) { + return new CombinedCollector(args[1], myFinisher, args[0]); + } + } + return this; + } + } + + private static class SimplifyCollectorFix implements LocalQuickFix { + private String myMethodName; + + public SimplifyCollectorFix(String methodName) { + myMethodName = methodName; + } + + @Nls + @NotNull + @Override + public String getName() { + return InspectionsBundle.message("inspection.simplify.collector.fix.name", myMethodName); + } + + @Nls + @NotNull + @Override + public String getFamilyName() { + return InspectionsBundle.message("inspection.simplify.collector.fix.family.name"); + } + + @Override + public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) { + PsiMethodCallExpression call = PsiTreeUtil.getParentOfType(descriptor.getStartElement(), PsiMethodCallExpression.class); + if (!isCollectorMethod(call, "groupingBy", "groupingByConcurrent")) return; + PsiExpression[] args = call.getArgumentList().getExpressions(); + if (args.length != 2 && args.length != 3) return; + CombinedCollector combinedCollector = new CombinedCollector(ArrayUtil.getLastElement(args), null, null); + // Unwrap at most twice to gather collectingAndThen and/or mapping + combinedCollector = combinedCollector.tryUnwrap().tryUnwrap(); + PsiMethodCallExpression downstream = ObjectUtils.tryCast(combinedCollector.myDownstream, PsiMethodCallExpression.class); + if (downstream == null || + !MethodCallUtils.isFunctionalReferenceTo(combinedCollector.myFinisher, CommonClassNames.JAVA_UTIL_OPTIONAL, null, "get")) { + return; + } + PsiExpression[] downstreamArgs = downstream.getArgumentList().getExpressions(); + if (downstreamArgs.length != 1) return; + PsiExpression downstreamArg = downstreamArgs[0]; + String downstreamName = downstream.getMethodExpression().getReferenceName(); + if (downstreamName == null) return; + CommentTracker ct = new CommentTracker(); + PsiType collectorType = call.getType(); + PsiType mapType = PsiUtil.substituteTypeParameter(collectorType, "java.util.stream.Collector", 2, false); + PsiType valueType = PsiUtil.substituteTypeParameter(mapType, CommonClassNames.JAVA_UTIL_MAP, 1, false); + String valueTypeArg = valueType == null ? "" : "<" + valueType.getCanonicalText() + ">"; + String merger; + switch (downstreamName) { + case "minBy": + case "maxBy": + merger = "java.util.function.BinaryOperator." + valueTypeArg + downstreamName + "(" + ct.text(downstreamArg) + ")"; + break; + case "reducing": + merger = ct.text(downstreamArg); + break; + default: + return; + } + String keyMapper = ct.text(args[0]); + String mapSupplier = args.length == 3 ? ct.text(args[1]) : null; + String valueMapper = + combinedCollector.myMapper == null ? CommonClassNames.JAVA_UTIL_FUNCTION_FUNCTION + "." + valueTypeArg + "identity()" : + ct.text(combinedCollector.myMapper); + String replacement = StreamEx.of(keyMapper, valueMapper, merger, mapSupplier).nonNull() + .joining(",", CommonClassNames.JAVA_UTIL_STREAM_COLLECTORS + "." + myMethodName + "(", ")"); + PsiElement result = ct.replaceAndRestoreComments(call, replacement); + PsiDiamondTypeUtil.removeRedundantTypeArguments(result); + result = JavaCodeStyleManager.getInstance(project).shortenClassReferences(result); + CodeStyleManager.getInstance(project).reformat(result); + } + } +} diff --git a/java/java-tests/testData/inspection/simplifyCollector/afterMaxBy.java b/java/java-tests/testData/inspection/simplifyCollector/afterMaxBy.java new file mode 100644 index 000000000000..d940dbcfea70 --- /dev/null +++ b/java/java-tests/testData/inspection/simplifyCollector/afterMaxBy.java @@ -0,0 +1,16 @@ +// "Use 'Collectors.toConcurrentMap' collector" "true" +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.function.BinaryOperator; +import java.util.function.Function; +import java.util.stream.Collectors; + +public class Test { + public static void main(String[] args) { + List input = Arrays.asList("a", "bbb", "cc", "ddd", " x", "ee"); + Map map3 = input.stream().collect(Collectors.toConcurrentMap(String::length, Function.identity(), BinaryOperator.maxBy(String::compareTo))); + System.out.println(map3); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/simplifyCollector/afterReducing.java b/java/java-tests/testData/inspection/simplifyCollector/afterReducing.java new file mode 100644 index 000000000000..50a274add9ab --- /dev/null +++ b/java/java-tests/testData/inspection/simplifyCollector/afterReducing.java @@ -0,0 +1,19 @@ +// "Use 'Collectors.toMap' collector" "true" +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.function.Function; +import java.util.stream.Collectors; + +public class Test { + public static void main(String[] args) { + List input = Arrays.asList("a", "bbb", "cc", "ddd", " x", "ee"); + // group by length + // <= reduce to max + /*need to unwrap optional*/ + Map map2 = input.stream().collect( + Collectors.toMap(String::length, Function.identity(), (s1, s2) -> s1.compareTo(s2) > 0 ? /*find max*/ s1 : s2)); + System.out.println(map2); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/simplifyCollector/afterReducingMapping.java b/java/java-tests/testData/inspection/simplifyCollector/afterReducingMapping.java new file mode 100644 index 000000000000..bd89029b2cdc --- /dev/null +++ b/java/java-tests/testData/inspection/simplifyCollector/afterReducingMapping.java @@ -0,0 +1,14 @@ +// "Use 'Collectors.toMap' collector" "true" +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.stream.Collectors; + +public class Test { + public static void main(String[] args) { + List input = Arrays.asList("a", "bbb", "cc", "ddd", " x", "ee"); + Map map = input.stream().collect(Collectors.toMap(String::length, String::trim, (s1, s2) -> s1.compareTo(s2) > 0 ? s1 : s2)); + System.out.println(map); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/simplifyCollector/beforeMaxBy.java b/java/java-tests/testData/inspection/simplifyCollector/beforeMaxBy.java new file mode 100644 index 000000000000..32d5c81c9eaf --- /dev/null +++ b/java/java-tests/testData/inspection/simplifyCollector/beforeMaxBy.java @@ -0,0 +1,15 @@ +// "Use 'Collectors.toConcurrentMap' collector" "true" +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.stream.Collectors; + +public class Test { + public static void main(String[] args) { + List input = Arrays.asList("a", "bbb", "cc", "ddd", " x", "ee"); + Map map3 = input.stream().collect(Collectors.groupingByConcurrent(String::length, Collectors + .collectingAndThen(Collectors.maxBy(String::compareTo), Optional::get))); + System.out.println(map3); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/simplifyCollector/beforeReducing.java b/java/java-tests/testData/inspection/simplifyCollector/beforeReducing.java new file mode 100644 index 000000000000..512ce04f7092 --- /dev/null +++ b/java/java-tests/testData/inspection/simplifyCollector/beforeReducing.java @@ -0,0 +1,18 @@ +// "Use 'Collectors.toMap' collector" "true" +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.stream.Collectors; + +public class Test { + public static void main(String[] args) { + List input = Arrays.asList("a", "bbb", "cc", "ddd", " x", "ee"); + Map map2 = input.stream().collect( + Collectors.groupingBy(String::length, // group by length + Collectors.collectingAndThen( + Collectors.reducing((s1, s2) -> s1.compareTo(s2) > 0 ? /*find max*/ s1 : s2), // <= reduce to max + /*need to unwrap optional*/ s -> s.get()))); + System.out.println(map2); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/simplifyCollector/beforeReducingMapping.java b/java/java-tests/testData/inspection/simplifyCollector/beforeReducingMapping.java new file mode 100644 index 000000000000..bff6855630f5 --- /dev/null +++ b/java/java-tests/testData/inspection/simplifyCollector/beforeReducingMapping.java @@ -0,0 +1,17 @@ +// "Use 'Collectors.toMap' collector" "true" +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.stream.Collectors; + +public class Test { + public static void main(String[] args) { + List input = Arrays.asList("a", "bbb", "cc", "ddd", " x", "ee"); + Map map = input.stream().collect(Collectors + .groupingBy(String::length, Collectors.mapping(String::trim, Collectors + .collectingAndThen(Collectors + .reducing((s1, s2) -> s1.compareTo(s2) > 0 ? s1 : s2), Optional::get)))); + System.out.println(map); + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/codeInspection/SimplifyCollectorInspectionTest.java b/java/java-tests/testSrc/com/intellij/codeInspection/SimplifyCollectorInspectionTest.java new file mode 100644 index 000000000000..0fd84179fd2f --- /dev/null +++ b/java/java-tests/testSrc/com/intellij/codeInspection/SimplifyCollectorInspectionTest.java @@ -0,0 +1,40 @@ +/* + * Copyright 2000-2017 JetBrains s.r.o. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.intellij.codeInspection; + +import com.intellij.codeInsight.daemon.quickFix.LightQuickFixParameterizedTestCase; +import org.jetbrains.annotations.NotNull; + +/** + * @author Tagir Valeev + */ +public class SimplifyCollectorInspectionTest extends LightQuickFixParameterizedTestCase { + + @NotNull + @Override + protected LocalInspectionTool[] configureLocalInspectionTools() { + return new LocalInspectionTool[]{new SimplifyCollectorInspection()}; + } + + public void test() throws Exception { + doAllTests(); + } + + @Override + protected String getBasePath() { + return "/inspection/simplifyCollector"; + } +} diff --git a/platform/platform-resources-en/src/messages/InspectionsBundle.properties b/platform/platform-resources-en/src/messages/InspectionsBundle.properties index 7e087f7dec24..d9f1c8a28346 100644 --- a/platform/platform-resources-en/src/messages/InspectionsBundle.properties +++ b/platform/platform-resources-en/src/messages/InspectionsBundle.properties @@ -761,3 +761,7 @@ inspection.replace.with.bulk.wrap.arrays=Use Arrays.asList() to wrap arrays inspection.replace.loop.with.arrays.setall.message=Loop can be replaced with Arrays.setAll inspection.replace.loop.with.arrays.setall.fix.family.name=Replace loop with Arrays.setAll + +inspection.simplify.collector.message=Can be simplified using ''{0}'' collector +inspection.simplify.collector.fix.family.name=Simplify cascaded collector +inspection.simplify.collector.fix.name=Use ''Collectors.{0}'' collector diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/MethodCallUtils.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/MethodCallUtils.java index 67dfffd8e77b..d308861ba61e 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/MethodCallUtils.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/MethodCallUtils.java @@ -21,8 +21,10 @@ import com.intellij.psi.search.GlobalSearchScope; import com.intellij.psi.util.MethodSignatureUtil; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.psi.util.PsiUtil; +import com.intellij.util.ArrayUtil; import com.intellij.util.ObjectUtils; import com.siyeh.HardcodedMethodConstants; +import one.util.streamex.StreamEx; import org.jetbrains.annotations.NonNls; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -260,6 +262,46 @@ public class MethodCallUtils { return variable.equals(element); } + /** + * Returns true if the supplied expression is the functional expression (method reference or lambda) + * which refers to the given method call + * + * @param expression expression to test + * @param className class name where the wanted method should be located + * @param returnType the return type of the wanted method (null if should not be checked) + * @param methodName method name of the wanted method + * @param parameterTypes wanted method parameter types (nulls for parameters which should not be checked) + * @return true if the supplied expression references the wanted call + */ + public static boolean isFunctionalReferenceTo(PsiExpression expression, String className, PsiType returnType, + String methodName, PsiType... parameterTypes) { + expression = PsiUtil.skipParenthesizedExprDown(expression); + if (expression instanceof PsiMethodReferenceExpression) { + PsiMethodReferenceExpression methodRef = (PsiMethodReferenceExpression)expression; + PsiMethod method = ObjectUtils.tryCast(methodRef.resolve(), PsiMethod.class); + PsiReferenceExpression ref = ObjectUtils.tryCast(methodRef.getQualifier(), PsiReferenceExpression.class); + return ref != null && + method != null && + MethodUtils.methodMatches(method, className, returnType, methodName, parameterTypes) && + ref.isReferenceTo(method.getContainingClass()); + } + if (expression instanceof PsiLambdaExpression) { + PsiLambdaExpression lambda = (PsiLambdaExpression)expression; + PsiExpression body = PsiUtil.skipParenthesizedExprDown(LambdaUtil.extractSingleExpressionFromBody(lambda.getBody())); + PsiMethodCallExpression call = ObjectUtils.tryCast(body, PsiMethodCallExpression.class); + if (call == null || !isCallToMethod(call, className, returnType, methodName, parameterTypes)) return false; + PsiParameter[] parameters = lambda.getParameterList().getParameters(); + PsiExpression[] args = call.getArgumentList().getExpressions(); + PsiMethod method = call.resolveMethod(); + if (method != null && !method.hasModifierProperty(PsiModifier.STATIC)) { + args = ArrayUtil.prepend(call.getMethodExpression().getQualifierExpression(), args); + } + if (parameters.length != args.length || StreamEx.zip(args, parameters, ExpressionUtils::isReferenceTo).has(false)) return false; + return isCallToMethod(call, className, returnType, methodName, parameterTypes); + } + return false; + } + @Nullable public static PsiMethod findMethodWithReplacedArgument(@NotNull PsiCall call, @NotNull PsiExpression target, @NotNull PsiExpression replacement) { diff --git a/resources-en/src/inspectionDescriptions/SimplifyCollector.html b/resources-en/src/inspectionDescriptions/SimplifyCollector.html new file mode 100644 index 000000000000..6f482738e5af --- /dev/null +++ b/resources-en/src/inspectionDescriptions/SimplifyCollector.html @@ -0,0 +1,9 @@ + + +This inspection detects collectors which could be simplified. In particular some cascaded groupingBy collectors +could be expressed with simpler toMap collector which also will likely be more performant. + +

This inspection is available since Java 8 only.

+New in 2017.1 + + \ No newline at end of file diff --git a/resources/src/META-INF/IdeaPlugin.xml b/resources/src/META-INF/IdeaPlugin.xml index 391d22cad893..f3ae9485628c 100644 --- a/resources/src/META-INF/IdeaPlugin.xml +++ b/resources/src/META-INF/IdeaPlugin.xml @@ -865,6 +865,11 @@ groupKey="group.names.declaration.redundancy" enabledByDefault="true" level="WARNING" implementationClass="com.intellij.codeInspection.SimplifyStreamApiCallChainsInspection" displayName="Simplify stream API call chains"/> +