From fb5bf4994b6f6711ad90c0cb136fcf70e5450350 Mon Sep 17 00:00:00 2001 From: Bas Leijdekkers Date: Tue, 12 Sep 2017 13:11:07 +0200 Subject: [PATCH] improve inspection wording, description and highlighting --- ...anBeReplacedWithConstructorInspection.java | 117 ++++++++++-------- .../afterAddAllWithReferences.java | 2 +- .../afterSimple.java | 2 +- .../afterSplitDeclarationAndAssignment.java | 2 +- .../afterUseParameter.java | 2 +- .../beforeAddAllWithReferences.java | 4 +- .../beforeSimple.java | 4 +- .../beforeSplitDeclarationAndAssignment.java | 4 +- .../beforeUseParameter.java | 4 +- ...ionAddAllCanBeReplacedWithConstructor.html | 12 +- .../src/messages/QuickFixBundle.properties | 5 +- resources/src/META-INF/IdeaPlugin.xml | 2 +- 12 files changed, 91 insertions(+), 69 deletions(-) diff --git a/java/java-impl/src/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorInspection.java b/java/java-impl/src/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorInspection.java index 941b3a5a5fc2..4e6ebedd9574 100644 --- a/java/java-impl/src/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/CollectionAddAllCanBeReplacedWithConstructorInspection.java @@ -1,5 +1,5 @@ /* - * Copyright 2000-2016 JetBrains s.r.o. + * 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. @@ -16,7 +16,6 @@ package com.intellij.codeInspection; import com.intellij.codeInsight.daemon.QuickFixBundle; -import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.project.Project; import com.intellij.openapi.util.InvalidDataException; import com.intellij.openapi.util.Pair; @@ -31,6 +30,7 @@ import com.intellij.util.containers.ContainerUtil; import com.siyeh.ig.performance.CollectionsListSettings; import com.siyeh.ig.psiutils.ExpressionUtils; import org.jdom.Element; +import org.jetbrains.annotations.Nls; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -43,7 +43,6 @@ import java.util.List; * @author Dmitry Batkovich */ public class CollectionAddAllCanBeReplacedWithConstructorInspection extends BaseJavaBatchLocalInspectionTool { - private final static Logger LOG = Logger.getInstance(CollectionAddAllCanBeReplacedWithConstructorInspection.class); private final CollectionsListSettings mySettings = new CollectionsListSettings() { @Override @@ -76,55 +75,58 @@ public class CollectionAddAllCanBeReplacedWithConstructorInspection extends Base return new JavaElementVisitor() { @Override public void visitMethodCallExpression(PsiMethodCallExpression expression) { - final String methodName = expression.getMethodExpression().getReferenceName(); - if ("addAll".equals(methodName) || "putAll".equals(methodName)) { - if (expression.getArgumentList().getExpressions().length != 1) { - return; - } - final PsiExpression qualifierExpression = expression.getMethodExpression().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) { - 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; - } + 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) { + 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 (!isAddAllReplaceable(expression, assignmentExpression) || !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", methodName), - new ReplaceAddAllWithConstructorFix(assignmentExpression, expression)); } + } else { + return; + } + if (!isAddAllReplaceable(expression, assignmentExpression) || !checkUsages(variable, expression, assignmentExpression)) { + return; + } + final PsiMethod method = expression.resolveMethod(); + if (method != null) { + //noinspection DialogTitleCapitalization + holder.registerProblem(nameElement, QuickFixBundle.message("collection.addall.can.be.replaced.with.constructor.fix.description"), + new ReplaceAddAllWithConstructorFix(assignmentExpression, expression, methodName)); } } }; @@ -169,7 +171,7 @@ public class CollectionAddAllCanBeReplacedWithConstructorInspection extends Base return argumentList != null && argumentList.getExpressions().length == 0; } - private boolean hasProperConstructor(PsiClass psiClass) { + private static boolean hasProperConstructor(PsiClass psiClass) { for (PsiMethod psiMethod : psiClass.getConstructors()) { PsiParameterList parameterList = psiMethod.getParameterList(); if(parameterList.getParametersCount() == 1) { @@ -231,17 +233,26 @@ public class CollectionAddAllCanBeReplacedWithConstructorInspection extends Base private static class ReplaceAddAllWithConstructorFix implements LocalQuickFix { private final SmartPsiElementPointer myMethodCallExpression; private final SmartPsiElementPointer myAssignmentExpression; + private final String methodName; - private ReplaceAddAllWithConstructorFix(PsiNewExpression assignmentExpression, PsiMethodCallExpression expression) { + ReplaceAddAllWithConstructorFix(PsiNewExpression assignmentExpression, PsiMethodCallExpression expression, String methodName) { final SmartPointerManager smartPointerManager = SmartPointerManager.getInstance(assignmentExpression.getProject()); myMethodCallExpression = smartPointerManager.createSmartPsiElementPointer(expression); myAssignmentExpression = smartPointerManager.createSmartPsiElementPointer(assignmentExpression); + this.methodName = methodName; + } + + @Nls + @NotNull + @Override + public String getName() { + return QuickFixBundle.message("collection.addall.can.be.replaced.with.constructor.fix.name", methodName); } @NotNull @Override public String getFamilyName() { - return QuickFixBundle.message("collection.addall.can.be.replaced.with.constructor.fix.title"); + return QuickFixBundle.message("collection.addall.can.be.replaced.with.constructor.fix.family.name"); } @Override diff --git a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterAddAllWithReferences.java b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterAddAllWithReferences.java index 3f55ef093c49..e52d610ba50e 100644 --- a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterAddAllWithReferences.java +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterAddAllWithReferences.java @@ -1,4 +1,4 @@ -// "Replace 'addAll/putAll' method with parametrized constructor call" "true" +// "Replace 'addAll()' call with parametrized constructor call" "true" import java.lang.String; import java.util.ArrayList; import java.util.List; diff --git a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSimple.java b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSimple.java index 209e0bb9e564..194df6bd5773 100644 --- a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSimple.java +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSimple.java @@ -1,4 +1,4 @@ -// "Replace 'addAll/putAll' method with parametrized constructor call" "true" +// "Replace 'addAll()' call with parametrized constructor call" "true" import java.lang.String; import java.util.ArrayList; import java.util.List; diff --git a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSplitDeclarationAndAssignment.java b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSplitDeclarationAndAssignment.java index 22e7435b4330..03b735f0c92f 100644 --- a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSplitDeclarationAndAssignment.java +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterSplitDeclarationAndAssignment.java @@ -1,4 +1,4 @@ -// "Replace 'addAll/putAll' method with parametrized constructor call" "true" +// "Replace 'addAll()' call with parametrized constructor call" "true" import java.lang.String; import java.util.ArrayList; import java.util.List; diff --git a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterUseParameter.java b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterUseParameter.java index c81c64a5346e..3866753bb67d 100644 --- a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterUseParameter.java +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/afterUseParameter.java @@ -1,4 +1,4 @@ -// "Replace 'addAll/putAll' method with parametrized constructor call" "true" +// "Replace 'addAll()' call with parametrized constructor call" "true" import java.lang.String; import java.util.ArrayList; import java.util.Arrays; diff --git a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeAddAllWithReferences.java b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeAddAllWithReferences.java index 7f018bdf1bce..2a9adba08093 100644 --- a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeAddAllWithReferences.java +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeAddAllWithReferences.java @@ -1,4 +1,4 @@ -// "Replace 'addAll/putAll' method with parametrized constructor call" "true" +// "Replace 'addAll()' call with parametrized constructor call" "true" import java.lang.String; import java.util.ArrayList; import java.util.List; @@ -11,6 +11,6 @@ class C { s.add(101); s.add(102); final List strings = new ArrayList(); - strings.addAll(s); + 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 index 4ee892c25563..48dc95556662 100644 --- a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeSimple.java +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeSimple.java @@ -1,4 +1,4 @@ -// "Replace 'addAll/putAll' method with parametrized constructor call" "true" +// "Replace 'addAll()' call with parametrized constructor call" "true" import java.lang.String; import java.util.ArrayList; import java.util.List; @@ -8,6 +8,6 @@ import java.util.Collection; class C { void m() { final Collection strings = new ArrayList(); - strings.addAll(new HashSet()); + 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 index 76b9fc2efa64..4326503e12a8 100644 --- a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeSplitDeclarationAndAssignment.java +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeSplitDeclarationAndAssignment.java @@ -1,4 +1,4 @@ -// "Replace 'addAll/putAll' method with parametrized constructor call" "true" +// "Replace 'addAll()' call with parametrized constructor call" "true" import java.lang.String; import java.util.ArrayList; import java.util.List; @@ -8,6 +8,6 @@ class C { void m() { final List strings; strings = new ArrayList(); - strings.addAll(new HashSet()); + strings.addAll(new HashSet()); } } \ No newline at end of file diff --git a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeUseParameter.java b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeUseParameter.java index 0cdace362abd..4458864c6e0c 100644 --- a/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeUseParameter.java +++ b/java/java-tests/testData/inspection/collectionAddAllCanBeReplacedWithConstructor/beforeUseParameter.java @@ -1,4 +1,4 @@ -// "Replace 'addAll/putAll' method with parametrized constructor call" "true" +// "Replace 'addAll()' call with parametrized constructor call" "true" import java.lang.String; import java.util.ArrayList; import java.util.Arrays; @@ -9,6 +9,6 @@ class C { void m(String s) { final List strings; strings = new ArrayList(); - strings.addAll(Arrays.asList(s, ",")); + strings.addAll(Arrays.asList(s, ",")); } } \ No newline at end of file diff --git a/resources-en/src/inspectionDescriptions/CollectionAddAllCanBeReplacedWithConstructor.html b/resources-en/src/inspectionDescriptions/CollectionAddAllCanBeReplacedWithConstructor.html index 25c8551cb7d5..4cb019b6ef31 100644 --- a/resources-en/src/inspectionDescriptions/CollectionAddAllCanBeReplacedWithConstructor.html +++ b/resources-en/src/inspectionDescriptions/CollectionAddAllCanBeReplacedWithConstructor.html @@ -1,5 +1,15 @@ -Inspection reports usages of Collection.addAll() and Map.putAll() methods after instantiation of object using parameter-less constructor. +Reports Collection.addAll() and Map.putAll() calls after instantiation of a collection using a constructor call without arguments. +Such constructs can be replaced with a single call to a parametrized constructor. +For example: +

+  Set<String> set = new HashSet<>();
+  set.addAll(Arrays.asList("alpha", "beta", "gamma"));
+
+can be replaced with: +

+  Set<String> set = new HashSet<>(Arrays.asList("alpha", "beta", "gamma"));
+
\ No newline at end of file diff --git a/resources-en/src/messages/QuickFixBundle.properties b/resources-en/src/messages/QuickFixBundle.properties index 0c7be0f97025..e764339a7d74 100644 --- a/resources-en/src/messages/QuickFixBundle.properties +++ b/resources-en/src/messages/QuickFixBundle.properties @@ -277,8 +277,9 @@ add.method.qualifier.fix.text=Add qualifier ''{0}'' to method add.method.qualifier.fix.family=Add method qualifier collection.addall.can.be.replaced.with.constructor.fix.options.title=Classes to check -collection.addall.can.be.replaced.with.constructor.fix.description=''{0}()'' method can be replaced with parametrized constructor -collection.addall.can.be.replaced.with.constructor.fix.title=Replace 'addAll/putAll' method with parametrized constructor call +collection.addall.can.be.replaced.with.constructor.fix.description='#ref()' call can be replaced with parametrized constructor call +collection.addall.can.be.replaced.with.constructor.fix.family.name=Replace 'addAll()/putAll()' call with parametrized constructor call +collection.addall.can.be.replaced.with.constructor.fix.name=Replace ''{0}()'' call with parametrized constructor call add.exception.from.field.initializer.to.constructor.throws.text=Add exception to class {0, choice, 0#default constructor|1#constructor|2#constructors} signature add.exception.from.field.initializer.to.constructor.throws.family.text=Add exception to class constructors signature diff --git a/resources/src/META-INF/IdeaPlugin.xml b/resources/src/META-INF/IdeaPlugin.xml index b5f57783277a..7cff6f1fc19d 100644 --- a/resources/src/META-INF/IdeaPlugin.xml +++ b/resources/src/META-INF/IdeaPlugin.xml @@ -873,7 +873,7 @@ groupBundle="messages.InspectionsBundle" groupKey="group.names.performance.issues" enabledByDefault="false" level="WARNING" implementationClass="com.intellij.codeInspection.CollectionAddAllCanBeReplacedWithConstructorInspection" - displayName="Collection.addAll() can be replaced with parametrized constructor"/> + displayName="Redundant 'Collection.addAll()' call"/>