Better diagnostics for anonymous bit-fields with attributes or an initializer

[aaron.ballman]

Summary

The C++ grammar allows you to specify an attribute list on an anonymous bit-field, but we were not properly parsing that case. This would lead to a rejects-valid diagnostic with code like:

struct A {
  int x, [[]] : 0;
};

This patch addresses it by optionally parsing an attribute specifier in the case the identifier is elided. This fixes PR46562.

[rsmith]

I think we established rough consensus in CWG (at least, no one objected) that this is a grammar bug -- there's nothing for these attributes to appertain to, so they shouldn't be permitted. Unless CWG indicates it wants to change direction here I think we should continue to reject.

[aaron.ballman]

In D88333#2296252, @rsmith wrote:

I think we established rough consensus in CWG (at least, no one objected) that this is a grammar bug -- there's nothing for these attributes to appertain to, so they shouldn't be permitted. Unless CWG indicates it wants to change direction here I think we should continue to reject.

I agree -- are you okay if I change this around to drop support for the brace or init list for an anonymous bit-field as part of that consensus? If so, would you like me to ask Mike for a core issue # to track that change against?

[aaron.ballman]

I've updated the patch to continue to reject attributes on anonymous bit-fields, but with a better diagnostic. In addition, I changed how we handle anonymous bit-fields with an initializer (based on discussion on the Core reflector) -- instead of rejecting the construct as a semantic issue, I reject it at the parser level with a similar diagnostic (I went with "in-class initializer" because that's used by other parser diagnostics, but I'm fine with either formulation).

[rsmith]

clang/include/clang/Basic/DiagnosticParseKinds.td

875–876

Please retain the diagnostic wording using proper standard terminology; the other diagnostics say "in-class initializer" because they predate the existence of the standard terminology and haven't been fixed yet. (Fixing them -- and renaming the corresponding functions throughout Clang -- would be great if you feel so inclined.)

clang/lib/Parse/ParseDecl.cpp

4132–4134

I think this will regress our diagnostics for this (probably more common) case:

struct X { 
  int a, [[attr]] b;
};

Instead, how about we unconditionally DiagnoseAndSkipCXX11Attributes() before and after we parse GNU attributes in the if (!FirstDeclarator) check up above? (Aside: we should probably be better about handling mixed sequences of GNU and C++11 attributes in general.)

clang/lib/Parse/ParseDeclCXX.cpp

2311–2318

Please mention that this is a proposed bugfix, not the standard grammar.

2321–2323

(Same comment as for the C side of things.)

[aaron.ballman]

Updated based on review feedback.

clang/include/clang/Basic/DiagnosticParseKinds.td

875–876

Sounds good to me, and I can do the rest of the cleanup as an NFC commit once this lands.

clang/lib/Parse/ParseDecl.cpp

4132–4134

Good catch, I've made the changes. And I agree about attribute parsing order; I'd really like to take another stab at fixing that if I get the chance.

[rsmith]

Thanks!

clang/lib/Parse/ParseDecl.cpp

4126

I think this change is redundant now.

clang/lib/Parse/ParseDeclCXX.cpp

2317

Similarly here, I think the check for an attribute specifier is now redundant.

[aaron.ballman]

Thank you for the reviews, I've committed in 538762fef0b662048be2a261ebc12da249efa977

clang/lib/Parse/ParseDecl.cpp

4126

Ah, good catch! I've corrected this (and the other one).

clang/include/clang/Basic/DiagnosticParseKinds.td

def warn_cxx98_compat_nonstatic_member_init : Warning<
   "in-class initialization of non-static data members is incompatible with C++98">,
   InGroup<CXX98Compat>, DefaultIgnore;
 def ext_bitfield_member_init: ExtWarn<
   "default member initializer for bit-field is a C++20 extension">,
   InGroup<CXX20>;
 def warn_cxx17_compat_bitfield_member_init: Warning<
   "default member initializer for bit-field is incompatible with "
   "C++ standards before C++20">, InGroup<CXXPre20Compat>, DefaultIgnore;
