[arcmt] Remove an unused -autorelease, without failing with error, for this

idiom that is used commonly in setters:

   [backingValue autorelease];
   backingValue = [newValue retain]; // in general a +1 assign

rdar://9914061

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

10 files changed, 203 insertions, 70 deletions

diff --git a/lib/ARCMigrate/TransProperties.cpp b/lib/ARCMigrate/TransProperties.cpp
index 8f65fc3a8b..5ec918fe93 100644
--- a/lib/ARCMigrate/TransProperties.cpp
+++ b/lib/ARCMigrate/TransProperties.cpp
@@ -309,17 +309,8 @@ private:
         if (RE->getDecl() != Ivar)
           return true;
 
-      if (ObjCMessageExpr *
-            ME = dyn_cast<ObjCMessageExpr>(E->getRHS()->IgnoreParenCasts()))
-        if (ME->getMethodFamily() == OMF_retain)
+        if (isPlusOneAssign(E))
           return false;
-
-      ImplicitCastExpr *implCE = dyn_cast<ImplicitCastExpr>(E->getRHS());
-      while (implCE && implCE->getCastKind() ==  CK_BitCast)
-        implCE = dyn_cast<ImplicitCastExpr>(implCE->getSubExpr());
-
-      if (implCE && implCE->getCastKind() == CK_ARCConsumeObject)
-        return false;
       }
 
       return true;
diff --git a/lib/ARCMigrate/TransRetainReleaseDealloc.cpp b/lib/ARCMigrate/TransRetainReleaseDealloc.cpp
index 11a6553341..df3cd5858e 100644
--- a/lib/ARCMigrate/TransRetainReleaseDealloc.cpp
+++ b/lib/ARCMigrate/TransRetainReleaseDealloc.cpp
@@ -64,14 +64,16 @@ public:
       return true;
     case OMF_autorelease:
       if (isRemovable(E)) {
-        // An unused autorelease is badness. If we remove it the receiver
-        // will likely die immediately while previously it was kept alive
-        // by the autorelease pool. This is bad practice in general, leave it
-        // and emit an error to force the user to restructure his code.
-        Pass.TA.reportError("it is not safe to remove an unused 'autorelease' "
-            "message; its receiver may be destroyed immediately",
-            E->getLocStart(), E->getSourceRange());
-        return true;
+        if (!isCommonUnusedAutorelease(E)) {
+          // An unused autorelease is badness. If we remove it the receiver
+          // will likely die immediately while previously it was kept alive
+          // by the autorelease pool. This is bad practice in general, leave it
+          // and emit an error to force the user to restructure his code.
+          Pass.TA.reportError("it is not safe to remove an unused 'autorelease' "
+              "message; its receiver may be destroyed immediately",
+              E->getLocStart(), E->getSourceRange());
+          return true;
+        }
       }
       // Pass through.
     case OMF_retain:
@@ -156,6 +158,80 @@ public:
   }
 
 private:
