[clang-format] Don't remove braces if a 1-statement body would wrap

[owenpan]

Summary

Reimplement the RemoveBracesLLVM feature which handles a single-statement block that would get wrapped.

Fixes https://github.com/llvm/llvm-project/issues/53543.

[Jeroen]

Will it also add braces if they where not there yet?

clang/unittests/Format/FormatTest.cpp

25374

Will it also add braces if they where not there yet?

[owenpan]

clang/unittests/Format/FormatTest.cpp

25374

No, but you can insert braces by setting InsertBraces to true and RemoveBracesLLVM to false.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineParser.cpp

465

A bit explanation what it means that something might fit in one line would be nice.

479

Especially this one is not clear to me, why do we return true here?

481

Is a FormatToken "big", or expensive to copy? If not I'd save them directly, otherwise I'd prefer a unique_ptr.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

465

Will do.

479

We will remove braces only if the previous line (i.e. the line before the closing brace) ends with a semi or comment. I should move this check to the caller or rename the function.

481

We have to make a copy of the FormatToken here so that we can restore it after the TokenAnnotator modifies it directly.

484

We must save Token.Children as well.

[owenpan]

Added saving/restoring the children of UnwrappedLineNode and addressed review comments.

[curdeius]

LGTM with nits.

clang/lib/Format/UnwrappedLineParser.cpp

810

It might be a matter of taste but adding this variable makes the code harder to read to me.

821

So the token's children are modified (moved)? Is it done so that children be not considered by the annotator?

831

Why not like this?

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

810

The last token of ParsedLine is of interest here. We want the annotator to compute its TotalLength to determine whether the line might fit on a single line. I'm open to renaming the variable if you have a better suggestion.

821

Both the constructor and the destructor of AnnotatedLine clear the children of UnwrappedLineNode, so we save them beforehand and restore them afterward.

831

Why not like this?

See the assertion on line 781 above. We are computing the TotalLength of LastToken via Line. Either way works, but I prefer the simpler expression. I can change it though if you insist.

[curdeius]

clang/lib/Format/UnwrappedLineParser.cpp

831

Yeah, what I meant is to just get rid of LastToken variable and write the suggested code.
But well, both ways work. No strong feelings.

clang/lib/Format/UnwrappedLineParser.h

enum class IfStmtKind {
     IfOnly,  // An if statement without the else clause.
     IfElse,  // An if statement followed by else but not else if.
     IfElseIf // An if statement followed by else if.
   };
 
   void reset();
   void parseFile();
   bool precededByCommentOrPPDirective() const;
+  bool mightFitOnOneLine() const;
   bool parseLevel(bool HasOpeningBrace, bool CanContainBracedList,
                   IfStmtKind *IfKind = nullptr,
                   TokenType NextLBracesType = TT_Unknown);
   IfStmtKind parseBlock(bool MustBeDeclaration = false, unsigned AddLevels = 1u,
                         bool MunchSemi = true,
                         bool UnindentWhitesmithsBraces = false,
                         bool CanContainBracedList = true,
                         TokenType NextLBracesType = TT_Unknown);

clang/lib/Format/UnwrappedLineParser.cpp

 //===--- UnwrappedLineParser.cpp - Format C++ code ------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// This file contains the implementation of the UnwrappedLineParser,
 /// which turns a stream of tokens into UnwrappedLines.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "UnwrappedLineParser.h"
 #include "FormatToken.h"
+#include "TokenAnnotator.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include <algorithm>
 #include <utility>
 
 #define DEBUG_TYPE "format-parser"

 bool UnwrappedLineParser::precededByCommentOrPPDirective() const {
   if (!Lines.empty() && Lines.back().InPPDirective)
     return true;
 
   const FormatToken *Previous = Tokens->getPreviousToken();
   return Previous && Previous->is(tok::comment) &&
          (Previous->IsMultiline || Previous->NewlinesBefore > 0);
 }
+
+bool UnwrappedLineParser::mightFitOnOneLine() const {
+  const auto ColumnLimit = Style.ColumnLimit;
+  if (ColumnLimit == 0)
+    return true;
+
+  if (Lines.empty())
+    return true;
+
+  const auto &PreviousLine = Lines.back();
+  const auto &Tokens = PreviousLine.Tokens;
+  assert(!Tokens.empty());
+  const auto *LastToken = Tokens.back().Tok;
+  assert(LastToken);
+  if (!LastToken->isOneOf(tok::semi, tok::comment))
+    return true;
+
+  SmallVector<FormatToken *> SavedTokens;
+  for (const auto &Token : PreviousLine.Tokens) {
+    FormatToken *Tok = new FormatToken;
+    Tok->copyFrom(*Token.Tok);
+    SavedTokens.push_back(Tok);
+  }
+
+  AnnotatedLine Line(PreviousLine);
+  assert(Line.Last == LastToken);
+
+  TokenAnnotator Annotator(Style, Keywords);
+  Annotator.annotate(Line);
+  Annotator.calculateFormattingInformation(Line);
+
+  const int Length = LastToken->TotalLength;
+
+  int I = 0;
+  for (const auto &Token : PreviousLine.Tokens) {
+    const FormatToken *Tok = SavedTokens[I++];
+    Token.Tok->copyFrom(*Tok);
+    delete Tok;
+  }
+
+  return Line.Level * Style.IndentWidth + Length <= ColumnLimit;
+}
+
 /// \brief Parses a level, that is ???.
 /// \param HasOpeningBrace If that level is started by an opening brace.
 /// \param CanContainBracedList If the content can contain (at any level) a
 /// braced list.
 /// \param NextLBracesType The type for left brace found in this level.
 /// \returns true if a simple block, or false otherwise. (A simple block has a
