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.

llvm-svn: 283508

clang/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.

clang/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;
     }
 

clang/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();
   }
 

clang/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

clang/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();

clang/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();

clang/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;
   }
 }

clang/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

clang/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}}
+}

clang/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