[clang-format] Discourage breaks in submessage entries

[krasimir]

Summary

THIS IS JUST A SKETCH

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 is just a rough sketch of an approach to achieve this: add a penalty for breaking before the {.
Want to discuss the validity of this approach.

[sammccall]

All else equal, I'd expect this to be a hard rule, at least in google-style.

"foo\n { a: b }" --> "foo {\n a: b\n}" only makes the first line longer, by 2 chars. So if 78 < indent + len("foo") <= 80 we're breaking the line limit, but that seems vanishingly rare.

If the penalty is sufficient to stop it happening almost always, maybe it works out the same.

[krasimir]

In D48034#1128475, @sammccall wrote:

All else equal, I'd expect this to be a hard rule, at least in google-style.

"foo\n { a: b }" --> "foo {\n a: b\n}" only makes the first line longer, by 2 chars. So if 78 < indent + len("foo") <= 80 we're breaking the line limit, but that seems vanishingly rare.

If the penalty is sufficient to stop it happening almost always, maybe it works out the same.

If we make this a hard rule, there's a better way: update canBreak to return false.

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)                                                                 \
+  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 (HasTrailingCall && State.Stack.size() > 1 &&
       State.Stack.back().LastSpace = State.Column;
   }
 }
 
 unsigned ContinuationIndenter::addTokenOnNewLine(LineState &State,
                                                  bool DryRun) {
   FormatToken &Current = *State.NextToken;
   const FormatToken &Previous = *State.NextToken->Previous;
+  DBG(Current.TokenText);
 
   // Extra penalty that needs to be added because of the way certain line
   // breaks are chosen.
   unsigned Penalty = 0;
 
   const FormatToken *PreviousNonComment = Current.getPreviousNonComment();
   const FormatToken *NextNonComment = Previous.getNextNonComment();
   if (!NextNonComment)
unsigned ContinuationIndenter::addTokenOnNewLine(LineState &State,
   // short. Also always add the penalty if the LHS is split over multiple lines
   // to avoid unnecessary line breaks that just work around this penalty.
   if (NextNonComment->is(tok::lessless) &&
       State.Stack.back().FirstLessLess == 0 &&
       (State.Column <= Style.ColumnLimit / 3 ||
        State.Stack.back().BreakBeforeParameter))
     Penalty += Style.PenaltyBreakFirstLessLess;
 
+  // Breaking before the "{" in a text proto submessage is not desirable.
+  if (Style.Language == FormatStyle::LK_TextProto) {
+    if (Current.is(tok::l_brace)) {
+      Penalty += Style.PenaltyExcessCharacter;
+    }
+  }
+
   State.Column = getNewLineColumn(State);
 
   // Indent nested blocks relative to this column, unless in a very specific
   // JavaScript special case where:
   //
   //   var loooooong_name =
   //       function() {
   //     // code

unittests/Format/FormatTestTextProto.cpp

verifyFormat("sub: {\n"
                "# comment\n");
   verifyFormat("sub: {\n"
                "  key: 1\n"
                "  # comment\n"
                "  sub: {}\n"
                "}");
 }
 
+TEST_F(FormatTestTextProto, TODO) {
+  verifyFormat("submessage: {\n"
+               "  key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'\n"
+               "}");
+}
+
 } // end namespace tooling
 } // end namespace clang