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

diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index f7c1313d8f107ae582057283d1ba963a12b2f537..476631d7a8f94f30e9f6105fb59427ada772f116 100644 (file)
--- 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 e47f35528f68b5c4731d64d9ca7384792cc2742a..76877e1f2f917ed335a5670c60360111378cac3e 100644 (file)
--- 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 8d20ac2a5d801ffa70ef0cb7558638dd05d31f29..ad102f8d203211b55a267969247762d9758677b6 100644 (file)
--- 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 c10a164b738105e637dffdda8a3d9ee93fa4806b..7ca85a907ead8ac284d80ecfc706a13a081601f9 100644 (file)
--- 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 c93d800f96d1e16d9b86e6e534722b3b7e53fc4e..a6140184cf684041b71594e674c19ba5d88db54a 100644 (file)
--- 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 901f875840ec509ae668d12b772f849e5c216a49..e261b67afbd9ff0df70e23bd01fef987d991bcc3 100644 (file)
--- 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 69cb776c33042b644431af643f6b95dd45a1764e..d598cfa33bd448bdee045637f5d3aa44a3bd07ca 100644 (file)
--- 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 fe4a264587f5a18ba7fdba6880344b173f007b72..dedc5bd376f921108edc3241ee15d92f74bef0c3 100644 (file)
--- 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 1a3c102f034c03bed72b1037f6464b117b94d57b..276c1b07b5d9413ded9528cbd284584be59243d0 100644 (file)
--- 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 93bd3619ccdf90eb4bd6eaedaba57e0503be095e..5d1d56b76614487e9f06697d95a65ea629bc751a 100644 (file)
--- 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