From ac57af32843f0670b00eea5cf9f993241907f5ce Mon Sep 17 00:00:00 2001 From: Akira Hatanaka Date: Wed, 17 Apr 2019 23:14:44 +0000 Subject: [PATCH] [Sema][ObjC] Don't warn about an implicitly retained self if the retaining block and all of the enclosing blocks are non-escaping. If the block implicitly retaining 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 an implicitly retained self inside a c++ lambda nested inside a block. rdar://problem/25059955 Differential Revision: https://reviews.llvm.org/D60736 llvm-svn: 358624 --- clang/include/clang/AST/DeclBase.h | 15 ++++++++ clang/include/clang/Sema/Sema.h | 5 +++ clang/lib/Sema/SemaDecl.cpp | 31 ++++++++++++++++ clang/lib/Sema/SemaDeclObjC.cpp | 1 + clang/lib/Sema/SemaExpr.cpp | 8 ++--- clang/test/SemaObjC/warn-implicit-self-in-block.m | 18 ---------- .../test/SemaObjCXX/warn-implicit-self-in-block.mm | 42 ++++++++++++++++++++++ 7 files changed, 97 insertions(+), 23 deletions(-) delete mode 100644 clang/test/SemaObjC/warn-implicit-self-in-block.m create mode 100644 clang/test/SemaObjCXX/warn-implicit-self-in-block.mm diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h index 4b63e54..b358177 100644 --- a/clang/include/clang/AST/DeclBase.h +++ b/clang/include/clang/AST/DeclBase.h @@ -41,6 +41,7 @@ namespace clang { class ASTContext; class ASTMutationListener; class Attr; +class BlockDecl; class DeclContext; class ExternalSourceSymbolAttr; class FunctionDecl; @@ -1792,6 +1793,20 @@ public: 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(Ctx); + Ctx = Ctx->getParent(); + } while (Ctx); + + return nullptr; + } + bool isObjCContainer() const { switch (getDeclKind()) { case Decl::ObjCCategory: diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 351b6aa..8582872 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -1213,6 +1213,11 @@ public: /// of -Wselector. llvm::MapVector ReferencedSelectors; + /// List of SourceLocations where 'self' is implicitly retained inside a + /// block. + llvm::SmallVector, 1> + ImplicitlyRetainedSelfLocs; + /// Kinds of C++ special members. enum CXXSpecialMember { CXXDefaultConstructor, diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index fbe258b..8e0badf 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -13150,6 +13150,35 @@ private: bool IsLambda = false; }; +static void diagnoseImplicitlyRetainedSelf(Sema &S) { + llvm::DenseMap 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 &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; @@ -13361,6 +13390,8 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body, 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. diff --git a/clang/lib/Sema/SemaDeclObjC.cpp b/clang/lib/Sema/SemaDeclObjC.cpp index d0674c6..430c2a2 100644 --- a/clang/lib/Sema/SemaDeclObjC.cpp +++ b/clang/lib/Sema/SemaDeclObjC.cpp @@ -359,6 +359,7 @@ HasExplicitOwnershipAttr(Sema &S, ParmVarDecl *Param) { /// 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(D); diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index ef39092..e64eeec 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -2575,11 +2575,9 @@ Sema::LookupInObjCMethod(LookupResult &Lookup, Scope *S, !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; } diff --git a/clang/test/SemaObjC/warn-implicit-self-in-block.m b/clang/test/SemaObjC/warn-implicit-self-in-block.m deleted file mode 100644 index a7ee16e..0000000 --- a/clang/test/SemaObjC/warn-implicit-self-in-block.m +++ /dev/null @@ -1,18 +0,0 @@ -// 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 diff --git a/clang/test/SemaObjCXX/warn-implicit-self-in-block.mm b/clang/test/SemaObjCXX/warn-implicit-self-in-block.mm new file mode 100644 index 0000000..4842b4b --- /dev/null +++ b/clang/test/SemaObjCXX/warn-implicit-self-in-block.mm @@ -0,0 +1,42 @@ +// 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 -- 2.7.4