[clang-format] text protos: put entries on separate lines if there is a submessage

[krasimir]

Summary

This patch updates clang-format text protos to put entries of a submessage into separate lines if the submessage contains at least two entries and contains at least one submessage entry.

For example, the entries here are kept on separate lines even if putting them on a single line would be under the column limit:

message: {
  entry: 1
  submessage: { key: value }
}

Messages containing a single submessage or several scalar entries can still be put on one line if they fit:

message { submessage { key: value } }
message { x: 1 y: 2 z: 3 }

[krasimir]

• Polish

[krasimir]

• Update comments

[krasimir]

• Add comments tests

[sammccall]

LG

lib/Format/TokenAnnotator.cpp

2936

One more high-level piece could be added to this comment, e.g.

// We enforce this by breaking before a submessage field that has previous siblings, *and*
// breaking before a field that follows a submessage field.

2937

nit: the ] is the TT_SelectorName

2938

This could be slightly clearer I think...

... but we don't want to break inside the brackets. We ensure elsewhere that extensions are always on their own line.

2947

nit: this can't be a paren. calling it "brace" is probably ok

2977

nit: preceded

[krasimir]

• Address review comments

lib/Format/ContinuationIndenter.cpp

 #include "FormatInternal.h"
 #include "WhitespaceManager.h"
 #include "clang/Basic/OperatorPrecedence.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Format/Format.h"
 #include "llvm/Support/Debug.h"
 
 #define DEBUG_TYPE "format-indenter"
