Implement `gn edit "remove attr value(s)"` Change-Id: Ib4e05cd443a60e16843d386565d0a35b6a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/25440 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: Matt Stark <msta@google.com>
diff --git a/docs/reference.md b/docs/reference.md index 671d3d0..44a5842 100644 --- a/docs/reference.md +++ b/docs/reference.md
@@ -740,6 +740,12 @@ Example: gn edit "remove testonly" //src/tools:* + remove <attribute> <value(s)> + Removes <value(s)> from the list attribute <attribute>. + + Example: + gn edit "remove deps //base" //src/tools:* + set <attribute>[:list] <value(s)> Sets or overwrites the target's <attribute> to <value(s)>. If multiple values are provided, or if the ":list" suffix is
diff --git a/src/gn/build_file_editor.cc b/src/gn/build_file_editor.cc index e784edc..b8c5721 100644 --- a/src/gn/build_file_editor.cc +++ b/src/gn/build_file_editor.cc
@@ -24,19 +24,99 @@ namespace { -std::optional<std::string> AsStringLiteral(const ParseNode* node) { +std::optional<Value> AsLiteralValue(const ParseNode* node) { auto* literal = node->AsLiteral(); - if (!literal || literal->value().type() != Token::STRING) { + if (!literal) { return std::nullopt; } Scope scope(static_cast<const Settings*>(nullptr)); Err err; Value v = literal->Execute(&scope, &err); - // Because we provide an empty scope, "${b}" will result in an error. - if (err.has_error() || v.type() != Value::STRING) { + // Literals should *usually* not error out, but there are some cases they do. + // Eg. the string literal "${foo}" with no variable foo in scope. + // When this happens, just treat them as if they're opaque things we don't + // know about. + if (err.has_error()) { return std::nullopt; } - return std::move(v.string_value()); + return v; +} + +std::optional<std::string> AsStringLiteral(const ParseNode* node) { + auto val = AsLiteralValue(node); + if (val && val->type() == Value::STRING) { + return std::move(val->string_value()); + } + return std::nullopt; +} + +// Returns true if a node in the tree is a literal node matching the user's +// request. +bool Matches(const EditTarget& target, + const ParseNode* node, + const Value& value) { + auto got_value = AsLiteralValue(node); + if (!got_value) { + return false; + } + if (*got_value == value) { + return true; + } + if (got_value->type() == Value::STRING && value.type() == Value::STRING) { + // If the user requests "remove deps //foo:bar" //foo:baz, and //foo:baz + // contains the literal ":bar", that should match. + Err err; + Label got_label = + Label::Resolve(target.label.dir(), "", target.label.GetToolchainLabel(), + *got_value, &err); + Label want_label = Label::Resolve( + target.label.dir(), "", target.label.GetToolchainLabel(), value, &err); + return !err.has_error() && got_label == want_label; + } + return false; +} + +// Finds matching nodes in an expression. +template <typename T> +void FindExpressionRecursive( + ParseNode* node, + std::vector<ParseNode*>& stack, + const std::function<std::optional<T>(TreeNode&)>& transform, + std::vector<T>* results) { + if (!node) + return; + + stack.push_back(node); + + TreeNode node_ref(stack); + if (auto mapped = transform(node_ref)) { + results->push_back(std::move(*mapped)); + } + + if (auto* list = node->AsListMut()) { + for (auto& item : list->contents()) { + FindExpressionRecursive(item.get(), stack, transform, results); + } + } else if (auto* op = node->AsBinaryOpMut()) { + if (op->op().type() == Token::PLUS) { + FindExpressionRecursive(op->left(), stack, transform, results); + FindExpressionRecursive(op->right(), stack, transform, results); + } + } + + stack.pop_back(); +} + +// Finds matching nodes in an expression. +template <typename T> +std::vector<T> FindExpression( + const TreeNode& root, + const std::function<std::optional<T>(TreeNode&)>& transform) { + std::vector<T> results; + std::vector<ParseNode*> stack = root.stack(); + stack.pop_back(); + FindExpressionRecursive<T>(root.node(), stack, transform, &results); + return results; } // Resolves a single LabelPattern to matching SourceFiles. @@ -88,6 +168,39 @@ } // namespace +std::vector<TreeNode> FindListElementInAssignment(const EditTarget& target, + const TreeNode& root, + const Value& value) { + auto* node = root.AsAssignment(); + if (!node) + return {}; + return FindExpression<TreeNode>( + root.Descend(node->right()), + [&](TreeNode& node_ref) -> std::optional<TreeNode> { + if (node_ref.parent() && node_ref.parent()->AsList() && + Matches(target, node_ref.node(), value)) { + return node_ref; + } + return std::nullopt; + }); +} + +TreeNode TreeNode::Descend(ParseNode* child) const { + std::vector<ParseNode*> s = stack_; + s.push_back(child); + return TreeNode(std::move(s)); +} + +BinaryOpNode* TreeNode::AsAssignment() const { + if (auto* op = node()->AsBinaryOpMut()) { + if (op->op().type() == Token::EQUAL || + op->op().type() == Token::PLUS_EQUALS) { + return op; + } + } + return nullptr; +} + bool TreeNode::is_conditional() const { DCHECK(!stack_.empty()) << "stack should never be empty"; for (auto it = stack_.rbegin() + 1; it != stack_.rend(); ++it) {
diff --git a/src/gn/build_file_editor.h b/src/gn/build_file_editor.h index c4a2813..dd39702 100644 --- a/src/gn/build_file_editor.h +++ b/src/gn/build_file_editor.h
@@ -38,6 +38,9 @@ return stack_.size() > 1 ? stack_[stack_.size() - 2] : nullptr; } + // Returns the BinaryOpNode if the node is an assignment ("=" or "+="). + BinaryOpNode* AsAssignment() const; + // Returns whether the node is conditional in a target. // Note that if the target itself is conditional, this will return false. bool is_conditional() const; @@ -55,6 +58,10 @@ ParseNode* operator->() const { return stack_.back(); } + const std::vector<ParseNode*>& stack() const { return stack_; } + + TreeNode Descend(ParseNode* child) const; + private: // Low-level deletion from parent block or list. void RemoveSelf() const; @@ -106,6 +113,12 @@ return results; } +// Finds an element in an assignment expression ("=" or "+=") whose right-hand +// side likely evaluates to a list. +std::vector<TreeNode> FindListElementInAssignment(const EditTarget& target, + const TreeNode& root, + const Value& value); + // Represents a set of patterns within a build file. class LabelMatcher { public:
diff --git a/src/gn/edit_command.cc b/src/gn/edit_command.cc index 2ca0d3d..7a464c3 100644 --- a/src/gn/edit_command.cc +++ b/src/gn/edit_command.cc
@@ -47,6 +47,12 @@ " Example:\n" " gn edit \"remove testonly\" //src/tools:*\n" "\n" + " remove <attribute> <value(s)>\n" + " Removes <value(s)> from the list attribute <attribute>.\n" + "\n" + " Example:\n" + " gn edit \"remove deps //base\" //src/tools:*\n" + "\n" " set <attribute>[:list] <value(s)>\n" " Sets or overwrites the target's <attribute> to <value(s)>.\n" " If multiple values are provided, or if the \":list\" suffix is\n"
diff --git a/src/gn/edit_command_unittest.cc b/src/gn/edit_command_unittest.cc index cea3a60..25c0948 100644 --- a/src/gn/edit_command_unittest.cc +++ b/src/gn/edit_command_unittest.cc
@@ -203,6 +203,53 @@ "attribute \"nonexistent_attribute\".")}})); } +TEST_F(EditCommandTest, RemoveFromAttributeSubcommand) { + EXPECT_SUCCESS(DoEdit("remove deps //base :bar", + R"( +executable("foo") { + deps = [ + "//base", + "//:bar", + "//other:bar", + ] +} +)"), + Edited(R"( +executable("foo") { + deps = [ "//other:bar" ] +} +)")); + + EXPECT_SUCCESS(DoEdit("remove deps //base //nonexistent:glob", + R"( +executable("foo") { + deps = [ "//base" ] + [ "//foo:bar" ] +} +)"), + Edited(R"( +executable("foo") { + deps = [] + [ "//foo:bar" ] +} +)")); + + EXPECT_SUCCESS(DoEdit("remove deps //nonexistent", {"//:foo"}, + R"( +executable("foo") { + deps = [ "//base" ] +} +)"), + Edited(R"( +executable("foo") { + deps = [ "//base" ] +} +)", + EditState{{}, + {Err(Location(), + "Target \"//:foo\" does not contain the " + "value \"//nonexistent\" in attribute " + "\"deps\".")}})); +} + TEST_F(EditCommandTest, SetSubcommand) { // New bool attribute EXPECT_SUCCESS(DoEdit("set testonly true",
diff --git a/src/gn/edit_subcommands.cc b/src/gn/edit_subcommands.cc index 31c18f5..26a7dbb 100644 --- a/src/gn/edit_subcommands.cc +++ b/src/gn/edit_subcommands.cc
@@ -65,6 +65,28 @@ }; } +bool RemoveFromTarget(const EditTarget& target, + const std::string& attribute, + const Value& value, + EditState& state) { + bool done = false; + for (auto& assignment : target.assignments(attribute)) { + auto matches = FindListElementInAssignment(target, assignment, value); + + for (const auto& match : matches) { + match.RemoveSelf(state, target); + } + done |= !matches.empty(); + } + + if (!done && target.is_explicit) { + target.add_warning(state, "does not contain the value " + + value.ToString(true) + " in attribute \"" + + attribute + "\"."); + } + return done; +} + EditCommand DeleteCommand() { return EditTargetCommand([](BuildFile& build_file, const EditTarget& target, EditState& state) -> Err { @@ -89,6 +111,19 @@ }); } +EditCommand RemoveFromAttributeCommand(std::string attribute, + std::vector<Value> values) { + return EditTargetCommand( + [attribute = std::move(attribute), values = std::move(values)]( + BuildFile& build_file, const EditTarget& target, + EditState& state) -> Err { + for (const auto& value : values) { + RemoveFromTarget(target, attribute, value, state); + } + return Ok(); + }); +} + // Sets an attribute to a value. EditCommand SetCommand(std::string attribute, Value value) { return EditTargetCommand([=](BuildFile& build_file, const EditTarget& target, @@ -125,11 +160,15 @@ } return DeleteCommand(); } else if (args[0] == "remove") { - if (args.size() != 2) { + if (args.size() < 2) { return Err(Location(), "Invalid remove command.", - "Usage: remove <attribute>"); + "Usage: remove <attribute> [<value(s)>]"); + } else if (args.size() == 2) { + return RemoveAttributeCommand(args[1]); } - return RemoveAttributeCommand(args[1]); + ASSIGN_OR_RETURN(std::vector<Value> values, + ParseValues(base::make_span(args).subspan(2))); + return RemoveFromAttributeCommand(args[1], std::move(values)); } else if (args[0] == "set") { if (args.size() < 3) { return Err(Location(),