[clang-format] Remove braces of else blocks that embody an if block

[owenpan]

Summary

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

[curdeius]

clang/unittests/Format/FormatTest.cpp

25570–25575

Why isn't this block changed like this?
I might be missing something...

[owenpan]

clang/unittests/Format/FormatTest.cpp

25570–25575

It would cause a dangling else error because the last else would be paired with the inner merged else if.

[curdeius]

LGTM. Thanks!

clang/unittests/Format/FormatTest.cpp

25570–25575

True. There are indeed no braces in the outer if.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

2633

To be precise. It doesn't matter now but will when we configure removing braces for other styles.

[owenpan]

Update IfLeftBrace in parseLevel() only if the level is a simple block.

[curdeius]

Still looks good. Was there a particular case where the previous version didn't work?

[owenpan]

In D127260#3567476, @curdeius wrote:

Still looks good. Was there a particular case where the previous version didn't work?

The assertion I added on line 2626, which would ensure that the RemoveBraces lambda on line 898 didn't miss any potentially removable braces, would fail because parseLevel() was indiscriminately passing the l_brace of the last if in the level to its caller. This new version is also a tiny bit more efficient because IfLBrace on line 2624 will be null if the else block that encloses the if block is not a simple block.

Because neither version would remove non-optional braces, I decided not to add a test case just for the assertion, e.g.,

if (a)
  b;
else {
  c;
  if (d) {
    e;
    f;
  }
}

clang/lib/Format/UnwrappedLineParser.h

private:
   };
 
   void reset();
   void parseFile();
   bool precededByCommentOrPPDirective() const;
   bool parseLevel(const FormatToken *OpeningBrace = nullptr,
                   bool CanContainBracedList = true,
                   TokenType NextLBracesType = TT_Unknown,
-                  IfStmtKind *IfKind = nullptr);
+                  IfStmtKind *IfKind = nullptr,
+                  FormatToken **IfLeftBrace = nullptr);
   bool mightFitOnOneLine(UnwrappedLine &Line,
                          const FormatToken *OpeningBrace = nullptr) const;
-  void parseBlock(bool MustBeDeclaration = false, unsigned AddLevels = 1u,
-                  bool MunchSemi = true, bool KeepBraces = true,
-                  IfStmtKind *IfKind = nullptr,
+  FormatToken *parseBlock(bool MustBeDeclaration = false,
+                          unsigned AddLevels = 1u, bool MunchSemi = true,
+                          bool KeepBraces = true, IfStmtKind *IfKind = nullptr,
                           bool UnindentWhitesmithsBraces = false,
                           bool CanContainBracedList = true,
                           TokenType NextLBracesType = TT_Unknown);
   void parseChildBlock(bool CanContainBracedList = true,
                        TokenType NextLBracesType = TT_Unknown);
   void parsePPDirective();
   void parsePPDefine();
   void parsePPIf(bool IfDef);
   void parsePPElIf();
   void parsePPElse();
   void parsePPEndIf();
   void parsePPUnknown();
   void readTokenWithJavaScriptASI();
   void parseStructuralElement(bool IsTopLevel = false,
                               TokenType NextLBracesType = TT_Unknown,
                               IfStmtKind *IfKind = nullptr,
+                              FormatToken **IfLeftBrace = nullptr,
                               bool *HasDoWhile = nullptr,
                               bool *HasLabel = nullptr);
   bool tryToParseBracedList();
   bool parseBracedList(bool ContinueOnSemicolons = false, bool IsEnum = false,
                        tok::TokenKind ClosingBraceKind = tok::r_brace);
   void parseParens(TokenType AmpAmpTokenType = TT_Unknown);
   void parseSquare(bool LambdaIntroducer = false);
   void keepAncestorBraces();

clang/lib/Format/UnwrappedLineParser.cpp

return Previous && Previous->is(tok::comment) &&
          (Previous->IsMultiline || Previous->NewlinesBefore > 0);
 }
 
 /// \brief Parses a level, that is ???.
 /// \param OpeningBrace Opening brace (\p nullptr if absent) of that level
 /// \param CanContainBracedList If the content can contain (at any level) a
 /// braced list.
 /// \param NextLBracesType The type for left brace found in this level.
