bazel syntax: generalize Concatter to HasBinary, for binary operators
A Starlark value that implements HasBinary may define its own
semantics for binary operators such as x+y. See HasBinary for details.
This brings us a step closer to moving Selector{List,Value} to lib.packages.
Also:
- remove the Location parameter from the operator method.
This API simplification causes a minor usability regression:
SkylarkInfo instances created by x+y will no longer record the
location of the plus operator.
- make the control tree for binary operator cases shallower
(that is, avoid unnecessary re-tests of instanceof),
with a single fall-through.
- The specialized error message for 'in' is no longer feasible
as the set of valid operand types is no longer closed.
- Simplify arithmetic division, borrowing from go.starlark.net.
This is a breaking API change for Copybara.
PiperOrigin-RevId: 291726807
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 55b3bac..59f567f 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
@@ -568,7 +568,7 @@
selectorValueList.add(new SelectorValue(element.getEntries(), element.getNoMatchError()));
}
try {
- printer.repr(com.google.devtools.build.lib.syntax.SelectorList.of(null, selectorValueList));
+ printer.repr(com.google.devtools.build.lib.syntax.SelectorList.of(selectorValueList));
} catch (EvalException e) {
throw new IllegalStateException("this list should have been validated on creation");
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java
index 77ca87d..c08afdd 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java
@@ -19,17 +19,18 @@
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.syntax.ClassObject;
-import com.google.devtools.build.lib.syntax.Concatable;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
+import com.google.devtools.build.lib.syntax.HasBinary;
import com.google.devtools.build.lib.syntax.Starlark;
+import com.google.devtools.build.lib.syntax.TokenKind;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
/** An Info (provider instance) for providers defined in Starlark. */
-public final class SkylarkInfo extends StructImpl implements Concatable, ClassObject {
+public final class SkylarkInfo extends StructImpl implements HasBinary, ClassObject {
// For a n-element info, the table contains n key strings, sorted,
// followed by the n corresponding legal Starlark values.
@@ -187,23 +188,22 @@
}
@Override
- public Concatter getConcatter() {
- return SkylarkInfo::concat;
+ public SkylarkInfo binaryOp(TokenKind op, Object that, boolean thisLeft) throws EvalException {
+ if (op == TokenKind.PLUS && that instanceof SkylarkInfo) {
+ return thisLeft
+ ? plus(this, (SkylarkInfo) that) //
+ : plus((SkylarkInfo) that, this);
+ }
+ return null;
}
- private static Concatable concat(Concatable left, Concatable right, Location loc)
- throws EvalException {
- // Casts are safe because this Concatter is only used by SkylarkInfo.
- SkylarkInfo x = (SkylarkInfo) left;
- SkylarkInfo y = (SkylarkInfo) right;
+ private static SkylarkInfo plus(SkylarkInfo x, SkylarkInfo y) throws EvalException {
Provider xprov = x.getProvider();
Provider yprov = y.getProvider();
if (!xprov.equals(yprov)) {
- throw new EvalException(
- loc,
- String.format(
- "Cannot use '+' operator on instances of different providers (%s and %s)",
- xprov.getPrintableName(), yprov.getPrintableName()));
+ throw Starlark.errorf(
+ "Cannot use '+' operator on instances of different providers (%s and %s)",
+ xprov.getPrintableName(), yprov.getPrintableName());
}
// ztable = merge(x.table, y.table)
@@ -227,8 +227,7 @@
ztable[zi + zsize] = y.table[yi + ysize];
yi++;
} else {
- throw new EvalException(
- loc, String.format("cannot add struct instances with common field '%s'", xk));
+ throw Starlark.errorf("cannot add struct instances with common field '%s'", xk);
}
zi++;
}
@@ -245,6 +244,6 @@
zi++;
}
- return new SkylarkInfo(xprov, ztable, loc, x.unknownFieldError);
+ return new SkylarkInfo(xprov, ztable, Location.BUILTIN, x.unknownFieldError);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BUILD b/src/main/java/com/google/devtools/build/lib/syntax/BUILD
index e2bc985..1f9ec0d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BUILD
@@ -94,7 +94,6 @@
"CallUtils.java",
"Callstack.java",
"ClassObject.java",
- "Concatable.java",
"Debug.java",
"Debugger.java",
"Depset.java",
@@ -105,6 +104,7 @@
"EvalUtils.java",
"FlagGuardedValue.java",
"FormatParser.java",
+ "HasBinary.java",
"MethodDescriptor.java",
"MethodLibrary.java",
"Module.java",
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Concatable.java b/src/main/java/com/google/devtools/build/lib/syntax/Concatable.java
deleted file mode 100644
index 81f489f..0000000
--- a/src/main/java/com/google/devtools/build/lib/syntax/Concatable.java
+++ /dev/null
@@ -1,36 +0,0 @@
-// Copyright 2016 The Bazel Authors. All Rights Reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.lib.syntax;
-
-import com.google.devtools.build.lib.events.Location;
-import javax.annotation.Nullable;
-
-/**
- * Skylark values that support '+' operator should implement this interface.
- */
-public interface Concatable {
-
- /**
- * Implements 'plus' operator on ClassObjects.
- */
- interface Concatter {
- Concatable concat(Concatable lval, Concatable rval, Location loc) throws EvalException;
- }
-
- /* Returns a concatter for this {@link Concatable}.
- * Two {@link Concatable}s can be added together if their concatters are equal.
- */
- @Nullable
- Concatter getConcatter();
-}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
index 7c2cbd2..2a4f6c6 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
@@ -390,15 +390,13 @@
Expression lhs = stmt.getLHS();
TokenKind op = stmt.getOperator();
Expression rhs = stmt.getRHS();
- // TODO(adonovan): don't materialize Locations before an error has occurred.
- // (Requires syntax tree to record offsets and defer Location conversion.)
- Location loc = stmt.getStartLocation(); // TODO(adonovan): use operator location
if (lhs instanceof Identifier) {
Object x = eval(fr, lhs);
Object y = eval(fr, rhs);
- Object z = inplaceBinaryOp(fr, op, x, y, loc);
+ Object z = inplaceBinaryOp(fr, op, x, y);
assignIdentifier(fr, (Identifier) lhs, z);
+
} else if (lhs instanceof IndexExpression) {
// object[index] op= y
// The object and key should be evaluated only once, so we don't use lhs.eval().
@@ -408,23 +406,27 @@
Object x = EvalUtils.index(fr.thread.mutability(), fr.thread.getSemantics(), object, key);
// Evaluate rhs after lhs.
Object y = eval(fr, rhs);
- Object z = inplaceBinaryOp(fr, op, x, y, loc);
+ Object z = inplaceBinaryOp(fr, op, x, y);
try {
assignItem(object, key, z);
} catch (EvalException ex) {
+ Location loc = stmt.getStartLocation(); // TODO(adonovan): use operator location
throw ex.ensureLocation(loc);
}
+
} else if (lhs instanceof ListExpression) {
+ Location loc = stmt.getStartLocation(); // TODO(adonovan): use operator location
throw new EvalException(loc, "cannot perform augmented assignment on a list literal");
+
} else {
// Not possible for validated ASTs.
+ Location loc = stmt.getStartLocation(); // TODO(adonovan): use operator location
throw new EvalException(loc, "cannot perform augmented assignment on '" + lhs + "'");
}
}
private static Object inplaceBinaryOp(
- StarlarkThread.CallFrame fr, TokenKind op, Object x, Object y, Location location)
- throws EvalException {
+ StarlarkThread.CallFrame fr, TokenKind op, Object x, Object y) throws EvalException {
// list += iterable behaves like list.extend(iterable)
// TODO(b/141263526): following Python, allow list+=iterable (but not list+iterable).
if (op == TokenKind.PLUS && x instanceof StarlarkList && y instanceof StarlarkList) {
@@ -432,7 +434,7 @@
list.extend(y);
return list;
}
- return EvalUtils.binaryOp(op, x, y, fr.thread, location);
+ return EvalUtils.binaryOp(op, x, y, fr.thread.getSemantics(), fr.thread.mutability());
}
// ---- expressions ----
@@ -474,9 +476,14 @@
return Starlark.truth(x) ? x : eval(fr, binop.getY());
default:
Object y = eval(fr, binop.getY());
- // TODO(adonovan): use operator location
- return EvalUtils.binaryOp(
- binop.getOperator(), x, y, fr.thread, binop.getStartLocation());
+ try {
+ return EvalUtils.binaryOp(
+ binop.getOperator(), x, y, fr.thread.getSemantics(), fr.thread.mutability());
+ } catch (EvalException ex) {
+ // TODO(adonovan): use operator location
+ ex.ensureLocation(binop.getStartLocation());
+ throw ex;
+ }
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
index 4e1b680..88cc999 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
@@ -21,7 +21,6 @@
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
-import com.google.devtools.build.lib.syntax.Concatable.Concatter;
import com.google.devtools.build.lib.util.SpellChecker;
import java.util.IllegalFormatException;
import java.util.List;
@@ -370,75 +369,285 @@
}
/** Evaluates an eager binary operation, {@code x op y}. (Excludes AND and OR.) */
- static Object binaryOp(TokenKind op, Object x, Object y, StarlarkThread thread, Location location)
+ static Object binaryOp(
+ TokenKind op, Object x, Object y, StarlarkSemantics semantics, Mutability mu)
throws EvalException {
- try {
- switch (op) {
- case PLUS:
- return plus(x, y, thread, location);
+ switch (op) {
+ case PLUS:
+ if (x instanceof Integer) {
+ if (y instanceof Integer) {
+ // int + int
+ int xi = (Integer) x;
+ int yi = (Integer) y;
+ int z = xi + yi;
+ // Overflow Detection, §2-13 Hacker's Delight:
+ // "operands have the same sign and the sum
+ // has a sign opposite to that of the operands."
+ if (((xi ^ z) & (yi ^ z)) < 0) {
+ throw Starlark.errorf("integer overflow in addition");
+ }
+ return z;
+ }
- case PIPE:
- return pipe(thread.getSemantics(), x, y);
+ } else if (x instanceof String) {
+ if (y instanceof String) {
+ // string + string
+ return (String) x + (String) y;
+ }
- case AMPERSAND:
- return and(x, y);
+ } else if (x instanceof Tuple) {
+ if (y instanceof Tuple) {
+ // tuple + tuple
+ return Tuple.concat((Tuple<?>) x, (Tuple<?>) y);
+ }
- case CARET:
- return xor(x, y);
+ } else if (x instanceof StarlarkList) {
+ if (y instanceof StarlarkList) {
+ // list + list
+ return StarlarkList.concat((StarlarkList<?>) x, (StarlarkList<?>) y, mu);
+ }
- case GREATER_GREATER:
- return rightShift(x, y);
+ } else if (x instanceof Depset) {
+ // depset + any
+ // TODO(bazel-team): Remove deprecated operator.
+ if (semantics.incompatibleDepsetUnion()) {
+ throw Starlark.errorf(
+ "`+` operator on a depset is forbidden. See "
+ + "https://docs.bazel.build/versions/master/skylark/depsets.html for "
+ + "recommendations. Use --incompatible_depset_union=false "
+ + "to temporarily disable this check.");
+ }
+ return Depset.unionOf((Depset) x, y);
+ }
+ break;
- case LESS_LESS:
- return leftShift(x, y);
+ case PIPE:
+ if (x instanceof Integer) {
+ if (y instanceof Integer) {
+ // int | int
+ return ((Integer) x) | (Integer) y;
+ }
+ } else if (x instanceof Depset) {
+ // depset | any
+ if (semantics.incompatibleDepsetUnion()) {
+ throw Starlark.errorf(
+ "`|` operator on a depset is forbidden. See "
+ + "https://docs.bazel.build/versions/master/skylark/depsets.html for "
+ + "recommendations. Use --incompatible_depset_union=false "
+ + "to temporarily disable this check.");
+ }
+ return Depset.unionOf((Depset) x, y);
+ }
+ break;
- case MINUS:
- return minus(x, y);
+ case AMPERSAND:
+ if (x instanceof Integer && y instanceof Integer) {
+ // int & int
+ return (Integer) x & (Integer) y;
+ }
+ break;
- case STAR:
- return star(thread.mutability(), x, y);
+ case CARET:
+ if (x instanceof Integer && y instanceof Integer) {
+ // int ^ int
+ return (Integer) x ^ (Integer) y;
+ }
+ break;
- case SLASH:
- throw Starlark.errorf(
- "The `/` operator is not allowed. Please use the `//` operator for integer"
- + " division.");
+ case GREATER_GREATER:
+ if (x instanceof Integer && y instanceof Integer) {
+ // int >> int
+ int xi = (Integer) x;
+ int yi = (Integer) y;
+ if (yi < 0) {
+ throw Starlark.errorf("negative shift count: %d", yi);
+ } else if (yi >= Integer.SIZE) {
+ return xi < 0 ? -1 : 0;
+ }
+ return xi >> yi;
+ }
+ break;
- case SLASH_SLASH:
- return slash(x, y);
+ case LESS_LESS:
+ if (x instanceof Integer && y instanceof Integer) {
+ // int << int
+ int xi = (Integer) x;
+ int yi = (Integer) y;
+ if (yi < 0) {
+ throw Starlark.errorf("negative shift count: %d", yi);
+ }
+ int z = xi << yi;
+ if (z >> yi != xi) {
+ throw Starlark.errorf("integer overflow");
+ }
+ return z;
+ }
+ break;
- case PERCENT:
- return percent(x, y);
+ case MINUS:
+ if (x instanceof Integer && y instanceof Integer) {
+ // int - int
+ int xi = (Integer) x;
+ int yi = (Integer) y;
+ int z = xi - yi;
+ if (((xi ^ yi) & (xi ^ z)) < 0) {
+ throw Starlark.errorf("integer overflow in subtraction");
+ }
+ return z;
+ }
+ break;
- case EQUALS_EQUALS:
- return x.equals(y);
+ case STAR:
+ if (x instanceof Integer) {
+ int xi = (Integer) x;
+ if (y instanceof Integer) {
+ // int * int
+ long z = (long) xi * (long) (Integer) y;
+ if ((int) z != z) {
+ throw Starlark.errorf("integer overflow in multiplication");
+ }
+ return (int) z;
+ } else if (y instanceof String) {
+ // int * string
+ return repeatString((String) y, xi);
+ } else if (y instanceof Tuple) {
+ // int * tuple
+ return ((Tuple<?>) y).repeat(xi);
+ } else if (y instanceof StarlarkList) {
+ // int * list
+ return ((StarlarkList<?>) y).repeat(xi, mu);
+ }
- case NOT_EQUALS:
- return !x.equals(y);
+ } else if (x instanceof String) {
+ if (y instanceof Integer) {
+ // string * int
+ return repeatString((String) x, (Integer) y);
+ }
- case LESS:
- return compare(x, y) < 0;
+ } else if (x instanceof Tuple) {
+ if (y instanceof Integer) {
+ // tuple * int
+ return ((Tuple<?>) x).repeat((Integer) y);
+ }
- case LESS_EQUALS:
- return compare(x, y) <= 0;
+ } else if (x instanceof StarlarkList) {
+ if (y instanceof Integer) {
+ // list * int
+ return ((StarlarkList<?>) x).repeat((Integer) y, mu);
+ }
+ }
+ break;
- case GREATER:
- return compare(x, y) > 0;
+ case SLASH:
+ throw Starlark.errorf("The `/` operator is not allowed. For integer division, use `//`.");
- case GREATER_EQUALS:
- return compare(x, y) >= 0;
+ case SLASH_SLASH:
+ if (x instanceof Integer && y instanceof Integer) {
+ // int // int
+ int xi = (Integer) x;
+ int yi = (Integer) y;
+ if (yi == 0) {
+ throw Starlark.errorf("integer division by zero");
+ }
+ // http://python-history.blogspot.com/2010/08/why-pythons-integer-division-floors.html
+ int quo = xi / yi;
+ int rem = xi % yi;
+ if ((xi < 0) != (yi < 0) && rem != 0) {
+ quo--;
+ }
+ return quo;
+ }
+ break;
- case IN:
- return in(thread.getSemantics(), x, y);
+ case PERCENT:
+ if (x instanceof Integer) {
+ if (y instanceof Integer) {
+ // int % int
+ int xi = (Integer) x;
+ int yi = (Integer) y;
+ if (yi == 0) {
+ throw Starlark.errorf("integer modulo by zero");
+ }
+ // In Starlark, the sign of the result is the sign of the divisor.
+ int z = xi % yi;
+ if ((xi < 0) != (yi < 0) && z != 0) {
+ z += yi;
+ }
+ return z;
+ }
- case NOT_IN:
- return !in(thread.getSemantics(), x, y);
+ } else if (x instanceof String) {
+ // string % any
+ String xs = (String) x;
+ try {
+ if (y instanceof Tuple) {
+ return Starlark.formatWithList(xs, (Tuple) y);
+ } else {
+ return Starlark.format(xs, y);
+ }
+ } catch (IllegalFormatException ex) {
+ throw new EvalException(null, ex.getMessage());
+ }
+ }
+ break;
- default:
- throw new AssertionError("Unsupported binary operator: " + op);
- }
- } catch (ArithmeticException ex) {
- throw new EvalException(null, ex.getMessage());
+ case EQUALS_EQUALS:
+ return x.equals(y);
+
+ case NOT_EQUALS:
+ return !x.equals(y);
+
+ case LESS:
+ return compare(x, y) < 0;
+
+ case LESS_EQUALS:
+ return compare(x, y) <= 0;
+
+ case GREATER:
+ return compare(x, y) > 0;
+
+ case GREATER_EQUALS:
+ return compare(x, y) >= 0;
+
+ case IN:
+ if (y instanceof SkylarkQueryable) {
+ return ((SkylarkQueryable) y).containsKey(semantics, x);
+ } else if (y instanceof String) {
+ if (!(x instanceof String)) {
+ throw Starlark.errorf(
+ "'in <string>' requires string as left operand, not '%s'", Starlark.type(x));
+ }
+ return ((String) y).contains((String) x);
+ }
+ break;
+
+ case NOT_IN:
+ Object z = binaryOp(TokenKind.IN, x, y, semantics, mu);
+ if (z != null) {
+ return !Starlark.truth(z);
+ }
+ break;
+
+ default:
+ throw new AssertionError("not a binary operator: " + op);
}
+
+ // custom binary operator?
+ if (x instanceof HasBinary) {
+ Object z = ((HasBinary) x).binaryOp(op, y, true);
+ if (z != null) {
+ return z;
+ }
+ }
+ if (y instanceof HasBinary) {
+ Object z = ((HasBinary) y).binaryOp(op, x, false);
+ if (z != null) {
+ return z;
+ }
+ }
+
+ throw Starlark.errorf(
+ "unsupported binary operation: %s %s %s", Starlark.type(x), op, Starlark.type(y));
}
/** Implements comparison operators. */
@@ -450,232 +659,8 @@
}
}
- /** Implements 'x + y'. */
- // StarlarkThread is needed for Mutability and Semantics.
- // Location is exposed to Selector{List,Value} and Concatter.
- static Object plus(Object x, Object y, StarlarkThread thread, Location location)
- throws EvalException {
- // int + int
- if (x instanceof Integer && y instanceof Integer) {
- return Math.addExact((Integer) x, (Integer) y);
- }
-
- // string + string
- if (x instanceof String && y instanceof String) {
- return (String) x + (String) y;
- }
-
- if (x instanceof SelectorValue
- || y instanceof SelectorValue
- || x instanceof SelectorList
- || y instanceof SelectorList) {
- return SelectorList.concat(location, x, y);
- }
-
- if (x instanceof Tuple && y instanceof Tuple) {
- return Tuple.concat((Tuple<?>) x, (Tuple<?>) y);
- }
-
- if (x instanceof StarlarkList && y instanceof StarlarkList) {
- return StarlarkList.concat((StarlarkList<?>) x, (StarlarkList<?>) y, thread.mutability());
- }
-
- if (x instanceof Concatable && y instanceof Concatable) {
- Concatable lobj = (Concatable) x;
- Concatable robj = (Concatable) y;
- Concatter concatter = lobj.getConcatter();
- if (concatter != null && concatter.equals(robj.getConcatter())) {
- return concatter.concat(lobj, robj, location);
- } else {
- throw unknownBinaryOperator(x, y, TokenKind.PLUS);
- }
- }
-
- // TODO(bazel-team): Remove deprecated operator.
- if (x instanceof Depset) {
- if (thread.getSemantics().incompatibleDepsetUnion()) {
- throw new EvalException(
- location,
- "`+` operator on a depset is forbidden. See "
- + "https://docs.bazel.build/versions/master/skylark/depsets.html for "
- + "recommendations. Use --incompatible_depset_union=false "
- + "to temporarily disable this check.");
- }
- return Depset.unionOf((Depset) x, y);
- }
- throw unknownBinaryOperator(x, y, TokenKind.PLUS);
- }
-
- /** Implements 'x | y'. */
- private static Object pipe(StarlarkSemantics semantics, Object x, Object y) throws EvalException {
- if (x instanceof Integer && y instanceof Integer) {
- return ((Integer) x) | ((Integer) y);
- } else if (x instanceof Depset) {
- if (semantics.incompatibleDepsetUnion()) {
- throw Starlark.errorf(
- "`|` operator on a depset is forbidden. See "
- + "https://docs.bazel.build/versions/master/skylark/depsets.html for "
- + "recommendations. Use --incompatible_depset_union=false "
- + "to temporarily disable this check.");
- }
- return Depset.unionOf((Depset) x, y);
- }
- throw unknownBinaryOperator(x, y, TokenKind.PIPE);
- }
-
- /** Implements 'x - y'. */
- private static Object minus(Object x, Object y) throws EvalException {
- if (x instanceof Integer && y instanceof Integer) {
- return Math.subtractExact((Integer) x, (Integer) y);
- }
- throw unknownBinaryOperator(x, y, TokenKind.MINUS);
- }
-
- /** Implements 'x * y'. */
- private static Object star(Mutability mu, Object x, Object y) throws EvalException {
- Integer number = null;
- Object otherFactor = null;
-
- if (x instanceof Integer) {
- number = (Integer) x;
- otherFactor = y;
- } else if (y instanceof Integer) {
- number = (Integer) y;
- otherFactor = x;
- }
-
- if (number != null) {
- if (otherFactor instanceof Integer) {
- return Math.multiplyExact(number, (Integer) otherFactor);
- } else if (otherFactor instanceof String) {
- return Strings.repeat((String) otherFactor, Math.max(0, number));
- } else if (otherFactor instanceof Tuple) {
- return ((Tuple<?>) otherFactor).repeat(number, mu);
- } else if (otherFactor instanceof StarlarkList) {
- return ((StarlarkList<?>) otherFactor).repeat(number, mu);
- }
- }
- throw unknownBinaryOperator(x, y, TokenKind.STAR);
- }
-
- /** Implements 'x // y'. */
- private static Object slash(Object x, Object y) throws EvalException {
- // int / int
- if (x instanceof Integer && y instanceof Integer) {
- if (y.equals(0)) {
- throw Starlark.errorf("integer division by zero");
- }
- // Integer division doesn't give the same result in Java and in Python 2 with
- // negative numbers.
- // Java: -7/3 = -2
- // Python: -7/3 = -3
- // We want to follow Python semantics, so we use float division and round down.
- return (int) Math.floor(Double.valueOf((Integer) x) / (Integer) y);
- }
- throw unknownBinaryOperator(x, y, TokenKind.SLASH_SLASH);
- }
-
- /** Implements 'x % y'. */
- private static Object percent(Object x, Object y) throws EvalException {
- // int % int
- if (x instanceof Integer && y instanceof Integer) {
- if (y.equals(0)) {
- throw Starlark.errorf("integer modulo by zero");
- }
- // Python and Java implement division differently, wrt negative numbers.
- // In Python, sign of the result is the sign of the divisor.
- int div = (Integer) y;
- int result = ((Integer) x).intValue() % Math.abs(div);
- if (result > 0 && div < 0) {
- result += div; // make the result negative
- } else if (result < 0 && div > 0) {
- result += div; // make the result positive
- }
- return result;
- }
-
- // string % tuple, string % dict, string % anything-else
- if (x instanceof String) {
- String pattern = (String) x;
- try {
- if (y instanceof Tuple) {
- return Starlark.formatWithList(pattern, (Tuple) y);
- }
- return Starlark.format(pattern, y);
- } catch (IllegalFormatException ex) {
- throw new EvalException(null, ex.getMessage());
- }
- }
- throw unknownBinaryOperator(x, y, TokenKind.PERCENT);
- }
-
- /** Implements 'x & y'. */
- private static Object and(Object x, Object y) throws EvalException {
- if (x instanceof Integer && y instanceof Integer) {
- return (Integer) x & (Integer) y;
- }
- throw unknownBinaryOperator(x, y, TokenKind.AMPERSAND);
- }
-
- /** Implements 'x ^ y'. */
- private static Object xor(Object x, Object y) throws EvalException {
- if (x instanceof Integer && y instanceof Integer) {
- return (Integer) x ^ (Integer) y;
- }
- throw unknownBinaryOperator(x, y, TokenKind.CARET);
- }
-
- /** Implements 'x >> y'. */
- private static Object rightShift(Object x, Object y) throws EvalException {
- if (x instanceof Integer && y instanceof Integer) {
- if ((Integer) y < 0) {
- throw Starlark.errorf("negative shift count: %s", y);
- } else if ((Integer) y >= Integer.SIZE) {
- return ((Integer) x < 0) ? -1 : 0;
- }
- return (Integer) x >> (Integer) y;
- }
- throw unknownBinaryOperator(x, y, TokenKind.GREATER_GREATER);
- }
-
- /** Implements 'x << y'. */
- private static Object leftShift(Object x, Object y) throws EvalException {
- if (x instanceof Integer && y instanceof Integer) {
- if ((Integer) y < 0) {
- throw Starlark.errorf("negative shift count: %s", y);
- }
- Integer result = (Integer) x << (Integer) y;
- if (!rightShift(result, y).equals(x)) {
- throw new ArithmeticException("integer overflow");
- }
- return result;
- }
- throw unknownBinaryOperator(x, y, TokenKind.LESS_LESS);
- }
-
- /** Implements 'x in y'. */
- private static boolean in(StarlarkSemantics semantics, Object x, Object y) throws EvalException {
- if (y instanceof SkylarkQueryable) {
- return ((SkylarkQueryable) y).containsKey(semantics, x);
- } else if (y instanceof String) {
- if (x instanceof String) {
- return ((String) y).contains((String) x);
- } else {
- throw Starlark.errorf(
- "'in <string>' requires string as left operand, not '%s'", Starlark.type(x));
- }
- } else {
- throw Starlark.errorf(
- "unsupported binary operation: %s in %s (right operand must be string, sequence, or"
- + " dict)",
- Starlark.type(x), Starlark.type(y));
- }
- }
-
- /** Returns an exception signifying incorrect types for the given operator. */
- private static EvalException unknownBinaryOperator(Object x, Object y, TokenKind op) {
- return Starlark.errorf(
- "unsupported binary operation: %s %s %s", Starlark.type(x), op, Starlark.type(y));
+ private static String repeatString(String s, int n) {
+ return n <= 0 ? "" : Strings.repeat(s, n);
}
/** Evaluates a unary operation. */
@@ -686,12 +671,11 @@
case MINUS:
if (x instanceof Integer) {
- try {
- return Math.negateExact((Integer) x);
- } catch (ArithmeticException e) {
- // Fails for -MIN_INT.
- throw new EvalException(null, e.getMessage());
+ int xi = (Integer) x;
+ if (xi == Integer.MIN_VALUE) {
+ throw Starlark.errorf("integer overflow in negation");
}
+ return -xi;
}
break;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/HasBinary.java b/src/main/java/com/google/devtools/build/lib/syntax/HasBinary.java
new file mode 100644
index 0000000..d5b5566
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/syntax/HasBinary.java
@@ -0,0 +1,40 @@
+// Copyright 2020 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.syntax;
+
+import javax.annotation.Nullable;
+
+/**
+ * A Starlark value that supports binary operators such as {@code x+y}.
+ *
+ * <p>During evaluation of a Starlark binary operation, if none of the built-in cases match, then
+ * the left operand is queried; if it implements HasBinary, its {@link #binaryOp} method is called.
+ * If the left operand does not implement HasBinary, or declines to implement the particular
+ * operation by returning null, then the right operand is queried for HasBinary and its {@link
+ * #binaryOp} method is called. If neither operand defines the operator, evaluation fails.
+ *
+ * <p>Subclasses should strive for appropriate symmetries in their implementations, such as {@code x
+ * * y == y * x}.
+ */
+// TODO(adonovan): rename BinaryOperand?
+public interface HasBinary extends StarlarkValue {
+
+ /**
+ * Returns {@code this op that}, if thisLeft, or {@code that op this} otherwise. May return null
+ * to indicate that the operation is not supported, or may throw a specific exception.
+ */
+ @Nullable
+ Object binaryOp(TokenKind op, Object that, boolean thisLeft) throws EvalException;
+}
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 52d464c..57b57a7 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
@@ -16,9 +16,9 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
-import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
+import java.util.Arrays;
import java.util.List;
import java.util.Objects;
@@ -44,10 +44,11 @@
doc = "A selector between configuration-dependent entities.",
documented = false)
@AutoCodec
-public final class SelectorList implements StarlarkValue {
- // TODO(build-team): Selectors are currently split between .packages and .syntax . They should
- // really all be in .packages, but then we'd need to figure out a way how to extend binary
- // operators, which is a non-trivial problem.
+public final class SelectorList implements StarlarkValue, HasBinary {
+
+ // TODO(adonovan): move to lib.packages.
+ // TODO(adonovan): combine Selector{List,Value}. There's no need for two data types.
+
private final Class<?> type;
private final List<Object> elements;
@@ -80,14 +81,21 @@
}
/**
- * Creates a list that concatenates two values, where each value may be a native
- * type, a select over that type, or a selector list over that type.
+ * Creates a list that concatenates two values, where each value may be a native type, a select
+ * over that type, or a selector list over that type.
*
* @throws EvalException if the values don't have the same underlying type
*/
- public static SelectorList concat(Location location, Object value1, Object value2)
- throws EvalException {
- return of(location, value1, value2);
+ public static SelectorList concat(Object x, Object y) throws EvalException {
+ return of(Arrays.asList(x, y));
+ }
+
+ @Override
+ public SelectorList binaryOp(TokenKind op, Object that, boolean thisLeft) throws EvalException {
+ if (op == TokenKind.PLUS) {
+ return thisLeft ? concat(this, that) : concat(that, this);
+ }
+ return null;
}
/**
@@ -96,17 +104,7 @@
*
* @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
- */
- public static SelectorList of(Location location, Iterable<?> values) throws EvalException {
+ public static SelectorList of(Iterable<?> values) throws EvalException {
Preconditions.checkArgument(!Iterables.isEmpty(values));
ImmutableList.Builder<Object> elements = ImmutableList.builder();
Object firstValue = null;
@@ -121,11 +119,9 @@
firstValue = value;
}
if (!canConcatenate(getNativeType(firstValue), getNativeType(value))) {
- throw new EvalException(
- location,
- String.format(
- "'+' operator applied to incompatible types (%s, %s)",
- getTypeName(firstValue), getTypeName(value)));
+ throw Starlark.errorf(
+ "'+' operator applied to incompatible types (%s, %s)",
+ getTypeName(firstValue), getTypeName(value));
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SelectorValue.java b/src/main/java/com/google/devtools/build/lib/syntax/SelectorValue.java
index 97c5005..ba6359c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SelectorValue.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SelectorValue.java
@@ -37,10 +37,11 @@
doc = "A selector between configuration-dependent entities.",
documented = false)
@AutoCodec
-public final class SelectorValue implements StarlarkValue {
- // TODO(bazel-team): Selectors are currently split between .packages and .syntax . They should
- // really all be in .packages, but then we'd need to figure out a way how to extend binary
- // operators, which is a non-trivial problem.
+public final class SelectorValue implements StarlarkValue, HasBinary {
+
+ // TODO(adonovan): move to lib.packages.
+ // TODO(adonovan): combine Selector{List,Value}. There's no need for two data types.
+
private final ImmutableMap<?, ?> dictionary;
private final Class<?> type;
private final String noMatchError;
@@ -74,6 +75,14 @@
}
@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;
+ }
+
+ @Override
public void repr(Printer printer) {
printer.format("select(%r)", dictionary);
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Tuple.java b/src/main/java/com/google/devtools/build/lib/syntax/Tuple.java
index aa8dd19..d502e7d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Tuple.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Tuple.java
@@ -221,7 +221,7 @@
}
/** Returns a Tuple containing n consecutive repeats of this tuple. */
- public Tuple<E> repeat(int n, Mutability mutability) {
+ Tuple<E> repeat(int n) {
if (n <= 0 || isEmpty()) {
return empty();
}
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 b18a701..9db91b9 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
@@ -23,7 +23,6 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryName;
-import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.packages.BuildType.LabelConversionContext;
import com.google.devtools.build.lib.packages.BuildType.Selector;
import com.google.devtools.build.lib.packages.Type.ConversionException;
@@ -461,7 +460,7 @@
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);
+ SelectorList result = SelectorList.concat(selectorList, list);
assertThat(result).isNotNull();
assertThat(result.getType()).isAssignableTo(List.class);
}
@@ -473,7 +472,7 @@
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);
+ SelectorList result = SelectorList.concat(selectorValue, list);
assertThat(result).isNotNull();
assertThat(result.getType()).isAssignableTo(List.class);
}
@@ -485,7 +484,7 @@
arrayList.add("//a:a");
// Creating a SelectorList from two lists of different types should work properly.
- SelectorList result = SelectorList.of(Location.BUILTIN, list, arrayList);
+ SelectorList result = SelectorList.concat(list, arrayList);
assertThat(result).isNotNull();
assertThat(result.getType()).isAssignableTo(List.class);
}
@@ -495,7 +494,7 @@
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"));
+ assertThrows(EvalException.class, () -> SelectorList.concat(list, "A string"));
}
/**
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java
index 2fa6234..b5ab54f 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java
@@ -22,6 +22,7 @@
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.StarlarkValue;
+import com.google.devtools.build.lib.syntax.TokenKind;
import javax.annotation.Nullable;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -84,8 +85,7 @@
SkylarkInfo info1 = makeInfoWithF1F2Values(provider1, 4, 5);
SkylarkInfo info2 = makeInfoWithF1F2Values(provider2, 4, 5);
EvalException expected =
- assertThrows(
- EvalException.class, () -> info1.getConcatter().concat(info1, info2, Location.BUILTIN));
+ assertThrows(EvalException.class, () -> info1.binaryOp(TokenKind.PLUS, info2, true));
assertThat(expected).hasMessageThat()
.contains("Cannot use '+' operator on instances of different providers");
}
@@ -96,8 +96,7 @@
SkylarkInfo info1 = makeInfoWithF1F2Values(provider1, 4, 5);
SkylarkInfo info2 = makeInfoWithF1F2Values(provider1, 4, null);
EvalException expected =
- assertThrows(
- EvalException.class, () -> info1.getConcatter().concat(info1, info2, Location.BUILTIN));
+ assertThrows(EvalException.class, () -> info1.binaryOp(TokenKind.PLUS, info2, true));
assertThat(expected)
.hasMessageThat()
.contains("cannot add struct instances with common field 'f1'");
@@ -108,7 +107,7 @@
SkylarkProvider provider = makeProvider();
SkylarkInfo info1 = makeInfoWithF1F2Values(provider, 4, null);
SkylarkInfo info2 = makeInfoWithF1F2Values(provider, null, 5);
- SkylarkInfo result = (SkylarkInfo) info1.getConcatter().concat(info1, info2, Location.BUILTIN);
+ SkylarkInfo result = info1.binaryOp(TokenKind.PLUS, info2, true);
assertThat(result.getFieldNames()).containsExactly("f1", "f2");
assertThat(result.getValue("f1")).isEqualTo(4);
assertThat(result.getValue("f2")).isEqualTo(5);
@@ -119,7 +118,7 @@
SkylarkProvider provider = makeProvider();
SkylarkInfo info1 = makeInfoWithF1F2Values(provider, 4, null);
SkylarkInfo info2 = makeInfoWithF1F2Values(provider, null, 5);
- SkylarkInfo result = (SkylarkInfo) info1.getConcatter().concat(info1, info2, Location.BUILTIN);
+ SkylarkInfo result = info1.binaryOp(TokenKind.PLUS, info2, true);
assertThat(result.getFieldNames()).containsExactly("f1", "f2");
assertThat(result.getValue("f1")).isEqualTo(4);
assertThat(result.getValue("f2")).isEqualTo(5);
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
index 60a0d1d..c930845 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -737,8 +737,7 @@
newTest()
.testIfErrorContains(
"'in <string>' requires string as left operand, not 'int'", "1 in '123'")
- .testIfErrorContains(
- "unsupported binary operation: string in int (right operand must be", "'a' in 1");
+ .testIfErrorContains("unsupported binary operation: string in int", "'a' in 1");
}
@Test
