[Objective-C] Fix "weak-unavailable" warning with -fobjc-weak

[bkelley]

Summary

clang should produce the same errors Objective-C classes that cannot be assigned to weak pointers under both -fobjc-arc and -fobjc-weak. Check for ObjCWeak along with ObjCAutoRefCount when analyzing pointer conversions. Add an -fobjc-weak pass to the existing arc-unavailable-for-weakref test cases to verify the behavior is the same.

[rjmccall]

lib/Sema/SemaCast.cpp

125

Unlike the other patches, we do clearly need to be checking the language options in places like this. Still, it's a shame to repeat the same condition in a million places.

I think the right thing to do here is to add a helper method to LangOpts:

/// Returns true if any types in the program might have non-trivial lifetime qualifiers.
bool allowsNonTrivialObjCLifetimeQualifiers() const {
  return ObjCAutoRefCount || ObjCWeak;
}

[bkelley]

lib/Sema/SemaCast.cpp

125

Thanks for the suggestion. I was hesitant to add a method to LangOpts since it has so few derived state functions, but it certainly makes everything else cleaner.

[bkelley]

Updated with feedback from @rjmccall

[rjmccall]

LGTM.

lib/Sema/SemaCast.cpp

125

There's probably two main reasons for that:

• Often, when there's a feature that cuts across different language configurations, there's also a specific language option for it.
• People are too reticent about adding derived state functions to LangOpts. :)

[bkelley]

Thank you @rjmccall for the approval. I don't have commit access; would someone be willing to commit this path for me please? Thanks!

include/clang/Basic/LangOptions.h

#include "clang/Basic/LangOptions.def"
 
   /// \brief Reset all of the options that are not considered when building a
   /// module.
   void resetNonModularOptions();
 
   /// \brief Is this a libc/libm function that is no longer recognized as a
   /// builtin because a -fno-builtin-* option has been specified?
   bool isNoBuiltinFunc(StringRef Name) const;
