[clang-format] Handle attributes before case label.

[curdeius]

Summary

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

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

578

Can we remove this empty line?

clang/lib/Format/UnwrappedLineParser.h

127–128

Can we move them 1 line up so that the order of the declarations here will be the same as that of the definitions in the .cpp file?

[curdeius]

clang/lib/Format/UnwrappedLineParser.cpp

578

Will do.

clang/lib/Format/UnwrappedLineParser.h

127–128

Will do.

[jgorbe]

Hi,

We've observed that this patch introduces infinite loops in some cases. Here's a reduced test case:

export class Foo extends Bar {
  get case(): Case {
    return (
        (this.Bar$has('case')) ? (this.Bar$get('case')) :
                                 (this.case = new Case()));
  }
}

Saving this as /tmp/test.ts and running clang-format /tmp/test.ts loops indefinitely with this patch (I stopped it after 1m27s) and finishes instantly (0.12s) after reverting the patch locally. I'm going to go ahead and push a revert. Please let me know if you need help reproducing the problem.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineParser.cpp

1826

Here's the loop. In the switch before that worked, since it was not in a loop. Here we need to do something. (nextToken()?)

[curdeius]

In D121450#3397861, @jgorbe wrote:

Hi,

We've observed that this patch introduces infinite loops in some cases. Here's a reduced test case:

export class Foo extends Bar {
  get case(): Case {
    return (
        (this.Bar$has('case')) ? (this.Bar$get('case')) :
                                 (this.case = new Case()));
  }
}

Saving this as /tmp/test.ts and running clang-format /tmp/test.ts loops indefinitely with this patch (I stopped it after 1m27s) and finishes instantly (0.12s) after reverting the patch locally. I'm going to go ahead and push a revert. Please let me know if you need help reproducing the problem.

Thanks for the reproducer.

clang/lib/Format/UnwrappedLineParser.cpp

1826

Yes, it seems so. I'll add a test case and fix it.

[curdeius]

Relanded in https://github.com/llvm/llvm-project/commit/4e88cb6825eefca3c0eb66b5ae40ab123fcc7073 with a fix and a regression test.

clang/lib/Format/UnwrappedLineParser.h

void parseStructuralElement(IfStmtKind *IfKind = 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();
   void parseUnbracedBody(bool CheckEOF = false);
+  void handleAttributes();
+  bool handleCppAttributes();
   FormatToken *parseIfThenElse(IfStmtKind *IfKind, bool KeepBraces = false);
   void parseTryCatch();
   void parseForOrWhileLoop();
   void parseDoWhile();
   void parseLabel(bool LeftAlignLabel = false);
   void parseCaseLabel();
   void parseSwitch();
   void parseNamespace();
   void parseModuleImport();
   void parseNew();
   void parseAccessSpecifier();

clang/lib/Format/UnwrappedLineParser.cpp

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)
       kind = tok::r_brace;
 
     auto ParseDefault = [this, HasOpeningBrace, IfKind, NextLevelLBracesType,
                          &HasLabel, &StatementCount] {
case tok::kw_case:
       parseStructuralElement();
       break;
     case tok::l_square:
       if (Style.isCSharp()) {
         nextToken();
         parseCSharpAttribute();
         break;
       }
+      if (handleCppAttributes())
+        break;
       LLVM_FALLTHROUGH;
     default:
       ParseDefault();
       break;
     }
   } while (!eof());
   return false;
 }
 
case tok::kw_default:
     nextToken();
     if (FormatTok->is(tok::colon)) {
       parseLabel();
       return;
     }
     // e.g. "default void f() {}" in a Java interface.
     break;
   case tok::kw_case:
-    if (Style.isJavaScript() && Line->MustBeDeclaration)
+    if (Style.isJavaScript() && Line->MustBeDeclaration) {
       // 'case: string' field declaration.
+      nextToken();
       break;
+    }
     parseCaseLabel();
     return;
   case tok::kw_try:
   case tok::kw___try:
     if (Style.isJavaScript() && Line->MustBeDeclaration)
       // field/method declaration.
       break;
     parseTryCatch();
case tok::equal:
       }
       break;
     case tok::l_square:
       parseSquare();
       break;
     case tok::kw_new:
       parseNew();
       break;
