Author: Michael Wyman Date: 2020-02-10T08:56:28-07:00 New Revision: 0151ddc2e834ab4949789cbed4e03a958284cd54
URL: https://github.com/llvm/llvm-project/commit/0151ddc2e834ab4949789cbed4e03a958284cd54 DIFF: https://github.com/llvm/llvm-project/commit/0151ddc2e834ab4949789cbed4e03a958284cd54.diff LOG: Create a clang-tidy check to warn when -dealloc is implemented inside an ObjC class category. Summary: Such implementations may override the class's own implementation, and even be a danger in case someone later comes and adds one to the class itself. Most times this has been encountered have been a mistake. Reviewers: stephanemoore, benhamilton, dmaclach Reviewed By: stephanemoore, benhamilton, dmaclach Subscribers: dmaclach, mgorny, cfe-commits Tags: #clang-tools-extra, #clang Differential Revision: https://reviews.llvm.org/D72876 Added: clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.cpp clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.h clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst clang-tools-extra/test/clang-tidy/checkers/objc-dealloc-in-category.m Modified: clang-tools-extra/clang-tidy/objc/CMakeLists.txt clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/list.rst Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/objc/CMakeLists.txt b/clang-tools-extra/clang-tidy/objc/CMakeLists.txt index 68dda6530f7f..1e39e02e7b92 100644 --- a/clang-tools-extra/clang-tidy/objc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/objc/CMakeLists.txt @@ -2,6 +2,7 @@ set(LLVM_LINK_COMPONENTS support) add_clang_library(clangTidyObjCModule AvoidNSErrorInitCheck.cpp + DeallocInCategoryCheck.cpp ForbiddenSubclassingCheck.cpp MissingHashCheck.cpp ObjCTidyModule.cpp diff --git a/clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.cpp b/clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.cpp new file mode 100644 index 000000000000..468065742670 --- /dev/null +++ b/clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.cpp @@ -0,0 +1,46 @@ +//===--- DeallocInCategoryCheck.cpp - clang-tidy -------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "DeallocInCategoryCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/DeclObjC.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace objc { + +void DeallocInCategoryCheck::registerMatchers(MatchFinder *Finder) { + // This check should only be applied to Objective-C sources. + if (!getLangOpts().ObjC) + return; + + // Non-NSObject/NSProxy-derived objects may not have -dealloc as a special + // method. However, it seems highly unrealistic to expect many false-positives + // by warning on -dealloc in categories on classes without one of those + // base classes. + Finder->addMatcher( + objcMethodDecl(isInstanceMethod(), hasName("dealloc"), + hasDeclContext(objcCategoryImplDecl().bind("impl"))) + .bind("dealloc"), + this); +} + +void DeallocInCategoryCheck::check(const MatchFinder::MatchResult &Result) { + const auto *DeallocDecl = Result.Nodes.getNodeAs<ObjCMethodDecl>("dealloc"); + const auto *CID = Result.Nodes.getNodeAs<ObjCCategoryImplDecl>("impl"); + assert(DeallocDecl != nullptr); + diag(DeallocDecl->getLocation(), "category %0 should not implement -dealloc") + << CID; +} + +} // namespace objc +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.h b/clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.h new file mode 100644 index 000000000000..f8e1f70e216b --- /dev/null +++ b/clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.h @@ -0,0 +1,36 @@ +//===--- DeallocInCategoryCheck.h - clang-tidy ------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_DEALLOCINCATEGORYCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_DEALLOCINCATEGORYCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace objc { + +/// Finds implementations of -dealloc in Objective-C categories. The category +/// implementation will override any dealloc in the class implementation, +/// potentially causing issues. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/objc-dealloc-in-category.html +class DeallocInCategoryCheck final : public ClangTidyCheck { +public: + DeallocInCategoryCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace objc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_DEALLOCINCATEGORYCHECK_H diff --git a/clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp b/clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp index 69913125451c..ff220b88df4f 100644 --- a/clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" #include "AvoidNSErrorInitCheck.h" +#include "DeallocInCategoryCheck.h" #include "ForbiddenSubclassingCheck.h" #include "MissingHashCheck.h" #include "PropertyDeclarationCheck.h" @@ -26,6 +27,8 @@ class ObjCModule : public ClangTidyModule { void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { CheckFactories.registerCheck<AvoidNSErrorInitCheck>( "objc-avoid-nserror-init"); + CheckFactories.registerCheck<DeallocInCategoryCheck>( + "objc-dealloc-in-category"); CheckFactories.registerCheck<ForbiddenSubclassingCheck>( "objc-forbidden-subclassing"); CheckFactories.registerCheck<MissingHashCheck>( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 2c61591bdc34..7dfcd1a3c745 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -81,13 +81,18 @@ New checks <clang-tidy/checks/bugprone-reserved-identifier>` check. Checks for usages of identifiers reserved for use by the implementation. - + - New :doc:`cert-oop57-cpp <clang-tidy/checks/cert-oop57-cpp>` check. - + Flags use of the `C` standard library functions ``memset``, ``memcpy`` and ``memcmp`` and similar derivatives on non-trivial types. +- New :doc:`objc-dealloc-in-category + <clang-tidy/checks/objc-dealloc-in-category>` check. + + Finds implementations of -dealloc in Objective-C categories. + New check aliases ^^^^^^^^^^^^^^^^^ @@ -111,8 +116,8 @@ Changes in existing checks - Improved :doc:`readability-redundant-string-init <clang-tidy/checks/readability-redundant-string-init>` check now supports a - `StringNames` option enabling its application to custom string classes. The - check now detects in class initializers and constructor initializers which + `StringNames` option enabling its application to custom string classes. The + check now detects in class initializers and constructor initializers which are deemed to be redundant. Renamed checks diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index a6796e118d23..fb51e6f7a722 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -232,6 +232,7 @@ Clang-Tidy Checks `mpi-buffer-deref <mpi-buffer-deref.html>`_, "Yes" `mpi-type-mismatch <mpi-type-mismatch.html>`_, "Yes" `objc-avoid-nserror-init <objc-avoid-nserror-init.html>`_, + `objc-dealloc-in-category <objc-dealloc-in-category.html>`_, `objc-forbidden-subclassing <objc-forbidden-subclassing.html>`_, `objc-missing-hash <objc-missing-hash.html>`_, `objc-property-declaration <objc-property-declaration.html>`_, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst b/clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst new file mode 100644 index 000000000000..6d889089fe20 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst @@ -0,0 +1,13 @@ +.. title:: clang-tidy - objc-dealloc-in-category + +objc-dealloc-in-category +======================== + +Finds implementations of ``-dealloc`` in Objective-C categories. The category +implementation will override any ``-dealloc`` in the class implementation, +potentially causing issues. + +Classes implement ``-dealloc`` to perform important actions to deallocate +an object. If a category on the class implements ``-dealloc``, it will +override the class's implementation and unexpected deallocation behavior +may occur. diff --git a/clang-tools-extra/test/clang-tidy/checkers/objc-dealloc-in-category.m b/clang-tools-extra/test/clang-tidy/checkers/objc-dealloc-in-category.m new file mode 100644 index 000000000000..47855032d600 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/objc-dealloc-in-category.m @@ -0,0 +1,47 @@ +// RUN: %check_clang_tidy %s objc-dealloc-in-category %t + +@interface NSObject +// Used to quash warning about missing base class. +- (void)dealloc; +@end + +@interface Foo : NSObject +@end + +@implementation Foo +- (void)dealloc { + // No warning should be generated here. +} +@end + +@interface Bar : NSObject +@end + +@interface Bar (BarCategory) +@end + +@implementation Bar (BarCategory) ++ (void)dealloc { + // Should not trigger on class methods. +} + +- (void)dealloc { + // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: category 'BarCategory' should not implement -dealloc [objc-dealloc-in-category] +} +@end + +@interface Baz : NSObject +@end + +@implementation Baz +- (void)dealloc { + // Should not trigger on implementation in the class itself, even with + // it declared in the category (below). +} +@end + +@interface Baz (BazCategory) +// A declaration in a category @interface does not by itself provide an +// overriding implementation, and should not generate a warning. +- (void)dealloc; +@end _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
