[clang-tidy] Make google-objc-function-naming ignore implicit functions 🙈

[stephanemoore]

Summary

Implicit functions are outside the control of source authors and should
be exempt from style restrictions.

Tested via running clang tools tests.

[aaron.ballman]

clang-tidy/google/FunctionNamingCheck.cpp

96–97

This comment is drifting more and more out of sync with the code.

test/clang-tidy/google-objc-function-naming.m

5–8

Excuse my ignorance, but I don't see what about this function definition is testing an implicitly-defined function. Doesn't the #import above bring in the declaration for printf(), while TestImplicitlyDefinedFunction() itself is explicitly defined?

[stephanemoore]

Updated the comment describing the matching behavior.

[stephanemoore]

clang-tidy/google/FunctionNamingCheck.cpp

96–97

Good catch; updated the comment.

test/clang-tidy/google-objc-function-naming.m

5–8

tl;dr: Yes, we have an explicit function declaration but clang generates an implicit function declaration as well.

I found this surprising as well but the AST generated from this includes an implicit function declaration. Even though we have an explicit function declaration for printf from <stdio.h>, clang still generates an implicit declaration of printf 🤨 Similar behavior occurs with other functions, e.g., memset and various C11 atomic functions.

I have been spelunking in search of the reason but I do not have a full understanding yet. I believe I have insight into the underlying mechanism though: the behavior seems linked to builtin functions (e.g., printf is declared as a builtin function here). I have attached a small sample program and an associated AST dump below to demonstrate the behavior.

❧

$ cat /tmp/test.c
extern int printf(const char *format, ...);

int main(int argc, const char **argv) {
  printf("%d", argc);
  return 0;
}

$ clang -cc1 -ast-dump /tmp/test.c 
TranslationUnitDecl 0x561b9f7fe570 <<invalid sloc>> <invalid sloc>
|-TypedefDecl 0x561b9f7feac0 <<invalid sloc>> <invalid sloc> implicit __int128_t '__int128'
| `-BuiltinType 0x561b9f7fe7e0 '__int128'
|-TypedefDecl 0x561b9f7feb28 <<invalid sloc>> <invalid sloc> implicit __uint128_t 'unsigned __int128'
| `-BuiltinType 0x561b9f7fe800 'unsigned __int128'
|-TypedefDecl 0x561b9f7fedf8 <<invalid sloc>> <invalid sloc> implicit __NSConstantString 'struct __NSConstantString_tag'
| `-RecordType 0x561b9f7fec00 'struct __NSConstantString_tag'
|   `-Record 0x561b9f7feb78 '__NSConstantString_tag'
|-TypedefDecl 0x561b9f7fee90 <<invalid sloc>> <invalid sloc> implicit __builtin_ms_va_list 'char *'
| `-PointerType 0x561b9f7fee50 'char *'
|   `-BuiltinType 0x561b9f7fe600 'char'
|-TypedefDecl 0x561b9f7ff158 <<invalid sloc>> <invalid sloc> implicit __builtin_va_list 'struct __va_list_tag [1]'
| `-ConstantArrayType 0x561b9f7ff100 'struct __va_list_tag [1]' 1 
|   `-RecordType 0x561b9f7fef70 'struct __va_list_tag'
|     `-Record 0x561b9f7feee0 '__va_list_tag'
|-FunctionDecl 0x561b9f851860 </tmp/test.c:1:12> col:12 implicit used printf 'int (const char *, ...)' extern
| |-ParmVarDecl 0x561b9f8518f8 <<invalid sloc>> <invalid sloc> 'const char *'
| `-FormatAttr 0x561b9f851960 <col:12> Implicit printf 1 2
|-FunctionDecl 0x561b9f8519b8 prev 0x561b9f851860 <col:1, col:42> col:12 used printf 'int (const char *, ...)' extern
| |-ParmVarDecl 0x561b9f7ff1c0 <col:19, col:31> col:31 format 'const char *'
| `-FormatAttr 0x561b9f851a90 <col:12> Inherited printf 1 2
`-FunctionDecl 0x561b9f851c48 <line:3:1, line:6:1> line:3:5 main 'int (int, const char **)'
  |-ParmVarDecl 0x561b9f851ac8 <col:10, col:14> col:14 used argc 'int'
  |-ParmVarDecl 0x561b9f851b70 <col:20, col:33> col:33 argv 'const char **'
  `-CompoundStmt 0x561b9f851f08 <col:39, line:6:1>
    |-CallExpr 0x561b9f851e50 <line:4:3, col:20> 'int'
    | |-ImplicitCastExpr 0x561b9f851e38 <col:3> 'int (*)(const char *, ...)' <FunctionToPointerDecay>
    | | `-DeclRefExpr 0x561b9f851d58 <col:3> 'int (const char *, ...)' Function 0x561b9f8519b8 'printf' 'int (const char *, ...)'
    | |-ImplicitCastExpr 0x561b9f851ea0 <col:10> 'const char *' <BitCast>
    | | `-ImplicitCastExpr 0x561b9f851e88 <col:10> 'char *' <ArrayToPointerDecay>
    | |   `-StringLiteral 0x561b9f851db8 <col:10> 'char [3]' lvalue "%d"
    | `-ImplicitCastExpr 0x561b9f851eb8 <col:16> 'int' <LValueToRValue>
    |   `-DeclRefExpr 0x561b9f851de8 <col:16> 'int' lvalue ParmVar 0x561b9f851ac8 'argc' 'int'
    `-ReturnStmt 0x561b9f851ef0 <line:5:3, col:10>
      `-IntegerLiteral 0x561b9f851ed0 <col:10> 'int' 0