+#define DBG(A)                                                                 \
+  DEBUG({                                                                      \
+    llvm::dbgs() << __func__ << ":" << __LINE__ << #A << " = " << A << "\n";   \
+  });
 
 namespace clang {
 namespace format {
 
 // Returns true if a TT_SelectorName should be indented when wrapped,
 // false otherwise.
 static bool shouldIndentWrappedSelectorName(const FormatStyle &Style,
                                             LineType LineType) {
if ((startsNextParameter(Current, Style) || Previous.is(tok::semi) ||
         (Previous.NestingLevel == 1 || Style.BinPackParameters)) ||
        (Style.BreakBeforeTernaryOperators && Current.is(TT_ConditionalExpr) &&
         Previous.isNot(tok::question)) ||
        (!Style.BreakBeforeTernaryOperators &&
         Previous.is(TT_ConditionalExpr))) &&
       State.Stack.back().BreakBeforeParameter && !Current.isTrailingComment() &&
       !Current.isOneOf(tok::r_paren, tok::r_brace))
     return true;
+
+  /*
+  if (Style.Language == FormatStyle::LK_TextProto) {
+    if (Current.is(TT_SelectorName) && Current.Next) {
+      FormatToken *LBrace =
+          Current.Next->is(tok::comma) ? Current.Next->Next : Current.Next;
+      if (LBrace && ((LBrace->is(TT_DictLiteral) && LBrace->is(tok::l_brace)) ||
+                     LBrace->is(TT_ArrayInitializerLSquare) ||
+                     opensProtoMessageField(*LBrace, Style))) {
+        DBG(Current.TokenText);
+        if (Previous.ParameterCount != 1)
+          return true;
+      }
+    }
+  }
+  */
   if (((Previous.is(TT_DictLiteral) && Previous.is(tok::l_brace)) ||
        (Previous.is(TT_ArrayInitializerLSquare) &&
         Previous.ParameterCount > 1) ||
        opensProtoMessageField(Previous, Style)) &&
-      Style.ColumnLimit > 0 &&
-      getLengthToMatchingParen(Previous, State.Stack) + State.Column - 1 >
+      Style.ColumnLimit > 0) {
+    if (getLengthToMatchingParen(Previous, State.Stack) + State.Column - 1 >
           getColumnLimit(State))
       return true;
+  }
 
   const FormatToken &BreakConstructorInitializersToken =
       Style.BreakConstructorInitializers == FormatStyle::BCIS_AfterColon
           ? Previous
           : Current;
   if (BreakConstructorInitializersToken.is(TT_CtorInitializerColon) &&
       (State.Column + State.Line->Last->TotalLength - Previous.TotalLength >
            getColumnLimit(State) ||

lib/Format/TokenAnnotator.cpp

 //===----------------------------------------------------------------------===//
 
 #include "TokenAnnotator.h"
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/Support/Debug.h"
 
 #define DEBUG_TYPE "format-token-annotator"
+#define DBG(A)                                                                 \
+  DEBUG({                                                                      \
+    llvm::dbgs() << __func__ << ":" << __LINE__ << #A << " = " << A << "\n";   \
+  });
 
 namespace clang {
 namespace format {
 
 namespace {
 
 /// Returns \c true if the token can be used as an identifier in
 /// an Objective-C \c @selector, \c false otherwise.
if ((Style.Language == FormatStyle::LK_Java ||
       Left.is(TT_LeadingJavaAnnotation) &&
       Right.isNot(TT_LeadingJavaAnnotation) && Right.isNot(tok::l_paren) &&
       (Line.Last->is(tok::l_brace) || Style.BreakAfterJavaFieldAnnotations))
     return true;
 
   if (Right.is(TT_ProtoExtensionLSquare))
     return true;
 
+  // If a submessage A contains a submessage B and has at least 2 entries,
+  // put all of the entries of A on separate lines by forcing the selector of
+  // the submessage to be put on a newline.
+  //
+  // Example: these can stay on one line:
+  // a { scalar_1: 1 scalar_2: 2 }
+  // a { b { key: value } }
+  //
+  // and these need to be broken:
+  // a {
+  //   scalar: 1
+  //   b { key: value }
+  // }
+  //
+  // Be careful to exclude cases like [proto.ext] { ..., since the r_square is
+  // a TT_SelectorName there.
+  if ((Style.Language == FormatStyle::LK_Proto ||
+       Style.Language == FormatStyle::LK_TextProto) &&
+      Right.is(TT_SelectorName) && !Right.is(tok::r_square) && Right.Next) {
+    // Look for the scope opener after selector in cases like:
+    // selector { ...
+    // selector: { ...
+    FormatToken *LParen =
+        Right.Next->is(tok::colon) ? Right.Next->Next : Right.Next;
+    if (LParen &&
+        // The scope opener looks like one of {, [, <:
+        // selector { ... }
+        // selector [ ... ]
+        // selector < ... >
+        //
+        // In case of selector { ... }, the l_brace is TT_DictLiteral.
+        // In case of selector {}, the l_brace is not TT_DictLiteral, so we
+        // check for immediately following r_brace.
+        ((LParen->is(tok::l_brace) &&
+          (LParen->is(TT_DictLiteral) ||
+           (LParen->Next && LParen->Next->is(tok::r_brace)))) ||
+         LParen->is(TT_ArrayInitializerLSquare) || LParen->is(tok::less))) {
+      // If the submessage is the second or later entry, we're done.
+      if (Left.ParameterCount != 1)
+        return true;
+      // However, is the submessage is the first entry in a submessage,
+      // ParameterCount is 1. We deal with this case later by detecting an entry
+      // following a closing paren of this submessage.
+    }
+    // If this is an entry immediately following a submessage, it will be
+    // preceeded by a closing paren of that submessage, like in:
+    //     left---.  .---right
+    //            v  v
+    // sub: { ... } key: value
+    if (Left.isOneOf(tok::r_brace, tok::greater, tok::r_square))
+      return true;
+  }
+
   return false;
 }
 
 bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
                                     const FormatToken &Right) {
   const FormatToken &Left = *Right.Previous;
 
   // Language-specific stuff.
   if (Style.Language == FormatStyle::LK_Java) {
     if (Left.isOneOf(Keywords.kw_throws, Keywords.kw_extends,
                      Keywords.kw_implements))
       return false;

lib/Format/UnwrappedLineFormatter.cpp

 //===--- UnwrappedLineFormatter.cpp - Format C++ code ---------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "NamespaceEndCommentsFixer.h"
 #include "UnwrappedLineFormatter.h"
 #include "WhitespaceManager.h"
 #include "llvm/Support/Debug.h"
 #include <queue>
 
 #define DEBUG_TYPE "format-formatter"
+#define DBG(A)                                                                 \
+  DEBUG({                                                                      \
+    llvm::dbgs() << __func__ << ":" << __LINE__ << #A << " = " << A << "\n";   \
+  });
 
 namespace clang {
 namespace format {
 
 namespace {
 
 bool startsExternCBlock(const AnnotatedLine &Line) {
   const FormatToken *Next = Line.First->getNextNonComment();

unittests/Format/FormatTestProto.cpp

verifyFormat("option (MyProto.options) = {\n"
                "      operator: 2\n"
                "      operator: 3\n"
                "      operator { key: value }\n"
                "    >\n"
                "  >\n"
                "};");
 }
 
+TEST_F(FormatTestProto, BreaksEntriesOfSubmessagesContainingSubmessages) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_TextProto);
+  Style.ColumnLimit = 60;
+  // The column limit allows for the keys submessage to be put on 1 line, but we
+  // break it since it contains a submessage an another entry.
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    item: 'aaaaaaaaaaaaaaaa'\n"
+               "    sub <>\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    item: 'aaaaaaaaaaaaaaaa'\n"
+               "    sub {}\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    sub {}\n"
+               "    sub: <>\n"
+               "    sub: []\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    item: 'aaaaaaaaaaa'\n"
+               "    sub { msg: 1 }\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    item: 'aaaaaaaaaaa'\n"
+               "    sub: { msg: 1 }\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    item: 'aaaaaaaaaaa'\n"
+               "    sub < msg: 1 >\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    item: 'aaaaaaaaaaa'\n"
+               "    sub: [ msg: 1 ]\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: <\n"
+               "    item: 'aaaaaaaaaaa'\n"
+               "    sub: [ 1, 2 ]\n"
+               "  >\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    sub {}\n"
+               "    item: 'aaaaaaaaaaaaaaaa'\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    sub: []\n"
+               "    item: 'aaaaaaaaaaaaaaaa'\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    sub <>\n"
+               "    item: 'aaaaaaaaaaaaaaaa'\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    sub { key: value }\n"
+               "    item: 'aaaaaaaaaaaaaaaa'\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    sub: [ 1, 2 ]\n"
+               "    item: 'aaaaaaaaaaaaaaaa'\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    sub < sub_2: {} >\n"
+               "    item: 'aaaaaaaaaaaaaaaa'\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    item: data\n"
+               "    sub: [ 1, 2 ]\n"
+               "    item: 'aaaaaaaaaaaaaaaa'\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    item: data\n"
+               "    sub < sub_2: {} >\n"
+               "    item: 'aaaaaaaaaaaaaaaa'\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  sub: {\n"
+               "    key: valueeeeeeee\n"
+               "    keys: {\n"
+               "      sub: [ 1, 2 ]\n"
+               "      item: 'aaaaaaaaaaaaaaaa'\n"
+               "    }\n"
+               "  }\n"
+               "}");
+}
+
 } // end namespace tooling
 } // end namespace clang

unittests/Format/FormatTestRawStrings.cpp

expect_eq(
       format(
           R"test(P p = TP(R"pb(  item_1 :  1   )pb");)test",
           getRawStringPbStyleWithColumns(40)));
   expect_eq(
       R"test(P p = TP(R"pb(item_1: 1 item_2: 2)pb");)test",
       format(
           R"test(P p = TP(R"pb(item_1:1 item_2:2)pb");)test",
           getRawStringPbStyleWithColumns(40)));
-  expect_eq(
-      R"test(P p = TP(R"pb(item_1 < 1 > item_2: { 2 })pb");)test",
-      format(
-          R"test(P p = TP(R"pb(item_1<1> item_2:{2})pb");)test",
-          getRawStringPbStyleWithColumns(40)));
-
   // Merge two short lines into one.
   expect_eq(R"test(
 std::string s = R"pb(
   item_1: 1 item_2: 2
 )pb";
 )test",
             format(R"test(
 std::string s = R"pb(
   item_1:1
   item_2:2
 )pb";
 )test",
                    getRawStringPbStyleWithColumns(40)));
 }
 
+TEST_F(FormatTestRawStrings, BreaksShortRawStringsWhenNeeded) {
+  // The raw string contains multiple submessage entries, so break for
+  // readability.
+  expect_eq(R"test(
+P p = TP(R"pb(item_1 < 1 >
+              item_2: { 2 })pb");)test",
+      format(
+          R"test(
+P p = TP(R"pb(item_1<1> item_2:{2})pb");)test",
+          getRawStringPbStyleWithColumns(40)));
+}
+
 TEST_F(FormatTestRawStrings, BreaksRawStringsExceedingColumnLimit) {
   expect_eq(R"test(
 P p = TPPPPPPPPPPPPPPP(
     R"pb(item_1: 1, item_2: 2)pb");)test",
             format(R"test(
 P p = TPPPPPPPPPPPPPPP(R"pb(item_1: 1, item_2: 2)pb");)test",
                    getRawStringPbStyleWithColumns(40)));
 

