[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

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

2579

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

2579

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

2579

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();

include/clang/Sema/ScopeInfo.h

public:
   /// initializer within a class that has at least one initializer marked as a
   /// designated initializer.
   bool ObjCIsSecondaryInit : 1;
 
   /// This starts true for a secondary initializer method and will be set to
   /// false if there is an invocation of an initializer on 'self'.
   bool ObjCWarnForNoInitDelegation : 1;
 
+  /// A flag indicating the scope is an ObjC method definition.
+  bool IsObjCMethod : 1;
+
   /// True only when this function has not already built, or attempted
   /// to build, the initial and final coroutine suspend points
   bool NeedsCoroutineSuspends : 1;
 
   /// An enumeration represeting the kind of the first coroutine statement
   /// in the function. One of co_return, co_await, or co_yield.
   unsigned char FirstCoroutineStmtKind : 2;
 
public:
 
   /// The initial and final coroutine suspend points.
   std::pair<Stmt *, Stmt *> CoroutineSuspends;
 
   /// The stack of currently active compound stamement scopes in the
   /// function.
   SmallVector<CompoundScopeInfo, 4> CompoundScopes;
 
-  /// The set of blocks that are introduced in this function.
-  llvm::SmallPtrSet<const BlockDecl *, 1> Blocks;
+  /// The list of blocks that are directly or indirectly introduced in this
+  /// function and aren't enclosed in another block. The boolean flag indicates
+  /// whether the block is introduced in the current function scope. For
+  /// example, when function 'foo1' is parsed in the code below, the flag is
+  /// false for the first block and is true for the second block. The block that
+  /// calls 'foo2(2)' isn't added to the list since it is nested inside another
+  /// block.
+  ///
+  /// void foo1() {
+  ///   [](){ ^{ foo2(0); }; }();
+  ///   ^{ foo2(1); }();
+  ///   ^{ ^{ foo2(2); }(); }();
+  /// }
+  ///
+  /// The blocks are stored to the list in the order they appear in the
+  /// function.
+  llvm::SmallVector<std::pair<const BlockDecl *, bool>, 1> Blocks;
 
   /// The set of __block variables that are introduced in this function.
   llvm::TinyPtrVector<VarDecl *> ByrefBlockVars;
 
   /// A list of PartialDiagnostics created but delayed within the
   /// current function scope.  These diagnostics are vetted for reachability
   /// prior to being emitted.
   SmallVector<PossiblyUnreachableDiag, 4> PossiblyUnreachableDiags;
public:
   }
 
   bool NeedsScopeChecking() const {
     return !HasDroppedStmt &&
         (HasIndirectGoto ||
           (HasBranchProtectedScope && HasBranchIntoScope));
   }
 
