[clang] Fix new-expression with elaborated-type-specifier

[Fznamznon]

Summary

Expressions like

struct A {};
...
new struct A {};
struct A* b = (1 == 1) ? new struct A : new struct A;

Were parsed as redefinitions of struct A and failed, however as clarified by
CWG2141 new-expression cannot define a type, so both these examples
should be considered as references to the previously declared struct A.
The patch adds a "new" kind context for parsing declaration specifiers in
addition to already existing declarator context in order to track that
the parser is inside of a new expression.

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

[aaron.ballman]

Thank you for the fix! Just to clarify some things before diving into the review too much... From the patch summary:

Expressions like

new struct A {};
struct A* b = (1 == 1) ? new struct A : new struct A;
Were parsed as definitions of struct A and failed, however as clarified by
CWG2141 new-expression cannot define a type, so both these examples
should be considered as valid.

There's a typo there -- the new struct A{}; bit should be struct A {}; (dropping the new), right?

I think the root cause of that issue is that we don't implement DR2141 (https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#2141); are you intending to cover that DR? If so, there should be tests added to clang/test/CXX/drs/dr21xx.cpp.

clang/include/clang/Parse/Parser.h

2221

Adding the comma so the next person doesn't have to, and realigning to the usual indentation

[aaron.ballman]

In D153857#4453092, @aaron.ballman wrote:

Thank you for the fix! Just to clarify some things before diving into the review too much... From the patch summary:

Expressions like

new struct A {};
struct A* b = (1 == 1) ? new struct A : new struct A;
Were parsed as definitions of struct A and failed, however as clarified by
CWG2141 new-expression cannot define a type, so both these examples
should be considered as valid.

There's a typo there -- the new struct A{}; bit should be struct A {}; (dropping the new), right?

I think the root cause of that issue is that we don't implement DR2141 (https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#2141); are you intending to cover that DR? If so, there should be tests added to clang/test/CXX/drs/dr21xx.cpp.

With offline help from @erichkeane and @shafik I realized where my confusion came from here. The patch summary presumes struct A is already defined in order for us to accept those code examples. I was thrown off by the DR and the original issue using struct A {}; as the first relevant line in the code example and was thinking there was confusion as to how the DR was resolved.

How about the summary be changed to something along the lines of:

With code like struct A {}; <and the rest of your example>, the expressions were parsed as redefining struct A and failed. However, as clarified by CWG2141, new-expression cannot define a type, so both these expressions should be considered as valid references to the previously declared struct A.

[Fznamznon]

With code like struct A {}; <and the rest of your example>, the expressions were parsed as redefining struct A and failed. However, as clarified by CWG2141, new-expression cannot define a type, so both these expressions should be considered as valid references to the previously declared struct A.

Sure, thank you and sorry for the confusion! I should have added struct A {}; to the example in the first place.

I think the root cause of that issue is that we don't implement DR2141 (https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#2141); are you intending to cover that DR? If so, there should be tests added to clang/test/CXX/drs/dr21xx.cpp.

The original intent was to fix the bug because I wasn't confident enough that I'll be able to cover all possible cases by the tests. I could try go deeper and work to support DR2141. Will the simple examples that I'm already adding to clang/test/Parser/cxx11-type-specifier.cpp test be enough for the first iteration?

[shafik]

What about

template <typename T>
struct A { };

  void foo() {
    new struct A<int> {};
  }

I think this should be allowed, right? Note MSVC accepts it: https://godbolt.org/z/j7djYs8bq

[shafik]

MSVC rejects the first but accepts the second which I think is wrong but worth testing:

alignof(struct B {});
sizeof(struct B{});

https://godbolt.org/z/153jYqaPM

clang/include/clang/Parse/Parser.h

2359

Should this be below since DeclaratorContext::CXXNew was grouped w/ DSC_normal normal before?

[Fznamznon]

What about
template <typename T>
struct A { };

void foo() {
  new struct A<int> {};
}

I think this should be allowed, right?

Yes, I think so. The patch helps to allow. Thanks, I'll add it to the test.

MSVC rejects the first but accepts the second which I think is wrong but worth testing:

alignof(struct B {});
sizeof(struct B{});

These don't have new so they are not affected by the patch ATM.
But why MSVC wrong? Operand of alignof is a type-id which is not defining-type-id meaning it shouldn't be parsed as type definition. So, struct B{} there is a reference to struct B with braced initializer, i.e. object creation. That is not a type-id, right? On the opposite, sizeof can accept either type-id or an expression, I guess this is why MSVC accepts.

clang/include/clang/Parse/Parser.h

