Implement a flag for extend-like behavior of the `+=` operator for lists

Usage: --incompatible_list_plus_equals=true (the default value is false).
PiperOrigin-RevId: 155084916

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();
     }
   }