-/// \param IfKind The if statement kind in the level.
+/// \param IfKind The \p if statement kind in the level.
+/// \param IfLeftBrace The left brace of the \p if block in the level.
 /// \returns true if a simple block of if/else/for/while, or false otherwise.
 /// (A simple block has a single statement.)
 bool UnwrappedLineParser::parseLevel(const FormatToken *OpeningBrace,
                                      bool CanContainBracedList,
                                      TokenType NextLBracesType,
-                                     IfStmtKind *IfKind) {
+                                     IfStmtKind *IfKind,
+                                     FormatToken **IfLeftBrace) {
   auto NextLevelLBracesType = NextLBracesType == TT_CompoundRequirementLBrace
                                   ? TT_BracedListLBrace
                                   : TT_Unknown;
   const bool IsPrecededByCommentOrPPDirective =
       !Style.RemoveBracesLLVM || precededByCommentOrPPDirective();
+  FormatToken *IfLBrace = nullptr;
   bool HasDoWhile = false;
   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)
       kind = tok::r_brace;
 
     auto ParseDefault = [this, OpeningBrace, NextLevelLBracesType, IfKind,
-                         &HasDoWhile, &HasLabel, &StatementCount] {
+                         &IfLBrace, &HasDoWhile, &HasLabel, &StatementCount] {
       parseStructuralElement(!OpeningBrace, NextLevelLBracesType, IfKind,
-                             HasDoWhile ? nullptr : &HasDoWhile,
+                             &IfLBrace, HasDoWhile ? nullptr : &HasDoWhile,
                              HasLabel ? nullptr : &HasLabel);
       ++StatementCount;
       assert(StatementCount > 0 && "StatementCount overflow!");
     };
 
     switch (kind) {
     case tok::comment:
       nextToken();
case tok::r_brace:
           return false;
         }
         if (FormatTok->isNot(tok::r_brace) || StatementCount != 1 || HasLabel ||
             HasDoWhile || 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;
+        if (IfLeftBrace)
+          *IfLeftBrace = IfLBrace;
+        return true;
       }
       nextToken();
       addUnwrappedLine();
       break;
     case tok::kw_default: {
       unsigned StoredPosition = Tokens->getPosition();
       FormatToken *Next;
       do {
for (auto &Token : Tokens) {
     Token.Tok->copyFrom(*SavedToken.Tok);
     Token.Children = std::move(SavedToken.Children);
     delete SavedToken.Tok;
   }
 
   return Line.Level * Style.IndentWidth + Length <= ColumnLimit;
 }
 
-void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, unsigned AddLevels,
-                                     bool MunchSemi, bool KeepBraces,
-                                     IfStmtKind *IfKind,
-                                     bool UnindentWhitesmithsBraces,
-                                     bool CanContainBracedList,
-                                     TokenType NextLBracesType) {
+FormatToken *UnwrappedLineParser::parseBlock(
+    bool MustBeDeclaration, unsigned AddLevels, bool MunchSemi, bool KeepBraces,
+    IfStmtKind *IfKind, bool UnindentWhitesmithsBraces,
+    bool CanContainBracedList, TokenType NextLBracesType) {
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
          "'{' or macro block token expected");
   FormatToken *Tok = FormatTok;
   const bool FollowedByComment = Tokens->peekNextToken()->is(tok::comment);
   auto Index = CurrentLines->size();
   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);
 
   // Bail out if there are too many levels. Otherwise, the stack might overflow.
   if (Line->Level > 300)
-    return;
+    return nullptr;
 
   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);
   if (AddLevels > 0u && Style.BreakBeforeBraces != FormatStyle::BS_Whitesmiths)
     Line->Level += AddLevels;
 
+  FormatToken *IfLBrace = nullptr;
   const bool SimpleBlock =
-      parseLevel(Tok, CanContainBracedList, NextLBracesType, IfKind);
+      parseLevel(Tok, CanContainBracedList, NextLBracesType, IfKind, &IfLBrace);
 
   if (eof())
