[PATCH] D72876: Create a clang-tidy check to warn when -dealloc is implemented inside an ObjC class category.

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore added a comment.

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



================
Comment at: clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.cpp:34
+
+void DeallocInCategoryCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<ObjCMethodDecl>("dealloc");
----------------
What do you think of making a fixit that deletes the `-dealloc` implementation?


================
Comment at: clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.cpp:36
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<ObjCMethodDecl>("dealloc");
+  if (MatchedDecl) {
+    diag(MatchedDecl->getLocation(),
----------------
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


================
Comment at: clang-tools-extra/clang-tidy/objc/DeallocInCategoryCheck.cpp:37-38
+  if (MatchedDecl) {
+    diag(MatchedDecl->getLocation(),
+         "method -dealloc should not be implemented in a category");
+  }
----------------
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)


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst:7
+Finds implementations of ``-dealloc`` in Objective-C categories. The category
+implementation will override any dealloc in the class implementation,
+potentially causing issues.
----------------
``-dealloc``


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst:10-11
+
+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
----------------
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


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst:11
+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
----------------
Replace ", but if" with ". If"?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst:11
+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
----------------
stephanemoore wrote:
> Replace ", but if" with ". If"?
I //think// we should add a comma after ``-dealloc``?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-category.rst:12-13
+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``.
----------------
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"?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72876/new/

https://reviews.llvm.org/D72876