+
+  /// \brief True if any ObjC types may have non-trivial lifetime qualifiers.
+  bool allowsNonTrivialObjCLifetimeQualifiers() const {
+    return ObjCAutoRefCount || ObjCWeak;
+  }
 };
 
 /// \brief Floating point control options
 class FPOptions {
 public:
   unsigned fp_contract : 1;
 
   FPOptions() : fp_contract(0) {}

include/clang/Sema/Sema.h

   ExprResult BuildCXXFunctionalCastExpr(TypeSourceInfo *TInfo, QualType Type,
                                         SourceLocation LParenLoc,
                                         Expr *CastExpr,
                                         SourceLocation RParenLoc);
 
   enum ARCConversionResult { ACR_okay, ACR_unbridged, ACR_error };
 
   /// \brief Checks for invalid conversions and casts between
-  /// retainable pointers and other pointer kinds.
-  ARCConversionResult CheckObjCARCConversion(SourceRange castRange,
-                                             QualType castType, Expr *&op,
-                                             CheckedConversionKind CCK,
-                                             bool Diagnose = true,
-                                             bool DiagnoseCFAudited = false,
-                                             BinaryOperatorKind Opc = BO_PtrMemD
-                                             );
+  /// retainable pointers and other pointer kinds for ARC and Weak.
+  ARCConversionResult CheckObjCConversion(SourceRange castRange,
 
   Expr *stripARCUnbridgedCast(Expr *e);
   void diagnoseARCUnbridgedCast(Expr *e);
 
   bool CheckObjCARCUnavailableWeakConversion(QualType castType,
                                              QualType ExprType);
 
   /// checkRetainCycles - Check whether an Objective-C message send

lib/Sema/SemaCast.cpp

struct CastOperation {
     bool isPlaceholder(BuiltinType::Kind K) const {
       return PlaceholderKind == K;
     }
 
     void checkCastAlign() {
       Self.CheckCastAlign(SrcExpr.get(), DestType, OpRange);
     }
 
-    void checkObjCARCConversion(Sema::CheckedConversionKind CCK) {
-      assert(Self.getLangOpts().ObjCAutoRefCount);
+    void checkObjCConversion(Sema::CheckedConversionKind CCK) {
+      assert(Self.getLangOpts().allowsNonTrivialObjCLifetimeQualifiers());
 
       Expr *src = SrcExpr.get();
-      if (Self.CheckObjCARCConversion(OpRange, DestType, src, CCK) ==
-            Sema::ACR_unbridged)
+      if (Self.CheckObjCConversion(OpRange, DestType, src, CCK) ==
         IsARCUnbridgedCast = true;
       SrcExpr = src;
     }
 
     /// Check for and handle non-overload placeholder expressions.
     void checkNonOverloadPlaceholders() {
       if (!isPlaceholder() || isPlaceholder(BuiltinType::Overload))
         return;
if (tcr != TC_Success && msg != 0)
 
     } else {
       diagnoseBadCast(Self, msg, CT_Reinterpret, OpRange, SrcExpr.get(),
                       DestType, /*listInitialization=*/false);
     }
     SrcExpr = ExprError();
   } else if (tcr == TC_Success) {
     if (Self.getLangOpts().ObjCAutoRefCount)
-      checkObjCARCConversion(Sema::CCK_OtherCast);
+      checkObjCConversion(Sema::CCK_OtherCast);
     DiagnoseReinterpretUpDownCast(Self, SrcExpr.get(), DestType, OpRange);
   }
 }
 
 
 /// CheckStaticCast - Check that a static_cast\<DestType\>(SrcExpr) is valid.
 /// Refer to C++ 5.2.9 for details. Static casts are mostly used for making
 /// implicit conversions explicit and getting rid of data loss warnings.
if (SrcExpr.get()->getType() == Self.Context.OverloadTy) {
       diagnoseBadCast(Self, msg, CT_Static, OpRange, SrcExpr.get(), DestType,
                       /*listInitialization=*/false);
     }
     SrcExpr = ExprError();
   } else if (tcr == TC_Success) {
     if (Kind == CK_BitCast)
       checkCastAlign();
     if (Self.getLangOpts().ObjCAutoRefCount)
-      checkObjCARCConversion(Sema::CCK_OtherCast);
+      checkObjCConversion(Sema::CCK_OtherCast);
   } else if (Kind == CK_BitCast) {
     checkCastAlign();
   }
 }
 
 /// TryStaticCast - Check if a static cast can be performed, and do so if
 /// possible. If @p CStyle, ignore access restrictions on hierarchy casting
 /// and casting away constness.
if (tcr == TC_NotApplicable) {
       // ... and finally a reinterpret_cast, ignoring const.
       tcr = TryReinterpretCast(Self, SrcExpr, DestType, /*CStyle*/true,
                                OpRange, msg, Kind);
       if (SrcExpr.isInvalid())
         return;
     }
   }
 
