IDEA-184862 Detect dangerous toArray with size calls

Now ToArrayCallWithZeroLengthArrayArgumentInspection may warn in both ways suggesting either to convert pre-sized array to empty or vice versa

plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/InspectionGadgetsBundle.properties

@@ -1585,9 +1585,12 @@ new.string.buffer.with.char.argument.quickfix=Replace char argument with String
 suspicious.comparator.compare.display.name=Suspicious 'Comparator.compare()' implementation
 suspicious.comparator.compare.descriptor.parameter.not.used='compare()' parameter <code>#ref</code> is not used #loc
 suspicious.comparator.compare.descriptor.non.reflexive=Comparator does not return 0 for equal elements
-to.array.call.with.zero.length.array.argument.display.name=Call to 'Collection.toArray()' with zero-length array argument
-to.array.call.with.zero.length.array.argument.problem.descriptor=Call to <code>#ref()</code> with zero-length array argument ''{0}'' #loc
-to.array.call.with.zero.length.array.argument.quickfix=Replace argument with correctly sized array
+to.array.call.style.display.name='Collection.toArray()' call style
+to.array.call.style.problem.descriptor.zero=Call to <code>#ref()</code> with empty array argument ''{0}'' #loc
+to.array.call.style.problem.descriptor.presized=Call to <code>#ref()</code> with pre-sized array argument ''{0}'' #loc
+to.array.call.style.quickfix.family.name=Fix size of the array passed to 'toArray' call
+to.array.call.style.quickfix.make.presized=Replace argument with pre-sized array
+to.array.call.style.quickfix.make.zero=Replace argument with empty array
 throwable.instance.never.thrown.runtime.exception.problem.descriptor=Runtime exception instance <code>#ref</code> is not thrown #loc
 throwable.instance.never.thrown.checked.exception.problem.descriptor=Checked exception instance <code>#ref</code> is not thrown #loc
 throwable.instance.never.thrown.error.problem.descriptor=Error instance <code>#ref</code> is not thrown #loc

plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/performance/ToArrayCallWithZeroLengthArrayArgumentInspectionBase.java

@@ -1,100 +0,0 @@
-/*
- * 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.siyeh.ig.performance;
-
-import com.intellij.psi.*;
-import com.intellij.psi.util.InheritanceUtil;
-import com.siyeh.InspectionGadgetsBundle;
-import com.siyeh.ig.BaseInspection;
-import com.siyeh.ig.BaseInspectionVisitor;
-import com.siyeh.ig.psiutils.CollectionUtils;
-import com.siyeh.ig.psiutils.ConstructionUtils;
-import org.jetbrains.annotations.Nls;
-import org.jetbrains.annotations.NonNls;
-import org.jetbrains.annotations.NotNull;
-
-public class ToArrayCallWithZeroLengthArrayArgumentInspectionBase extends BaseInspection {
-  @Override
-  @Nls
-  @NotNull
-  public String getDisplayName() {
-    return InspectionGadgetsBundle.message(
-      "to.array.call.with.zero.length.array.argument.display.name");
-  }
-
-  @Override
-  @NotNull
-  protected String buildErrorString(Object... infos) {
-    final PsiExpression argument = (PsiExpression)infos[1];
-    return InspectionGadgetsBundle.message(
-      "to.array.call.with.zero.length.array.argument.problem.descriptor",
-      argument.getText());
-  }
-
-  @Override
-  public BaseInspectionVisitor buildVisitor() {
-    return new ToArrayCallWithZeroLengthArrayArgument();
-  }
-
-  private static class ToArrayCallWithZeroLengthArrayArgument extends BaseInspectionVisitor {
-
-    @Override
-    public void visitMethodCallExpression(PsiMethodCallExpression expression) {
-      super.visitMethodCallExpression(expression);
-      final PsiReferenceExpression methodExpression = expression.getMethodExpression();
-      @NonNls final String methodName = methodExpression.getReferenceName();
-      if (!"toArray".equals(methodName)) {
-        return;
-      }
-      final PsiExpressionList argumentList = expression.getArgumentList();
-      final PsiExpression[] arguments = argumentList.getExpressions();
-      if (arguments.length != 1) {
-        return;
-      }
-      final PsiExpression argument = arguments[0];
-      final PsiType type = argument.getType();
-      if (!(type instanceof PsiArrayType)) {
-        return;
-      }
-      if (type.getArrayDimensions() != 1) {
-        return;
-      }
-      if (argument instanceof PsiReferenceExpression) {
-        final PsiReferenceExpression referenceExpression = (PsiReferenceExpression)argument;
-        final PsiElement element = referenceExpression.resolve();
-        if (!(element instanceof PsiField)) {
-          return;
-        }
-        final PsiField field = (PsiField)element;
-        if (!CollectionUtils.isConstantEmptyArray(field)) {
-          return;
-        }
-      }
-      else if (!ConstructionUtils.isEmptyArrayInitializer(argument)) {
-        return;
-      }
-      final PsiMethod method = expression.resolveMethod();
-      if (method == null) {
-        return;
-      }
-      final PsiClass containingClass = method.getContainingClass();
-      if (!InheritanceUtil.isInheritor(containingClass, CommonClassNames.JAVA_UTIL_COLLECTION)) {
-        return;
-      }
-      registerMethodCallError(expression, expression, argument);
-    }
-  }
-}

plugins/InspectionGadgets/src/META-INF/InspectionGadgets.xml

@@ -2033,8 +2033,8 @@
                      groupBundle="messages.InspectionsBundle" groupKey="group.names.performance.issues" enabledByDefault="true"
                      level="INFORMATION" implementationClass="com.siyeh.ig.performance.TailRecursionInspection"/>
     <localInspection groupPath="Java" language="JAVA" shortName="ToArrayCallWithZeroLengthArrayArgument" bundle="com.siyeh.InspectionGadgetsBundle"
-                     key="to.array.call.with.zero.length.array.argument.display.name" groupBundle="messages.InspectionsBundle"
-                     groupKey="group.names.performance.issues" enabledByDefault="false" level="WARNING"
+                     key="to.array.call.style.display.name" groupBundle="messages.InspectionsBundle"
+                     groupKey="group.names.performance.issues" enabledByDefault="true" level="WARNING"
                      implementationClass="com.siyeh.ig.performance.ToArrayCallWithZeroLengthArrayArgumentInspection"/>
     <localInspection groupPath="Java" language="JAVA" suppressId="ConcatenationWithEmptyString" shortName="TrivialStringConcatenation"
                      bundle="com.siyeh.InspectionGadgetsBundle" key="trivial.string.concatenation.display.name"

plugins/InspectionGadgets/src/com/siyeh/ig/performance/ToArrayCallWithZeroLengthArrayArgumentInspection.java

@@ -15,33 +15,194 @@
  */
 package com.siyeh.ig.performance;
 
