[arcmt] It's not safe to remove the -release on "[[someivar delegate] release];"...

that, after migration, the object that was passed to 'setDelegate:' will not be properly retained, e.g:

-whatever {
  id x = [[MyDoHicky alloc] init];
  [someivar setDelegate: x]; // x won't get retained in ARC.
}
-dealloc {
  [[someivar delegate] release]; // give migration error here.
}

rdar://8858009

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

diff --git a/lib/ARCMigrate/TransRetainReleaseDealloc.cpp b/lib/ARCMigrate/TransRetainReleaseDealloc.cpp
index 6f917b324655f6ccc6f3844a1e8ce3256d7dc6cd..ed6ed0adfdf2fac4653e438076e1411a49b7e0e2 100644 (file)
--- a/lib/ARCMigrate/TransRetainReleaseDealloc.cpp
+++ b/lib/ARCMigrate/TransRetainReleaseDealloc.cpp
@@ -19,9 +19,7 @@
 
 #include "Transforms.h"
 #include "Internals.h"
-#include "clang/Sema/Sema.h"
 #include "clang/Sema/SemaDiagnostic.h"
-#include "clang/Lex/Preprocessor.h"
 #include "clang/AST/ParentMap.h"
 
 using namespace clang;
@@ -39,9 +37,14 @@ class RetainReleaseDeallocRemover :
   ExprSet Removables;
   llvm::OwningPtr<ParentMap> StmtMap;
 
+  Selector DelegateSel;
+
 public:
   RetainReleaseDeallocRemover(MigrationPass &pass)
-    : Body(0), Pass(pass) { }
+    : Body(0), Pass(pass) {
+    DelegateSel =
+        Pass.Ctx.Selectors.getNullarySelector(&Pass.Ctx.Idents.get("delegate"));
+  }
 
   void transformBody(Stmt *body) {
     Body = body;
@@ -60,10 +63,9 @@ public:
         // 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.
-        std::string err = "it is not safe to remove an unused '";
-        err += E->getSelector().getAsString() + "'; its receiver may be "
-            "destroyed immediately";
-        Pass.TA.reportError(err, E->getLocStart(), E->getSourceRange());
+        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.
@@ -89,6 +91,14 @@ public:
             Pass.TA.reportError(err, rec->getLocStart());
             return true;
           }
+
+          if (E->getMethodFamily() == OMF_release && isDelegateMessage(rec)) {
+            Pass.TA.reportError("it is not safe to remove 'retain' "
+                "message on the result of a 'delegate' message; "
+                "the object that was passed to 'setDelegate:' may not be "
+                "properly retained", rec->getLocStart());
+            return true;
+          }
         }
     case OMF_dealloc:
       break;
@@ -120,8 +130,7 @@ public:
       // 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.SemaRef.getPreprocessor()
-                        .getIdentifierInfo("nil")->hasMacroDefinition())
+      if (Pass.Ctx.Idents.get("nil").hasMacroDefinition())
         Pass.TA.insertAfterToken(rec->getLocEnd(), " = nil");
       else
         Pass.TA.insertAfterToken(rec->getLocEnd(), " = 0");
@@ -145,6 +154,19 @@ private:
                             loc);
   }
 
+  bool isDelegateMessage(Expr *E) const {
+    if (!E) return false;
+
+    E = E->IgnoreParenCasts();
+    if (ObjCMessageExpr *ME = dyn_cast<ObjCMessageExpr>(E))
+      return (ME->isInstanceMessage() && ME->getSelector() == DelegateSel);
+
+    if (ObjCPropertyRefExpr *propE = dyn_cast<ObjCPropertyRefExpr>(E))
+      return propE->getGetterSelector() == DelegateSel;
+
+    return false;
+  }
+
   bool isInAtFinally(Expr *E) const {
     assert(E);
     Stmt *S = E;

diff --git a/test/ARCMT/checking.m b/test/ARCMT/checking.m
index 2e4ec122d64c7f9ff29f4b77474a4e426817b25b..eea7a9f2547cb29fd0bf8c90992445d7f0630f3d 100644 (file)
--- a/test/ARCMT/checking.m
+++ b/test/ARCMT/checking.m
@@ -20,6 +20,7 @@ struct UnsafeS {
 - (oneway void)release;
 - (void)dealloc;
 -(void)test;
+-(id)delegate;
 @end
 
 @implementation A
@@ -34,11 +35,17 @@ struct UnsafeS {
 - (id)retainCount { return self; } // expected-error {{ARC forbids implementation}}
 - (id)autorelease { return self; } // expected-error {{ARC forbids implementation}}
 - (oneway void)release { } // expected-error {{ARC forbids implementation}}
+
+-(id)delegate { return self; }
 @end
 
 id global_foo;
 
 void test1(A *a, BOOL b, struct UnsafeS *unsafeS) {
+  [[a delegate] release]; // expected-error {{it is not safe to remove 'retain' message on the result of a 'delegate' message; the object that was passed to 'setDelegate:' may not be properly retained}} \
+                          // expected-error {{ARC forbids explicit message send}}
+  [a.delegate release]; // expected-error {{it is not safe to remove 'retain' message on the result of a 'delegate' message; the object that was passed to 'setDelegate:' may not be properly retained}} \
+                        // expected-error {{ARC forbids explicit message send}}
   [unsafeS->unsafeObj retain]; // expected-error {{it is not safe to remove 'retain' message on an __unsafe_unretained type}} \
                                // expected-error {{ARC forbids explicit message send}}
   id foo = [unsafeS->unsafeObj retain]; // no warning.
@@ -50,7 +57,7 @@ void test1(A *a, BOOL b, struct UnsafeS *unsafeS) {
   [a retain];
   [a retainCount]; // expected-error {{ARC forbids explicit message send of 'retainCount'}}
   [a release];
-  [a autorelease]; // expected-error {{it is not safe to remove an unused 'autorelease'; its receiver may be destroyed immediately}} \
+  [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}}
 
   CFStringRef cfstr;