[Sema] More changes to fix Objective-C fallout from r249995.

[bob.wilson]

Summary

This is a follow-up to PR26085. That was fixed in r257710 but the testcase there was incomplete. There is a related issue where the overload resolution for Objective-C incorrectly picks a method that is not valid without a bridge cast. The call to Sema::CheckSingleAssignmentConstraints that was added to SemaOverload.cpp's IsStandardConversion() function does not catch that case and reports that the method is Compatible even when it is not.

The root cause here is that various Objective-C-related functions in Sema do not consistently return a value to indicate whether there was an error. This was fine in the past because they would report diagnostics when needed, but r257710 changed them to suppress reporting diagnostics when checking during overload resolution.

This patch adds a new ACR_error result to the ARCConversionResult enum and updates Sema::CheckObjCARCConversion to return that value when there is an error. Most of the calls to that function do not check the return value, so adding this new result does not affect them. The one exception is in SemaCast.cpp where it specifically checks for ACR_unbridged, so that is also OK. The call in Sema::CheckSingleAssignmentConstraints can then check for an ACR_okay result and identify assignments as Incompatible. To preserve the existing behavior, it only changes the return value to Incompatible when the new Diagnose argument (from r257710) is false.

Similarly, the CheckObjCBridgeRelatedConversions and ConversionToObjCStringLiteralCheck need to identify when an assignment is Incompatible. Those functions already return appropriate values but they need some fixes related to the new Diagnose argument.

[george.burgess.iv]

Sorry that the change to overload resolution turned out to be problematic. :)

One nit, and this LGTM.

lib/Sema/SemaExpr.cpp

7580

Because we're doing a similar thing at line 7573 (not necessarily replacing an expression, but still answering differently depending on the value of Diagnose), can we either put a similar comment there, or make this comment a bit more general and move it above if (getLangOpts().ObjCAutoRefCount && [...]?

[bob.wilson]

Committed in r260787.

I had previously missed a regression in one of the existing ovl-check.m tests. The testTakesCFTypeRef function was checking that the CFTypeRef overload was selected. However, that does not match the behavior prior to r249995, where neither overload candidate is chosen as the "best match" and clang falls back to using the first one. With my change here, the previous behavior is restored. I reordered the two overload candidates in the test to match that. While I was at it, I also merged my "Type2" typedef with the existing CFTypeRef in the test.

lib/Sema/SemaExpr.cpp

7580

Good idea. I'm going to put a comment above the "if" but I'll also leave something to explain why the expression is replaced. (I'm also going to change those "result = Incompatible" lines to just return, since there is no point in continuing with further checks if we're not reporting the diagnostics anyway.)

include/clang/Sema/Sema.h

                                         Expr *CastExpr,
                                         SourceLocation RParenLoc);
 
-  enum ARCConversionResult { ACR_okay, ACR_unbridged };
+  enum ARCConversionResult { ACR_okay, ACR_unbridged, ACR_error };
 
   /// \brief Checks for invalid conversions and casts between
   /// retainable pointers and other pointer kinds.

lib/Sema/SemaExpr.cpp

   if (result != Incompatible && RHS.get()->getType() != LHSType) {
     QualType Ty = LHSType.getNonLValueExprType(Context);
     Expr *E = RHS.get();
-    if (getLangOpts().ObjCAutoRefCount)
+    if (getLangOpts().ObjCAutoRefCount &&
         CheckObjCARCConversion(SourceRange(), Ty, E, CCK_ImplicitConversion,
-                             Diagnose, DiagnoseCFAudited);
+                               Diagnose, DiagnoseCFAudited) != ACR_okay) {
+      if (!Diagnose)
+        result = Incompatible;
+    }
     if (getLangOpts().ObjC1 &&
         (CheckObjCBridgeRelatedConversions(E->getLocStart(), LHSType,
                                            E->getType(), E, Diagnose) ||
          ConversionToObjCStringLiteralCheck(LHSType, E, Diagnose))) {
-      RHS = E;
-      return Compatible;
+      if (Diagnose) {
+        // If an error was diagnosed here, replace the expression with a
+        // corrected version and continue so we can find further errors.
+        RHS = E;
+        return Compatible;
+      }
+      result = Incompatible;
     }
     
     if (ConvertRHS)

   StringLiteral *SL = dyn_cast<StringLiteral>(SrcExpr);
   if (!SL || !SL->isAscii())
     return false;
-  if (Diagnose)
+  if (Diagnose) {
     Diag(SL->getLocStart(), diag::err_missing_atsign_prefix)
       << FixItHint::CreateInsertion(SL->getLocStart(), "@");
     Exp = BuildObjCStringLiteral(SL->getLocStart(), SL).get();
+  }
   return true;
 }
 

lib/Sema/SemaExprObjC.cpp

     return;
 
   QualType castExprType = castExpr->getType();
