[clang-format] Ignore UnbreakableTailLength sometimes during breaking

[krasimir]

Summary

This patch fixes an issue where the UnbreakableTailLength would be counted towards
the length of a token during breaking, even though we can break after the token.

For example, this proto text with column limit 20

# ColumnLimit: 20  V
foo: {
  bar: {
    bazoo: "aaaaaaa"
  }
}

was broken:

# ColumnLimit: 20  V
foo: {
  bar: {
    bazoo:
        "aaaaaaa"
  }
}

because the 2 closing } were counted towards the string literal's UnbreakableTailLength.

[krasimir]

• Cleanup

[krasimir]

• Cleanup

[djasper]

I don't understand why the closing braces would count towards the string literals UnbreakableTailLength. Isn't that a bug?

[djasper]

I think I understand now. I think I'd prefer pulling all of the "+ UnbreakableTailLength" calculations out of getRemainingLength so that you don't have to pass around the new parameter CanBreakAfter. Instead, only add it if necessary outside of the function.

lib/Format/BreakableToken.cpp

503

Forgot to check CanBreakAfter here?

[krasimir]

Thanks! I created https://reviews.llvm.org/D42376 which does the same in a much simpler way!

[krasimir]

Superseded by https://reviews.llvm.org/D42376

lib/Format/BreakableToken.h

public:
   ///    at \p LineIndex.
   ///
   /// \p StartColumn is the column at which the text starts in the formatted
   ///    file, needed to compute tab stops correctly.
   ///
   /// For breakable tokens that never use extra space at the end of a line, this
   /// is equivalent to getRangeLength with a Length of StringRef::npos.
   virtual unsigned getRemainingLength(unsigned LineIndex, unsigned Offset,
-                                      unsigned StartColumn) const {
+                                      unsigned StartColumn,
+                                      bool canBreakAfter) const {
     return getRangeLength(LineIndex, Offset, StringRef::npos, StartColumn);
   }
 
   /// \brief Returns the column at which content in line \p LineIndex starts,
   /// assuming no reflow.
   ///
   /// If \p Break is true, returns the column at which the line should start
   /// after the line break.
void insertBreak(unsigned LineIndex, unsigned TailOffset, Split Split,
                    WhitespaceManager &Whitespaces) const override;
   void compressWhitespace(unsigned LineIndex, unsigned TailOffset, Split Split,
                           WhitespaceManager &Whitespaces) const override {}
   unsigned getLineCount() const override;
   unsigned getRangeLength(unsigned LineIndex, unsigned Offset,
                           StringRef::size_type Length,
                           unsigned StartColumn) const override;
   unsigned getRemainingLength(unsigned LineIndex, unsigned Offset,
-                              unsigned StartColumn) const override;
+                              unsigned StartColumn,
+                              bool CanBreakAfter) const override;
   unsigned getContentStartColumn(unsigned LineIndex, bool Break) const override;
 
 protected:
   // The column in which the token starts.
   unsigned StartColumn;
   // The prefix a line needs after a break in the token.
   StringRef Prefix;
   // The postfix a line needs before introducing a break.
BreakableBlockComment(const FormatToken &Token, unsigned StartColumn,
                         unsigned OriginalStartColumn, bool FirstInLine,
                         bool InPPDirective, encoding::Encoding Encoding,
                         const FormatStyle &Style);
 
   unsigned getRangeLength(unsigned LineIndex, unsigned Offset,
                           StringRef::size_type Length,
                           unsigned StartColumn) const override;
   unsigned getRemainingLength(unsigned LineIndex, unsigned Offset,
-                              unsigned StartColumn) const override;
+                              unsigned StartColumn,
+                              bool canBreakAfter) const override;
   unsigned getContentStartColumn(unsigned LineIndex, bool Break) const override;
   void insertBreak(unsigned LineIndex, unsigned TailOffset, Split Split,
                    WhitespaceManager &Whitespaces) const override;
   Split getReflowSplit(unsigned LineIndex,
                        llvm::Regex &CommentPragmasRegex) const override;
   void reflow(unsigned LineIndex,
               WhitespaceManager &Whitespaces) const override;
   bool introducesBreakBeforeToken() const override;

lib/Format/BreakableToken.cpp

 unsigned BreakableStringLiteral::getRangeLength(unsigned LineIndex,
                                                 unsigned Offset,
                                                 StringRef::size_type Length,
                                                 unsigned StartColumn) const {
   llvm_unreachable("Getting the length of a part of the string literal "
                    "indicates that the code tries to reflow it.");
 }
 
