diff options
6 files changed, 112 insertions, 29 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/AugmentedAssignmentStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/AugmentedAssignmentStatement.java index c11722469e..d869abae86 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/AugmentedAssignmentStatement.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/AugmentedAssignmentStatement.java @@ -14,8 +14,6 @@ package com.google.devtools.build.lib.syntax; -import com.google.devtools.build.lib.events.Location; - /** Syntax node for an augmented assignment statement. */ public final class AugmentedAssignmentStatement extends Statement { @@ -54,8 +52,7 @@ public final class AugmentedAssignmentStatement extends Statement { @Override void doExec(Environment env) throws EvalException, InterruptedException { - Location loc = getLocation(); - lvalue.assign(env, loc, expression, operator); + lvalue.assign(env, getLocation(), expression, operator); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java index e6d37905c5..ec81e07219 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java @@ -92,19 +92,14 @@ public final class BinaryOperatorExpression extends Expression { } } - /** - * Helper method. Reused from AugmentedAssignmentStatement class which falls back to this method - * in most of the cases. - */ + /** Helper method. Reused from the LValue class. */ public static Object evaluate( - Operator operator, Expression lhs, Expression rhs, Environment env, Location location) - throws EvalException, InterruptedException { - Object lval = lhs.eval(env); - return evaluate(operator, lval, rhs, env, location); - } - - public static Object evaluate( - Operator operator, Object lval, Expression rhs, Environment env, Location location) + Operator operator, + Object lval, + Expression rhs, + Environment env, + Location location, + boolean isAugmented) throws EvalException, InterruptedException { // Short-circuit operators if (operator == Operator.AND) { @@ -127,7 +122,7 @@ public final class BinaryOperatorExpression extends Expression { switch (operator) { case PLUS: - return plus(lval, rval, env, location); + return plus(lval, rval, env, location, isAugmented); case PIPE: return pipe(lval, rval, location); @@ -175,7 +170,7 @@ public final class BinaryOperatorExpression extends Expression { @Override Object doEval(Environment env) throws EvalException, InterruptedException { - return evaluate(operator, lhs, rhs, env, getLocation()); + return evaluate(operator, lhs.eval(env), rhs, env, getLocation(), false); } @Override @@ -190,7 +185,8 @@ public final class BinaryOperatorExpression extends Expression { } /** Implements Operator.PLUS. */ - private static Object plus(Object lval, Object rval, Environment env, Location location) + private static Object plus( + Object lval, Object rval, Environment env, Location location, boolean isAugmented) throws EvalException { // int + int if (lval instanceof Integer && rval instanceof Integer) { @@ -213,6 +209,12 @@ public final class BinaryOperatorExpression extends Expression { } if ((lval instanceof MutableList) && (rval instanceof MutableList)) { + if (isAugmented && env.getSemantics().incompatibleListPlusEquals) { + @SuppressWarnings("unchecked") + MutableList<Object> list = (MutableList) lval; + list.addAll((MutableList<?>) rval, location, env); + return list; + } return MutableList.concat((MutableList) lval, (MutableList) rval, env); } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java index ecc72073fe..1b7278b6a7 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java @@ -53,7 +53,8 @@ public class LValue implements Serializable { public void assign(Environment env, Location loc, Expression rhs, Operator operator) throws EvalException, InterruptedException { if (expr instanceof Identifier) { - Object result = BinaryOperatorExpression.evaluate(operator, expr, rhs, env, loc); + Object result = + BinaryOperatorExpression.evaluate(operator, expr.eval(env), rhs, env, loc, true); assign(env, loc, (Identifier) expr, result); return; } @@ -64,7 +65,8 @@ public class LValue implements Serializable { Object evaluatedLhsObject = indexExpression.getObject().eval(env); Object evaluatedLhs = indexExpression.eval(env, evaluatedLhsObject); Object key = indexExpression.getKey().eval(env); - Object result = BinaryOperatorExpression.evaluate(operator, evaluatedLhs, rhs, env, loc); + Object result = + BinaryOperatorExpression.evaluate(operator, evaluatedLhs, rhs, env, loc, true); assignItem(env, loc, evaluatedLhsObject, key, result); return; } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java index 0c83c25760..5909ea7155 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java @@ -58,4 +58,14 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable help = "If set to true, disables the keyword-only argument syntax in function definition." ) public boolean incompatibleKeywordOnlySyntax; + + @Option( + name = "incompatible_list_plus_equals", + defaultValue = "false", + category = "incompatible changes", + help = + "If set to true, `+=` on lists works like the `extend` method mutating the original " + + "list. Otherwise it copies the original list without mutating it." + ) + public boolean incompatibleListPlusEquals; } diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java index 848498b229..a71f656070 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java @@ -987,6 +987,73 @@ public class SkylarkEvaluationTest extends EvaluationTest { } @Test + public void testPlusEqualsOnListCopying() throws Exception { + new SkylarkTest("--incompatible_list_plus_equals=false") + .setUp( + "def func():", + " l1 = [1, 2]", + " l2 = l1", + " l2 += [3, 4]", + " return l1, l2", + "lists = str(func())") + .testLookup("lists", "([1, 2], [1, 2, 3, 4])"); + } + + @Test + public void testPlusEqualsOnListMutating() throws Exception { + new SkylarkTest("--incompatible_list_plus_equals=true") + .setUp( + "def func():", + " l1 = [1, 2]", + " l2 = l1", + " l2 += [3, 4]", + " return l1, l2", + "lists = str(func())") + .testLookup("lists", "([1, 2, 3, 4], [1, 2, 3, 4])"); + + // The same but with += after an IndexExpression + new SkylarkTest("--incompatible_list_plus_equals=true") + .setUp( + "def func():", + " l = [1, 2]", + " d = {0: l}", + " d[0] += [3, 4]", + " return l, d[0]", + "lists = str(func())") + .testLookup("lists", "([1, 2, 3, 4], [1, 2, 3, 4])"); + } + + @Test + public void testPlusEqualsOnTuple() throws Exception { + new SkylarkTest("--incompatible_list_plus_equals=false") + .setUp( + "def func():", + " t1 = (1, 2)", + " t2 = t1", + " t2 += (3, 4)", + " return t1, t2", + "tuples = func()") + .testLookup("tuples", SkylarkList.Tuple.of( + SkylarkList.Tuple.of(1, 2), + SkylarkList.Tuple.of(1, 2, 3, 4) + )); + + // This behavior should remain the same regardless of the incompatible_list_plus_equals flag + new SkylarkTest("--incompatible_list_plus_equals=true") + .setUp( + "def func():", + " t1 = (1, 2)", + " t2 = t1", + " t2 += (3, 4)", + " return t1, t2", + "tuples = func()") + .testLookup("tuples", SkylarkList.Tuple.of( + SkylarkList.Tuple.of(1, 2), + SkylarkList.Tuple.of(1, 2, 3, 4) + )); + } + + @Test public void testPlusEqualsOnDict() throws Exception { new SkylarkTest().setUp("def func():", " d = {'a' : 1}", diff --git a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java index 0aa9b0a1e4..33ce2d8d09 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java @@ -115,20 +115,21 @@ public class EvaluationTestCase { /** * Sets the specified {@code TestMode} and tries to create the appropriate {@code Environment} + * * @param testMode * @throws Exception */ - protected void setMode(TestMode testMode) throws Exception { + protected void setMode(TestMode testMode, String... skylarkOptions) throws Exception { this.testMode = testMode; - env = newEnvironment(); + env = newEnvironmentWithSkylarkOptions(skylarkOptions); } - protected void enableSkylarkMode() throws Exception { - setMode(TestMode.SKYLARK); + protected void enableSkylarkMode(String... skylarkOptions) throws Exception { + setMode(TestMode.SKYLARK, skylarkOptions); } - protected void enableBuildMode() throws Exception { - setMode(TestMode.BUILD); + protected void enableBuildMode(String... skylarkOptions) throws Exception { + setMode(TestMode.BUILD, skylarkOptions); } public EventHandler getEventHandler() { @@ -560,11 +561,15 @@ public class EvaluationTestCase { * A class that runs all tests in Skylark mode */ protected class SkylarkTest extends ModalTestCase { - public SkylarkTest() {} + private final String[] skylarkOptions; + + public SkylarkTest(String... skylarkOptions) { + this.skylarkOptions = skylarkOptions; + } @Override protected void run(Testable testable) throws Exception { - enableSkylarkMode(); + enableSkylarkMode(skylarkOptions); testable.run(); } } |