-/// single statement.)
+/// single statement that fits on a single line.)
 bool UnwrappedLineParser::parseLevel(bool HasOpeningBrace,
                                      bool CanContainBracedList,
                                      IfStmtKind *IfKind,
                                      TokenType NextLBracesType) {
   auto NextLevelLBracesType = NextLBracesType == TT_CompoundRequirementLBrace
                                   ? TT_BracedListLBrace
                                   : TT_Unknown;
   const bool IsPrecededByCommentOrPPDirective =
case tok::r_brace:
       if (HasOpeningBrace) {
         if (!Style.RemoveBracesLLVM)
           return false;
         if (FormatTok->isNot(tok::r_brace) || StatementCount != 1 || HasLabel ||
             IsPrecededByCommentOrPPDirective ||
             precededByCommentOrPPDirective())
           return false;
         const FormatToken *Next = Tokens->peekNextToken();
-        return Next->isNot(tok::comment) || Next->NewlinesBefore > 0;
+        if (Next->is(tok::comment) && Next->NewlinesBefore == 0)
+          return false;
+        return mightFitOnOneLine();
       }
       nextToken();
       addUnwrappedLine();
       break;
     case tok::kw_default: {
       unsigned StoredPosition = Tokens->getPosition();
       FormatToken *Next;
       do {
UnwrappedLineParser::parseBlock(bool MustBeDeclaration, unsigned AddLevels,
                                 bool MunchSemi, bool UnindentWhitesmithsBraces,
                                 bool CanContainBracedList,
                                 TokenType NextLBracesType) {
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
          "'{' or macro block token expected");
   FormatToken *Tok = FormatTok;
   const bool MacroBlock = FormatTok->is(TT_MacroBlockBegin);
   FormatTok->setBlockKind(BK_Block);
 
   // For Whitesmiths mode, jump to the next level prior to skipping over the
   // braces.
   if (AddLevels > 0 && Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths)
     ++Line->Level;
 
   size_t PPStartHash = computePPHash();
 
   unsigned InitialLevel = Line->Level;
   nextToken(/*LevelDifference=*/AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();
 
   size_t NbPreprocessorDirectives =
       CurrentLines == &Lines ? PreprocessorDirectives.size() : 0;
   addUnwrappedLine();
   size_t OpeningLineIndex =
       CurrentLines->empty()
           ? (UnwrappedLine::kInvalidIndex)
           : (CurrentLines->size() - 1 - NbPreprocessorDirectives);
 
   // Whitesmiths is weird here. The brace needs to be indented for the namespace
   // block, but the block itself may not be indented depending on the style
   // settings. This allows the format to back up one level in those cases.
   if (UnindentWhitesmithsBraces)
     --Line->Level;
 
   ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
                                           MustBeDeclaration);

clang/unittests/Format/FormatTest.cpp

verifyFormat("if (a) {\n"
                Style);
 
   verifyFormat("if (a) {\n"
                "  f();\n"
                "Label:\n"
                "}",
                Style);
 
-  // FIXME: See https://github.com/llvm/llvm-project/issues/53543.
-#if 0
   Style.ColumnLimit = 65;
 
   verifyFormat("if (condition) {\n"
                "  ff(Indices,\n"
                "     [&](unsigned LHSI, unsigned RHSI) { return true; });\n"
                "} else {\n"
                "  ff(Indices,\n"
                "     [&](unsigned LHSI, unsigned RHSI) { return true; });\n"
                "}",
                Style);
 
   Style.ColumnLimit = 20;
 
   verifyFormat("if (a) {\n"
                "  b = c + // 1 -\n"
                "      d;\n"
                "}",
                Style);
 
   verifyFormat("if (a) {\n"
                "  b = c >= 0 ? d\n"
                "             : e;\n"
                "}",
                "if (a) {\n"
                "  b = c >= 0 ? d : e;\n"
                "}",
                Style);
-#endif
-
-  Style.ColumnLimit = 20;
 
   verifyFormat("if (a)\n"
                "  b = c > 0 ? d : e;",
                "if (a) {\n"
                "  b = c > 0 ? d : e;\n"
                "}",
                Style);