Review feedback

bazelbuild · Apr 30, 2022 · aa0c989 · aa0c989
1 parent 77753b4
commit aa0c989
Show file tree

Hide file tree

Showing 7 changed files with 58 additions and 20 deletions.
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
@@ -489,11 +489,7 @@ public void repr(Printer printer) {
       for (Selector<T> 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
@@ -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<Object> 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
@@ -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
@@ -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
@@ -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
@@ -457,7 +457,7 @@ 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.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
@@ -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)");
   }
 }