+import com.intellij.codeInsight.PsiEquivalenceUtil;
 import com.intellij.codeInspection.ProblemDescriptor;
 import com.intellij.openapi.project.Project;
+import com.intellij.openapi.ui.VerticalFlowLayout;
 import com.intellij.psi.*;
+import com.intellij.psi.util.InheritanceUtil;
 import com.intellij.psi.util.PsiTreeUtil;
+import com.intellij.psi.util.PsiUtil;
 import com.intellij.util.IncorrectOperationException;
+import com.intellij.util.ObjectUtils;
 import com.siyeh.InspectionGadgetsBundle;
+import com.siyeh.ig.BaseInspection;
+import com.siyeh.ig.BaseInspectionVisitor;
 import com.siyeh.ig.InspectionGadgetsFix;
-import com.siyeh.ig.PsiReplacementUtil;
-import com.siyeh.ig.psiutils.HighlightUtils;
-import org.jetbrains.annotations.NonNls;
-import org.jetbrains.annotations.NotNull;
-import org.jetbrains.annotations.Nullable;
+import com.siyeh.ig.callMatcher.CallMatcher;
+import com.siyeh.ig.psiutils.*;
+import one.util.streamex.StreamEx;
+import org.jdom.Element;
+import org.jetbrains.annotations.*;
 
