[clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

[MyDeveloperDay]

Summary

Addresses PR40696 - https://bugs.llvm.org/show_bug.cgi?id=40696

The BreakAfterReturnType didn't work if it had a single arguments which was a template with an integer template parameter

int  foo(A<8> a) { return a; }

When run with the Mozilla style. would not break after the int

int TestFn(A<8> a)
{
  return a;
}

This revision resolves this issue by allowing numeric constants to be considered function parameters if if seen inside <>

[klimek]

Why did the numeric constant break this? Which path would the function run through?

[MyDeveloperDay]

In D59309#1428799, @klimek wrote:

Why did the numeric constant break this? Which path would the function run through?

as its looking though the parameter list is doesn't recognize a numeric constant as being a valid thing it would see in an argument list and so falls through the bottom to return false, it will only be where there is a numeric_constant as the first argument, if it has a second argument and that argument is just a simple type it would still fail, but switch them around and it won't

As I look more at this, even this would (and still would) fail to break

int TestFn(A=1)
{
  return a;
}

originally I didn't have the counting of the <> but one of the unit tests failed where we had

verifyFormat("LoooooooooooooooooooooooooooooooooooooooongType\n"
             "    LoooooooooooooooooooooooooooooooooooooooongVariable(1);");

I guess they wanted to do something like this when the type and name is very long

int
variable(1);

but I think this gets thought of as being a potential FunctionDecl

so I held off just adding tok::numeric_constant to the isOneOf(), and went with the use case

I think perhaps I could solve the default argument by adding a

startsSequence(tok::equal, tok::numeric_constant)

[klimek]

In D59309#1428807, @MyDeveloperDay wrote:

In D59309#1428799, @klimek wrote:

Why did the numeric constant break this? Which path would the function run through?

as its looking though the parameter list is doesn't recognize a numeric constant as being a valid thing it would see in an argument list and so falls through the bottom to return false, it will only be where there is a numeric_constant as the first argument, if it has a second argument and that argument is just a simple type it would still fail, but switch them around and it won't

As I look more at this, even this would (and still would) fail to break

int TestFn(A=1)
{
  return a;
}

originally I didn't have the counting of the <> but one of the unit tests failed where we had

verifyFormat("LoooooooooooooooooooooooooooooooooooooooongType\n"
             "    LoooooooooooooooooooooooooooooooooooooooongVariable(1);");

I guess they wanted to do something like this when the type and name is very long

int
variable(1);

but I think this gets thought of as being a potential FunctionDecl

so I held off just adding tok::numeric_constant to the isOneOf(), and went with the use case

I think perhaps I could solve the default argument by adding a

startsSequence(tok::equal, tok::numeric_constant)

How does this even work for

void Test(A a)
{
}

This code seems to deeply assume that the { is the last thing in the line for a definition. One question is whether we can just mark this as a function definition when we see the { in the UnwrappedLineParser...

[MyDeveloperDay]

That works because the argument list is just tok::identifier and TT_StartOfName

so if we see TT_StartOfName we are going to return true

tok::identifier will simply be skipped round the loop

int
Test(A a)
{}

AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=int L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x11be8c48b20 Text=int
 M=1 C=1 T=FunctionDeclarationName S=1 B=0 BK=0 P=220 Name=identifier L=88 PPK=2 FakeLParens= FakeRParens=0 II=0x11be8c42d00 Text=Test
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=l_paren L=89 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=(
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=140 Name=identifier L=90 PPK=2 FakeLParens= FakeRParens=0 II=0x11be8c42c70 Text=A
 M=0 C=1 T=StartOfName S=1 B=0 BK=0 P=240 Name=identifier L=92 PPK=2 FakeLParens= FakeRParens=0 II=0x11be8c42ca0 Text=a
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=43 Name=r_paren L=93 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=)

Where you are correct in its handling is this,

int Test(A) {}

because it doesn't see the TT_StartOfName it doesn't return true and so falls out the bottom of the function with false
and hence doesn't break the return type

AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=int L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x245bb8e7140 Text=int
 M=0 C=1 T=StartOfName S=1 B=0 BK=0 P=220 Name=identifier L=8 PPK=2 FakeLParens= FakeRParens=0 II=0x245bb8ebdc0 Text=Test
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=l_paren L=9 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=(
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=140 Name=identifier L=10 PPK=2 FakeLParens= FakeRParens=0 II=0x245bb8ebd30 Text=A
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=43 Name=r_paren L=11 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=)

[klimek]

In D59309#1428969, @MyDeveloperDay wrote:

That works because the argument list is just tok::identifier and TT_StartOfName

Should we fix isStartOfName then?

In A<bool> or A<8> A should be TT_StartOfName, too, right?

[MyDeveloperDay]

of course, this isn't legal

int TestFn(A=1)
{
  return a;
}

and, would see the TT_StartOfName for (a) and would break

int
TestFn(A a = 1)
{
  return a;
}

[MyDeveloperDay]

In D59309#1428970, @klimek wrote:

In D59309#1428969, @MyDeveloperDay wrote:

That works because the argument list is just tok::identifier and TT_StartOfName

Should we fix isStartOfName then?

In A<bool> or A<8> A should be TT_StartOfName, too, right?

Maybe that is a better way, let me look..

I've realized that defining a function Test which returns a class, and has an unnamed class parameter in its definition

B Test(A);

is indistinguishable from a variable declaration Test of type B which passes a variable A into its constructor, without knowing that A is a class and not a variable, there isn't enough information to say this is a function or not

B Test(A);

Which is why we should always name our parameters in function declarations ;-)

[MyDeveloperDay]

In D59309#1428970, @klimek wrote:

In D59309#1428969, @MyDeveloperDay wrote:

That works because the argument list is just tok::identifier and TT_StartOfName

Should we fix isStartOfName then?

In A<bool> or A<8> A should be TT_StartOfName, too, right?

I'm not sure that would work, the <8> is part of the type not the TT_StartOfName, or did I miss something

M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=int L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x263a5ddd090 Text='int'
M=1 C=1 T=FunctionDeclarationName S=1 B=0 BK=0 P=220 Name=identifier L=88 PPK=2 FakeLParens= FakeRParens=0 II=0x263a5dda5a0 Text='Test'
M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=l_paren L=89 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
M=0 C=1 T=Unknown S=0 B=0 BK=0 P=140 Name=identifier L=90 PPK=2 FakeLParens= FakeRParens=0 II=0x263a5dda510 Text='A'
M=0 C=1 T=StartOfName S=1 B=0 BK=0 P=240 Name=identifier L=92 PPK=2 FakeLParens= FakeRParens=0 II=0x263a5dda540 Text='a'
M=0 C=0 T=Unknown S=0 B=0 BK=0 P=43 Name=r_paren L=93 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')

M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=int L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x263a5ddd090 Text='int'
M=1 C=1 T=FunctionDeclarationName S=1 B=0 BK=0 P=220 Name=identifier L=88 PPK=2 FakeLParens= FakeRParens=0 II=0x263a5dda4e0 Text='TestFn'
M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=l_paren L=89 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
M=0 C=1 T=Unknown S=0 B=0 BK=0 P=140 Name=identifier L=90 PPK=2 FakeLParens= FakeRParens=0 II=0x263a5dda510 Text='A'
M=0 C=0 T=TemplateOpener S=0 B=0 BK=0 P=50 Name=less L=91 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<'
M=0 C=1 T=Unknown S=0 B=0 BK=0 P=380 Name=bool L=95 PPK=2 FakeLParens= FakeRParens=0 II=0x263a5ddd670 Text='bool'
M=0 C=0 T=TemplateCloser S=0 B=0 BK=0 P=290 Name=greater L=96 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='>'
M=0 C=1 T=StartOfName S=1 B=0 BK=0 P=240 Name=identifier L=98 PPK=2 FakeLParens= FakeRParens=0 II=0x263a5dda540 Text='a'
M=0 C=0 T=Unknown S=0 B=0 BK=0 P=43 Name=r_paren L=99 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'

[klimek]

Yea, the
a b(c)
problem is the olden C++ problem clang-format will never be able to solve, thus all these crazy heuristics.

So, inching closer to understanding this - I totally missed the isLiteral() that short-circuits out before. I'm more happy with your current approach, except I'd turn it around:
I'd only short-circuit out of the loop returning false in line 2064 if isLiteral() && (TemplateOpenerLevel > 0).
Alternatively, we could try to skip things within template <> - don't we have the closing > for an opening < in MatchingParen?

[MyDeveloperDay]

Addressing code review comment

@klimek I think is what you meant, a much smaller and neater version of what I had before.

[MyDeveloperDay]

Sorry for the spam:

• write this a little better

[klimek]

Minus me understanding the TT_TemplateString change, the rest looks nice now, thanks!

clang/lib/Format/TokenAnnotator.cpp

3181–3182

I don't understand this yet, which test broke? The TT_TemplateString looks like it's a JS thing?

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

3181–3182

I'm not sure what you mean here?

[MyDeveloperDay]

friendly ping

[klimek]

lg

clang/lib/Format/TokenAnnotator.cpp

3181–3182

I don't see the change any more - perhaps the diff was broken? Sorry :(

clang/lib/Format/TokenAnnotator.cpp

if (Next->MatchingParen->Next &&
       Next->MatchingParen->Next->is(TT_PointerOrReference))
     return true;
   for (const FormatToken *Tok = Next->Next; Tok && Tok != Next->MatchingParen;
        Tok = Tok->Next) {
     if (Tok->is(tok::l_paren) && Tok->MatchingParen) {
       Tok = Tok->MatchingParen;
       continue;
     }
+    if (Tok->is(TT_TemplateOpener) && Tok->MatchingParen) {
+      Tok = Tok->MatchingParen;
+      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;
   }
   return false;
if (Left.is(Keywords.kw_declare) &&
       // See grammar for 'declare' statements at:
       // https://github.com/Microsoft/TypeScript/blob/master/doc/spec.md#A.10
       return false;
     if (Left.isOneOf(Keywords.kw_module, tok::kw_namespace) &&
         Right.isOneOf(tok::identifier, tok::string_literal))
       return false; // must not break in "module foo { ...}"
     if (Right.is(TT_TemplateString) && Right.closesScope())
       return false;
     // Don't split tagged template literal so there is a break between the tag
     // identifier and template string.
     if (Left.is(tok::identifier) && Right.is(TT_TemplateString)) {
       return false;
     }
     if (Left.is(TT_TemplateString) && Left.opensScope())
       return true;
   }
 
   if (Left.is(tok::at))

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ReturnTypeBreakingStyle) {
   verifyFormat("template <class T>\n"
                "T *\n"     // Problem here: no line break
                "f(T &c)\n" // Break here.
                "{\n"
                "  return NULL;\n"
                "}\n"
                "template <class T> T *f(T &c);\n", // No break here.
                Style);
+  verifyFormat("int\n"
+               "foo(A<bool> a)\n"
+               "{\n"
+               "  return a;\n"
+               "}\n",
+               Style);
+  verifyFormat("int\n"
+               "foo(A<8> a)\n"
+               "{\n"
+               "  return a;\n"
+               "}\n",
+               Style);
+  verifyFormat("int\n"
+               "foo(A<B<bool>, 8> a)\n"
+               "{\n"
+               "  return a;\n"
+               "}\n",
+               Style);
+  verifyFormat("int\n"
+               "foo(A<B<8>, bool> a)\n"
+               "{\n"
+               "  return a;\n"
+               "}\n",
+               Style);
+  verifyFormat("int\n"
+               "foo(A<B<bool>, bool> a)\n"
+               "{\n"
+               "  return a;\n"
+               "}\n",
+               Style);
+  verifyFormat("int\n"
+               "foo(A<B<8>, 8> a)\n"
+               "{\n"
+               "  return a;\n"
+               "}\n",
+               Style);
 }
 
 TEST_F(FormatTest, AlwaysBreakBeforeMultilineStrings) {
   FormatStyle NoBreak = getLLVMStyle();
   NoBreak.AlwaysBreakBeforeMultilineStrings = false;
   FormatStyle Break = getLLVMStyle();
   Break.AlwaysBreakBeforeMultilineStrings = true;
   verifyFormat("aaaa = \"bbbb\"\n"