Remove some appending behavior from GN.

tools/gn/operators.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "tools/gn/operators.h"
 
 #include "base/strings/string_number_conversions.h"
 #include "tools/gn/err.h"
 #include "tools/gn/parse_tree.h"
 #include "tools/gn/scope.h"
@@ skipping 78 matching lines @@
       }
       break;
 
     default:
       break;
   }
 }
 
 // Assignment -----------------------------------------------------------------
 
+// We return a null value from this rather than the result of doing the append.
+// See ValuePlusEquals for rationale.
 Value ExecuteEquals(Scope* scope,
                     const BinaryOpNode* op_node,
                     const Token& left,
                     const Value& right,
                     Err* err) {
   const Value* old_value = scope->GetValue(left.value(), false);
   if (old_value) {
-    if (scope->IsSetButUnused(left.value())) {
-      // Throw an error for re-assigning without using the value first. The
-      // exception is that you can overwrite an empty list with another list
-      // since this is the way to get around the "can't overwrite a nonempty
-      // list with another nonempty list" restriction.
-      if (old_value->type() != Value::LIST ||
-          !old_value->list_value().empty()) {
-        *err = Err(op_node->left()->GetRange(), "Overwriting unused variable.",
-            "This overwrites a previous assignment to \"" +
-            left.value().as_string() + "\" that had no effect.");
-        err->AppendSubErr(Err(*scope->GetValue(left.value()),
-                              "Previously set here.",
-                              "Maybe you wanted \"+=\" to append instead?"));
-        return Value();
-      }
-    } else {
-      // Throw an error when overwriting a nonempty list with another nonempty
-      // list item. This is to detect the case where you write
-      //   defines = ["FOO"]
-      // and you overwrote inherited ones, when instead you mean to append:
-      //   defines += ["FOO"]
-      if (old_value->type() == Value::LIST &&
-          !old_value->list_value().empty() &&
-          right.type() == Value::LIST &&
-          !right.list_value().empty()) {
-        *err = Err(op_node->left()->GetRange(), "Replacing nonempty list.",
-            std::string("This overwrites a previously-defined nonempty list ") +
-            "(length " +
-            base::IntToString(static_cast<int>(old_value->list_value().size()))
-            + ").");
-        err->AppendSubErr(Err(*old_value, "for previous definition",
-            "with another one (length " +
-            base::IntToString(static_cast<int>(right.list_value().size())) +
-            "). Did you mean " +
-            "\"+=\" to append instead? If you\nreally want to do this, do\n  " +
-            left.value().as_string() + " = []\nbefore reassigning."));
-        return Value();
-      }
+    // Throw an error when overwriting a nonempty list with another nonempty
+    // list item. This is to detect the case where you write
+    //   defines = ["FOO"]
+    // and you overwrote inherited ones, when instead you mean to append:
+    //   defines += ["FOO"]
+    if (old_value->type() == Value::LIST &&
+        !old_value->list_value().empty() &&
+        right.type() == Value::LIST &&
+        !right.list_value().empty()) {
+      *err = Err(op_node->left()->GetRange(), "Replacing nonempty list.",
+          std::string("This overwrites a previously-defined nonempty list ") +
+          "(length " +
+          base::IntToString(static_cast<int>(old_value->list_value().size()))
+          + ").");
+      err->AppendSubErr(Err(*old_value, "for previous definition",
+          "with another one (length " +
+          base::IntToString(static_cast<int>(right.list_value().size())) +
+          "). Did you mean " +
+          "\"+=\" to append instead? If you\nreally want to do this, do\n  " +
+          left.value().as_string() + " = []\nbefore reassigning."));
+      return Value();
     }
   }
   if (err->has_error())
     return Value();
 
   if (right.type() == Value::LIST && left.value() == kSourcesName) {
     // Assigning to sources, filter the list. Here we do the filtering and
     // copying in one step to save an extra list copy (the lists may be
     // long).
     Value* set_value = scope->SetValue(left.value(),
                                        Value(op_node, Value::LIST), op_node);
     set_value->list_value().reserve(right.list_value().size());
     AppendFilteredSourcesToValue(scope, right, set_value);
   } else {
     // Normal value set, just copy it.
     scope->SetValue(left.value(), right, op_node->right());
   }
   return Value();
 }
 
 // allow_type_conversion indicates if we're allowed to change the type of the
 // left value. This is set to true when doing +, and false when doing +=.
+//
+// Note that we return Value() from here, which is different than C++. This
+// means you can't do clever things like foo = [ bar += baz ] to simultaneously
+// append to and use a value. This is basically never needed in out build
+// scripts and is just as likely an error as the intended behavior, and it also
+// involves a copy of the value when it's returned. Many times we're appending
+// to large lists, and copying the value to discard it for the next statement
+// is very wasteful.
 void ValuePlusEquals(const Scope* scope,
                      const BinaryOpNode* op_node,
                      const Token& left_token,
                      Value* left,
                      const Value& right,
                      bool allow_type_conversion,
                      Err* err) {
   switch (left->type()) {
     // Left-hand-side int.
     case Value::INTEGER:
@@ skipping 27 matching lines @@
           return;
 
         default:
           break;
       }
       break;
 
     // Left-hand-side list.
     case Value::LIST:
       switch (right.type()) {
-        case Value::INTEGER:  // list + integer -> list append.
-        case Value::STRING:  // list + string -> list append.
-          if (left_token.value() == kSourcesName)
-            AppendFilteredSourcesToValue(scope, right, left);
-          else
-            left->list_value().push_back(right);
-          return;
-
         case Value::LIST:  // list + list -> list concat.
           if (left_token.value() == kSourcesName) {
             // Filter additions through the assignment filter.
             AppendFilteredSourcesToValue(scope, right, left);
           } else {
             // Normal list concat.
             for (size_t i = 0; i < right.list_value().size(); i++)
               left->list_value().push_back(right.list_value()[i]);
           }
           return;
 
         default:
-          break;
+          *err = Err(op_node->op(), "Incompatible types to add.",
+              "To append a single item to a list do \"foo += [ bar ]\".");
+          return;
       }
 
     default:
       break;
   }
 
   *err = Err(op_node->op(), "Incompatible types to add.",
       std::string("I see a ") + Value::DescribeType(left->type()) + " and a " +
       Value::DescribeType(right.type()) + ".");
 }
