From 9f1f941e973b5d64cdc24f347e48a7bc5fa8715f Mon Sep 17 00:00:00 2001 From: jcater Date: Thu, 26 Oct 2017 21:58:37 +0200 Subject: Fix SelectorList.isListType to properly consider all classes that can be assigned to java.util.List, not just ArrayList. PiperOrigin-RevId: 173577936 --- .../devtools/build/lib/syntax/SelectorList.java | 24 +++++++---- .../devtools/build/lib/packages/BuildTypeTest.java | 49 ++++++++++++++++++++++ 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 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 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 list = ImmutableList.of("//a:a", "//b:b"); + List 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 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. -- cgit v1.2.3