Create a clang-tidy check to warn when -dealloc is implemented inside an ObjC class category.

[mwyman]

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.

[dmaclach]

clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-categories.rst

11

Need an end quote on dealloc

[mwyman]

Fixed missing end quote pointed out in review comment.

[stephanemoore]

clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.cpp

21

Add isInstanceMethod() within the objcMethodDecl?

clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.h

24

What do you think of the name DeallocInCategoryCheck?

clang-tools-extra/test/clang-tidy/checkers/objc-dealloc-in-categories.m

23

Please add a class method +dealloc and make sure that the check does not trigger on that.

35

Perhaps add an @implementation for Baz (not a category implementation) that contains -dealloc to ensure that a declaration in a category with a definition in the implementation does not trigger?

[stephanemoore]

clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.cpp

19

Early return if the language is not an Objective-C variant:

// This check should only be applied to Objective-C sources.
if (!getLangOpts().ObjC)
  return;

21

Technically, isn't -dealloc specific to certain classes, e.g., [NSObject](https://developer.apple.com/documentation/objectivec/nsobject/1571947-dealloc?language=objc)) and NSProxy? If that is true, we should technically check that the category is on a class that is or is derived from a relevant class like NSObject or NSProxy.

[mwyman]

Addresses reviewer feedback.

[mwyman]

Updated the diff based on review feedback.

clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.cpp

21

Technically true, but given the prevalence of extant ObjC documentation talking about the -dealloc method in terms of deallocation, it seems highly unlikely any non-NSObject/NSProxy-rooted class hierarchies are going to use -dealloc in any other context, and want to allow implementations in a category.

clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.h

24

Much better.

[stephanemoore]

Looks in good shape 👌 A couple nits and polish ideas.

clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.cpp

34 ↗