2359

Not really. Since there was no DeclSpecContext for new expression, it was mostly handled as if it was DSC_type_specifier.

[Fznamznon]

Add comma and test

[Fznamznon]

clang/include/clang/Parse/Parser.h

2221

Added comma, but the indentation is what clang-format gave me. I'm afraid I won't pass the pre-commit with different .

[shafik]

In D153857#4455480, @Fznamznon wrote:

MSVC rejects the first but accepts the second which I think is wrong but worth testing:

alignof(struct B {});
sizeof(struct B{});

These don't have new so they are not affected by the patch ATM.
But why MSVC wrong? Operand of alignof is a type-id which is not defining-type-id meaning it shouldn't be parsed as type definition. So, struct B{} there is a reference to struct B with braced initializer, i.e. object creation. That is not a type-id, right? On the opposite, sizeof can accept either type-id or an expression, I guess this is why MSVC accepts.

new is special b/c it is has new-initializer after the type-id so that can be an initialization: https://eel.is/c++draft/expr.new#nt:new-expression in sizeof it is a definition which is not allowed and so it should be rejected.

if you do:

struct B{ int x; };

void f() {
   struct B{};
}

That is a definition in f: https://godbolt.org/z/38eM6z17K

[Fznamznon]

Move test cases to CXX/drs/dr21xx.cpp, update DR status page, add alignof/sizeof
cases, rebase

[aaron.ballman]

LGTM, thank you!

clang/docs/ReleaseNotes.rst

556

clang/include/clang/Parse/Parser.h

2221

Ah, we don't care if pre-commit CI fails because of clang-format due to following the surrounding formatting style. That said, this formatting is already inconsistent (I hadn't noticed the first three are at a different indentation level as well), so this is fine.

clang/docs/ReleaseNotes.rst

- Fixed a failing assertion when implicitly defining a function within a GNU
   statement expression that appears outside of a function block scope. The
   assertion was benign outside of asserts builds and would only fire in C.
   (`#48579 <https://github.com/llvm/llvm-project/issues/48579>`_).
 - Fixed a failing assertion when applying an attribute to an anonymous union.
   The assertion was benign outside of asserts builds and would only fire in C++.
   (`#48512 <https://github.com/llvm/llvm-project/issues/48512>`_).
 - Fixed a failing assertion when parsing incomplete destructor.
   (`#63503 <https://github.com/llvm/llvm-project/issues/63503>`_)
 - Fix C++17 mode assert when parsing malformed code and the compiler is
   attempting to see if it could be type template for class template argument
   deduction. This fixes
   (`Issue 57495 <https://github.com/llvm/llvm-project/issues/57495>`_)
 - Fix missing destructor calls and therefore memory leaks in generated code
   when an immediate invocation appears as a part of an expression that produces
   temporaries.
   (`#60709 <https://github.com/llvm/llvm-project/issues/60709>`_).
 - Fixed a missed integer overflow warning with temporary values.
   (`#63629 <https://github.com/llvm/llvm-project/issues/63629>`_)
