Fix SelectorList.isListType to properly consider all classes that can be assigned to java.util.List, not just ArrayList.

PiperOrigin-RevId: 173577936

2 files changed, 64 insertions, 9 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SelectorList.java b/src/main/java/com/google/devtools/build/lib/syntax/SelectorList.java
index 7ec367cb89..3180483777 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SelectorList.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SelectorList.java
@@ -20,8 +20,6 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
 import com.google.devtools.build.lib.util.Preconditions;
-import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.List;
 
 /**
@@ -86,12 +84,22 @@ public final class SelectorList implements SkylarkValue {
    */
   public static SelectorList concat(Location location, Object value1, Object value2)
       throws EvalException {
-    return of(location, Arrays.asList(value1, value2));
+    return of(location, value1, value2);
   }
 
   /**
-   * Creates a list from the given sequence of values, which must be non-empty. Each value may be
-   * a native type, a select over that type, or a selector list over that type.
+   * Creates a list from the given sequence of values, which must be non-empty. Each value may be a
+   * native type, a select over that type, or a selector list over that type.
+   *
+   * @throws EvalException if all values don't have the same underlying type
+   */
+  public static SelectorList of(Location location, Object... values) throws EvalException {
+    return SelectorList.of(location, ImmutableList.copyOf(values));
+  }
+
+  /**
+   * Creates a list from the given sequence of values, which must be non-empty. Each value may be a
+   * native type, a select over that type, or a selector list over that type.
    *
    * @throws EvalException if all values don't have the same underlying type
    */
@@ -122,9 +130,7 @@ public final class SelectorList implements SkylarkValue {
     return new SelectorList(getNativeType(firstValue), elements.build());
   }
 
-  // TODO(bazel-team): match on the List interface, not the actual implementation. For now,
-  // we verify this is the right class through test coverage.
-  private static final Class<?> NATIVE_LIST_TYPE = ArrayList.class;
+  private static final Class<?> NATIVE_LIST_TYPE = List.class;
 
   private static Class<?> getNativeType(Object value) {
     if (value instanceof SelectorList) {
@@ -137,7 +143,7 @@ public final class SelectorList implements SkylarkValue {
   }
 
   private static boolean isListType(Class<?> type) {
-    return type == NATIVE_LIST_TYPE
+    return NATIVE_LIST_TYPE.isAssignableFrom(type)
         || type.getSuperclass() == SkylarkList.class
         || type == GlobList.class;
   }
diff --git a/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java b/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java
index de6d81a257..a3604c31ce 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.packages;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 import static org.junit.Assert.fail;
 
 import com.google.common.base.Joiner;
@@ -22,13 +23,16 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.BuildType.Selector;
+import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.EvalUtils;
 import com.google.devtools.build.lib.syntax.Printer;
 import com.google.devtools.build.lib.syntax.SelectorList;
 import com.google.devtools.build.lib.syntax.SelectorValue;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.syntax.Type.ConversionException;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
@@ -377,6 +381,51 @@ public class BuildTypeTest {
     }
   }
 
+  @Test
+  public void testSelectorList_concatenate_selectorList() throws Exception {
+    SelectorList selectorList =
+        SelectorList.of(
+            new SelectorValue(ImmutableMap.of("//conditions:a", ImmutableList.of("//a:a")), ""));
+    List<String> list = ImmutableList.of("//a:a", "//b:b");
+
+    // Creating a SelectorList from a SelectorList and a list should work properly.
+    SelectorList result = SelectorList.of(Location.BUILTIN, selectorList, list);
+    assertThat(result).isNotNull();
+    assertThat(result.getType()).isAssignableTo(List.class);
+  }
+
+  @Test
+  public void testSelectorList_concatenate_selectorValue() throws Exception {
+    SelectorValue selectorValue =
+        new SelectorValue(ImmutableMap.of("//conditions:a", ImmutableList.of("//a:a")), "");
+    List<String> list = ImmutableList.of("//a:a", "//b:b");
+
+    // Creating a SelectorList from a SelectorValue and a list should work properly.
+    SelectorList result = SelectorList.of(Location.BUILTIN, selectorValue, list);
+    assertThat(result).isNotNull();
+    assertThat(result.getType()).isAssignableTo(List.class);
+  }
+
+  @Test
+  public void testSelectorList_concatenate_differentListTypes() throws Exception {
+    List<String> list = ImmutableList.of("//a:a", "//b:b");
+    List<String> arrayList = new ArrayList<>();
+    arrayList.add("//a:a");
+
+    // Creating a SelectorList from two lists of different types should work properly.
+    SelectorList result = SelectorList.of(Location.BUILTIN, list, arrayList);
+    assertThat(result).isNotNull();
+    assertThat(result.getType()).isAssignableTo(List.class);
+  }
+
+  @Test
+  public void testSelectorList_concatenate_invalidType() throws Exception {
+    List<String> list = ImmutableList.of("//a:a", "//b:b");
+
+    // Creating a SelectorList from a list and a non-list should fail.
+    assertThrows(EvalException.class, () -> SelectorList.of(Location.BUILTIN, list, "A string"));
+  }
+
   /**
    * Tests that {@link BuildType#selectableConvert} returns either the native type or a selector
    * on that type, in accordance with the provided input.