+  /// \brief Checks for idioms where an unused -autorelease is common.
+  ///
+  /// Currently only returns true for this idiom which is common in property
+  /// setters:
+  ///
+  ///   [backingValue autorelease];
+  ///   backingValue = [newValue retain]; // in general a +1 assign
+  ///
+  bool isCommonUnusedAutorelease(ObjCMessageExpr *E) {
+    Expr *Rec = E->getInstanceReceiver();
+    if (!Rec)
+      return false;
+
+    Decl *RefD = getReferencedDecl(Rec);
+    if (!RefD)
+      return false;
+
+    Stmt *OuterS = E, *InnerS;
+    do {
+      InnerS = OuterS;
+      OuterS = StmtMap->getParent(InnerS);
+    }
+    while (OuterS && (isa<ParenExpr>(OuterS) ||
+                      isa<CastExpr>(OuterS) ||
+                      isa<ExprWithCleanups>(OuterS)));
+    
+    if (!OuterS)
+      return false;
+
+    // Find next statement after the -autorelease.
+
+    Stmt::child_iterator currChildS = OuterS->child_begin();
+    Stmt::child_iterator childE = OuterS->child_end();
+    for (; currChildS != childE; ++currChildS) {
+      if (*currChildS == InnerS)
+        break;
+    }
+    if (currChildS == childE)
+      return false;
+    ++currChildS;
+    if (currChildS == childE)
+      return false;
+
+    Stmt *nextStmt = *currChildS;
+    if (!nextStmt)
+      return false;
+    nextStmt = nextStmt->IgnoreImplicit();
+
+    // Check for "RefD = [+1 retained object];".
+    
+    if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(nextStmt)) {
+      if (RefD != getReferencedDecl(Bop->getLHS()))
+        return false;
+      if (isPlusOneAssign(Bop))
+        return true;
+    }
+    return false;
+  }
+
+  Decl *getReferencedDecl(Expr *E) {
+    if (!E)
+      return 0;
+
+    E = E->IgnoreParenCasts();
+    if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
+      return DRE->getDecl();
+    if (MemberExpr *ME = dyn_cast<MemberExpr>(E))
+      return ME->getMemberDecl();
+    if (ObjCIvarRefExpr *IRE = dyn_cast<ObjCIvarRefExpr>(E))
+      return IRE->getDecl();
+
+    return 0;
+  }
+
   /// \brief Check if the retain/release is due to a GCD/XPC macro that are
   /// defined as:
   ///
diff --git a/lib/ARCMigrate/Transforms.cpp b/lib/ARCMigrate/Transforms.cpp
index 50bea95f12..24b650561c 100644
--- a/lib/ARCMigrate/Transforms.cpp
+++ b/lib/ARCMigrate/Transforms.cpp
@@ -14,6 +14,7 @@
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Analysis/DomainSpecific/CocoaConventions.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/DenseSet.h"
 #include <map>
@@ -56,6 +57,47 @@ bool trans::canApplyWeak(ASTContext &Ctx, QualType type,
   return true;
 }
 
+bool trans::isPlusOneAssign(const BinaryOperator *E) {
+  if (E->getOpcode() != BO_Assign)
+    return false;
+
+  if (const ObjCMessageExpr *
+        ME = dyn_cast<ObjCMessageExpr>(E->getRHS()->IgnoreParenCasts()))
+    if (ME->getMethodFamily() == OMF_retain)
+      return true;
+
+  if (const CallExpr *
+        callE = dyn_cast<CallExpr>(E->getRHS()->IgnoreParenCasts())) {
+    if (const FunctionDecl *FD = callE->getDirectCallee()) {
+      if (FD->getAttr<CFReturnsRetainedAttr>())
+        return true;
+
+      if (FD->isGlobal() &&
+          FD->getIdentifier() &&
+          FD->getParent()->isTranslationUnit() &&
+          FD->getLinkage() == ExternalLinkage &&
+          ento::cocoa::isRefType(callE->getType(), "CF",
+                                 FD->getIdentifier()->getName())) {
+        StringRef fname = FD->getIdentifier()->getName();
+        if (fname.endswith("Retain") ||
+            fname.find("Create") != StringRef::npos ||
+            fname.find("Copy") != StringRef::npos) {
+          return true;
+        }
+      }
+    }
+  }
+
+  const ImplicitCastExpr *implCE = dyn_cast<ImplicitCastExpr>(E->getRHS());
+  while (implCE && implCE->getCastKind() ==  CK_BitCast)
+    implCE = dyn_cast<ImplicitCastExpr>(implCE->getSubExpr());
+
+  if (implCE && implCE->getCastKind() == CK_ARCConsumeObject)
+    return true;
+
+  return false;
+}
+
 /// \brief 'Loc' is the end of a statement range. This returns the location
 /// immediately after the semicolon following the statement.
 /// If no semicolon is found or the location is inside a macro, the returned
diff --git a/lib/ARCMigrate/Transforms.h b/lib/ARCMigrate/Transforms.h
index 445c3e599d..7abc0304b1 100644
--- a/lib/ARCMigrate/Transforms.h
+++ b/lib/ARCMigrate/Transforms.h
@@ -154,6 +154,8 @@ public:
 bool canApplyWeak(ASTContext &Ctx, QualType type,
                   bool AllowOnUnknownClass = false);
 