unittests/Format/FormatTestTextProto.cpp

TEST_F(FormatTestTextProto, SupportsAngleBracketMessageFields) {
   verifyFormat("msg_field <>");
   verifyFormat("msg_field: <>");
   verifyFormat("msg_field < field_a: OK >");
   verifyFormat("msg_field: < field_a: 123 >");
   verifyFormat("msg_field < field_a <> >");
   verifyFormat("msg_field < field_a < field_b <> > >");
   verifyFormat("msg_field: < field_a < field_b: <> > >");
   verifyFormat("msg_field < field_a: OK, field_b: \"OK\" >");
-  verifyFormat("msg_field < field_a: OK field_b: <>, field_c: OK >");
-  verifyFormat("msg_field < field_a { field_b: 1 }, field_c: < f_d: 2 > >");
   verifyFormat("msg_field: < field_a: OK, field_b: \"OK\" >");
-  verifyFormat("msg_field: < field_a: OK field_b: <>, field_c: OK >");
-  verifyFormat("msg_field: < field_a { field_b: 1 }, field_c: < fd_d: 2 > >");
-  verifyFormat("field_a: \"OK\", msg_field: < field_b: 123 >, field_c: {}");
-  verifyFormat("field_a < field_b: 1 >, msg_fid: < fiel_b: 123 >, field_c <>");
-  verifyFormat("field_a < field_b: 1 > msg_fied: < field_b: 123 > field_c <>");
-  verifyFormat("field < field < field: <> >, field <> > field: < field: 1 >");
-
   // Multiple lines tests
   verifyFormat("msg_field <\n"
                "  field_a: OK\n"
+               "  field_b: <>,\n"
+               "  field_c: OK\n"
+               ">");
+
+  verifyFormat("msg_field <\n"
+               "  field_a { field_b: 1 },\n"
+               "  field_c: < f_d: 2 >\n"
+               ">");
+
+  verifyFormat("msg_field: <\n"
+               "  field_a: OK\n"
+               "  field_b: <>,\n"
+               "  field_c: OK\n"
+               ">");
+
+  verifyFormat("msg_field: <\n"
+               "  field_a { field_b: 1 },\n"
+               "  field_c: < fd_d: 2 >\n"
+               ">");
+
+  verifyFormat("field_a: \"OK\",\n"
+               "msg_field: < field_b: 123 >,\n"
+               "field_c: {}");
+
+  verifyFormat("field_a < field_b: 1 >,\n"
+               "msg_fid: < fiel_b: 123 >,\n" 
+               "field_c <>");
+
+  verifyFormat("field_a < field_b: 1 >\n"
+               "msg_fied: < field_b: 123 >\n"
+               "field_c <>");
+
+  verifyFormat("field <\n"
+               "  field < field: <> >,\n"
+               "  field <>\n"
+               ">\n"
+               "field: < field: 1 >");
+
+  verifyFormat("msg_field <\n"
+               "  field_a: OK\n"
                "  field_b: \"OK\"\n"
                "  field_c: 1\n"
                "  field_d: 12.5\n"
                "  field_e: OK\n"
                ">");
 
   verifyFormat("msg_field: <>\n"
                "field_c: \"OK\",\n"
TEST_F(FormatTestTextProto, SupportsAngleBracketMessageFields) {
 
   verifyFormat("field_a: {\n"
                "  field_d: ok\n"
                "  field_b: < field_c: 1 >\n"
                "  field_d: ok\n"
                "  field_d: ok\n"
                "}");
 
-  verifyFormat("field_a: < f1: 1, f2: <> >\n"
+  verifyFormat("field_a: <\n"
+               "  f1: 1,\n"
+               "  f2: <>\n"
+               ">\n"
                "field_b <\n"
                "  field_b1: <>\n"
                "  field_b2: ok,\n"
                "  field_b3: <\n"
                "    field_x {}  // Comment\n"
                "    field_y: { field_z: 1 }\n"
                "    field_w: ok\n"
                "  >\n"
TEST_F(FormatTestTextProto, BreaksAfterBraceFollowedByClosingBraceOnNextLine) {
   verifyFormat("keys: [\n"
                "  data: { item: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' }\n"
                "]");
   verifyFormat("keys: <\n"
                "  data: { item: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' }\n"
                ">");
 }
 
+TEST_F(FormatTestTextProto, BreaksEntriesOfSubmessagesContainingSubmessages) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_TextProto);
+  Style.ColumnLimit = 60;
+  // The column limit allows for the keys submessage to be put on 1 line, but we
+  // break it since it contains a submessage an another entry.
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: {\n"
+               "  item: 'aaaaaaaaaaaaaaaa'\n"
+               "  sub <>\n"
+               "}");
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: {\n"
+               "  item: 'aaaaaaaaaaaaaaaa'\n"
+               "  sub {}\n"
+               "}");
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: {\n"
+               "  sub {}\n"
+               "  sub: <>\n"
+               "  sub: []\n"
+               "}");
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: {\n"
+               "  item: 'aaaaaaaaaaa'\n"
+               "  sub { msg: 1 }\n"
+               "}");
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: {\n"
+               "  item: 'aaaaaaaaaaa'\n"
+               "  sub: { msg: 1 }\n"
+               "}");
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: {\n"
+               "  item: 'aaaaaaaaaaa'\n"
+               "  sub < msg: 1 >\n"
+               "}");
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: {\n"
+               "  item: 'aaaaaaaaaaa'\n"
+               "  sub: [ msg: 1 ]\n"
+               "}");
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: <\n"
+               "  item: 'aaaaaaaaaaa'\n"
+               "  sub: [ 1, 2 ]\n"
+               ">");
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: {\n"
+               "  sub {}\n"
+               "  item: 'aaaaaaaaaaaaaaaa'\n"
+               "}");
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: {\n"
+               "  sub: []\n"
+               "  item: 'aaaaaaaaaaaaaaaa'\n"
+               "}");
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: {\n"
+               "  sub <>\n"
+               "  item: 'aaaaaaaaaaaaaaaa'\n"
+               "}");
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: {\n"
+               "  sub { key: value }\n"
+               "  item: 'aaaaaaaaaaaaaaaa'\n"
+               "}");
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: {\n"
+               "  sub: [ 1, 2 ]\n"
+               "  item: 'aaaaaaaaaaaaaaaa'\n"
+               "}");
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: {\n"
+               "  sub < sub_2: {} >\n"
+               "  item: 'aaaaaaaaaaaaaaaa'\n"
+               "}");
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: {\n"
+               "  item: data\n"
+               "  sub: [ 1, 2 ]\n"
+               "  item: 'aaaaaaaaaaaaaaaa'\n"
+               "}");
+  verifyFormat("key: valueeeeeeee\n"
+               "keys: {\n"
+               "  item: data\n"
+               "  sub < sub_2: {} >\n"
+               "  item: 'aaaaaaaaaaaaaaaa'\n"
+               "}");
+  verifyFormat("sub: {\n"
+               "  key: valueeeeeeee\n"
+               "  keys: {\n"
+               "    sub: [ 1, 2 ]\n"
+               "    item: 'aaaaaaaaaaaaaaaa'\n"
+               "  }\n"
+               "}");
+}
+
 } // end namespace tooling
 } // end namespace clang