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