+def err_anon_bitfield_member_init : Error<
+  "anonymous bit-field cannot have a default member initializer">;
 def err_incomplete_array_member_init: Error<
   "array bound cannot be deduced from an in-class initializer">;
 
 // C++11 alias-declaration
 def ext_alias_declaration : ExtWarn<
   "alias declarations are a C++11 extension">, InGroup<CXX11>;
 def warn_cxx98_compat_alias_declaration : Warning<
   "alias declarations are incompatible with C++98">,

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_anon_bitfield_has_negative_width : Error<
   "anonymous bit-field has negative width (%0)">;
 def err_bitfield_has_zero_width : Error<"named bit-field %0 has zero width">;
 def err_bitfield_width_exceeds_type_width : Error<
   "width of bit-field %0 (%1 bits) exceeds %select{width|size}2 "
   "of its type (%3 bit%s3)">;
 def err_anon_bitfield_width_exceeds_type_width : Error<
   "width of anonymous bit-field (%0 bits) exceeds %select{width|size}1 "
   "of its type (%2 bit%s2)">;
-def err_anon_bitfield_init : Error<
-  "anonymous bit-field cannot have a default member initializer">;
 def err_incorrect_number_of_vector_initializers : Error<
   "number of elements must be either one or match the size of the vector">;
 
 // Used by C++ which allows bit-fields that are wider than the type.
 def warn_bitfield_width_exceeds_type_width: Warning<
   "width of bit-field %0 (%1 bits) exceeds the width of its type; value will "
   "be truncated to %2 bit%s2">, InGroup<BitFieldWidth>;
 def warn_anon_bitfield_width_exceeds_type_width : Warning<

clang/lib/Parse/ParseDecl.cpp