(On Diff #240716)

What do you think of making a fixit that deletes the -dealloc implementation?

36 ↗

(On Diff #240716)

I can't speak authoritatively regarding project convention but I believe that it's rare to condition on a nonnull match when matching on a single AST node (in other words, check would not have been called if MatchedDecl were null). I believe that convention is either to omit the expected condition or to assert the expected condition.

Examples:
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/objc/MissingHashCheck.cpp#L54
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/objc/ForbiddenSubclassingCheck.cpp#L73

37–38 ↗

(On Diff #240716)

What do you think of binding the category implementation declaration and including that in the diagnostic message?

Finder->addMatcher(objcMethodDecl(isInstanceMethod(), hasName("dealloc"),
                                  hasDeclContext(objcCategoryImplDecl().bind("impl")))
                       .bind("dealloc"),
                   this);

// ...

const auto *CID = Result.Nodes.getNodeAs<ObjCCategoryImplDecl>("impl");
diag(MatchedDecl->getLocation(),
     "category %0 should not implement -dealloc") << CID;

(admittedly, I don't recall off the top of my head how ObjCCategoryImplDecl renders in diagnostic messages so some additional tuning could be warranted)

clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst

7 ↗

(On Diff #240716)

`-dealloc`

10–11 ↗

(On Diff #240716)

Replace "just before an object is deallocated" with "to deallocate an object".

"Deallocates the memory occupied by the receiver."
https://developer.apple.com/documentation/objectivec/nsobject/1571947-dealloc?language=objc

11 ↗

(On Diff #240716)

Replace ", but if" with ". If"?

11 ↗

(On Diff #240716)

I think we should add a comma after `-dealloc`?

12–13 ↗

(On Diff #240716)

Isn't it a bit more nefarious than that? -dealloc in the main implementation becomes inaccessible (though the superclass can still be invoked). Perhaps it would suffice to say that "unexpected deallocation behavior may occur"?

[mwyman]

Implemented review feedback.

Updated diagnostic message to mention the category name.

clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.cpp

34 ↗

(On Diff #240716)

I would assume any non-empty -dealloc has some code the developer would want to migrate elsewhere rather than just delete, so I would prefer not to offer to outright delete it. Might be good for empty -dealloc methods or ones that just call super (pre-ARC, bah humbug!)—but I'd prefer to take that up in a follow-up change.

37–38 ↗

(On Diff #240716)

I think it's a great idea! 🙌

Done!

[stephanemoore]

One last correction and I think that you're all set!

clang-tools-extra/docs/clang-tidy/checks/list.rst

282

Please revert this line.

[mwyman]

Revert script-changed file.

[benhamilton]

LGTM, just one nit-pick.

clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.h

24 ↗

(On Diff #242371)

class DeallocInCategoryCheck final

[mwyman]

Make check class final, based on feedback.`

[mwyman]

Updated based on feedback.

[stephanemoore]

Looks good!

clang-tools-extra/clang-tidy/objc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyObjCModule
   AvoidNSErrorInitCheck.cpp
+  DeallocInCategoriesCheck.cpp
   ForbiddenSubclassingCheck.cpp
   MissingHashCheck.cpp
   ObjCTidyModule.cpp
   PropertyDeclarationCheck.cpp
   SuperSelfCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.h

+//===--- DeallocInCategoriesCheck.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_DEALLOCINCATEGORIESCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_DEALLOCINCATEGORIESCHECK_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-categories.html
+class DeallocInCategoriesCheck : public ClangTidyCheck {
+public:
+  DeallocInCategoriesCheck(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_DEALLOCINCATEGORIESCHECK_H

clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.cpp

+//===--- DeallocInCategoriesCheck.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 "DeallocInCategoriesCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+void DeallocInCategoriesCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      objcMethodDecl(hasName("dealloc"), hasDeclContext(objcCategoryImplDecl()))
+          .bind("dealloc"),
+      this);
+}
+
+void DeallocInCategoriesCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<ObjCMethodDecl>("dealloc");
+  if (MatchedDecl) {
+    diag(MatchedDecl->getLocation(),
+         "method -dealloc should not be implemented in a category");
+  }
+}
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp

 //===--- ObjCTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidNSErrorInitCheck.h"
+#include "DeallocInCategoriesCheck.h"
 #include "ForbiddenSubclassingCheck.h"
 #include "MissingHashCheck.h"
 #include "PropertyDeclarationCheck.h"
 #include "SuperSelfCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace objc {
 
 class ObjCModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidNSErrorInitCheck>(
         "objc-avoid-nserror-init");
+    CheckFactories.registerCheck<DeallocInCategoriesCheck>(
+        "objc-dealloc-in-categories");
     CheckFactories.registerCheck<ForbiddenSubclassingCheck>(
         "objc-forbidden-subclassing");
     CheckFactories.registerCheck<MissingHashCheck>(
         "objc-missing-hash");
     CheckFactories.registerCheck<PropertyDeclarationCheck>(
         "objc-property-declaration");
     CheckFactories.registerCheck<SuperSelfCheck>(
         "objc-super-self");

clang-tools-extra/docs/ReleaseNotes.rst

 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 New checks
 ^^^^^^^^^^
 
+- New :doc:`objc-dealloc-in-categories
+  <clang-tidy/checks/objc-dealloc-in-categories>` check.
+
+  Finds implementations of -dealloc in Objective-C categories.
 
 New aliases
 ^^^^^^^^^^^
 
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `modernize-use-override <modernize-use-override.html>`_, "Yes"
    `modernize-use-trailing-return-type <modernize-use-trailing-return-type.html>`_, "Yes"
    `modernize-use-transparent-functors <modernize-use-transparent-functors.html>`_, "Yes"
    `modernize-use-uncaught-exceptions <modernize-use-uncaught-exceptions.html>`_, "Yes"
    `modernize-use-using <modernize-use-using.html>`_, "Yes"
    `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-categories <objc-dealloc-in-categories.html>`_,
    `objc-forbidden-subclassing <objc-forbidden-subclassing.html>`_,
    `objc-missing-hash <objc-missing-hash.html>`_,
    `objc-property-declaration <objc-property-declaration.html>`_, "Yes"
    `objc-super-self <objc-super-self.html>`_, "Yes"
    `openmp-exception-escape <openmp-exception-escape.html>`_,
    `openmp-use-default-none <openmp-use-default-none.html>`_,
    `performance-faster-string-find <performance-faster-string-find.html>`_, "Yes"
    `performance-for-range-copy <performance-for-range-copy.html>`_, "Yes"
.. csv-table::
    `readability-qualified-auto <readability-qualified-auto.html>`_, "Yes"
    `readability-redundant-access-specifiers <readability-redundant-access-specifiers.html>`_, "Yes"
    `readability-redundant-control-flow <readability-redundant-control-flow.html>`_, "Yes"
    `readability-redundant-declaration <readability-redundant-declaration.html>`_, "Yes"
    `readability-redundant-function-ptr-dereference <readability-redundant-function-ptr-dereference.html>`_, "Yes"
    `readability-redundant-member-init <readability-redundant-member-init.html>`_, "Yes"
    `readability-redundant-preprocessor <readability-redundant-preprocessor.html>`_,
    `readability-redundant-smartptr-get <readability-redundant-smartptr-get.html>`_, "Yes"
    `readability-redundant-string-cstr <readability-redundant-string-cstr.html>`_,
    `readability-redundant-string-init <readability-redundant-string-init.html>`_, "Yes"
    `readability-simplify-boolean-expr <readability-simplify-boolean-expr.html>`_, "Yes"
    `readability-simplify-subscript-expr <readability-simplify-subscript-expr.html>`_, "Yes"
    `readability-static-accessed-through-instance <readability-static-accessed-through-instance.html>`_, "Yes"
    `readability-static-definition-in-anonymous-namespace <readability-static-definition-in-anonymous-namespace.html>`_, "Yes"
    `readability-string-compare <readability-string-compare.html>`_, "Yes"
    `readability-uniqueptr-delete-release <readability-uniqueptr-delete-release.html>`_, "Yes"
    `readability-uppercase-literal-suffix <readability-uppercase-literal-suffix.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-categories.rst

+.. title:: clang-tidy - objc-dealloc-in-categories
+
+objc-dealloc-in-categories
+==========================
+
+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 just before an
+object is deallocated, but if a category on the class implements ``-dealloc``
+it will override the class's implementation and those important actions may
+not be handled by the overriding ``-dealloc``.

clang-tools-extra/test/clang-tidy/checkers/objc-dealloc-in-categories.m

+// RUN: %check_clang_tidy %s objc-dealloc-in-categories %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 (Category)
+@end
+
+@implementation Bar (Category)
+- (void)dealloc {
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: method -dealloc should not be implemented in a category [objc-dealloc-in-categories]
+}
+@end
+
+@interface Baz : NSObject
+@end
+
+@interface Baz (Category)
+// A declaration in a category @interface does not by itself provide an
+// overriding implementation, and should not generate a warning.
+- (void)dealloc;
+@end