From ec54441d8d35f03764d79e679dbc0fccfe103524 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Mon, 14 Dec 2020 17:17:26 +0700 Subject: [PATCH] [java-inspections] IDEA-257647 Invalid optimization hint for PriorityQueue constructor usage GitOrigin-RevId: bce3692127bd4115eb81e482ea177b1d2952e46f --- ...anBeReplacedWithConstructorInspection.java | 29 ++++++++ .../afterComparators.java | 56 +++++++++++++++ .../beforeComparators.java | 71 +++++++++++++++++++ 3 files changed, 156 insertions(+) create mode 100644 java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterComparators.java create mode 100644 java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeComparators.java diff --git a/java/java-impl/src/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorInspection.java b/java/java-impl/src/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorInspection.java index 59245b6b8720..8bea393f5cc9 100644 --- a/java/java-impl/src/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorInspection.java @@ -2,6 +2,9 @@ package com.intellij.codeInspection; import com.intellij.codeInsight.daemon.QuickFixBundle; +import com.intellij.codeInspection.dataFlow.CommonDataflow; +import com.intellij.codeInspection.dataFlow.TypeConstraint; +import com.intellij.codeInspection.dataFlow.TypeConstraints; import com.intellij.openapi.project.Project; import com.intellij.openapi.util.InvalidDataException; import com.intellij.openapi.util.TextRange; @@ -92,12 +95,38 @@ public class CollectionAddAllCanBeReplacedWithConstructorInspection extends Abst } if (assignmentExpression == null || !isAddAllReplaceable(expression, assignmentExpression)) return; + if (mayInheritComparator(args, argType, assignmentExpression)) return; final PsiMethod method = expression.resolveMethod(); if (method != null) { holder.registerProblem(nameElement, QuickFixBundle.message("collection.addall.can.be.replaced.with.constructor.fix.description"), new ReplaceAddAllWithConstructorFix(assignmentExpression, expression, methodName)); } } + + private boolean mayInheritComparator(PsiExpression[] args, PsiType argType, PsiNewExpression assignmentExpression) { + PsiType type = assignmentExpression.getType(); + PsiClass collectionType = Objects.requireNonNull(PsiUtil.resolveClassInClassTypeOnly(type)); + String name = Objects.requireNonNull(collectionType.getQualifiedName()); + switch (name) { + case "java.util.TreeSet": + case "java.util.concurrent.ConcurrentSkipListSet": + // If declared arg type inherits SortedSet, the (SortedSet) copy constructor will be invoked, which inherits the comparator + return InheritanceUtil.isInheritor(argType, "java.util.SortedSet"); + case "java.util.TreeMap": + case "java.util.concurrent.ConcurrentSkipListMap": + // If declared arg type inherits SortedMap, the (SortedMap) copy constructor will be invoked, which inherits the comparator + return InheritanceUtil.isInheritor(argType, "java.util.SortedMap"); + case "java.util.PriorityQueue": + case "java.util.concurrent.PriorityBlockingQueue": + // Here even (Collection) copy constructor inherits the comparator using runtime type checks, so we should be more conservative + TypeConstraint constraint = TypeConstraint.fromDfType(CommonDataflow.getDfType(args[0])); + PsiClassType sortedSet = JavaPsiFacade.getElementFactory(holder.getProject()).createTypeByFQClassName("java.util.SortedSet"); + return constraint.meet(TypeConstraints.instanceOf(sortedSet)) != TypeConstraints.BOTTOM || + constraint.meet(TypeConstraints.instanceOf(type)) != TypeConstraints.BOTTOM; + default: + return false; + } + } }; } diff --git a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterComparators.java b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterComparators.java new file mode 100644 index 000000000000..5a15fb6cca64 --- /dev/null +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterComparators.java @@ -0,0 +1,56 @@ +// "Fix all 'Redundant 'Collection.addAll()' call' problems in file" "true" +import java.util.*; +import java.util.concurrent.ConcurrentSkipListMap; +import java.util.concurrent.PriorityBlockingQueue; + +public class InheritComparator { + public static void main(String[] args) { + SortedSet c1 = new TreeSet<>(Comparator.reverseOrder()); + Collection c2 = new TreeSet<>(Comparator.reverseOrder()); + Collection c3 = new HashSet<>(); + Collection c4 = new PriorityQueue<>(); + Collection c5 = new PriorityBlockingQueue<>(); + + Collection hs1 = new HashSet<>(c1); + Collection hs2 = new HashSet<>(c2); + Collection hs3 = new HashSet<>(c3); + Collection hs4 = new HashSet<>(c4); + Collection hs5 = new HashSet<>(c5); + + Collection ts1 = new TreeSet<>(); + ts1.addAll(c1); + Collection ts2 = new TreeSet<>(c2); + Collection ts3 = new TreeSet<>(c3); + Collection ts4 = new TreeSet<>(c4); + Collection ts5 = new TreeSet<>(c5); + + Collection pq1 = new PriorityQueue<>(); + pq1.addAll(c1); + Collection pq2 = new PriorityQueue<>(); + pq2.addAll(c2); + Collection pq3 = new PriorityQueue<>(c3); + Collection pq4 = new PriorityQueue<>(); + pq4.addAll(c4); + Collection pq5 = new PriorityQueue<>(c5); + + Collection pbq1 = new PriorityBlockingQueue<>(); + pbq1.addAll(c1); + Collection pbq2 = new PriorityBlockingQueue<>(); + pbq2.addAll(c2); + Collection pbq3 = new PriorityBlockingQueue<>(c3); + Collection pbq4 = new PriorityBlockingQueue<>(c4); + Collection pbq5 = new PriorityBlockingQueue<>(); + pbq5.addAll(c5); + + SortedMap m1 = new TreeMap<>(Comparator.reverseOrder()); + Map m2 = new TreeMap<>(Comparator.reverseOrder()); + + Map tm1 = new TreeMap<>(); + tm1.putAll(m1); + Map tm2 = new TreeMap<>(m2); + + Map cslm1 = new ConcurrentSkipListMap<>(); + cslm1.putAll(m1); + Map cslm2 = new ConcurrentSkipListMap<>(m2); + } +} diff --git a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeComparators.java b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeComparators.java new file mode 100644 index 000000000000..6f2944c5d60d --- /dev/null +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeComparators.java @@ -0,0 +1,71 @@ +// "Fix all 'Redundant 'Collection.addAll()' call' problems in file" "true" +import java.util.*; +import java.util.concurrent.ConcurrentSkipListMap; +import java.util.concurrent.PriorityBlockingQueue; + +public class InheritComparator { + public static void main(String[] args) { + SortedSet c1 = new TreeSet<>(Comparator.reverseOrder()); + Collection c2 = new TreeSet<>(Comparator.reverseOrder()); + Collection c3 = new HashSet<>(); + Collection c4 = new PriorityQueue<>(); + Collection c5 = new PriorityBlockingQueue<>(); + + Collection hs1 = new HashSet<>(); + hs1.addAll(c1); + Collection hs2 = new HashSet<>(); + hs2.addAll(c2); + Collection hs3 = new HashSet<>(); + hs3.addAll(c3); + Collection hs4 = new HashSet<>(); + hs4.addAll(c4); + Collection hs5 = new HashSet<>(); + hs5.addAll(c5); + + Collection ts1 = new TreeSet<>(); + ts1.addAll(c1); + Collection ts2 = new TreeSet<>(); + ts2.addAll(c2); + Collection ts3 = new TreeSet<>(); + ts3.addAll(c3); + Collection ts4 = new TreeSet<>(); + ts4.addAll(c4); + Collection ts5 = new TreeSet<>(); + ts5.addAll(c5); + + Collection pq1 = new PriorityQueue<>(); + pq1.addAll(c1); + Collection pq2 = new PriorityQueue<>(); + pq2.addAll(c2); + Collection pq3 = new PriorityQueue<>(); + pq3.addAll(c3); + Collection pq4 = new PriorityQueue<>(); + pq4.addAll(c4); + Collection pq5 = new PriorityQueue<>(); + pq5.addAll(c5); + + Collection pbq1 = new PriorityBlockingQueue<>(); + pbq1.addAll(c1); + Collection pbq2 = new PriorityBlockingQueue<>(); + pbq2.addAll(c2); + Collection pbq3 = new PriorityBlockingQueue<>(); + pbq3.addAll(c3); + Collection pbq4 = new PriorityBlockingQueue<>(); + pbq4.addAll(c4); + Collection pbq5 = new PriorityBlockingQueue<>(); + pbq5.addAll(c5); + + SortedMap m1 = new TreeMap<>(Comparator.reverseOrder()); + Map m2 = new TreeMap<>(Comparator.reverseOrder()); + + Map tm1 = new TreeMap<>(); + tm1.putAll(m1); + Map tm2 = new TreeMap<>(); + tm2.putAll(m2); + + Map cslm1 = new ConcurrentSkipListMap<>(); + cslm1.putAll(m1); + Map cslm2 = new ConcurrentSkipListMap<>(); + cslm2.putAll(m2); + } +}