+- Fixed parsing of elaborated type specifier inside of a new expression.
+  (`#34341 <https://github.com/llvm/llvm-project/issues/34341>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed a bug where attribute annotations on type specifiers (enums, classes,
   structs, unions, and scoped enums) were not properly ignored, resulting in

clang/include/clang/Parse/Parser.h

enum class DeclSpecContext {
     DSC_conv_operator,      // C++ type-specifier-seq in an conversion operator
     DSC_top_level,          // top-level/namespace declaration context
     DSC_template_param,     // template parameter context
     DSC_template_arg,       // template argument context
     DSC_template_type_arg,  // template type argument context
     DSC_objc_method_result, // ObjC method result context, enables
                             // 'instancetype'
     DSC_condition,          // condition declaration context
-    DSC_association // A _Generic selection expression's type association
+    DSC_association, // A _Generic selection expression's type association
+    DSC_new,         // C++ new expression
   };
 
   /// Is this a context in which we are parsing just a type-specifier (or
   /// trailing-type-specifier)?
   static bool isTypeSpecifier(DeclSpecContext DSC) {
     switch (DSC) {
     case DeclSpecContext::DSC_normal:
     case DeclSpecContext::DSC_template_param:
     case DeclSpecContext::DSC_template_arg:
     case DeclSpecContext::DSC_class:
     case DeclSpecContext::DSC_top_level:
     case DeclSpecContext::DSC_objc_method_result:
     case DeclSpecContext::DSC_condition:
       return false;
 
     case DeclSpecContext::DSC_template_type_arg:
     case DeclSpecContext::DSC_type_specifier:
     case DeclSpecContext::DSC_conv_operator:
     case DeclSpecContext::DSC_trailing:
     case DeclSpecContext::DSC_alias_declaration:
     case DeclSpecContext::DSC_association:
+    case DeclSpecContext::DSC_new:
       return true;
     }
     llvm_unreachable("Missing DeclSpecContext case");
   }
 
   /// Whether a defining-type-specifier is permitted in a given context.
   enum class AllowDefiningTypeSpec {
     /// The grammar doesn't allow a defining-type-specifier here, and we must
isDefiningTypeSpecifierContext(DeclSpecContext DSC, bool IsCPlusPlus) {
 
     case DeclSpecContext::DSC_association:
       return IsCPlusPlus ? AllowDefiningTypeSpec::NoButErrorRecovery
                          : AllowDefiningTypeSpec::Yes;
 
     case DeclSpecContext::DSC_trailing:
     case DeclSpecContext::DSC_conv_operator:
     case DeclSpecContext::DSC_template_arg:
+    case DeclSpecContext::DSC_new:
       return AllowDefiningTypeSpec::No;
     }
     llvm_unreachable("Missing DeclSpecContext case");
   }
 
   /// Is this a context in which an opaque-enum-declaration can appear?
   static bool isOpaqueEnumDeclarationContext(DeclSpecContext DSC) {
     switch (DSC) {
     case DeclSpecContext::DSC_normal:
     case DeclSpecContext::DSC_class:
     case DeclSpecContext::DSC_top_level:
       return true;
 
     case DeclSpecContext::DSC_alias_declaration:
     case DeclSpecContext::DSC_objc_method_result:
     case DeclSpecContext::DSC_condition:
     case DeclSpecContext::DSC_template_param:
     case DeclSpecContext::DSC_template_type_arg:
     case DeclSpecContext::DSC_type_specifier:
     case DeclSpecContext::DSC_trailing:
     case DeclSpecContext::DSC_association:
     case DeclSpecContext::DSC_conv_operator:
     case DeclSpecContext::DSC_template_arg:
+    case DeclSpecContext::DSC_new:
 
       return false;
     }
     llvm_unreachable("Missing DeclSpecContext case");
   }
 
   /// Is this a context in which we can perform class template argument
   /// deduction?
   static bool isClassTemplateDeductionContext(DeclSpecContext DSC) {
     switch (DSC) {
     case DeclSpecContext::DSC_normal:
     case DeclSpecContext::DSC_template_param:
     case DeclSpecContext::DSC_template_arg:
     case DeclSpecContext::DSC_class:
     case DeclSpecContext::DSC_top_level:
     case DeclSpecContext::DSC_condition:
     case DeclSpecContext::DSC_type_specifier:
     case DeclSpecContext::DSC_association:
     case DeclSpecContext::DSC_conv_operator:
+    case DeclSpecContext::DSC_new:
       return true;
 
     case DeclSpecContext::DSC_objc_method_result:
     case DeclSpecContext::DSC_template_type_arg:
     case DeclSpecContext::DSC_trailing:
     case DeclSpecContext::DSC_alias_declaration:
       return false;
     }
     llvm_unreachable("Missing DeclSpecContext case");
   }
 
   // Is this a context in which an implicit 'typename' is allowed?
   static ImplicitTypenameContext
   getImplicitTypenameContext(DeclSpecContext DSC) {
     switch (DSC) {
     case DeclSpecContext::DSC_class:
     case DeclSpecContext::DSC_top_level:
     case DeclSpecContext::DSC_type_specifier:
     case DeclSpecContext::DSC_template_type_arg:
     case DeclSpecContext::DSC_trailing:
     case DeclSpecContext::DSC_alias_declaration:
     case DeclSpecContext::DSC_template_param:
+    case DeclSpecContext::DSC_new:
       return ImplicitTypenameContext::Yes;
 
     case DeclSpecContext::DSC_normal:
     case DeclSpecContext::DSC_objc_method_result:
     case DeclSpecContext::DSC_condition:
     case DeclSpecContext::DSC_template_arg:
     case DeclSpecContext::DSC_conv_operator:
     case DeclSpecContext::DSC_association:

clang/lib/Parse/ParseDecl.cpp

Parser::getDeclSpecContextFromDeclaratorContext(DeclaratorContext Context) {
   case DeclaratorContext::Association:
     return DeclSpecContext::DSC_association;
   case DeclaratorContext::TypeName:
     return DeclSpecContext::DSC_type_specifier;
   case DeclaratorContext::Condition:
     return DeclSpecContext::DSC_condition;
   case DeclaratorContext::ConversionId:
     return DeclSpecContext::DSC_conv_operator;
+  case DeclaratorContext::CXXNew:
+    return DeclSpecContext::DSC_new;
   case DeclaratorContext::Prototype:
   case DeclaratorContext::ObjCResult:
   case DeclaratorContext::ObjCParameter:
   case DeclaratorContext::KNRTypeList:
   case DeclaratorContext::FunctionalCast:
   case DeclaratorContext::Block:
   case DeclaratorContext::ForInit:
   case DeclaratorContext::SelectionInit:
-  case DeclaratorContext::CXXNew:
   case DeclaratorContext::CXXCatch:
   case DeclaratorContext::ObjCCatch:
   case DeclaratorContext::BlockLiteral:
   case DeclaratorContext::LambdaExpr:
   case DeclaratorContext::LambdaExprParameter:
   case DeclaratorContext::RequiresExpr:
     return DeclSpecContext::DSC_normal;
   }

clang/lib/Parse/ParseExprCXX.cpp

if (PlacementArgs.empty()) {
                                   &Parser::ParseDirectNewDeclarator);
         }
       }
     }
   } else {
     // A new-type-id is a simplified type-id, where essentially the
     // direct-declarator is replaced by a direct-new-declarator.
     MaybeParseGNUAttributes(DeclaratorInfo);
-    if (ParseCXXTypeSpecifierSeq(DS))
+    if (ParseCXXTypeSpecifierSeq(DS, DeclaratorContext::CXXNew))
       DeclaratorInfo.setInvalidType(true);
     else {
       DeclaratorInfo.SetSourceRange(DS.getSourceRange());
       ParseDeclaratorInternal(DeclaratorInfo,
                               &Parser::ParseDirectNewDeclarator);
     }
   }
   if (DeclaratorInfo.isInvalidType()) {

clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/p3-0x.cpp

void f() {
   int arr[3] = {1,2,3};
 
   for (struct S { S(int) {} } s : arr) { // expected-error {{types may not be defined in a for range declaration}}
   }
 
   for (struct S { S(int) {} } s : Undeclared); // expected-error{{types may not be defined in a for range declaration}}
                                                // expected-error@-1{{use of undeclared identifier 'Undeclared'}}
 
-  new struct T {}; // expected-error {{'T' cannot be defined in a type specifier}}
-  new struct A {}; // expected-error {{'A' cannot be defined in a type specifier}}
+  new struct T {}; // expected-error {{allocation of incomplete type 'struct T'}} //expected-note{{forward declaration of 'T'}}
+  new struct A {};
 
   try {} catch (struct U {}) {} // expected-error {{'U' cannot be defined in a type specifier}}
 
   (void)(struct V { V(int); })0; // expected-error {{'V' cannot be defined in a type specifier}}
 
   (void)dynamic_cast<struct W {}*>((K*)0); // expected-error {{'W' cannot be defined in a type specifier}}
   (void)static_cast<struct X {}*>(0); // expected-error {{'X' cannot be defined in a type specifier}}
   (void)reinterpret_cast<struct Y {}*>(0); // expected-error {{'Y' cannot be defined in a type specifier}}

clang/test/CXX/drs/dr19xx.cpp

 }
 
 namespace dr1966 { // dr1966: 11
 #if __cplusplus >= 201103L
   struct A {
     enum E : int {1}; // expected-error {{expected identifier}} (not bit-field)
   };
   auto *p1 = new enum E : int; // expected-error {{only permitted as a standalone declaration}}
-  auto *p2 = new enum F : int {}; // expected-error {{cannot be defined in a type specifier}}
+  auto *p2 = new enum F : int {}; // expected-error {{only permitted as a standalone declaration}}
   auto *p3 = true ? new enum G : int {}; // expected-error {{forward reference}} expected-error {{incomplete}} expected-note {{declaration}}
   auto h() -> enum E : int {}; // expected-error {{only permitted as a standalone declaration}}
 
   enum X : enum Y : int {} {}; // expected-error {{cannot be defined in a type specifier}}
   struct Q {
     enum X : enum Y : int {} {}; // expected-error +{{}}
   };
 #endif

clang/test/CXX/drs/dr21xx.cpp

#if __cplusplus >= 201103L
   union U { int a; decltype(nullptr) b; };
   constexpr int *test(U u) {
     return u.b;
   }
   static_assert(!test({123}), "u.b should be valid even when b is inactive");
 #endif
 }
 
+namespace dr2141 { // dr2141: 17
+struct A{};
+
+template <typename T>
+struct B{};
+
+void foo() {
+  struct A *b = (1 == 1) ? new struct A : new struct A;
+  struct S *a = (1 == 1) ? new struct S : new struct S; // expected-error 2{{allocation of incomplete type}} // expected-note 2{{forward}}
+
+#if __cplusplus >= 201103L
+  A *aa = new struct A{};
+  B<int> *bb = new struct B<int>{};
+  (void)new struct C{}; // expected-error {{allocation of incomplete type }} // expected-note {{forward}}
+
+  struct A *c = (1 == 1) ? new struct A {} : new struct A {};
+
+  alignof(struct D{}); // expected-error {{cannot be defined in a type specifier}}
+#endif
+
+  sizeof(struct E{}); // expected-error {{cannot be defined in a type specifier}}
+
+}
+}
+
 namespace dr2157 { // dr2157: 11
 #if __cplusplus >= 201103L
   enum E : int;
   struct X {
     enum dr2157::E : int(); // expected-error {{only allows ':' in member enumeration declaration to introduce a fixed underlying type}}
   };
 #endif
 }

clang/test/CXX/drs/dr6xx.cpp

void f() {
     (void)static_cast<struct C*>(0);
     (void)static_cast<struct C{}*>(0); // expected-error {{cannot be defined in a type specifier}}
     (void)reinterpret_cast<struct D*>(0);
     (void)reinterpret_cast<struct D{}*>(0); // expected-error {{cannot be defined in a type specifier}}
     (void)const_cast<struct E*>(0); // expected-error {{not allowed}}
     (void)const_cast<struct E{}*>(0); // expected-error {{cannot be defined in a type specifier}}
     (void)sizeof(struct F*);
     (void)sizeof(struct F{}*); // expected-error {{cannot be defined in a type specifier}}
-    (void)new struct G*;
-    (void)new struct G{}*; // expected-error {{cannot be defined in a type specifier}}
+    (void)new struct G*; // expected-note {{forward}}
+    (void)new struct G{}*; // expected-error {{incomplete}}
 #if __cplusplus >= 201103L
+    // expected-error@-2 {{expected expression}}
     (void)alignof(struct H*);
     (void)alignof(struct H{}*); // expected-error {{cannot be defined in a type specifier}}
 #endif
     (void)(struct I*)0;
     (void)(struct I{}*)0; // expected-error {{cannot be defined in a type specifier}}
     if (struct J *p = 0) {}
     if (struct J {} *p = 0) {} // expected-error {{cannot be defined in a condition}}
     for (struct K *p = 0; struct L *q = 0; ) {}

clang/test/Parser/cxx11-type-specifier.cpp

 // RUN: %clang_cc1 -verify %s -std=c++11 -fcxx-exceptions
 
 // Tests for parsing of type-specifier-seq
 
 struct S {
   operator constexpr int(); // expected-error{{type name does not allow constexpr}}
 };
 enum E { e };
 
 void f() {
   try {
     (void) new constexpr int; // expected-error{{type name does not allow constexpr}}
   } catch (constexpr int) { // expected-error{{type name does not allow constexpr}}
   }
 
-  // These parse as type definitions, not as type references with braced
-  // initializers. Sad but true...
-  (void) new struct S {}; // expected-error{{'S' cannot be defined in a type specifier}}
-  (void) new enum E { e }; // expected-error{{'E' cannot be defined in a type specifier}}
+  (void) new struct S {};
+  (void) new enum E { e };
 }
 
 // And for trailing-type-specifier-seq
 
 auto f() -> unknown; // expected-error{{unknown type name 'unknown'}}

clang/www/cxx_dr_status.html

<tr id="2140">
     <td>CD4</td>
     <td>Lvalue-to-rvalue conversion of <TT>std::nullptr_t</TT></td>
     <td class="full" align="center">Clang 9</td>
   </tr>
   <tr id="2141">
     <td><a href="https://cplusplus.github.io/CWG/issues/2141.html">2141</a></td>
     <td>CD4</td>
     <td>Ambiguity in <I>new-expression</I> with <I>elaborated-type-specifier</I></td>
-    <td class="none" align="center">Unknown</td>
+    <td class="unreleased" align="center">Clang 17</td>
   </tr>
   <tr id="2142">
     <td><a href="https://cplusplus.github.io/CWG/issues/2142.html">2142</a></td>
     <td>NAD</td>
     <td>Missing definition of associated classes and namespaces</td>
     <td class="none" align="center">Unknown</td>
   </tr>
   <tr id="2143">