From 575e6c8350368d9e7c78e4f52eae9de593c177cf Mon Sep 17 00:00:00 2001
From: Richard Smith <richard-llvm@metafoo.co.uk>
Date: Thu, 6 Oct 2016 23:12:58 +0000
Subject: [PATCH] PR25890: Fix incoherent error handling in
PerformImplicitConversion and CheckSingleAssignmentConstraints. These no
longer produce ExprError() when they have not emitted an error, and reliably
inform the caller when they *have* emitted an error.
This fixes some serious issues where we would fail to emit any diagnostic for
invalid code and then attempt to emit code for an invalid AST, and conversely
some issues where we would emit two diagnostics for the same problem.
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@283508 91177308-0d34-0410-b5e6-96231b3b80d8
---
include/clang/Sema/Sema.h | 26 ++++++++++++-------
lib/Sema/SemaExpr.cpp | 16 +++++++-----
lib/Sema/SemaExprCXX.cpp | 4 +++
lib/Sema/SemaOverload.cpp | 3 ++-
lib/Sema/SemaPseudoObject.cpp | 3 ++-
lib/Sema/SemaStmt.cpp | 2 ++
test/CXX/drs/dr0xx.cpp | 5 ++--
test/CXX/except/except.spec/p5-pointers.cpp | 12 ++++-----
.../ambig-user-defined-conversions.cpp | 5 ++++
test/SemaCXX/derived-to-base-ambig.cpp | 4 +--
10 files changed, 51 insertions(+), 29 deletions(-)
diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index f7c1313d8f1..476631d7a8f 100644
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -8835,15 +8835,23 @@ public:
CastKind &Kind,
bool ConvertRHS = true);
- // CheckSingleAssignmentConstraints - Currently used by
- // CheckAssignmentOperands, and ActOnReturnStmt. Prior to type checking,
- // this routine performs the default function/array converions, if ConvertRHS
- // is true.
- AssignConvertType CheckSingleAssignmentConstraints(QualType LHSType,
- ExprResult &RHS,
- bool Diagnose = true,
- bool DiagnoseCFAudited = false,
- bool ConvertRHS = true);
+ /// Check assignment constraints for an assignment of RHS to LHSType.
+ ///
+ /// \brief LHSType The destination type for the assignment.
+ /// \brief RHS The source expression for the assignment.
+ /// \brief Diagnose If \c true, diagnostics may be produced when checking
+ /// for assignability. If a diagnostic is produced, \p RHS will be
+ /// set to ExprError(). Note that this function may still return
+ /// without producing a diagnostic, even for an invalid assignment.
+ /// \brief DiagnoseCFAudited If \c true, the target is a function parameter
+ /// in an audited Core Foundation API and does not need to be checked
+ /// for ARC retain issues.
+ /// \brief ConvertRHS If \c true, \p RHS will be updated to model the
+ /// conversions necessary to perform the assignment. If \c false,
+ /// \p Diagnose must also be \c false.
+ AssignConvertType CheckSingleAssignmentConstraints(
+ QualType LHSType, ExprResult &RHS, bool Diagnose = true,
+ bool DiagnoseCFAudited = false, bool ConvertRHS = true);
// \brief If the lhs type is a transparent union, check whether we
// can initialize the transparent union with the given expression.
diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index e47f35528f6..76877e1f2f9 100644
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -7793,6 +7793,10 @@ Sema::CheckSingleAssignmentConstraints(QualType LHSType, ExprResult &CallerRHS,
bool Diagnose,
bool DiagnoseCFAudited,
bool ConvertRHS) {
+ // We need to be able to tell the caller whether we diagnosed a problem, if
+ // they ask us to issue diagnostics.
+ assert((ConvertRHS || !Diagnose) && "can't indicate whether we diagnosed");
+
// If ConvertRHS is false, we want to leave the caller's RHS untouched. Sadly,
// we can't avoid *all* modifications at the moment, so we need some somewhere
// to put the updated value.
@@ -7804,9 +7808,9 @@ Sema::CheckSingleAssignmentConstraints(QualType LHSType, ExprResult &CallerRHS,
// C++ 5.17p3: If the left operand is not of class type, the
// expression is implicitly converted (C++ 4) to the
// cv-unqualified type of the left operand.
- ExprResult Res;
+ QualType RHSType = RHS.get()->getType();
if (Diagnose) {
- Res = PerformImplicitConversion(RHS.get(), LHSType.getUnqualifiedType(),
+ RHS = PerformImplicitConversion(RHS.get(), LHSType.getUnqualifiedType(),
AA_Assigning);
} else {
ImplicitConversionSequence ICS =
@@ -7818,17 +7822,15 @@ Sema::CheckSingleAssignmentConstraints(QualType LHSType, ExprResult &CallerRHS,
/*AllowObjCWritebackConversion=*/false);
if (ICS.isFailure())
return Incompatible;
- Res = PerformImplicitConversion(RHS.get(), LHSType.getUnqualifiedType(),
+ RHS = PerformImplicitConversion(RHS.get(), LHSType.getUnqualifiedType(),
ICS, AA_Assigning);
}
- if (Res.isInvalid())
+ if (RHS.isInvalid())
return Incompatible;
Sema::AssignConvertType result = Compatible;
if (getLangOpts().ObjCAutoRefCount &&
- !CheckObjCARCUnavailableWeakConversion(LHSType,
- RHS.get()->getType()))
+ !CheckObjCARCUnavailableWeakConversion(LHSType, RHSType))
result = IncompatibleObjCWeakRef;
- RHS = Res;
return result;
}
diff --git a/lib/Sema/SemaExprCXX.cpp b/lib/Sema/SemaExprCXX.cpp
index 8d20ac2a5d8..ad102f8d203 100644
--- a/lib/Sema/SemaExprCXX.cpp
+++ b/lib/Sema/SemaExprCXX.cpp
@@ -3320,6 +3320,10 @@ Sema::PerformImplicitConversion(Expr *From, QualType ToType,
llvm_unreachable("Cannot perform an ellipsis conversion");
case ImplicitConversionSequence::BadConversion:
+ bool Diagnosed =
+ DiagnoseAssignmentResult(Incompatible, From->getExprLoc(), ToType,
+ From->getType(), From, Action);
+ assert(Diagnosed && "failed to diagnose bad conversion"); (void)Diagnosed;
return ExprError();
}
diff --git a/lib/Sema/SemaOverload.cpp b/lib/Sema/SemaOverload.cpp
index c10a164b738..7ca85a907ea 100644
--- a/lib/Sema/SemaOverload.cpp
+++ b/lib/Sema/SemaOverload.cpp
@@ -5344,6 +5344,7 @@ TryContextuallyConvertToObjCPointer(Sema &S, Expr *From) {
/// PerformContextuallyConvertToObjCPointer - Perform a contextual
/// conversion of the expression From to an Objective-C pointer type.
+/// Returns a valid but null ExprResult if no conversion sequence exists.
ExprResult Sema::PerformContextuallyConvertToObjCPointer(Expr *From) {
if (checkPlaceholderForOverload(*this, From))
return ExprError();
@@ -5353,7 +5354,7 @@ ExprResult Sema::PerformContextuallyConvertToObjCPointer(Expr *From) {
TryContextuallyConvertToObjCPointer(*this, From);
if (!ICS.isBad())
return PerformImplicitConversion(From, Ty, ICS, AA_Converting);
- return ExprError();
+ return ExprResult();
}
/// Determine whether the provided type is an integral type, or an enumeration
diff --git a/lib/Sema/SemaPseudoObject.cpp b/lib/Sema/SemaPseudoObject.cpp
index c93d800f96d..a6140184cf6 100644
--- a/lib/Sema/SemaPseudoObject.cpp
+++ b/lib/Sema/SemaPseudoObject.cpp
@@ -770,7 +770,8 @@ ExprResult ObjCPropertyOpBuilder::buildSet(Expr *op, SourceLocation opcLoc,
ExprResult opResult = op;
Sema::AssignConvertType assignResult
= S.CheckSingleAssignmentConstraints(paramType, opResult);
- if (S.DiagnoseAssignmentResult(assignResult, opcLoc, paramType,
+ if (opResult.isInvalid() ||
+ S.DiagnoseAssignmentResult(assignResult, opcLoc, paramType,
op->getType(), opResult.get(),
Sema::AA_Assigning))
return ExprError();
diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp
index 901f875840e..e261b67afbd 100644
--- a/lib/Sema/SemaStmt.cpp
+++ b/lib/Sema/SemaStmt.cpp
@@ -3493,6 +3493,8 @@ Sema::ActOnObjCAtSynchronizedOperand(SourceLocation atLoc, Expr *operand) {
<< type << operand->getSourceRange();
ExprResult result = PerformContextuallyConvertToObjCPointer(operand);
+ if (result.isInvalid())
+ return ExprError();
if (!result.isUsable())
return Diag(atLoc, diag::error_objc_synchronized_expects_object)
<< type << operand->getSourceRange();
diff --git a/test/CXX/drs/dr0xx.cpp b/test/CXX/drs/dr0xx.cpp
index 69cb776c330..d598cfa33bd 100644
--- a/test/CXX/drs/dr0xx.cpp
+++ b/test/CXX/drs/dr0xx.cpp
@@ -286,10 +286,9 @@ namespace dr25 { // dr25: yes
void (A::*i2)() throw () = 0;
void (A::*j)() throw (int, char) = &A::f;
void x() {
- // FIXME: Don't produce the second error here.
- g2 = f; // expected-error {{is not superset}} expected-error {{incompatible}}
+ g2 = f; // expected-error {{is not superset}}
h = f;
- i2 = &A::f; // expected-error {{is not superset}} expected-error {{incompatible}}
+ i2 = &A::f; // expected-error {{is not superset}}
j = &A::f;
}
}
diff --git a/test/CXX/except/except.spec/p5-pointers.cpp b/test/CXX/except/except.spec/p5-pointers.cpp
index fe4a264587f..dedc5bd376f 100644
--- a/test/CXX/except/except.spec/p5-pointers.cpp
+++ b/test/CXX/except/except.spec/p5-pointers.cpp
@@ -41,25 +41,25 @@ void fnptrs()
{
// Assignment and initialization of function pointers.
void (*t1)() throw() = &s1; // valid
- t1 = &s2; // expected-error {{not superset}} expected-error {{incompatible type}}
- t1 = &s3; // expected-error {{not superset}} expected-error {{incompatible type}}
+ t1 = &s2; // expected-error {{not superset}}
+ t1 = &s3; // expected-error {{not superset}}
void (&t2)() throw() = s2; // expected-error {{not superset}}
void (*t3)() throw(int) = &s2; // valid
void (*t4)() throw(A) = &s1; // valid
t4 = &s3; // valid
t4 = &s4; // valid
- t4 = &s5; // expected-error {{not superset}} expected-error {{incompatible type}}
+ t4 = &s5; // expected-error {{not superset}}
void (*t5)() = &s1; // valid
t5 = &s2; // valid
t5 = &s6; // valid
t5 = &s7; // valid
- t1 = t3; // expected-error {{not superset}} expected-error {{incompatible type}}
+ t1 = t3; // expected-error {{not superset}}
t3 = t1; // valid
void (*t6)() throw(B1);
- t6 = t4; // expected-error {{not superset}} expected-error {{incompatible type}}
+ t6 = t4; // expected-error {{not superset}}
t4 = t6; // valid
t5 = t1; // valid
- t1 = t5; // expected-error {{not superset}} expected-error {{incompatible type}}
+ t1 = t5; // expected-error {{not superset}}
// return types and arguments must match exactly, no inheritance allowed
void (*(*t7)())() throw(B1) = &s8; // valid
diff --git a/test/SemaCXX/ambig-user-defined-conversions.cpp b/test/SemaCXX/ambig-user-defined-conversions.cpp
index 1a3c102f034..276c1b07b5d 100644
--- a/test/SemaCXX/ambig-user-defined-conversions.cpp
+++ b/test/SemaCXX/ambig-user-defined-conversions.cpp
@@ -65,3 +65,8 @@ namespace rdar8876150 {
bool f(D d) { return !d; } // expected-error{{ambiguous conversion from derived class 'rdar8876150::D' to base class 'rdar8876150::A':}}
}
+
+namespace assignment {
+ struct A { operator short(); operator bool(); }; // expected-note 2{{candidate}}
+ void f(int n, A a) { n = a; } // expected-error{{ambiguous}}
+}
diff --git a/test/SemaCXX/derived-to-base-ambig.cpp b/test/SemaCXX/derived-to-base-ambig.cpp
index 93bd3619ccd..5d1d56b7661 100644
--- a/test/SemaCXX/derived-to-base-ambig.cpp
+++ b/test/SemaCXX/derived-to-base-ambig.cpp
@@ -6,7 +6,7 @@ class D : public B, public C { };
void f(D* d) {
A* a;
- a = d; // expected-error{{ambiguous conversion from derived class 'D' to base class 'A':}} expected-error{{assigning to 'A *' from incompatible type 'D *'}}
+ a = d; // expected-error{{ambiguous conversion from derived class 'D' to base class 'A':}}
}
class Object2 { };
@@ -20,7 +20,7 @@ class F2 : public E2, public A2 { }; // expected-warning{{direct base 'A2' is in
void g(E2* e2, F2* f2) {
Object2* o2;
o2 = e2;
- o2 = f2; // expected-error{{ambiguous conversion from derived class 'F2' to base class 'Object2':}} expected-error{{assigning to 'Object2 *' from incompatible type 'F2 *'}}
+ o2 = f2; // expected-error{{ambiguous conversion from derived class 'F2' to base class 'Object2':}}
}
// Test that ambiguous/inaccessibility checking does not trigger too
--
GitLab