[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

25397

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

[owenpan]

clang/unittests/Format/FormatTest.cpp

25397

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

766

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

777

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

787

Why not like this?

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

766

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.

777

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

787

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

787

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

private:
   };
 
   void reset();
   void parseFile();
   bool precededByCommentOrPPDirective() const;
   bool parseLevel(bool HasOpeningBrace, bool CanContainBracedList,
                   IfStmtKind *IfKind = nullptr,
                   TokenType NextLBracesType = TT_Unknown);
+  bool mightFitOnOneLine(UnwrappedLine &Line) const;
   IfStmtKind parseBlock(bool MustBeDeclaration = false, unsigned AddLevels = 1u,
                         bool MunchSemi = true,
                         bool UnindentWhitesmithsBraces = false,
                         bool CanContainBracedList = true,
                         TokenType NextLBracesType = TT_Unknown);
   void parseChildBlock(bool CanContainBracedList = true,
                        TokenType NextLBracesType = TT_Unknown);
   void parsePPDirective();

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);
 }
+
 /// \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.)
 bool UnwrappedLineParser::parseLevel(bool HasOpeningBrace,
                                      bool CanContainBracedList,
                                      IfStmtKind *IfKind,
                                      TokenType NextLBracesType) {
   auto NextLevelLBracesType = NextLBracesType == TT_CompoundRequirementLBrace
                                   ? TT_BracedListLBrace
                                   : TT_Unknown;
   const bool IsPrecededByCommentOrPPDirective =
       !Style.RemoveBracesLLVM || precededByCommentOrPPDirective();
   bool HasLabel = false;
   unsigned StatementCount = 0;
   bool SwitchLabelEncountered = false;
   do {
     if (FormatTok->getType() == TT_AttributeMacro) {
       nextToken();
       continue;
     }
     tok::TokenKind kind = FormatTok->Tok.getKind();
     if (FormatTok->getType() == TT_MacroBlockBegin)
       kind = tok::l_brace;
     else if (FormatTok->getType() == TT_MacroBlockEnd)
size_t UnwrappedLineParser::computePPHash() const {
   size_t h = 0;
   for (const auto &i : PPStack) {
     hash_combine(h, size_t(i.Kind));
     hash_combine(h, i.Line);
   }
   return h;
 }
 
+// Checks whether \p ParsedLine might fit on a single line. We must clone the
+// tokens of \p ParsedLine before running the token annotator on it so that we
+// can restore them afterward.
+bool UnwrappedLineParser::mightFitOnOneLine(UnwrappedLine &ParsedLine) const {
+  const auto ColumnLimit = Style.ColumnLimit;
+  if (ColumnLimit == 0)
+    return true;
+
+  auto &Tokens = ParsedLine.Tokens;
+  assert(!Tokens.empty());
+  const auto *LastToken = Tokens.back().Tok;
+  assert(LastToken);
+
+  SmallVector<UnwrappedLineNode> SavedTokens(Tokens.size());
+
+  int Index = 0;
+  for (const auto &Token : Tokens) {
+    assert(Token.Tok);
+    auto &SavedToken = SavedTokens[Index++];
+    SavedToken.Tok = new FormatToken;
+    SavedToken.Tok->copyFrom(*Token.Tok);
+    SavedToken.Children = std::move(Token.Children);
+  }
+
+  AnnotatedLine Line(ParsedLine);
+  assert(Line.Last == LastToken);
+
+  TokenAnnotator Annotator(Style, Keywords);
+  Annotator.annotate(Line);
+  Annotator.calculateFormattingInformation(Line);
+
+  const int Length = LastToken->TotalLength;
+
+  Index = 0;
+  for (auto &Token : Tokens) {
+    const auto &SavedToken = SavedTokens[Index++];
+    Token.Tok->copyFrom(*SavedToken.Tok);
+    Token.Children = std::move(SavedToken.Children);
+    delete SavedToken.Tok;
+  }
+
+  return Line.Level * Style.IndentWidth + Length <= ColumnLimit;
+}
+
 UnwrappedLineParser::IfStmtKind
 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;
if (MacroBlock ? !FormatTok->is(TT_MacroBlockEnd)
     return IfKind;
   }
 
   if (SimpleBlock && Tok->is(tok::l_brace)) {
     assert(FormatTok->is(tok::r_brace));
     const FormatToken *Previous = Tokens->getPreviousToken();
     assert(Previous);
     if (Previous->isNot(tok::r_brace) || Previous->Optional) {
+      assert(!CurrentLines->empty());
+      if (mightFitOnOneLine(CurrentLines->back())) {
         Tok->MatchingParen = FormatTok;
         FormatTok->MatchingParen = Tok;
       }
     }
+  }
 
   size_t PPEndHash = computePPHash();
 
   // Munch the closing brace.
   nextToken(/*LevelDifference=*/-AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();

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("int ab = [](int i) {\n"
+               "  if (i > 0) {\n"
+               "    i = 12345678 -\n"
+               "        i;\n"
+               "  }\n"
+               "  return i;\n"
+               "};",
+               Style);
+
   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);