[Clang] Support label at end of compound statement

[Izaron]

Summary

Implements paper P2324R2
https://wg21.link/p2324r2
https://github.com/cplusplus/papers/issues/1006

[Izaron]

I thought it was a good idea to slightly rename err_label_end_of_compound_statement to err_SWITCH_label_end_of_compound_statement because we aren't going to support empty labels in switch statements. (GCC also doesn't compile it - https://godbolt.org/z/xabKaon8v)

So now we need to clarify that we don't support empty labels in SWITCH compound statements (just there).

[cor3ntin]

Thanks for doing that work.

Given that this paper was touted as a C compatibility feature, I think we should implement the C feature at the same time
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2508.pdf

If my understanding is correct the change implemented here should just be allowed - without warning in C23
What do you think @aaron.ballman ?

[shafik]

The proposal says:

A label at the end of a compound statement is treated as if it were followed by a null statement

Does it make sense to write an AST test to verify this?

[aaron.ballman]

In D133887#3790414, @cor3ntin wrote:

Thanks for doing that work.

Given that this paper was touted as a C compatibility feature, I think we should implement the C feature at the same time
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2508.pdf

If my understanding is correct the change implemented here should just be allowed - without warning in C23
What do you think @aaron.ballman ?

I don't think the two papers diverge in any way, so I think the implementation for one should basically cover the work for the other (aside from test cases, etc). It should definitely be without warning in C23 mode and give an extension warning in earlier modes.

In D133887#3791243, @shafik wrote:

The proposal says:

A label at the end of a compound statement is treated as if it were followed by a null statement

Does it make sense to write an AST test to verify this?

I think a test like that would be reasonable, yes.

[Izaron]

Change C2x implementation status and C2x release notes

Add an AST test

[Izaron]

It should definitely be without warning in C23 mode and give an extension warning in earlier modes.

@aaron.ballman we have this extension warning for pre-C++23:

def ext_label_end_of_compound_statement : ExtWarn<
  "label at end of compound statement is a C++2b extension">,
   InGroup<CXX2b>;

Should I add a new warning for pre-C23? We could have two warnings for each language:

ext_cxx_label_end_of_compound_statement
ext_c_label_end_of_compound_statement

[cor3ntin]

In D133887#3797491, @Izaron wrote:

It should definitely be without warning in C23 mode and give an extension warning in earlier modes.

@aaron.ballman we have this extension warning for pre-C++23:

def ext_label_end_of_compound_statement : ExtWarn<
  "label at end of compound statement is a C++2b extension">,
   InGroup<CXX2b>;

Should I add a new warning for pre-C23? We could have two warnings for each language:

ext_cxx_label_end_of_compound_statement
ext_c_label_end_of_compound_statement

I think that's reasonable, yes - given that we need different warning groups

[Izaron]

Add diagnostics for C language

[cor3ntin]

clang/lib/Parse/ParseStmt.cpp

738

I do not understand this. Why checking getLangOpts().C99 instead of just C? Whyt no compatibility warning in C23 mode?
I think the code should look very similar for both C++ and C

[Izaron]

Why checking getLangOpts().C99 instead of just C

There is no getLangOpts().C. Here are possible C/C++ opt flags:
https://github.com/llvm/llvm-project/blob/7914e53e312074828293356f569d190ac6eae3bd/clang/include/clang/Basic/LangOptions.def#L86-L100
I have no understanding why there is no getLangOpts().C flag. Maybe the C89 standard is a subset of all other C/C++/ObjC standards, so we don't need the flag?..

Whyt no compatibility warning in C23 mode

@aaron.ballman said so in https://reviews.llvm.org/D133887#3793027

It should definitely be without warning in C23 mode and give an extension warning in earlier modes.

I don't know much about extension/incompatible warnings policy (when to apply and not apply them), could you please help me to figure this out with Aaron? 😃

[cor3ntin]

In D133887#3797614, @Izaron wrote:

Why checking getLangOpts().C99 instead of just C

There is no getLangOpts().C. Here are possible C/C++ opt flags:
https://github.com/llvm/llvm-project/blob/7914e53e312074828293356f569d190ac6eae3bd/clang/include/clang/Basic/LangOptions.def#L86-L100
I have no understanding why there is no getLangOpts().C flag. Maybe the C89 standard is a subset of all other C/C++/ObjC standards, so we don't need the flag?..

I missed that.
Yes, i think the way do that is to check !getLangOpts().CPlusPlus

Whyt no compatibility warning in C23 mode

@aaron.ballman said so in https://reviews.llvm.org/D133887#3793027

It should definitely be without warning in C23 mode and give an extension warning in earlier modes.

I don't know much about extension/incompatible warnings policy (when to apply and not apply them), could you please help me to figure this out with Aaron? 😃

I checked with Aaron, he meant no warning _by default_, but there should definitely be an extension warning. exact same thing as C++.
Sorry for the confusion.

[Izaron]

Fix diagnostics for C. Thanks to @cor3ntin and @aaron.ballman!

[cor3ntin]

Thanks.
This LGTM now

[aaron.ballman]

Thank you for working on this! I spotted an issue where we're not quite complete with this implementation work. I pushed up a new test case in https://github.com/llvm/llvm-project/commit/a244194f73788de6dfd6d753ff09be3625613f9f that shows it (and set the C status page back to Partial in case this takes a while to address), but case and default labels should be accepted the same as any other label at the end of a compound statement.

Btw, when working on C standards features, I've started adding test coverage that demonstrates we fully implement the paper into the clang/test/C/ directory, under the correct standard version. This helps us to track the status of proposals a bit more easily within the code base.

clang/include/clang/Basic/DiagnosticParseKinds.td

298–299

This is not an error in C2x or in C++2b.

[Izaron]

In D133887#3799310, @aaron.ballman wrote:

Thank you for working on this! I spotted an issue where we're not quite complete with this implementation work. I pushed up a new test case in https://github.com/llvm/llvm-project/commit/a244194f73788de6dfd6d753ff09be3625613f9f that shows it (and set the C status page back to Partial in case this takes a while to address), but case and default labels should be accepted the same as any other label at the end of a compound statement.

Btw, when working on C standards features, I've started adding test coverage that demonstrates we fully implement the paper into the clang/test/C/ directory, under the correct standard version. This helps us to track the status of proposals a bit more easily within the code base.

Hi! Thanks for this information! How did you find out that we should accept labels at end of statement in C, but not in C++? That is not obvious for me unfortunately. If C supports it, why would C++ not support it?

[aaron.ballman]

In D133887#3799347, @Izaron wrote:

In D133887#3799310, @aaron.ballman wrote:

Thank you for working on this! I spotted an issue where we're not quite complete with this implementation work. I pushed up a new test case in https://github.com/llvm/llvm-project/commit/a244194f73788de6dfd6d753ff09be3625613f9f that shows it (and set the C status page back to Partial in case this takes a while to address), but case and default labels should be accepted the same as any other label at the end of a compound statement.

Btw, when working on C standards features, I've started adding test coverage that demonstrates we fully implement the paper into the clang/test/C/ directory, under the correct standard version. This helps us to track the status of proposals a bit more easily within the code base.

Hi! Thanks for this information! How did you find out that we should accept labels at end of statement in C, but not in C++? That is not obvious for me unfortunately. If C supports it, why would C++ not support it?

I knew the answer for C off the top of my head, but was still looking up the info for C++. C++ does actually behave the same way:

https://eel.is/c++draft/stmt.label#1 -- case and default are both a label
https://eel.is/c++draft/stmt.block#1.sentence-2 -- all labels at the end of a compound statement are treated as if they are followed by a null statement
https://eel.is/c++draft/stmt.stmt#nt:selection-statement -- a switch is followed by a statement
https://eel.is/c++draft/stmt.stmt#stmt.pre-1 -- a compound-statement is a statement