-    return;
+    return IfLBrace;
 
   if (MacroBlock ? !FormatTok->is(TT_MacroBlockEnd)
                  : !FormatTok->is(tok::r_brace)) {
     Line->Level = InitialLevel;
     FormatTok->setBlockKind(BK_Block);
-    return;
+    return IfLBrace;
   }
 
   auto RemoveBraces = [=]() mutable {
-    if (KeepBraces || !SimpleBlock)
+    if (!SimpleBlock)
       return false;
     assert(Tok->isOneOf(TT_ControlStatementLBrace, TT_ElseLBrace));
     assert(FormatTok->is(tok::r_brace));
     const bool WrappedOpeningBrace = !Tok->Previous;
     if (WrappedOpeningBrace && FollowedByComment)
       return false;
+    const bool HasRequiredIfBraces = IfLBrace && !IfLBrace->Optional;
+    if (KeepBraces && !HasRequiredIfBraces)
+      return false;
+    if (Tok->isNot(TT_ElseLBrace) || !HasRequiredIfBraces) {
       const FormatToken *Previous = Tokens->getPreviousToken();
       assert(Previous);
       if (Previous->is(tok::r_brace) && !Previous->Optional)
         return false;
+    }
     assert(!CurrentLines->empty());
     if (!mightFitOnOneLine(CurrentLines->back()))
       return false;
     if (Tok->is(TT_ElseLBrace))
       return true;
     if (WrappedOpeningBrace) {
       assert(Index > 0);
       --Index; // The line above the wrapped l_brace.
FormatToken *UnwrappedLineParser::parseBlock(
   if (PPStartHash == PPEndHash) {
     Line->MatchingOpeningBlockLineIndex = OpeningLineIndex;
     if (OpeningLineIndex != UnwrappedLine::kInvalidIndex) {
       // Update the opening line to add the forward reference as well
       (*CurrentLines)[OpeningLineIndex].MatchingClosingBlockLineIndex =
           CurrentLines->size() - 1;
     }
   }
+
+  return IfLBrace;
 }
 
 static bool isGoogScope(const UnwrappedLine &Line) {
   // FIXME: Closure-library specific stuff should not be hard-coded but be
   // configurable.
   if (Line.Tokens.size() < 4)
     return false;
   auto I = Line.Tokens.begin();
if (NextMustBeValue && !NextEndsTemplateExpr && !PreviousStartsTemplateExpr &&
     return addUnwrappedLine();
   }
   if ((PreviousMustBeValue || Previous->is(tok::r_paren)) &&
       isJSDeclOrStmt(Keywords, Next)) {
     return addUnwrappedLine();
   }
 }
 
-void UnwrappedLineParser::parseStructuralElement(bool IsTopLevel,
-                                                 TokenType NextLBracesType,
-                                                 IfStmtKind *IfKind,
-                                                 bool *HasDoWhile,
-                                                 bool *HasLabel) {
+void UnwrappedLineParser::parseStructuralElement(
+    bool IsTopLevel, TokenType NextLBracesType, IfStmtKind *IfKind,
+    FormatToken **IfLeftBrace, bool *HasDoWhile, bool *HasLabel) {
   if (Style.Language == FormatStyle::LK_TableGen &&
       FormatTok->is(tok::pp_include)) {
     nextToken();
     if (FormatTok->is(tok::string_literal))
       nextToken();
     addUnwrappedLine();
     return;
   }
void UnwrappedLineParser::parseStructuralElement(
   case tok::kw_private:
     if (Style.Language == FormatStyle::LK_Java || Style.isJavaScript() ||
         Style.isCSharp()) {
       nextToken();
     } else {
       parseAccessSpecifier();
     }
     return;
-  case tok::kw_if:
+  case tok::kw_if: {
     if (Style.isJavaScript() && Line->MustBeDeclaration) {
       // field/method declaration.
       break;
     }
-    parseIfThenElse(IfKind);
+    FormatToken *Tok = parseIfThenElse(IfKind);
+    if (IfLeftBrace)
+      *IfLeftBrace = Tok;
     return;
+  }
   case tok::kw_for:
   case tok::kw_while:
     if (Style.isJavaScript() && Line->MustBeDeclaration) {
       // field/method declaration.
       break;
     }
     parseForOrWhileLoop();
     return;
if (FormatTok->is(tok::kw_else)) {
     }
     nextToken();
     handleAttributes();
     if (FormatTok->is(tok::l_brace)) {
       FormatTok->setFinalizedType(TT_ElseLBrace);
       ElseLeftBrace = FormatTok;
       CompoundStatementIndenter Indenter(this, Style, Line->Level);
       IfStmtKind ElseBlockKind = IfStmtKind::NotIf;
+      FormatToken *IfLBrace =
           parseBlock(/*MustBeDeclaration=*/false, /*AddLevels=*/1u,
                      /*MunchSemi=*/true, KeepElseBraces, &ElseBlockKind);
-      if ((ElseBlockKind == IfStmtKind::IfOnly ||
-           ElseBlockKind == IfStmtKind::IfElseIf) &&
-          FormatTok->is(tok::kw_else)) {
+      if (FormatTok->is(tok::kw_else)) {
+        KeepElseBraces = KeepElseBraces ||
+                         ElseBlockKind == IfStmtKind::IfOnly ||
+                         ElseBlockKind == IfStmtKind::IfElseIf;
+      } else if (IfLBrace && !IfLBrace->Optional) {
         KeepElseBraces = true;
+        assert(ElseLeftBrace->MatchingParen);
+        markOptionalBraces(ElseLeftBrace);
       }
       addUnwrappedLine();
     } else if (FormatTok->is(tok::kw_if)) {
       const FormatToken *Previous = Tokens->getPreviousToken();
       assert(Previous);
       const bool IsPrecededByComment = Previous->is(tok::comment);
       if (IsPrecededByComment) {
         addUnwrappedLine();
         ++Line->Level;
       }
       bool TooDeep = true;
       if (Style.RemoveBracesLLVM) {
         Kind = IfStmtKind::IfElseIf;
         TooDeep = NestedTooDeep.pop_back_val();

clang/unittests/Format/FormatTest.cpp

verifyFormat("if (a)\n"
                "      e;\n"
                "    else if (f)\n"
                "      g;\n"
                "  }\n"
                "else\n"
                "  h;",
                Style);
 
+  verifyFormat("if (a) {\n"
+               "  b;\n"
+               "} else if (c) {\n"
+               "  d;\n"
+               "  e;\n"
+               "}",
+               "if (a) {\n"
+               "  b;\n"
+               "} else {\n"
+               "  if (c) {\n"
+               "    d;\n"
+               "    e;\n"
+               "  }\n"
+               "}",
+               Style);
+
+  verifyFormat("if (a) {\n"
+               "  b;\n"
+               "  c;\n"
+               "} else if (d) {\n"
+               "  e;\n"
+               "  f;\n"
+               "}",
+               "if (a) {\n"
+               "  b;\n"
+               "  c;\n"
+               "} else {\n"
+               "  if (d) {\n"
+               "    e;\n"
+               "    f;\n"
+               "  }\n"
+               "}",
+               Style);
+
+  verifyFormat("if (a) {\n"
+               "  b;\n"
+               "} else if (c) {\n"
+               "  d;\n"
+               "} else {\n"
+               "  e;\n"
+               "  f;\n"
+               "}",
+               "if (a) {\n"
+               "  b;\n"
+               "} else {\n"
+               "  if (c) {\n"
+               "    d;\n"
+               "  } else {\n"
+               "    e;\n"
+               "    f;\n"
+               "  }\n"
+               "}",
+               Style);
+
+  verifyFormat("if (a) {\n"
+               "  b;\n"
+               "} else if (c) {\n"
+               "  d;\n"
+               "} else if (e) {\n"
+               "  f;\n"
+               "  g;\n"
+               "}",
+               "if (a) {\n"
+               "  b;\n"
+               "} else {\n"
+               "  if (c) {\n"
+               "    d;\n"
+               "  } else if (e) {\n"
+               "    f;\n"
+               "    g;\n"
+               "  }\n"
+               "}",
+               Style);
+
+  verifyFormat("if (a) {\n"
+               "  if (b)\n"
+               "    c;\n"
+               "  else if (d) {\n"
+               "    e;\n"
+               "    f;\n"
+               "  }\n"
+               "} else {\n"
+               "  g;\n"
+               "}",
+               "if (a) {\n"
+               "  if (b)\n"
+               "    c;\n"
+               "  else {\n"
+               "    if (d) {\n"
+               "      e;\n"
+               "      f;\n"
+               "    }\n"
+               "  }\n"
+               "} else {\n"
+               "  g;\n"
+               "}",
+               Style);
+
+  verifyFormat("if (a)\n"
+               "  if (b)\n"
+               "    c;\n"
+               "  else {\n"
+               "    if (d) {\n"
+               "      e;\n"
+               "      f;\n"
+               "    }\n"
+               "  }\n"
+               "else\n"
+               "  g;",
+               Style);
+
   verifyFormat("if (a)\n"
                "  b;\n"
                "else if (c)\n"
                "  while (d)\n"
                "    e;\n"
                "// comment",
                "if (a)\n"
                "{\n"