Ensure `gn edit` follows the style guide for variable ordering. Instead of just inserting new variables at the end, we now find the appropriate place to insert them. Change-Id: I612079782ac5861e46f80c87baefb37b6a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26160 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: Matt Stark <msta@google.com>
diff --git a/src/gn/build_file_editor.cc b/src/gn/build_file_editor.cc index c3cecc4..cf63da2 100644 --- a/src/gn/build_file_editor.cc +++ b/src/gn/build_file_editor.cc
@@ -158,6 +158,112 @@ return matched_files; } +// See style_guide.md, "Ordering within a target" +std::optional<int> GetAttributeOrder(std::string_view attribute) { + // 100 => Target metadata + if (attribute == "output_name" || attribute == "output_prefix_override" || + attribute == "output_dir" || attribute == "output_extension") { + return 100; + } else if (attribute == "visibility") { + return 110; + } else if (attribute == "testonly") { + return 120; + } else if (attribute == "friend") { + return 130; + // 200 => inputs / outputs + } else if (attribute == "script") { + return 200; + } else if (attribute == "args") { + return 210; + } else if (attribute == "sources") { + return 220; + } else if (attribute == "public") { + return 230; + } else if (attribute == "inputs") { + return 240; + } else if (attribute == "outputs") { + return 250; + } else if (attribute == "depfile" || attribute == "response_file_contents") { + return 260; + // 300 => config + } else if (attribute == "defines" || attribute == "include_dirs" || + attribute.starts_with("cflags") || attribute == "asmflags" || + attribute == "ldflags" || attribute == "arflags" || + attribute == "data" || attribute == "rustflags" || + attribute.ends_with("configs")) { + return 300; + // 400 => deps + } else if (attribute == "public_deps") { + return 400; + } else if (attribute == "deps") { + return 410; + } else if (attribute == "data_deps") { + return 420; + } else { + return std::nullopt; + } +} + +std::optional<int> GetOrder(const ParseNode* node); + +std::vector<std::optional<int>> GetOrders( + const std::vector<std::unique_ptr<ParseNode>>& nodes) { + std::vector<std::optional<int>> orders; + orders.reserve(nodes.size()); + for (const auto& node : nodes) { + orders.push_back(GetOrder(node.get())); + } + return orders; +} + +std::optional<int> GetOrder(const ParseNode* node) { + if (!node) + return std::nullopt; + + if (const auto* op = node->AsBinaryOp()) { + if (op->op().type() == Token::EQUAL || + op->op().type() == Token::PLUS_EQUALS || + op->op().type() == Token::MINUS_EQUALS) { + if (const auto* id = op->left()->AsIdentifier()) { + return GetAttributeOrder(id->value().value()); + } + } + } else if (const auto* condition = node->AsCondition()) { + std::vector<std::optional<int>> orders; + if (condition->if_true()) { + auto true_orders = GetOrders(condition->if_true()->statements()); + orders.insert(orders.end(), true_orders.begin(), true_orders.end()); + } + if (const auto* if_false = condition->if_false()) { + if (const auto* block = if_false->AsBlock()) { + auto false_orders = GetOrders(block->statements()); + orders.insert(orders.end(), false_orders.begin(), false_orders.end()); + } else if (auto false_order = GetOrder(if_false)) { + orders.push_back(false_order); + } + } + // The style guide says: + // Simple conditions affecting just one variable (e.g. adding a single + // source or adding a flag for one particular OS) can go beneath the + // variable they affect. More complicated conditions affecting more than + // one thing should go at the bottom. + std::optional<int> result = std::nullopt; + for (const auto& order : orders) { + if (order && result && order != result) { + // Affects more than one thing, goes to the bottom + return std::numeric_limits<int>::max(); + } else if (!result) { + result = order; + } + } + if (result) { + // Affects a single variable. + return *result; + } + } + return std::nullopt; +} + } // namespace std::optional<std::string> AsStringLiteral(const ParseNode* node) { @@ -529,8 +635,11 @@ std::unique_ptr<BinaryOpNode> BuildFile::create_assignment( std::string_view name, - std::unique_ptr<ParseNode> value) { - auto left = create_identifier(name); + std::unique_ptr<ParseNode> value, + Location loc) { + StringAtom atom(name); + auto left = std::make_unique<IdentifierNode>( + Token(loc.is_null() ? location() : loc, Token::IDENTIFIER, atom.str())); auto assign = std::make_unique<BinaryOpNode>(); assign->set_op(Token(location(), Token::EQUAL, "=")); @@ -540,6 +649,49 @@ return assign; } +void BuildFile::assign_in_block( + BlockNode* block, + std::vector<std::unique_ptr<ParseNode>>::const_iterator it, + std::string_view name, + std::unique_ptr<ParseNode> value) { + CHECK(block); + Location loc; + // If GN sees: + // a = 1 (line 10) + // b = 2 (line 1 - defaulted) + // c = 3 (line 11) + // The formatter will decide to insert a blank line between b and c because + // there's a gap of more than one line. Thus, we attach it up to an adjacent + // element to prevent blank lines being inserted. + if (block->statements().empty()) { + loc = block->GetRange().begin(); + } else if (it == block->statements().end()) { + loc = block->statements().back()->GetRange().end(); + } else { + loc = (*it)->GetRange().begin(); + } + block->statements().insert(it, + create_assignment(name, std::move(value), loc)); +} + +void BuildFile::assign_in_block(BlockNode* block, + std::string_view name, + std::unique_ptr<ParseNode> value) { + CHECK(block); + auto target_order = GetAttributeOrder(name); + auto it = block->statements().end(); + if (target_order) { + auto orders = GetOrders(block->statements()); + for (size_t i = 0; i < orders.size(); ++i) { + if (orders[i] && *orders[i] >= *target_order) { + it = block->statements().begin() + i; + break; + } + } + } + assign_in_block(block, it, name, std::move(value)); +} + std::unique_ptr<BlockNode> BuildFile::create_block( std::vector<std::unique_ptr<ParseNode>> statements) { auto block = std::make_unique<BlockNode>(BlockNode::DISCARDS_RESULT);
diff --git a/src/gn/build_file_editor.h b/src/gn/build_file_editor.h index dd0ee0b..8872f29 100644 --- a/src/gn/build_file_editor.h +++ b/src/gn/build_file_editor.h
@@ -245,6 +245,21 @@ // Creates a node for `a = b` std::unique_ptr<BinaryOpNode> create_assignment( std::string_view name, + std::unique_ptr<ParseNode> value, + Location loc = Location()); + + // Inserts an assignment `name = value` into `block` at the canonically sorted + // location according to the style guide. + void assign_in_block(BlockNode* block, + std::string_view name, + std::unique_ptr<ParseNode> value); + + // Inserts an assignment `name = value` into `block` at the specified + // iterator. + void assign_in_block( + BlockNode* block, + std::vector<std::unique_ptr<ParseNode>>::const_iterator it, + std::string_view name, std::unique_ptr<ParseNode> value); // Creates a BlockNode `{ ... }` with the given statements.
diff --git a/src/gn/edit_command_unittest.cc b/src/gn/edit_command_unittest.cc index 833af41..7673738 100644 --- a/src/gn/edit_command_unittest.cc +++ b/src/gn/edit_command_unittest.cc
@@ -183,7 +183,6 @@ Edited(R"( executable("foo") { deps = [ "//base" ] - if (is_linux) { deps += [ "//dep" ] } @@ -216,6 +215,40 @@ deps = [ "//base" ] + other_deps } )")); + + EXPECT_SUCCESS(DoEdit("add public_deps //base", + R"( +executable("foo") { + sources = [ "foo.cc" ] + deps = [ "//dep" ] +} +)"), + Edited(R"( +executable("foo") { + sources = [ "foo.cc" ] + public_deps = [ "//base" ] + deps = [ "//dep" ] +} +)")); + + EXPECT_SUCCESS(DoEdit("add deps //base", + R"( +executable("foo") { + sources = [ "foo.cc" ] + if (is_linux) { + deps = [ "//dep" ] + } +} +)"), + Edited(R"( +executable("foo") { + sources = [ "foo.cc" ] + deps = [ "//base" ] + if (is_linux) { + deps += [ "//dep" ] + } +} +)")); } TEST_F(EditCommandTest, DeleteSubcommand) { @@ -631,6 +664,21 @@ } )")); + EXPECT_SUCCESS(DoEdit("set testonly true", + R"( +executable("foo") { + sources = [ "foo.cc" ] + deps = [ "//dep" ] +} +)"), + Edited(R"( +executable("foo") { + testonly = true + sources = [ "foo.cc" ] + deps = [ "//dep" ] +} +)")); + // Replacing existing attribute EXPECT_SUCCESS(DoEdit("set testonly false", R"( @@ -704,21 +752,19 @@ executable("foo") { if (is_linux) { deps = [ "//linux" ] - public_deps = [ "//linux" ] } } )"), Edited( R"( executable("foo") { + deps = [ "//foo" ] if (is_linux) { # TODO(gn edit: set deps:list //foo): This would normally be deleted but is # conditional. Manual intervention is required to decide whether it should # actually be deleted. deps = [ "//linux" ] - public_deps = [ "//linux" ] } - deps = [ "//foo" ] } )", EditState({Label(SourceDir("//"), "foo")})));
diff --git a/src/gn/edit_subcommands.cc b/src/gn/edit_subcommands.cc index dbc6b67..b2b7de4 100644 --- a/src/gn/edit_subcommands.cc +++ b/src/gn/edit_subcommands.cc
@@ -219,8 +219,8 @@ target_list->append_item(build_file.to_node(value)); } } else if (!assignments.empty()) { - // Case B: attr is only defined conditionally -> add attr = [value] at the - // start of the block, change all other assignments to "+=". + // Case B: attr is only defined conditionally -> add attr = [value] right + // before the conditional statement, change all other assignments to "+=". for (auto& assignment : assignments) { if (auto* op = assignment->AsBinaryOpMut()) { if (op->op().type() == Token::EQUAL) { @@ -228,17 +228,25 @@ } } } - target.block->statements().insert( - target.block->statements().begin(), - build_file.create_assignment( - attribute, - build_file.to_node(Value(nullptr, std::vector<Value>(to_add))))); - } else { - // Case C: attr is not defined -> add attr = [value] at the end of the - // block. - target.block->append_statement(build_file.create_assignment( + + auto stack = assignments[0].stack(); + while (stack.size() >= 2 && stack[stack.size() - 2] != target.block) + stack.pop_back(); + + build_file.assign_in_block( + target.block, + std::find_if( + target.block->statements().begin(), + target.block->statements().end(), + [node = stack.back()](const auto& s) { return s.get() == node; }), attribute, - build_file.to_node(Value(nullptr, std::vector<Value>(to_add))))); + build_file.to_node(Value(nullptr, std::vector<Value>(to_add)))); + } else { + // Case C: attr is not defined -> insert attr = [value] at the canonically + // sorted position. + build_file.assign_in_block( + target.block, attribute, + build_file.to_node(Value(nullptr, std::vector<Value>(to_add)))); } } @@ -383,8 +391,7 @@ if (first) { (*first)->AsBinaryOpMut()->set_right(std::move(node)); } else { - target.block->append_statement( - build_file.create_assignment(attribute, std::move(node))); + build_file.assign_in_block(target.block, attribute, std::move(node)); } return Ok();