I can fix up the C++ status page as well if you'd like, or I can hold off if you're already working on this, it's up to you!

[Izaron]

In D133887#3799399, @aaron.ballman wrote:

I can fix up the C++ status page as well if you'd like, or I can hold off if you're already working on this, it's up to you!

Thank you! I'll support empty labels in switch statements in a new pull request as soon as I can 😁

[aaron.ballman]

In D133887#3799450, @Izaron wrote:

In D133887#3799399, @aaron.ballman wrote:

I can fix up the C++ status page as well if you'd like, or I can hold off if you're already working on this, it's up to you!

Thank you! I'll support empty labels in switch statements in a new pull request as soon as I can 😁

Thank you, I appreciate it!

clang/docs/ReleaseNotes.rst

- Adjusted ``-Wformat`` warnings according to `WG14 N2562 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2562.pdf>`_.
   ``%hd``.
 
 C2x Feature Support
 -------------------
 - Implemented `WG14 N2662 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2662.pdf>`_,
   so the [[maybe_unused]] attribute may be applied to a label to silence an
   ``-Wunused-label`` warning.
 
+- Implemented `WG14 N508 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2508.pdf>`_,
+  so labels can placed everywhere inside a compound statement.
+
 C++ Language Changes in Clang
 -----------------------------
 - Implemented DR692, DR1395 and DR1432. Use the ``-fclang-abi-compat=15`` option
   to get the old partial ordering behavior regarding packs.
 - Clang's default C++/ObjC++ standard is now ``gnu++17`` instead of ``gnu++14``.
   This means Clang will by default accept code using features from C++17 and
   conforming GNU extensions. Projects incompatible with C++17 can add
   ``-std=gnu++14`` to their build settings to restore the previous behaviour.
- Skip rebuilding lambda expressions in arguments of immediate invocations.
   `Issue 51695 <https://github.com/llvm/llvm-project/issues/51695>`_,
   `Issue 50455 <https://github.com/llvm/llvm-project/issues/50455>`_,
   `Issue 54872 <https://github.com/llvm/llvm-project/issues/54872>`_,
   `Issue 54587 <https://github.com/llvm/llvm-project/issues/54587>`_.
 
 C++2b Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 
+- Support label at end of compound statement (`P2324 <https://wg21.link/p2324r2>`_).
+
 CUDA/HIP Language Changes in Clang
 ----------------------------------
 
 Objective-C Language Changes in Clang
 -------------------------------------
 
 OpenCL C Language Changes in Clang
 ----------------------------------

clang/include/clang/Basic/DiagnosticParseKinds.td

def warn_missing_selector_name : Warning<
   "%0 used as the name of the previous parameter rather than as part "
   "of the selector">,
   InGroup<DiagGroup<"missing-selector-name">>;
 def note_missing_selector_name : Note<
   "introduce a parameter name to make %0 part of the selector">;
 def note_force_empty_selector_name : Note<
   "or insert whitespace before ':' to use %0 as parameter name "
   "and have an empty entry in the selector">;
-def err_label_end_of_compound_statement : Error<
-  "label at end of compound statement: expected statement">;
+def err_switch_label_end_of_compound_statement : Error<
+  "label at end of switch compound statement: expected statement">;
+def ext_c_label_end_of_compound_statement : ExtWarn<
+  "label at end of compound statement is a C2x extension">,
+   InGroup<C2x>;
+def ext_cxx_label_end_of_compound_statement : ExtWarn<
+  "label at end of compound statement is a C++2b extension">,
+   InGroup<CXX2b>;
+def warn_c2x_compat_label_end_of_compound_statement : Warning<
+  "label at end of compound statement is incompatible with C standards before C2x">,
+  InGroup<CPre2xCompat>, DefaultIgnore;
+def warn_cxx20_compat_label_end_of_compound_statement : Warning<
+  "label at end of compound statement is incompatible with C++ standards before C++2b">,
+  InGroup<CXXPre2bCompat>, DefaultIgnore;
 def err_address_of_label_outside_fn : Error<
   "use of address-of-label extension outside of a function body">;
 def err_asm_operand_wide_string_literal : Error<
   "cannot use %select{unicode|wide|an empty}0 string literal in 'asm'">;
 def err_expected_selector_for_method : Error<
   "expected selector for Objective-C method">;
 def err_expected_property_name : Error<"expected property name">;
 

