Page MenuHomePhabricator

[clang-tidy] Added check for the Google style guide's category method naming rule.
Needs RevisionPublic

Authored by dgatwood on Aug 7 2019, 6:25 PM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Eugene.Zelenko added inline comments.Aug 7 2019, 10:06 PM
clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
32

Please enclose QED in double back-ticks.

Eugene.Zelenko retitled this revision from Added clang-tidy module for the Google style guide's category method naming rule. to [clang-tidy] Added check for the Google style guide's category method naming rule..Aug 7 2019, 10:06 PM
Eugene.Zelenko added a project: Restricted Project.
aaron.ballman added inline comments.Aug 8 2019, 12:53 PM
clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
44

Please keep this list in alphabetical order.

clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
22

You should elide the braces here per our usual coding conventions (also applies elsewhere).

26

No need for the local here, I'd just inline it below.

38

I think this interface should accept a StringRef argument and either return a std::string or an llvm::Optional<std::string> rather than allocating a pointer.

44

No need for messy C APIs here -- you can compare a StringRef to a std::string directly.

58

This should use getName() to get a StringRef to avoid the copy.

63

Can use getName() and assign to a StringRef

101

Drop the period at the end of the diagnostic -- clang-tidy diagnostics are not meant to be grammatically correct.

102

You should pass method_declaration instead; it will get quoted and converted to the proper identifier automatically.

clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.h
29

You should pick names that match the LLVM coding style naming rules (here and elsewhere in the patch).

Whoops. I was expecting to get emailed about review comments and never got anything, so I thought it was just not getting reviewed. I'll work on this again. Sorry about that. :-)

dgatwood updated this revision to Diff 218478.Sep 3 2019, 10:02 AM

Incorporated feedback.

Eugene.Zelenko added inline comments.Sep 3 2019, 10:04 AM
clang-tools-extra/docs/ReleaseNotes.rst
70

Please sort new checks list alphabetically.

dgatwood marked 17 inline comments as done.Sep 3 2019, 10:16 AM
dgatwood added inline comments.
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
58

That's actually what I originally tried, but that method won't work here, unless I'm missing something. The getName() method crashes with a message saying that "Name is not a simple identifier".

clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
20

Removed the text in question, because it duplicates the Options info.

aaron.ballman added inline comments.Sep 4 2019, 2:40 PM
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
17

This comment also meant that you should drop the anonymous namespace (per our coding conventions -- we only use anonymous namespaces for types).

20

Finder

31

ExpectedPrefixes here as well.

Should there be a default list of these?

40–44

llvm::find_if(PrefixArray, [ClassName](const std::string &Str) { return ClassName.startswith(Str); });

50

MethodDeclaration

57

