From 4baf8a956146a03c41472caf65e42e7df51054c5 Mon Sep 17 00:00:00 2001 From: Dmitry Batkovich Date: Thu, 2 Apr 2015 17:12:41 +0300 Subject: [PATCH] IDEA-138456 inspection to replace collections.addAll() with properly parametrized constructor. --- ...anBeReplacedWithConstructorInspection.java | 296 ++++++++++++++++++ .../afterAddAllWithReferences.java | 15 + .../afterSimple.java | 12 + .../afterSplitDeclarationAndAssignment.java | 12 + .../beforeAddAllWithReferences.java | 16 + .../beforeAddAllWithReferencesNotShown.java | 16 + .../beforeSimple.java | 13 + .../beforeSplitDeclarationAndAssignment.java | 13 + ...ddAllCanBeReplacedWithConstructorTest.java | 41 +++ ...ionAddAllCanBeReplacedWithConstructor.html | 5 + .../src/messages/QuickFixBundle.properties | 6 +- resources/src/META-INF/IdeaPlugin.xml | 5 + 12 files changed, 449 insertions(+), 1 deletion(-) create mode 100644 java/java-impl/src/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorInspection.java create mode 100644 java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterAddAllWithReferences.java create mode 100644 java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSimple.java create mode 100644 java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSplitDeclarationAndAssignment.java create mode 100644 java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeAddAllWithReferences.java create mode 100644 java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeAddAllWithReferencesNotShown.java create mode 100644 java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeSimple.java create mode 100644 java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeSplitDeclarationAndAssignment.java create mode 100644 java/java-tests/testSrc/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorTest.java create mode 100644 resources-en/src/inspectionDescriptions/CollectionAddAllCanBeReplacedWithConstructor.html diff --git a/java/java-impl/src/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorInspection.java b/java/java-impl/src/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorInspection.java new file mode 100644 index 000000000000..e89b4831bd58 --- /dev/null +++ b/java/java-impl/src/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorInspection.java @@ -0,0 +1,296 @@ +/* + * Copyright 2000-2015 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.QuickFixBundle; +import com.intellij.codeInspection.ui.ListTable; +import com.intellij.codeInspection.ui.ListWrappingTableModel; +import com.intellij.openapi.diagnostic.Logger; +import com.intellij.openapi.project.Project; +import com.intellij.openapi.util.InvalidDataException; +import com.intellij.openapi.util.Pair; +import com.intellij.openapi.util.WriteExternalException; +import com.intellij.psi.*; +import com.intellij.psi.util.InheritanceUtil; +import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.util.NullableFunction; +import com.intellij.util.containers.ContainerUtil; +import com.siyeh.ig.ui.UiUtils; +import org.jdom.Element; +import org.jetbrains.annotations.Nls; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +import javax.swing.*; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; + +/** + * @author Dmitry Batkovich + */ +public class CollectionAddAllCanBeReplacedWithConstructorInspection extends BaseJavaBatchLocalInspectionTool { + private final static Logger LOG = Logger.getInstance(CollectionAddAllCanBeReplacedWithConstructorInspection.class); + + private final List myCollectionClassesToCheck = resetDefault(new ArrayList()); + + @Override + public void readSettings(@NotNull Element node) throws InvalidDataException { + resetDefault(myCollectionClassesToCheck); + for (Element element : node.getChildren()) { + final String classFQN = element.getAttributeValue("name"); + if (element.getAttribute("delete") == null) { + myCollectionClassesToCheck.add(classFQN); + } + else { + myCollectionClassesToCheck.remove(classFQN); + } + } + } + + @Override + public void writeSettings(@NotNull Element node) throws WriteExternalException { + List defaultClasses = resetDefault(new ArrayList()); + for (String aClass : myCollectionClassesToCheck) { + defaultClasses.remove(aClass); + } + for (String aClass : defaultClasses) { + node.addContent(new Element("cls").setAttribute("name", aClass).setAttribute("delete", "true")); + } + defaultClasses = resetDefault(new ArrayList()); + for (String aClass : myCollectionClassesToCheck) { + if (!defaultClasses.contains(aClass)) { + node.addContent(new Element("cls").setAttribute("name", aClass)); + } + } + } + + @Override + @Nullable + public JComponent createOptionsPanel() { + final String title = QuickFixBundle.message("collection.addall.can.be.replaced.with.constructor.fix.options.title"); + final ListTable table = new ListTable(new ListWrappingTableModel(myCollectionClassesToCheck, title)); + return UiUtils.createAddRemoveTreeClassChooserPanel(table, title, CommonClassNames.JAVA_UTIL_COLLECTION); + } + + @NotNull + @Override + public PsiElementVisitor buildVisitor(@NotNull final ProblemsHolder holder, + boolean isOnTheFly, + @NotNull LocalInspectionToolSession session) { + if (myCollectionClassesToCheck.isEmpty()) { + return PsiElementVisitor.EMPTY_VISITOR; + } + return new JavaRecursiveElementWalkingVisitor() { + @Override + public void visitMethodCallExpression(PsiMethodCallExpression expression) { + final String methodName = expression.getMethodExpression().getReferenceName(); + if ("addAll".equals(methodName)) { + if (expression.getArgumentList().getExpressions().length != 1) { + return; + } + final PsiExpression qualifierExpression = expression.getMethodExpression().getQualifierExpression(); + if (!(qualifierExpression instanceof PsiReferenceExpression)) { + 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)) { + return; + } + final PsiClass variableClass = ((PsiClassType)variableType).resolve(); + if (variableClass == null || !InheritanceUtil.isInheritor(variableClass, CommonClassNames.JAVA_UTIL_COLLECTION)) { + return; + } + PsiNewExpression assignmentExpression; + final Pair 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 (!checkUsages(variable, expression, assignmentExpression)) { + return; + } + final PsiMethod method = expression.resolveMethod(); + if (method != null) { + //noinspection DialogTitleCapitalization + holder.registerProblem(expression, QuickFixBundle.message("collection.addall.can.be.replaced.with.constructor.fix.description"), + new ReplaceAddAllWithConstructorFix(assignmentExpression, expression)); + } + } + } + }; + } + + private boolean checkLocalVariableAssignmentOrInitializer(PsiExpression initializer) { + if (!(initializer instanceof PsiNewExpression)) { + return false; + } + final PsiNewExpression newExpression = (PsiNewExpression)initializer; + final PsiJavaCodeReferenceElement classReference = newExpression.getClassReference(); + if (classReference == null) { + return false; + } + final PsiClass initializerClass = (PsiClass)classReference.resolve(); + if (initializerClass == null || !myCollectionClassesToCheck.contains(initializerClass.getQualifiedName())) { + return false; + } + final PsiExpressionList argumentList = newExpression.getArgumentList(); + return argumentList != null && argumentList.getExpressions().length == 0; + } + + private Pair 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)) { + final PsiExpression lExpression = expression.getLExpression(); + if (lExpression instanceof PsiReferenceExpression && ((PsiReferenceExpression)lExpression).isReferenceTo(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 class ReplaceAddAllWithConstructorFix implements LocalQuickFix { + private final SmartPsiElementPointer myMethodCallExpression; + private final SmartPsiElementPointer myAssignmentExpression; + + private ReplaceAddAllWithConstructorFix(PsiNewExpression assignmentExpression, PsiMethodCallExpression expression) { + final SmartPointerManager smartPointerManager = SmartPointerManager.getInstance(assignmentExpression.getProject()); + myMethodCallExpression = smartPointerManager.createSmartPsiElementPointer(expression); + myAssignmentExpression = smartPointerManager.createSmartPsiElementPointer(assignmentExpression); + } + + @Nls + @NotNull + @Override + public String getName() { + return getFamilyName(); + } + + @NotNull + @Override + public String getFamilyName() { + return QuickFixBundle.message("collection.addall.can.be.replaced.with.constructor.fix.title"); + } + + @Override + public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) { + final PsiMethodCallExpression methodCallExpression = myMethodCallExpression.getElement(); + LOG.assertTrue(methodCallExpression != null); + final PsiElement parameter = methodCallExpression.getArgumentList().getExpressions()[0].copy(); + final PsiNewExpression element = myAssignmentExpression.getElement(); + LOG.assertTrue(element != null); + final PsiExpressionList constructorArguments = element.getArgumentList(); + LOG.assertTrue(constructorArguments != null); + constructorArguments.add(parameter); + methodCallExpression.delete(); + } + } + + private static List resetDefault(final List classes) { + classes.clear(); + classes.add(CommonClassNames.JAVA_UTIL_ARRAY_LIST); + classes.add(CommonClassNames.JAVA_UTIL_HASH_SET); + classes.add("java.util.Vector"); + classes.add("java.util.concurrent.CopyOnWriteArrayList"); + return classes; + } + + private static List extractReferencedElementsFromParameter(PsiMethodCallExpression expression) { + final PsiExpression psiExpression = expression.getArgumentList().getExpressions()[0]; + final Collection references = + new ArrayList(PsiTreeUtil.findChildrenOfType(psiExpression, PsiReferenceExpression.class)); + if (psiExpression instanceof PsiReferenceExpression) { + references.add((PsiReferenceExpression)psiExpression); + } + return ContainerUtil.mapNotNull(references, new NullableFunction() { + @Nullable + @Override + public PsiElement fun(PsiReferenceExpression expression) { + return expression.resolve(); + } + }); + } + + private static boolean isReferenceToOneOf(PsiReferenceExpression reference, List elements) { + for (PsiElement element : elements) { + if (reference.isReferenceTo(element)) { + return true; + } + } + return false; + } + + 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 referencedElementsFromParameter = extractReferencedElementsFromParameter(methodCallExpression); + + while (nextStatement != null) { + nextStatement = PsiTreeUtil.getNextSiblingOfType(nextStatement, PsiStatement.class); + if (nextStatement == methodCallStatement) { + return true; + } + 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; + } + } + } + } + return false; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterAddAllWithReferences.java b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterAddAllWithReferences.java new file mode 100644 index 000000000000..9b83f0250de5 --- /dev/null +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterAddAllWithReferences.java @@ -0,0 +1,15 @@ +// "Replace 'addAll()' method with parametrized constructor call" "true" +import java.lang.String; +import java.util.ArrayList; +import java.util.List; +import java.util.HashSet; + +class C { + void m() { + Set s = new HashSet(); + s.add(100); + s.add(101); + s.add(102); + final List strings = new ArrayList(s); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSimple.java b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSimple.java new file mode 100644 index 000000000000..826f5e2017a8 --- /dev/null +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSimple.java @@ -0,0 +1,12 @@ +// "Replace 'addAll()' method with parametrized constructor call" "true" +import java.lang.String; +import java.util.ArrayList; +import java.util.List; +import java.util.HashSet; +import java.util.Collection; + +class C { + void m() { + final Collection strings = new ArrayList(new HashSet()); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSplitDeclarationAndAssignment.java b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSplitDeclarationAndAssignment.java new file mode 100644 index 000000000000..609faa4d1e1a --- /dev/null +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSplitDeclarationAndAssignment.java @@ -0,0 +1,12 @@ +// "Replace 'addAll()' method with parametrized constructor call" "true" +import java.lang.String; +import java.util.ArrayList; +import java.util.List; +import java.util.HashSet; + +class C { + void m() { + final List strings; + strings = new ArrayList(new HashSet()); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeAddAllWithReferences.java b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeAddAllWithReferences.java new file mode 100644 index 000000000000..a1c3c003f0ae --- /dev/null +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeAddAllWithReferences.java @@ -0,0 +1,16 @@ +// "Replace 'addAll()' method with parametrized constructor call" "true" +import java.lang.String; +import java.util.ArrayList; +import java.util.List; +import java.util.HashSet; + +class C { + void m() { + Set s = new HashSet(); + s.add(100); + s.add(101); + s.add(102); + final List strings = new ArrayList(); + strings.addAll(s); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeAddAllWithReferencesNotShown.java b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeAddAllWithReferencesNotShown.java new file mode 100644 index 000000000000..f8b94c91e2f3 --- /dev/null +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeAddAllWithReferencesNotShown.java @@ -0,0 +1,16 @@ +// "Replace 'addAll()' method with parametrized constructor call" "false" +import java.lang.String; +import java.util.ArrayList; +import java.util.List; +import java.util.HashSet; + +class C { + void m() { + final List strings = new ArrayList(); + Set s = new HashSet(); + s.add(100); + s.add(101); + s.add(102); + strings.addAll(s); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeSimple.java b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeSimple.java new file mode 100644 index 000000000000..1766c44975ed --- /dev/null +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeSimple.java @@ -0,0 +1,13 @@ +// "Replace 'addAll()' method with parametrized constructor call" "true" +import java.lang.String; +import java.util.ArrayList; +import java.util.List; +import java.util.HashSet; +import java.util.Collection; + +class C { + void m() { + final Collection strings = new ArrayList(); + strings.addAll(new HashSet()); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeSplitDeclarationAndAssignment.java b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeSplitDeclarationAndAssignment.java new file mode 100644 index 000000000000..bde244689076 --- /dev/null +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeSplitDeclarationAndAssignment.java @@ -0,0 +1,13 @@ +// "Replace 'addAll()' method with parametrized constructor call" "true" +import java.lang.String; +import java.util.ArrayList; +import java.util.List; +import java.util.HashSet; + +class C { + void m() { + final List strings; + strings = new ArrayList(); + strings.addAll(new HashSet()); + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorTest.java b/java/java-tests/testSrc/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorTest.java new file mode 100644 index 000000000000..03abd6009b7f --- /dev/null +++ b/java/java-tests/testSrc/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorTest.java @@ -0,0 +1,41 @@ +/* + * Copyright 2000-2015 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 Dmitry Batkovich + */ +public class CollectionAddAllCanBeReplacedWithConstructorTest extends LightQuickFixParameterizedTestCase { + + @NotNull + @Override + protected LocalInspectionTool[] configureLocalInspectionTools() { + return new LocalInspectionTool[]{ + new CollectionAddAllCanBeReplacedWithConstructorInspection(), + }; + } + + public void test() throws Exception { doAllTests(); } + + @Override + protected String getBasePath() { + return "/inspection/collectionAddAllCanBeReplacedWithConstructor"; + } + +} diff --git a/resources-en/src/inspectionDescriptions/CollectionAddAllCanBeReplacedWithConstructor.html b/resources-en/src/inspectionDescriptions/CollectionAddAllCanBeReplacedWithConstructor.html new file mode 100644 index 000000000000..47efe4b81f23 --- /dev/null +++ b/resources-en/src/inspectionDescriptions/CollectionAddAllCanBeReplacedWithConstructor.html @@ -0,0 +1,5 @@ + + +Inspection reports usages of Collection.addAll() method after instantiation of object using parameter-less constructor. + + \ No newline at end of file diff --git a/resources-en/src/messages/QuickFixBundle.properties b/resources-en/src/messages/QuickFixBundle.properties index 5a14eea46721..ad9637397503 100644 --- a/resources-en/src/messages/QuickFixBundle.properties +++ b/resources-en/src/messages/QuickFixBundle.properties @@ -273,4 +273,8 @@ annotations.fix=Annotations add.missing.annotation.parameters.fix=Add missing annotation parameters - {0} add.missing.annotation.single.parameter.fix=Add missing annotation parameter ''{0}'' -add.method.qualifier.fix.text=Add qualifier {0} to method \ No newline at end of file +add.method.qualifier.fix.text=Add qualifier {0} to method + +collection.addall.can.be.replaced.with.constructor.fix.options.title=Classes to check +collection.addall.can.be.replaced.with.constructor.fix.description='addAll()' method can be replaced with parametrized constructor +collection.addall.can.be.replaced.with.constructor.fix.title=Replace 'addAll()' method with parametrized constructor call \ No newline at end of file diff --git a/resources/src/META-INF/IdeaPlugin.xml b/resources/src/META-INF/IdeaPlugin.xml index 523ca13801b5..aa814ab8d713 100644 --- a/resources/src/META-INF/IdeaPlugin.xml +++ b/resources/src/META-INF/IdeaPlugin.xml @@ -728,6 +728,11 @@ groupName="Probable bugs" enabledByDefault="true" level="WARNING" implementationClass="com.intellij.codeInspection.StringTokenizerDelimiterInspection" displayName="Duplicated delimiters in java.util.StringTokenizer"/> + com.intellij.codeInsight.daemon.quickFix.RedundantLambdaParameterTypeIntention