Page MenuHomePhabricator

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

Authored by mwyman on Jan 16 2020, 2:05 PM.

Details

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.

Diff Detail

Event Timeline

mwyman created this revision.Jan 16 2020, 2:05 PM
dmaclach requested changes to this revision.Jan 16 2020, 2:13 PM
dmaclach added a subscriber: dmaclach.
dmaclach added inline comments.
clang-tools-extra/docs/clang-tidy/checks/objc-dealloc-in-categories.rst
10 ↗(On Diff #238611)

Need an end quote on dealloc

This revision now requires changes to proceed.Jan 16 2020, 2:13 PM
mwyman updated this revision to Diff 238621.Jan 16 2020, 2:19 PM

Fixed missing end quote pointed out in review comment.

mwyman marked an inline comment as done.Jan 16 2020, 2:19 PM
mgehre removed a subscriber: mgehre.Jan 16 2020, 2:44 PM
dmaclach accepted this revision.Jan 16 2020, 2:48 PM
This revision is now accepted and ready to land.Jan 16 2020, 2:48 PM
stephanemoore requested changes to this revision.Jan 24 2020, 4:39 PM
stephanemoore added inline comments.
clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.cpp
21 ↗(On Diff #238621)

Add isInstanceMethod() within the objcMethodDecl?

clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.h
24 ↗(On Diff #238621)

What do you think of the name DeallocInCategoryCheck?

clang-tools-extra/test/clang-tidy/checkers/objc-dealloc-in-categories.m
23 ↗(On Diff #238621)

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

35 ↗(On Diff #238621)

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?

This revision now requires changes to proceed.Jan 24 2020, 4:39 PM
stephanemoore added inline comments.Jan 24 2020, 4:59 PM
clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.cpp
19 ↗(On Diff #238621)

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 ↗(On Diff #238621)

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 updated this revision to Diff 240716.Jan 27 2020, 4:21 PM
mwyman marked 7 inline comments as done.

Addresses reviewer feedback.

Updated the diff based on review feedback.

clang-tools-extra/clang-tidy/objc/DeallocInCategoriesCheck.cpp
21 ↗(On Diff #238621)

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 ↗(On Diff #238621)

Much better.

mwyman marked an inline comment as done.Jan 28 2020, 11:50 AM

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

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

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

37

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

38–39

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
8

`-dealloc`

11–12

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

12

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

12

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

13–14

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 updated this revision to Diff 241573.Jan 30 2020, 1:40 PM
mwyman marked 9 inline comments as done.

Implemented review feedback.

Updated diagnostic message to mention the category name.

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

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.

38–39

I think it's a great idea! 🙌

Done!

stephanemoore requested changes to this revision.Jan 31 2020, 6:24 PM

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

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

Please revert this line.

This revision now requires changes to proceed.Jan 31 2020, 6:24 PM
mwyman updated this revision to Diff 242371.Feb 4 2020, 10:30 AM

Revert script-changed file.

mwyman marked an inline comment as done.Feb 4 2020, 10:30 AM
benhamilton accepted this revision.Feb 6 2020, 10:01 AM

LGTM, just one nit-pick.

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

class DeallocInCategoryCheck final

mwyman updated this revision to Diff 242947.Feb 6 2020, 10:43 AM
mwyman marked an inline comment as done.

Make check class final, based on feedback.`

Updated based on feedback.

stephanemoore accepted this revision.Feb 6 2020, 2:10 PM

Looks good!

This revision is now accepted and ready to land.Feb 6 2020, 2:10 PM
This revision was automatically updated to reflect the committed changes.