[clang-format] Improve heuristic for detecting function declarations

[arichardson]

Summary

This change also comma-separated identifiers inside parentheses as a
possible function declaration.

When using the always break after return type setting:
Before:
SomeType funcdecl(SomeType);
SomeType funcdecl(SomeType, OtherType);

After:
SomeType
funcdecl(SomeType);
SomeType
funcdecl(SomeType, OtherType);

Depends on D87007 (to apply cleanly)

[arichardson]

fix tests

[MyDeveloperDay]

Thanks for the patch, I think this looks like a comprehensive improvement, a new nits only

clang/lib/Format/TokenAnnotator.cpp

2427

how hard would it be to do the TODO?

clang/unittests/Format/FormatTest.cpp

5491

I want to approve this change, but I HATE changing unit tests (Beyonce rule), I'm struggling to see if we are changing anything here? or if you are just qualifying it a little better because the usage is different depending on where its used (as a function,as a variable)

6699

if you have to put a comment in the test then you probably should have broken the verifyFormat

6714

break it here

6724

break it here.

6746

Nit that is a mother of an assert, but when it fails.. how easy is it going to be to debug where it goes wrong exactly.

Could we break it up a little?

[arichardson]

clang/lib/Format/TokenAnnotator.cpp

2427

I am not that familiar with the clang-format codebase yet, but it appears to me that this information is not tracked at all. It should be quite easy to add another Scope (or similar) member that is an enum for global/function/namespace/class/other and then update that in all cases that also change the level member.

clang/unittests/Format/FormatTest.cpp

5491

Before a non-empty () with a single indentifier inside was always treated as a variable, with this change it's (in this case incorrectly) parsed as function.

6746

