[clang-format] Correctly format goto labels followed by blocks

[sstwcw]

Summary

There doesn't seem to be an issue on GitHub. But previously, a space
would be inserted before the goto colon in the code below.

switch (x) {
case 0:
goto_0: {
  action();
  break;
}
}

Previously, the colon following a goto label would be annotated as
TT_InheritanceColon. A goto label followed by an opening brace
wasn't recognized. It is easy to add another line to have
spaceRequiredBefore function recognize the case, but I believed it
is more proper to avoid doing the same thing in UnwrappedLineParser
and TokenAnnotator. So now the label colons would be labeled in
UnwrappedLineParser, and spaceRequiredBefore would rely on that.

Previously we had the type TT_GotoLabelColon intended for both goto
labels and case labels. But since handling of goto labels and case
labels differ somewhat, I split it into separate types for goto and
case labels.

This patch doesn't change the behavior for case labels. I added the
lines annotating case labels because they would previously be
mistakenly annotated as TT_InheritanceColon just like goto labels.
And since I added the annotations, the checks for the case and
default keywords in spaceRequiredBefore are not necessary anymore.

[sstwcw]

clang/lib/Format/UnwrappedLineFormatter.cpp

714 ↗

(On Diff #514060)

Should I make this change?

Without it:

label: { break; }

With it:

label: {
  break;
}

Without the entire patch:

label : { break; }

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

4550

how is the semi case handled or is it not needed

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineFormatter.cpp

714 ↗

(On Diff #514060)

I'd say no. To not change this (too much).

clang/unittests/Format/FormatTest.cpp

3020–3024

Why did you remove that?

clang/unittests/Format/TokenAnnotatorTest.cpp

1711

remove

[sstwcw]

• Remove change in line wrapping

[sstwcw]

clang/lib/Format/TokenAnnotator.cpp

4550

It is not needed anymore. It was added to handle a semicolon following a goto label. Now the type TT_GotoLabelColon includes the case.

clang/unittests/Format/FormatTest.cpp

3020–3024

It was an accident.

[sstwcw]

Is this patch accepted or rejected?

clang/lib/Format/FormatToken.h

#define LIST_TOKEN_TYPES                                                       \
   TYPE(AttributeColon)                                                         \
   TYPE(AttributeMacro)                                                         \
   TYPE(AttributeParen)                                                         \
   TYPE(AttributeSquare)                                                        \
   TYPE(BinaryOperator)                                                         \
   TYPE(BitFieldColon)                                                          \
   TYPE(BlockComment)                                                           \
   TYPE(BracedListLBrace)                                                       \
+  /* The colon at the end of a case label. */                                  \
+  TYPE(CaseLabelColon)                                                         \
   TYPE(CastRParen)                                                             \
   TYPE(ClassLBrace)                                                            \
   TYPE(CompoundRequirementLBrace)                                              \
   /* ternary ?: expression */                                                  \
   TYPE(ConditionalExpr)                                                        \
   /* the condition in an if statement */                                       \
   TYPE(ConditionLParen)                                                        \
   TYPE(ConflictAlternative)                                                    \
#define LIST_TOKEN_TYPES                                                       \
   TYPE(ForEachMacro)                                                           \
   TYPE(FunctionAnnotationRParen)                                               \
   TYPE(FunctionDeclarationName)                                                \
   TYPE(FunctionLBrace)                                                         \
   TYPE(FunctionLikeOrFreestandingMacro)                                        \
   TYPE(FunctionTypeLParen)                                                     \
   /* The colons as part of a C11 _Generic selection */                         \
   TYPE(GenericSelectionColon)                                                  \
-  /* The colon at the end of a goto label or a case label. Currently only used \
-   * for Verilog. */                                                           \
+  /* The colon at the end of a goto label. */                                  \
   TYPE(GotoLabelColon)                                                         \
   TYPE(IfMacro)                                                                \
   TYPE(ImplicitStringLiteral)                                                  \
   TYPE(InheritanceColon)                                                       \
   TYPE(InheritanceComma)                                                       \
   TYPE(InlineASMBrace)                                                         \
   TYPE(InlineASMColon)                                                         \
   TYPE(InlineASMSymbolicNameLSquare)                                           \
return Tok.isOneOf(tok::kw_case, tok::kw_class, kw_function, kw_module,
                        kw_clocking, kw_covergroup, kw_macromodule, kw_primitive,
                        kw_program, kw_property, kw_randcase, kw_randsequence,
                        kw_task);
   }
 
   bool isVerilogEndOfLabel(const FormatToken &Tok) const {
     const FormatToken *Next = Tok.getNextNonComment();
     // In Verilog the colon in a default label is optional.
-    return Tok.is(TT_GotoLabelColon) ||
+    return Tok.is(TT_CaseLabelColon) ||
            (Tok.is(tok::kw_default) &&
             !(Next && Next->isOneOf(tok::colon, tok::semi, kw_clocking, kw_iff,
                                     kw_input, kw_output, kw_sequence)));
   }
 
   /// Returns whether \p Tok is a Verilog keyword that starts a
   /// structured procedure like 'always'.
   bool isVerilogStructuredProcedure(const FormatToken &Tok) const {

clang/lib/Format/TokenAnnotator.cpp

bool consumeToken() {
     case tok::plus:
     case tok::minus:
       if (!Tok->Previous && Line.MustBeDeclaration)
         Tok->setType(TT_ObjCMethodSpecifier);
       break;
     case tok::colon:
       if (!Tok->Previous)
         return false;
+      // Goto labels and case labels are already identified in
+      // UnwrappedLineParser.
+      if (Tok->isTypeFinalized())
+        break;
       // Colons from ?: are handled in parseConditional().
       if (Style.isJavaScript()) {
         if (Contexts.back().ColonIsForRangeExpr || // colon in for loop
             (Contexts.size() == 1 &&               // switch/case labels
              !Line.First->isOneOf(tok::kw_enum, tok::kw_case)) ||
             Contexts.back().ContextKind == tok::l_paren ||  // function params
             Contexts.back().ContextKind == tok::l_square || // array type
             (!Contexts.back().IsExpression &&
case tok::colon:
         } else if (Contexts.back().ContextKind == tok::l_square) {
           Tok->setType(TT_BitFieldColon);
         } else if (Contexts.back().ColonIsDictLiteral) {
           Tok->setType(TT_DictLiteral);
         } else if (Contexts.size() == 1) {
           // In Verilog a case label doesn't have the case keyword. We
           // assume a colon following an expression is a case label.
           // Colons from ?: are annotated in parseConditional().
-          Tok->setType(TT_GotoLabelColon);
+          Tok->setType(TT_CaseLabelColon);
           if (Line.Level > 1 || (!Line.InPPDirective && Line.Level > 0))
             --Line.Level;
         }
         break;
       }
       if (Line.First->isOneOf(Keywords.kw_module, Keywords.kw_import) ||
           Line.First->startsSequence(tok::kw_export, Keywords.kw_module) ||
           Line.First->startsSequence(tok::kw_export, Keywords.kw_import)) {
if (Right.is(TT_RangeBasedForLoopColon) &&
       !Style.SpaceBeforeRangeBasedForLoopColon) {
     return false;
   }
   if (Left.is(TT_BitFieldColon)) {
     return Style.BitFieldColonSpacing == FormatStyle::BFCS_Both ||
            Style.BitFieldColonSpacing == FormatStyle::BFCS_After;
   }
   if (Right.is(tok::colon)) {
-    if (Right.is(TT_GotoLabelColon) ||
-        (!Style.isVerilog() &&
-         Line.First->isOneOf(tok::kw_default, tok::kw_case))) {
+    if (Right.is(TT_CaseLabelColon))
       return Style.SpaceBeforeCaseColon;
-    }
-    const FormatToken *Next = Right.getNextNonComment();
-    if (!Next || Next->is(tok::semi))
+    if (Right.is(TT_GotoLabelColon))
+      return false;
+    // `private:` and `public:`.
+    if (!Right.getNextNonComment())
       return false;
     if (Right.is(TT_ObjCMethodExpr))
       return false;
     if (Left.is(tok::question))
       return false;
     if (Right.is(TT_InlineASMColon) && Left.is(tok::coloncolon))
       return false;
     if (Right.is(TT_DictLiteral))

clang/lib/Format/UnwrappedLineParser.cpp

case tok::kw_default:
     if (Style.isVerilog())
       break;
     if (Style.isJavaScript() && Line->MustBeDeclaration) {
       // 'default: string' field declaration.
       break;
     }
     nextToken();
     if (FormatTok->is(tok::colon)) {
+      FormatTok->setFinalizedType(TT_CaseLabelColon);
       parseLabel();
       return;
     }
     // e.g. "default void f() {}" in a Java interface.
     break;
   case tok::kw_case:
     // Proto: there are no switch/case statements.
     if (Style.isProto()) {
case tok::identifier: {
           ++I;
         return I != E && (++I == E);
       };
       if (OneTokenSoFar()) {
         // In Verilog labels can be any expression, so we don't do them here.
         if (!Style.isVerilog() && FormatTok->is(tok::colon) &&
             !Line->MustBeDeclaration) {
           Line->Tokens.begin()->Tok->MustBreakBefore = true;
+          FormatTok->setFinalizedType(TT_GotoLabelColon);
           parseLabel(!Style.IndentGotoLabels);
           if (HasLabel)
             *HasLabel = true;
           return;
         }
         // Recognize function-like macro usages without trailing semicolon as
         // well as free-standing macros like Q_OBJECT.
         bool FunctionLike = FormatTok->is(tok::l_paren);

 }
 
 void UnwrappedLineParser::parseCaseLabel() {
   assert(FormatTok->is(tok::kw_case) && "'case' expected");
 
   // FIXME: fix handling of complex expressions here.
   do {
     nextToken();
-  } while (!eof() && !FormatTok->is(tok::colon));
+    if (FormatTok->is(tok::colon)) {
+      FormatTok->setFinalizedType(TT_CaseLabelColon);
+      break;
+    }
+  } while (!eof());
   parseLabel();
 }
 
 void UnwrappedLineParser::parseSwitch() {
   assert(FormatTok->is(tok::kw_switch) && "'switch' expected");
   nextToken();
   if (FormatTok->is(tok::l_paren))
     parseParens();

clang/unittests/Format/FormatTest.cpp

verifyFormat("{\n"
                "test_label:\n"
                "  some_other_code();\n"
                "}");
   verifyFormat("{\n"
                "  some_code();\n"
                "test_label:;\n"
                "  int i = 0;\n"
                "}");
+  verifyFormat("{\n"
+               "  some_code();\n"
+               "test_label: { some_other_code(); }\n"
+               "}");
+  verifyFormat("{\n"
+               "  some_code();\n"
+               "test_label: {\n"
+               "  some_other_code();\n"
+               "  some_other_code();\n"
+               "}\n"
+               "}");
   FormatStyle Style = getLLVMStyle();
   Style.IndentGotoLabels = false;
   verifyFormat("void f() {\n"
                "  some_code();\n"
                "test_label:\n"
                "  some_other_code();\n"
                "  {\n"
                "    some_more_code();\n"
                "another_label:\n"
                "    some_more_code();\n"
                "  }\n"
                "}",
                Style);
   verifyFormat("{\n"
                "  some_code();\n"
                "test_label:\n"
                "  some_other_code();\n"
                "}",
                Style);
   verifyFormat("{\n"
                "  some_code();\n"
                "test_label:;\n"
                "  int i = 0;\n"
                "}");
+  verifyFormat("{\n"
+               "  some_code();\n"
+               "test_label: { some_other_code(); }\n"
+               "}",
+               Style);
+  // The opening brace may either be on the same unwrapped line as the colon or
+  // on a separate one. The formatter should recognize both.
+  Style = getLLVMStyle();
+  Style.BreakBeforeBraces = FormatStyle::BraceBreakingStyle::BS_Allman;
+  verifyFormat("{\n"
+               "  some_code();\n"
+               "test_label:\n"
+               "{\n"
+               "  some_other_code();\n"
+               "}\n"
+               "}",
+               Style);
 }
 
 TEST_F(FormatTest, MultiLineControlStatements) {
   FormatStyle Style = getLLVMStyleWithColumns(20);
   Style.BreakBeforeBraces = FormatStyle::BraceBreakingStyle::BS_Custom;
   Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_MultiLine;
   // Short lines should keep opening brace on same line.
   EXPECT_EQ("if (foo) {\n"
verifyFormat("switch (allBraces) {\n"
                "case 2 : {\n"
                "  [[fallthrough]];\n"
                "}\n"
                "default : {\n"
                "  break;\n"
                "}\n"
                "}",
                CaseStyle);
+  // Goto labels should not be affected.
+  verifyFormat("switch (x) {\n"
+               "goto_label:\n"
+               "default :\n"
+               "}",
+               CaseStyle);
+  verifyFormat("switch (x) {\n"
+               "goto_label: { break; }\n"
+               "default : {\n"
+               "  break;\n"
+               "}\n"
+               "}",
+               CaseStyle);
 
   FormatStyle NoSpaceStyle = getLLVMStyle();
   EXPECT_EQ(NoSpaceStyle.SpaceBeforeCaseColon, false);
   NoSpaceStyle.SpaceBeforeCtorInitializerColon = false;
   NoSpaceStyle.SpaceBeforeInheritanceColon = false;
   NoSpaceStyle.SpaceBeforeRangeBasedForLoopColon = false;
   verifyFormat("class Foo: public Bar {};", NoSpaceStyle);
   verifyFormat("Foo::Foo(): foo(1) {}", NoSpaceStyle);

clang/unittests/Format/TokenAnnotatorTest.cpp

TEST_F(TokenAnnotatorTest, UnderstandsVerilogOperators) {
   ASSERT_EQ(Tokens.size(), 10u) << Tokens;
   EXPECT_TOKEN(Tokens[4], tok::colon, TT_BitFieldColon);
   // Test case labels and ternary operators.
   Tokens = Annotate("case (x)\n"
                     "  x:\n"
                     "    x;\n"
                     "endcase\n");
   ASSERT_EQ(Tokens.size(), 10u) << Tokens;
-  EXPECT_TOKEN(Tokens[5], tok::colon, TT_GotoLabelColon);
+  EXPECT_TOKEN(Tokens[5], tok::colon, TT_CaseLabelColon);
   Tokens = Annotate("case (x)\n"
                     "  x ? x : x:\n"
                     "    x;\n"
                     "endcase\n");
   ASSERT_EQ(Tokens.size(), 14u) << Tokens;
   EXPECT_TOKEN(Tokens[5], tok::question, TT_ConditionalExpr);
   EXPECT_TOKEN(Tokens[7], tok::colon, TT_ConditionalExpr);
-  EXPECT_TOKEN(Tokens[9], tok::colon, TT_GotoLabelColon);
+  EXPECT_TOKEN(Tokens[9], tok::colon, TT_CaseLabelColon);
   // Non-blocking assignments.
   Tokens = Annotate("a <= b;");
   ASSERT_EQ(Tokens.size(), 5u) << Tokens;
   EXPECT_TOKEN(Tokens[1], tok::lessequal, TT_BinaryOperator);
   EXPECT_TOKEN_PRECEDENCE(Tokens[1], prec::Assignment);
   Tokens = Annotate("if (a <= b) break;");
   ASSERT_EQ(Tokens.size(), 9u) << Tokens;
   EXPECT_TOKEN(Tokens[3], tok::lessequal, TT_BinaryOperator);
TEST_F(TokenAnnotatorTest, UnderstandConstructors) {
   EXPECT_TOKEN(Tokens[8], tok::r_brace, TT_Unknown);
   EXPECT_TOKEN(Tokens[9], tok::comma, TT_CtorInitializerComma);
   EXPECT_TOKEN(Tokens[10], tok::identifier, TT_Unknown);
   EXPECT_TOKEN(Tokens[11], tok::l_brace, TT_Unknown);
   EXPECT_TOKEN(Tokens[12], tok::r_brace, TT_Unknown);
   EXPECT_TOKEN(Tokens[13], tok::l_brace, TT_FunctionLBrace);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsConditionParens) {
   auto Tokens = annotate("if (x) {}");
   ASSERT_EQ(Tokens.size(), 7u) << Tokens;
   EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_ConditionLParen);
   Tokens = annotate("if constexpr (x) {}");
   ASSERT_EQ(Tokens.size(), 8u) << Tokens;
   EXPECT_TOKEN(Tokens[2], tok::l_paren, TT_ConditionLParen);
   Tokens = annotate("if CONSTEXPR (x) {}");
   ASSERT_EQ(Tokens.size(), 8u) << Tokens;
TEST_F(TokenAnnotatorTest, CSharpNullableTypes) {
   EXPECT_EQ(Tokens.size(), 6u) << Tokens;
   EXPECT_TOKEN(Tokens[1], tok::question, TT_ConditionalExpr);
 
   Tokens = annotate("cond ? cond2 ? : id1 : id2", Style);
   EXPECT_EQ(Tokens.size(), 9u) << Tokens;
   EXPECT_TOKEN(Tokens[1], tok::question, TT_ConditionalExpr);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsLabels) {
+  auto Tokens = annotate("{ x: break; }");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[2], tok::colon, TT_GotoLabelColon);
+  Tokens = annotate("{ case x: break; }");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::colon, TT_CaseLabelColon);
+  Tokens = annotate("{ x: { break; } }");
+  ASSERT_EQ(Tokens.size(), 9u) << Tokens;
+  EXPECT_TOKEN(Tokens[2], tok::colon, TT_GotoLabelColon);
+  Tokens = annotate("{ case x: { break; } }");
+  ASSERT_EQ(Tokens.size(), 10u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::colon, TT_CaseLabelColon);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang