[clang] 13a86c2 - [Sema] Preserve invalid CXXCtorInitializers using RecoveryExpr in initializer
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 10 06:21:53 PDT 2021
Author: Sam McCall
Date: 2021-08-10T15:16:52+02:00
New Revision: 13a86c2bb465edf315ecbccccac622d73d39abe7
URL: https://github.com/llvm/llvm-project/commit/13a86c2bb465edf315ecbccccac622d73d39abe7
DIFF: https://github.com/llvm/llvm-project/commit/13a86c2bb465edf315ecbccccac622d73d39abe7.diff
LOG: [Sema] Preserve invalid CXXCtorInitializers using RecoveryExpr in initializer
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)
Differential Revision: https://reviews.llvm.org/D101641
Added:
Modified:
clang/lib/Sema/SemaDeclCXX.cpp
clang/test/AST/ast-dump-recovery.cpp
clang/test/CXX/drs/dr6xx.cpp
clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
Removed:
################################################################################
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 708b5cd8cb76d..112722be2fa1e 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -4162,7 +4162,8 @@ Sema::BuildMemInitializer(Decl *ConstructorD,
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();
@@ -4375,18 +4376,25 @@ Sema::BuildMemberInitializer(ValueDecl *Member, Expr *Init,
InitializationSequence InitSeq(*this, MemberEntity, Kind, Args);
ExprResult MemberInit = InitSeq.Perform(*this, MemberEntity, Kind, Args,
nullptr);
- if (MemberInit.isInvalid())
- return true;
-
- // 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;
-
- Init = MemberInit.get();
+ 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()) {
+ // 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) {
@@ -4428,29 +4436,35 @@ Sema::BuildDelegatingInitializer(TypeSourceInfo *TInfo, Expr *Init,
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() &&
- "Delegating constructor with no target?");
+ 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())
- return true;
+ // 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 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;
+ 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>(),
@@ -4474,7 +4488,12 @@ Sema::BuildBaseInitializer(QualType BaseType, TypeSourceInfo *BaseTInfo,
// 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()) {
@@ -4561,26 +4580,30 @@ Sema::BuildBaseInitializer(QualType BaseType, TypeSourceInfo *BaseTInfo,
InitRange.getEnd());
InitializationSequence InitSeq(*this, BaseEntity, Kind, Args);
ExprResult BaseInit = InitSeq.Perform(*this, BaseEntity, Kind, Args, nullptr);
- if (BaseInit.isInvalid())
- return true;
-
- // 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())
- 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 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;
+ 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(),
diff --git a/clang/test/AST/ast-dump-recovery.cpp b/clang/test/AST/ast-dump-recovery.cpp
index b6d7ac1d0a8e2..5ea9c08519695 100644
--- a/clang/test/AST/ast-dump-recovery.cpp
+++ b/clang/test/AST/ast-dump-recovery.cpp
@@ -296,3 +296,58 @@ void InvalidCondition() {
// 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>'
+ };
+}
diff --git a/clang/test/CXX/drs/dr6xx.cpp b/clang/test/CXX/drs/dr6xx.cpp
index 1ec723b04b9af..e32d6b2afb131 100644
--- a/clang/test/CXX/drs/dr6xx.cpp
+++ b/clang/test/CXX/drs/dr6xx.cpp
@@ -606,11 +606,13 @@ namespace dr654 { // dr654: sup 1423
namespace dr655 { // dr655: yes
struct A { A(int); }; // expected-note 2-3{{not viable}}
+ // expected-note at -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 at -1 {{must explicitly initialize the member 'a'}}
};
}
diff --git a/clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp b/clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
index da9895ca6d22c..2aa7dbf8a7633 100644
--- a/clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
+++ b/clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
@@ -102,7 +102,9 @@ struct HasMixins : public Mixins... {
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 { };
@@ -123,7 +125,9 @@ 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 at -1 {{no matching constructor for initialization of 'A'}}
+// expected-error at -2 {{no matching constructor for initialization of 'B'}}
void test_has_mixins() {
HasMixins<A, B> ab;
More information about the cfe-commits
mailing list