-  if (Self.getLangOpts().ObjCAutoRefCount && tcr == TC_Success)
-    checkObjCARCConversion(CCK);
+  if (Self.getLangOpts().allowsNonTrivialObjCLifetimeQualifiers() &&
+      tcr == TC_Success)
+    checkObjCConversion(CCK);
 
   if (tcr != TC_Success && msg != 0) {
     if (SrcExpr.get()->getType() == Self.Context.OverloadTy) {
       DeclAccessPair Found;
       FunctionDecl *Fn = Self.ResolveAddressOfOverloadedFunction(SrcExpr.get(),
                                 DestType,
                                 /*Complain*/ true,
                                 Found);
if (DestType->isHalfType()) {
       Self.Diag(SrcExpr.get()->getLocStart(), diag::err_opencl_cast_to_half)
         << DestType << SrcExpr.get()->getSourceRange();
       SrcExpr = ExprError();
       return;
     }
   }
 
   // ARC imposes extra restrictions on casts.
-  if (Self.getLangOpts().ObjCAutoRefCount) {
-    checkObjCARCConversion(Sema::CCK_CStyleCast);
+  if (Self.getLangOpts().allowsNonTrivialObjCLifetimeQualifiers()) {
+    checkObjCConversion(Sema::CCK_CStyleCast);
     if (SrcExpr.isInvalid())
       return;
-    
-    if (const PointerType *CastPtr = DestType->getAs<PointerType>()) {
+    const PointerType *CastPtr = DestType->getAs<PointerType>();
+    if (Self.getLangOpts().ObjCAutoRefCount && CastPtr) {
       if (const PointerType *ExprPtr = SrcType->getAs<PointerType>()) {
         Qualifiers CastQuals = CastPtr->getPointeeType().getQualifiers();
         Qualifiers ExprQuals = ExprPtr->getPointeeType().getQualifiers();
         if (CastPtr->getPointeeType()->isObjCLifetimeType() && 
             ExprPtr->getPointeeType()->isObjCLifetimeType() &&
             !CastQuals.compatiblyIncludesObjCLifetime(ExprQuals)) {
           Self.Diag(SrcExpr.get()->getLocStart(), 
                     diag::err_typecheck_incompatible_ownership)

lib/Sema/SemaExpr.cpp

Sema::CheckAssignmentConstraints(QualType LHSType, ExprResult &RHS,
 
   // Conversions to Objective-C pointers.
   if (isa<ObjCObjectPointerType>(LHSType)) {
     // A* -> B*
     if (RHSType->isObjCObjectPointerType()) {
       Kind = CK_BitCast;
       Sema::AssignConvertType result = 
         checkObjCPointerTypesForAssignment(*this, LHSType, RHSType);
-      if (getLangOpts().ObjCAutoRefCount &&
+      if (getLangOpts().allowsNonTrivialObjCLifetimeQualifiers() &&
           result == Compatible && 
           !CheckObjCARCUnavailableWeakConversion(OrigLHSType, RHSType))
         result = IncompatibleObjCWeakRef;
       return result;
     }
 
     // int or null -> A*
     if (RHSType->isIntegerType()) {
if (!LHSType->isRecordType() && !LHSType->isAtomicType()) {
         if (ICS.isFailure())
           return Incompatible;
         RHS = PerformImplicitConversion(RHS.get(), LHSType.getUnqualifiedType(),
                                         ICS, AA_Assigning);
       }
       if (RHS.isInvalid())
         return Incompatible;
       Sema::AssignConvertType result = Compatible;
-      if (getLangOpts().ObjCAutoRefCount &&
+      if (getLangOpts().allowsNonTrivialObjCLifetimeQualifiers() &&
           !CheckObjCARCUnavailableWeakConversion(LHSType, RHSType))
         result = IncompatibleObjCWeakRef;
       return result;
     }
 
     // FIXME: Currently, we fall through and treat C++ classes like C
     // structures.
     // FIXME: We also fall through for atomics; not sure what should
Sema::CheckSingleAssignmentConstraints(QualType LHSType, ExprResult &CallerRHS,
   // does not have reference type.
   if (result != Incompatible && RHS.get()->getType() != LHSType) {
     QualType Ty = LHSType.getNonLValueExprType(Context);
     Expr *E = RHS.get();
 
     // Check for various Objective-C errors. If we are not reporting
     // diagnostics and just checking for errors, e.g., during overload
     // resolution, return Incompatible to indicate the failure.
-    if (getLangOpts().ObjCAutoRefCount &&
-        CheckObjCARCConversion(SourceRange(), Ty, E, CCK_ImplicitConversion,
-                               Diagnose, DiagnoseCFAudited) != ACR_okay) {
+    if (getLangOpts().allowsNonTrivialObjCLifetimeQualifiers() &&
+        CheckObjCConversion(SourceRange(), Ty, E, CCK_ImplicitConversion,
       if (!Diagnose)
         return Incompatible;
     }
     if (getLangOpts().ObjC1 &&
         (CheckObjCBridgeRelatedConversions(E->getLocStart(), LHSType,
                                            E->getType(), E, Diagnose) ||
          ConversionToObjCStringLiteralCheck(LHSType, E, Diagnose))) {
       if (!Diagnose)
if (LPT || RPT) {
       if (!LPtrToVoid && !RPtrToVoid &&
           !Context.typesAreCompatible(LHSType, RHSType)) {
         diagnoseDistinctPointerComparison(*this, Loc, LHS, RHS,
                                           /*isError*/false);
       }
       if (LHSIsNull && !RHSIsNull) {
         Expr *E = LHS.get();
         if (getLangOpts().ObjCAutoRefCount)
-          CheckObjCARCConversion(SourceRange(), RHSType, E, CCK_ImplicitConversion);
+          CheckObjCConversion(SourceRange(), RHSType, E,
+                              CCK_ImplicitConversion);
         LHS = ImpCastExprToType(E, RHSType,
                                 RPT ? CK_BitCast :CK_CPointerToObjCPointerCast);
       }
       else {
         Expr *E = RHS.get();
         if (getLangOpts().ObjCAutoRefCount)
-          CheckObjCARCConversion(SourceRange(), LHSType, E,
-                                 CCK_ImplicitConversion, /*Diagnose=*/true,
-                                 /*DiagnoseCFAudited=*/false, Opc);
+          CheckObjCConversion(SourceRange(), LHSType, E, CCK_ImplicitConversion,
+                              /*Diagnose=*/true,
         RHS = ImpCastExprToType(E, LHSType,
                                 LPT ? CK_BitCast :CK_CPointerToObjCPointerCast);
       }
       return ResultTy;
     }
     if (LHSType->isObjCObjectPointerType() &&
         RHSType->isObjCObjectPointerType()) {
       if (!Context.areComparableObjCPointerTypes(LHSType, RHSType))

lib/Sema/SemaExprCXX.cpp

if (SCS.IncompatibleObjC && Action != AA_Casting) {
         Diag(From->getLocStart(),
              diag::ext_typecheck_convert_incompatible_pointer)
           << From->getType() << ToType << Action
           << From->getSourceRange() << 0;
 
       if (From->getType()->isObjCObjectPointerType() &&
           ToType->isObjCObjectPointerType())
         EmitRelatedResultTypeNote(From);
-    }
-    else if (getLangOpts().ObjCAutoRefCount &&
+    } else if (getLangOpts().allowsNonTrivialObjCLifetimeQualifiers() &&
                !CheckObjCARCUnavailableWeakConversion(ToType,
                                                       From->getType())) {
       if (Action == AA_Initializing)
         Diag(From->getLocStart(),
              diag::err_arc_weak_unavailable_assign);
       else
         Diag(From->getLocStart(),
              diag::err_arc_convesion_of_weak_unavailable)
           << (Action == AA_Casting) << From->getType() << ToType
           << From->getSourceRange();
     }
 
     CastKind Kind = CK_Invalid;
     CXXCastPath BasePath;
     if (CheckPointerConversion(From, ToType, Kind, BasePath, CStyle))
       return ExprError();
 
     // Make sure we extend blocks if necessary.
     // FIXME: doing this here is really ugly.
     if (Kind == CK_BlockPointerToObjCPointerCast) {
       ExprResult E = From;
       (void) PrepareCastToObjCObjectPointer(E);
       From = E.get();
     }
-    if (getLangOpts().ObjCAutoRefCount)
-      CheckObjCARCConversion(SourceRange(), ToType, From, CCK);
+    if (getLangOpts().allowsNonTrivialObjCLifetimeQualifiers())
+      CheckObjCConversion(SourceRange(), ToType, From, CCK);
     From = ImpCastExprToType(From, ToType, Kind, VK_RValue, &BasePath, CCK)
              .get();
     break;
   }
 
   case ICK_Pointer_Member: {
     CastKind Kind = CK_Invalid;
     CXXCastPath BasePath;

lib/Sema/SemaExprObjC.cpp

if (InstanceMethod) {
       }
       return true;
     }
   }
   return false;
 }
 
 Sema::ARCConversionResult
-Sema::CheckObjCARCConversion(SourceRange castRange, QualType castType,
-                             Expr *&castExpr, CheckedConversionKind CCK,
-                             bool Diagnose,
-                             bool DiagnoseCFAudited,
-                             BinaryOperatorKind Opc) {
+Sema::CheckObjCConversion(SourceRange castRange, QualType castType,
+                          bool Diagnose, bool DiagnoseCFAudited,
   QualType castExprType = castExpr->getType();
 
   // For the purposes of the classification, we assume reference types
   // will bind to temporaries.
   QualType effCastType = castType;
   if (const ReferenceType *ref = castType->getAs<ReferenceType>())
     effCastType = ref->getPointeeType();
   
if (castACTC == ACTC_retainable &&
                                                     : castExpr->getExprLoc());
           Diag(loc, diag::err_arc_nolifetime_behavior);
         }
         return ACR_error;
       }
     }
     return ACR_okay;
   }
-  
+  // The life-time qualifier cast check above is all we need for ObjCWeak.
+  // ObjCAutoRefCount has more restrictions on what is legal.
+  if (!getLangOpts().ObjCAutoRefCount)
+    return ACR_okay;
+
   if (isAnyCLike(exprACTC) && isAnyCLike(castACTC)) return ACR_okay;
 
   // Allow all of these types to be cast to integer types (but not
   // vice-versa).
   if (castACTC == ACTC_none && castType->isIntegralType(Context))
     return ACR_okay;
   
   // Allow casts between pointers to lifetime types (e.g., __strong id*)

lib/Sema/SemaPseudoObject.cpp

IdentifierInfo *KeyIdents[] = {
     &S.Context.Idents.get("objectForKeyedSubscript")  
   };
   Selector GetterSelector = S.Context.Selectors.getSelector(1, KeyIdents);
   ObjCMethodDecl *Getter = S.LookupMethodInObjectType(GetterSelector, ContainerT, 
                                                       true /*instance*/);
   if (!Getter)
     return;
   QualType T = Getter->parameters()[0]->getType();
-  S.CheckObjCARCConversion(Key->getSourceRange(), 
-                         T, Key, Sema::CCK_ImplicitConversion);
+  S.CheckObjCConversion(Key->getSourceRange(), T, Key,
+                        Sema::CCK_ImplicitConversion);
 }
 
 bool ObjCSubscriptOpBuilder::findAtIndexGetter() {
   if (AtIndexGetter)
     return true;
   
   Expr *BaseExpr = RefExpr->getBaseExpr();
   QualType BaseT = BaseExpr->getType();

test/SemaObjC/arc-unavailable-for-weakref.m

 // RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -verify -Wno-objc-root-class %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-weak -verify -Wno-objc-root-class %s
 // rdar://9693477
 
 __attribute__((objc_arc_weak_reference_unavailable))
 @interface NSOptOut1072  // expected-note {{class is declared here}}
 @end
 
 @interface sub : NSOptOut1072 @end // expected-note 2 {{class is declared here}}
 

test/SemaObjCXX/arc-unavailable-for-weakref.mm

 // RUN: %clang_cc1 -x objective-c++ -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -verify %s
+// RUN: %clang_cc1 -x objective-c++ -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-weak -verify %s
 // rdar://9693477
 
 __attribute__((objc_arc_weak_reference_unavailable))
 @interface NSOptOut1072  // expected-note {{class is declared here}}
 @end
 
 @interface sub : NSOptOut1072 @end // expected-note 2 {{class is declared here}}