Add warning for blocks capturing {'this', raw pointers, references}

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -1641,6 +1641,17 @@ def warn_implicitly_retains_self : Warning <
   "block implicitly retains 'self'; explicitly mention 'self' to indicate "
   "this is intended behavior">,
   InGroup<DiagGroup<"implicit-retain-self">>, DefaultIgnore;
+def warn_blocks_capturing_this : Warning<"block implicitly captures 'this'">,
+                                 InGroup<DiagGroup<"blocks-capturing-this">>,
+                                 DefaultIgnore;
+def warn_blocks_capturing_reference
+    : Warning<"block implicitly captures a C++ reference">,
+      InGroup<DiagGroup<"blocks-capturing-reference">>,
+      DefaultIgnore;
+def warn_blocks_capturing_raw_pointer
+    : Warning<"block implicitly captures a raw pointer">,
+      InGroup<DiagGroup<"blocks-capturing-raw-pointer">>,
+      DefaultIgnore;
 def warn_arc_possible_repeated_use_of_weak : Warning <
   "weak %select{variable|property|implicit property|instance variable}0 %1 may "
   "be accessed multiple times in this %select{function|method|block|lambda}2 "

clang/include/clang/Sema/Sema.h

@@ -8227,6 +8227,22 @@ class Sema final : public SemaBase {
     }
   };
 
+  /// Store information about a diagnosable block catpure.
+  struct BlockCapture {
+    /// Enumeration representing types of block captures that may be
+    /// diagnosable because they could be problematic.
+    enum CaptureType {
+      Self,
+      This,
+      Reference,
+      RawPointer,
+    };
+
+    SourceLocation Loc;
+    const BlockDecl *BD;
+    CaptureType Type;
+  };
+
   /// Check an argument list for placeholders that we won't try to
   /// handle later.
   bool CheckArgsForPlaceholders(MultiExprArg args);
