diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java b/src/main/java/com/google/devtools/build/lib/packages/BuildType.java index d9e73bb7dc8e73..99dce6d7acbc65 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java +++ b/src/main/java/com/google/devtools/build/lib/packages/BuildType.java @@ -489,11 +489,7 @@ public void repr(Printer printer) { for (Selector element : elements) { selectorValueList.add(new SelectorValue(element.getEntries(), element.getNoMatchError())); } - try { - printer.repr(com.google.devtools.build.lib.packages.SelectorList.of(selectorValueList)); - } catch (EvalException e) { - throw new IllegalStateException("this list should have been validated on creation"); - } + printer.repr(com.google.devtools.build.lib.packages.SelectorList.of(selectorValueList)); } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/SelectorList.java b/src/main/java/com/google/devtools/build/lib/packages/SelectorList.java index a70722937b7ff9..acf68ecc2826b8 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/SelectorList.java +++ b/src/main/java/com/google/devtools/build/lib/packages/SelectorList.java @@ -110,6 +110,11 @@ static SelectorList concat(Object x, Object y) throws EvalException { @Override public SelectorList binaryOp(TokenKind op, Object that, boolean thisLeft) throws EvalException { + if (!canConcatenate(getNativeType(this), getNativeType(that))) { + throw Starlark.errorf( + "'%s' operator applied to incompatible types (%s, %s)", + op.toString(), getTypeName(thisLeft ? this : that), getTypeName(thisLeft ? that : this)); + } if (getNativeType(that).equals(Dict.class)) { if (op == TokenKind.PIPE) { return thisLeft ? concat(this, that) : concat(that, this); @@ -127,7 +132,7 @@ else if (op == TokenKind.PLUS) { * * @throws EvalException if all values don't have the same underlying type */ - static SelectorList of(Iterable values) throws EvalException { + static SelectorList of(Iterable values) { Preconditions.checkArgument(!Iterables.isEmpty(values)); ImmutableList.Builder elements = ImmutableList.builder(); Object firstValue = null; @@ -142,9 +147,10 @@ static SelectorList of(Iterable values) throws EvalException { firstValue = value; } if (!canConcatenate(getNativeType(firstValue), getNativeType(value))) { - throw Starlark.errorf( - "'+' operator applied to incompatible types (%s, %s)", - getTypeName(firstValue), getTypeName(value)); + throw new IllegalStateException( + String.format( + "This list should have been validated on creation but found incompatible types (%s, %s)", + getTypeName(firstValue), getTypeName(value))); } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/SelectorValue.java b/src/main/java/com/google/devtools/build/lib/packages/SelectorValue.java index f443034b2b1536..a52bcef78d3f40 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/SelectorValue.java +++ b/src/main/java/com/google/devtools/build/lib/packages/SelectorValue.java @@ -81,10 +81,7 @@ public String toString() { @Override public SelectorList binaryOp(TokenKind op, Object that, boolean thisLeft) throws EvalException { - if (op == TokenKind.PLUS) { - return thisLeft ? SelectorList.concat(this, that) : SelectorList.concat(that, this); - } - return null; + return SelectorList.of(this).binaryOp(op, that, thisLeft); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java index 68edc9942538e9..d656565355d09c 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java @@ -700,11 +700,7 @@ private static Object starlarkifyValue(Mutability mu, Object val, Package pkg) ((Map) starlarkifyValue(mu, selector.getEntries(), pkg)), selector.getNoMatchError())); } - try { - return SelectorList.of(selectors); - } catch (EvalException e) { - throw new NotRepresentableException(e.getMessage()); - } + return SelectorList.of(selectors); } if (val instanceof StarlarkValue) { diff --git a/src/main/java/com/google/devtools/build/lib/packages/Type.java b/src/main/java/com/google/devtools/build/lib/packages/Type.java index b7084fa0edfd86..6c6a7262e869be 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Type.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Type.java @@ -208,7 +208,8 @@ public LabelClass getLabelClass() { /** * Implementation of concatenation for this type, as if by {@code elements[0] + ... + - * elements[n-1]}). Returns null to indicate concatenation isn't supported. This method exists to + * elements[n-1]}) for scalars or lists, or {@code elements[0] | ... | elements[n-1]} for dicts. + * Returns null to indicate concatenation isn't supported. This method exists to * support deferred additions {@code select + T} for catenable types T such as string, int, and * list. */ 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 da851f91dafbf1..1885f43113b57d 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 @@ -457,7 +457,7 @@ 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.concat(list, "A string")); + assertThrows(IllegalStateException.class, () -> SelectorList.concat(list, "A string")); } /** diff --git a/src/test/java/com/google/devtools/build/lib/packages/SelectTest.java b/src/test/java/com/google/devtools/build/lib/packages/SelectTest.java index 3bcd5add4b2468..fdae9c2e27e00c 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/SelectTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/SelectTest.java @@ -70,11 +70,53 @@ public void testPlus() throws Exception { @Test public void testPlusIncompatibleType() throws Exception { + assertFails( "select({'foo': ['FOO'], 'bar': ['BAR']}) + 1", "'+' operator applied to incompatible types (select of list, int)"); assertFails( "select({'foo': ['FOO']}) + select({'bar': 2})", "'+' operator applied to incompatible types (select of list, select of int)"); + + assertFails( + "select({'foo': ['FOO']}) + select({'bar': {'a': 'a'}})", + "'+' operator applied to incompatible types (select of list, select of dict)"); + assertFails( + "select({'bar': {'a': 'a'}}) + select({'foo': ['FOO']})", + "'+' operator applied to incompatible types (select of dict, select of list)"); + assertFails( + "['FOO'] + select({'bar': {'a': 'a'}})", + "'+' operator applied to incompatible types (list, select of dict)"); + assertFails( + "select({'bar': {'a': 'a'}}) + ['FOO']", + "'+' operator applied to incompatible types (select of dict, list)"); + assertFails( + "select({'foo': ['FOO']}) + {'a': 'a'}", + "'+' operator applied to incompatible types (select of list, dict)"); + assertFails( + "{'a': 'a'} + select({'foo': ['FOO']})", + "'+' operator applied to incompatible types (dict, select of list)"); + } + + @Test + public void testUnionIncompatibleType() throws Exception { + assertFails( + "select({'foo': ['FOO']}) | select({'bar': {'a': 'a'}})", + "'|' operator applied to incompatible types (select of list, select of dict)"); + assertFails( + "select({'bar': {'a': 'a'}}) | select({'foo': ['FOO']})", + "'|' operator applied to incompatible types (select of dict, select of list)"); + assertFails( + "['FOO'] | select({'bar': {'a': 'a'}})", + "'|' operator applied to incompatible types (list, select of dict)"); + assertFails( + "select({'bar': {'a': 'a'}}) | ['FOO']", + "'|' operator applied to incompatible types (select of dict, list)"); + assertFails( + "select({'foo': ['FOO']}) | {'a': 'a'}", + "'|' operator applied to incompatible types (select of list, dict)"); + assertFails( + "{'a': 'a'} | select({'foo': ['FOO']})", + "'|' operator applied to incompatible types (dict, select of list)"); } }