Merge branch 'pr-1767'

docs/pages/release_notes.md

@@ -39,6 +39,7 @@ Being based on a proper Antlr grammar, CPD can:
     *   [#1751](https://github.com/pmd/pmd/issues/1751): \[go] Parsing errors encountered with escaped backslash
 *   java
     *   [#1532](https://github.com/pmd/pmd/issues/1532): \[java] NPE with incomplete auxclasspath
+    *   [#1691](https://github.com/pmd/pmd/issues/1691): \[java] Possible Data Race in JavaTypeDefinitionSimple.getGenericType
     *   [#1729](https://github.com/pmd/pmd/issues/1729): \[java] JavaRuleViolation loses information in `className` field when class has package-private access level
 *   java-bestpractices
     *   [#1190](https://github.com/pmd/pmd/issues/1190): \[java] UnusedLocalVariable/UnusedPrivateField false-positive 

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/typedefinition/JavaTypeDefinitionSimple.java

@@ -16,8 +16,7 @@ import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
 import java.lang.reflect.TypeVariable;
 import java.lang.reflect.WildcardType;
-import java.util.ArrayList;
-import java.util.Collections;
+import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
@@ -28,7 +27,7 @@ import java.util.logging.Logger;
 
 /* default */ class JavaTypeDefinitionSimple extends JavaTypeDefinition {
     private final Class<?> clazz;
-    private final List<JavaTypeDefinition> genericArgs;
+    private final JavaTypeDefinition[] genericArgs;
     // cached because calling clazz.getTypeParameters().length create a new array every time
     private final int typeParameterCount;
     private final boolean isGeneric;
@@ -36,6 +35,7 @@ import java.util.logging.Logger;
     private final JavaTypeDefinition enclosingClass;
 
     private static final Logger LOG = Logger.getLogger(JavaTypeDefinitionSimple.class.getName());
+    private static final JavaTypeDefinition[] NO_GENERICS = {};
 
     protected JavaTypeDefinitionSimple(Class<?> clazz, JavaTypeDefinition... boundGenerics) {
         super(EXACT);
@@ -59,12 +59,10 @@ import java.util.logging.Logger;
         isRawType = isGeneric && boundGenerics.length == 0;
 
         if (isGeneric) {
-            // Generics will be lazily loaded
-            this.genericArgs = new ArrayList<>(typeParameters.length);
-            // boundGenerics would be empty if this is a raw type, hence the lazy loading
-            Collections.addAll(this.genericArgs, boundGenerics);
+            // Generics will be lazily loaded if not already known
+            this.genericArgs = Arrays.copyOf(boundGenerics, typeParameterCount);
         } else {
-            this.genericArgs = Collections.emptyList();
+            this.genericArgs = NO_GENERICS;
         }
 
         enclosingClass = forClass(clazz.getEnclosingClass());
@@ -82,7 +80,7 @@ import java.util.logging.Logger;
 
     @Override
     public boolean isGeneric() {
-        return !genericArgs.isEmpty();
+        return isGeneric;
     }
 
     private JavaTypeDefinition getGenericType(final String parameterName, Method method,
@@ -126,29 +124,22 @@ import java.util.logging.Logger;
     @Override
     public JavaTypeDefinition getGenericType(final int index) {
         // Check if it has been lazily initialized first
-        if (genericArgs.size() > index) {
-            final JavaTypeDefinition cachedDefinition = genericArgs.get(index);
-            if (cachedDefinition != null) {
-                return cachedDefinition;
-            }
-        }
-
-        // Force the list to have enough elements
-        for (int i = genericArgs.size(); i <= index; i++) {
-            genericArgs.add(null);
+        final JavaTypeDefinition cachedDefinition = genericArgs[index];
+        if (cachedDefinition != null) {
+            return cachedDefinition;
         }
 
         /*
-         * Set a default to circuit-brake any recursions (ie: raw types with no generic info)
+         * Set a default to circuit-break any recursions (ie: raw types with no generic info)
          * Object.class is a right answer in those scenarios
          */
-        genericArgs.set(index, forClass(Object.class));
+        genericArgs[index] = forClass(Object.class);
 
         final TypeVariable<?> typeVariable = clazz.getTypeParameters()[index];
         final JavaTypeDefinition typeDefinition = resolveTypeDefinition(typeVariable.getBounds()[0]);
 
         // cache result
-        genericArgs.set(index, typeDefinition);
+        genericArgs[index] = typeDefinition;
         return typeDefinition;
     }
 
@@ -279,7 +270,7 @@ import java.util.logging.Logger;
                 .append(", genericArgs=[");
 
         // Forcefully resolve all generic types
-        for (int i = 0; i < genericArgs.size(); i++) {
+        for (int i = 0; i < genericArgs.length; i++) {
             getGenericType(i);
         }
 
@@ -287,7 +278,7 @@ import java.util.logging.Logger;
             sb.append(jtd.shallowString()).append(", ");
         }
 
-        if (!genericArgs.isEmpty()) {
+        if (genericArgs.length != 0) {
             sb.replace(sb.length() - 3, sb.length() - 1, "");   // remove last comma
         }
 

pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/typedefinition/JavaTypeDefinitionSimpleTest.java

@@ -0,0 +1,66 @@
+/**
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.typeresolution.typedefinition;
+
+import java.util.ArrayList;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import net.sourceforge.pmd.lang.java.typeresolution.typedefinition.JavaTypeDefinition;
+
+public class JavaTypeDefinitionSimpleTest {
+
+    /**
+     * Tests the raw type {@code ArrayList}.
+     */
+    @Test
+    public void arrayListWithoutBoundGenerics() {
+        JavaTypeDefinition typeDef = JavaTypeDefinition.forClass(ArrayList.class);
+        Assert.assertTrue(typeDef.isGeneric());
+        Assert.assertTrue(typeDef.isRawType());
+        Assert.assertEquals(1, typeDef.getTypeParameterCount());
+
+        JavaTypeDefinition genericType = typeDef.getGenericType(0);
+        Assert.assertFalse(genericType.isGeneric());
+        Assert.assertEquals(Object.class, genericType.getType());
+    }
+
+    /**
+     * Tests the type {@code ArrayList<String>}.
+     */
+    @Test
+    public void arrayListOfString() {
+        JavaTypeDefinition typeDef = JavaTypeDefinition.forClass(ArrayList.class, JavaTypeDefinition.forClass(String.class));
+        Assert.assertTrue(typeDef.isGeneric());
+        Assert.assertEquals(1, typeDef.getTypeParameterCount());
+        Assert.assertTrue(typeDef.isClassOrInterface());
+        Assert.assertFalse(typeDef.isArrayType());
+
+        JavaTypeDefinition genericType = typeDef.getGenericType(0);
+        Assert.assertFalse(genericType.isGeneric());
+        Assert.assertEquals(String.class, genericType.getType());
+
+        JavaTypeDefinition genericTypeByName = typeDef.getGenericType("E");
+        Assert.assertEquals(String.class, genericTypeByName.getType());
+    }
+
+    @Test
+    public void array() {
+        JavaTypeDefinition typeDef = JavaTypeDefinition.forClass(String[].class);
+        Assert.assertFalse(typeDef.isGeneric());
+        Assert.assertTrue(typeDef.isArrayType());
+        Assert.assertFalse(typeDef.isClassOrInterface());
+        Assert.assertEquals(String.class, typeDef.getElementType().getType());
+        Assert.assertFalse(typeDef.isPrimitive());
+    }
+
+    @Test
+    public void primitive() {
+        JavaTypeDefinition typeDef = JavaTypeDefinition.forClass(int.class);
+        Assert.assertTrue(typeDef.isPrimitive());
+        Assert.assertFalse(typeDef.isClassOrInterface());
+    }
+}