[Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

[bkelley]

Summary

-Warc-repeated-use-of-weak should produce the same warnings with -fobjc-weak as it does with -objc-arc. Also check for ObjCWeak along with ObjCAutoRefCount when recording the use of an evaluated weak variable. Add a -fobjc-weak run to the existing arc-repeated-weak test case and adapt it slightly to work in both modes.

[jordan_rose]

The warning-related parts look reasonable to me.

lib/Sema/SemaDecl.cpp

10184

This condition's getting complicated, and it shows up in a few places. Would it make sense to factor it out?

lib/Sema/SemaExpr.cpp

706–707

This is the one section that isn't dealing with the warning. I'd like @rjmccall to verify that it's correct.

10336

Does ObjCAutoRefCount imply ObjCWeak? If so, you could just use the latter.

[bkelley]

lib/Sema/SemaDecl.cpp

10184

What do you think about adding a member function like hasMRCNonTrivialWeakObjCLifetime(const ASTContext &Context) to QualType to factor out lines 10183-10184? We could use that in D31003, D31004, here, and D31007.

lib/Sema/SemaExpr.cpp

10336

I don't believe so. For Snow Leopard, ARC without weak references was supported so they can be independent.

[jordan_rose]

lib/Sema/SemaDecl.cpp

10184

I'm fine with it myself but I don't work on Clang very much anymore. Maybe someone else can say whether it's actually a good idea.

(By the way, the conventional abbreviation for this mode is "MRR" for "Manual Retain/Release", even though it's "ARC" and "Automated Reference Counting".)

lib/Sema/SemaExpr.cpp

10336

Sure, but in that case we don't need the warning, right?

[arphaman]

lib/Sema/SemaDecl.cpp

10184

Do you want to extract the out the entire

(!getLangOpts().ObjCAutoRefCount && getLangOpts().ObjCWeak &&
          VDecl->getType().getObjCLifetime() != Qualifiers::OCL_Weak)

?

It looks like the others patches use only getLangOpts().ObjCWeak && Type.getObjCLifetime() != Qualifiers::OCL_Weak so the use of hasMRCNonTrivialWeakObjCLifetime won't be equivalent to the original code in the other patches if you extract all code from lines 10183-10184.

test/SemaObjC/arc-repeated-weak.mm

468

NIT: You can keep the code in foo1 active and just use the #if on the expected-error, like:

void foo1() {
  INTFPtrTy tmp = (INTFPtrTy)e1; 
#if __has_feature(objc_arc)
// expected-error@-2 {{cast of 'E' to 'INTFPtrTy' (aka 'INTF *') is disallowed with ARC}}
#endif
}

[bkelley]

lib/Sema/SemaDecl.cpp

10184

Yeah, my suspicion was that the addition of !getLangOpts().ObjCAutoRefCount() would have been fine, but most of the other code was simplified by using hasNonTrivialObjCLifetime() or another means, so this new function seems to only be necessary in this patch. I misnamed the proposed function, which would imply the qualifier is OCL_Weak, but we need not that, so my new proposed name is the odd looking isNonWeakInMRRWithObjCWeak().

lib/Sema/SemaExpr.cpp

10336

Oh, I see. Yeah, looks like I can update most of the checks to just use ObjCWeak. I think we need both conditions here, however, since checkUnsafeExprAssigns() emits the "assigning retained object to unsafe property" warning, which is only applicable in ARC.

[bkelley]

Updated with feedback from @jordan_rose and @arphaman

[bkelley]

Thanks again for the feedback. Is there anything further I should update in this diff or is it looking good?

Thanks!
Brian

[rjmccall]

Just a couple of minor requests.

lib/Sema/SemaExpr.cpp

707

Much like the other patches, it's probably more efficient to just check the qualifier here instead of testing the language option first.

2515–2516

Same thing.

lib/Sema/SemaExprMember.cpp

1504–1505

Same.

lib/Sema/SemaPseudoObject.cpp

843–844

Same.

[bkelley]

Updated with feedback from @rjmccall

[bkelley]

Sorry for missing the unnecessary LangOpts checks here. Thanks again for the feedback!

[rjmccall]

LGTM, thanks.

[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/AST/Type.h

public:
   bool hasNonTrivialObjCLifetime() const {
     return getQualifiers().hasNonTrivialObjCLifetime();
   }
 
   bool hasStrongOrWeakObjCLifetime() const {
     return getQualifiers().hasStrongOrWeakObjCLifetime();
   }
 
+  // true when Type is objc's weak and weak is enabled but ARC isn't.
+  bool isNonWeakInMRRWithObjCWeak(const ASTContext &Context) const;
+
   enum DestructionKind {
     DK_none,
     DK_cxx_destructor,
     DK_objc_strong_lifetime,
     DK_objc_weak_lifetime
   };
 
   /// Returns a nonzero value if objects of this type require

lib/AST/Type.cpp

if (const RecordType *RT = CanonicalType->getAs<RecordType>()) {
 
     return true;
   }
 
   // No other types can match.
   return false;
 }
 
-
+bool QualType::isNonWeakInMRRWithObjCWeak(const ASTContext &Context) const {
+  return !Context.getLangOpts().ObjCAutoRefCount &&
+         Context.getLangOpts().ObjCWeak &&
+         getObjCLifetime() != Qualifiers::OCL_Weak;
+}
 
 bool Type::isLiteralType(const ASTContext &Ctx) const {
   if (isDependentType())
     return false;
 
   // C++1y [basic.types]p10:
   //   A type is a literal type if it is:
   //   -- cv void; or

lib/Sema/SemaDecl.cpp

if (!VDecl->isInvalidDecl()) {
 
     // It is safe to assign a weak reference into a strong variable.
     // Although this code can still have problems:
     //   id x = self.weakProp;
     //   id y = self.weakProp;
     // we do not warn to warn spuriously when 'x' and 'y' are on separate
     // paths through the function. This should be revisited if
     // -Wrepeated-use-of-weak is made flow-sensitive.
-    if (VDecl->getType().getObjCLifetime() == Qualifiers::OCL_Strong &&
+    if ((VDecl->getType().getObjCLifetime() == Qualifiers::OCL_Strong ||
+         VDecl->getType().isNonWeakInMRRWithObjCWeak(Context)) &&
         !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak,
                          Init->getLocStart()))
       getCurFunction()->markSafeWeakUse(Init);
   }
 
   // The initialization is usually a full-expression.
   //
   // FIXME: If this is a braced initialization of an aggregate, it is not
   // an expression, and each individual field initializer is a separate

lib/Sema/SemaExpr.cpp

ExprResult Sema::DefaultLvalueConversion(Expr *E) {
   // Under the MS ABI, lock down the inheritance model now.
   if (T->isMemberPointerType() &&
       Context.getTargetInfo().getCXXABI().isMicrosoft())
     (void)isCompleteType(E->getExprLoc(), T);
 
   UpdateMarkingForLValueToRValue(E);
   
   // Loading a __weak object implicitly retains the value, so we need a cleanup to 
   // balance that.
-  if (getLangOpts().ObjCAutoRefCount &&
-      E->getType().getObjCLifetime() == Qualifiers::OCL_Weak)
+  if (E->getType().getObjCLifetime() == Qualifiers::OCL_Weak)
     Cleanup.setExprNeedsCleanups(true);
 
   ExprResult Res = ImplicitCastExpr::Create(Context, T, CK_LValueToRValue, E,
                                             nullptr, VK_RValue);
 
   // C11 6.3.2.1p2:
   //   ... if the lvalue has atomic type, the value has the non-atomic version 
   //   of the type of the lvalue ...
if (IFace && (IV = IFace->lookupInstanceVariable(II, ClassDeclared))) {
       if (MF != OMF_init && MF != OMF_dealloc && MF != OMF_finalize &&
           !IvarBacksCurrentMethodAccessor(IFace, CurMethod, IV))
         Diag(Loc, diag::warn_direct_ivar_access) << IV->getDeclName();
 
       ObjCIvarRefExpr *Result = new (Context)
           ObjCIvarRefExpr(IV, IV->getUsageType(SelfExpr.get()->getType()), Loc,
                           IV->getLocation(), SelfExpr.get(), true, true);
 
-      if (getLangOpts().ObjCAutoRefCount) {
-        if (IV->getType().getObjCLifetime() == Qualifiers::OCL_Weak) {
-          if (!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Loc))
-            recordUseOfEvaluatedWeak(Result);
-        }
+      if (getLangOpts().ObjCAutoRefCount) {
         if (CurContext->isClosure())
           Diag(Loc, diag::warn_implicitly_retains_self)
             << FixItHint::CreateInsertion(Loc, "self->");
       }
       
       return Result;
     }
   } else if (CurMethod->isInstanceMethod()) {
     // We should warn if a local variable hides an ivar.
if (ConvTy == Compatible) {
       if (LHSType.getObjCLifetime() == Qualifiers::OCL_Strong) {
         // Warn about retain cycles where a block captures the LHS, but
         // not if the LHS is a simple variable into which the block is
         // being stored...unless that variable can be captured by reference!
         const Expr *InnerLHS = LHSExpr->IgnoreParenCasts();
         const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(InnerLHS);
         if (!DRE || DRE->getDecl()->hasAttr<BlocksAttr>())
           checkRetainCycles(LHSExpr, RHS.get());
+      }
 
+      if (LHSType.getObjCLifetime() == Qualifiers::OCL_Strong ||
+          LHSType.isNonWeakInMRRWithObjCWeak(Context)) {
         // It is safe to assign a weak reference into a strong variable.
         // Although this code can still have problems:
         //   id x = self.weakProp;
         //   id y = self.weakProp;
         // we do not warn to warn spuriously when 'x' and 'y' are on separate
         // paths through the function. This should be revisited if
         // -Wrepeated-use-of-weak is made flow-sensitive.
+        // For ObjCWeak only, we do not warn if the assign is to a non-weak
+        // variable, which will be valid for the current autorelease scope.
         if (!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak,
                              RHS.get()->getLocStart()))
           getCurFunction()->markSafeWeakUse(RHS.get());
 
-      } else if (getLangOpts().ObjCAutoRefCount) {
+      } else if (getLangOpts().ObjCAutoRefCount || getLangOpts().ObjCWeak) {
         checkUnsafeExprAssigns(Loc, LHSExpr, RHS.get());
       }
     }
   } else {
     // Compound assignment "x += y"
     ConvTy = CheckAssignmentConstraints(Loc, LHSType, RHSType);
   }
 

lib/Sema/SemaExprMember.cpp

if (IV->getAccessControl() != ObjCIvarDecl::Public &&
               << IV->getDeclName();
         } else if (!IDecl->isSuperClassOf(ClassOfMethodDecl))
           // @protected
           S.Diag(MemberLoc, diag::err_protected_ivar_access)
               << IV->getDeclName();
       }
     }
     bool warn = true;