Note the presence of implicit and explicit declarations of printf in the AST dump 🤔

[aaron.ballman]

LGTM with a commenting request.

test/clang-tidy/google-objc-function-naming.m

5–8

Well that certainly is neat; I didn't know that! Can you add some comments above the function call to explain that to the next person reading the test?

[stephanemoore]

Add comment explaining that builtin function call generates implicit function declaration and update calling function name for improved clarity.

[stephanemoore]

Thanks for the review!

[stephanemoore]

test/clang-tidy/google-objc-function-naming.m

3

It turns out importing <stdio.h> is problematic and breaks the build (though everything built successfully for me locally 🤔). I believe that the import is not strictly necessary and I can embed a function declaration for printf to reproduce the implicit function declaration with the caveat that the check will trigger on a printf declaration that is not from a system header. I suppose it might be reasonable to suppress the check output on printf?

I have reverted this change for now and will follow up with appropriate fixes.

[aaron.ballman]

test/clang-tidy/google-objc-function-naming.m

3

It turns out importing <stdio.h> is problematic and breaks the build (though everything built successfully for me locally 🤔).

Sorry about not catching that during review. I'm used to mentioning that you can't do #include but I didn't recall if the same was true for #import.

I believe that the import is not strictly necessary and I can embed a function declaration for printf to reproduce the implicit function declaration with the caveat that the check will trigger on a printf declaration that is not from a system header. I suppose it might be reasonable to suppress the check output on printf?

You can use line markers to specify that the declaration is part of a system header with something like this:

# 1 "system_header.h" 3
int printf(const char *, ...);
# 1 "google-objc-function-naming.m" 1

(Note, you may need to pull a more decorated signature for printf() to be accurate.)

[stephanemoore]

test/clang-tidy/google-objc-function-naming.m

3

Thanks for the tip! I'll try that out and send out a followup!

clang-tidy/google/FunctionNamingCheck.cpp

 
 } // 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.
+  // Enforce Objective-C function naming conventions on all functions except:
+  // • Functions defined in system headers.
+  // • C++ member functions.
+  // • Namespaced functions.
+  // • Implicitly defined functions.
+  // • The main function.
   Finder->addMatcher(
       functionDecl(
           unless(anyOf(isExpansionInSystemHeader(), cxxMethodDecl(),
-                       hasAncestor(namespaceDecl()), isMain(),
+                       hasAncestor(namespaceDecl()), isMain(), isImplicit(),
                        matchesName(validFunctionNameRegex(true)),
                        allOf(isStaticStorageClass(),
                              matchesName(validFunctionNameRegex(false))))))
           .bind("function"),
       this);
 }
 
 void FunctionNamingCheck::check(const MatchFinder::MatchResult &Result) {

test/clang-tidy/google-objc-function-naming.m

 // RUN: %check_clang_tidy %s google-objc-function-naming %t
 
+#import <stdio.h>
+
+static void TestImplicitFunctionDeclaration(int a) {
+  // Call a builtin function so that the compiler generates an implicit
+  // function declaration.
+  printf("%d", a);
+}
+
 typedef _Bool bool;
 
 static bool ispositive(int a) { return a > 0; }
 // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: static function named 'ispositive'
 // must be in Pascal case as required 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; }