clang/lib/Parse/ParseStmt.cpp

 ///
 StmtResult Parser::ParseSEHLeaveStatement() {
   SourceLocation LeaveLoc = ConsumeToken();  // eat the '__leave'.
   return Actions.ActOnSEHLeaveStmt(LeaveLoc, getCurScope());
 }
 
 /// ParseLabeledStatement - We have an identifier and a ':' after it.
 ///
+///       label:
+///         identifier ':'
+/// [GNU]   identifier ':' attributes[opt]
+///
 ///       labeled-statement:
-///         identifier ':' statement
-/// [GNU]   identifier ':' attributes[opt] statement
+///         label statement
 ///
 StmtResult Parser::ParseLabeledStatement(ParsedAttributes &Attrs,
                                          ParsedStmtContext StmtCtx) {
   assert(Tok.is(tok::identifier) && Tok.getIdentifierInfo() &&
          "Not an identifier!");
 
   // The substatement is always a 'statement', not a 'declaration', but is
   // otherwise in the same context as the labeled-statement.
else {
       ParsedAttributes EmptyCXX11Attrs(AttrFactory);
       SubStmt = ParseStatementOrDeclarationAfterAttributes(
           Stmts, StmtCtx, nullptr, EmptyCXX11Attrs, TempAttrs);
       if (!TempAttrs.empty() && !SubStmt.isInvalid())
         SubStmt = Actions.ActOnAttributedStmt(TempAttrs, SubStmt.get());
     }
   }
 
+  // The label may have no statement following it
+  if (SubStmt.isUnset() && Tok.is(tok::r_brace)) {
+    if (getLangOpts().CPlusPlus) {
+      Diag(Tok, getLangOpts().CPlusPlus2b
+                    ? diag::warn_cxx20_compat_label_end_of_compound_statement
+                    : diag::ext_cxx_label_end_of_compound_statement);
+    } else {
+      Diag(Tok, getLangOpts().C2x
+                    ? diag::warn_c2x_compat_label_end_of_compound_statement
+                    : diag::ext_c_label_end_of_compound_statement);
+    }
+    SubStmt = Actions.ActOnNullStmt(ColonLoc);
+  }
+
   // If we've not parsed a statement yet, parse one now.
   if (!SubStmt.isInvalid() && !SubStmt.isUsable())
     SubStmt = ParseStatement(nullptr, StmtCtx);
 
   // Broken substmt shouldn't prevent the label from being added to the AST.
   if (SubStmt.isInvalid())
     SubStmt = Actions.ActOnNullStmt(ColonLoc);
 
