[arcmt] More automatic transformations and safety improvements; rdar://9615812 :

- Replace calling -zone with 'nil'. -zone is obsolete in ARC.
- Allow removing retain/release on a static global var.
- Fix assertion hit when scanning for name references outside a NSAutoreleasePool scope.
- Automatically add bridged casts for results of objc method calls and when calling CFRetain, for example:

NSString *s;
CFStringRef ref = [s string];   -> CFStringRef ref = (__bridge CFStringRef)([s string]);
ref = s.string;                 -> ref = (__bridge CFStringRef)(s.string);
ref = [NSString new];           -> ref = (__bridge_retained CFStringRef)([NSString new]);
ref = [s newString];            -> ref = (__bridge_retained CFStringRef)([s newString]);
ref = [[NSString alloc] init];  -> ref = (__bridge_retained CFStringRef)([[NSString alloc] init]);
ref = [[s string] retain];      -> ref = (__bridge_retained CFStringRef)([s string]);
ref = CFRetain(s);              -> ref = (__bridge_retained CFTypeRef)(s);
ref = [s retain];               -> ref = (__bridge_retained CFStringRef)(s);

- Emit migrator error when trying to cast to CF type the result of autorelease/release:
  for

CFStringRef f3() {
  return (CFStringRef)[[[NSString alloc] init] autorelease];
}

emits:

t.m:12:10: error: [rewriter] it is not safe to cast to 'CFStringRef' the result of 'autorelease' message; a __bridge cast may result in a pointer to a destroyed object and a __bridge_retained may leak the object
  return (CFStringRef)[[[NSString alloc] init] autorelease];
         ^            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
t.m:12:3: note: [rewriter] remove the cast and change return type of function to 'NSString *' to have the object automatically autoreleased
  return (CFStringRef)[[[NSString alloc] init] autorelease];
  ^

- Before changing attributes to weak/unsafe_unretained, check if the backing ivar
  is set to a +1 object, in which case use 'strong' instead.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@136208 91177308-0d34-0410-b5e6-96231b3b80d8

Author: akyrtzi

include/clang/AST/ParentMap.h