+  // Defer emitting a diagnostic for bridge-related casts; that will be
+  // handled by CheckObjCBridgeRelatedConversions.
   TypedefNameDecl *TDNDecl = nullptr;
   if ((castACTC == ACTC_coreFoundation &&  exprACTC == ACTC_retainable &&
        ObjCBridgeRelatedAttrFromType(castType, TDNDecl)) ||

           << FixItHint::CreateInsertion(SrcExprEndLoc, "]");
         Diag(RelatedClass->getLocStart(), diag::note_declared_at);
         Diag(TDNDecl->getLocStart(), diag::note_declared_at);
-      }
       
         QualType receiverType = Context.getObjCInterfaceType(RelatedClass);
         // Argument.
         Expr *args[] = { SrcExpr };
         ExprResult msg = BuildClassMessageImplicit(receiverType, false,
                                       ClassMethod->getLocation(),
                                       ClassMethod->getSelector(), ClassMethod,
                                       MultiExprArg(args, 1));
         SrcExpr = msg.get();
+      }
       return true;
     }
   }

         }
         Diag(RelatedClass->getLocStart(), diag::note_declared_at);
         Diag(TDNDecl->getLocStart(), diag::note_declared_at);
-      }
       
         ExprResult msg =
           BuildInstanceMessageImplicit(SrcExpr, SrcType,
                                        InstanceMethod->getLocation(),
                                        InstanceMethod->getSelector(),
                                        InstanceMethod, None);
         SrcExpr = msg.get();
+      }
       return true;
     }
   }

   ARCConversionTypeClass exprACTC = classifyTypeForARCConversion(castExprType);
   ARCConversionTypeClass castACTC = classifyTypeForARCConversion(effCastType);
   if (exprACTC == castACTC) {
-    // check for viablity and report error if casting an rvalue to a
+    // Check for viability and report error if casting an rvalue to a
     // life-time qualifier.
-    if (Diagnose && castACTC == ACTC_retainable &&
+    if (castACTC == ACTC_retainable &&
         (CCK == CCK_CStyleCast || CCK == CCK_OtherCast) &&
         castType != castExprType) {
       const Type *DT = castType.getTypePtr();

         QDT = AT->desugar();
       if (QDT != castType &&
           QDT.getObjCLifetime() !=  Qualifiers::OCL_None) {
-        SourceLocation loc =
-          (castRange.isValid() ? castRange.getBegin() 
+        if (Diagnose) {
+          SourceLocation loc = (castRange.isValid() ? castRange.getBegin() 
                                                     : castExpr->getExprLoc());
           Diag(loc, diag::err_arc_nolifetime_behavior);
+        }
+        return ACR_error;
       }
     }
     return ACR_okay;

       CCK != CCK_ImplicitConversion)
     return ACR_unbridged;
 
-  // Do not issue bridge cast" diagnostic when implicit casting a cstring
-  // to 'NSString *'. Let caller issue a normal mismatched diagnostic with
-  // suitable fix-it.
+  // Issue a diagnostic about a missing @-sign when implicit casting a cstring
+  // to 'NSString *', instead of falling through to report a "bridge cast"
+  // diagnostic.
   if (castACTC == ACTC_retainable && exprACTC == ACTC_none &&
       ConversionToObjCStringLiteralCheck(castType, castExpr, Diagnose))
-    return ACR_okay;
+    return ACR_error;
   
   // Do not issue "bridge cast" diagnostic when implicit casting
   // a retainable object to a CF type parameter belonging to an audited
   // CF API function. Let caller issue a normal type mismatched diagnostic
   // instead.
-  if (Diagnose &&
-      (!DiagnoseCFAudited || exprACTC != ACTC_retainable ||
-        castACTC != ACTC_coreFoundation))
-    if (!(exprACTC == ACTC_voidPtr && castACTC == ACTC_retainable &&
-          (Opc == BO_NE || Opc == BO_EQ)))
+  if ((!DiagnoseCFAudited || exprACTC != ACTC_retainable ||
+       castACTC != ACTC_coreFoundation) &&
+      !(exprACTC == ACTC_voidPtr && castACTC == ACTC_retainable &&
+        (Opc == BO_NE || Opc == BO_EQ))) {
+    if (Diagnose)
       diagnoseObjCARCConversion(*this, castRange, castType, castACTC, castExpr,
                                 castExpr, exprACTC, CCK);
+    return ACR_error;
+  }
   return ACR_okay;
 }
 

test/SemaObjC/ovl-check.m

 // in overload resolution in ObjC.
 
 struct Type1 { int a; };
+typedef const __attribute__((objc_bridge(id))) void * Type2;
 @interface Iface1 @end
 
 @interface NeverCalled
 - (void) test:(struct Type1 *)arg;
+- (void) test2:(Type2 )arg;
 @end
 
 @interface TakesIface1
 - (void) test:(Iface1 *)arg;
+- (void) test2:(Iface1 *)arg;
 @end
 
 // PR26085, rdar://problem/24111333
 void testTakesIface1(id x, Iface1 *arg) {
   // This should resolve silently to `TakesIface1`.
   [x test:arg];
+  [x test2:arg];
 }
 
 @class NSString;