[PATCH] D65917: [clang-tidy] Added check for the Google style guide's category method naming rule.

[David Gatwood via Phabricator via cfe-commits]

dgatwood marked an inline comment as done.
dgatwood added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:23
+  Finder->addMatcher(objcMethodDecl(hasDeclContext(
+      objcCategoryDecl())).bind(CustomCategoryMethodIdentifier), this);
+}
----------------
stephanemoore wrote:
> dgatwood wrote:
> > stephanemoore wrote:
> > > Technically category method prefixing is only strictly enforced on classes that are shared:
> > > "If a class is not shared with other projects, categories extending it may omit name prefixes and method name prefixes."
> > > https://github.com/google/styleguide/blob/gh-pages/objcguide.md#category-naming
> > > 
> > > With that in mind, perhaps we should match on categories on classes that extend classes that are declared in system headers? I think you can accomplish that by adding a custom inner matcher in `objcCategoryDecl` which pulls out the Objective-C interface using [clang::ObjCCategoryDecl::getClassInterface](https://clang.llvm.org/doxygen/classclang_1_1ObjCCategoryDecl.html#acdb14eeca277cfa745a4e8e842312008) and then add `isExpansionInSystemHeader` as an inner matcher on the custom inner matcher. WDYT? Let me know if you need help putting together.
> > Requiring users to specify which classes should be covered by this checker doesn't scale well.  System classes are a tiny fraction of the shared code that we bring in.  Proto classes alone probably outnumber system framework classes 10:1, plus all the shared code from other internal framework teams.  A list of every shared class that we bring in would be massive, and generating it programmatically would be relatively expensive.  The same problem exists for a list of prefixes to protect.
> > 
> > Also with that approach, a mistake by a person or script that maintains such a list would result in **not** getting warnings.  Silent failures are the worst kind of failure, because you don't even know that something is going wrong.  By contrast, if you require the user to specify a list of prefixes to ignore, as this patch does, then any mistake by someone maintaining the lest results in getting **extra** warnings, which makes it obvious that something is wrong and needs to be fixed.
> > 
> > I realize that ostensibly a team could own some code that is also shared, and it could have the same prefix as their app.  But there's no real reason to care about that case.  After all, if someone changes the shared code and it breaks the category, it's the same team, so their tests should catch the breakage, unlike changes made by some far-flung team on the other side of the world.  Also, if they break their own code, they also have permission to fix that breakage without additional approvals.  So that edge case is largely academic; if anybody asks for a way to not ignore specific classes that have an exempt prefix, we can certainly add that feature later pretty easily, but I really doubt anybody would bother to use it.  :-)
> > Requiring users to specify which classes should be covered by this checker doesn't scale well.
> 
> I am not convinced that listing exemptions scales well either. In [D51832](https://reviews.llvm.org/D51832), we abandoned a growing list of exemptions that were being used to enforce naming conventions. I am worried that a list of exempt prefixes will also end up growing in a similar fashion.
> 
> > System classes are a tiny fraction of the shared code that we bring in.
> 
> That is true.
> 
> However, system classes generally have the most breadth and are the most sensitive (for example, there was a recent bug introduced when someone declared a category on `UIImageView` with an unprefixed getter named `animationDuration` which overrode the actual [`animationDuration`](https://developer.apple.com/documentation/uikit/uiimageview/1621058-animationduration?language=objc) property on `UIImageView`).
> 
> > Proto classes alone probably outnumber system framework classes 10:1, plus all the shared code from other internal framework teams.
> 
> That is true but proto classes and internal shared code also tend to have much more limited scope.
> 
> My concern with the check in its current form is that it has a strong potential to be noisy by default. That is, if you enable this check without specifying any exempt prefixes then it will warn for _any category_, including categories on classes that are not shared which do not require a prefix. Moreover, the check provides no option to exempt classes that do not have prefixes at all (prefixes are only required for classes that are shared per [Google Objective-C class naming guidelines](https://github.com/google/styleguide/blob/gh-pages/objcguide.md#class-names)).
> 
> Generated Objective-C Protocol Buffer classes should be pretty easy to target since they have a common base class of `GPBMessage`. It should be pretty simple to check if the category is on a class that is derived from `GPBMessage`. Note, however, that there are situations where an unprefixed method declaration in a category on an Objective-C Protocol Buffer class is justified, e..g, when overriding `-isEqual:` or `-hash` as [suggested by the Objective-C Protocol Buffers library](https://github.com/protocolbuffers/protobuf/blob/ffa6bfc/objectivec/GPBMessage.m#L2749).
> 
> In general, I would expect there to be less shared code than there is unshared code (compare how many classes are declared in Apple system framework headers to how many classes are collectively declared in macOS/iOS/watchOS/tvOS applications globally). For that reason, I think a better approach would be to opt shared code into enforcement rather than opt unshared code out.
> 
> I can imagine a matching structure like so (`hasInterface`,  `baseClassRequiresMethodPrefixing`, and `classRequiresMethodPrefixing` would be custom matchers that would need to be implemented in this example):
> ```
> objcMethodDecl(
>   hasDeclContext(
>     objcCategoryDecl(
>       hasInterface(
>         anyOf(
>           isExpansionInSystemHeader(),
>           isDerivedFrom(baseClassRequiresMethodPrefixing(BaseClassNames)),
>           classRequiresMethodPrefixing(ClassNamePatterns)
>         )
>       )
>     )
>   )
> )
> ```
> That would allow opting in class hierarchies by base class (handling Objective-C Protocol Buffer classes) as well as opting in classes in shared libraries using the prefix of the shared library.
> 
> WDYT?
The difference is that the exemption list would be per-project, created by whoever turns on the check for the project.  I was assuming that this checker would be turned on by each team, rather than globally, and that part of doing so would be specifying which class prefixes should be ignored for that particular project.

That said, if you want to turn this check on globally, we probably should support either approach, with the default being to check only categories on system frameworks and classes derived from a specific set of opted-in base classes.  Then, if a project owner provides a set of prefixes to exempt, it would expand the check to cover everything not exempted.  (That's the mode in which we would prefer to use the feature for our team's project.)

Either way, it is a good idea to auto-exempt any class that isn’t prefixed.

So basically, I'm thinking:

**If no prefix list is provided:**

Categories on:

  - System framework classes.
  - A specific list of protected classes and subclasses thereof.

**If a prefix list is provided:**

Categories on all classes EXCEPT:

  - Classes whose prefixes are explicitly excepted.
  - Classes that do not start with at least three capital letters (non-prefixed classes).

Does that approach seem reasonable to you?


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

https://reviews.llvm.org/D65917