-  // Add a block introduced in this function.
-  void addBlock(const BlockDecl *BD) {
-    Blocks.insert(BD);
+  bool isObjCMethod() const {
+    return IsObjCMethod;
+  }
+
+  void setIsObjCMethod() {
+    IsObjCMethod = true;
+  }
+
+  // Add a block to the list.
+  void addBlock(const BlockDecl *BD, bool IsIntroducedInCurrentScope) {
+    Blocks.push_back({BD, IsIntroducedInCurrentScope});
   }
 
   // Add a __block variable introduced in this function.
   void addByrefBlockVar(VarDecl *VD) {
     ByrefBlockVars.push_back(VD);
   }
 
+  bool isBlockScope() const { return Kind == SK_Block; }
+
   bool isCoroutine() const { return !FirstCoroutineStmtLoc.isInvalid(); }
 
   void setFirstCoroutineStmt(SourceLocation Loc, StringRef Keyword) {
     assert(FirstCoroutineStmtLoc.isInvalid() &&
                    "first coroutine statement location already set");
     FirstCoroutineStmtLoc = Loc;
     FirstCoroutineStmtKind = llvm::StringSwitch<unsigned char>(Keyword)
             .Case("co_return", 0)

lib/Sema/Sema.cpp

 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/PrettyDeclStackTrace.h"
 #include "clang/AST/StmtCXX.h"
+#include "clang/AST/StmtVisitor.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/CXXFieldCollector.h"
 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/ExternalSemaSource.h"
if (CXXDestructorDecl *DD = RD->getDestructor()) {
       auto *FPT = DD->getType()->getAs<FunctionProtoType>();
       S.ResolveExceptionSpec(Loc, FPT);
     }
 }
 
 static void markEscapingByrefs(const FunctionScopeInfo &FSI, Sema &S) {
   // Set the EscapingByref flag of __block variables captured by
   // escaping blocks.
-  for (const BlockDecl *BD : FSI.Blocks) {
-    if (BD->doesNotEscape())
+  for (const std::pair<const BlockDecl *, bool> &P : FSI.Blocks) {
+    // Skip blocks that aren't directly introduced in this function. We've
+    // already seen them.
+    if (!P.second)
       continue;
-    for (const BlockDecl::Capture &BC : BD->captures()) {
+
+    if (P.first->doesNotEscape())
+      continue;
+
+    for (const BlockDecl::Capture &BC : P.first->captures()) {
       VarDecl *VD = BC.getVariable();
       if (VD->hasAttr<BlocksAttr>())
         VD->setEscapingByref();
     }
   }
 
   for (VarDecl *VD : FSI.ByrefBlockVars) {
     // __block variables might require us to capture a copy-initializer.
     if (!VD->isEscapingByref())
       continue;
     // It's currently invalid to ever have a __block variable with an
     // array type; should we diagnose that here?
     // Regardless, we don't want to ignore array nesting when
     // constructing this copy.
     if (VD->getType()->isStructureOrClassType())
       checkEscapingByref(VD, S);
   }
 }
 
+namespace {
+
+class DiagImplicitRetainedSelf
+    : public ConstStmtVisitor<DiagImplicitRetainedSelf> {
+  Sema &S;
+
+  // A flag indicating whether we are visiting a Stmt that is inside a block
+  // that can possibly escape.
+  bool VisitingEscapingBlock = false;
+
+public:
+  DiagImplicitRetainedSelf(Sema &S) : S(S) {}
+
+  void VisitBlockDecl(const BlockDecl *BD) {
+    bool OldVisitingEscapingBlock = VisitingEscapingBlock;
+    VisitingEscapingBlock = VisitingEscapingBlock || !BD->doesNotEscape();
+    Visit(BD->getBody());
+    VisitingEscapingBlock = OldVisitingEscapingBlock;
+  }
+
+  void VisitBlockExpr(const BlockExpr *BE) {
+    VisitBlockDecl(BE->getBlockDecl());
+  }
+
+  void VisitObjCIvarRefExpr(const ObjCIvarRefExpr *ORE) {
+    if (!VisitingEscapingBlock || !ORE->isFreeIvar())
+      return;
+    SourceLocation Loc = ORE->getExprLoc();
+    S.Diag(Loc, diag::warn_implicitly_retains_self)
+        << FixItHint::CreateInsertion(Loc, "self->");
+  }
+
+  void VisitStmt(const Stmt *S) {
+    for (const Stmt *SubStmt : S->children())
+      if (SubStmt)
+        Visit(SubStmt);
+  }
+};
+
+} // namespace
+
+static void diagnoseImplicitlyRetainedSelf(const FunctionScopeInfo &FSI,
+                                           Sema &S) {
+  DiagImplicitRetainedSelf D(S);
+  for (const std::pair<const BlockDecl *, bool> &P : FSI.Blocks)
+    D.VisitBlockDecl(P.first);
+}
+
 void Sema::PopFunctionScopeInfo(const AnalysisBasedWarnings::Policy *WP,
                                 const Decl *D, const BlockExpr *blkExpr) {
   assert(!FunctionScopes.empty() && "mismatched push/pop!");
 
   // This function shouldn't be called after popping the current function scope.
   // markEscapingByrefs calls PerformMoveOrCopyInitialization, which can call
   // PushFunctionScope, which can cause clearing out PreallocatedFunctionScope
   // when FunctionScopes is empty.
   markEscapingByrefs(*FunctionScopes.back(), *this);
 
   FunctionScopeInfo *Scope = FunctionScopes.pop_back_val();
 
+  if (getLangOpts().ObjCAutoRefCount && Scope->isObjCMethod())
+    diagnoseImplicitlyRetainedSelf(*Scope, *this);
+
+  // If the current scope isn't a block, add the blocks in the list to the
+  // enclosing function's block list.
+  if (!Scope->isBlockScope() && !FunctionScopes.empty())
+    for (std::pair<const BlockDecl *, bool> &p : Scope->Blocks)
+      FunctionScopes.back()->addBlock(
+          p.first, /*IsIntroducedInCurrentScope*/false);
+
   if (LangOpts.OpenMP)
     popOpenMPFunctionRegion(Scope);
 
   // Issue any analysis-based warnings.
   if (WP && D)
     AnalysisWarnings.IssueWarnings(*WP, Scope, D, blkExpr);
   else
     for (const auto &PUD : Scope->PossiblyUnreachableDiags)

lib/Sema/SemaDeclObjC.cpp

if (!ResultType->isDependentType() && !ResultType->isVoidType() &&
       !MDecl->isInvalidDecl() &&
       RequireCompleteType(MDecl->getLocation(), ResultType,
                           diag::err_func_def_incomplete_result))
     MDecl->setInvalidDecl();
 
   // Allow all of Sema to see that we are entering a method definition.
   PushDeclContext(FnBodyScope, MDecl);
   PushFunctionScope();
+  getCurFunction()->setIsObjCMethod();
 
   // Create Decl objects for each parameter, entrring them in the scope for
   // binding to their use.
 
   // Insert the invisible arguments, self and _cmd!
   MDecl->createImplicitParams(Context, MDecl->getClassInterface());
 
   PushOnScopeChains(MDecl->getSelfDecl(), FnBodyScope);

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->");
-      }
 
       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)) {
         if (IV->getAccessControl() != ObjCIvarDecl::Private ||
             declaresSameEntity(IFace, ClassDeclared))
if (LangOpts.CPlusPlus) {
     if (MangleNumberingContext *MCtx =
             getCurrentMangleNumberContext(Block->getDeclContext(),
                                           ManglingContextDecl)) {
       unsigned ManglingNumber = MCtx->getManglingNumber(Block);
       Block->setBlockMangling(ManglingNumber, ManglingContextDecl);
     }
   }
 
+  if (getCurFunction())
+    getCurFunction()->addBlock(Block, /*IsIntroducedInCurrentScope*/true);
+
   PushBlockScope(CurScope, Block);
   CurContext->addDecl(Block);
   if (CurScope)
     PushDeclContext(CurScope, Block);
   else
     CurContext = Block;
 
   getCurBlock()->HasImplicitReturnType = true;
for (const auto &CI : Result->getBlockDecl()->captures()) {
       const VarDecl *var = CI.getVariable();
       if (var->getType().isDestructedType() != QualType::DK_none) {
         setFunctionHasBranchProtectedScope();
         break;
       }
     }
   }
 
-  if (getCurFunction())
-    getCurFunction()->addBlock(BD);
-
   return Result;
 }
 
 ExprResult Sema::ActOnVAArg(SourceLocation BuiltinLoc, Expr *E, ParsedType Ty,
                             SourceLocation RPLoc) {
   TypeSourceInfo *TInfo;
   GetTypeFromParser(Ty, &TInfo);
   return BuildVAArgExpr(BuiltinLoc, E, TInfo, RPLoc);

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

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