[clang-tidy/checks] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜

[stephanemoore]

Summary

§1 Description

This check finds function names in function declarations in Objective-C files that do not follow the naming pattern described in the Google Objective-C Style Guide. Function names should be in UpperCamelCase and functions that are not of static storage class should have an appropriate prefix as described in the Google Objective-C Style Guide. The function main is a notable exception. Function declarations in expansions in system headers are ignored.

Example conforming function definitions:

static bool IsPositive(int i) { return i > 0; }
static bool ABIsPositive(int i) { return i > 0; }
bool ABIsNegative(int i) { return i < 0; }

A fixit hint is generated for functions of static storage class but otherwise the check does not generate a fixit hint because an appropriate prefix for the function cannot be determined.

§2 Test Notes

• Verified clang-tidy tests pass successfully.
• Used check_clang_tidy.py to verify expected output of processing google-objc-function-naming.m

[stephanemoore]

Minor fixes:

• Fixed header guard.
• Removed unnecessary imports in header.

[Eugene.Zelenko]

docs/ReleaseNotes.rst

60 ↗

(On Diff #163638)

Please use alphabetical order.

[stephanemoore]

Fixed alphabetical ordering of clang-tidy improvements in release notes.

[stephanemoore]

docs/ReleaseNotes.rst

60 ↗

(On Diff #163638)

Good catch. Fixed.

[hokein]

Nice! looks mostly good to me.

clang-tidy/google/FunctionNamingCheck.cpp

57 ↗

(On Diff #163647)

any reason why we restrict to definitions only? I think we can consider declarations too.

unittests/clang-tidy/GoogleModuleTest.cpp

109 ↗

(On Diff #163647)

nit: we don't need unittest for the check here, as it is well covered in the littest.

[stephanemoore]

Updated with changes:

• Removed unit tests as other tests have been indicated to provide adequate coverage.
• Added a comment explaining why only function definitions are evaluated.

[stephanemoore]

clang-tidy/google/FunctionNamingCheck.cpp

57 ↗

(On Diff #163647)

I restricted the check to function definitions because function declarations are sometimes associated with functions outside the control of the author. I have personally observed unfortunate cases where functions declared in the iOS SDK had incorrect—or seemingly incorrect—availability attributes that caused fatal dyld assertions during application startup. The check currently intends to avoid flagging function declarations because of the rare circumstances where an inflexible function declaration without a corresponding function definition is required.

I have added a comment explaining why only function definitions are flagged.

I am still open to discussion though.

unittests/clang-tidy/GoogleModuleTest.cpp

109 ↗

(On Diff #163647)

Removed the unit tests.

[hokein]

clang-tidy/google/FunctionNamingCheck.cpp

57 ↗

(On Diff #163647)

Thanks for the explanations.

I have a concern about the heuristic using here, it seems fragile -- if there is an inline function defined in a base header, the check will still give a warning to it if the source file .m #includes this header; it also limits the scope of the check, I think this check is flagged mostly on file-local functions (e.g. static functions, functions defined in anonymous namespace).

Flagging on function declaration doesn't seem too problematic (we already have a similar check objc-property-declaration does the same thing) -- our internal review system shows check warnings on changed code, if user's code includes a common header which violates the check, warnings on the header will not be shown; for violations in iOS SDK, we can use some filtering matchers (isExpansionInSystemHeader maybe work) to ignore all functions from these files.

[Wizard]

clang-tidy/google/FunctionNamingCheck.cpp

50 ↗

(On Diff #163656)

Can we do some simple check to see if some easy fix can be provided just like objc-property-declaration check?
Something like static bool isPositive to static bool IsPositive and static bool is_upper_camel to IsUpperCamel. Such check can help provide code fix for a lot of very common mistake at a low cost (i.e. if the naming pattern cannot be simply recognized, just provide no fix).

[benhamilton]

Thanks for this! Let's consolidate this with the property name checker (either simplify the logic there and allow arBiTRAryCapSAnYWHere or apply the same registered acronym logic here).

clang-tidy/google/FunctionNamingCheck.cpp

35 ↗

(On Diff #163656)

Any reason why this is different from the implementation in the property name checker? Either we should allow both of:

void ARBiTraRilyNameDFuncTioN();
// ...
@property (...) id arBItrArIlyNameD;

or we should require that acronyms in the middle of the name be registered/known acronyms for both properties and functions.

I believe this diff allows arbitrary capitalization for functions, but we disallowed that for property names, so I think we should be consistent.

50 ↗

(On Diff #163656)

+1, I think the two checks should be substantially similar.

clang-tidy/google/FunctionNamingCheck.h

21 ↗

(On Diff #163656)

Worth mentioning this does not apply to Objective-C method names, nor Objective-C properties.

[benhamilton]

clang-tidy/google/FunctionNamingCheck.cpp

35 ↗

(On Diff #163656)

(And, just to be clear, I don't feel strongly which direction we go — it's certainly less work to allow arbitrarily-named functions and properties than to maintain the acronym list.)

[stephanemoore]

Implemented the following suggested changes:
• Restricted matching to function declarations that are not in expansions in system headers.
• Extended the check to all function declarations rather than just function definitions.
• Implemented a fixit hint for functions of static storage class.

[stephanemoore]

Implemented the following suggested changes:
• Added a note that FunctionNamingCheck does not apply to Objective-C method name or property declarations.

[stephanemoore]

Cleaned up comment about FunctionNamingCheck not applying to Objective-C method or property declarations.

[stephanemoore]

clang-tidy/google/FunctionNamingCheck.cpp

57 ↗

(On Diff #163647)

Good idea to use isExpansionInSystemHeader. I wasn't aware of that particular matcher. I have incorporated that into the matching.

I am still wary about flagging function declarations but I think that false positives should generally be marginal and we can monitor for them. I have extended the check to also include function declarations.

35 ↗

(On Diff #163656)

I have been meaning to send out a proposal to change the objc-property-declaration check to allow arbitrary capitalization but I ended up sending this out first. Let me prep my proposal for that change simultaneously.

50 ↗

(On Diff #163656)

Implemented a fixit hint for functions of static storage class.

clang-tidy/google/FunctionNamingCheck.h

21 ↗

(On Diff #163656)

Added a note that this check does not apply to Objective-C methods or properties.

[stephanemoore]

https://reviews.llvm.org/D51832 is in review to update the objc-property-declaration check to allow arbitrary acronyms and initialisms.

[stephanemoore]

Updated diff after pulling and merging.

[benhamilton]

docs/clang-tidy/checks/google-objc-function-naming.rst

20 ↗

(On Diff #172296)

This is not actually handled by the check, right? (You even have a test which confirms this.)

As far as I can tell, this check essentially looks for at least one capital letter and no _ in function names, right?

[stephanemoore]

Added a test case for an extern function without an appropriate prefix (IsPrime).

[stephanemoore]

docs/clang-tidy/checks/google-objc-function-naming.rst

20 ↗

(On Diff #172296)

It does flag this declaration because it's missing an appropriate prefix.

It looks like I neglected to add a test case for that. I have done so now.

[aaron.ballman]

clang-tidy/google/FunctionNamingCheck.cpp

55 ↗

(On Diff #173244)

Elide braces.

59–60 ↗

(On Diff #173244)

Do not use auto as the type is not mentioned in the initialization.

66 ↗

(On Diff #173244)

Please make this the while loop condition rather than an explicit break.

86 ↗

(On Diff #173244)

Elide braces.

98 ↗

(On Diff #173244)

Elide braces. Also, didn't we get rid of the distinction between ObjcC1 and ObjC2?

116 ↗

(On Diff #173244)

No need for this assertion. The check() function cannot be called without this being nonnull.

119 ↗

(On Diff #173244)

I don't think this assert adds value.

122–124 ↗

(On Diff #173244)

You can drop the explicit quotes around %0 and instead pass in MatchedDecl rather than MatchedDecl->getName().

[stephanemoore]

Updated with the following changes:
• Elided conditional braces to comply with LLVM style.
• Converted conditional loop break to loop condition in generateFixItHint.
• Fixed Objective-C language option check.
• Removed unnecessary assertions.
• Use MatchedDecl directly for formatting the diagnostic message.

[aaron.ballman]

Generally LGTM, but I leave it to someone more well-versed in ObjC to sign off that this actually does everything those users would expect.

docs/clang-tidy/checks/google-objc-function-naming.rst

12 ↗

(On Diff #173544)

"Upper camel case" is more commonly known as Pascal case, btw (http://wiki.c2.com/?PascalCase).

[stephanemoore]

Reworded "upper camel case" to "Pascal case".

[stephanemoore]

Thanks for the review everyone!

Let me know if there are any further changes that you want me to make or any further action required on my part to land this 😁

[aaron.ballman]

In D51575#1297664, @stephanemoore wrote:

Thanks for the review everyone!

Let me know if there are any further changes that you want me to make or any further action required on my part to land this 😁

It's good to go in -- do you have commit access, or would you like someone to land this for you?

[stephanemoore]

I received commit access yesterday so I believe I should be able to land this myself (after I get myself set up and land a test commit).

[stephanemoore]

clang-tidy/google/FunctionNamingCheck.cpp

63 ↗

(On Diff #173936)

I forgot to invert this condition when I refactored the early return into the loop condition 😓

[stephanemoore]

Fixed loop condition in generateFixItHint.

clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyGoogleModule
   AvoidCStyleCastsCheck.cpp
   AvoidThrowingObjCExceptionCheck.cpp
   DefaultArgumentsCheck.cpp
   ExplicitConstructorCheck.cpp
   ExplicitMakePairCheck.cpp
+  FunctionNamingCheck.cpp
   GlobalNamesInHeadersCheck.cpp
   GlobalVariableDeclarationCheck.cpp
   GoogleTidyModule.cpp
   IntegerTypesCheck.cpp
   NonConstReferences.cpp
   OverloadedUnaryAndCheck.cpp
   TodoCommentCheck.cpp
   UnnamedNamespaceInHeaderCheck.cpp

clang-tools-extra/trunk/clang-tidy/google/FunctionNamingCheck.h

+//===--- FunctionNamingCheck.h - clang-tidy ---------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_OBJC_FUNCTION_NAMING_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_OBJC_FUNCTION_NAMING_CHECK_H
+
+#include "../ClangTidy.h"
+#include "llvm/ADT/StringRef.h"
+
+namespace clang {
+namespace tidy {
+namespace google {
+namespace objc {
+
+/// Finds function names that do not conform to the recommendations of the
+/// Google Objective-C Style Guide. Function names should be in upper camel case
+/// including capitalized acronyms and initialisms. Functions that are not of
+/// static storage class must also have an appropriate prefix. The function
+/// `main` is an exception. Note that this check does not apply to Objective-C
+/// method or property declarations.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/google-objc-function-naming.html
+class FunctionNamingCheck : public ClangTidyCheck {
+public:
+  FunctionNamingCheck(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 google
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_OBJC_FUNCTION_NAMING_CHECK_H

clang-tools-extra/trunk/clang-tidy/google/FunctionNamingCheck.cpp

+//===--- FunctionNamingCheck.cpp - clang-tidy -----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "FunctionNamingCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/Support/Regex.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace google {
+namespace objc {
+
+namespace {
+
+std::string validFunctionNameRegex(bool RequirePrefix) {
+  // Allow the following name patterns for all functions:
+  // • ABFoo (prefix + UpperCamelCase)
+  // • ABURL (prefix + capitalized acronym/initialism)
+  //
+  // If no prefix is required, additionally allow the following name patterns:
+  // • Foo (UpperCamelCase)
+  // • URL (capitalized acronym/initialism)
+  //
+  // The function name following the prefix can contain standard and
+  // non-standard capitalized character sequences including acronyms,
+  // initialisms, and prefixes of symbols (e.g., UIColorFromNSString). For this
+  // reason, the regex only verifies that the function name after the prefix
+  // begins with a capital letter followed by an arbitrary sequence of
+  // alphanumeric characters.
+  //
+  // If a prefix is required, the regex checks for a capital letter followed by
+  // another capital letter or number that is part of the prefix and another
+  // capital letter or number that begins the name following the prefix.
+  std::string FunctionNameMatcher =
+      std::string(RequirePrefix ? "[A-Z][A-Z0-9]+" : "") + "[A-Z][a-zA-Z0-9]*";
+  return std::string("::(") + FunctionNameMatcher + ")$";
+}
+
+/// For now we will only fix functions of static storage class with names like
+/// 'functionName' or 'function_name' and convert them to 'FunctionName'. For
+/// other cases the user must determine an appropriate name on their own.
+FixItHint generateFixItHint(const FunctionDecl *Decl) {
+  // A fixit can be generated for functions of static storage class but
+  // otherwise the check cannot determine the appropriate function name prefix
+  // to use.
+  if (Decl->getStorageClass() != SC_Static)
+    return FixItHint();
+
+  StringRef Name = Decl->getName();
+  std::string NewName = Decl->getName().str();
+
+  size_t Index = 0;
+  bool AtWordBoundary = true;
+  while (Index < NewName.size()) {
+    char ch = NewName[Index];
+    if (isalnum(ch)) {
+      // Capitalize the first letter after every word boundary.
+      if (AtWordBoundary) {
+        NewName[Index] = toupper(NewName[Index]);
+        AtWordBoundary = false;
+      }
+
+      // Advance the index after every alphanumeric character.
+      Index++;
+    } else {
+      // Strip out any characters other than alphanumeric characters.
+      NewName.erase(Index, 1);
+      AtWordBoundary = true;
+    }
+  }
+
+  // Generate a fixit hint if the new name is different.
+  if (NewName != Name)
+    return FixItHint::CreateReplacement(
+        CharSourceRange::getTokenRange(SourceRange(Decl->getLocation())),
+        llvm::StringRef(NewName));
+
+  return FixItHint();
+}
+
+} // namespace
+
+void FunctionNamingCheck::registerMatchers(MatchFinder *Finder) {
+  // This check should only be applied to Objective-C sources.
+  if (!getLangOpts().ObjC)
+    return;
+
+  // Match function declarations that are not in system headers and are not
+  // main.
+  Finder->addMatcher(
+      functionDecl(
+          unless(isExpansionInSystemHeader()),
+          unless(anyOf(isMain(), matchesName(validFunctionNameRegex(true)),
+                       allOf(isStaticStorageClass(),
+                             matchesName(validFunctionNameRegex(false))))))
+          .bind("function"),
+      this);
+}
+
+void FunctionNamingCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("function");
+
+  diag(MatchedDecl->getLocation(),
+       "function name %0 not using function naming conventions described by "
+       "Google Objective-C style guide")
+      << MatchedDecl << generateFixItHint(MatchedDecl);
+}
+
+} // namespace objc
+} // namespace google
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp

 #include "../readability/BracesAroundStatementsCheck.h"
 #include "../readability/FunctionSizeCheck.h"
 #include "../readability/NamespaceCommentCheck.h"
 #include "AvoidCStyleCastsCheck.h"
 #include "AvoidThrowingObjCExceptionCheck.h"
 #include "DefaultArgumentsCheck.h"
 #include "ExplicitConstructorCheck.h"
 #include "ExplicitMakePairCheck.h"
+#include "FunctionNamingCheck.h"
 #include "GlobalNamesInHeadersCheck.h"
 #include "GlobalVariableDeclarationCheck.h"
 #include "IntegerTypesCheck.h"
 #include "NonConstReferences.h"
 #include "OverloadedUnaryAndCheck.h"
 #include "TodoCommentCheck.h"
 #include "UnnamedNamespaceInHeaderCheck.h"
 #include "UsingNamespaceDirectiveCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<DefaultArgumentsCheck>(
         "google-default-arguments");
     CheckFactories.registerCheck<ExplicitConstructorCheck>(
         "google-explicit-constructor");
     CheckFactories.registerCheck<readability::GlobalNamesInHeadersCheck>(
         "google-global-names-in-headers");
     CheckFactories.registerCheck<objc::AvoidThrowingObjCExceptionCheck>(
         "google-objc-avoid-throwing-exception");
+    CheckFactories.registerCheck<objc::FunctionNamingCheck>(
+        "google-objc-function-naming");
     CheckFactories.registerCheck<objc::GlobalVariableDeclarationCheck>(
         "google-objc-global-variable-declaration");
     CheckFactories.registerCheck<runtime::IntegerTypesCheck>(
         "google-runtime-int");
     CheckFactories.registerCheck<runtime::OverloadedUnaryAndCheck>(
         "google-runtime-operator");
     CheckFactories.registerCheck<runtime::NonConstReferences>(
         "google-runtime-references");

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- New :doc:`bugprone-too-small-loop-variable
   iteration range.
 
 - New :doc:`cppcoreguidelines-macro-usage
   <clang-tidy/checks/cppcoreguidelines-macro-usage>` check.
 
   Finds macro usage that is considered problematic because better language
   constructs exist for the task.
 
+- New :doc:`google-objc-function-naming
+  <clang-tidy/checks/google-objc-function-naming>` check.
+
+  Checks that function names in function declarations comply with the naming
+  conventions described in the Google Objective-C Style Guide.
+
 - New :doc:`misc-non-private-member-variables-in-classes
   <clang-tidy/checks/misc-non-private-member-variables-in-classes>` check.
 
   Finds classes that not only contain the data (non-static member variables),
   but also have logic (non-static member functions), and diagnoses all member
   variables that have any other scope other than ``private``.
 
 - New :doc:`modernize-avoid-c-arrays

clang-tools-extra/trunk/docs/clang-tidy/checks/google-objc-function-naming.rst

+.. title:: clang-tidy - google-objc-function-naming
+
+google-objc-function-naming
+===========================
+
+Finds function declarations in Objective-C files that do not follow the pattern
+described in the Google Objective-C Style Guide.
+
+The corresponding style guide rule can be found here:
+https://google.github.io/styleguide/objcguide.html#function-names
+
+All function names should be in Pascal case. Functions whose storage class is
+not static should have an appropriate prefix.
+
+The following code sample does not follow this pattern:
+
+.. code-block:: objc
+
+  static bool is_positive(int i) { return i > 0; }
+  bool IsNegative(int i) { return i < 0; }
+
+The sample above might be corrected to the following code:
+
+.. code-block:: objc
+
+  static bool IsPositive(int i) { return i > 0; }
+  bool *ABCIsNegative(int i) { return i < 0; }

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

.. toctree::
    fuchsia-virtual-inheritance
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace
    google-default-arguments
    google-explicit-constructor
    google-global-names-in-headers
    google-objc-avoid-throwing-exception
+   google-objc-function-naming
    google-objc-global-variable-declaration
    google-readability-braces-around-statements (redirects to readability-braces-around-statements) <google-readability-braces-around-statements>
    google-readability-casting
    google-readability-function-size (redirects to readability-function-size) <google-readability-function-size>
    google-readability-namespace-comments (redirects to llvm-namespace-comment) <google-readability-namespace-comments>
    google-readability-todo
    google-runtime-int
    google-runtime-operator

clang-tools-extra/trunk/test/clang-tidy/google-objc-function-naming.m

+// RUN: %check_clang_tidy %s google-objc-function-naming %t
+
+typedef _Bool bool;
+
+static bool ispositive(int a) { return a > 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'ispositive' not using function naming conventions described by Google Objective-C style guide
+// CHECK-FIXES: static bool Ispositive(int a) { return a > 0; }
+
+static bool is_positive(int a) { return a > 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'is_positive' not using function naming conventions described by Google Objective-C style guide
+// CHECK-FIXES: static bool IsPositive(int a) { return a > 0; }
+
+static bool isPositive(int a) { return a > 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'isPositive' not using function naming conventions described by Google Objective-C style guide
+// CHECK-FIXES: static bool IsPositive(int a) { return a > 0; }
+
+static bool Is_Positive(int a) { return a > 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'Is_Positive' not using function naming conventions described by Google Objective-C style guide
+// CHECK-FIXES: static bool IsPositive(int a) { return a > 0; }
+
+static bool IsPositive(int a) { return a > 0; }
+
+bool ispalindrome(const char *str);
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function name 'ispalindrome' not using function naming conventions described by Google Objective-C style guide
+
+static const char *md5(const char *str) { return 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: function name 'md5' not using function naming conventions described by Google Objective-C style guide
+// CHECK-FIXES: static const char *Md5(const char *str) { return 0; }
+
+static const char *MD5(const char *str) { return 0; }
+
+static const char *URL(void) { return "https://clang.llvm.org/"; }
+
+static const char *DEFURL(void) { return "https://clang.llvm.org/"; }
+
+static const char *DEFFooURL(void) { return "https://clang.llvm.org/"; }
+
+static const char *StringFromNSString(id str) { return ""; }
+
+void ABLog_String(const char *str);
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function name 'ABLog_String' not using function naming conventions described by Google Objective-C style guide
+
+void ABLogString(const char *str);
+
+bool IsPrime(int a);
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function name 'IsPrime' not using function naming conventions described by Google Objective-C style guide
+
+const char *ABURL(void) { return "https://clang.llvm.org/"; }
+
+const char *ABFooURL(void) { return "https://clang.llvm.org/"; }
+
+int main(int argc, const char **argv) { return 0; }