gn format: fix comments at end of blocks

Fixes comments at the end of a block like:

  stuff() {
    sources = []
    # wee
  }

This is done by storing an EndNode (instead of just a Token) as the
end of Block and List nodes. This means there's a node in the tree
traversal for the trailing ], and gives a place to attach the comment.

Also collapses some of the expression output code to reuse Sequence.

R=brettw@chromium.org
BUG=348474

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

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

tools/gn/command_format.cc

@@ -50,6 +50,7 @@ class Printer {
     kSequenceStyleFunctionCall,
     kSequenceStyleList,
     kSequenceStyleBlock,
+    kSequenceStyleBracedBlock,
   };
 
   enum ExprStyle {
@@ -78,8 +79,13 @@ class Printer {
   // added to the output.
   ExprStyle Expr(const ParseNode* root);
 
+  // Format a list of values using the given style.
+  // |end| holds any trailing comments to be printed just before the closing
+  // bracket.
   template <class PARSENODE>  // Just for const covariance.
-  void Sequence(SequenceStyle style, const std::vector<PARSENODE*>& list);
+  void Sequence(SequenceStyle style,
+                const std::vector<PARSENODE*>& list,
+                const ParseNode* end);
 
   std::string output_;           // Output buffer.
   std::vector<Token> comments_;  // Pending end-of-line comments.
@@ -210,18 +216,14 @@ Printer::ExprStyle Printer::Expr(const ParseNode* root) {
     Print(" ");
     Expr(binop->right());
   } else if (const BlockNode* block = root->AsBlock()) {
-    Sequence(kSequenceStyleBlock, block->statements());
+    Sequence(kSequenceStyleBracedBlock, block->statements(), block->End());
   } else if (const ConditionNode* condition = root->AsConditionNode()) {
     Print("if (");
     Expr(condition->condition());
-    Print(") {");
-    margin_ += kIndentSize;
-    Newline();
-    Block(condition->if_true());
-    margin_ -= kIndentSize;
-    Trim();
-    PrintMargin();
-    Print("}");
+    Print(") ");
+    Sequence(kSequenceStyleBracedBlock,
+             condition->if_true()->statements(),
+             condition->if_true()->End());
     if (condition->if_false()) {
       Print(" else ");
       // If it's a block it's a bare 'else', otherwise it's an 'else if'. See
@@ -230,31 +232,24 @@ Printer::ExprStyle Printer::Expr(const ParseNode* root) {
       if (is_else_if) {
         Expr(condition->if_false());
       } else {
-        Print("{");
-        margin_ += kIndentSize;
-        Newline();
-        Block(condition->if_false());
-        margin_ -= kIndentSize;
-        Trim();
-        PrintMargin();
-        Print("}");
+        Sequence(kSequenceStyleBracedBlock,
+                 condition->if_false()->AsBlock()->statements(),
+                 condition->if_false()->AsBlock()->End());
       }
     }
   } else if (const FunctionCallNode* func_call = root->AsFunctionCall()) {
     Print(func_call->function().value());
-    Sequence(kSequenceStyleFunctionCall, func_call->args()->contents());
-    Print(" {");
-    margin_ += kIndentSize;
-    Newline();
-    Block(func_call->block());
-    margin_ -= kIndentSize;
-    Trim();
-    PrintMargin();
-    Print("}");
+    Sequence(kSequenceStyleFunctionCall,
+             func_call->args()->contents(),
+             func_call->args()->End());
+    Print(" ");
+    Sequence(kSequenceStyleBracedBlock,
+             func_call->block()->statements(),
+             func_call->block()->End());
   } else if (const IdentifierNode* identifier = root->AsIdentifier()) {
     Print(identifier->value().value());
   } else if (const ListNode* list = root->AsList()) {
-    Sequence(kSequenceStyleList, list->contents());
+    Sequence(kSequenceStyleList, list->contents(), list->End());
   } else if (const LiteralNode* literal = root->AsLiteral()) {
     // TODO(scottmg): Quoting?
     Print(literal->value().value());
@@ -264,6 +259,8 @@ Printer::ExprStyle Printer::Expr(const ParseNode* root) {
   } else if (const BlockCommentNode* block_comment = root->AsBlockComment()) {
     Print(block_comment->comment().value());
     result = kExprStyleComment;
+  } else if (const EndNode* end = root->AsEnd()) {
+    Print(end->value().value());
   } else {
     CHECK(false) << "Unhandled case in Expr.";
   }
@@ -280,14 +277,20 @@ Printer::ExprStyle Printer::Expr(const ParseNode* root) {
 
 template <class PARSENODE>
 void Printer::Sequence(SequenceStyle style,
-                       const std::vector<PARSENODE*>& list) {
+                       const std::vector<PARSENODE*>& list,
+                       const ParseNode* end) {
   bool force_multiline = false;
   if (style == kSequenceStyleFunctionCall)
     Print("(");
   else if (style == kSequenceStyleList)
     Print("[");
+  else if (style == kSequenceStyleBracedBlock)
+    Print("{");
+
+  if (style == kSequenceStyleBlock || style == kSequenceStyleBracedBlock)
+    force_multiline = true;
 
-  if (style == kSequenceStyleBlock)
+  if (end && end->comments() && !end->comments()->before().empty())
     force_multiline = true;
 
   // If there's before line comments, make sure we have a place to put them.
@@ -302,7 +305,7 @@ void Printer::Sequence(SequenceStyle style,
     if (style != kSequenceStyleFunctionCall)
       Print(" ");
     Expr(list[0]);
-    CHECK(list[0]->comments()->after().empty());
+    CHECK(!list[0]->comments() || list[0]->comments()->after().empty());
     if (style != kSequenceStyleFunctionCall)
       Print(" ");
   } else {
@@ -311,16 +314,28 @@ void Printer::Sequence(SequenceStyle style,
     for (const auto& x : list) {
       Newline();
       ExprStyle expr_style = Expr(x);
-      CHECK(x->comments()->after().empty());
+      CHECK(!x->comments() || x->comments()->after().empty());
       if (i < list.size() - 1 || style == kSequenceStyleList) {
-        if (expr_style == kExprStyleRegular)
+        if ((style == kSequenceStyleList || kSequenceStyleFunctionCall) &&
+            expr_style == kExprStyleRegular) {
           Print(",");
-        else
+        } else {
           Newline();
+        }
       }
       ++i;
     }
 
+    // Trailing comments.
+    if (end->comments()) {
+      if (!list.empty())
+        Newline();
+      for (const auto& c : end->comments()->before()) {
+        Newline();
+        TrimAndPrintToken(c);
+      }
+    }
+
     margin_ -= kIndentSize;
     Newline();
   }
@@ -329,6 +344,8 @@ void Printer::Sequence(SequenceStyle style,
     Print(")");
   else if (style == kSequenceStyleList)
     Print("]");
+  else if (style == kSequenceStyleBracedBlock)
+    Print("}");
 }
 
 }  // namespace

tools/gn/command_format_unittest.cc

@@ -24,7 +24,7 @@ bool FormatFileToString(const std::string& input_filename,
                            FILE_PATH_LITERAL(#n)                       \
                                FILE_PATH_LITERAL(".golden")),          \
         &expected));                                                   \
-    EXPECT_EQ(out, expected);                                          \
+    EXPECT_EQ(expected, out);                                          \
   }
 
 // These are expanded out this way rather than a runtime loop so that
@@ -43,3 +43,6 @@ FORMAT_TEST(011)
 FORMAT_TEST(012)
 FORMAT_TEST(013)
 FORMAT_TEST(014)
+FORMAT_TEST(015)
+// TODO(scottmg): Requires precedence/parentheses FORMAT_TEST(016)
+FORMAT_TEST(017)

tools/gn/format_test_data/015.golden

+if (is_win) {
+  sources = [ "a.cc" ]
+
+  # Some comment at end.
+}

tools/gn/format_test_data/017.gn

+executable("win" # Suffix comment on arg.
+           # And a strangely positioned line comment for some reason
+          ) {
+    defines = [ # Defines comment, suffix at end position.
+    ]
+
+    deps = [
+      # Deps comment, should be forced multiline.
+    ]
+    sources = [
+      "a.cc"
+      # End of single sources comment, should be forced multiline.
+    ]
+    # End of block comment.
+}

tools/gn/format_test_data/017.golden

+executable(
+  "win"  # Suffix comment on arg.
+
+  # And a strangely positioned line comment for some reason
+) {
+  defines = []  # Defines comment, suffix at end position.
+
+  deps = [
+    # Deps comment, should be forced multiline.
+  ]
+
+  sources = [
+    "a.cc",
+
+    # End of single sources comment, should be forced multiline.
+  ]
+
+  # End of block comment.
+}

tools/gn/parse_tree.cc

@@ -40,14 +40,15 @@ ParseNode::~ParseNode() {
 
 const AccessorNode* ParseNode::AsAccessor() const { return NULL; }
 const BinaryOpNode* ParseNode::AsBinaryOp() const { return NULL; }
+const BlockCommentNode* ParseNode::AsBlockComment() const { return NULL; }
 const BlockNode* ParseNode::AsBlock() const { return NULL; }
 const ConditionNode* ParseNode::AsConditionNode() const { return NULL; }
+const EndNode* ParseNode::AsEnd() const { return NULL; }
 const FunctionCallNode* ParseNode::AsFunctionCall() const { return NULL; }
 const IdentifierNode* ParseNode::AsIdentifier() const { return NULL; }
 const ListNode* ParseNode::AsList() const { return NULL; }
 const LiteralNode* ParseNode::AsLiteral() const { return NULL; }
 const UnaryOpNode* ParseNode::AsUnaryOp() const { return NULL; }
-const BlockCommentNode* ParseNode::AsBlockComment() const { return NULL; }
 
 Comments* ParseNode::comments_mutable() {
   if (!comments_)
@@ -263,8 +264,8 @@ Value BlockNode::Execute(Scope* containing_scope, Err* err) const {
 
 LocationRange BlockNode::GetRange() const {
   if (begin_token_.type() != Token::INVALID &&
-      end_token_.type() != Token::INVALID) {
-    return begin_token_.range().Union(end_token_.range());
+      end_->value().type() != Token::INVALID) {
+    return begin_token_.range().Union(end_->value().range());
   } else if (!statements_.empty()) {
     return statements_[0]->GetRange().Union(
         statements_[statements_.size() - 1]->GetRange());
@@ -282,6 +283,8 @@ void BlockNode::Print(std::ostream& out, int indent) const {
   PrintComments(out, indent);
   for (size_t i = 0; i < statements_.size(); i++)
     statements_[i]->Print(out, indent + 1);
+  if (end_ && end_->comments())
+    end_->Print(out, indent + 1);
 }
 
 Value BlockNode::ExecuteBlockInScope(Scope* our_scope, Err* err) const {
@@ -478,7 +481,8 @@ Value ListNode::Execute(Scope* scope, Err* err) const {
 }
 
 LocationRange ListNode::GetRange() const {
-  return LocationRange(begin_token_.location(), end_token_.location());
+  return LocationRange(begin_token_.location(),
+                       end_->value().location());
 }
 
 Err ListNode::MakeErrorDescribing(const std::string& msg,
@@ -491,6 +495,8 @@ void ListNode::Print(std::ostream& out, int indent) const {
   PrintComments(out, indent);
   for (size_t i = 0; i < contents_.size(); i++)
     contents_[i]->Print(out, indent + 1);
+  if (end_ && end_->comments())
+    end_->Print(out, indent + 1);
 }
 
 // LiteralNode -----------------------------------------------------------------
@@ -610,3 +616,34 @@ void BlockCommentNode::Print(std::ostream& out, int indent) const {
   out << IndentFor(indent) << "BLOCK_COMMENT(" << comment_.value() << ")\n";
   PrintComments(out, indent);
 }
+
+
+// EndNode ---------------------------------------------------------------------
+
+EndNode::EndNode(const Token& token) : value_(token) {
+}
+
+EndNode::~EndNode() {
+}
+
+const EndNode* EndNode::AsEnd() const {
+  return this;
+}
+
+Value EndNode::Execute(Scope* scope, Err* err) const {
+  return Value();
+}
+
+LocationRange EndNode::GetRange() const {
+  return value_.range();
+}
+
+Err EndNode::MakeErrorDescribing(const std::string& msg,
+                                        const std::string& help) const {
+  return Err(value_, msg, help);
+}
+
+void EndNode::Print(std::ostream& out, int indent) const {
+  out << IndentFor(indent) << "END(" << value_.value() << ")\n";
+  PrintComments(out, indent);
+}

tools/gn/parse_tree.h

@@ -16,15 +16,16 @@
 
 class AccessorNode;
 class BinaryOpNode;
+class BlockCommentNode;
 class BlockNode;
 class ConditionNode;
+class EndNode;
 class FunctionCallNode;
 class IdentifierNode;
 class ListNode;
 class LiteralNode;
 class Scope;
 class UnaryOpNode;
-class BlockCommentNode;
 
 class Comments {
  public:
@@ -74,14 +75,15 @@ class ParseNode {
 
   virtual const AccessorNode* AsAccessor() const;
   virtual const BinaryOpNode* AsBinaryOp() const;
+  virtual const BlockCommentNode* AsBlockComment() const;
   virtual const BlockNode* AsBlock() const;
   virtual const ConditionNode* AsConditionNode() const;
+  virtual const EndNode* AsEnd() const;
   virtual const FunctionCallNode* AsFunctionCall() const;
   virtual const IdentifierNode* AsIdentifier() const;
   virtual const ListNode* AsList() const;
   virtual const LiteralNode* AsLiteral() const;
   virtual const UnaryOpNode* AsUnaryOp() const;
-  virtual const BlockCommentNode* AsBlockComment() const;
 
   virtual Value Execute(Scope* scope, Err* err) const = 0;
 
@@ -226,7 +228,8 @@ class BlockNode : public ParseNode {
   virtual void Print(std::ostream& out, int indent) const OVERRIDE;
 
   void set_begin_token(const Token& t) { begin_token_ = t; }
-  void set_end_token(const Token& t) { end_token_ = t; }
+  void set_end(scoped_ptr<EndNode> e) { end_ = e.Pass(); }
+  const EndNode* End() const { return end_.get(); }
 
   const std::vector<ParseNode*>& statements() const { return statements_; }
   void append_statement(scoped_ptr<ParseNode> s) {
@@ -239,9 +242,10 @@ class BlockNode : public ParseNode {
  private:
   bool has_scope_;
 
-  // Tokens corresponding to { and }, if any (may be NULL).
+  // Tokens corresponding to { and }, if any (may be NULL). The end is stored
+  // in a custom parse node so that it can have comments hung off of it.
   Token begin_token_;
-  Token end_token_;
+  scoped_ptr<EndNode> end_;
 
   // Owning pointers, use unique_ptr when we can use C++11.
   std::vector<ParseNode*> statements_;
@@ -367,7 +371,8 @@ class ListNode : public ParseNode {
   virtual void Print(std::ostream& out, int indent) const OVERRIDE;
 
   void set_begin_token(const Token& t) { begin_token_ = t; }
-  void set_end_token(const Token& t) { end_token_ = t; }
+  void set_end(scoped_ptr<EndNode> e) { end_ = e.Pass(); }
+  const EndNode* End() const { return end_.get(); }
 
   void append_item(scoped_ptr<ParseNode> s) {
     contents_.push_back(s.release());
@@ -375,9 +380,10 @@ class ListNode : public ParseNode {
   const std::vector<const ParseNode*>& contents() const { return contents_; }
 
  private:
-  // Tokens corresponding to the [ and ].
+  // 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.
   Token begin_token_;
-  Token end_token_;
+  scoped_ptr<EndNode> end_;
 
   // Owning pointers, use unique_ptr when we can use C++11.
   std::vector<const ParseNode*> contents_;
@@ -469,4 +475,32 @@ class BlockCommentNode : public ParseNode {
   DISALLOW_COPY_AND_ASSIGN(BlockCommentNode);
 };
 
+// EndNode ---------------------------------------------------------------------
+
+// This node type is used as the end_ object for lists and blocks (rather than
+// just the end ']', '}', or ')' token). This is so that during formatting
+// traversal there is a node that appears at the end of the block to which
+// comments can be attached.
+class EndNode : public ParseNode {
+ public:
+  EndNode(const Token& token);
+  virtual ~EndNode();
+
+  virtual const EndNode* AsEnd() const OVERRIDE;
+  virtual Value Execute(Scope* scope, Err* err) const OVERRIDE;
+  virtual LocationRange GetRange() const OVERRIDE;
+  virtual Err MakeErrorDescribing(
+      const std::string& msg,
+      const std::string& help = std::string()) const OVERRIDE;
+  virtual void Print(std::ostream& out, int indent) const OVERRIDE;
+
+  const Token& value() const { return value_; }
+  void set_value(const Token& t) { value_ = t; }
+
+ private:
+  Token value_;
+
+  DISALLOW_COPY_AND_ASSIGN(EndNode);
+};
+
 #endif  // TOOLS_GN_PARSE_TREE_H_

tools/gn/parser.cc

@@ -285,7 +285,7 @@ scoped_ptr<ParseNode> Parser::IdentifierOrCall(scoped_ptr<ParseNode> left,
                                                Token token) {
   scoped_ptr<ListNode> list(new ListNode);
   list->set_begin_token(token);
-  list->set_end_token(token);
+  list->set_end(make_scoped_ptr(new EndNode(token)));
   scoped_ptr<BlockNode> block;
   bool has_arg = false;
   if (LookAhead(Token::LEFT_PAREN)) {
@@ -418,7 +418,7 @@ scoped_ptr<ListNode> Parser::ParseList(Token start_token,
     *err_ = Err(cur_token(), "Trailing comma");
     return scoped_ptr<ListNode>();
   }
-  list->set_end_token(cur_token());
+  list->set_end(make_scoped_ptr(new EndNode(cur_token())));
   return list.Pass();
 }
 
@@ -479,7 +479,7 @@ scoped_ptr<BlockNode> Parser::ParseBlock() {
 
   for (;;) {
     if (LookAhead(Token::RIGHT_BRACE)) {
-      block->set_end_token(Consume());
+      block->set_end(make_scoped_ptr(new EndNode(Consume())));
       break;
     }
 
@@ -526,6 +526,7 @@ void Parser::TraverseOrder(const ParseNode* root,
           ++i) {
         TraverseOrder(*i, pre, post);
       }
+      TraverseOrder(block->End(), pre, post);
     } else if (const ConditionNode* condition = root->AsConditionNode()) {
       TraverseOrder(condition->condition(), pre, post);
       TraverseOrder(condition->if_true(), pre, post);
@@ -542,12 +543,15 @@ void Parser::TraverseOrder(const ParseNode* root,
           ++i) {
         TraverseOrder(*i, pre, post);
       }
+      TraverseOrder(list->End(), pre, post);
     } else if (root->AsLiteral()) {
       // Nothing.
     } else if (const UnaryOpNode* unaryop = root->AsUnaryOp()) {
       TraverseOrder(unaryop->operand(), pre, post);
     } else if (root->AsBlockComment()) {
       // Nothing.
+    } else if (root->AsEnd()) {
+      // Nothing.
     } else {
       CHECK(false) << "Unhandled case in TraverseOrder.";
     }
@@ -592,7 +596,7 @@ void Parser::AssignComments(ParseNode* file) {
        ++i) {
     // Don't assign suffix comments to the function call or list, but instead
     // to the last thing inside.
-    if ((*i)->AsFunctionCall() || (*i)->AsList())
+    if ((*i)->AsFunctionCall() || (*i)->AsList() || (*i)->AsEnd())
       continue;
 
     const Location& start = (*i)->GetRange().begin();
@@ -620,6 +624,7 @@ void Parser::AssignComments(ParseNode* file) {
 
     // Suffix comments were assigned in reverse, so if there were multiple on
     // the same node, they need to be reversed.
-    const_cast<ParseNode*>(*i)->comments_mutable()->ReverseSuffix();
+    if ((*i)->comments() && !(*i)->comments()->suffix().empty())
+      const_cast<ParseNode*>(*i)->comments_mutable()->ReverseSuffix();
   }
 }

tools/gn/parser_unittest.cc

@@ -633,6 +633,41 @@ TEST(Parser, CommentsConnectedInList) {
   DoParserPrintTest(input, expected);
 }
 
+TEST(Parser, CommentsAtEndOfBlock) {
+  const char* input =
+    "if (is_win) {\n"
+    "  sources = [\"a.cc\"]\n"
+    "  # Some comment at end.\n"
+    "}\n";
+  const char* expected =
+    "BLOCK\n"
+    " CONDITION\n"
+    "  IDENTIFIER(is_win)\n"
+    "  BLOCK\n"
+    "   BINARY(=)\n"
+    "    IDENTIFIER(sources)\n"
+    "    LIST\n"
+    "     LITERAL(\"a.cc\")\n"
+    "   END(})\n"
+    "    +BEFORE_COMMENT(\"# Some comment at end.\")\n";
+  DoParserPrintTest(input, expected);
+}
+
+// TODO(scottmg): I could be convinced this is incorrect. It's not clear to me
+// which thing this comment is intended to be attached to.
+TEST(Parser, CommentsEndOfBlockSingleLine) {
+  const char* input =
+    "defines = [ # EOL defines.\n"
+    "]\n";
+  const char* expected =
+    "BLOCK\n"
+    " BINARY(=)\n"
+    "  IDENTIFIER(defines)\n"
+    "   +SUFFIX_COMMENT(\"# EOL defines.\")\n"
+    "  LIST\n";
+  DoParserPrintTest(input, expected);
+}
+
 TEST(Parser, HangingIf) {
   DoParserErrorTest("if", 1, 1);
 }