CollectionAddAllCanBeReplacedWithConstructor: fixed, enabled by default

IDEA-178761 Inspection 'Collection.addAll() can be replaced with parametrized constructor' should be turned on by default
Fixes comments handling, possible semantics change; now suggested in more cases
This commit is contained in:
Tagir Valeev
2017-09-27 14:24:13 +07:00
parent ae3e99cf14
commit 5bb73b4408
19 changed files with 317 additions and 130 deletions
@@ -18,17 +18,22 @@ package com.intellij.codeInspection;
import com.intellij.codeInsight.daemon.QuickFixBundle;
import com.intellij.openapi.project.Project;
import com.intellij.openapi.util.InvalidDataException;
import com.intellij.openapi.util.Pair;
import com.intellij.openapi.util.TextRange;
import com.intellij.openapi.util.WriteExternalException;
import com.intellij.psi.*;
import com.intellij.psi.search.LocalSearchScope;
import com.intellij.psi.search.searches.ReferencesSearch;
import com.intellij.psi.util.InheritanceUtil;
import com.intellij.psi.util.PsiTreeUtil;
import com.intellij.util.NullableFunction;
import com.intellij.psi.util.PsiUtil;
import com.intellij.util.ObjectUtils;
import com.intellij.util.containers.ContainerUtil;
import com.siyeh.ig.performance.CollectionsListSettings;
import com.siyeh.ig.psiutils.CommentTracker;
import com.siyeh.ig.psiutils.ControlFlowUtils;
import com.siyeh.ig.psiutils.ControlFlowUtils.InitializerUsageStatus;
import com.siyeh.ig.psiutils.ExpressionUtils;
import com.siyeh.ig.psiutils.VariableAccessUtils;
import org.jdom.Element;
import org.jetbrains.annotations.Nls;
import org.jetbrains.annotations.NotNull;
@@ -37,7 +42,7 @@ import org.jetbrains.annotations.Nullable;
import javax.swing.*;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Objects;
/**
* @author Dmitry Batkovich
@@ -47,7 +52,7 @@ public class CollectionAddAllCanBeReplacedWithConstructorInspection extends Base
private final CollectionsListSettings mySettings = new CollectionsListSettings() {
@Override
protected Collection<String> getDefaultSettings() {
return DEFAULT_COLLECTION_LIST;
return ContainerUtil.append(new ArrayList<>(DEFAULT_COLLECTION_LIST), "java.util.TreeSet", "java.util.TreeMap");
}
};
@@ -78,50 +83,33 @@ public class CollectionAddAllCanBeReplacedWithConstructorInspection extends Base
final PsiReferenceExpression methodExpression = expression.getMethodExpression();
final PsiElement nameElement = methodExpression.getReferenceNameElement();
final String methodName = methodExpression.getReferenceName();
if (nameElement == null || !"addAll".equals(methodName) && !"putAll".equals(methodName)) {
return;
}
if (expression.getArgumentList().getExpressions().length != 1) {
return;
}
final PsiExpression qualifierExpression = methodExpression.getQualifierExpression();
if (!(qualifierExpression instanceof PsiReferenceExpression)) {
return;
}
final PsiElement parent = expression.getParent();
if (!(parent instanceof PsiExpressionStatement)) {
return;
}
final PsiElement resolvedReference = ((PsiReferenceExpression)qualifierExpression).resolve();
if (!(resolvedReference instanceof PsiLocalVariable)) {
return;
}
PsiLocalVariable variable = (PsiLocalVariable)resolvedReference;
final PsiType variableType = variable.getType();
if (!(variableType instanceof PsiClassType) || statementHasSubsequentAddAll(parent, variable, methodName)) {
return;
}
final PsiClass variableClass = ((PsiClassType)variableType).resolve();
if (variableClass == null) {
if (nameElement == null || !"addAll".equals(methodName) && !"putAll".equals(methodName)) return;
PsiExpression[] args = expression.getArgumentList().getExpressions();
if (args.length != 1) return;
final PsiExpressionStatement parent = ObjectUtils.tryCast(expression.getParent(), PsiExpressionStatement.class);
if (parent == null) return;
PsiLocalVariable variable = ExpressionUtils.resolveLocalVariable(methodExpression.getQualifierExpression());
if (variable == null) return;
if (PsiUtil.resolveClassInClassTypeOnly(variable.getType()) == null) return;
if (statementHasSubsequentAddAll(parent, variable, methodName)) return;
PsiType argType = args[0].getType();
if (!InheritanceUtil.isInheritor(argType, "putAll".equals(methodName) ?
CommonClassNames.JAVA_UTIL_MAP : CommonClassNames.JAVA_UTIL_COLLECTION)) {
return;
}
InitializerUsageStatus status = ControlFlowUtils.getInitializerUsageStatus(variable, parent);
PsiNewExpression assignmentExpression;
final Pair<Boolean, PsiNewExpression> pair = isProperAssignmentStatementFound(variable, expression);
if (pair.getFirst()) {
assignmentExpression = pair.getSecond();
if (assignmentExpression == null) {
if (checkLocalVariableAssignmentOrInitializer(variable.getInitializer())) {
assignmentExpression = (PsiNewExpression)variable.getInitializer();
} else {
return;
}
}
} else {
return;
if (status == InitializerUsageStatus.DECLARED_JUST_BEFORE || status == InitializerUsageStatus.AT_WANTED_PLACE_ONLY) {
if (!isCollectionConstructor(variable.getInitializer())) return;
assignmentExpression = (PsiNewExpression)variable.getInitializer();
}
if (!isAddAllReplaceable(expression, assignmentExpression) || !checkUsages(variable, expression, assignmentExpression)) {
return;
else {
assignmentExpression = getPreviousAssignment(variable, parent);
}
if (assignmentExpression == null || !isAddAllReplaceable(expression, assignmentExpression)) return;
final PsiMethod method = expression.resolveMethod();
if (method != null) {
//noinspection DialogTitleCapitalization
@@ -132,6 +120,18 @@ public class CollectionAddAllCanBeReplacedWithConstructorInspection extends Base
};
}
private PsiNewExpression getPreviousAssignment(PsiLocalVariable variable, PsiStatement statement) {
while (true) {
statement = PsiTreeUtil.getPrevSiblingOfType(statement, PsiStatement.class);
if (statement == null) return null;
PsiExpression expression = ExpressionUtils.getAssignmentTo(statement, variable);
if (isCollectionConstructor(expression)) {
return (PsiNewExpression)expression;
}
if (VariableAccessUtils.variableIsUsed(variable, statement)) return null;
}
}
private static boolean statementHasSubsequentAddAll(@NotNull PsiElement statement,
@NotNull PsiLocalVariable referent,
@NotNull String previousMethodName) {
@@ -152,7 +152,7 @@ public class CollectionAddAllCanBeReplacedWithConstructorInspection extends Base
return false;
}
private boolean checkLocalVariableAssignmentOrInitializer(PsiExpression initializer) {
private boolean isCollectionConstructor(PsiExpression initializer) {
if (!(initializer instanceof PsiNewExpression)) {
return false;
}
@@ -189,25 +189,6 @@ public class CollectionAddAllCanBeReplacedWithConstructorInspection extends Base
return false;
}
private Pair<Boolean, PsiNewExpression> isProperAssignmentStatementFound(PsiLocalVariable localVariable, PsiMethodCallExpression addAllExpression) {
PsiStatement currentStatement = PsiTreeUtil.getParentOfType(addAllExpression, PsiStatement.class);
final PsiStatement localVariableDefinitionStatement = PsiTreeUtil.getParentOfType(localVariable, PsiStatement.class);
while (currentStatement != null) {
currentStatement = PsiTreeUtil.getPrevSiblingOfType(currentStatement, PsiStatement.class);
if (currentStatement == localVariableDefinitionStatement) {
return Pair.create(true, null);
}
for (PsiAssignmentExpression expression : PsiTreeUtil.findChildrenOfType(currentStatement, PsiAssignmentExpression.class)) {
if (ExpressionUtils.isReferenceTo(expression.getLExpression(), localVariable)) {
final PsiExpression rExpression = expression.getRExpression();
final boolean isValid = checkLocalVariableAssignmentOrInitializer(rExpression);
return Pair.create(isValid, isValid ? (PsiNewExpression)rExpression : null);
}
}
}
return Pair.create(true, null);
}
private static boolean isAddAllReplaceable(final PsiExpression addAllExpression, PsiNewExpression newExpression) {
final boolean[] isReplaceable = new boolean[]{true};
final PsiFile newExpressionContainingFile = newExpression.getContainingFile();
@@ -232,13 +213,13 @@ public class CollectionAddAllCanBeReplacedWithConstructorInspection extends Base
private static class ReplaceAddAllWithConstructorFix implements LocalQuickFix {
private final SmartPsiElementPointer<PsiMethodCallExpression> myMethodCallExpression;
private final SmartPsiElementPointer<PsiNewExpression> myAssignmentExpression;
private final SmartPsiElementPointer<PsiNewExpression> myNewExpression;
private final String methodName;
ReplaceAddAllWithConstructorFix(PsiNewExpression assignmentExpression, PsiMethodCallExpression expression, String methodName) {
final SmartPointerManager smartPointerManager = SmartPointerManager.getInstance(assignmentExpression.getProject());
ReplaceAddAllWithConstructorFix(PsiNewExpression newExpression, PsiMethodCallExpression expression, String methodName) {
final SmartPointerManager smartPointerManager = SmartPointerManager.getInstance(newExpression.getProject());
myMethodCallExpression = smartPointerManager.createSmartPsiElementPointer(expression);
myAssignmentExpression = smartPointerManager.createSmartPsiElementPointer(assignmentExpression);
myNewExpression = smartPointerManager.createSmartPsiElementPointer(newExpression);
this.methodName = methodName;
}
@@ -259,71 +240,46 @@ public class CollectionAddAllCanBeReplacedWithConstructorInspection extends Base
public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) {
final PsiMethodCallExpression methodCallExpression = myMethodCallExpression.getElement();
if (methodCallExpression == null) return;
final PsiElement parameter = methodCallExpression.getArgumentList().getExpressions()[0].copy();
final PsiNewExpression element = myAssignmentExpression.getElement();
if (element == null) return;
final PsiExpressionList constructorArguments = element.getArgumentList();
if (constructorArguments == null) return;
constructorArguments.add(parameter);
methodCallExpression.delete();
}
}
private static List<PsiElement> extractReferencedElementsFromParameter(PsiMethodCallExpression expression) {
final PsiExpression psiExpression = expression.getArgumentList().getExpressions()[0];
final Collection<PsiReferenceExpression> references =
new ArrayList<>(PsiTreeUtil.findChildrenOfType(psiExpression, PsiReferenceExpression.class));
if (psiExpression instanceof PsiReferenceExpression) {
references.add((PsiReferenceExpression)psiExpression);
}
return ContainerUtil.mapNotNull(references, (NullableFunction<PsiReferenceExpression, PsiElement>)expression1 -> expression1.resolve());
}
private static boolean isReferenceToOneOf(PsiReferenceExpression reference, List<PsiElement> elements) {
for (PsiElement element : elements) {
if (reference.isReferenceTo(element)) {
return true;
PsiExpressionStatement expressionStatement = ObjectUtils.tryCast(methodCallExpression.getParent(), PsiExpressionStatement.class);
if (expressionStatement == null) return;
final PsiNewExpression newExpression = myNewExpression.getElement();
if (newExpression == null) return;
PsiElement parent = PsiUtil.skipParenthesizedExprUp(newExpression.getParent());
PsiVariable variable = null;
if (parent instanceof PsiVariable) {
variable = (PsiVariable)parent;
}
}
return false;
}
else if (parent instanceof PsiAssignmentExpression) {
variable = ExpressionUtils.resolveLocalVariable(((PsiAssignmentExpression)parent).getLExpression());
}
if (variable == null) return;
PsiJavaCodeReferenceElement reference = newExpression.getClassReference();
if (reference == null) return;
private static boolean checkUsages(PsiLocalVariable variable,
PsiMethodCallExpression methodCallExpression,
PsiNewExpression variableAssignmentExpression) {
final PsiCodeBlock variableAssignmentBlock = PsiTreeUtil.getParentOfType(variableAssignmentExpression, PsiCodeBlock.class);
final PsiCodeBlock methodCallBlock = PsiTreeUtil.getParentOfType(methodCallExpression, PsiCodeBlock.class);
if (variableAssignmentBlock == null || variableAssignmentBlock != methodCallBlock) {
return false;
}
final PsiStatement variableDeclarationStatement = PsiTreeUtil.getParentOfType(variableAssignmentExpression, PsiStatement.class);
final PsiStatement methodCallStatement = PsiTreeUtil.getParentOfType(methodCallExpression, PsiStatement.class);
if (variableDeclarationStatement == null ||
methodCallStatement == null ||
variableDeclarationStatement.getParent() != methodCallStatement.getParent()) {
return false;
}
PsiElement nextStatement = variableDeclarationStatement;
final List<PsiElement> referencedElementsFromParameter = extractReferencedElementsFromParameter(methodCallExpression);
while (nextStatement != null) {
nextStatement = PsiTreeUtil.getNextSiblingOfType(nextStatement, PsiStatement.class);
if (nextStatement == methodCallStatement) {
return true;
CommentTracker ct = new CommentTracker();
final PsiElement parameter = methodCallExpression.getArgumentList().getExpressions()[0];
String replacement = "new " + reference.getText() + "(" + ct.text(parameter) + ")";
if (parent instanceof PsiAssignmentExpression) {
ct.delete(parent);
}
else {
for (PsiReferenceExpression referenceExpression : PsiTreeUtil.findChildrenOfType(nextStatement, PsiReferenceExpression.class)) {
if (referenceExpression.isReferenceTo(variable) || isReferenceToOneOf(referenceExpression, referencedElementsFromParameter)) {
return false;
}
}
for (PsiLocalVariable localVariable : PsiTreeUtil.findChildrenOfType(nextStatement, PsiLocalVariable.class)) {
if (referencedElementsFromParameter.contains(localVariable)) {
return false;
if (variable.getParent() instanceof PsiDeclarationStatement &&
((PsiDeclarationStatement)variable.getParent()).getDeclaredElements().length == 1) {
PsiElement scope = PsiTreeUtil.getParentOfType(expressionStatement, PsiMember.class, PsiStatement.class, PsiLambdaExpression.class);
if (scope != null &&
ReferencesSearch.search(variable).forEach((PsiReference ref) -> PsiTreeUtil.isAncestor(scope, ref.getElement(), true))) {
PsiDeclarationStatement newDeclaration =
JavaPsiFacade.getElementFactory(project).createVariableDeclarationStatement("x", PsiType.INT, null, methodCallExpression);
PsiVariable newVariable = (PsiVariable)newDeclaration.getDeclaredElements()[0].replace(variable);
ct.delete(variable);
ct.replace(Objects.requireNonNull(newVariable.getInitializer()), replacement);
ct.replaceAndRestoreComments(expressionStatement, newDeclaration);
return;
}
}
ct.delete(newExpression);
}
ct.replaceAndRestoreComments(methodCallExpression, variable.getName() + "=" + replacement);
}
return false;
}
}
@@ -3,6 +3,7 @@ import java.lang.String;
import java.util.ArrayList;
import java.util.List;
import java.util.HashSet;
import java.util.Set;
class C {
void m() {
@@ -10,6 +11,6 @@ class C {
s.add(100);
s.add(101);
s.add(102);
final List<String> strings = new ArrayList<String>(s);
final List<String> strings = new ArrayList<String>(s);
}
}
@@ -0,0 +1,14 @@
// "Replace 'addAll()' call with parametrized constructor call" "true"
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
public class Test {
void test(Object c) {
//noinspection unchecked
/*inside*/
// after
List<String> list = new ArrayList<>((/*cast!*/Collection<String>) c);
System.out.println(list);
}
}
@@ -0,0 +1,13 @@
// "Replace 'addAll()' call with parametrized constructor call" "true"
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
public class Test {
void test(Object c) {
List<String> list, other;
//noinspection unchecked
list = new ArrayList<>((Collection<String>) c);
System.out.println(list);
}
}
@@ -0,0 +1,13 @@
// "Replace 'addAll()' call with parametrized constructor call" "true"
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
public class Test {
void test(Object c) {
List<String> other, list;
//noinspection unchecked
list = new ArrayList<>((Collection<String>) c);
System.out.println(list);
}
}
@@ -0,0 +1,23 @@
// "Replace 'addAll()' call with parametrized constructor call" "true"
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
public class Test {
String foo;
void test() {
foo = "bar";
List<String> list = new ArrayList<>(getSomething());
System.out.println(list);
}
private Collection<String> getSomething() {
return Collections.singleton(foo);
}
public static void main(String[] args) {
new Test().test();
}
}
@@ -0,0 +1,25 @@
// "Replace 'addAll()' call with parametrized constructor call" "true"
import java.util.*;
public class Test {
String foo;
void test() {
final List<String> list;
if(Math.random() > 0) {
foo = "bar";
list = new ArrayList<>(getSomething());
System.out.println(list);
} else {
list = new LinkedList<>();
}
}
private Collection<String> getSomething() {
return Collections.singleton(foo);
}
public static void main(String[] args) {
new Test().test();
}
}
@@ -0,0 +1,26 @@
// "Replace 'addAll()' call with parametrized constructor call" "true"
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
public class Test {
String foo;
void test() {
List<String> list;
synchronized (this) {
foo = "bar";
list = new ArrayList<>(getSomething());
}
System.out.println(list);
}
private Collection<String> getSomething() {
return Collections.singleton(foo);
}
public static void main(String[] args) {
new Test().test();
}
}
@@ -7,6 +7,6 @@ import java.util.Collection;
class C {
void m() {
final Collection<String> strings = new ArrayList<String>(new HashSet<String>());
final Collection<String> strings = new ArrayList<String>(new HashSet<String>());
}
}
@@ -7,6 +7,6 @@ import java.util.HashSet;
class C {
void m() {
final List<String> strings;
strings = new ArrayList<String>(new HashSet<String>());
strings = new ArrayList<String>(new HashSet<String>());
}
}
@@ -8,6 +8,6 @@ import java.util.HashSet;
class C {
void m(String s) {
final List<String> strings;
strings = new ArrayList<String>(Arrays.asList(s, ","));
strings = new ArrayList<String>(Arrays.asList(s, ","));
}
}
@@ -3,6 +3,7 @@ import java.lang.String;
import java.util.ArrayList;
import java.util.List;
import java.util.HashSet;
import java.util.Set;
class C {
void m() {
@@ -0,0 +1,13 @@
// "Replace 'addAll()' call with parametrized constructor call" "true"
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
public class Test {
void test(Object c) {
List<String> list = new ArrayList<>();
//noinspection unchecked
list./*inside*/ad<caret>dAll((/*cast!*/Collection<String>) c); // after
System.out.println(list);
}
}
@@ -0,0 +1,13 @@
// "Replace 'addAll()' call with parametrized constructor call" "true"
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
public class Test {
void test(Object c) {
List<String> list = new ArrayList<>(), other;
//noinspection unchecked
list.ad<caret>dAll((Collection<String>) c);
System.out.println(list);
}
}
@@ -0,0 +1,13 @@
// "Replace 'addAll()' call with parametrized constructor call" "true"
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
public class Test {
void test(Object c) {
List<String> other, list = new ArrayList<>();
//noinspection unchecked
list.ad<caret>dAll((Collection<String>) c);
System.out.println(list);
}
}
@@ -0,0 +1,24 @@
// "Replace 'addAll()' call with parametrized constructor call" "true"
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
public class Test {
String foo;
void test() {
List<String> list = new ArrayList<>();
foo = "bar";
list.a<caret>ddAll(getSomething());
System.out.println(list);
}
private Collection<String> getSomething() {
return Collections.singleton(foo);
}
public static void main(String[] args) {
new Test().test();
}
}
@@ -0,0 +1,26 @@
// "Replace 'addAll()' call with parametrized constructor call" "true"
import java.util.*;
public class Test {
String foo;
void test() {
final List<String> list;
if(Math.random() > 0) {
list = new ArrayList<>();
foo = "bar";
list.a<caret>ddAll(getSomething());
System.out.println(list);
} else {
list = new LinkedList<>();
}
}
private Collection<String> getSomething() {
return Collections.singleton(foo);
}
public static void main(String[] args) {
new Test().test();
}
}
@@ -0,0 +1,26 @@
// "Replace 'addAll()' call with parametrized constructor call" "true"
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
public class Test {
String foo;
void test() {
List<String> list = new ArrayList<>();
synchronized (this) {
foo = "bar";
list.a<caret>ddAll(getSomething());
}
System.out.println(list);
}
private Collection<String> getSomething() {
return Collections.singleton(foo);
}
public static void main(String[] args) {
new Test().test();
}
}
+1 -1
View File
@@ -880,7 +880,7 @@
displayName="Duplicated delimiters in java.util.StringTokenizer"/>
<localInspection groupPath="Java" language="JAVA" shortName="CollectionAddAllCanBeReplacedWithConstructor"
groupBundle="messages.InspectionsBundle"
groupKey="group.names.performance.issues" enabledByDefault="false" level="WARNING"
groupKey="group.names.performance.issues" enabledByDefault="true" level="WARNING"
implementationClass="com.intellij.codeInspection.CollectionAddAllCanBeReplacedWithConstructorInspection"
displayName="Redundant 'Collection.addAll()' call"/>
<localInspection groupPath="Java,Java language level migration aids" language="JAVA" shortName="AnonymousHasLambdaAlternative" displayName="Anonymous type has shorter lambda alternative"