-unsigned
-BreakableStringLiteral::getRemainingLength(unsigned LineIndex, unsigned Offset,
-                                           unsigned StartColumn) const {
-  return UnbreakableTailLength + Postfix.size() +
+unsigned BreakableStringLiteral::getRemainingLength(unsigned LineIndex,
+                                                    unsigned Offset,
+                                                    unsigned StartColumn,
+                                                    bool CanBreakAfter) const {
+  return (CanBreakAfter ? 0 : UnbreakableTailLength) + Postfix.size() +
          encoding::columnWidthWithTabs(Line.substr(Offset, StringRef::npos),
                                        StartColumn, Style.TabWidth, Encoding);
 }
 
 unsigned BreakableStringLiteral::getContentStartColumn(unsigned LineIndex,
                                                        bool Break) const {
   return StartColumn + Prefix.size();
 }
if (Content[LineIndex].substr(Offset, StringRef::npos).empty()) {
       LineLength -= Decoration.size();
     }
   }
   return LineLength;
 }
 
 unsigned BreakableBlockComment::getRemainingLength(unsigned LineIndex,
                                                    unsigned Offset,
-                                                   unsigned StartColumn) const {
+                                                   unsigned StartColumn,
+                                                   bool CanBreakAfter) const {
   return UnbreakableTailLength +
          getRangeLength(LineIndex, Offset, StringRef::npos, StartColumn);
 }
 
 unsigned BreakableBlockComment::getContentStartColumn(unsigned LineIndex,
                                                       bool Break) const {
   if (Break)
     return IndentAtLineBreak;
   return std::max(0, ContentColumn[LineIndex]);

lib/Format/ContinuationIndenter.cpp

ContinuationIndenter::breakProtrudingToken(const FormatToken &Current,
   if (!Token)
     return {0, false};
   assert(Token->getLineCount() > 0);
   unsigned ColumnLimit = getColumnLimit(State);
   if (Current.is(TT_LineComment)) {
     // We don't insert backslashes when breaking line comments.
     ColumnLimit = Style.ColumnLimit;
   }
-  if (Current.UnbreakableTailLength >= ColumnLimit)
+  bool CanBreakAfter = State.NextToken && canBreak(State);
+  if (!CanBreakAfter && Current.UnbreakableTailLength >= ColumnLimit)
     return {0, false};
   // ColumnWidth was already accounted into State.Column before calling
   // breakProtrudingToken.
   unsigned StartColumn = State.Column - Current.ColumnWidth;
   unsigned NewBreakPenalty = Current.isStringLiteral()
                                  ? Style.PenaltyBreakString
                                  : Style.PenaltyBreakComment;
   // Stores whether we intentionally decide to let a line exceed the column
ContinuationIndenter::breakProtrudingToken(const FormatToken &Current,
   bool Reflow = false;
   // Keep track of where we are in the token:
   // Where we are in the content of the current logical line.
   unsigned TailOffset = 0;
   // The column number we're currently at.
   unsigned ContentStartColumn =
       Token->getContentStartColumn(0, /*Break=*/false);
   // The number of columns left in the current logical line after TailOffset.
-  unsigned RemainingTokenColumns =
-      Token->getRemainingLength(0, TailOffset, ContentStartColumn);
+  unsigned RemainingTokenColumns = Token->getRemainingLength(
+      0, TailOffset, ContentStartColumn, CanBreakAfter);
   // Adapt the start of the token, for example indent.
   if (!DryRun)
     Token->adaptStartOfLine(0, Whitespaces);
 
   unsigned Penalty = 0;
   DEBUG(llvm::dbgs() << "Breaking protruding token at column " << StartColumn
                      << ".\n");
   for (unsigned LineIndex = 0, EndIndex = Token->getLineCount();
while (ContentStartColumn + RemainingTokenColumns > ColumnLimit) {
 
         BreakableToken::Split NextSplit = Token->getSplit(
             LineIndex, TailOffset + Split.first + Split.second, ColumnLimit,
             ContentStartColumn + ToSplitColumns + 1, CommentPragmasRegex);
         // Compute the columns necessary to fit the next non-breakable sequence
         // into the current line.
         unsigned ToNextSplitColumns = 0;
         if (NextSplit.first == StringRef::npos) {
-          ToNextSplitColumns = Token->getRemainingLength(LineIndex, TailOffset,
-                                                         ContentStartColumn);
+          ToNextSplitColumns = Token->getRemainingLength(
+              LineIndex, TailOffset, ContentStartColumn, CanBreakAfter);
         } else {
           ToNextSplitColumns = Token->getRangeLength(
               LineIndex, TailOffset,
               Split.first + Split.second + NextSplit.first, ContentStartColumn);
         }
         // Compress the whitespace between the break and the start of the next
         // unbreakable sequence.
         ToNextSplitColumns =
while (ContentStartColumn + RemainingTokenColumns > ColumnLimit) {
           if (!DryRun)
             Token->compressWhitespace(LineIndex, TailOffset, Split,
                                       Whitespaces);
           // When we continue on the same line, leave one space between content.
           ContentStartColumn += ToSplitColumns + 1;
           Penalty += ExcessCharactersPenalty;
           TailOffset += Split.first + Split.second;
           RemainingTokenColumns = Token->getRemainingLength(
-              LineIndex, TailOffset, ContentStartColumn);
+              LineIndex, TailOffset, ContentStartColumn, CanBreakAfter);
           continue;
         }
       }
       DEBUG(llvm::dbgs() << "    Breaking...\n");
       ContentStartColumn =
           Token->getContentStartColumn(LineIndex, /*Break=*/true);
       unsigned NewRemainingTokenColumns = Token->getRemainingLength(
           LineIndex, TailOffset + Split.first + Split.second,
-          ContentStartColumn);
+          ContentStartColumn, CanBreakAfter);
 
       // When breaking before a tab character, it may be moved by a few columns,
       // but will still be expanded to the next tab stop, so we don't save any
       // columns.
       if (NewRemainingTokenColumns == RemainingTokenColumns) {
         // FIXME: Do we need to adjust the penalty?
         break;
       }
if (LineIndex + 1 != EndIndex) {
                            << "\n    Potential reflow split: ");
         if (SplitBeforeNext.first != StringRef::npos) {
           DEBUG(llvm::dbgs() << SplitBeforeNext.first << ", "
                              << SplitBeforeNext.second << "\n");
           TailOffset = SplitBeforeNext.first + SplitBeforeNext.second;
           // If the rest of the next line fits into the current line below the
           // column limit, we can safely reflow.
           RemainingTokenColumns = Token->getRemainingLength(
-              NextLineIndex, TailOffset, ContentStartColumn);
+              NextLineIndex, TailOffset, ContentStartColumn, CanBreakAfter);
           Reflow = true;
           if (ContentStartColumn + RemainingTokenColumns > ColumnLimit) {
             DEBUG(llvm::dbgs() << "    Over limit after reflow, need: "
                                << (ContentStartColumn + RemainingTokenColumns)
                                << ", space: " << ColumnLimit
                                << ", reflown prefix: " << ContentStartColumn
                                << ", offset in line: " << TailOffset << "\n");
             // If the whole next line does not fit, try to find a point in
if (LineIndex + 1 != EndIndex) {
       if (!Reflow) {
         // If we didn't reflow into the next line, the only space to consider is
         // the next logical line. Reset our state to match the start of the next
         // line.
         TailOffset = 0;
         ContentStartColumn =
             Token->getContentStartColumn(NextLineIndex, /*Break=*/false);
         RemainingTokenColumns = Token->getRemainingLength(
-            NextLineIndex, TailOffset, ContentStartColumn);
+            NextLineIndex, TailOffset, ContentStartColumn, CanBreakAfter);
         // Adapt the start of the token, for example indent.
         if (!DryRun)
           Token->adaptStartOfLine(NextLineIndex, Whitespaces);
       } else {
         // If we found a reflow split and have added a new break before the next
         // line, we are going to remove the line break at the start of the next
         // logical line. For example, here we'll add a new line break after
         // 'text', and subsequently delete the line break between 'that' and
if (SplitAfterLastLine.first != StringRef::npos) {
     if (!DryRun)
       Token->replaceWhitespaceAfterLastLine(TailOffset, SplitAfterLastLine,
                                             Whitespaces);
     ContentStartColumn =
         Token->getContentStartColumn(Token->getLineCount() - 1, /*Break=*/true);
     RemainingTokenColumns = Token->getRemainingLength(
         Token->getLineCount() - 1,
         TailOffset + SplitAfterLastLine.first + SplitAfterLastLine.second,
-        ContentStartColumn);
+        ContentStartColumn, CanBreakAfter);
   }
 
   State.Column = ContentStartColumn + RemainingTokenColumns -
                  Current.UnbreakableTailLength;
 
   if (BreakInserted) {
     // If we break the token inside a parameter list, we need to break before
     // the next parameter on all levels, so that the next parameter is clearly

unittests/Format/FormatTestRawStrings.cpp

expect_eq(R"test(a = R"proto(key: value)proto";)test",
             format(R"test(a = R"pb(key:value)pb";)test", Style));
 
   // Don't update to canonical delimiter if it occurs as a raw string suffix in
   // the raw string content.
   expect_eq(R"test(a = R"pb(key: ")proto")pb";)test",
             format(R"test(a = R"pb(key:")proto")pb";)test", Style));
 }
 
+TEST_F(FormatTestRawStrings, TODO) {
+  FormatStyle Style = getRawStringPbStyleWithColumns(80);
+  expect_eq("", format(R"test(
+R"pb(
+  foo: {
+    bar: {
+      baz: {
+        quux: {
+          important_text: "asdsadsadsadsadsadasdsaddsadsaasdsadsadsadsadsadasdsaddsadsaa"
+        }
+      }
+    }
+  }
+)pb";)test",
+                       Style));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang

unittests/Format/FormatTestTextProto.cpp

verifyFormat("app_id: 'com.javax.swing.salsa.latino'\n"
                "product_data <product <1>>");
 
   verifyFormat("app <\n"
                "  app_id: 'com.javax.swing.salsa.latino'\n"
                "  headheadheadheadheadhead_id: 1\n"
                "  product_data <product {1}>\n"
                ">");
 }
+
+TEST_F(FormatTestTextProto, DiscardsUnbreakableTailIfCanBreakAfter) {
+  // The two closing braces count towards the string UnbreakableTailLength, but
+  // since we have broken after the corresponding opening braces, we don't
+  // consider that length for string breaking.
+  verifyFormat(
+      "foo: {\n"
+      "  bar: {\n"
+      "    text: \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\"\n"
+      "  }\n"
+      "}");
+}
 } // end namespace tooling
 } // end namespace clang