[clang-format] Indent after breaking Javadoc annotated line

[krasimir]

Summary

This patch makes clang-format indent the subsequent lines created by breaking a
long javadoc annotated line.

[krasimir]

• Update tests

[mprobst]

lib/Format/BreakableToken.cpp

531

Shouldn't this check the set above?

unittests/Format/FormatTestComments.cpp

3109

add a test using @return or @type?

3154

do we already have a test that @param {oh noes this is really long} foo isn't broken?

[mprobst]

unittests/Format/FormatTestComments.cpp

3109

as discussed, make sure to test for LK_JavaScript, because that affects this with comment pragmas (that we should probably disable).

[krasimir]

• Address review comments

[krasimir]

lib/Format/BreakableToken.cpp

531

ahahah, sure!

unittests/Format/FormatTestComments.cpp

3154

As discussed offline, we'll gonna go with breaking that.

[mprobst]

lib/Format/Format.cpp

814 ↗

(On Diff #157681)

@exports isn't really a thing in JS, it is really @export.

But more generally, where do you get stuff like @module from, and what is it supposed to do?

google3 has @pintomodule, @mods and @modName, but I don't think those need special treatment.

I think I'd drop everything except the @see here.

[krasimir]

lib/Format/Format.cpp

814 ↗

(On Diff #157681)

There were existing tests having these.
I can find a reference to @exports and @module here: http://usejsdoc.org/tags-exports.html.
I think that in general if the annotation is supposed to only have the form @tag some-long-path-separated-machine-readable-identifier, that should be left on a single line.
For @mods, I vaguely remember adding that test case while fixing a bug that it shouldn't be broken.
Still, happy to remove these and we'll wait to see if anyone complains.

[mprobst]

lib/Format/Format.cpp

814 ↗

(On Diff #157681)

Please don't change the test code that uses @export, that's a commonly used Closure Compiler annotation.

JSDoc isn't really a consistently applied standard, over time implementations have diverged a lot, so I'm afraid those docs might not apply to Closure Compiler's use of JSDoc. I don't think this hurts here, but I wouldn't expect anybody getting much use out of this either, so it might be cleaner to drop.

[krasimir]

• Address comments

lib/Format/BreakableToken.h

public:
   ///
   /// If \p Break is true, returns the column at which the line should start
   /// after the line break.
   /// If \p Break is false, returns the column at which the line itself will
   /// start.
   virtual unsigned getContentStartColumn(unsigned LineIndex,
                                          bool Break) const = 0;
 
+  /// Returns additional content indent required for the second line after the
+  /// content at line \p LineIndex is broken.
+  ///
+  /// For example, Javadoc @param annotations require and indent of 4 spaces and
+  /// in this example getContentIndex(1) returns 4.
+  /// /**
+  ///  * @param loooooooooooooong line
+  ///  *     continuation
+  ///  */
+  virtual unsigned getContentIndent(unsigned LineIndex) const {
+    return 0;
+  }
+
   /// Returns a range (offset, length) at which to break the line at
   /// \p LineIndex, if previously broken at \p TailOffset. If possible, do not
   /// violate \p ColumnLimit, assuming the text starting at \p TailOffset in
   /// the token is formatted starting at ContentStartColumn in the reformatted
   /// file.
   virtual Split getSplit(unsigned LineIndex, unsigned TailOffset,
                          unsigned ColumnLimit, unsigned ContentStartColumn,
                          llvm::Regex &CommentPragmasRegex) const = 0;
 
   /// Emits the previously retrieved \p Split via \p Whitespaces.
   virtual void insertBreak(unsigned LineIndex, unsigned TailOffset, Split Split,
+                           unsigned ContentIndent,
                            WhitespaceManager &Whitespaces) const = 0;
 
   /// Returns the number of columns needed to format
   /// \p RemainingTokenColumns, assuming that Split is within the range measured
   /// by \p RemainingTokenColumns, and that the whitespace in Split is reduced
   /// to a single space.
   unsigned getLengthAfterCompression(unsigned RemainingTokenColumns,
                                      Split Split) const;
public:
   }
 
   /// Replaces the whitespace from \p SplitAfterLastLine on the last line
   /// after the last line has been formatted by performing a reformatting.
   void replaceWhitespaceAfterLastLine(unsigned TailOffset,
                                       Split SplitAfterLastLine,
                                       WhitespaceManager &Whitespaces) const {
     insertBreak(getLineCount() - 1, TailOffset, SplitAfterLastLine,
-                Whitespaces);
+                /*ContentIndent=*/0, Whitespaces);
   }
 
   /// Updates the next token of \p State to the next token after this
   /// one. This can be used when this token manages a set of underlying tokens
   /// as a unit and is responsible for the formatting of the them.
   virtual void updateNextToken(LineState &State) const {}
 
 protected:
BreakableStringLiteral(const FormatToken &Tok, unsigned StartColumn,
                          StringRef Prefix, StringRef Postfix,
                          unsigned UnbreakableTailLength, bool InPPDirective,
                          encoding::Encoding Encoding, const FormatStyle &Style);
 
   Split getSplit(unsigned LineIndex, unsigned TailOffset, unsigned ColumnLimit,
                  unsigned ContentStartColumn,
                  llvm::Regex &CommentPragmasRegex) const override;
   void insertBreak(unsigned LineIndex, unsigned TailOffset, Split Split,
+                   unsigned ContentIndent,
                    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,
BreakableBlockComment(const FormatToken &Token, unsigned StartColumn,
                         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 getContentStartColumn(unsigned LineIndex, bool Break) const override;
+  unsigned getContentIndent(unsigned LineIndex) const override;
   void insertBreak(unsigned LineIndex, unsigned TailOffset, Split Split,
+                   unsigned ContentIndent,
                    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;
   void adaptStartOfLine(unsigned LineIndex,
                         WhitespaceManager &Whitespaces) const override;
BreakableLineCommentSection(const FormatToken &Token, unsigned StartColumn,
                               bool InPPDirective, encoding::Encoding Encoding,
                               const FormatStyle &Style);
 
   unsigned getRangeLength(unsigned LineIndex, unsigned Offset,
                           StringRef::size_type Length,
                           unsigned StartColumn) const override;
   unsigned getContentStartColumn(unsigned LineIndex, bool Break) const override;
   void insertBreak(unsigned LineIndex, unsigned TailOffset, Split Split,
+                   unsigned ContentIndent,
                    WhitespaceManager &Whitespaces) const override;
   Split getReflowSplit(unsigned LineIndex,
                        llvm::Regex &CommentPragmasRegex) const override;
   void reflow(unsigned LineIndex,
               WhitespaceManager &Whitespaces) const override;
   void adaptStartOfLine(unsigned LineIndex,
                         WhitespaceManager &Whitespaces) const override;
   void updateNextToken(LineState &State) const override;

lib/Format/BreakableToken.cpp

 #include "ContinuationIndenter.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Format/Format.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Debug.h"
 #include <algorithm>
 
 #define DEBUG_TYPE "format-token-breaker"
+#define DBG(A)                                                                 \
+  LLVM_DEBUG({                                                                 \
+    llvm::errs() << __func__ << ":" << __LINE__ << ":" << #A << " = " << A     \
+                 << "\n";                                                      \
+  })
 
 namespace clang {
 namespace format {
 
 static const char *const Blanks = " \t\v\f\r";
 static bool IsBlank(char C) {
   switch (C) {
   case ' ':
BreakableToken::Split BreakableStringLiteral::getSplit(
     unsigned LineIndex, unsigned TailOffset, unsigned ColumnLimit,
     unsigned ContentStartColumn, llvm::Regex &CommentPragmasRegex) const {
   return getStringSplit(Line.substr(TailOffset), ContentStartColumn,
                         ColumnLimit - Postfix.size(), Style.TabWidth, Encoding);
 }
 
 void BreakableStringLiteral::insertBreak(unsigned LineIndex,
                                          unsigned TailOffset, Split Split,
+                                         unsigned ContentIndent,
                                          WhitespaceManager &Whitespaces) const {
   Whitespaces.replaceWhitespaceInToken(
       Tok, Prefix.size() + TailOffset + Split.first, Split.second, Postfix,
       Prefix, InPPDirective, 1, StartColumn);
 }
 
 BreakableComment::BreakableComment(const FormatToken &Token,
                                    unsigned StartColumn, bool InPPDirective,

 
 unsigned BreakableBlockComment::getContentStartColumn(unsigned LineIndex,
                                                       bool Break) const {
   if (Break)
     return IndentAtLineBreak;
   return std::max(0, ContentColumn[LineIndex]);
 }
 
+unsigned BreakableBlockComment::getContentIndent(unsigned LineIndex) const {
+  if (Content[LineIndex].startswith("@param"))
+    return 4;
+  return 0;
+}
+
 void BreakableBlockComment::insertBreak(unsigned LineIndex, unsigned TailOffset,
-                                        Split Split,
+                                        Split Split, unsigned ContentIndent,
                                         WhitespaceManager &Whitespaces) const {
   StringRef Text = Content[LineIndex].substr(TailOffset);
   StringRef Prefix = Decoration;
   // We need this to account for the case when we have a decoration "* " for all
   // the lines except for the last one, where the star in "*/" acts as a
   // decoration.
   unsigned LocalIndentAtLineBreak = IndentAtLineBreak;
   if (LineIndex + 1 == Lines.size() &&
       Text.size() == Split.first + Split.second) {
     // For the last line we need to break before "*/", but not to add "* ".
     Prefix = "";
     if (LocalIndentAtLineBreak >= 2)
       LocalIndentAtLineBreak -= 2;
   }
   // The split offset is from the beginning of the line. Convert it to an offset
   // from the beginning of the token text.
   unsigned BreakOffsetInToken =
       Text.data() - tokenAt(LineIndex).TokenText.data() + Split.first;
   unsigned CharsToRemove = Split.second;
   assert(LocalIndentAtLineBreak >= Prefix.size());
+  std::string PrefixWithTrailingIndent = Prefix;
+  for (unsigned I = 0; I < ContentIndent; ++I)
+    PrefixWithTrailingIndent += " ";
+  DBG(LocalIndentAtLineBreak);
   Whitespaces.replaceWhitespaceInToken(
-      tokenAt(LineIndex), BreakOffsetInToken, CharsToRemove, "", Prefix,
-      InPPDirective, /*Newlines=*/1,
-      /*Spaces=*/LocalIndentAtLineBreak - Prefix.size());
+      tokenAt(LineIndex), BreakOffsetInToken, CharsToRemove, "",
+      PrefixWithTrailingIndent, InPPDirective, /*Newlines=*/1,
+      /*Spaces=*/LocalIndentAtLineBreak + ContentIndent -
+          PrefixWithTrailingIndent.size());
 }
 
 BreakableToken::Split
 BreakableBlockComment::getReflowSplit(unsigned LineIndex,
                                       llvm::Regex &CommentPragmasRegex) const {
   if (!mayReflow(LineIndex, CommentPragmasRegex))
     return Split(StringRef::npos, 0);
   
+  // If we're reflowing into a line with content indent, only reflow the next
+  // line if its starting whitespace matches the content indent.
   size_t Trimmed = Content[LineIndex].find_first_not_of(Blanks);
+  if (LineIndex) {
+    unsigned PreviousContentIndent = getContentIndent(LineIndex - 1);
+    if (PreviousContentIndent && Trimmed != StringRef::npos &&
+        Trimmed != PreviousContentIndent)
+      return Split(StringRef::npos, 0);
+  }
+
   return Split(0, Trimmed != StringRef::npos ? Trimmed : 0);
 }
 
 bool BreakableBlockComment::introducesBreakBeforeToken() const {
   // A break is introduced when we want delimiters on newline.
   return DelimitersOnNewline &&
          Lines[0].substr(1).find_first_not_of(Blanks) != StringRef::npos;
 }

 void BreakableBlockComment::adaptStartOfLine(
     unsigned LineIndex, WhitespaceManager &Whitespaces) const {
   if (LineIndex == 0) {
     if (DelimitersOnNewline) {
       // Since we're breaking at index 1 below, the break position and the
       // break length are the same.
       size_t BreakLength = Lines[0].substr(1).find_first_not_of(Blanks);
       if (BreakLength != StringRef::npos)
-        insertBreak(LineIndex, 0, Split(1, BreakLength), Whitespaces);
+        insertBreak(LineIndex, 0, Split(1, BreakLength), /*ContentIndent=*/0,
+                    Whitespaces);
     }
     return;
   }
   // Here no reflow with the previous line will happen.
   // Fix the decoration of the line at LineIndex.
   StringRef Prefix = Decoration;
   if (Content[LineIndex].empty()) {
     if (LineIndex + 1 == Lines.size()) {
unsigned BreakableLineCommentSection::getContentStartColumn(unsigned LineIndex,
                                                             bool Break) const {
   if (Break)
     return OriginalContentColumn[LineIndex];
   return ContentColumn[LineIndex];
 }
 
 void BreakableLineCommentSection::insertBreak(
     unsigned LineIndex, unsigned TailOffset, Split Split,
-    WhitespaceManager &Whitespaces) const {
+    unsigned ContentIndent, WhitespaceManager &Whitespaces) const {
   StringRef Text = Content[LineIndex].substr(TailOffset);
   // Compute the offset of the split relative to the beginning of the token
   // text.
   unsigned BreakOffsetInToken =
       Text.data() - tokenAt(LineIndex).TokenText.data() + Split.first;
   unsigned CharsToRemove = Split.second;
   // Compute the size of the new indent, including the size of the new prefix of
   // the newly broken line.

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_DEBUG({                                                                 \
+    llvm::errs() << __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) {
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);
   // Adapt the start of the token, for example indent.
   if (!DryRun)
     Token->adaptStartOfLine(0, Whitespaces);
 
+  unsigned ContentIndent = 0;
   unsigned Penalty = 0;
   LLVM_DEBUG(llvm::dbgs() << "Breaking protruding token at column "
                           << StartColumn << ".\n");
   for (unsigned LineIndex = 0, EndIndex = Token->getLineCount();
        LineIndex != EndIndex; ++LineIndex) {
     LLVM_DEBUG(llvm::dbgs()
                << "  Line: " << LineIndex << " (Reflow: " << Reflow << ")\n");
     NewBreakBefore = false;
while (ContentStartColumn + RemainingTokenColumns > ColumnLimit) {
           Penalty += ExcessCharactersPenalty;
           TailOffset += Split.first + Split.second;
           RemainingTokenColumns = Token->getRemainingLength(
               LineIndex, TailOffset, ContentStartColumn);
           continue;
         }
       }
       LLVM_DEBUG(llvm::dbgs() << "    Breaking...\n");
-      ContentStartColumn =
-          Token->getContentStartColumn(LineIndex, /*Break=*/true);
+      // Update the ContentIndent only if the current line was not reflown with
+      // the previous line, since in that case the previous line should still
+      // determine the ContentIndent.
+      if (!Reflow) ContentIndent = Token->getContentIndent(LineIndex);
+      DBG(ContentIndent);
+      ContentStartColumn = ContentIndent + Token->getContentStartColumn(
+                                               LineIndex, /*Break=*/true);
+      DBG(ContentStartColumn);
+
       unsigned NewRemainingTokenColumns = Token->getRemainingLength(
           LineIndex, TailOffset + Split.first + Split.second,
           ContentStartColumn);
 
       // 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;
       }
       assert(NewRemainingTokenColumns < RemainingTokenColumns);
 
       LLVM_DEBUG(llvm::dbgs() << "    Breaking at: " << TailOffset + Split.first
                               << ", " << Split.second << "\n");
       if (!DryRun)
-        Token->insertBreak(LineIndex, TailOffset, Split, Whitespaces);
+        Token->insertBreak(LineIndex, TailOffset, Split, ContentIndent,
+                           Whitespaces);
 
       Penalty += NewBreakPenalty;
       TailOffset += Split.first + Split.second;
       RemainingTokenColumns = NewRemainingTokenColumns;
       BreakInserted = true;
       NewBreakBefore = true;
     }
     // In case there's another line, prepare the state for the start of the next

unittests/Format/FormatTestComments.cpp

"// \\ <log_file> -- --output_directory=\"<output_directory>\"",
   format(
 "// How to run:\n"
 "// bbbbb run \\\n"
 "// rrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr \\\n"
 "// <log_file> -- --output_directory=\"<output_directory>\""));
 // clang-format on
 }
 
+TEST_F(FormatTestComments, IndentsLongJavadocAnnotatedLines) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_Java);
+  Style.ColumnLimit = 60;
+  FormatStyle Style20 = getGoogleStyle(FormatStyle::LK_Java);
+  Style20.ColumnLimit = 20;
+  EXPECT_EQ(
+      "/**\n"
+      " * @param x long long long long long long long long long\n"
+      " *     long\n"
+      " */\n",
+      format("/**\n"
+             " * @param x long long long long long long long long long long\n"
+             " */\n",
+             Style));
+  EXPECT_EQ("/**\n"
+            " * @param x long long long long long long long long long\n"
+            " *     long long long long long long long long long long\n"
+            " */\n",
+            format("/**\n"
+                   " * @param x long long long long long long long long long "
+                   "long long long long long long long long long long\n"
+                   " */\n",
+                   Style));
+  EXPECT_EQ("/**\n"
+            " * @param x long long long long long long long long long\n"
+            " *     long long long long long long long long long long\n"
+            " *     long\n"
+            " */\n",
+            format("/**\n"
+                   " * @param x long long long long long long long long long "
+                   "long long long long long long long long long long long\n"
+                   " */\n",
+                   Style));
+  EXPECT_EQ(
+      "/**\n"
+      " * Sentence that\n"
+      " * should be broken.\n"
+      " * @param short\n"
+      " * keep indentation\n"
+      " */\n", format(
+          "/**\n"
+          " * Sentence that should be broken.\n"
+          " * @param short\n"
+          " * keep indentation\n"
+          " */\n", Style20));
+
+  EXPECT_EQ("/**\n"
+            " * @param l1 long1\n"
+            " *     to break\n"
+            " * @param l2 long2\n"
+            " *     to break\n"
+            " */\n",
+            format("/**\n"
+                   " * @param l1 long1 to break\n"
+                   " * @param l2 long2 to break\n"
+                   " */\n",
+                   Style20));
+
+  EXPECT_EQ("/**\n"
+            " * @param xx to\n"
+            " *     break\n"
+            " * no reflow\n"
+            " */\n",
+            format("/**\n"
+                   " * @param xx to break\n"
+                   " * no reflow\n"
+                   " */\n",
+                   Style20));
+
+  EXPECT_EQ("/**\n"
+            " * @param xx to\n"
+            " *     break yes\n"
+            " *     reflow\n"
+            " */\n",
+            format("/**\n"
+                   " * @param xx to break\n"
+                   " *     yes reflow\n"
+                   " */\n",
+                   Style20));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang

unittests/Format/FormatTestJS.cpp

verifyFormat("/**\n"
                " */",
                "/**\n"
                " * @see http://very/very/long/url/is/long\n"
                " */",
                getGoogleJSStyleWithColumns(20));
   verifyFormat(
       "/**\n"
       " * @param This is a\n"
-      " * long comment but\n"
-      " * no type\n"
+      " *     long comment\n"
+      " *     but no type\n"
       " */",
       "/**\n"
       " * @param This is a long comment but no type\n"
       " */",
       getGoogleJSStyleWithColumns(20));
   // Don't break @param line, but reindent it and reflow unrelated lines.
   verifyFormat("{\n"
                "  /**\n"