[Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape

[ahatanak]

Summary

If the block implicitly referencing self doesn't escape, there is no risk of creating retain cycles, so clang shouldn't diagnose it and force users to add self-> to silence the diagnostic.

Also, fix a bug where clang was failing to diagnose self referenced inside a block that was nested inside a c++ lambda.

rdar://problem/25059955

[ahatanak]

Sorry, clang was failing to diagnose self referenced inside a c++ lambda that was nested inside a block.

[erik.pilkington]

Akira and I were just talking about an alternative approach to this: Keep a vector of pairs of BlockDecls and SourceLocations in the enclosing method's FunctionScopeInfo, and emit any unsuppressed diagnostics when popping the method. This would avoid having to traverse all the blocks in methods in ARC mode, at the cost of a small amount of memory.

lib/Sema/Sema.cpp

1682–1687 ↗

(On Diff #195262)

Maybe call this "diagnoseBlockDecl" or something so it doesn't sounds like a CRTP-overridden method.

[ahatanak]

Keep a list of pairs of BlockDecl and SourceLocation and, after the body of an ObjC method is parsed, emit a diagnostic if a SourceLocation in the list is inside an escaping block.

[rjmccall]

lib/Sema/SemaExpr.cpp

2580 ↗

(On Diff #195498)

IIRC this check can be expensive enough that it's probably not worth doing if you expect these entries to typically not result in diagnostics.

[ahatanak]

Remove a potentially expensive check.

lib/Sema/SemaExpr.cpp

2580 ↗

(On Diff #195498)

DiagStateMap::lookup is doing a binary search. Is that what makes this check expensive?

[rjmccall]

include/clang/AST/DeclBase.h

1798 ↗

(On Diff #195589)

"innermost" is one word, so this should be getInnermostBlockDecl.

lib/Sema/SemaExpr.cpp

2580 ↗

(On Diff #195498)

Yeah. It's not immensely expensive, but adding some entries to a vector in the fast path and then processing them later probably makes more sense.

[ahatanak]

Rename function.

[erik.pilkington]

lib/Parse/ParseObjc.cpp

3696–3699 ↗

(On Diff #195498)

Any reason not to do this check in ActOnFinishFunctionBody (which is called by ParseFunctionStatementBody)? Seems like there are some similar diagnostics implemented there.

[rjmccall]

Alright, LGTM.

[ahatanak]

Diagnose implicitly retained self in ActOnFinishFunctionBody.

[erik.pilkington]

LGTM too, thanks!

lib/Sema/SemaDecl.cpp

13169 ↗

(On Diff #195639)

This can just be R = !CurBD->doesNotEscape();

cfe/trunk/include/clang/AST/DeclBase.h

 #include <type_traits>
 #include <utility>
 
 namespace clang {
 
 class ASTContext;
 class ASTMutationListener;
 class Attr;
+class BlockDecl;
 class DeclContext;
 class ExternalSourceSymbolAttr;
 class FunctionDecl;
 class FunctionType;
 class IdentifierInfo;
 enum Linkage : unsigned char;
 class LinkageSpecDecl;
 class Module;
public:
   }
 
   ASTContext &getParentASTContext() const {
     return cast<Decl>(this)->getASTContext();
   }
 
   bool isClosure() const { return getDeclKind() == Decl::Block; }
 
+  /// Return this DeclContext if it is a BlockDecl. Otherwise, return the
+  /// innermost enclosing BlockDecl or null if there are no enclosing blocks.
+  const BlockDecl *getInnermostBlockDecl() const {
+    const DeclContext *Ctx = this;
+
+    do {
+      if (Ctx->isClosure())
+        return cast<BlockDecl>(Ctx);
+      Ctx = Ctx->getParent();
+    } while (Ctx);
+
+    return nullptr;
+  }
+
   bool isObjCContainer() const {
     switch (getDeclKind()) {
     case Decl::ObjCCategory:
     case Decl::ObjCCategoryImpl:
     case Decl::ObjCImplementation:
     case Decl::ObjCInterface:
     case Decl::ObjCProtocol:
       return true;

cfe/trunk/include/clang/Sema/Sema.h

public:
   /// At the head of the list it is recorded whether there were 0, 1, or >= 2
   /// methods inside categories with a particular selector.
   GlobalMethodPool MethodPool;
 
   /// Method selectors used in a \@selector expression. Used for implementation
   /// of -Wselector.
   llvm::MapVector<Selector, SourceLocation> ReferencedSelectors;
 
+  /// List of SourceLocations where 'self' is implicitly retained inside a
+  /// block.
+  llvm::SmallVector<std::pair<SourceLocation, const BlockDecl *>, 1>
+      ImplicitlyRetainedSelfLocs;
+
   /// Kinds of C++ special members.
   enum CXXSpecialMember {
     CXXDefaultConstructor,
     CXXCopyConstructor,
     CXXMoveConstructor,
     CXXCopyAssignment,
     CXXMoveAssignment,
     CXXDestructor,

cfe/trunk/lib/Sema/SemaDecl.cpp

if (!IsLambda)
       S.PopExpressionEvaluationContext();
   }
 
 private:
   Sema &S;
   bool IsLambda = false;
 };
 
+static void diagnoseImplicitlyRetainedSelf(Sema &S) {
+  llvm::DenseMap<const BlockDecl *, bool> EscapeInfo;
+
+  auto IsOrNestedInEscapingBlock = [&](const BlockDecl *BD) {
+    if (EscapeInfo.count(BD))
+      return EscapeInfo[BD];
+
+    bool R = false;
+    const BlockDecl *CurBD = BD;
+
+    do {
+      R = !CurBD->doesNotEscape();
+      if (R)
+        break;
+      CurBD = CurBD->getParent()->getInnermostBlockDecl();
+    } while (CurBD);
+
+    return EscapeInfo[BD] = 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->");
+}
+
 Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
                                     bool IsInstantiation) {
   FunctionDecl *FD = dcl ? dcl->getAsFunction() : nullptr;
 
   sema::AnalysisBasedWarnings::Policy WP = AnalysisWarnings.getDefaultPolicy();
   sema::AnalysisBasedWarnings::Policy *ActivePolicy = nullptr;
 
   if (getLangOpts().Coroutines && getCurFunction()->isCoroutine())
if (FD) {
     }
     if (getCurFunction()->ObjCWarnForNoInitDelegation) {
       // Don't issue this warning for unavaialable inits.
       if (!MD->isUnavailable())
         Diag(MD->getLocation(),
              diag::warn_objc_secondary_init_missing_init_call);
       getCurFunction()->ObjCWarnForNoInitDelegation = false;
     }
+
+    diagnoseImplicitlyRetainedSelf(*this);
   } else {
     // Parsing the function declaration failed in some way. Pop the fake scope
     // we pushed on.
     PopFunctionScopeInfo(ActivePolicy, dcl);
     return nullptr;
   }
 
   if (Body && getCurFunction()->HasPotentialAvailabilityViolations)

cfe/trunk/lib/Sema/SemaDeclObjC.cpp

HasExplicitOwnershipAttr(Sema &S, ParmVarDecl *Param) {
   // If we have a lifetime qualifier, but it's local, we must have
   // inferred it. So, it is implicit.
   return !T.getLocalQualifiers().hasObjCLifetime();
 }
 
 /// ActOnStartOfObjCMethodDef - This routine sets up parameters; invisible
 /// and user declared, in the method definition's AST.
 void Sema::ActOnStartOfObjCMethodDef(Scope *FnBodyScope, Decl *D) {
+  ImplicitlyRetainedSelfLocs.clear();
   assert((getCurMethodDecl() == nullptr) && "Methodparsing confused");
   ObjCMethodDecl *MDecl = dyn_cast_or_null<ObjCMethodDecl>(D);
 
   PushExpressionEvaluationContext(ExprEvalContexts.back().Context);
 
   // If we don't have a valid method decl, simply return.
   if (!MDecl)
     return;

cfe/trunk/lib/Sema/SemaExpr.cpp

if (IFace && (IV = IFace->lookupInstanceVariable(II, ClassDeclared))) {
           ObjCIvarRefExpr(IV, IV->getUsageType(SelfExpr.get()->getType()), Loc,
                           IV->getLocation(), SelfExpr.get(), true, true);
 
       if (IV->getType().getObjCLifetime() == Qualifiers::OCL_Weak) {
         if (!isUnevaluatedContext() &&
             !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Loc))
           getCurFunction()->recordUseOfWeak(Result);
       }
-      if (getLangOpts().ObjCAutoRefCount) {
-        if (CurContext->isClosure())
-          Diag(Loc, diag::warn_implicitly_retains_self)
-            << FixItHint::CreateInsertion(Loc, "self->");
-      }
+      if (getLangOpts().ObjCAutoRefCount)
+        if (const BlockDecl *BD = CurContext->getInnermostBlockDecl())
+          ImplicitlyRetainedSelfLocs.push_back({Loc, BD});
 
       return Result;
     }
   } else if (CurMethod->isInstanceMethod()) {
     // We should warn if a local variable hides an ivar.
     if (ObjCInterfaceDecl *IFace = CurMethod->getClassInterface()) {
       ObjCInterfaceDecl *ClassDeclared;
       if (ObjCIvarDecl *IV = IFace->lookupInstanceVariable(II, ClassDeclared)) {

cfe/trunk/test/SemaObjC/warn-implicit-self-in-block.m

-// RUN: %clang_cc1 -x objective-c -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
-// rdar://11194874
-
-@interface Root @end
-
-@interface I : Root
-{
-  int _bar;
-}
-@end
-
-@implementation I
-  - (void)foo{
-      ^{
-           _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
-       }();
-  }
-@end

cfe/trunk/test/SemaObjCXX/warn-implicit-self-in-block.mm

+// RUN: %clang_cc1 -x objective-c++ -std=c++11 -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
+// rdar://11194874
+
+typedef void (^BlockTy)();
+
+void noescapeFunc(__attribute__((noescape)) BlockTy);
+void escapeFunc(BlockTy);
+
+@interface Root @end
+
+@interface I : Root
+{
+  int _bar;
+}
+@end
+
+@implementation I
+  - (void)foo{
+      ^{
+           _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+       }();
+  }
+
+  - (void)testNested{
+    noescapeFunc(^{
+      (void)_bar;
+      escapeFunc(^{
+        (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+        noescapeFunc(^{
+          (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+        });
+        (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+      });
+      (void)_bar;
+    });
+  }
+
+  - (void)testLambdaInBlock{
+    noescapeFunc(^{ [&](){ (void)_bar; }(); });
+    escapeFunc(^{ [&](){ (void)_bar; }(); }); // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+  }
+@end