+bool isPlusOneAssign(const BinaryOperator *E);
+
 /// \brief 'Loc' is the end of a statement range. This returns the location
 /// immediately after the semicolon following the statement.
 /// If no semicolon is found or the location is inside a macro, the returned
diff --git a/test/ARCMT/Common.h b/test/ARCMT/Common.h
index 16856ed1b4..708e407c92 100644
--- a/test/ARCMT/Common.h
+++ b/test/ARCMT/Common.h
@@ -68,3 +68,14 @@ typedef const void* objc_objectptr_t;
 extern __attribute__((ns_returns_retained)) id objc_retainedObject(objc_objectptr_t __attribute__((cf_consumed)) pointer);
 extern __attribute__((ns_returns_not_retained)) id objc_unretainedObject(objc_objectptr_t pointer);
 extern objc_objectptr_t objc_unretainedPointer(id object);
+
+#define dispatch_retain(object) ({ dispatch_object_t _o = (object); _dispatch_object_validate(_o); (void)[_o retain]; })
+#define dispatch_release(object) ({ dispatch_object_t _o = (object); _dispatch_object_validate(_o); [_o release]; })
+#define xpc_retain(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o retain]; })
+#define xpc_release(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o release]; })
+
+typedef id dispatch_object_t;
+typedef id xpc_object_t;
+
+void _dispatch_object_validate(dispatch_object_t object);
+void _xpc_object_validate(xpc_object_t object);
diff --git a/test/ARCMT/autoreleases.m b/test/ARCMT/autoreleases.m
index 3acddb71e7..a131bc5339 100644
--- a/test/ARCMT/autoreleases.m
+++ b/test/ARCMT/autoreleases.m
@@ -3,29 +3,21 @@
 // RUN: diff %t %s.result
 // DISABLE: mingw32
 
-typedef unsigned char BOOL;
+#include "Common.h"
 
-@interface NSObject {
-  id isa;
-}
-+new;
-+alloc;
--init;
--autorelease;
-@end
-
-@interface NSAutoreleasePool : NSObject
-- drain;
-@end
- 
 @interface A : NSObject {
 @package
     id object;
 }
 @end
 
-@interface B : NSObject
+@interface B : NSObject {
+  id _prop;
+  xpc_object_t _xpc_prop;
+}
 - (BOOL)containsSelf:(A*)a;
+@property (retain) id prop;
+@property (retain) xpc_object_t xpc_prop;
 @end
 
 @implementation A
@@ -35,6 +27,26 @@ typedef unsigned char BOOL;
 - (BOOL)containsSelf:(A*)a {
     return a->object == self;
 }
+
+-(id) prop {
+  return _prop;
+}
+-(void) setProp:(id) newVal {
+  [_prop autorelease];
+  _prop = [newVal retain];
+}
+-(void) setProp2:(CFTypeRef) newVal {
+  [_prop autorelease];
+  _prop = (id)CFRetain(newVal);
+}
+
+-(id) xpc_prop {
+  return _xpc_prop;
+}
+-(void) setXpc_prop:(xpc_object_t) newVal {
+  [_xpc_prop autorelease];
+  _xpc_prop = xpc_retain(newVal);
+}
 @end
 
 void NSLog(id, ...);
@@ -47,3 +59,8 @@ int main (int argc, const char * argv[]) {
     [pool drain];
     return 0;
 }
+
+void test(A *prevVal, A *newVal) {
+  [prevVal autorelease];
+  prevVal = [newVal retain];
+}
diff --git a/test/ARCMT/autoreleases.m.result b/test/ARCMT/autoreleases.m.result
index 49bc32141e..93ec9717dd 100644
--- a/test/ARCMT/autoreleases.m.result
+++ b/test/ARCMT/autoreleases.m.result
@@ -3,29 +3,21 @@
 // RUN: diff %t %s.result
 // DISABLE: mingw32
 
-typedef unsigned char BOOL;
+#include "Common.h"
 
-@interface NSObject {
-  id isa;
-}
-+new;
-+alloc;
--init;
--autorelease;
-@end
-
-@interface NSAutoreleasePool : NSObject
-- drain;
-@end
- 
 @interface A : NSObject {
 @package
     id object;
 }
 @end
 