-    if (S.getLangOpts().ObjCAutoRefCount) {
+    if (S.getLangOpts().ObjCWeak) {
       Expr *BaseExp = BaseExpr.get()->IgnoreParenImpCasts();
       if (UnaryOperator *UO = dyn_cast<UnaryOperator>(BaseExp))
         if (UO->getOpcode() == UO_Deref)
           BaseExp = UO->getSubExpr()->IgnoreParenCasts();
       
       if (DeclRefExpr *DE = dyn_cast<DeclRefExpr>(BaseExp))
         if (DE->getType().getObjCLifetime() == Qualifiers::OCL_Weak) {
           S.Diag(DE->getLocation(), diag::err_arc_weak_ivar_access);
if (warn) {
       }
       if (warn)
         S.Diag(MemberLoc, diag::warn_direct_ivar_access) << IV->getDeclName();
     }
 
     ObjCIvarRefExpr *Result = new (S.Context) ObjCIvarRefExpr(
         IV, IV->getUsageType(BaseType), MemberLoc, OpLoc, BaseExpr.get(),
         IsArrow);
 
-    if (S.getLangOpts().ObjCAutoRefCount) {
-      if (IV->getType().getObjCLifetime() == Qualifiers::OCL_Weak) {
-        if (!S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, MemberLoc))
-          S.recordUseOfEvaluatedWeak(Result);
-      }
-    }
 
     return Result;
   }
 
   // Objective-C property access.
   const ObjCObjectPointerType *OPT;
   if (!IsArrow && (OPT = BaseType->getAs<ObjCObjectPointerType>())) {
     if (!SS.isEmpty() && !SS.isInvalid()) {

lib/Sema/SemaExprObjC.cpp

if (Result->getMethodFamily() == OMF_init &&
         Result->setDelegateInitCall(true);
         return Result;
       }
     }
 
     // In ARC, check for message sends which are likely to introduce
     // retain cycles.
     checkRetainCycles(Result);
+  }
 
