gn format: Have format sort sources

Sorts sources blocks as part of gn format. This ended up being more
complicated than I expected because of the subtlety of header comments,
and because no other formatting reorders expressions at the moment.
But, there seems to be quite a lot of unsorted lists per below, so I
guess it's a good thing to have.

Application to tree at https://codereview.chromium.org/960413003.

R=brettw@chromium.org
BUG=348474, 456014

Review URL: https://codereview.chromium.org/962003002

Cr-Commit-Position: refs/heads/master@{#318580}

tools/gn/command_format.cc

@@ -129,6 +129,9 @@ class Printer {
   // Flag assignments to sources, deps, etc. to make their RHSs multiline.
   void AnnotatePreferredMultilineAssignment(const BinaryOpNode* binop);
 
+  // Alphabetically a list on the RHS if the LHS is 'sources'.
+  void SortIfSources(const BinaryOpNode* binop);
+
   // Heuristics to decide if there should be a blank line added between two
   // items. For various "small" items, it doesn't look nice if there's too much
   // vertical whitespace added.
@@ -305,13 +308,29 @@ void Printer::AnnotatePreferredMultilineAssignment(const BinaryOpNode* binop) {
   }
 }
 
+void Printer::SortIfSources(const BinaryOpNode* binop) {
+  const IdentifierNode* ident = binop->left()->AsIdentifier();
+  const ListNode* list = binop->right()->AsList();
+  // TODO(scottmg): Sort more than 'sources'?
+  if ((binop->op().value() == "=" || binop->op().value() == "+=" ||
+       binop->op().value() == "-=") &&
+      ident && list) {
+    const base::StringPiece lhs = ident->value().value();
+    if (lhs == "sources")
+      const_cast<ListNode*>(list)->SortAsStringsList();
+  }
+}
+
+
 bool Printer::ShouldAddBlankLineInBetween(const ParseNode* a,
                                           const ParseNode* b) {
   LocationRange a_range = a->GetRange();
   LocationRange b_range = b->GetRange();
   // If they're already separated by 1 or more lines, then we want to keep a
   // blank line.
-  return b_range.begin().line_number() > a_range.end().line_number() + 1;
+  return (b_range.begin().line_number() > a_range.end().line_number() + 1) ||
+         // Always put a blank line before a block comment.
+         b->AsBlockComment();
 }
 
 int Printer::CurrentColumn() const {
@@ -441,6 +460,8 @@ int Printer::Expr(const ParseNode* root,
     CHECK(precedence_.find(binop->op().value()) != precedence_.end());
     AnnotatePreferredMultilineAssignment(binop);
 
+    SortIfSources(binop);
+
     Precedence prec = precedence_[binop->op().value()];
 
     // Since binary operators format left-to-right, it is ok for the left side
@@ -648,11 +669,9 @@ void Printer::Sequence(SequenceStyle style,
       Expr(x, kPrecedenceLowest, want_comma ? "," : std::string());
       CHECK(!x->comments() || x->comments()->after().empty());
       if (body_of_list) {
-        if (!want_comma) {
-          if (i < list.size() - 1 &&
-              ShouldAddBlankLineInBetween(list[i], list[i + 1]))
-            Newline();
-        }
+        if (i < list.size() - 1 &&
+            ShouldAddBlankLineInBetween(list[i], list[i + 1]))
+          Newline();
       }
       ++i;
     }

tools/gn/command_format_unittest.cc

@@ -99,3 +99,4 @@ FORMAT_TEST(058)
 FORMAT_TEST(059)
 FORMAT_TEST(060)
 FORMAT_TEST(061)
+FORMAT_TEST(062)

tools/gn/format_test_data/031.golden

 deps += [
   ":packed_extra_resources",
   ":packed_resources",
+
   # This shouldn't crash.
 
   # This shouldn't crash 2.

tools/gn/format_test_data/062.gn

+# Sorting, making sure we don't detach comments.
+
+sources = []
+
+sources = ["x.cc"]
+
+sources = [
+  "/a",
+  "/b",
+  "/c",
+  # End of block.
+]
+
+sources += [
+  # Start of block, separate.
+
+  "c",
+  "a",
+  "b",
+]
+
+sources += [
+  "z",
+  "z2",
+  # Attached comment.
+  "y.h",
+  "y.cc",
+  "y.mm",
+  "y.rc",
+  "a"
+]
+
+sources += [
+  "z",
+  "z2",
+
+  # Block comment.
+
+  "y.h",
+  "y.cc",
+  "y.mm",
+  "y.rc",
+  "a"
+]
+
+sources += [
+  "z",
+  "z2",
+
+  #
+  # Multiline block comment.
+  #
+
+  "y.h",
+  "y.cc",
+  "y.mm",
+  "y.rc",
+  "a"
+]
+
+# With identifiers.
+sources += [
+  "a",
+  "b",
+  "c",
+  some_other_thing,
+  abcd,
+]
+
+# With accessors.
+sources += [
+  "a",
+  wee[0],
+  "b",
+  invoker.stuff,
+  "c",
+]
+
+# Various separated blocks.
+sources -= [
+  # Fix this test to build on Windows.
+  "focus_cycler_unittest.cc",
+
+  # All tests for multiple displays: not supported on Windows Ash.
+  "wm/drag_window_resizer_unittest.cc",
+
+  # Accelerometer is only available on Chrome OS.
+  "wm/maximize_mode/maximize_mode_controller_unittest.cc",
+
+  # Can't resize on Windows Ash. http://crbug.com/165962
+  "autoclick/autoclick_unittest.cc",
+  "magnifier/magnification_controller_unittest.cc",
+  # Attached 1.
+  # Attached 2.
+  "wm/workspace/workspace_window_resizer_unittest.cc",
+  "sticky_keys/sticky_keys_overlay_unittest.cc",
+  "system/tray/media_security/multi_profile_media_tray_item_unittest.cc",
+  "virtual_keyboard_controller_unittest.cc",
+
+  # Separated at end.
+  "zzzzzzzzzzzzzz.cc",
+]
+
+sources += [
+  "srtp/crypto/include/xfm.h",
+
+  # sources
+  "srtp/srtp/ekt.c",
+  "srtp/srtp/srtp.c",
+  "srtp/crypto/rng/prng.c",
+  "srtp/crypto/rng/rand_source.c",
+]

tools/gn/format_test_data/062.golden

+# Sorting, making sure we don't detach comments.
+
+sources = []
+
+sources = [
+  "x.cc",
+]
+
+sources = [
+  "/a",
+  "/b",
+  "/c",
+
+  # End of block.
+]
+
+sources += [
+  # Start of block, separate.
+
+  "a",
+  "b",
+  "c",
+]
+
+sources += [
+  "a",
+  "y.cc",
+
+  # Attached comment.
+  "y.h",
+  "y.mm",
+  "y.rc",
+  "z",
+  "z2",
+]
+
+sources += [
+  "z",
+  "z2",
+
+  # Block comment.
+
+  "a",
+  "y.cc",
+  "y.h",
+  "y.mm",
+  "y.rc",
+]
+
+sources += [
+  "z",
+  "z2",
+
+  #
+  # Multiline block comment.
+  #
+
+  "a",
+  "y.cc",
+  "y.h",
+  "y.mm",
+  "y.rc",
+]
+
+# With identifiers.
+sources += [
+  "a",
+  "b",
+  "c",
+  abcd,
+  some_other_thing,
+]
+
+# With accessors.
+sources += [
+  "a",
+  "b",
+  "c",
+  invoker.stuff,
+  wee[0],
+]
+
+# Various separated blocks.
+sources -= [
+  # Fix this test to build on Windows.
+  "focus_cycler_unittest.cc",
+
+  # All tests for multiple displays: not supported on Windows Ash.
+  "wm/drag_window_resizer_unittest.cc",
+
+  # Accelerometer is only available on Chrome OS.
+  "wm/maximize_mode/maximize_mode_controller_unittest.cc",
+
+  # Can't resize on Windows Ash. http://crbug.com/165962
+  "autoclick/autoclick_unittest.cc",
+  "magnifier/magnification_controller_unittest.cc",
+  "sticky_keys/sticky_keys_overlay_unittest.cc",
+  "system/tray/media_security/multi_profile_media_tray_item_unittest.cc",
+  "virtual_keyboard_controller_unittest.cc",
+
+  # Attached 1.
+  # Attached 2.
+  "wm/workspace/workspace_window_resizer_unittest.cc",
+
+  # Separated at end.
+  "zzzzzzzzzzzzzz.cc",
+]
+
+sources += [
+  "srtp/crypto/include/xfm.h",
+
+  # sources
+  "srtp/crypto/rng/prng.c",
+  "srtp/crypto/rng/rand_source.c",
+  "srtp/srtp/ekt.c",
+  "srtp/srtp/srtp.c",
+]

tools/gn/parse_tree.cc

@@ -19,6 +19,27 @@ std::string IndentFor(int value) {
   return std::string(value, ' ');
 }
 
+bool IsSortRangeSeparator(const ParseNode* node, const ParseNode* prev) {
+  // If it's a block comment, or has an attached comment with a blank line
+  // before it, then we break the range at this point.
+  return node->AsBlockComment() != nullptr ||
+         (prev && node->comments() && !node->comments()->before().empty() &&
+          (node->GetRange().begin().line_number() >
+           prev->GetRange().end().line_number() +
+               static_cast<int>(node->comments()->before().size() + 1)));
+}
+
+base::StringPiece GetStringRepresentation(const ParseNode* node) {
+  DCHECK(node->AsLiteral() || node->AsIdentifier() || node->AsAccessor());
+  if (node->AsLiteral())
+    return node->AsLiteral()->value().value();
+  else if (node->AsIdentifier())
+    return node->AsIdentifier()->value().value();
+  else if (node->AsAccessor())
+    return node->AsAccessor()->base().value();
+  return base::StringPiece();
+}
+
 }  // namespace
 
 Comments::Comments() {
@@ -194,6 +215,12 @@ Value AccessorNode::ExecuteScopeAccess(Scope* scope, Err* err) const {
   return *result;
 }
 
+void AccessorNode::SetNewLocation(int line_number) {
+  Location old = base_.location();
+  base_.set_location(
+      Location(old.file(), line_number, old.char_offset(), old.byte()));
+}
+
 // BinaryOpNode ---------------------------------------------------------------
 
 BinaryOpNode::BinaryOpNode() {
@@ -436,6 +463,12 @@ void IdentifierNode::Print(std::ostream& out, int indent) const {
   PrintComments(out, indent);
 }
 
+void IdentifierNode::SetNewLocation(int line_number) {
+  Location old = value_.location();
+  value_.set_location(
+      Location(old.file(), line_number, old.char_offset(), old.byte()));
+}
+
 // ListNode -------------------------------------------------------------------
 
 ListNode::ListNode() : prefer_multiline_(false) {
@@ -490,6 +523,116 @@ void ListNode::Print(std::ostream& out, int indent) const {
     end_->Print(out, indent + 1);
 }
 
+void ListNode::SortAsStringsList() {
+  // Sorts alphabetically. Partitions first on BlockCommentNodes and sorts each
+  // partition separately.
+  for (auto sr : GetSortRanges()) {
+    // Save the original line number so that we can re-assign ranges. We assume
+    // they're contiguous lines because GetSortRanges() does so above. We need
+    // to re-assign these line numbers primiarily because `gn format` uses them
+    // to determine whether two nodes were initially separated by a blank line
+    // or not.
+    int start_line = contents_[sr.begin]->GetRange().begin().line_number();
+    const ParseNode* original_first = contents_[sr.begin];
+    std::sort(contents_.begin() + sr.begin, contents_.begin() + sr.end,
+              [](const ParseNode* a, const ParseNode* b) {
+                base::StringPiece astr = GetStringRepresentation(a);
+                base::StringPiece bstr = GetStringRepresentation(b);
+                return astr < bstr;
+              });
+    // If the beginning of the range had before comments, and the first node
+    // moved during the sort, then move its comments to the new head of the
+    // range.
+    if (original_first->comments() && contents_[sr.begin] != original_first) {
+      for (const auto& hc : original_first->comments()->before()) {
+        const_cast<ParseNode*>(contents_[sr.begin])
+            ->comments_mutable()
+            ->append_before(hc);
+      }
+      const_cast<ParseNode*>(original_first)
+          ->comments_mutable()
+          ->clear_before();
+    }
+    const ParseNode* prev = nullptr;
+    for (size_t i = sr.begin; i != sr.end; ++i) {
+      const ParseNode* node = contents_[i];
+      DCHECK(node->AsLiteral() || node->AsIdentifier() || node->AsAccessor());
+      int line_number =
+          prev ? prev->GetRange().end().line_number() + 1 : start_line;
+      if (node->AsLiteral()) {
+        const_cast<LiteralNode*>(node->AsLiteral())
+            ->SetNewLocation(line_number);
+      } else if (node->AsIdentifier()) {
+        const_cast<IdentifierNode*>(node->AsIdentifier())
+            ->SetNewLocation(line_number);
+      } else if (node->AsAccessor()) {
+        const_cast<AccessorNode*>(node->AsAccessor())
+            ->SetNewLocation(line_number);
+      }
+      prev = node;
+    }
+  }
+}
+
+// Breaks the ParseNodes of |contents| up by ranges that should be separately
+// sorted. In particular, we break at a block comment, or an item that has an
+// attached "before" comment and is separated by a blank line from the item
+// before it. The assumption is that both of these indicate a separate 'section'
+// of a sources block across which items should not be inter-sorted.
+std::vector<ListNode::SortRange> ListNode::GetSortRanges() const {
+  std::vector<SortRange> ranges;
+  const ParseNode* prev = nullptr;
+  size_t begin = 0;
+  for (size_t i = begin; i < contents_.size(); prev = contents_[i++]) {
+    if (IsSortRangeSeparator(contents_[i], prev)) {
+      if (i > begin) {
+        ranges.push_back(SortRange(begin, i));
+        // If |i| is an item with an attached comment, then we start the next
+        // range at that point, because we want to include it in the sort.
+        // Otherwise, it's a block comment which we skip over entirely because
+        // we don't want to move or include it in the sort. The two cases are:
+        //
+        // sources = [
+        //   "a",
+        //   "b",
+        //
+        //   #
+        //   # This is a block comment.
+        //   #
+        //
+        //   "c",
+        //   "d",
+        // ]
+        //
+        // which contains 5 elements, and for which the ranges would be { [0,
+        // 2), [3, 5) } (notably excluding 2, the block comment), and:
+        //
+        // sources = [
+        //   "a",
+        //   "b",
+        //
+        //   # This is a header comment.
+        //   "c",
+        //   "d",
+        // ]
+        //
+        // which contains 4 elements, index 2 containing an attached 'before'
+        // comments, and the ranges should be { [0, 2), [2, 4) }.
+        if (!contents_[i]->AsBlockComment())
+          begin = i;
+        else
+          begin = i + 1;
+      } else {
+        // If it was a one item range, just skip over it.
+        begin = i + 1;
+      }
+    }
+  }
+  if (begin != contents_.size())
+    ranges.push_back(SortRange(begin, contents_.size()));
+  return ranges;
+}
+
 // LiteralNode -----------------------------------------------------------------
 
 LiteralNode::LiteralNode() {
@@ -544,6 +687,12 @@ void LiteralNode::Print(std::ostream& out, int indent) const {
   PrintComments(out, indent);
 }
 
+void LiteralNode::SetNewLocation(int line_number) {
+  Location old = value_.location();
+  value_.set_location(
+      Location(old.file(), line_number, old.char_offset(), old.byte()));
+}
+
 // UnaryOpNode ----------------------------------------------------------------
 
 UnaryOpNode::UnaryOpNode() {
@@ -608,7 +757,6 @@ void BlockCommentNode::Print(std::ostream& out, int indent) const {
   PrintComments(out, indent);
 }
 
-
 // EndNode ---------------------------------------------------------------------
 
 EndNode::EndNode(const Token& token) : value_(token) {

tools/gn/parse_tree.h

@@ -32,23 +32,18 @@ class Comments {
   virtual ~Comments();
 
   const std::vector<Token>& before() const { return before_; }
-  void append_before(Token c) {
-    before_.push_back(c);
-  }
+  void append_before(Token c) { before_.push_back(c); }
+  void clear_before() { before_.clear(); }
 
   const std::vector<Token>& suffix() const { return suffix_; }
-  void append_suffix(Token c) {
-    suffix_.push_back(c);
-  }
+  void append_suffix(Token c) { suffix_.push_back(c); }
   // Reverse the order of the suffix comments. When walking the tree in
   // post-order we append suffix comments in reverse order, so this fixes them
   // up.
   void ReverseSuffix();
 
   const std::vector<Token>& after() const { return after_; }
-  void append_after(Token c) {
-    after_.push_back(c);
-  }
+  void append_after(Token c) { after_.push_back(c); }
 
  private:
   // Whole line comments before the expression.
@@ -160,6 +155,8 @@ class AccessorNode : public ParseNode {
   const IdentifierNode* member() const { return member_.get(); }
   void set_member(scoped_ptr<IdentifierNode> i) { member_ = i.Pass(); }
 
+  void SetNewLocation(int line_number);
+
  private:
   Value ExecuteArrayAccess(Scope* scope, Err* err) const;
   Value ExecuteScopeAccess(Scope* scope, Err* err) const;
@@ -348,6 +345,8 @@ class IdentifierNode : public ParseNode {
   const Token& value() const { return value_; }
   void set_value(const Token& t) { value_ = t; }
 
+  void SetNewLocation(int line_number);
+
  private:
   Token value_;
 
@@ -378,6 +377,8 @@ class ListNode : public ParseNode {
   }
   const std::vector<const ParseNode*>& contents() const { return contents_; }
 
+  void SortAsStringsList();
+
   // During formatting, do we want this list to always be multliline? This is
   // used to make assignments to deps, sources, etc. always be multiline lists,
   // rather than collapsed to a single line when they're one element.
@@ -386,6 +387,14 @@ class ListNode : public ParseNode {
     prefer_multiline_ = prefer_multiline;
   }
 
+  struct SortRange {
+    size_t begin;
+    size_t end;
+    SortRange(size_t begin, size_t end) : begin(begin), end(end) {}
+  };
+  // Only public for testing.
+  std::vector<SortRange> GetSortRanges() const;
+
  private:
   // Tokens corresponding to the [ and ]. The end token is stored in inside an
   // custom parse node so that it can have comments hung off of it.
@@ -418,6 +427,8 @@ class LiteralNode : public ParseNode {
   const Token& value() const { return value_; }
   void set_value(const Token& t) { value_ = t; }
 
+  void SetNewLocation(int line_number);
+
  private:
   Token value_;
 

tools/gn/parse_tree_unittest.cc

@@ -104,3 +104,106 @@ TEST(ParseTree, OriginForDereference) {
   EXPECT_EQ(2, err.location().line_number());
   EXPECT_EQ(20, err.location().char_offset());
 }
+
+TEST(ParseTree, SortRangeExtraction) {
+  TestWithScope setup;
+
+  // Ranges are [begin, end).
+
+  {
+    TestParseInput input(
+        "sources = [\n"
+        "  \"a\",\n"
+        "  \"b\",\n"
+        "  \n"
+        "  #\n"
+        "  # Block\n"
+        "  #\n"
+        "  \n"
+        "  \"c\","
+        "  \"d\","
+        "]\n");
+    EXPECT_FALSE(input.has_error());
+    ASSERT_TRUE(input.parsed()->AsBlock());
+    ASSERT_TRUE(input.parsed()->AsBlock()->statements()[0]->AsBinaryOp());
+    const BinaryOpNode* binop =
+        input.parsed()->AsBlock()->statements()[0]->AsBinaryOp();
+    ASSERT_TRUE(binop->right()->AsList());
+    const ListNode* list = binop->right()->AsList();
+    EXPECT_EQ(5u, list->contents().size());
+    auto ranges = list->GetSortRanges();
+    ASSERT_EQ(2u, ranges.size());
+    EXPECT_EQ(0u, ranges[0].begin);
+    EXPECT_EQ(2u, ranges[0].end);
+    EXPECT_EQ(3u, ranges[1].begin);
+    EXPECT_EQ(5u, ranges[1].end);
+  }
+
+  {
+    TestParseInput input(
+        "sources = [\n"
+        "  \"a\",\n"
+        "  \"b\",\n"
+        "  \n"
+        "  # Attached comment.\n"
+        "  \"c\","
+        "  \"d\","
+        "]\n");
+    EXPECT_FALSE(input.has_error());
+    ASSERT_TRUE(input.parsed()->AsBlock());
+    ASSERT_TRUE(input.parsed()->AsBlock()->statements()[0]->AsBinaryOp());
+    const BinaryOpNode* binop =
+        input.parsed()->AsBlock()->statements()[0]->AsBinaryOp();
+    ASSERT_TRUE(binop->right()->AsList());
+    const ListNode* list = binop->right()->AsList();
+    EXPECT_EQ(4u, list->contents().size());
+    auto ranges = list->GetSortRanges();
+    ASSERT_EQ(2u, ranges.size());
+    EXPECT_EQ(0u, ranges[0].begin);
+    EXPECT_EQ(2u, ranges[0].end);
+    EXPECT_EQ(2u, ranges[1].begin);
+    EXPECT_EQ(4u, ranges[1].end);
+  }
+
+  {
+    TestParseInput input(
+        "sources = [\n"
+        "  # At end of list.\n"
+        "  \"zzzzzzzzzzz.cc\","
+        "]\n");
+    EXPECT_FALSE(input.has_error());
+    ASSERT_TRUE(input.parsed()->AsBlock());
+    ASSERT_TRUE(input.parsed()->AsBlock()->statements()[0]->AsBinaryOp());
+    const BinaryOpNode* binop =
+        input.parsed()->AsBlock()->statements()[0]->AsBinaryOp();
+    ASSERT_TRUE(binop->right()->AsList());
+    const ListNode* list = binop->right()->AsList();
+    EXPECT_EQ(1u, list->contents().size());
+    auto ranges = list->GetSortRanges();
+    ASSERT_EQ(1u, ranges.size());
+    EXPECT_EQ(0u, ranges[0].begin);
+    EXPECT_EQ(1u, ranges[0].end);
+  }
+
+  {
+    TestParseInput input(
+        "sources = [\n"
+        "  # Block at start.\n"
+        "  \n"
+        "  \"z.cc\","
+        "  \"y.cc\","
+        "]\n");
+    EXPECT_FALSE(input.has_error());
+    ASSERT_TRUE(input.parsed()->AsBlock());
+    ASSERT_TRUE(input.parsed()->AsBlock()->statements()[0]->AsBinaryOp());
+    const BinaryOpNode* binop =
+        input.parsed()->AsBlock()->statements()[0]->AsBinaryOp();
+    ASSERT_TRUE(binop->right()->AsList());
+    const ListNode* list = binop->right()->AsList();
+    EXPECT_EQ(3u, list->contents().size());
+    auto ranges = list->GetSortRanges();
+    ASSERT_EQ(1u, ranges.size());
+    EXPECT_EQ(1u, ranges[0].begin);
+    EXPECT_EQ(3u, ranges[0].end);
+  }
+}

tools/gn/token.h

@@ -62,6 +62,7 @@ class Token {
   Type type() const { return type_; }
   const base::StringPiece& value() const { return value_; }
   const Location& location() const { return location_; }
+  void set_location(Location location) { location_ = location; }
   LocationRange range() const {
     return LocationRange(
         location_,