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 ↗

(On Diff #294554)

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

4127–4129 ↗

(On Diff #294554)

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–2313

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

2316–2318

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

[aaron.ballman]

Updated based on review feedback.

clang/include/clang/Basic/DiagnosticParseKinds.td

875–876 ↗

(On Diff #294554)

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

4127–4129 ↗

(On Diff #294554)

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 ↗

(On Diff #294729)

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 ↗

(On Diff #294729)

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

clang/lib/Parse/ParseDeclCXX.cpp

 /// 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 requires-clause
   //   declarator brace-or-equal-initializer[opt]
-  //   identifier[opt] ':' constant-expression
-  if (Tok.isNot(tok::colon))
+  //   identifier[opt] attribute-specifier-seq[opt] ':' constant-expression
+  if (Tok.isNot(tok::colon) && !isCXX11AttributeSpecifier())
     ParseDeclarator(DeclaratorInfo);
-  else
+  else {
     DeclaratorInfo.SetIdentifier(nullptr, Tok.getLocation());
+    MaybeParseCXX11Attributes(DeclaratorInfo);
+  }
 
   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);

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

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 %s
+// expected-no-diagnostics
+
+constexpr int foo() { return 1; }
+
+struct A {
+  int a [[]] : 1;
+  int x, [[]] : 0;
+  int [[]] : 0;
+  int b [[]] : 1 = 1;
+  int c [[]] : 1 {1};
+  int d : foo() = foo();
+};
+