MethodName (and so on, I'll stop commenting on them.)

58

You can call getIdentifier() instead, and if you get a non-null object back, you can call getName() on that. If it is null, there's nothing to check.

58

Drop the clang::

65

Can use const auto * because the type is in the initialization. However, drop the clang:: as it's not needed.

76

I notice we never actually care about the string contained within MatchingPrefix, which suggests that matchingWhitelistedPrefix() could just return a bool.

clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.h
23–24

Finder and Result per coding style.

29–32

I think the name ExpectedPrefixes and matchesExpectedPrefix() are more clear.

32

class_name should be ClassName.

clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
5

Underlining looks off here.

dgatwood updated this revision to Diff 218812.Sep 4 2019, 4:51 PM
dgatwood marked 22 inline comments as done.

Incorporated additional feedback.

aaron.ballman added inline comments.Sep 5 2019, 4:59 AM
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
16

Drop the k prefix (we don't use Hungarian notation).

31

Still wondering whether we should have a default list of expected prefixes or not.

37–41

Sorry for not recognizing this earlier, but since we don't care about which item was found, we can go with:

return llvm::any_of(PrefixArray, ...);
58

The comment to use getIdentifier() was marked as done but the changes were not applied; was that a mistake? I'm pushing back on getNameAsString() because the function is commented as having its use discouraged, so we should not be adding new uses of it.

clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.h
23–24

finder -> Finder
result -> Result

32

class_name -> ClassName

dgatwood updated this revision to Diff 218953.Sep 5 2019, 10:40 AM
dgatwood marked 7 inline comments as done.

Fixed a couple of variable names in the .h file, renamed kCustomCategoryMethodIdentifier to CustomCategoryMethodIdentifier, and switched to llvm::any_of.

Ah. I expected my comments to be submitted as part of uploading a new revision. Still learning this UI. :-/

clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
31

Done. And no, there should be no default, unless somehow Xcode's project prefix makes it down as far as LLVM, in which case maybe that could be the default.

The idea is that you can whitelist your own Xcode project's prefix, along with the prefixes of your own in-house libraries, so that each individual team/workgroup can add categories on their own classes, but will get warned when they try to add unprefixed category methods on classes that they don't own (e.g. classes in system frameworks, third-party frameworks, etc.).

31

This is weird. I don't know why this comment system didn't submit my comment before.

No, there should be no default, unless somehow Xcode's project prefix makes it down as far as LLVM, in which case maybe that could be the default.

The idea is that you can whitelist your own Xcode project's prefix, along with the prefixes of your own in-house libraries, so that each individual team/workgroup can add categories on their own classes, but will get warned when they try to add unprefixed category methods on classes that they don't own (e.g. classes in system frameworks, third-party frameworks, etc.).

57

Yeah. I finally realized I should just grep for '_' instead of looking for them by hand — one of the joys of converting something originally written under Google's style guide so that it conforms to the llvm style guide instead. Sorry about that. I hope I got all of them this time.

58

I just tried it, and getIdentifier() returns NULL consistently for every category method, so I changed it back to getNameAsString(), which works.

58

I marked that as done because I tried it and it didn't work. The getIdentifier() method returned NULL for every category method.

BTW, this isn't my first attempt at writing this code in a way that doesn't require that method. I literally fought with getting the name of category methods for a day or more when I first started writing this, because I kept getting NULLs or crashes. At one point, I think I even tried looking for the owning class and querying its interface. Nothing worked until I discovered getNameAsString().

I'm assuming that this is simply a bug somewhere in the LLVM core that nobody has noticed or bothered to fix, because it really should not be difficult to get the name of a method. :-/

76

We could. Originally, I was printing it for diagnostics, but now that I ripped out the debug logging, it isn't really needed. Done.

clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.h
23–24

What the heck? Oh, I fixed it in the .cc file and not the .h file. Sorry. Done.

aaron.ballman added inline comments.Sep 6 2019, 5:21 AM
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
31

Ah, thank you for the explanation!

57

No worries, things are looking good now!

58

I'm assuming that this is simply a bug somewhere in the LLVM core that nobody has noticed or bothered to fix, because it really should not be difficult to get the name of a method. :-/

I am not certain if it's a bug or not, but we shouldn't be using essentially deprecated APIs to work around bugs elsewhere. I'd really like to know what's going on here. I am looking through the logic of DeclarationName::print() (which is what getNameAsString() eventually calls into) and it doesn't look like it makes any special accommodations for ObjC method declarations, but it does for ObjC selectors. I'm not an ObjC expert, but I think that's what the name of an ObjC method is, isn't it? If so, I think you would do (assuming the methods can only be named using selectors):

MethodDeclaration->getDeclName().getObjCSelector().getAsString()
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.h
23–24

No worries!

clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
29

whitelist the QED three-letter prefix -> expect the three-letter prefix QED

dgatwood updated this revision to Diff 219151.Sep 6 2019, 11:59 AM
dgatwood marked 9 inline comments as done.

Tweaked documentation, and replaced the one remaining instance of getNameAsString().

clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
58

Yeah, any Objective-C method inside a class is inherently named by a selector. That code works. Thanks. :-)

clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
29

Applied approximately. :-)

This revision is now accepted and ready to land.Sep 7 2019, 1:14 PM
stephanemoore requested changes to this revision.Sep 11 2019, 4:52 AM
stephanemoore marked an inline comment as done.
stephanemoore added inline comments.
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
23–24