-@interface B : NSObject
+@interface B : NSObject {
+  id _prop;
+  xpc_object_t _xpc_prop;
+}
 - (BOOL)containsSelf:(A*)a;
+@property (strong) id prop;
+@property (strong) xpc_object_t xpc_prop;
 @end
 
 @implementation A
@@ -35,6 +27,23 @@ typedef unsigned char BOOL;
 - (BOOL)containsSelf:(A*)a {
     return a->object == self;
 }
+
+-(id) prop {
+  return _prop;
+}
+-(void) setProp:(id) newVal {
+  _prop = newVal;
+}
+-(void) setProp2:(CFTypeRef) newVal {
+  _prop = (__bridge_transfer id)CFRetain(newVal);
+}
+
+-(id) xpc_prop {
+  return _xpc_prop;
+}
+-(void) setXpc_prop:(xpc_object_t) newVal {
+  _xpc_prop = newVal;
+}
 @end
 
 void NSLog(id, ...);
@@ -47,3 +56,7 @@ int main (int argc, const char * argv[]) {
     }
     return 0;
 }
+
+void test(A *prevVal, A *newVal) {
+  prevVal = newVal;
+}
diff --git a/test/ARCMT/checking.m b/test/ARCMT/checking.m
index cf7161187f..71c679621b 100644
--- a/test/ARCMT/checking.m
+++ b/test/ARCMT/checking.m
@@ -91,6 +91,9 @@ void test1(A *a, BOOL b, struct UnsafeS *unsafeS) {
   [a release];
   [a autorelease]; // expected-error {{it is not safe to remove an unused 'autorelease' message; its receiver may be destroyed immediately}} \
                    // expected-error {{ARC forbids explicit message send}}
+  [a autorelease]; // expected-error {{it is not safe to remove an unused 'autorelease' message; its receiver may be destroyed immediately}} \
+                   // expected-error {{ARC forbids explicit message send}}
+  a = 0;
 
   CFStringRef cfstr;
   NSString *str = (NSString *)cfstr; // expected-error {{cast of C pointer type 'CFStringRef' (aka 'const struct __CFString *') to Objective-C pointer type 'NSString *' requires a bridged cast}} \
diff --git a/test/ARCMT/dispatch.m b/test/ARCMT/dispatch.m
index 75c4a83459..58c7769638 100644
--- a/test/ARCMT/dispatch.m
+++ b/test/ARCMT/dispatch.m
@@ -4,17 +4,6 @@
 
 #include "Common.h"
 
-#define dispatch_retain(object) ({ dispatch_object_t _o = (object); _dispatch_object_validate(_o); (void)[_o retain]; })
-#define dispatch_release(object) ({ dispatch_object_t _o = (object); _dispatch_object_validate(_o); [_o release]; })
-#define xpc_retain(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o retain]; })
-#define xpc_release(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o release]; })
-
-typedef id dispatch_object_t;
-typedef id xpc_object_t;
-
-void _dispatch_object_validate(dispatch_object_t object);
-void _xpc_object_validate(xpc_object_t object);
-
 dispatch_object_t getme(void);
 
 void func(dispatch_object_t o) {
diff --git a/test/ARCMT/dispatch.m.result b/test/ARCMT/dispatch.m.result
index e897672a26..55b65585e4 100644
--- a/test/ARCMT/dispatch.m.result
+++ b/test/ARCMT/dispatch.m.result
@@ -4,17 +4,6 @@
 
 #include "Common.h"
 
-#define dispatch_retain(object) ({ dispatch_object_t _o = (object); _dispatch_object_validate(_o); (void)[_o retain]; })
-#define dispatch_release(object) ({ dispatch_object_t _o = (object); _dispatch_object_validate(_o); [_o release]; })
-#define xpc_retain(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o retain]; })
-#define xpc_release(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o release]; })
-
-typedef id dispatch_object_t;
-typedef id xpc_object_t;
-
-void _dispatch_object_validate(dispatch_object_t object);
-void _xpc_object_validate(xpc_object_t object);
-
 dispatch_object_t getme(void);
 
 void func(dispatch_object_t o) {