+    case tok::kw_case:
+      if (Style.isJavaScript() && Line->MustBeDeclaration)
+        // 'case: string' field declaration.
+        break;
+      parseCaseLabel();
+      break;
     default:
       nextToken();
       break;
     }
   } while (!eof());
 }
 
 bool UnwrappedLineParser::tryToParsePropertyAccessor() {
static void markOptionalBraces(FormatToken *LeftBrace) {
   assert(RightBrace->is(tok::r_brace));
   assert(RightBrace->MatchingParen == LeftBrace);
   assert(LeftBrace->Optional == RightBrace->Optional);
 
   LeftBrace->Optional = true;
   RightBrace->Optional = true;
 }
 
-FormatToken *UnwrappedLineParser::parseIfThenElse(IfStmtKind *IfKind,
-                                                  bool KeepBraces) {
-  auto HandleAttributes = [this]() {
-    // Handle AttributeMacro, e.g. `if (x) UNLIKELY`.
-    if (FormatTok->is(TT_AttributeMacro))
-      nextToken();
-    // Handle [[likely]] / [[unlikely]] attributes.
-    if (FormatTok->is(tok::l_square) && tryToParseSimpleAttribute())
-      parseSquare();
-  };
+void UnwrappedLineParser::handleAttributes() {
+  handleCppAttributes();
+}
+
+bool UnwrappedLineParser::handleCppAttributes() {
+  if (FormatTok->is(tok::l_square) && tryToParseSimpleAttribute()) {
+    return true;
+  }
+  return false;
+}
 
+FormatToken *UnwrappedLineParser::parseIfThenElse(IfStmtKind *IfKind,
+                                                  bool KeepBraces) {
   assert(FormatTok->is(tok::kw_if) && "'if' expected");
   nextToken();
   if (FormatTok->is(tok::exclaim))
     nextToken();
   if (FormatTok->is(tok::kw_consteval)) {
     nextToken();
   } else {
     if (FormatTok->isOneOf(tok::kw_constexpr, tok::identifier))
       nextToken();
     if (FormatTok->is(tok::l_paren))
       parseParens();
   }
-  HandleAttributes();
+  handleAttributes();
 
   bool NeedsUnwrappedLine = false;
   keepAncestorBraces();
 
   FormatToken *IfLeftBrace = nullptr;
   IfStmtKind IfBlockKind = IfStmtKind::NotIf;
 
   if (FormatTok->is(tok::l_brace)) {
FormatToken *UnwrappedLineParser::parseIfThenElse(IfStmtKind *IfKind,
   IfStmtKind Kind = IfStmtKind::IfOnly;
 
   if (FormatTok->is(tok::kw_else)) {
     if (Style.RemoveBracesLLVM) {
       NestedTooDeep.back() = false;
       Kind = IfStmtKind::IfElse;
     }
     nextToken();
-    HandleAttributes();
+    handleAttributes();
     if (FormatTok->is(tok::l_brace)) {
       ElseLeftBrace = FormatTok;
       CompoundStatementIndenter Indenter(this, Style, Line->Level);
       if (parseBlock() == IfStmtKind::IfOnly)
         Kind = IfStmtKind::IfElseIf;
       addUnwrappedLine();
     } else if (FormatTok->is(tok::kw_if)) {
       FormatToken *Previous = Tokens->getPreviousToken();

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, FormatsSwitchStatement) {
 
   verifyFormat("switch (a) {\n"
                "case some_namespace::\n"
                "    some_constant:\n"
                "  return;\n"
                "}",
                getLLVMStyleWithColumns(34));
 
+  verifyFormat("switch (a) {\n"
+               "[[likely]] case 1:\n"
+               "  return;\n"
+               "}");
+  verifyFormat("switch (a) {\n"
+               "[[likely]] [[other::likely]] case 1:\n"
+               "  return;\n"
+               "}");
+  verifyFormat("switch (x) {\n"
+               "case 1:\n"
+               "  return;\n"
+               "[[likely]] case 2:\n"
+               "  return;\n"
+               "}");
+  verifyFormat("switch (a) {\n"
+               "case 1:\n"
+               "[[likely]] case 2:\n"
+               "  return;\n"
+               "}");
+  FormatStyle Attributes = getLLVMStyle();
+  Attributes.AttributeMacros.push_back("LIKELY");
+  Attributes.AttributeMacros.push_back("OTHER_LIKELY");
+  verifyFormat("switch (a) {\n"
+               "LIKELY case b:\n"
+               "  return;\n"
+               "}",
+               Attributes);
+  verifyFormat("switch (a) {\n"
+               "LIKELY OTHER_LIKELY() case b:\n"
+               "  return;\n"
+               "}",
+               Attributes);
+  verifyFormat("switch (a) {\n"
+               "case 1:\n"
+               "  return;\n"
+               "LIKELY case 2:\n"
+               "  return;\n"
+               "}",
+               Attributes);
+  verifyFormat("switch (a) {\n"
+               "case 1:\n"
+               "LIKELY case 2:\n"
+               "  return;\n"
+               "}",
+               Attributes);
+
   FormatStyle Style = getLLVMStyle();
   Style.IndentCaseLabels = true;
   Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Never;
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.AfterCaseLabel = true;
   Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always;
   EXPECT_EQ("switch (n)\n"
             "{\n"