What do you think of objcMethodDecl(hasDeclContext(objcCategoryDecl()))? I think that more narrowly targets the method declarations that we are interested in. There are other method declarations that we like to prefix but I consider those outside the domain of category method name prefixing practices.

31

Agreed that there should be no default.

This revision now requires changes to proceed.Sep 11 2019, 4:52 AM
dgatwood updated this revision to Diff 220123.Sep 13 2019, 9:20 AM

Switched to objcMethodDecl(hasDeclContext(objcCategoryDecl())).

dgatwood marked an inline comment as done.Sep 13 2019, 9:21 AM
stephanemoore requested changes to this revision.Sep 16 2019, 7:02 PM
stephanemoore added inline comments.
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
24

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 and then add isExpansionInSystemHeader as an inner matcher on the custom inner matcher. WDYT? Let me know if you need help putting together.

clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
42

This option seems to describe a list of class prefixes that are whitelisted. If that is the case, perhaps WhitelistedPrefixes or WhitelistedClassPrefixes would be a better name for the option? WDYT?

This revision now requires changes to proceed.Sep 16 2019, 7:02 PM
aaron.ballman added inline comments.Sep 17 2019, 4:34 AM
clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
42

No, please. We should try to avoid "white" and "black" lists in favor of more descriptive terms with less social connotations. I'd be fine with ExpectedClassPrefixes, AllowedPrefixes, KnownPrefixes, etc.

alexfh removed a reviewer: alexfh.Sep 17 2019, 5:32 AM
stephanemoore added inline comments.Sep 17 2019, 1:48 PM
clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
42

Makes sense. Would ExemptClassPrefixes work?

aaron.ballman added inline comments.Sep 18 2019, 5:05 AM
clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst
42

I think that'd be a great name.

dgatwood updated this revision to Diff 221128.Sep 20 2019, 4:16 PM
dgatwood marked 5 inline comments as done.

Renamed to ExemptClassPrefixes.

clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
24

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. :-)

dgatwood updated this revision to Diff 221142.Sep 20 2019, 4:59 PM

Updated the configuration key in the test file.

stephanemoore requested changes to this revision.Sep 20 2019, 5:42 PM
stephanemoore added inline comments.
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
24

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, 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).

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.

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?

This revision now requires changes to proceed.Sep 20 2019, 5:42 PM
dgatwood marked an inline comment as done.Sep 23 2019, 11:51 AM
dgatwood added inline comments.
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp
24

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?

(sorry for the delay 😅)

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.

The vision that I had in mind for this check was for it to be enabled more widely which would require it to be useful by default without requiring options to be specified to reduce noise.

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.

That's a good idea but note that detecting that a class name does not have a prefix is not easy because acronyms and initialisms are capitalized (e.g., a class named URLCache probably does not have a prefix).

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?

I think we are moving in a good direction. I suspect that a single option may not be the best to control the relevant behaviors.

It sounds like we want the following behaviors:
(1) Require method prefixes on categories on system classes.
(2) Require method prefixes on categories on user classes except (2A) user classes whose names begin with an exempted prefix and (2B) user classes that are derived from an exempted base class.

I think it's unlikely that we would want (1) to be conditional but I think the others are likely options that developers would want to control. Based on that judgment call, I would imagine three options like so (option names have not been thought through extensively): IncludeUserClasses, ExemptUserClassPrefixes, and ExemptUserBaseClasses. That way when the check is enabled without any options the developer gets enforcement on system classes which should reduce a common source of errors and developers that want enforcement on their own classes can opt in and use the additional options to control enforcement and reduce noise.

WDYT?

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

Technically this check is not specific to Google Objective-C:
"In order to avoid undefined behavior, it’s best practice to add a prefix to method names in categories on framework classes, just like you should add a prefix to the names of your own classes. You might choose to use the same three letters you use for your class prefixes, but lowercase to follow the usual convention for method names, then an underscore, before the rest of the method name."
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/CustomizingExistingClasses/CustomizingExistingClasses.html

I think we should consider moving this to the objc module (rename_check.py might be used if we decide that the move is justified).