[clang-format] Discourage breaks in submessage entries, hard rule

[krasimir]

Summary

Currently clang-format allows this for text protos:

submessage:
    { key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' }

when it is under the column limit and when putting it all on one line exceeds the column limit.

This is not a very intuitive formatting, so I'd prefer having

submessage: {
  key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'
}

instead, even if it takes one line more.

This patch prevents clang-format from inserting a break between : { and similar cases.

[krasimir]

• Add tests

[sammccall]

lib/Format/TokenAnnotator.cpp

3104 ↗

(On Diff #150926)

It's really hard to follow the boolean logic here.
Can you break this up into multiple if statements with comments, or extract some named subexpressions or something? e.g.

if (proto or textproto) {
  if (bool isSubmessage = ...)
    return false;
  if (right is string && !break before multiline)
    return false;
  return true;
}

[krasimir]

• Split-up the if-condition

cfe/trunk/lib/Format/TokenAnnotator.cpp

bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
   if (Right.is(TT_ObjCMethodExpr) && !Right.is(tok::r_square) &&
       Left.isNot(TT_SelectorName))
     return true;
 
   if (Right.is(tok::colon) &&
       !Right.isOneOf(TT_CtorInitializerColon, TT_InlineASMColon))
     return false;
   if (Left.is(tok::colon) && Left.isOneOf(TT_DictLiteral, TT_ObjCMethodExpr)) {
-    if ((Style.Language == FormatStyle::LK_Proto ||
-         Style.Language == FormatStyle::LK_TextProto) &&
-        !Style.AlwaysBreakBeforeMultilineStrings && Right.isStringLiteral())
+    if (Style.Language == FormatStyle::LK_Proto ||
+        Style.Language == FormatStyle::LK_TextProto) {
+      if (!Style.AlwaysBreakBeforeMultilineStrings && Right.isStringLiteral())
         return false;
+      // Prevent cases like:
+      //
+      // submessage:
+      //     { key: valueeeeeeeeeeee }
+      //
+      // when the snippet does not fit into one line.
+      // Prefer:
+      //
+      // submessage: {
+      //   key: valueeeeeeeeeeee
+      // }
+      //
+      // instead, even if it is longer by one line.
+      //
+      // Note that this allows allows the "{" to go over the column limit
+      // when the column limit is just between ":" and "{", but that does
+      // not happen too often and alternative formattings in this case are
+      // not much better.
+      //
+      // The code covers the cases:
+      //
+      // submessage: { ... }
+      // submessage: < ... >
+      // repeated: [ ... ]
+      if (((Right.is(tok::l_brace) || Right.is(tok::less)) &&
+           Right.is(TT_DictLiteral)) ||
+          Right.is(TT_ArrayInitializerLSquare))
+        return false;
+    }
     return true;
   }
   if (Right.is(tok::r_square) && Right.MatchingParen &&
       Right.MatchingParen->is(TT_ProtoExtensionLSquare))
     return false;
   if (Right.is(TT_SelectorName) || (Right.is(tok::identifier) && Right.Next &&
                                     Right.Next->is(TT_ObjCMethodExpr)))
     return Left.isNot(tok::period); // FIXME: Properly parse ObjC calls.

cfe/trunk/unittests/Format/FormatTestProto.cpp

verifyFormat("option (MyProto.options) = {\n"
                "    keys: {\n"
                "      sub: [ 1, 2 ]\n"
                "      item: 'aaaaaaaaaaaaaaaa'\n"
                "    }\n"
                "  }\n"
                "}");
 }
 
+TEST_F(FormatTestProto, PreventBreaksBetweenKeyAndSubmessages) {
+  verifyFormat("option (MyProto.options) = {\n"
+               "  submessage: {\n"
+               "    key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  submessage {\n"
+               "    key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'\n"
+               "  }\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  submessage: <\n"
+               "    key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'\n"
+               "  >\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  submessage <\n"
+               "    key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'\n"
+               "  >\n"
+               "}");
+  verifyFormat("option (MyProto.options) = {\n"
+               "  repeatedd: [\n"
+               "    'eyaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'\n"
+               "  ]\n"
+               "}");
+}
+
+
 } // end namespace tooling
 } // end namespace clang

cfe/trunk/unittests/Format/FormatTestTextProto.cpp

TEST_F(FormatTestTextProto, SpacesAroundPercents) {
   verifyFormat("key: 0x%04x");
   verifyFormat("key: \"%d %d\"");
 }
 
 TEST_F(FormatTestTextProto, FormatsRepeatedListInitializers) {
   verifyFormat("keys: []");
   verifyFormat("keys: [ 1 ]");
   verifyFormat("keys: [ 'ala', 'bala' ]");
-  verifyFormat("keys:\n"
-               "    [ 'ala', 'bala', 'porto', 'kala', 'too', 'long', 'ng' ]");
+  verifyFormat("keys: [\n"
+               "  'ala',\n"
+               "  'bala',\n"
+               "  'porto',\n"
+               "  'kala',\n"
+               "  'too',\n"
+               "  'long',\n"
+               "  'ng'\n"
+               "]");
   verifyFormat("key: item\n"
                "keys: [\n"
                "  'ala',\n"
                "  'bala',\n"
                "  'porto',\n"
                "  'kala',\n"
                "  'too',\n"
                "  'long',\n"
verifyFormat("sub: {\n"
                "# comment\n");
   verifyFormat("sub: {\n"
                "  key: 1\n"
                "  # comment\n"
                "  sub: {}\n"
                "}");
 }
 
+TEST_F(FormatTestTextProto, PreventBreaksBetweenKeyAndSubmessages) {
+  verifyFormat("submessage: {\n"
+               "  key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'\n"
+               "}");
+  verifyFormat("submessage {\n"
+               "  key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'\n"
+               "}");
+  verifyFormat("submessage: <\n"
+               "  key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'\n"
+               ">");
+  verifyFormat("submessage <\n"
+               "  key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'\n"
+               ">");
+  verifyFormat("repeatedd: [\n"
+               "  'eyaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'\n"
+               "]");
+  // "{" is going over the column limit.
+  verifyFormat(
+      "submessageeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee: {\n"
+      "  key: 'aaaaa'\n"
+      "}");
+}
+
 } // end namespace tooling
 } // end namespace clang