@@ -32,6 +32,7 @@ class ParentMap {
   Stmt *getParent(Stmt*) const;
   Stmt *getParentIgnoreParens(Stmt *) const;
   Stmt *getParentIgnoreParenCasts(Stmt *) const;
+  Stmt *getParentIgnoreParenImpCasts(Stmt *) const;
   Stmt *getOuterParenParent(Stmt *) const;
 
   const Stmt *getParent(const Stmt* S) const {

lib/ARCMigrate/TransAPIUses.cpp

@@ -9,17 +9,19 @@
 //
 // checkAPIUses:
 //
-// Emits error with some API uses that are not safe in ARC mode:
+// Emits error/fix with some API uses that are obsolete or not safe in ARC mode:
 //
 // - NSInvocation's [get/set]ReturnValue and [get/set]Argument are only safe
 //   with __unsafe_unretained objects.
 // - When a NSData's 'bytes' family of methods are used on a local var,
 //   add __attribute__((objc_precise_lifetime)) to make it safer.
+// - Calling -zone gets replaced with 'nil'.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Transforms.h"
 #include "Internals.h"
+#include "clang/Sema/SemaDiagnostic.h"
 
 using namespace clang;
 using namespace arcmt;
@@ -35,6 +37,8 @@ class APIChecker : public RecursiveASTVisitor<APIChecker> {
 
   Selector bytesSel, getBytesSel, getBytesLengthSel, getBytesRangeSel;
 
+  Selector zoneSel;
+
   llvm::DenseSet<VarDecl *> ChangedNSDataVars;
 public:
   APIChecker(MigrationPass &pass) : Pass(pass) {
@@ -57,9 +61,12 @@ class APIChecker : public RecursiveASTVisitor<APIChecker> {
     getBytesLengthSel = sels.getSelector(2, selIds);
     selIds[1] = &ids.get("range");
     getBytesRangeSel = sels.getSelector(2, selIds);
+
+    zoneSel = sels.getNullarySelector(&ids.get("zone"));
   }
 
   bool VisitObjCMessageExpr(ObjCMessageExpr *E) {
+    // NSInvocation.
     if (E->isInstanceMessage() &&
         E->getReceiverInterface() &&
         E->getReceiverInterface()->getName() == "NSInvocation") {
@@ -91,6 +98,7 @@ class APIChecker : public RecursiveASTVisitor<APIChecker> {
       return true;
     }
 
+    // NSData.
     if (E->isInstanceMessage() &&
         E->getReceiverInterface() &&
         E->getReceiverInterface()->getName() == "NSData" &&
@@ -111,6 +119,20 @@ class APIChecker : public RecursiveASTVisitor<APIChecker> {
           }
     }
 
+    // -zone.
+    if (E->isInstanceMessage() &&
+        E->getInstanceReceiver() &&
+        E->getSelector() == zoneSel &&
+        Pass.TA.hasDiagnostic(diag::err_unavailable,
+                              diag::err_unavailable_message,
+                              E->getInstanceReceiver()->getExprLoc())) {
+      // Calling -zone is meaningless in ARC, change it to nil.
+      Transaction Trans(Pass.TA);
+      Pass.TA.clearDiagnostic(diag::err_unavailable,
+                              diag::err_unavailable_message,
+                              E->getInstanceReceiver()->getExprLoc());
+      Pass.TA.replace(E->getSourceRange(), getNilString(Pass.Ctx));
+    }
     return true;
   }
 };

lib/ARCMigrate/TransAutoreleasePool.cpp

@@ -286,6 +286,9 @@ class AutoreleasePoolRewriter
     }
 
     bool isInScope(SourceLocation loc) {
+      if (loc.isInvalid())
+        return false;
+
       SourceManager &SM = Ctx.getSourceManager();
       if (SM.isBeforeInTranslationUnit(loc, ScopeRange.getBegin()))
         return false;

lib/ARCMigrate/TransProperties.cpp

@@ -45,6 +45,7 @@ namespace {
 
 class PropertiesRewriter {
   MigrationPass &Pass;
+  ObjCImplementationDecl *CurImplD;
 
   struct PropData {
     ObjCPropertyDecl *PropD;
@@ -62,6 +63,7 @@ class PropertiesRewriter {
   PropertiesRewriter(MigrationPass &pass) : Pass(pass) { }
 
   void doTransform(ObjCImplementationDecl *D) {
+    CurImplD = D;
     ObjCInterfaceDecl *iface = D->getClassInterface();
     if (!iface)
       return;
@@ -134,8 +136,16 @@ class PropertiesRewriter {
       return;
     }
 
-    if (propAttrs & ObjCPropertyDecl::OBJC_PR_assign)
+    if (propAttrs & ObjCPropertyDecl::OBJC_PR_assign) {
+      if (hasIvarAssignedAPlusOneObject(props)) {
+        rewriteAttribute("assign", "strong", atLoc);
+        return;
+      }
       return rewriteAssign(props, atLoc);
+    }
+
+    if (hasIvarAssignedAPlusOneObject(props))
+      return maybeAddStrongAttr(props, atLoc);
 
     return maybeAddWeakOrUnsafeUnretainedAttr(props, atLoc);
   }
@@ -162,15 +172,15 @@ class PropertiesRewriter {
   void maybeAddWeakOrUnsafeUnretainedAttr(PropsTy &props,
                                           SourceLocation atLoc) const {
     ObjCPropertyDecl::PropertyAttributeKind propAttrs = getPropertyAttrs(props);
-    if ((propAttrs & ObjCPropertyDecl::OBJC_PR_readonly) &&
-        hasNoBackingIvars(props))
-      return;
 
     bool canUseWeak = canApplyWeak(Pass.Ctx, getPropertyType(props));
-    bool addedAttr = addAttribute(canUseWeak ? "weak" : "unsafe_unretained",
-                                  atLoc);
-    if (!addedAttr)
-      canUseWeak = false;
+    if (!(propAttrs & ObjCPropertyDecl::OBJC_PR_readonly) ||
+        !hasAllIvarsBacked(props)) {
+      bool addedAttr = addAttribute(canUseWeak ? "weak" : "unsafe_unretained",
+                                    atLoc);
+      if (!addedAttr)
+        canUseWeak = false;
+    }
 
     for (PropsTy::iterator I = props.begin(), E = props.end(); I != E; ++I) {
       if (isUserDeclared(I->IvarD))
@@ -186,6 +196,25 @@ class PropertiesRewriter {
     }
   }
 
+  void maybeAddStrongAttr(PropsTy &props, SourceLocation atLoc) const {
+    ObjCPropertyDecl::PropertyAttributeKind propAttrs = getPropertyAttrs(props);
+
+    if (!(propAttrs & ObjCPropertyDecl::OBJC_PR_readonly) ||
+        !hasAllIvarsBacked(props)) {
+      addAttribute("strong", atLoc);
+    }
+
+    for (PropsTy::iterator I = props.begin(), E = props.end(); I != E; ++I) {
+      if (I->ImplD) {
+        Pass.TA.clearDiagnostic(diag::err_arc_assign_property_ownership,
+                                I->ImplD->getLocation());
+        Pass.TA.clearDiagnostic(
+                           diag::err_arc_objc_property_default_assign_on_object,
+                           I->ImplD->getLocation());
+      }
+    }
+  }
+
   bool rewriteAttribute(StringRef fromAttr, StringRef toAttr,
                         SourceLocation atLoc) const {
     if (atLoc.isMacroID())
@@ -290,6 +319,40 @@ class PropertiesRewriter {
     return true;
   }
 
+  class PlusOneAssign : public RecursiveASTVisitor<PlusOneAssign> {
+    ObjCIvarDecl *Ivar;
+  public:
+    PlusOneAssign(ObjCIvarDecl *D) : Ivar(D) {}
+
+    bool VisitBinAssign(BinaryOperator *E) {
+      Expr *lhs = E->getLHS()->IgnoreParenImpCasts();
+      if (ObjCIvarRefExpr *RE = dyn_cast<ObjCIvarRefExpr>(lhs)) {
+        if (RE->getDecl() != Ivar)
+          return true;
+
+      ImplicitCastExpr *implCE = dyn_cast<ImplicitCastExpr>(E->getRHS());
+      while (implCE && implCE->getCastKind() ==  CK_BitCast)
+        implCE = dyn_cast<ImplicitCastExpr>(implCE->getSubExpr());
+
+      if (implCE && implCE->getCastKind() == CK_ObjCConsumeObject)
+        return false;
+      }
+
+      return true;
+    }
+  };
+
+  bool hasIvarAssignedAPlusOneObject(PropsTy &props) const {
+    for (PropsTy::iterator I = props.begin(), E = props.end(); I != E; ++I) {
+      PlusOneAssign oneAssign(I->IvarD);
+      bool notFound = oneAssign.TraverseDecl(CurImplD);
+      if (!notFound)
+        return true;
+    }
+
+    return false;
+  }
+
   bool hasIvarWithExplicitOwnership(PropsTy &props) const {
     for (PropsTy::iterator I = props.begin(), E = props.end(); I != E; ++I) {
       if (isUserDeclared(I->IvarD)) {
@@ -304,9 +367,9 @@ class PropertiesRewriter {
     return false;    
   }
 
-  bool hasNoBackingIvars(PropsTy &props) const {
+  bool hasAllIvarsBacked(PropsTy &props) const {
     for (PropsTy::iterator I = props.begin(), E = props.end(); I != E; ++I)
-      if (I->IvarD)
+      if (!isUserDeclared(I->IvarD))
         return false;
 
     return true;

lib/ARCMigrate/TransRetainReleaseDealloc.cpp

@@ -129,10 +129,9 @@ class RetainReleaseDeallocRemover :
       // Change the -release to "receiver = nil" in a finally to avoid a leak
       // when an exception is thrown.
       Pass.TA.replace(E->getSourceRange(), rec->getSourceRange());
-      if (Pass.Ctx.Idents.get("nil").hasMacroDefinition())
-        Pass.TA.insertAfterToken(rec->getLocEnd(), " = nil");
-      else
-        Pass.TA.insertAfterToken(rec->getLocEnd(), " = 0");
+      std::string str = " = ";
+      str += getNilString(Pass.Ctx);
+      Pass.TA.insertAfterToken(rec->getLocEnd(), str);
       return true;
     }
 

lib/ARCMigrate/TransUnbridgedCasts.cpp

@@ -36,6 +36,7 @@
 #include "Internals.h"
 #include "clang/Analysis/DomainSpecific/CocoaConventions.h"
 #include "clang/Sema/SemaDiagnostic.h"
+#include "clang/AST/ParentMap.h"
 #include "clang/Basic/SourceManager.h"
 
 using namespace clang;
@@ -47,11 +48,18 @@ namespace {
 class UnbridgedCastRewriter : public RecursiveASTVisitor<UnbridgedCastRewriter>{
   MigrationPass &Pass;
   IdentifierInfo *SelfII;
+  llvm::OwningPtr<ParentMap> StmtMap;
+
 public:
   UnbridgedCastRewriter(MigrationPass &pass) : Pass(pass) {
     SelfII = &Pass.Ctx.Idents.get("self");
   }
 
+  void transformBody(Stmt *body) {
+    StmtMap.reset(new ParentMap(body));
+    TraverseStmt(body);
+  }
+
   bool VisitCastExpr(CastExpr *E) {
     if (E->getCastKind() != CK_AnyPointerToObjCPointerCast
         && E->getCastKind() != CK_BitCast)
@@ -138,13 +146,21 @@ class UnbridgedCastRewriter : public RecursiveASTVisitor<UnbridgedCastRewriter>{
   }
 
   void rewriteToBridgedCast(CastExpr *E, ObjCBridgeCastKind Kind) {
+    Transaction Trans(Pass.TA);
+    rewriteToBridgedCast(E, Kind, Trans);
+  }
+
+  void rewriteToBridgedCast(CastExpr *E, ObjCBridgeCastKind Kind,
+                            Transaction &Trans) {
     TransformActions &TA = Pass.TA;
 
     // We will remove the compiler diagnostic.
     if (!TA.hasDiagnostic(diag::err_arc_mismatched_cast,
                           diag::err_arc_cast_requires_bridge,
-                          E->getLocStart()))
+                          E->getLocStart())) {
+      Trans.abort();
       return;
+    }
 
     StringRef bridge;
     switch(Kind) {
@@ -156,7 +172,6 @@ class UnbridgedCastRewriter : public RecursiveASTVisitor<UnbridgedCastRewriter>{
       bridge = "__bridge_retained "; break;
     }
 
-    Transaction Trans(TA);
     TA.clearDiagnostic(diag::err_arc_mismatched_cast,
                        diag::err_arc_cast_requires_bridge,
                        E->getLocStart());
@@ -180,23 +195,90 @@ class UnbridgedCastRewriter : public RecursiveASTVisitor<UnbridgedCastRewriter>{
     }
   }
 
+  void rewriteCastForCFRetain(CastExpr *castE, CallExpr *callE) {
+    Transaction Trans(Pass.TA);
+    Pass.TA.replace(callE->getSourceRange(), callE->getArg(0)->getSourceRange());
+    rewriteToBridgedCast(castE, OBC_BridgeRetained, Trans);
+  }
+
   void transformObjCToNonObjCCast(CastExpr *E) {
     if (isSelf(E->getSubExpr()))
       return rewriteToBridgedCast(E, OBC_Bridge);
+
+    CallExpr *callE;
+    if (isPassedToCFRetain(E, callE))
+      return rewriteCastForCFRetain(E, callE);
+
+    ObjCMethodFamily family = getFamilyOfMessage(E->getSubExpr());
+    if (family == OMF_retain)
+      return rewriteToBridgedCast(E, OBC_BridgeRetained);
+
+    if (family == OMF_autorelease || family == OMF_release) {
+      std::string err = "it is not safe to cast to '";
+      err += E->getType().getAsString(Pass.Ctx.PrintingPolicy);
+      err += "' the result of '";
+      err += family == OMF_autorelease ? "autorelease" : "release";
+      err += "' message; a __bridge cast may result in a pointer to a "
+          "destroyed object and a __bridge_retained may leak the object";
+      Pass.TA.reportError(err, E->getLocStart(),
+                          E->getSubExpr()->getSourceRange());
+      Stmt *parent = E;
+      do {
+        parent = StmtMap->getParentIgnoreParenImpCasts(parent);
+      } while (parent && isa<ExprWithCleanups>(parent));
+
+      if (ReturnStmt *retS = dyn_cast_or_null<ReturnStmt>(parent)) {
+        std::string note = "remove the cast and change return type of function "
+            "to '";
+        note += E->getSubExpr()->getType().getAsString(Pass.Ctx.PrintingPolicy);
+        note += "' to have the object automatically autoreleased";
+        Pass.TA.reportNote(note, retS->getLocStart());
+      }
+    }
+
+    if (ImplicitCastExpr *implCE = dyn_cast<ImplicitCastExpr>(E->getSubExpr())){
+      if (implCE->getCastKind() == CK_ObjCConsumeObject)
+        return rewriteToBridgedCast(E, OBC_BridgeRetained);
+      if (implCE->getCastKind() == CK_ObjCReclaimReturnedObject)
+        return rewriteToBridgedCast(E, OBC_Bridge);
+    }
+  }
+
+  static ObjCMethodFamily getFamilyOfMessage(Expr *E) {
+    E = E->IgnoreParenCasts();
+    if (ObjCMessageExpr *ME = dyn_cast<ObjCMessageExpr>(E))
+      return ME->getMethodFamily();
+
+    return OMF_None;
+  }
+
+  bool isPassedToCFRetain(Expr *E, CallExpr *&callE) const {
+    if ((callE = dyn_cast_or_null<CallExpr>(
+                                     StmtMap->getParentIgnoreParenImpCasts(E))))
+      if (FunctionDecl *
+            FD = dyn_cast_or_null<FunctionDecl>(callE->getCalleeDecl()))
+        if (FD->getName() == "CFRetain" && FD->getNumParams() == 1 &&
+            FD->getParent()->isTranslationUnit() &&
+            FD->getLinkage() == ExternalLinkage)
+          return true;
+
+    return false;
   }
 
-  bool isSelf(Expr *E) {
+  bool isSelf(Expr *E) const {
     E = E->IgnoreParenLValueCasts();
     if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
-      if (DRE->getDecl()->getIdentifier() == SelfII)
-        return true;
+      if (ImplicitParamDecl *IPD = dyn_cast<ImplicitParamDecl>(DRE->getDecl()))
+        if (IPD->getIdentifier() == SelfII)
+          return true;
+
     return false;
   }
 };
 
 } // end anonymous namespace
 
 void trans::rewriteUnbridgedCasts(MigrationPass &pass) {
-  UnbridgedCastRewriter trans(pass);
+  BodyTransform<UnbridgedCastRewriter> trans(pass);
   trans.TraverseDecl(pass.Ctx.getTranslationUnitDecl());
 }