StmtResult Parser::ParseCaseStatement(ParsedStmtContext StmtCtx,
   if (Tok.isNot(tok::r_brace)) {
     SubStmt = ParseStatement(/*TrailingElseLoc=*/nullptr, StmtCtx);
   } else {
     // Nicely diagnose the common error "switch (X) { case 4: }", which is
     // not valid.  If ColonLoc doesn't point to a valid text location, there was
     // another parsing error, so avoid producing extra diagnostics.
     if (ColonLoc.isValid()) {
       SourceLocation AfterColonLoc = PP.getLocForEndOfToken(ColonLoc);
-      Diag(AfterColonLoc, diag::err_label_end_of_compound_statement)
+      Diag(AfterColonLoc, diag::err_switch_label_end_of_compound_statement)
           << FixItHint::CreateInsertion(AfterColonLoc, " ;");
     }
     SubStmt = StmtError();
   }
 
   // Install the body into the most deeply-nested case.
   if (DeepestParsedCaseStmt) {
     // Broken sub-stmt shouldn't prevent forming the case statement properly.
     if (SubStmt.isInvalid())
StmtResult Parser::ParseDefaultStatement(ParsedStmtContext StmtCtx) {
   StmtResult SubStmt;
 
   if (Tok.isNot(tok::r_brace)) {
     SubStmt = ParseStatement(/*TrailingElseLoc=*/nullptr, StmtCtx);
   } else {
     // Diagnose the common error "switch (X) {... default: }", which is
     // not valid.
     SourceLocation AfterColonLoc = PP.getLocForEndOfToken(ColonLoc);
-    Diag(AfterColonLoc, diag::err_label_end_of_compound_statement)
+    Diag(AfterColonLoc, diag::err_switch_label_end_of_compound_statement)
         << FixItHint::CreateInsertion(AfterColonLoc, " ;");
     SubStmt = true;
   }
 
   // Broken sub-stmt shouldn't prevent forming the case statement properly.
   if (SubStmt.isInvalid())
     SubStmt = Actions.ActOnNullStmt(ColonLoc);
 
   return Actions.ActOnDefaultStmt(DefaultLoc, ColonLoc,
IsStmtExprResult = GetLookAheadToken(LookAhead).is(tok::r_brace) &&
                        GetLookAheadToken(LookAhead + 1).is(tok::r_paren);
   }
 
   if (IsStmtExprResult)
     E = Actions.ActOnStmtExprResult(E);
   return Actions.ActOnExprStmt(E, /*DiscardedValue=*/!IsStmtExprResult);
 }
 
-/// ParseCompoundStatementBody - Parse a sequence of statements and invoke the
-/// ActOnCompoundStmt action.  This expects the '{' to be the current token, and
-/// consume the '}' at the end of the block.  It does not manipulate the scope
-/// stack.
+/// ParseCompoundStatementBody - Parse a sequence of statements optionally
+/// followed by a label and invoke the ActOnCompoundStmt action.  This expects
+/// the '{' to be the current token, and consume the '}' at the end of the
+/// block.  It does not manipulate the scope stack.
 StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) {
   PrettyStackTraceLoc CrashInfo(PP.getSourceManager(),
                                 Tok.getLocation(),
                                 "in compound statement ('{}')");
 
   // Record the current FPFeatures, restore on leaving the
   // compound statement.
   Sema::FPFeaturesStateRAII SaveFPFeatures(Actions);

clang/test/AST/ast-dump-stmt.c

label2:
   // CHECK-NEXT: VarDecl
   // CHECK-NEXT: AddrLabelExpr 0x{{[^ ]*}} <col:15, col:17> 'void *' label1 0x{{[^ ]*}}
 
   goto *ptr;
   // CHECK-NEXT: IndirectGotoStmt 0x{{[^ ]*}} <line:[[@LINE-1]]:3, col:9>
   // CHECK-NEXT: ImplicitCastExpr
   // CHECK-NEXT: ImplicitCastExpr
   // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}} <col:9> 'void *' lvalue Var 0x{{[^ ]*}} 'ptr' 'void *'
+
+label3:
+  // CHECK-NEXT: LabelStmt 0x{{[^ ]*}} <line:[[@LINE-1]]:1, col:7> 'label3'
+  // CHECK-NEXT: NullStmt 0x{{[^ ]*}} <col:7>
 }
 
 void TestSwitch(int i) {
   switch (i) {
   // CHECK: SwitchStmt 0x{{[^ ]*}} <line:[[@LINE-1]]:3, line:[[@LINE+32]]:3>
   // CHECK-NEXT: ImplicitCastExpr
   // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}} <col:11> 'int' lvalue ParmVar 0x{{[^ ]*}} 'i' 'int'
   // CHECK-NEXT: CompoundStmt 0x{{[^ ]*}} <col:14, line:[[@LINE+29]]:3>

clang/test/Parser/c2x-label.c

+// RUN: %clang_cc1 -fsyntax-only -std=c17 -Wc2x-compat -verify=c17 %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -Wpre-c2x-compat -verify=c2x %s
+
+void foo() {
+    int x;
+label1:
+    x = 1;
+label2: label3: label4:
+} // c17-warning {{label at end of compound statement is a C2x extension}} \
+     c2x-warning {{label at end of compound statement is incompatible with C standards before C2x}}

clang/test/Parser/cxx2b-label.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify=expected,cxx2b -std=c++2b -Wpre-c++2b-compat %s
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,cxx20 -std=c++20 %s
+
+void foo() {
+label1:
+    int x;
+label2:
+    x = 1;
+label3: label4: label5:
+} // cxx20-warning {{label at end of compound statement is a C++2b extension}} \
+     cxx2b-warning {{label at end of compound statement is incompatible with C++ standards before C++2b}}

clang/test/Parser/switch-recovery.cpp

while (x) {
       }
     }
   }
 }
 
 void missing_statement_case(int x) {
   switch (x) {
     case 1:
-    case 0: // expected-error {{label at end of compound statement: expected statement}}
+    case 0: // expected-error {{label at end of switch compound statement: expected statement}}
   }
 }
 
 void missing_statement_default(int x) {
   switch (x) {
     case 0:
-    default: // expected-error {{label at end of compound statement: expected statement}}
+    default: // expected-error {{label at end of switch compound statement: expected statement}}
   }
 }
 
 void pr19022_1() {
   switch (int x)  // expected-error {{variable declaration in condition must have an initializer}}
   case v: ;  // expected-error {{use of undeclared identifier 'v'}}
 }
 
 void pr19022_1a(int x) {
   switch(x) {
   case 1  // expected-error{{expected ':' after 'case'}} \
-          // expected-error{{label at end of compound statement: expected statement}}
+          // expected-error{{label at end of switch compound statement: expected statement}}
   }
 }
 
 void pr19022_1b(int x) {
   switch(x) {
   case v  // expected-error{{use of undeclared identifier 'v'}} \
           // expected-error{{expected ':' after 'case'}}
   } // expected-error{{expected statement}}

clang/www/c_status.html

<tr>
       <td>Allowing unnamed parameters in function definitions</td>
       <td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2480.pdf">N2480</a></td>
       <td class="full" align="center">Clang 11</td>
     </tr>
     <!-- Aug 2020 Papers -->
     <tr>
       <td>Free positioning of labels inside compound statements</td>
       <td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2508.pdf">N2508</a></td>
-      <td class="none" align="center">No</td>
+      <td class="unreleased" align="center">Clang 16</td>
     </tr>
     <tr>
       <td>Clarification request for C17 example of undefined behavior</td>
       <td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2517.pdf">N2517</a></td>
       <td class="none" align="center">No</td>
     </tr>
     <tr>
       <td>Querying attribute support</td>

clang/www/cxx_status.html

</tr>
     <tr>
       <td>Remove non-encodable wide character literals and multicharacter wide character literals</td>
       <td><a href="https://wg21.link/P2362R3">P2362R3</a></td>
       <td class="full" align="center">Clang 14</td>
     </tr>
     <tr>
       <td>Labels at the end of compound statements</td>
       <td><a href="https://wg21.link/P2324R2">P2324R2</a></td>
-      <td class="none" align="center">No</td>
+      <td class="unreleased" align="center">Clang 16</td>
     </tr>
     <tr>
       <td>Delimited escape sequences</td>
       <td><a href="https://wg21.link/P2290R3">P2290R3</a></td>
       <td class="full" align="center">Clang 15</td>
     </tr>
     <tr>
       <td>Named universal character escapes</td>