[clang-tools-extra] 0151ddc - Create a clang-tidy check to warn when -dealloc is implemented inside an ObjC class category.
Ben Hamilton via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 10 07:56:36 PST 2020
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
+
+ at interface NSObject
+// Used to quash warning about missing base class.
+- (void)dealloc;
+ at end
+
+ at interface Foo : NSObject
+ at end
+
+ at implementation Foo
+- (void)dealloc {
+ // No warning should be generated here.
+}
+ at end
+
+ at interface Bar : NSObject
+ at end
+
+ at interface Bar (BarCategory)
+ at end
+
+ at 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]
+}
+ at end
+
+ at interface Baz : NSObject
+ at end
+
+ at implementation Baz
+- (void)dealloc {
+ // Should not trigger on implementation in the class itself, even with
+ // it declared in the category (below).
+}
+ at end
+
+ at 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;
+ at end
More information about the cfe-commits
mailing list