+  if (getLangOpts().ObjCWeak) {
     if (!isImplicit && Method) {
       if (const ObjCPropertyDecl *Prop = Method->findPropertyDecl()) {
         bool IsWeak =
           Prop->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak;
         if (!IsWeak && Sel.isUnarySelector())
           IsWeak = ReturnType.getObjCLifetime() & Qualifiers::OCL_Weak;
         if (IsWeak &&
             !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, LBracLoc))

lib/Sema/SemaPseudoObject.cpp

if (RefExpr->isExplicitProperty() && result.get()->isRValue()) {
     QualType propType = RefExpr->getExplicitProperty()
                           ->getUsageType(receiverType);
     if (result.get()->getType()->isObjCIdType()) {
       if (const ObjCObjectPointerType *ptr
             = propType->getAs<ObjCObjectPointerType>()) {
         if (!ptr->isObjCIdType())
           result = S.ImpCastExprToType(result.get(), propType, CK_BitCast);
       }
     }
-    if (S.getLangOpts().ObjCAutoRefCount) {
-      Qualifiers::ObjCLifetime LT = propType.getObjCLifetime();
-      if (LT == Qualifiers::OCL_Weak)
-        if (!S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, RefExpr->getLocation()))
-              S.getCurFunction()->markSafeWeakUse(RefExpr);
-    }
-  }
+    if (propType.getObjCLifetime() == Qualifiers::OCL_Weak &&
+        !S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak,
+                           RefExpr->getLocation()))
 
   return result;
 }
 
 /// Try to build this as a call to a getter that returns a reference.
 ///
 /// \return true if it was possible, whether or not it actually
 ///   succeeded
