[Objective-C] Miscellaneous -fobjc-weak Fixes

[bkelley]

Summary

After examining the remaining uses of LangOptions.ObjCAutoRefCount, found a some additional places to also check for ObjCWeak not covered by previous test cases. Added a test file to verify all the code paths that were changed.

[rjmccall]

Same thing about parts of this patch — please try to just check T.hasNonTrivialObjCLifetime() when reasonable.

Thank you for all this, by the way. :)

[bkelley]

Thanks for all the feedback! I think things are looking a lot better now.

[bkelley]

Integrated feedback from @rjmccall

[rjmccall]

lib/AST/Type.cpp

3773 ↗

(On Diff #92220)

Is this method not identical in behavior to hasNonTrivialObjCLifetime()?

[bkelley]

lib/AST/Type.cpp

3773 ↗

(On Diff #92220)

Yes, but I didn't see how to extract that information, which is on the internal QualType. But after examining the usage of this function in SemaInit.cpp:6681, I think we need something similar to the isObjCLifetimeType() implementation above.

[rjmccall]

lib/Sema/SemaInit.cpp

6676–6677

Oh, I see what's happening here. The special case for ARC is actually wholly unnecessary. The code's been written as if MTE->getType() for an ARC temporary will just have type "id". In fact, it'll have a lifetime-qualified type, dependent on the type of the reference which has been bound to it. So we need to mark that there's a temporary cleanup if the type has non-trivial lifetime, i.e. if MTE->getType().hasNonTrivialObjCLifetime(); but this is already considered as part of MTE->getType().isDestructedType(), and the storage-duration condition there is correct as well. So really we should just delete this entire clause.

That should eliminate the need for Type::isNonTrivialObjCLifetimeType(). In general, we don't want methods like that to ever exist: lifetime is determined by qualifiers, and querying a Type instead of a QualType has the potential to implicitly remove qualifiers.

[bkelley]

Updated with latest feedback from @rjmccall

lib/Sema/SemaInit.cpp

6676–6677

Thanks for looking in to this and for the explanation. I've removed the check and the changes to Type.

[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!

lib/Sema/SemaCast.cpp

if (SrcExpr.get()->getType() == Self.Context.OverloadTy) {
       Self.NoteAllOverloadCandidates(SrcExpr.get());
 
     } else {
       diagnoseBadCast(Self, msg, CT_Reinterpret, OpRange, SrcExpr.get(),
                       DestType, /*listInitialization=*/false);
     }
     SrcExpr = ExprError();
   } else if (tcr == TC_Success) {
-    if (Self.getLangOpts().ObjCAutoRefCount)
+    if (Self.getLangOpts().allowsNonTrivialObjCLifetimeQualifiers())
       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
if (tcr != TC_Success && msg != 0) {
     } else {
       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)
+    if (Self.getLangOpts().allowsNonTrivialObjCLifetimeQualifiers())
       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

lib/Sema/SemaDecl.cpp

for (const auto *I : Record->decls()) {
         if (IFD->getDeclName())
           ++NumNamedMembers;
     }
   }
 
   // Verify that all the fields are okay.
   SmallVector<FieldDecl*, 32> RecFields;
 
-  bool ARCErrReported = false;
+  bool ObjCFieldLifetimeErrReported = false;
   for (ArrayRef<Decl *>::iterator i = Fields.begin(), end = Fields.end();
        i != end; ++i) {
     FieldDecl *FD = cast<FieldDecl>(*i);
 
     // Get the type for the field.
     const Type *FDTy = FD->getType().getTypePtr();
 
     if (!FD->isAnonymousStructOrUnion()) {
if (FDTy->isFunctionType()) {
       if (Record && FDTTy->getDecl()->hasVolatileMember())
         Record->setHasVolatileMember(true);
     } else if (FDTy->isObjCObjectType()) {
       /// A field cannot be an Objective-c object
       Diag(FD->getLocation(), diag::err_statically_allocated_object)
         << FixItHint::CreateInsertion(FD->getLocation(), "*");
       QualType T = Context.getObjCObjectPointerType(FD->getType());
       FD->setType(T);
-    } else if (getLangOpts().ObjCAutoRefCount && Record && !ARCErrReported &&
+    } else if (getLangOpts().allowsNonTrivialObjCLifetimeQualifiers() &&
+               Record && !ObjCFieldLifetimeErrReported &&
                (!getLangOpts().CPlusPlus || Record->isUnion())) {
-      // It's an error in ARC if a field has lifetime.
+      // It's an error in ARC or Weak if a field has lifetime.
       // We don't want to report this in a system header, though,
       // so we just make the field unavailable.
       // FIXME: that's really not sufficient; we need to make the type
       // itself invalid to, say, initialize or copy.
       QualType T = FD->getType();
-      Qualifiers::ObjCLifetime lifetime = T.getObjCLifetime();
-      if (lifetime && lifetime != Qualifiers::OCL_ExplicitNone) {
+      if (T.hasNonTrivialObjCLifetime()) {
         SourceLocation loc = FD->getLocation();
         if (getSourceManager().isInSystemHeader(loc)) {
           if (!FD->hasAttr<UnavailableAttr>()) {
             FD->addAttr(UnavailableAttr::CreateImplicit(Context, "",
                           UnavailableAttr::IR_ARCFieldWithOwnership, loc));
           }
         } else {
           Diag(FD->getLocation(), diag::err_arc_objc_object_in_tag)
             << T->isBlockPointerType() << Record->getTagKind();
         }
-        ARCErrReported = true;
+        ObjCFieldLifetimeErrReported = true;
       }
     } else if (getLangOpts().ObjC1 &&
                getLangOpts().getGC() != LangOptions::NonGC &&
                Record && !Record->hasObjectMember()) {
       if (FD->getType()->isObjCObjectPointerType() ||
           FD->getType().isObjCGCStrong())
         Record->setHasObjectMember(true);
       else if (Context.getAsArrayType(FD->getType())) {

lib/Sema/SemaDeclCXX.cpp

if (CSM == Sema::CXXDefaultConstructor && FI->hasInClassInitializer()) {
         S.Diag(FI->getLocation(), diag::note_nontrivial_in_class_init) << FI;
       return false;
     }
 
     // Objective C ARC 4.3.5:
     //   [...] nontrivally ownership-qualified types are [...] not trivially
     //   default constructible, copy constructible, move constructible, copy
     //   assignable, move assignable, or destructible [...]
-    if (S.getLangOpts().ObjCAutoRefCount &&
-        FieldType.hasNonTrivialObjCLifetime()) {
+    if (FieldType.hasNonTrivialObjCLifetime()) {
       if (Diagnose)
         S.Diag(FI->getLocation(), diag::note_nontrivial_objc_ownership)
           << RD << FieldType.getObjCLifetime();
       return false;
     }
 
     bool ConstRHS = ConstArg && !FI->isMutable();
     if (!checkTrivialSubobjectCall(S, FI->getLocation(), FieldType, ConstRHS,

lib/Sema/SemaInit.cpp

case SK_BindReferenceToTemporary: {
       // Maybe lifetime-extend the temporary's subobjects to match the
       // entity's lifetime.
       if (const InitializedEntity *ExtendingEntity =
               getEntityForTemporaryLifetimeExtension(&Entity))
         if (performReferenceExtension(MTE, ExtendingEntity))
           warnOnLifetimeExtension(S, Entity, CurInit.get(),
                                   /*IsInitializerList=*/false,
                                   ExtendingEntity->getDecl());
 
-      // If we're binding to an Objective-C object that has lifetime, we
-      // need cleanups. Likewise if we're extending this temporary to automatic
-      // storage duration -- we need to register its cleanup during the
-      // full-expression's cleanups.
-      if ((S.getLangOpts().ObjCAutoRefCount &&
-           MTE->getType()->isObjCLifetimeType()) ||
-          (MTE->getStorageDuration() == SD_Automatic &&
-           MTE->getType().isDestructedType()))
+      // If we're extending this temporary to automatic storage duration -- we
+      // need to register its cleanup during the full-expression's cleanups.
+      if (MTE->getStorageDuration() == SD_Automatic &&
+          MTE->getType().isDestructedType())
         S.Cleanup.setExprNeedsCleanups(true);
 
       CurInit = MTE;
       break;
     }
 
     case SK_FinalCopy:
       if (checkAbstractType(Step->Type))

test/SemaObjCXX/objc-weak.mm

+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-weak -fblocks -Wno-objc-root-class -std=c++98 -Wno-c++0x-extensions -verify %s
+
+@interface AnObject
+@property(weak) id value;
+@end
+
+__attribute__((objc_arc_weak_reference_unavailable))
+@interface NOWEAK : AnObject // expected-note 2 {{class is declared here}}
+@end
+
+struct S {
+  __weak id a; // expected-note {{because type 'S' has a member with __weak ownership}}
+};
+
+union U {
+  __weak id a; // expected-error {{ARC forbids Objective-C objects in union}}
+  S b;         // expected-error {{union member 'b' has a non-trivial copy constructor}}
+};
+
+void testCast(AnObject *o) {
+  __weak id a = reinterpret_cast<__weak NOWEAK *>(o); // expected-error {{class is incompatible with __weak references}} \
+                                                      // expected-error {{explicit ownership qualifier on cast result has no effect}} \
+                                                      // expected-error {{assignment of a weak-unavailable object to a __weak object}}
+
+  __weak id b = static_cast<__weak NOWEAK *>(o); // expected-error {{class is incompatible with __weak references}} \
+                                                 // expected-error {{explicit ownership qualifier on cast result has no effect}} \
+                                                 // expected-error {{assignment of a weak-unavailable object to a __weak object}}
+}