[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

5507

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)

6715

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

6730

break it here

6740

break it here.

6762

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

5507

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.

6762

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

static bool isFunctionDeclarationName(const FormatToken &Current,
   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;
+  // TODO: we should not make this decision based on the indentation level, but
+  // rather based on the type of scope (for example namespaces/classes/etc.
+  // should probably parse `SomeType decl(Type1, Type2)` as a function but
+  // inside a function this should always be treated as a variable.
+  return CouldBeTypeList && Line.Level == 0;
 }
 
 bool TokenAnnotator::mustBreakForReturnType(const AnnotatedLine &Line) const {
   assert(Line.MightBeFunctionDecl);
 
   if ((Style.AlwaysBreakAfterReturnType == FormatStyle::RTBS_TopLevel ||
        Style.AlwaysBreakAfterReturnType ==
            FormatStyle::RTBS_TopLevelDefinitions) &&

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, BreaksFunctionDeclarationsWithTrailingTokens) {
   verifyFormat("SomeFunction([](int i) LOCKS_EXCLUDED(a) {});");
 
   verifyFormat(
       "void aaaaaaaaaaaaaaaaaa()\n"
       "    __attribute__((aaaaaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaaa,\n"
       "                   aaaaaaaaaaaaaaaaaaaaaaaaa));");
   verifyFormat("bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "    __attribute__((unused));");
-  verifyGoogleFormat(
+  FormatStyle Google = getGoogleStyle();
+  verifyFormat(
       "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
-      "    GUARDED_BY(aaaaaaaaaaaa);");
-  verifyGoogleFormat(
+      "GUARDED_BY(aaaaaaaaaaaa);\n" // parsed as function decl
+      "void f() {\n"
+      "  bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+      "      GUARDED_BY(aaaaaaaaaaaa);\n" // parsed as variable decl
+      "}\n"
+      "class Cls {\n"
+      "  bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+      "      GUARDED_BY(aaaaaaaaaaaa);\n" // parsed as variable decl
+      "};",
+      Google);
+  verifyFormat(
       "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
-      "    GUARDED_BY(aaaaaaaaaaaa);");
+      "GUARDED_BY(aaaaaaaaaaaa);", // parsed as function decl
+      Google);
+  Google.AttributeMacros = {"GUARDED_BY"};
+  verifyFormat(
+      "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa GUARDED_BY(\n"
+      "    aaaaaaaaaaaa);", // parsed as variable decl
+      Google);
   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"
+               // TODO: Should we also parse these as a function declaration
+               //  and not as a variable inside namespaces?
+               "  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"
+               "  SomeType decl(SomeType);\n"
+               "  SomeType decl(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"
verifyFormat("LoooooooooooooooooooooooooooooooooooooooongType const\n"
                "    LoooooooooooooooooooooooooooooooooooooooongVariable;");
   verifyFormat("LoooooooooooooooooooooooooooooooooooooooongType\n"
                "    *LoooooooooooooooooooooooooooooooooooooooongVariable;");
 
   // Different ways of ()-initializiation.
   verifyFormat("LoooooooooooooooooooooooooooooooooooooooongType\n"
                "    LoooooooooooooooooooooooooooooooooooooooongVariable(1);");
   verifyFormat("LoooooooooooooooooooooooooooooooooooooooongType\n"
-               "    LoooooooooooooooooooooooooooooooooooooooongVariable(a);");
-  verifyFormat("LoooooooooooooooooooooooooooooooooooooooongType\n"
                "    LoooooooooooooooooooooooooooooooooooooooongVariable({});");
   verifyFormat("LoooooooooooooooooooooooooooooooooooooooongType\n"
                "    LoooooooooooooooooooooooooooooooooooooongVariable([A a]);");
+  // A single identifier inside parentheses is parsed as a function declaration
+  // in the global scope, but as a variable inside functions.
+  verifyFormat("LoooooooooooooooooooooooooooooooooooooooongType\n"
+               "LoooooooooooooooooooooooooooooooooooooooongVariable(a);\n"
+               "void f() {\n"
+               "  LoooooooooooooooooooooooooooooooooooooooongType\n"
+               "      LoooooooooooooooooooooooooooooooooooooooongVariable(a);\n"
+               "  LoooooooooooooooooooooooooooooooooooooooongType\n"
+               "      LooooooooooooooooooooooooooooooooooongVariable2(a + 1);\n"
+               "}");
 
   // Lambdas should not confuse the variable declaration heuristic.
   verifyFormat("LooooooooooooooooongType\n"
                "    variable(nullptr, [](A *a) {});",
                getLLVMStyleWithColumns(40));
 }
 
 TEST_F(FormatTest, BreaksLongDeclarations) {