GN: Throw an error overwriting a nonempty scope.

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 <stddef.h>
 #include <algorithm>
 
 #include "base/strings/string_number_conversions.h"
@@ skipping 191 matching lines @@
 }
 
 void ValueDestination::MakeUndefinedIdentifierForModifyError(Err* err) {
   // When Init() succeeds, the base of any accessor has already been resolved
   // and that list indices are in-range. This means any undefined identifiers
   // are for scope accesses.
   DCHECK(type_ == SCOPE);
   *err = Err(*name_token_, "Undefined identifier.");
 }
 
+// Computes an error message for overwriting a nonempty list/scope with another.
+Err MakeOverwriteError(const BinaryOpNode* op_node,
+                       const Value& old_value) {
+  std::string type_name;
+  std::string empty_def;
+
+  if (old_value.type() == Value::LIST) {
+    type_name = "list";
+    empty_def = "[]";
+  } else if (old_value.type() == Value::SCOPE) {
+    type_name = "scope";
+    empty_def = "{}";
+  } else {
+    NOTREACHED();
+  }
+
+  Err result(op_node->left()->GetRange(),
+      "Replacing nonempty " + type_name + ".",
+      "This overwrites a previously-defined nonempty " + type_name +
+      "with another nonempty " + type_name + ".");
+  result.AppendSubErr(Err(old_value, "for previous definition",
+      "Did you mean to append/modify instead? If you really want to overwrite, "
+      "do:\n"
+      "  foo = " + empty_def + "\nbefore reassigning."));
+  return result;
+}
+
 // -----------------------------------------------------------------------------
 
 Err MakeIncompatibleTypeError(const BinaryOpNode* op_node,
                               const Value& left,
                               const Value& right) {
   std::string msg =
       std::string("You can't do <") + Value::DescribeType(left.type()) + "> " +
       op_node->op().value().as_string() +
       " <" + Value::DescribeType(right.type()) + ">.";
   if (left.type() == Value::LIST) {
@@ skipping 68 matching lines @@
 
 // We return a null value from this rather than the result of doing the append.
 // See ValuePlusEquals for rationale.
 Value ExecuteEquals(Scope* exec_scope,
                     const BinaryOpNode* op_node,
                     ValueDestination* dest,
                     Value right,
                     Err* err) {
   const Value* old_value = dest->GetExistingValue();
   if (old_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"
-          "  foo = []\nbefore reassigning."));
+    // Check for overwriting nonempty scopes or lists with other nonempty
+    // scopes or lists. This prevents mistakes that clobber a value rather than
+    // appending to it. For cases where a user meant to clear a value, allow
+    // overwriting a nonempty list/scope with an empty one, which can then be
+    // modified.
+    if (old_value->type() == Value::LIST && right.type() == Value::LIST &&

[scottmg, 2016/08/09 22:20:33]

Something else will catch it if I try to overwrite a scope with a list or a list
with a scope, right?

[brettw, 2016/08/09 23:00:48]

Presumably that will just be wrong. If changing the type is actually OK, I don't
see a need to warn about it.

+        !old_value->list_value().empty() && !right.list_value().empty()) {
+      *err = MakeOverwriteError(op_node, *old_value);
+      return Value();
+    } else if (old_value->type() == Value::SCOPE &&
+               right.type() == Value::SCOPE &&
+               old_value->scope_value()->HasValues(Scope::SEARCH_CURRENT) &&
+               right.scope_value()->HasValues(Scope::SEARCH_CURRENT)) {
+      *err = MakeOverwriteError(op_node, *old_value);
       return Value();
     }
   }
-  if (err->has_error())
-    return Value();
 
   Value* written_value = dest->SetValue(std::move(right), op_node->right());
 
   // Optionally apply the assignment filter in-place.
   const PatternList* filter = dest->GetAssignmentFilter(exec_scope);
   if (filter) {
     std::vector<Value>& list_value = written_value->list_value();
     auto first_deleted = std::remove_if(
         list_value.begin(), list_value.end(),
         [filter](const Value& v) {
@@ skipping 420 matching lines @@
     return ExecuteGreaterEquals(scope, op_node, left_value, right_value, err);
   if (op.type() == Token::LESS_EQUAL)
     return ExecuteLessEquals(scope, op_node, left_value, right_value, err);
   if (op.type() == Token::GREATER_THAN)
     return ExecuteGreater(scope, op_node, left_value, right_value, err);
   if (op.type() == Token::LESS_THAN)
     return ExecuteLess(scope, op_node, left_value, right_value, err);
 
   return Value();
 }