@@ -8243,8 +8259,7 @@ class Sema final : public SemaBase {
 
   /// List of SourceLocations where 'self' is implicitly retained inside a
   /// block.
-  llvm::SmallVector<std::pair<SourceLocation, const BlockDecl *>, 1>
-      ImplicitlyRetainedSelfLocs;
+  llvm::SmallVector<BlockCapture, 1> DiagnosableBlockCaptures;
 
   /// Do an explicit extend of the given block pointer if we're in ARC.
   void maybeExtendBlockObject(ExprResult &E);

clang/lib/Sema/SemaDecl.cpp

@@ -16149,7 +16149,7 @@ class ExitFunctionBodyRAII {
   bool IsLambda = false;
 };
 
-static void diagnoseImplicitlyRetainedSelf(Sema &S) {
+static void diagnoseBlockCaptures(Sema &S) {
   llvm::DenseMap<const BlockDecl *, bool> EscapeInfo;
 
   auto IsOrNestedInEscapingBlock = [&](const BlockDecl *BD) {
@@ -16170,13 +16170,35 @@ static void diagnoseImplicitlyRetainedSelf(Sema &S) {
     return It->second = R;
   };
 
-  // If the location where 'self' is implicitly retained is inside a escaping
-  // block, emit a diagnostic.
-  for (const std::pair<SourceLocation, const BlockDecl *> &P :
-       S.ImplicitlyRetainedSelfLocs)
-    if (IsOrNestedInEscapingBlock(P.second))
-      S.Diag(P.first, diag::warn_implicitly_retains_self)
-          << FixItHint::CreateInsertion(P.first, "self->");
+  for (const auto &C : S.DiagnosableBlockCaptures) {
+    //  Do not emit a diagnostic if the location is invalid.
+    if (!C.Loc.isValid())
+      continue;
+
+    // Do not emit a diagnostic if the block is not escaping.
+    if (!IsOrNestedInEscapingBlock(C.BD))
+      continue;
+
+    // Emit the correct warning depending on the type of capture.
+    switch (C.Type) {
+    case Sema::BlockCapture::Self:
+      S.Diag(C.Loc, diag::warn_implicitly_retains_self)
+          << FixItHint::CreateInsertion(C.Loc, "self->");
+      break;
+
+    case Sema::BlockCapture::This:
+      S.Diag(C.Loc, diag::warn_blocks_capturing_this);
+      break;
+
+    case Sema::BlockCapture::Reference:
+      S.Diag(C.Loc, diag::warn_blocks_capturing_reference);
+      break;
+
+    case Sema::BlockCapture::RawPointer:
+      S.Diag(C.Loc, diag::warn_blocks_capturing_raw_pointer);
+      break;
+    }
+  }
 }
 
 static bool methodHasName(const FunctionDecl *FD, StringRef Name) {
@@ -16511,6 +16533,10 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
         }
       }
 
+      // Warn about block captures, unless this is a lambda function.
+      if (!isLambdaCallOperator(FD))
+        diagnoseBlockCaptures(*this);
+
       assert((FD == getCurFunctionDecl(/*AllowLambdas=*/true)) &&
              "Function parsing confused");
     } else if (ObjCMethodDecl *MD = dyn_cast_or_null<ObjCMethodDecl>(dcl)) {
@@ -16563,7 +16589,7 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
         FSI->ObjCWarnForNoInitDelegation = false;
       }
 
-      diagnoseImplicitlyRetainedSelf(*this);
+      diagnoseBlockCaptures(*this);
     } else {
       // Parsing the function declaration failed in some way. Pop the fake scope
       // we pushed on.

clang/lib/Sema/SemaDeclObjC.cpp

@@ -367,7 +367,7 @@ HasExplicitOwnershipAttr(Sema &S, ParmVarDecl *Param) {
 /// and user declared, in the method definition's AST.
 void SemaObjC::ActOnStartOfObjCMethodDef(Scope *FnBodyScope, Decl *D) {
   ASTContext &Context = getASTContext();
-  SemaRef.ImplicitlyRetainedSelfLocs.clear();
+  SemaRef.DiagnosableBlockCaptures.clear();
   assert((SemaRef.getCurMethodDecl() == nullptr) && "Methodparsing confused");
   ObjCMethodDecl *MDecl = dyn_cast_or_null<ObjCMethodDecl>(D);
 

clang/lib/Sema/SemaExpr.cpp

@@ -16536,6 +16536,11 @@ ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc,
   // Set the captured variables on the block.
   SmallVector<BlockDecl::Capture, 4> Captures;
   for (Capture &Cap : BSI->Captures) {
+    if (Cap.isThisCapture()) {
+      DiagnosableBlockCaptures.push_back(
+          Sema::BlockCapture{Cap.getLocation(), BD, Sema::BlockCapture::This});
+    }
+
     if (Cap.isInvalid() || Cap.isThisCapture())
       continue;
     // Cap.getVariable() is always a VarDecl because
@@ -16595,6 +16600,14 @@ ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc,
       }
     }
 
+    QualType Type = Cap.getCaptureType();
+    if (Type->isPointerOrReferenceType()) {
+      DiagnosableBlockCaptures.push_back(Sema::BlockCapture{
+          Cap.getLocation(), BD,
+          Type->isReferenceType() ? Sema::BlockCapture::Reference
+                                  : Sema::BlockCapture::RawPointer});
+    }
+
     BlockDecl::Capture NewCap(Var, Cap.isBlockCapture(), Cap.isNested(),
                               CopyExpr);
     Captures.push_back(NewCap);

clang/lib/Sema/SemaExprObjC.cpp

@@ -4913,8 +4913,10 @@ ExprResult SemaObjC::BuildIvarRefExpr(Scope *S, SourceLocation Loc,
       SemaRef.getCurFunction()->recordUseOfWeak(Result);
   }
   if (getLangOpts().ObjCAutoRefCount && !SemaRef.isUnevaluatedContext())
-    if (const BlockDecl *BD = SemaRef.CurContext->getInnermostBlockDecl())
-      SemaRef.ImplicitlyRetainedSelfLocs.push_back({Loc, BD});
+    if (const BlockDecl *BD = SemaRef.CurContext->getInnermostBlockDecl()) {
+      SemaRef.DiagnosableBlockCaptures.push_back(
+          Sema::BlockCapture{Loc, BD, Sema::BlockCapture::Self});
+    }
 
   return Result;
 }

clang/test/SemaObjCXX/warn-blocks-capturing-pointers.mm

@@ -0,0 +1,103 @@
+// RUN: %clang_cc1 -x objective-c++ -std=c++11 -fobjc-arc -fblocks -Wblocks-capturing-this -Wblocks-capturing-reference -Wblocks-capturing-raw-pointer -Wimplicit-retain-self -verify %s
+
+typedef void (^BlockTy)();
+
+void noescapeFunc(__attribute__((noescape)) BlockTy);
+void escapeFunc(BlockTy);
+
+@interface Root @end
+
+@interface I : Root
+- (void)foo;
+@end
+
+class F {
+ public:
+  void Foo() const;
+  void FooAsync(F* p, F& r, I* i) {
+    escapeFunc(
+        ^{
+          Foo(); // expected-warning {{block implicitly captures 'this'}}
+          p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
+          r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
+          [i foo];
+        });
+
+    ([=, &r]() {
+      escapeFunc(
+          ^{
+            Foo(); // expected-warning {{block implicitly captures 'this'}}
+            p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
+            r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
+            [i foo];
+          });
+    })();
+
+    escapeFunc(
+        ^{
+          ([=]() {
+            Foo(); // expected-warning {{block implicitly captures 'this'}}
+            p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
+            r.Foo();  // expected-warning {{block implicitly captures a C++ reference}}
+            [i foo];
+          })();
+        });
+
+    noescapeFunc(
+        ^{
+          Foo();
+          p->Foo();
+          r.Foo();
+          [i foo];
+        });
+  }
+};
+
+@implementation I {
+  int _bar;
+}
+
+- (void)doSomethingWithPtr:(F*)p
+                       ref:(F&)r
+                       obj:(I*)i {
+    escapeFunc(
+        ^{
+          (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+          p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
+          r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
+          [i foo];
+        });
+
+    ([=, &r]() {
+      escapeFunc(
+          ^{
+            (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+            p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
+            r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
+            [i foo];
+          });
+    })();
+
+    escapeFunc(
+        ^{
+          ([=]() {
+            (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+            p->Foo(); // expected-warning {{block implicitly captures a raw pointer}}
+            r.Foo(); // expected-warning {{block implicitly captures a C++ reference}}
+            [i foo];
+          })();
+        });
+
+    noescapeFunc(
+        ^{
+          (void)_bar;
+          p->Foo();
+          r.Foo();
+          [i foo];
+        });
+}
+
+- (void)foo {
+}
+
+@end