void Parser::ParseStructDeclaration(
   // Read struct-declarators until we find the semicolon.
   bool FirstDeclarator = true;
   SourceLocation CommaLoc;
   while (1) {
     ParsingFieldDeclarator DeclaratorInfo(*this, DS);
     DeclaratorInfo.D.setCommaLoc(CommaLoc);
 
     // Attributes are only allowed here on successive declarators.
-    if (!FirstDeclarator)
+    if (!FirstDeclarator) {
+      // However, this does not apply for [[]] attributes (which could show up
+      // before or after the __attribute__ attributes).
+      DiagnoseAndSkipCXX11Attributes();
       MaybeParseGNUAttributes(DeclaratorInfo.D);
+      DiagnoseAndSkipCXX11Attributes();
+    }
 
     /// struct-declarator: declarator
     /// struct-declarator: declarator[opt] ':' constant-expression
-    if (Tok.isNot(tok::colon)) {
+    if (Tok.isNot(tok::colon) && !isCXX11AttributeSpecifier()) {
       // Don't parse FOO:BAR as if it were a typo for FOO::BAR.
       ColonProtectionRAIIObject X(*this);
       ParseDeclarator(DeclaratorInfo.D);
     } else
       DeclaratorInfo.D.SetIdentifier(nullptr, Tok.getLocation());
 
     if (TryConsumeToken(tok::colon)) {
       ExprResult Res(ParseConstantExpression());
       if (Res.isInvalid())
         SkipUntil(tok::semi, StopBeforeMatch);
       else
         DeclaratorInfo.BitfieldSize = Res.get();
     }
 
     // If attributes exist after the declarator, parse them.
     MaybeParseGNUAttributes(DeclaratorInfo.D);

clang/lib/Parse/ParseDeclCXX.cpp

 }
 
 /// Parse a C++ member-declarator up to, but not including, the optional
 /// brace-or-equal-initializer or pure-specifier.
 bool Parser::ParseCXXMemberDeclaratorBeforeInitializer(
     Declarator &DeclaratorInfo, VirtSpecifiers &VS, ExprResult &BitfieldSize,
     LateParsedAttrList &LateParsedAttrs) {
   // member-declarator:
-  //   declarator pure-specifier[opt]
+  //   declarator virt-specifier-seq[opt] pure-specifier[opt]
   //   declarator requires-clause
   //   declarator brace-or-equal-initializer[opt]
-  //   identifier[opt] ':' constant-expression
-  if (Tok.isNot(tok::colon))
+  //   identifier attribute-specifier-seq[opt] ':' constant-expression
+  //       brace-or-equal-initializer[opt]
+  //   ':' constant-expression
+  //
+  // NOTE: the latter two productions are a proposed bugfix rather than the
+  // current grammar rules as of C++20.
+  if (Tok.isNot(tok::colon) && !isCXX11AttributeSpecifier())
     ParseDeclarator(DeclaratorInfo);
   else
     DeclaratorInfo.SetIdentifier(nullptr, Tok.getLocation());
 
   if (!DeclaratorInfo.isFunctionDeclarator() && TryConsumeToken(tok::colon)) {
     assert(DeclaratorInfo.isPastIdentifier() &&
            "don't know where identifier would go yet?");
     BitfieldSize = ParseConstantExpression();
     if (BitfieldSize.isInvalid())
       SkipUntil(tok::comma, StopAtSemi | StopBeforeMatch);
   } else if (Tok.is(tok::kw_requires)) {
     ParseTrailingRequiresClause(DeclaratorInfo);
   } else {
     ParseOptionalCXX11VirtSpecifierSeq(
if (Tok.is(tok::kw_asm)) {
     if (AsmLabel.isInvalid())
       SkipUntil(tok::comma, StopAtSemi | StopBeforeMatch);
 
     DeclaratorInfo.setAsmLabel(AsmLabel.get());
     DeclaratorInfo.SetRangeEnd(Loc);
   }
 
   // If attributes exist after the declarator, but before an '{', parse them.
+  // However, this does not apply for [[]] attributes (which could show up
+  // before or after the __attribute__ attributes).
+  DiagnoseAndSkipCXX11Attributes();
   MaybeParseGNUAttributes(DeclaratorInfo, &LateParsedAttrs);
+  DiagnoseAndSkipCXX11Attributes();
 
   // For compatibility with code written to older Clang, also accept a
   // virt-specifier *after* the GNU attributes.
   if (BitfieldSize.isUnset() && VS.isUnset()) {
     ParseOptionalCXX11VirtSpecifierSeq(
         VS, getCurrentClass().IsInterface,
         DeclaratorInfo.getDeclSpec().getFriendSpecLoc());
     if (!VS.isUnset()) {
Parser::ParseCXXClassMemberDeclaration(AccessSpecifier AS,
   // member-declarator-list:
   //   member-declarator
   //   member-declarator-list ',' member-declarator
 
   while (1) {
     InClassInitStyle HasInClassInit = ICIS_NoInit;
     bool HasStaticInitializer = false;
     if (Tok.isOneOf(tok::equal, tok::l_brace) && PureSpecLoc.isInvalid()) {
-      if (DeclaratorInfo.isDeclarationOfFunction()) {
+      // DRXXXX: Anonymous bit-fields cannot have a brace-or-equal-initializer.
+      if (BitfieldSize.isUsable() && !DeclaratorInfo.hasName()) {
+        // Diagnose the error and pretend there is no in-class initializer.
+        Diag(Tok, diag::err_anon_bitfield_member_init);
+        SkipUntil(tok::comma, StopAtSemi | StopBeforeMatch);
+      } else if (DeclaratorInfo.isDeclarationOfFunction()) {
         // It's a pure-specifier.
         if (!TryConsumePureSpecifier(/*AllowFunctionDefinition*/ false))
           // Parse it as an expression so that Sema can diagnose it.
           HasStaticInitializer = true;
       } else if (DeclaratorInfo.getDeclSpec().getStorageClassSpec() !=
                      DeclSpec::SCS_static &&
                  DeclaratorInfo.getDeclSpec().getStorageClassSpec() !=
                      DeclSpec::SCS_typedef &&
while (1) {
     // Parse the next declarator.
     DeclaratorInfo.clear();
     VS.clear();
     BitfieldSize = ExprResult(/*Invalid=*/false);
     EqualLoc = PureSpecLoc = SourceLocation();
     DeclaratorInfo.setCommaLoc(CommaLoc);
 
     // GNU attributes are allowed before the second and subsequent declarator.
+    // However, this does not apply for [[]] attributes (which could show up
+    // before or after the __attribute__ attributes).
+    DiagnoseAndSkipCXX11Attributes();
     MaybeParseGNUAttributes(DeclaratorInfo);
+    DiagnoseAndSkipCXX11Attributes();
 
     if (ParseCXXMemberDeclaratorBeforeInitializer(
             DeclaratorInfo, VS, BitfieldSize, LateParsedAttrs))
       break;
   }
 
   if (ExpectSemi &&
       ExpectAndConsume(tok::semi, diag::err_expected_semi_decl_list)) {

clang/lib/Sema/SemaDecl.cpp

   if (BitWidth) {
     BitWidth = VerifyBitField(Loc, II, T, Record->isMsStruct(Context), BitWidth,
                               &ZeroWidth).get();
     if (!BitWidth) {
       InvalidDecl = true;
       BitWidth = nullptr;
       ZeroWidth = false;
     }
-
-    // Only data members can have in-class initializers.
-    if (BitWidth && !II && InitStyle) {
-      Diag(Loc, diag::err_anon_bitfield_init);
-      InvalidDecl = true;
-      BitWidth = nullptr;
-      ZeroWidth = false;
-    }
   }
 
   // Check that 'mutable' is consistent with the type of the declaration.
   if (!InvalidDecl && Mutable) {
     unsigned DiagID = 0;
     if (T->isReferenceType())
       DiagID = getLangOpts().MSVCCompat ? diag::ext_mutable_reference
                                         : diag::err_mutable_reference;

clang/test/CXX/class/class.bit/p1.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 %s
+
+// Test various bit-field member declarations.
+constexpr int foo() { return 1; }
+struct A {
+  int a [[]] : 1;
+  int b, [[]] : 0; // expected-error {{an attribute list cannot appear here}}
+  int [[]] : 0; // OK, attribute applies to the type.
+  int [[]] c : 1; // OK, attribute applies to the type.
+  int : 2 = 1; // expected-error {{anonymous bit-field cannot have a default member initializer}}
+  int : 0 { 1 }; // expected-error {{anonymous bit-field cannot have a default member initializer}}
+  int : 0, d : 1 = 1;
+  int : 1 = 12, e : 1; // expected-error {{anonymous bit-field cannot have a default member initializer}}
+  int : 0, f : 1 = 1;
+  int g [[]] : 1 = 1;
+  int h [[]] : 1 {1};
+  int i : foo() = foo();
+  int j, [[]] k; // expected-error {{an attribute list cannot appear here}}
+};

clang/test/Parser/MicrosoftExtensions.cpp

namespace enum_class {
   enum E {};
   struct S {
     enum E : int(1); // expected-error {{anonymous bit-field}}
     enum E : int : 1; // OK, bit-field
     enum F : int a = {}; // OK, default member initializer
     // MSVC produces a "C4353 constant 0 as function expression" for this,
     // considering the final {} to be part of the bit-width. We follow P0683R1
     // and treat it as a default member initializer.
-    enum E : int : int{}{}; // expected-error {{anonymous bit-field cannot have a default member initializer}} expected-warning {{C++20 extension}}
+    enum E : int : int{}{}; // expected-error {{anonymous bit-field cannot have a default member initializer}}
   };
 }

clang/test/Parser/c2x-attributes.c

 
 struct [[]] S1 {
   int i [[]];
   int [[]] j;
   int k[10] [[]];
   int l[[]][10];
   [[]] int m, n;
   int o [[]] : 12;
+  int [[]] : 0; // OK, attribute applies to the type.
+  int p, [[]] : 0; // expected-error {{an attribute list cannot appear here}}
+  int q, [[]] r; // expected-error {{an attribute list cannot appear here}}
 };
 
 [[]] struct S2 { int a; }; // expected-error {{misplaced attributes}}
 struct S3 [[]] { int a; }; // expected-error {{an attribute list cannot appear here}}
 
 union [[]] U {
   double d [[]];
   [[]] int i;