S.Diag(opcLoc, diag::err_nogetter_property_incdec)
       << op->getSourceRange();
     return ExprError();
   }
 
   return PseudoOpBuilder::buildIncDecOperation(Sc, opcLoc, opcode, op);
 }
 
 ExprResult ObjCPropertyOpBuilder::complete(Expr *SyntacticForm) {
-  if (S.getLangOpts().ObjCAutoRefCount && isWeakProperty() &&
+  if (isWeakProperty() &&
       !S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak,
                          SyntacticForm->getLocStart()))
-      S.recordUseOfEvaluatedWeak(SyntacticRefExpr,
-                                 SyntacticRefExpr->isMessagingGetter());
 
   return PseudoOpBuilder::complete(SyntacticForm);
 }
 
 // ObjCSubscript build stuff.
 //
 
 /// objective-c subscripting-specific behavior for doing lvalue-to-rvalue 

test/SemaObjC/arc-repeated-weak.mm

 // RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-arc -fblocks -Wno-objc-root-class -std=c++11 -Warc-repeated-use-of-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-weak -fblocks -Wno-objc-root-class -std=c++11 -Warc-repeated-use-of-weak -verify %s
 
 @interface Test {
 @public
   Test *ivar;
   __weak id weakIvar;
 }
 @property(weak) Test *weakProp;
 @property(strong) Test *strongProp;

 @end
 
 // This used to crash in WeakObjectProfileTy::getBaseInfo when getBase() was
 // called on an ObjCPropertyRefExpr object whose receiver was an interface.
 
 @class NSString;
 @interface NSBundle
 +(NSBundle *)foo;
-@property (class) NSBundle *foo2;
-@property NSString *prop;
+@property (class, strong) NSBundle *foo2;
+@property (strong) NSString *prop;
 @property(weak) NSString *weakProp;
 @end
 
 @interface NSBundle2 : NSBundle
 @end
 
 void foo() {
   NSString * t = NSBundle.foo.prop;
   use(NSBundle.foo.weakProp); // expected-warning{{weak property 'weakProp' may be accessed multiple times}}
   use(NSBundle2.foo.weakProp); // expected-note{{also accessed here}}
 
   NSString * t2 = NSBundle.foo2.prop;
   use(NSBundle.foo2.weakProp); // expected-warning{{weak property 'weakProp' may be accessed multiple times}}
   use(NSBundle2.foo2.weakProp); // expected-note{{also accessed here}}
 }
 
 // This used to crash in the constructor of WeakObjectProfileTy when a
 // DeclRefExpr was passed that didn't reference a VarDecl.
 
 typedef INTF * INTFPtrTy;
 
 enum E {
   e1
 };
 
 void foo1() {
-  INTFPtrTy tmp = (INTFPtrTy)e1; // expected-error{{cast of 'E' to 'INTFPtrTy' (aka 'INTF *') is disallowed with ARC}}
+  INTFPtrTy tmp = (INTFPtrTy)e1;
+#if __has_feature(objc_arc)
+// expected-error@-2{{cast of 'E' to 'INTFPtrTy' (aka 'INTF *') is disallowed with ARC}}
+#endif
 }