-public class ToArrayCallWithZeroLengthArrayArgumentInspection extends ToArrayCallWithZeroLengthArrayArgumentInspectionBase {
+import javax.swing.*;
+
+public class ToArrayCallWithZeroLengthArrayArgumentInspection extends BaseInspection {
+  private static final CallMatcher COLLECTION_SIZE =
+    CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_COLLECTION, "size").parameterCount(0);
+  private static final String PREFER_EMPTY_ARRAY_SETTING = "PreferEmptyArray";
+
+  public enum PreferEmptyArray {
+    ALWAYS("Always"), BY_LEVEL("According to language level"), NEVER("Never (prefer pre-sized array)");
+
+    private final String myMessage;
+
+    PreferEmptyArray(String message) { myMessage = message; }
+
+    String getMessage() { return myMessage; }
+
+    boolean isEmptyPreferred(PsiExpression expression) {
+      switch (this) {
+        case ALWAYS:
+          return true;
+        case NEVER:
+          return false;
+        default:
+          return PsiUtil.isLanguageLevel7OrHigher(expression);
+      }
+    }
+
+    @NotNull
+    static PreferEmptyArray from(String name) {
+      return StreamEx.of(values()).filterBy(PreferEmptyArray::name, name).findFirst().orElse(ALWAYS);
+    }
+  }
+
+  @NotNull
+  public PreferEmptyArray myMode = PreferEmptyArray.ALWAYS;
+
+  @Nullable
+  @Override
+  public JComponent createOptionsPanel() {
+    final JPanel panel = new JPanel(new VerticalFlowLayout(VerticalFlowLayout.TOP, 0, 5, true, false));
+    panel.add(new JLabel("Prefer empty array:"));
+
+    ButtonGroup group = new ButtonGroup();
+    for (PreferEmptyArray mode : PreferEmptyArray.values()) {
+      JRadioButton radioButton = new JRadioButton(mode.getMessage(), mode == myMode);
+      radioButton.addActionListener(e -> myMode = mode);
+      panel.add(radioButton);
+      group.add(radioButton);
+    }
+
+    return panel;
+  }
 
   @Override
   @Nullable
   protected InspectionGadgetsFix buildFix(Object... infos) {
-    return new ToArrayCallWithZeroLengthArrayArgumentFix();
+    final PsiExpression argument = (PsiExpression)infos[1];
+    return new ToArrayCallWithZeroLengthArrayArgumentFix(myMode.isEmptyPreferred(argument));
+  }
+
+  @Override
+  @Nls
+  @NotNull
+  public String getDisplayName() {
+    return InspectionGadgetsBundle.message("to.array.call.style.display.name");
+  }
+
+  @Override
+  public void readSettings(@NotNull Element node) {
+    Element element = node.getChild(PREFER_EMPTY_ARRAY_SETTING);
+    if (element != null) {
+      myMode = PreferEmptyArray.from(element.getAttributeValue("value"));
+    }
+  }
+
+  @Override
+  public void writeSettings(@NotNull Element node) {
+    Element element = new Element(PREFER_EMPTY_ARRAY_SETTING);
+    element.setAttribute("value", myMode.toString());
+    node.addContent(element);
+  }
+
+  @Override
+  @NotNull
+  protected String buildErrorString(Object... infos) {
+    final PsiExpression argument = (PsiExpression)infos[1];
+    return myMode.isEmptyPreferred(argument) ?
+           InspectionGadgetsBundle.message("to.array.call.style.problem.descriptor.presized", argument.getText()) :
+           InspectionGadgetsBundle.message("to.array.call.style.problem.descriptor.zero", argument.getText());
+  }
+
+  @Override
+  public BaseInspectionVisitor buildVisitor() {
+    return new BaseInspectionVisitor() {
+      @Override
+      public void visitMethodCallExpression(PsiMethodCallExpression expression) {
+        super.visitMethodCallExpression(expression);
+        final PsiReferenceExpression methodExpression = expression.getMethodExpression();
+        @NonNls final String methodName = methodExpression.getReferenceName();
+        if (!"toArray".equals(methodName)) return;
+        final PsiExpressionList argumentList = expression.getArgumentList();
+        final PsiExpression[] arguments = argumentList.getExpressions();
+        if (arguments.length != 1) return;
+        final PsiExpression argument = arguments[0];
+        final PsiType type = argument.getType();
+        if (!(type instanceof PsiArrayType)) return;
+        final PsiMethod method = expression.resolveMethod();
+        if (method == null) return;
+        final PsiClass containingClass = method.getContainingClass();
+        if (!InheritanceUtil.isInheritor(containingClass, CommonClassNames.JAVA_UTIL_COLLECTION)) return;
+        if (type.getArrayDimensions() != 1) return;
+
+        boolean wrongArray =
+          myMode.isEmptyPreferred(argument)
+          ? isPresizedArray(argument, methodExpression.getQualifierExpression())
+          : isEmptyArray(argument);
+        if (wrongArray) {
+          registerMethodCallError(expression, expression, argument);
+        }
+      }
+    };
+  }
+
+  private static boolean isEmptyArray(@Nullable PsiExpression argument) {
+    if (argument instanceof PsiReferenceExpression) {
+      final PsiElement element = ((PsiReferenceExpression)argument).resolve();
+      if (!(element instanceof PsiField)) return false;
+      return CollectionUtils.isConstantEmptyArray((PsiField)element);
+    }
+    return ConstructionUtils.isEmptyArrayInitializer(argument);
+  }
+
+  @Contract("_, null -> false")
+  private static boolean isPresizedArray(@Nullable PsiExpression argument, @Nullable PsiExpression qualifier) {
+    if (qualifier == null) return false;
+    PsiNewExpression newExpression = ObjectUtils.tryCast(PsiUtil.skipParenthesizedExprDown(argument), PsiNewExpression.class);
+    if (newExpression == null) return false;
+    PsiExpression[] dimensions = newExpression.getArrayDimensions();
+    if (dimensions.length != 1) return false;
+    PsiMethodCallExpression maybeSizeCall =
+      ObjectUtils.tryCast(PsiUtil.skipParenthesizedExprDown(dimensions[0]), PsiMethodCallExpression.class);
+    if (COLLECTION_SIZE.test(maybeSizeCall)) {
+      PsiExpression sizeQualifier = maybeSizeCall.getMethodExpression().getQualifierExpression();
+      return sizeQualifier != null && PsiEquivalenceUtil.areElementsEquivalent(sizeQualifier, qualifier);
+    }
+    return false;
   }
 
   private static class ToArrayCallWithZeroLengthArrayArgumentFix extends InspectionGadgetsFix {
+    private boolean myEmptyPreferred;
+
+    public ToArrayCallWithZeroLengthArrayArgumentFix(boolean emptyPreferred) {
+      myEmptyPreferred = emptyPreferred;
+    }
+
+    @Nls
+    @NotNull
+    @Override
+    public String getName() {
+      return myEmptyPreferred ?
+             InspectionGadgetsBundle.message("to.array.call.style.quickfix.make.zero") :
+             InspectionGadgetsBundle.message("to.array.call.style.quickfix.make.presized");
+    }
 
     @Override
     @NotNull
     public String getFamilyName() {
-      return InspectionGadgetsBundle.message("to.array.call.with.zero.length.array.argument.quickfix");
+      return InspectionGadgetsBundle.message("to.array.call.style.quickfix.family.name");
     }
 
     @Override
@@ -49,44 +210,36 @@ public class ToArrayCallWithZeroLengthArrayArgumentInspection extends ToArrayCal
       final PsiElement element = descriptor.getPsiElement();
       final PsiElement parent = element.getParent();
       final PsiElement grandParent = parent.getParent();
-      if (!(grandParent instanceof PsiMethodCallExpression)) {
-        return;
-      }
+      if (!(grandParent instanceof PsiMethodCallExpression)) return;
       final PsiMethodCallExpression methodCallExpression = (PsiMethodCallExpression)grandParent;
       final PsiReferenceExpression methodExpression = methodCallExpression.getMethodExpression();
       final PsiExpression qualifier = methodExpression.getQualifierExpression();
       final PsiElementFactory factory = JavaPsiFacade.getElementFactory(project);
       final PsiExpressionList argumentList = methodCallExpression.getArgumentList();
       final PsiExpression[] arguments = argumentList.getExpressions();
-      if (arguments.length != 1) {
-        return;
-      }
+      if (arguments.length != 1) return;
       final PsiExpression argument = arguments[0];
-      if (qualifier == null) {
-        return;
-      }
+      if (qualifier == null) return;
+
       final String collectionText = qualifier.getText();
       final PsiType type = argument.getType();
-      if (type == null) {
-        return;
-      }
+      if (type == null) return;
       final PsiType componentType = type.getDeepComponentType();
       final String typeText = componentType.getCanonicalText();
-      if (!(qualifier instanceof PsiMethodCallExpression)) {
-        @NonNls final String replacementText = "new " + typeText + '[' + collectionText + ".size()]";
-        final String newExpressionText = PsiReplacementUtil.getElementText(methodCallExpression, argument, replacementText);
-        PsiReplacementUtil.replaceExpression(methodCallExpression, newExpressionText);
+
+
+      if (myEmptyPreferred || ExpressionUtils.isSimpleExpression(qualifier)) {
+        CommentTracker ct = new CommentTracker();
+        String sizeClause = myEmptyPreferred ? "0" : collectionText + ".size()";
+        @NonNls final String replacementText = "new " + typeText + '[' + sizeClause + "]";
+        ct.replaceAndRestoreComments(argument, replacementText);
         return;
       }
       // need to introduce a variable to prevent calling a method twice
       PsiStatement statement = PsiTreeUtil.getParentOfType(methodCallExpression, PsiStatement.class);
-      if (statement == null) {
-        return;
-      }
+      if (statement == null) return;
       final PsiType qualifierType = qualifier.getType();
-      if (qualifierType == null) {
-        return;
-      }
+      if (qualifierType == null) return;
       PsiDeclarationStatement declarationStatement = factory.createVariableDeclarationStatement("var", qualifierType, qualifier);
       PsiElement statementParent = statement.getParent();
       while (statementParent instanceof PsiLoopStatement || statementParent instanceof PsiIfStatement) {

plugins/InspectionGadgets/src/inspectionDescriptions/ToArrayCallWithZeroLengthArrayArgument.html

@@ -1,13 +1,22 @@
 <html>
 <body>
-Reports any call to <b>toArray()</b>
-on an object of type or subtype <b>java.util.Collection</b>
-with a zero-length array argument. When passing in an array of too small size, the 
-<b>toArray()</b> method has to construct a new array of
-the right size using reflection. On older JVMs this has worse performance than passing
-in an array of at least the size of the collection itself.
-<!-- tooltip end -->
+There are two styles to convert collection to array: either using a pre-sized array
+(like <b>c.toArray(new String[c.size()])</b>) or using an empty array (like
+<b>c.toArray(new String[0])</b>.
 <p>
-
+  In older Java versions using pre-sized array was recommended as a reflection
+  call which is necessary to create an array of proper size was quite slow.
+  However since late updates of OpenJDK 6 this call was intrinsified making
+  the performance of the empty array version the same and sometimes even better, comparing
+  to the pre-sized version. Also passing pre-sized array is dangerous for concurrent or
+  synchronized collection as data race is possible between the <b>size</b> and <b>toArray</b>
+  call which may result in extra nulls at the end of the array if the collection was concurrently
+  shrinked during the operation.
+</p>
+<p>
+  This inspection allows to follow the uniform style: either using an empty array
+  (which is recommended in modern Java)
+  or using a pre-sized array (which might be faster in older Java versions or some non-HotSpot based JVMs).
+</p>
 </body>
 </html>

plugins/InspectionGadgets/test/com/siyeh/igfixes/performance/to_array_call_with_zero_length_array_argument/PresizedToZero.after.java

@@ -0,0 +1,17 @@
+package com.siyeh.igfixes.performance.to_array_call_with_zero_length_array_argument;
+
+import java.util.List;
+
+class IntroduceVariable {
+
+  static List<String> someFunc()
+  {
+    return null;
+  }
+
+  public static void main(String... args)
+
+  {
+    String[] foo = someFunc().toArray(new String[0]);
+  }
+}

plugins/InspectionGadgets/test/com/siyeh/igfixes/performance/to_array_call_with_zero_length_array_argument/PresizedToZero.java

@@ -0,0 +1,17 @@
+package com.siyeh.igfixes.performance.to_array_call_with_zero_length_array_argument;
+
+import java.util.List;
+
+class IntroduceVariable {
+
+  static List<String> someFunc()
+  {
+    return null;
+  }
+
+  public static void main(String... args)
+
+  {
+    String[] foo = someFunc().toAr<caret>ray(new String[someFunc().size()]);
+  }
+}

plugins/InspectionGadgets/testsrc/com/siyeh/ig/fixes/performance/ToArrayCallWithZeroLengthArrayArgumentFixTest.java

@@ -18,21 +18,28 @@ package com.siyeh.ig.fixes.performance;
 import com.siyeh.InspectionGadgetsBundle;
 import com.siyeh.ig.IGQuickFixesTestCase;
 import com.siyeh.ig.performance.ToArrayCallWithZeroLengthArrayArgumentInspection;
+import com.siyeh.ig.performance.ToArrayCallWithZeroLengthArrayArgumentInspection.PreferEmptyArray;
 
 public class ToArrayCallWithZeroLengthArrayArgumentFixTest extends IGQuickFixesTestCase {
+  private ToArrayCallWithZeroLengthArrayArgumentInspection myInspection = new ToArrayCallWithZeroLengthArrayArgumentInspection();
 
   @Override
   protected void setUp() throws Exception {
     super.setUp();
-    myFixture.enableInspections(new ToArrayCallWithZeroLengthArrayArgumentInspection());
+    myFixture.enableInspections(myInspection);
   }
 
   public void testIntroduceVariable() {
-    doFixTest();
+    doFixTest(PreferEmptyArray.NEVER, InspectionGadgetsBundle.message("to.array.call.style.quickfix.make.presized"));
   }
 
-  private void doFixTest() {
-    doTest(getTestName(false), InspectionGadgetsBundle.message("to.array.call.with.zero.length.array.argument.quickfix"));
+  public void testPresizedToZero() {
+    doFixTest(PreferEmptyArray.ALWAYS, InspectionGadgetsBundle.message("to.array.call.style.quickfix.make.zero"));
+  }
+
+  private void doFixTest(PreferEmptyArray mode, String message) {
+    myInspection.myMode = mode;
+    doTest(getTestName(false), message);
   }
 
   @Override