@@ skipping 11 matching lines @@
     *err = Err(left, "Undefined variable for +=.",
         "I don't have something with this name in scope now.");
     return Value();
   }
   ValuePlusEquals(scope, op_node, left, left_value, right, false, err);
   left_value->set_origin(op_node);
   scope->MarkUnused(left.value());
   return Value();
 }
 
+// We return a null value from this rather than the result of doing the append.
+// See ValuePlusEquals for rationale.
 void ValueMinusEquals(const BinaryOpNode* op_node,
                       Value* left,
                       const Value& right,
                       bool allow_type_conversion,
                       Err* err) {
   switch (left->type()) {
     // Left-hand-side int.
     case Value::INTEGER:
       switch (right.type()) {
         case Value::INTEGER:  // int - int -> subtraction.
           left->int_value() -= right.int_value();
           return;
 
         default:
           break;
       }
       break;
 
     // Left-hand-side string.
     case Value::STRING:
       break;  // All are errors.
 
     // Left-hand-side list.
     case Value::LIST:
-      RemoveMatchesFromList(op_node, left, right, err);
+      if (right.type() != Value::LIST) {
+        *err = Err(op_node->op(), "Incompatible types to subtract.",
+            "To remove a single item from a list do \"foo -= [ bar ]\".");
+      } else {
+        RemoveMatchesFromList(op_node, left, right, err);
+      }
       return;
 
     default:
       break;
   }
 
   *err = Err(op_node->op(), "Incompatible types to subtract.",
       std::string("I see a ") + Value::DescribeType(left->type()) + " and a " +
       Value::DescribeType(right.type()) + ".");
 }
@@ skipping 285 matching lines @@
     return ExecuteLess(scope, op_node, left_value, right_value, err);
 
   // ||, &&.
   if (op.type() == Token::BOOLEAN_OR)
     return ExecuteOr(scope, op_node, left_value, right_value, err);
   if (op.type() == Token::BOOLEAN_AND)
     return ExecuteAnd(scope, op_node, left_value, right_value, err);
 
   return Value();
 }