I agree this is too long. I was just trying to avoid repeating the namespace {}/void fn()` prefix. Will try to break it up for next version of this patch.

clang/lib/Format/TokenAnnotator.cpp

if (Current.is(tok::kw_operator)) {
     }
   }
 
   // Check whether parameter list can belong to a function declaration.
   if (!Next || !Next->is(tok::l_paren) || !Next->MatchingParen)
     return false;
   // If the lines ends with "{", this is likely an function definition.
   if (Line.Last->is(tok::l_brace))
     return true;
   if (Next->Next == Next->MatchingParen)
     return true; // Empty parentheses.
   // If there is an &/&& after the r_paren, this is likely a function.
   if (Next->MatchingParen->Next &&
       Next->MatchingParen->Next->is(TT_PointerOrReference))
     return true;
+  // Treat  cases where the parameter list only contains comma-separated
+  // identifiers as function declarations. For example:
+  // `SomeType funcdecl(OtherType)` or `SomeType funcdecl(Type1, Type2)`
+  bool CouldBeTypeList = true;
   for (const FormatToken *Tok = Next->Next; Tok && Tok != Next->MatchingParen;
        Tok = Tok->Next) {
     if (Tok->is(TT_TypeDeclarationParen))
       return true;
     if (Tok->isOneOf(tok::l_paren, TT_TemplateOpener) && Tok->MatchingParen) {
       Tok = Tok->MatchingParen;
+      CouldBeTypeList = false;
       continue;
     }
     if (Tok->is(tok::kw_const) || Tok->isSimpleTypeSpecifier() ||
         Tok->isOneOf(TT_PointerOrReference, TT_StartOfName, tok::ellipsis))
       return true;
     if (Tok->isOneOf(tok::l_brace, tok::string_literal, TT_ObjCMethodExpr) ||
         Tok->Tok.isLiteral())
       return false;
+    if (!Tok->isOneOf(tok::identifier, tok::comma))
+      CouldBeTypeList = false;
   }
-  return false;
+  return CouldBeTypeList;
 }
 
 bool TokenAnnotator::mustBreakForReturnType(const AnnotatedLine &Line) const {
   assert(Line.MightBeFunctionDecl);
 
   if ((Style.AlwaysBreakAfterReturnType == FormatStyle::RTBS_TopLevel ||
        Style.AlwaysBreakAfterReturnType ==
            FormatStyle::RTBS_TopLevelDefinitions) &&
       Line.Level > 0)
     return false;
 
   switch (Style.AlwaysBreakAfterReturnType) {

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, BreaksFunctionDeclarationsWithTrailingTokens) {
   verifyFormat("bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "    __attribute__((unused));");
   verifyGoogleFormat(
       "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
       "    GUARDED_BY(aaaaaaaaaaaa);");
   verifyGoogleFormat(
       "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
       "    GUARDED_BY(aaaaaaaaaaaa);");
   verifyGoogleFormat(
       "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa GUARDED_BY(aaaaaaaaaaaa) =\n"
       "    aaaaaaaa::aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa;");
   verifyGoogleFormat(
       "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa GUARDED_BY(aaaaaaaaaaaa) =\n"
       "    aaaaaaaaaaaaaaaaaaaaaaaaa;");
 }
 
 TEST_F(FormatTest, FunctionAnnotations) {
verifyFormat("class A {\n"
                "};\n"
                "int f() { return 1; }\n"
                "int g();\n",
                Style);
 
   // All declarations and definitions should have the return type moved to its
   // own line.
   Style.AlwaysBreakAfterReturnType = FormatStyle::RTBS_All;
+  Style.NamespaceIndentation = FormatStyle::NI_All;
   Style.TypenameMacros = {"LIST"};
   verifyFormat("SomeType\n"
                "funcdecl(LIST(uint64_t));",
                Style);
+  verifyFormat("SomeType\n"
+               "funcdecl();\n"
+               "SomeType\n"
+               "funcdecl(SomeType paramname);\n"
+               "SomeType\n"
+               "funcdecl(_Atomic(uint64_t));\n"
+               "SomeType\n"
+               "funcdecl(SomeType param1, OtherType param2);\n"
+               // Also handle parameter lists declaration without names (but
+               // only at the top level, not inside functions
+               "SomeType\n"
+               "funcdecl(SomeType);\n"
+               "SomeType\n"
+               "funcdecl(SomeType, OtherType);\n"
+               // Check that any kind of expression/operator results in parsing
+               // it as a variable declaration and not a function
+               "SomeType vardecl(var + otherVar);\n"
+               "SomeType vardecl(func());\n"
+               "SomeType vardecl(someFunc(arg));\n"
+               "SomeType vardecl(var, var - otherVar);\n"
+               "SomeType x = var * funcdecl();\n"
+               "SomeType x = var * funcdecl(otherVar);\n"
+               "SomeType x = var * funcdecl(var, otherVar);\n"
+               "void\n"
+               "function_scope() {\n"
+               "  SomeType x = var * funcdecl();\n"
+               "  SomeType x = var * funcdecl(otherVar);\n"
+               "  SomeType x = var * funcdecl(var, otherVar);\n"
+               // While clang will parse these as function declarations, at
+               // least for now clang-format assumes they are variables.
+               "  SomeType *funcdecl();\n"
+               "  SomeType *funcdecl(SomeType);\n"
+               "  SomeType *funcdecl(SomeType, OtherType);\n"
+               "}\n"
+               "namespace namspace_scope {\n"
+               "  SomeType\n"
+               "  funcdecl();\n"
+               "  SomeType\n"
+               "  funcdecl(SomeType paramname);\n"
+               "  SomeType\n"
+               "  funcdecl(_Atomic(uint64_t));\n"
+               "  SomeType\n"
+               "  funcdecl(SomeType param1, OtherType param2);\n"
+               // Inside a namespace we should also parse these as a function
+               // declaration and not as a variable.
+               "  SomeType\n"
+               "  funcdecl(SomeType);\n"
+               "  SomeType\n"
+               "  funcdecl(SomeType, OtherType);\n"
+               "  SomeType vardecl(var + otherVar);\n"
+               "  SomeType vardecl(func());\n"
+               "  SomeType vardecl(someFunc(arg));\n"
+               "  SomeType vardecl(var, var - otherVar);\n"
+               "  SomeType x = var * funcdecl();\n"
+               "  SomeType x = var * funcdecl(otherVar);\n"
+               "  SomeType x = var * funcdecl(var, otherVar);\n"
+               "} // namespace namspace_scope\n",
+               Style);
   verifyFormat("class E {\n"
                "  int\n"
                "  f() {\n"
                "    return 1;\n"
                "  }\n"
                "  int\n"
                "  g();\n"
                "};\n"