[Sema] Preserve invalid CXXCtorInitializers using RecoveryExpr in initializer

[sammccall]

Summary

Before this patch, CXXCtorInitializers that don't typecheck get discarded in
most cases. In particular:

• typos that can't be corrected don't turn into RecoveryExpr. The full expr disappears instead, and without an init expr we discard the node.
• initializers that fail initialization (e.g. constructor overload resolution) are discarded too.

This patch addresses both these issues (a bit clunkily and repetitively, for
member/base/delegating initializers)

It does not preserve any AST nodes when the member/base can't be resolved or
other problems of that nature. That breaks invariants of CXXCtorInitializer
itself, and we don't have a "weak" RecoveryCtorInitializer like we do for Expr.

I believe the changes to diagnostics in existing tests are improvements.
(We're able to do some analysis on the non-broken parts of the initializer)

[hokein]

thanks, this code looks pretty solid, just a question.

clang/lib/Sema/SemaDeclCXX.cpp

4491–4496

it is unclear to me why we need a isDependentContext() here? looks like this would special-case RecoveryExpr, the following dependent code-path would not be executed for RecoveryExpr case?

[sammccall]

Rebase and add clarifying comment

[sammccall]

Doh, I forgot this never landed :-(

clang/lib/Sema/SemaDeclCXX.cpp

4491–4496

Right, if the init expr is type-dependent because it contains errors, but we're not actually in a template, we don't want to take the dependent path.

The reason is that the dependent path produces an "as-written" form of the CXXCtorInitializer, which has weaker invariants, because we know we're not going to examine it much until template instantiation time.
e.g. an initializer that is actually delegating is stored as a base initializer instead. (Thus avoiding the impossible requirement to check whether two dependent types are the same).
Later on in Sema::SetCtorInitializers, we check Constructor->isDependentContext() to decide whether to propagate the "as-written" initializers without checking, but of course this is false. So then we go and shuffle the initializers into the correct order, build default initializers for the uninitialized members etc, and the delegating initializer is just... lost in the shuffle. We think it's a base init, so it's put in a map of type->init, and then it never gets consumed from there because the type is not actually a base class.

I'll try to add a comment that hints at this, without getting into the concrete details...

[hokein]

clang/lib/Sema/SemaDeclCXX.cpp

4491–4496

I see, fair enough, thanks for the classifications.

clang/test/AST/ast-dump-recovery.cpp

305

For completeness, I would add case 'x(undefine())', even though it already works without this patch.

clang/lib/Sema/SemaDeclCXX.cpp

Sema::BuildMemInitializer(Decl *ConstructorD,
                           Scope *S,
                           CXXScopeSpec &SS,
                           IdentifierInfo *MemberOrBase,
                           ParsedType TemplateTypeTy,
                           const DeclSpec &DS,
                           SourceLocation IdLoc,
                           Expr *Init,
                           SourceLocation EllipsisLoc) {
-  ExprResult Res = CorrectDelayedTyposInExpr(Init);
+  ExprResult Res = CorrectDelayedTyposInExpr(Init, /*InitDecl=*/nullptr,
+                                             /*RecoverUncorrectedTypos=*/true);
   if (!Res.isUsable())
     return true;
   Init = Res.get();
 
   if (!ConstructorD)
     return true;
 
   AdjustDeclIfTemplate(ConstructorD);
InitializationKind Kind =
         InitList ? InitializationKind::CreateDirectList(
                        IdLoc, Init->getBeginLoc(), Init->getEndLoc())
                  : InitializationKind::CreateDirect(IdLoc, InitRange.getBegin(),
                                                     InitRange.getEnd());
 
     InitializationSequence InitSeq(*this, MemberEntity, Kind, Args);
     ExprResult MemberInit = InitSeq.Perform(*this, MemberEntity, Kind, Args,
                                             nullptr);
-    if (MemberInit.isInvalid())
-      return true;
-
+    if (!MemberInit.isInvalid()) {
       // C++11 [class.base.init]p7:
       //   The initialization of each base and member constitutes a
       //   full-expression.
       MemberInit = ActOnFinishFullExpr(MemberInit.get(), InitRange.getBegin(),
                                        /*DiscardedValue*/ false);
-    if (MemberInit.isInvalid())
-      return true;
+    }
 
+    if (MemberInit.isInvalid()) {
+      // Args were sensible expressions but we couldn't initialize the member
+      // from them. Preserve them in a RecoveryExpr instead.
+      Init = CreateRecoveryExpr(InitRange.getBegin(), InitRange.getEnd(), Args,
+                                Member->getType())
+                 .get();
+      if (!Init)
+        return true;
+    } else {
       Init = MemberInit.get();
     }
+  }
 
   if (DirectMember) {
     return new (Context) CXXCtorInitializer(Context, DirectMember, IdLoc,
                                             InitRange.getBegin(), Init,
                                             InitRange.getEnd());
   } else {
     return new (Context) CXXCtorInitializer(Context, IndirectMember, IdLoc,
                                             InitRange.getBegin(), Init,
Sema::BuildDelegatingInitializer(TypeSourceInfo *TInfo, Expr *Init,
   InitializationKind Kind =
       InitList ? InitializationKind::CreateDirectList(
                      NameLoc, Init->getBeginLoc(), Init->getEndLoc())
                : InitializationKind::CreateDirect(NameLoc, InitRange.getBegin(),
                                                   InitRange.getEnd());
   InitializationSequence InitSeq(*this, DelegationEntity, Kind, Args);
   ExprResult DelegationInit = InitSeq.Perform(*this, DelegationEntity, Kind,
                                               Args, nullptr);
-  if (DelegationInit.isInvalid())
-    return true;
-
-  assert(cast<CXXConstructExpr>(DelegationInit.get())->getConstructor() &&
+  if (!DelegationInit.isInvalid()) {
+    assert(DelegationInit.get()->containsErrors() ||
+           cast<CXXConstructExpr>(DelegationInit.get())->getConstructor() &&
                "Delegating constructor with no target?");
 
     // C++11 [class.base.init]p7:
     //   The initialization of each base and member constitutes a
     //   full-expression.
     DelegationInit = ActOnFinishFullExpr(
         DelegationInit.get(), InitRange.getBegin(), /*DiscardedValue*/ false);
+  }
+
+  if (DelegationInit.isInvalid()) {
+    DelegationInit =
+        CreateRecoveryExpr(InitRange.getBegin(), InitRange.getEnd(), Args,
+                           QualType(ClassDecl->getTypeForDecl(), 0));
     if (DelegationInit.isInvalid())
       return true;
-
+  } else {
     // If we are in a dependent context, template instantiation will
     // perform this type-checking again. Just save the arguments that we
     // received in a ParenListExpr.
     // FIXME: This isn't quite ideal, since our ASTs don't capture all
     // of the information that we have about the base
     // initializer. However, deconstructing the ASTs is a dicey process,
     // and this approach is far more likely to get the corner cases right.
     if (CurContext->isDependentContext())
       DelegationInit = Init;
+  }
 
   return new (Context) CXXCtorInitializer(Context, TInfo, InitRange.getBegin(),
                                           DelegationInit.getAs<Expr>(),
                                           InitRange.getEnd());
 }
 
 MemInitResult
 Sema::BuildBaseInitializer(QualType BaseType, TypeSourceInfo *BaseTInfo,
                            Expr *Init, CXXRecordDecl *ClassDecl,
                            SourceLocation EllipsisLoc) {
   SourceLocation BaseLoc
     = BaseTInfo->getTypeLoc().getLocalSourceRange().getBegin();
 
   if (!BaseType->isDependentType() && !BaseType->isRecordType())
     return Diag(BaseLoc, diag::err_base_init_does_not_name_class)
              << BaseType << BaseTInfo->getTypeLoc().getLocalSourceRange();
 
   // C++ [class.base.init]p2:
   //   [...] Unless the mem-initializer-id names a nonstatic data
   //   member of the constructor's class or a direct or virtual base
   //   of that class, the mem-initializer is ill-formed. A
   //   mem-initializer-list can initialize a base class using any
   //   name that denotes that base class type.
-  bool Dependent = BaseType->isDependentType() || Init->isTypeDependent();
+
+  // We can store the initializers in "as-written" form and delay analysis until
+  // instantiation if the constructor is dependent. But not for dependent
+  // (broken) code in a non-template! SetCtorInitializers does not expect this.
+  bool Dependent = CurContext->isDependentContext() &&
+                   (BaseType->isDependentType() || Init->isTypeDependent());
 
   SourceRange InitRange = Init->getSourceRange();
   if (EllipsisLoc.isValid()) {
     // This is a pack expansion.
     if (!BaseType->containsUnexpandedParameterPack())  {
       Diag(EllipsisLoc, diag::err_pack_expansion_without_parameter_packs)
         << SourceRange(BaseLoc, InitRange.getEnd());
 
Sema::BuildBaseInitializer(QualType BaseType, TypeSourceInfo *BaseTInfo,
   InitializedEntity BaseEntity =
     InitializedEntity::InitializeBase(Context, BaseSpec, VirtualBaseSpec);
   InitializationKind Kind =
       InitList ? InitializationKind::CreateDirectList(BaseLoc)
                : InitializationKind::CreateDirect(BaseLoc, InitRange.getBegin(),
                                                   InitRange.getEnd());
   InitializationSequence InitSeq(*this, BaseEntity, Kind, Args);
   ExprResult BaseInit = InitSeq.Perform(*this, BaseEntity, Kind, Args, nullptr);
-  if (BaseInit.isInvalid())
-    return true;
-
+  if (!BaseInit.isInvalid()) {
     // C++11 [class.base.init]p7:
     //   The initialization of each base and member constitutes a
     //   full-expression.
     BaseInit = ActOnFinishFullExpr(BaseInit.get(), InitRange.getBegin(),
                                    /*DiscardedValue*/ false);
+  }
+
+  if (BaseInit.isInvalid()) {
+    BaseInit = CreateRecoveryExpr(InitRange.getBegin(), InitRange.getEnd(),
+                                  Args, BaseType);
     if (BaseInit.isInvalid())
       return true;
-
+  } else {
     // If we are in a dependent context, template instantiation will
     // perform this type-checking again. Just save the arguments that we
     // received in a ParenListExpr.
     // FIXME: This isn't quite ideal, since our ASTs don't capture all
     // of the information that we have about the base
     // initializer. However, deconstructing the ASTs is a dicey process,
     // and this approach is far more likely to get the corner cases right.
     if (CurContext->isDependentContext())
       BaseInit = Init;
+  }
 
   return new (Context) CXXCtorInitializer(Context, BaseTInfo,
                                           BaseSpec->isVirtual(),
                                           InitRange.getBegin(),
                                           BaseInit.getAs<Expr>(),
                                           InitRange.getEnd(), EllipsisLoc);
 }
 

clang/test/AST/ast-dump-recovery.cpp

void InvalidCondition() {
   // FIXME: figure out why the type of ConditionalOperator is not int.
   // CHECK:      ConditionalOperator {{.*}} '<dependent type>' contains-errors
   // CHECK-NEXT: |-RecoveryExpr {{.*}} '<dependent type>' contains-errors
   // CHECK-NEXT: | `-UnresolvedLookupExpr {{.*}}
   // CHECK-NEXT: |-IntegerLiteral {{.*}} 'int' 1
   // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 2
   invalid() ? 1 : 2;
 }
+
+void CtorInitializer() {
+  struct S{int m};
+  class MemberInit {
+    int x, y, z;
+    S s;
+    MemberInit() : x(invalid), y(invalid, invalid), z(invalid()), s(1,2) {}
+    // CHECK:      CXXConstructorDecl {{.*}} MemberInit 'void ()'
+    // CHECK-NEXT: |-CXXCtorInitializer Field {{.*}} 'x' 'int'
+    // CHECK-NEXT: | `-ParenListExpr
+    // CHECK-NEXT: |   `-RecoveryExpr {{.*}} '<dependent type>'
+    // CHECK-NEXT: |-CXXCtorInitializer Field {{.*}} 'y' 'int'
+    // CHECK-NEXT: | `-ParenListExpr
+    // CHECK-NEXT: |   |-RecoveryExpr {{.*}} '<dependent type>'
+    // CHECK-NEXT: |   `-RecoveryExpr {{.*}} '<dependent type>'
+    // CHECK-NEXT: |-CXXCtorInitializer Field {{.*}} 'z' 'int'
+    // CHECK-NEXT: | `-ParenListExpr
+    // CHECK-NEXT: |   `-RecoveryExpr {{.*}} '<dependent type>'
+    // CHECK-NEXT: |     `-UnresolvedLookupExpr {{.*}} '<overloaded function type>'
+    // CHECK-NEXT: |-CXXCtorInitializer Field {{.*}} 's' 'S'
+    // CHECK-NEXT: | `-RecoveryExpr {{.*}} 'S' contains-errors
+    // CHECK-NEXT: |   |-IntegerLiteral {{.*}} 1
+    // CHECK-NEXT: |   `-IntegerLiteral {{.*}} 2
+  };
+  class BaseInit : S {
+    BaseInit(float) : S("no match") {}
+    // CHECK:      CXXConstructorDecl {{.*}} BaseInit 'void (float)'
+    // CHECK-NEXT: |-ParmVarDecl
+    // CHECK-NEXT: |-CXXCtorInitializer 'S'
+    // CHECK-NEXT: | `-RecoveryExpr {{.*}} 'S'
+    // CHECK-NEXT: |   `-StringLiteral
+
+    BaseInit(double) : S(invalid) {}
+    // CHECK:      CXXConstructorDecl {{.*}} BaseInit 'void (double)'
+    // CHECK-NEXT: |-ParmVarDecl
+    // CHECK-NEXT: |-CXXCtorInitializer 'S'
+    // CHECK-NEXT: | `-ParenListExpr
+    // CHECK-NEXT: |   `-RecoveryExpr {{.*}} '<dependent type>'
+  };
+  class DelegatingInit {
+    DelegatingInit(float) : DelegatingInit("no match") {}
+    // CHECK:      CXXConstructorDecl {{.*}} DelegatingInit 'void (float)'
+    // CHECK-NEXT: |-ParmVarDecl
+    // CHECK-NEXT: |-CXXCtorInitializer 'DelegatingInit'
+    // CHECK-NEXT: | `-RecoveryExpr {{.*}} 'DelegatingInit'
+    // CHECK-NEXT: |   `-StringLiteral
+
+    DelegatingInit(double) : DelegatingInit(invalid) {}
+    // CHECK:      CXXConstructorDecl {{.*}} DelegatingInit 'void (double)'
+    // CHECK-NEXT: |-ParmVarDecl
+    // CHECK-NEXT: |-CXXCtorInitializer 'DelegatingInit'
+    // CHECK-NEXT: | `-ParenListExpr
+    // CHECK-NEXT: |   `-RecoveryExpr {{.*}} '<dependent type>'
+  };
+}

clang/test/CXX/drs/dr6xx.cpp

void f() {
     void(true ? nullptr : 0);
     void(true ? 0 : nullptr);
   }
 }
 #endif
 
 namespace dr655 { // dr655: yes
   struct A { A(int); }; // expected-note 2-3{{not viable}}
+                        // expected-note@-1 {{'dr655::A' declared here}}
   struct B : A {
-    A a;
+    A a; // expected-note {{member is declared here}}
     B();
     B(int) : B() {} // expected-error 0-1 {{C++11}}
     B(int*) : A() {} // expected-error {{no matching constructor}}
+                     // expected-error@-1 {{must explicitly initialize the member 'a'}}
   };
 }
 
 namespace dr656 { // dr656: yes
   struct A { A(const A&) = delete; }; // expected-error 0-1 {{C++11}}
   struct B : A {};
   struct X { operator B(); } x;
   const A &r = x;

clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp

struct HasMixins : public Mixins... {
   HasMixins();
   HasMixins(const HasMixins&);
   HasMixins(int i);
 };
 
 struct A { }; // expected-note{{candidate constructor (the implicit copy constructor) not viable: no known conversion from 'int' to 'const A' for 1st argument}} \
 // expected-note{{candidate constructor (the implicit move constructor) not viable: no known conversion from 'int' to 'A' for 1st argument}} \
 // expected-note{{candidate constructor (the implicit default constructor) not viable: requires 0 arguments, but 1 was provided}}
-struct B { };
+struct B { }; // expected-note{{candidate constructor (the implicit copy constructor) not viable: no known conversion from 'int' to 'const B' for 1st argument}} \
+// expected-note{{candidate constructor (the implicit move constructor) not viable: no known conversion from 'int' to 'B' for 1st argument}} \
+// expected-note{{candidate constructor (the implicit default constructor) not viable: requires 0 arguments, but 1 was provided}}
 struct C { };
 struct D { };
 
 A *checkA = new HasMixins<A, B, C, D>;
 B *checkB = new HasMixins<A, B, C, D>;
 D *checkD = new HasMixins<A, B, C, D>;
 C *checkC = new HasMixins<A, B, D>; // expected-error{{cannot initialize a variable of type 'C *' with an rvalue of type 'HasMixins<A, B, D> *'}}
 HasMixins<> *checkNone = new HasMixins<>;
 
 template<typename Mixins>
 struct BrokenMixins : public Mixins... { }; // expected-error{{pack expansion does not contain any unexpanded parameter packs}}
 
 // In a mem-initializer-list (12.6.2); the pattern is a mem-initializer.
 template<typename ...Mixins>
 HasMixins<Mixins...>::HasMixins(): Mixins()... { }
 
 template<typename ...Mixins>
 HasMixins<Mixins...>::HasMixins(const HasMixins &other): Mixins(other)... { }
 
 template<typename ...Mixins>
-HasMixins<Mixins...>::HasMixins(int i): Mixins(i)... { } // expected-error{{no matching constructor for initialization of 'A'}}
+HasMixins<Mixins...>::HasMixins(int i): Mixins(i)... { }
+// expected-error@-1 {{no matching constructor for initialization of 'A'}}
+// expected-error@-2 {{no matching constructor for initialization of 'B'}}
 
 void test_has_mixins() {
   HasMixins<A, B> ab;
   HasMixins<A, B> ab2 = ab;
   HasMixins<A, B> ab3(17); // expected-note{{in instantiation of member function 'HasMixins<A, B>::HasMixins' requested here}}
 }
 
 template<typename T>