GN: Throw an error overwriting a nonempty scope.
Previously GN would throw an error if you assigned a nonempty list to another
nonempty list. This is easy to mess up when you actually meant to append.
However, it did not have this protection for "scope"-type values.
Adds such protection to scope-type values, and add unit tests for both
behaviors.
Does some unit test cleanup for the operators unit tests to make them easier to
write.
BUG=
Review-Url: https://codereview.chromium.org/2224343003
Cr-Original-Commit-Position: refs/heads/master@{#410874}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 56350de24ed370f955b299022e93229f8643ace8
diff --git a/tools/gn/operators.cc b/tools/gn/operators.cc
index ac6a37f..9ac18bf 100644
--- a/tools/gn/operators.cc
+++ b/tools/gn/operators.cc
@@ -209,6 +209,33 @@
*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,
@@ -297,31 +324,23 @@
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 &&
+ !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());
diff --git a/tools/gn/operators_unittest.cc b/tools/gn/operators_unittest.cc
index e2c396c..4fa3d11 100644
--- a/tools/gn/operators_unittest.cc
+++ b/tools/gn/operators_unittest.cc
@@ -26,15 +26,6 @@
return v.string_value() == s;
}
-// Returns a list populated with a single literal Value corresponding to the
-// given token. The token must outlive the list (since the list will just
-// copy the reference).
-std::unique_ptr<ListNode> ListWithLiteral(const Token& token) {
- std::unique_ptr<ListNode> list(new ListNode);
- list->append_item(std::unique_ptr<ParseNode>(new LiteralNode(token)));
- return list;
-}
-
// This parse node is for passing to tests. It returns a canned value for
// Execute().
class TestParseNode : public ParseNode {
@@ -59,6 +50,54 @@
Value value_;
};
+// Sets up a BinaryOpNode for testing.
+class TestBinaryOpNode : public BinaryOpNode {
+ public:
+ // Input token value string must outlive class.
+ TestBinaryOpNode(Token::Type op_token_type,
+ const char* op_token_value)
+ : BinaryOpNode(),
+ op_token_ownership_(Location(), op_token_type, op_token_value) {
+ set_op(op_token_ownership_);
+ }
+
+ void SetLeftToValue(const Value& value) {
+ set_left(std::unique_ptr<ParseNode>(new TestParseNode(value)));
+ }
+
+ // Sets the left-hand side of the operator to an identifier node, this is
+ // used for testing assignments. Input string must outlive class.
+ void SetLeftToIdentifier(const char* identifier) {
+ left_identifier_token_ownership_ =
+ Token(Location(), Token::IDENTIFIER, identifier);
+ set_left(std::unique_ptr<ParseNode>(
+ new IdentifierNode(left_identifier_token_ownership_)));
+ }
+
+ void SetRightToValue(const Value& value) {
+ set_right(std::unique_ptr<ParseNode>(new TestParseNode(value)));
+ }
+ void SetRightToListOfValue(const Value& value) {
+ Value list(nullptr, Value::LIST);
+ list.list_value().push_back(value);
+ set_right(std::unique_ptr<ParseNode>(new TestParseNode(list)));
+ }
+ void SetRightToListOfValue(const Value& value1, const Value& value2) {
+ Value list(nullptr, Value::LIST);
+ list.list_value().push_back(value1);
+ list.list_value().push_back(value2);
+ set_right(std::unique_ptr<ParseNode>(new TestParseNode(list)));
+ }
+
+ private:
+ // The base class takes the Token by reference, this manages the lifetime.
+ Token op_token_ownership_;
+
+ // When setting the left to an identifier, this manages the lifetime of
+ // the identifier token.
+ Token left_identifier_token_ownership_;
+};
+
} // namespace
TEST(Operators, SourcesAppend) {
@@ -70,15 +109,8 @@
setup.scope()->SetValue(sources, Value(nullptr, Value::LIST), nullptr);
// Set up the operator.
- BinaryOpNode node;
- const char token_value[] = "+=";
- Token op(Location(), Token::PLUS_EQUALS, token_value);
- node.set_op(op);
-
- // Append to the sources variable.
- Token identifier_token(Location(), Token::IDENTIFIER, sources);
- node.set_left(
- std::unique_ptr<ParseNode>(new IdentifierNode(identifier_token)));
+ TestBinaryOpNode node(Token::PLUS_EQUALS, "+=");
+ node.SetLeftToIdentifier(sources);
// Set up the filter on the scope to remove everything ending with "rm"
std::unique_ptr<PatternList> pattern_list(new PatternList);
@@ -86,31 +118,25 @@
setup.scope()->set_sources_assignment_filter(std::move(pattern_list));
// Append an integer.
- const char integer_value[] = "5";
- Token integer(Location(), Token::INTEGER, integer_value);
- node.set_right(ListWithLiteral(integer));
+ node.SetRightToListOfValue(Value(nullptr, static_cast<int64_t>(5)));
node.Execute(setup.scope(), &err);
EXPECT_FALSE(err.has_error());
// Append a string that doesn't match the pattern, it should get appended.
- const char string_1_value[] = "\"good\"";
- Token string_1(Location(), Token::STRING, string_1_value);
- node.set_right(ListWithLiteral(string_1));
+ const char string1[] = "good";
+ node.SetRightToListOfValue(Value(nullptr, string1));
node.Execute(setup.scope(), &err);
EXPECT_FALSE(err.has_error());
// Append a string that does match the pattern, it should be a no-op.
- const char string_2_value[] = "\"foo-rm\"";
- Token string_2(Location(), Token::STRING, string_2_value);
- node.set_right(ListWithLiteral(string_2));
+ const char string2[] = "foo-rm";
+ node.SetRightToListOfValue(Value(nullptr, string2));
node.Execute(setup.scope(), &err);
EXPECT_FALSE(err.has_error());
// Append a list with the two strings from above.
- ListNode list;
- list.append_item(std::unique_ptr<ParseNode>(new LiteralNode(string_1)));
- list.append_item(std::unique_ptr<ParseNode>(new LiteralNode(string_2)));
- ExecuteBinaryOperator(setup.scope(), &node, node.left(), &list, &err);
+ node.SetRightToListOfValue(Value(nullptr, string1), Value(nullptr, string2));
+ node.Execute(setup.scope(), &err);
EXPECT_FALSE(err.has_error());
// The sources variable in the scope should now have: [ 5, "good", "good" ]
@@ -133,23 +159,14 @@
const char foo[] = "foo";
setup.scope()->SetValue(foo, Value(nullptr, Value::LIST), nullptr);
- // Set up the operator.
- BinaryOpNode node;
- const char token_value[] = "+=";
- Token op(Location(), Token::PLUS_EQUALS, token_value);
- node.set_op(op);
-
- // Append to the foo variable.
- Token identifier_token(Location(), Token::IDENTIFIER, foo);
- node.set_left(
- std::unique_ptr<ParseNode>(new IdentifierNode(identifier_token)));
+ // Set up the operator to append to "foo".
+ TestBinaryOpNode node(Token::PLUS_EQUALS, "+=");
+ node.SetLeftToIdentifier(foo);
// Append a list with a list, the result should be a nested list.
- std::unique_ptr<ListNode> outer_list(new ListNode);
- const char twelve_str[] = "12";
- Token twelve(Location(), Token::INTEGER, twelve_str);
- outer_list->append_item(ListWithLiteral(twelve));
- node.set_right(std::move(outer_list));
+ Value inner_list(nullptr, Value::LIST);
+ inner_list.list_value().push_back(Value(nullptr, static_cast<int64_t>(12)));
+ node.SetRightToListOfValue(inner_list);
Value ret = ExecuteBinaryOperator(setup.scope(), &node, node.left(),
node.right(), &err);
@@ -177,7 +194,7 @@
EXPECT_TRUE(err.has_error());
err = Err();
- node.set_right(std::unique_ptr<ParseNode>(new LiteralNode(twelve)));
+ node.SetRightToValue(Value(nullptr, static_cast<int64_t>(12)));
ExecuteBinaryOperator(setup.scope(), &node, node.left(), node.right(), &err);
EXPECT_TRUE(err.has_error());
}
@@ -194,26 +211,14 @@
test_list.list_value().push_back(Value(nullptr, foo_str));
// Set up "var" with an the test list.
- const char var_str[] = "var";
- setup.scope()->SetValue(var_str, test_list, nullptr);
+ const char var[] = "var";
+ setup.scope()->SetValue(var, test_list, nullptr);
- // Set up the operator.
- BinaryOpNode node;
- const char token_value[] = "-=";
- Token op(Location(), Token::MINUS_EQUALS, token_value);
- node.set_op(op);
-
- // Do -= on the var.
- Token identifier_token(Location(), Token::IDENTIFIER, var_str);
- node.set_left(
- std::unique_ptr<ParseNode>(new IdentifierNode(identifier_token)));
+ TestBinaryOpNode node(Token::MINUS_EQUALS, "-=");
+ node.SetLeftToIdentifier(var);
// Subtract a list consisting of "foo".
- Value foo_list(nullptr, Value::LIST);
- foo_list.list_value().push_back(Value(nullptr, foo_str));
- std::unique_ptr<ParseNode> outer_list(new TestParseNode(foo_list));
- node.set_right(std::move(outer_list));
-
+ node.SetRightToListOfValue(Value(nullptr, foo_str));
Value result = ExecuteBinaryOperator(
setup.scope(), &node, node.left(), node.right(), &err);
EXPECT_FALSE(err.has_error());
@@ -224,7 +229,7 @@
// The "var" variable should have been updated. Both instances of "foo" are
// deleted.
- const Value* new_value = setup.scope()->GetValue(var_str);
+ const Value* new_value = setup.scope()->GetValue(var);
ASSERT_TRUE(new_value);
ASSERT_EQ(Value::LIST, new_value->type());
ASSERT_EQ(1u, new_value->list_value().size());
@@ -236,16 +241,9 @@
Err err;
TestWithScope setup;
- // Set up the operator.
- BinaryOpNode node;
- const char token_value[] = "&&";
- Token op(Location(), Token::BOOLEAN_AND, token_value);
- node.set_op(op);
-
- // Set the left to false.
- const char false_str[] = "false";
- Token false_tok(Location(), Token::FALSE_TOKEN, false_str);
- node.set_left(std::unique_ptr<ParseNode>(new LiteralNode(false_tok)));
+ // Set a && operator with the left to false.
+ TestBinaryOpNode node(Token::BOOLEAN_AND, "&&");
+ node.SetLeftToValue(Value(nullptr, false));
// Set right as foo, but don't define a value for it.
const char foo[] = "foo";
@@ -262,16 +260,9 @@
Err err;
TestWithScope setup;
- // Set up the operator.
- BinaryOpNode node;
- const char token_value[] = "||";
- Token op(Location(), Token::BOOLEAN_OR, token_value);
- node.set_op(op);
-
- // Set the left to false.
- const char false_str[] = "true";
- Token false_tok(Location(), Token::TRUE_TOKEN, false_str);
- node.set_left(std::unique_ptr<ParseNode>(new LiteralNode(false_tok)));
+ // Set a || operator with the left to true.
+ TestBinaryOpNode node(Token::BOOLEAN_OR, "||");
+ node.SetLeftToValue(Value(nullptr, true));
// Set right as foo, but don't define a value for it.
const char foo[] = "foo";
@@ -283,3 +274,59 @@
node.right(), &err);
EXPECT_FALSE(err.has_error());
}
+
+// Overwriting nonempty lists and scopes with other nonempty lists and scopes
+// should be disallowed.
+TEST(Operators, NonemptyOverwriting) {
+ Err err;
+ TestWithScope setup;
+
+ // Set up "foo" with a nonempty list.
+ const char foo[] = "foo";
+ Value old_value(nullptr, Value::LIST);
+ old_value.list_value().push_back(Value(nullptr, "string"));
+ setup.scope()->SetValue(foo, old_value, nullptr);
+
+ TestBinaryOpNode node(Token::EQUAL, "=");
+ node.SetLeftToIdentifier(foo);
+
+ // Assigning a nonempty list should fail.
+ node.SetRightToListOfValue(Value(nullptr, "string"));
+ node.Execute(setup.scope(), &err);
+ ASSERT_TRUE(err.has_error());
+ EXPECT_EQ("Replacing nonempty list.", err.message());
+ err = Err();
+
+ // Assigning an empty list should succeed.
+ node.SetRightToValue(Value(nullptr, Value::LIST));
+ node.Execute(setup.scope(), &err);
+ ASSERT_FALSE(err.has_error());
+ const Value* new_value = setup.scope()->GetValue(foo);
+ ASSERT_TRUE(new_value);
+ ASSERT_EQ(Value::LIST, new_value->type());
+ ASSERT_TRUE(new_value->list_value().empty());
+
+ // Set up "foo" with a nonempty scope.
+ const char bar[] = "bar";
+ old_value = Value(
+ nullptr, std::unique_ptr<Scope>(new Scope(setup.settings())));
+ old_value.scope_value()->SetValue(bar, Value(nullptr, "bar"), nullptr);
+ setup.scope()->SetValue(foo, old_value, nullptr);
+
+ // Assigning a nonempty scope should fail (re-use old_value copy).
+ node.SetRightToValue(old_value);
+ node.Execute(setup.scope(), &err);
+ ASSERT_TRUE(err.has_error());
+ EXPECT_EQ("Replacing nonempty scope.", err.message());
+ err = Err();
+
+ // Assigning an empty list should succeed.
+ node.SetRightToValue(
+ Value(nullptr, std::unique_ptr<Scope>(new Scope(setup.settings()))));
+ node.Execute(setup.scope(), &err);
+ ASSERT_FALSE(err.has_error());
+ new_value = setup.scope()->GetValue(foo);
+ ASSERT_TRUE(new_value);
+ ASSERT_EQ(Value::SCOPE, new_value->type());
+ ASSERT_FALSE(new_value->scope_value()->HasValues(Scope::SEARCH_CURRENT));
+}
diff --git a/tools/gn/scope.cc b/tools/gn/scope.cc
index 7e7c201..fbe73f4 100644
--- a/tools/gn/scope.cc
+++ b/tools/gn/scope.cc
@@ -71,6 +71,11 @@
mutable_containing_ = nullptr;
}
+bool Scope::HasValues(SearchNested search_nested) const {
+ DCHECK(search_nested == SEARCH_CURRENT);
+ return !values_.empty();
+}
+
const Value* Scope::GetValue(const base::StringPiece& ident,
bool counts_as_used) {
// First check for programmatically-provided values.
diff --git a/tools/gn/scope.h b/tools/gn/scope.h
index 3d36712..b87afcd 100644
--- a/tools/gn/scope.h
+++ b/tools/gn/scope.h
@@ -125,6 +125,14 @@
// self-sufficient.
void DetachFromContaining();
+ // Returns true if the scope has any values set. This does not check other
+ // things that may be set like templates or defaults.
+ //
+ // Currently this does not search nested scopes and this will assert if you
+ // want to search nested scopes. The enum is passed so the callers are
+ // unambiguous about nested scope handling. This can be added if needed.
+ bool HasValues(SearchNested search_nested) const;
+
// Returns NULL if there's no such value.
//
// counts_as_used should be set if the variable is being read in a way that
@@ -133,16 +141,6 @@
bool counts_as_used);
const Value* GetValue(const base::StringPiece& ident) const;
- // If the value exists in the current scope, destrictively moves it into the
- // return value. If it exists in a containing scope, copies it.
- //
- // This is for implementing modify-write operations where we want to read
- // the existing value and plan to immediately overwrite it. If the value is
- // in a containing scope, we never want to touch it (all writes go to the
- // current scope), but if it's in the current scope, avoid the copy since it
- // will be overwritten anyway.
- //Value DestructiveMoveOut(const base::StringPiece& ident);
-
// Returns the requested value as a mutable one if possible. If the value
// is not found in a mutable scope, then returns null. Note that the value
// could still exist in a const scope, so GetValue() could still return
