[clang-format] broken after lambda with return type template with boolean literal

[MyDeveloperDay]

Summary

A Lamdba with a return type template with a boolean literal (true,false) behaves differently to an integer literal

https://bugs.llvm.org/show_bug.cgi?id=40910

[jkorous]

Thanks for fixing this!

The patch LGTM.

I am just wondering what else are we missing here. I guess arithmetic and logical operators are also allowed:

template<int> struct foo {};

int main() { 
    auto lambda1 = []() -> foo<!1> { return foo<!1>{}; }; 
    auto lambda2 = []() -> foo<5+3> { return foo<5+3>{}; }; 
}

[jkorous]

I added couple other cases to your fix here: https://reviews.llvm.org/D58934

[jkorous]

BTW I suggest you also add the original test case since the test cases you added fail the expectation at FormatTest.cpp:74 and the message is a bit unclear whether the testcase or the actual implementation is at fault.

EXPECT_EQ(Expected.str(), format(Expected, Style))
    << "Expected code is not stable";

The original reported case IMO gives a better idea what is broken since it duplicates the end-of-namespace comment.
https://bugs.llvm.org/show_bug.cgi?id=40910

[MyDeveloperDay]

In D58922#1417605, @jkorous wrote:

BTW I suggest you also add the original test case since the test cases you added fail the expectation at FormatTest.cpp:74 and the message is a bit unclear whether the testcase or the actual implementation is at fault.

EXPECT_EQ(Expected.str(), format(Expected, Style))
    << "Expected code is not stable";

The original reported case IMO gives a better idea what is broken since it duplicates the end-of-namespace comment.
https://bugs.llvm.org/show_bug.cgi?id=40910

do you mean this case? as this seems to work for me?

verifyFormat("namespace bar {\n"
               "auto foo{[]() -> foo<false> { ; }};\n"
               "} // namespace bar");

$ ./tools/clang/unittests/Format/FormatTests.exe --gtest_filter=FormatTest.FormatsLambdas*
Note: Google Test filter = FormatTest.FormatsLambdas*
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from FormatTest
[ RUN      ] FormatTest.FormatsLambdas
[       OK ] FormatTest.FormatsLambdas (1353 ms)
[----------] 1 test from FormatTest (1354 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1362 ms total)
[  PASSED  ] 1 test.

BTW if you want me to land this before you land D58934: [clang-format] Fix lambdas returning template specialization that contains operator in parameter I need someone to mark this as accepted in Phabricator.

[jkorous]

In D58922#1418029, @MyDeveloperDay wrote:

do you mean this case? as this seems to work for me?

verifyFormat("namespace bar {\n"
               "auto foo{[]() -> foo<false> { ; }};\n"
               "} // namespace bar");

I meant that if you take the test and run it on unpatched master (simulating hypothetical test failure in the future) the message it produces starts by stating that the expected string (part of the testcase) is "unstable" which seems a bit unclear/confusing to me.

/Users/jankorous/src/llvm-monorepo/llvm-project/clang/unittests/Format/FormatTest.cpp:74: Failure
      Expected: Expected.str()
      Which is: "namespace bar {\nauto foo{[]() -> foo<false> { ; }};\n} // namespace bar"
To be equal to: format(Expected, Style)
      Which is: "namespace bar {\nauto foo{[]() -> foo<false>{;\n}\n}\n;\n} // namespace bar"
With diff:
@@ -1,3 +1,6 @@
 namespace bar {
-auto foo{[]() -> foo<false> { ; }};
+auto foo{[]() -> foo<false>{;
+}
+}
+;
 } // namespace bar

Whereas the full original reproducer (with the "// broken:" comment included) also starts with the same message but the diff implies that the implementation is wrong by duplicating the "// namespace bar" comment.

/Users/jankorous/src/llvm-monorepo/llvm-project/clang/unittests/Format/FormatTest.cpp:74: Failure
      Expected: Expected.str()
      Which is: "namespace bar {\n// broken:\nauto foo{[]() -> foo<5 + 2> { return {}; }};\n} // namespace bar"
To be equal to: format(Expected, Style)
      Which is: "namespace bar {\n// broken:\nauto foo{[]() -> foo<5 + 2>{return {};\n} // namespace bar\n}\n;\n} // namespace bar"
With diff:
@@ -1,4 +1,7 @@
 namespace bar {
 // broken:
-auto foo{[]() -> foo<5 + 2> { return {}; }};
+auto foo{[]() -> foo<5 + 2>{return {};
+} // namespace bar
+}
+;
 } // namespace bar

Anyway. LGTM.

lib/Format/UnwrappedLineParser.cpp

while (FormatTok->isNot(tok::l_brace)) {
     case tok::ampamp:
     case tok::caret:
     case tok::equalequal:
     case tok::exclaimequal:
     case tok::greaterequal:
     case tok::lessequal:
     case tok::question:
     case tok::colon:
+    case tok::kw_true:
+    case tok::kw_false:
       nextToken();
       break;
     case tok::arrow:
       // This might or might not actually be a lambda arrow (this could be an
       // ObjC method invocation followed by a dereferencing arrow). We might
       // reset this back to TT_Unknown in TokenAnnotator.
       FormatTok->Type = TT_LambdaArrow;
       nextToken();

unittests/Format/FormatTest.cpp

TEST_F(FormatTest, FormatsLambdas) {
   verifyFormat("namespace bar {\n"
               "// broken:\n"
               "auto foo{[]() -> foo<5 < 2> { return {}; }};\n"
               "} // namespace bar");
   verifyFormat("namespace bar {\n"
               "// broken:\n"
               "auto foo{[]() -> foo<2 ? 1 : 0> { return {}; }};\n"
               "} // namespace bar");
+  verifyFormat("[]() -> a<1> {};");
+  verifyFormat("[]() -> a<1> { ; };");
+  verifyFormat("[]() -> a<1> { ; }();");
+  verifyFormat("[a, a]() -> a<true> {};");
+  verifyFormat("[]() -> a<true> {};");
+  verifyFormat("[]() -> a<true> { ; };");
+  verifyFormat("[]() -> a<true> { ; }();");
+  verifyFormat("[a, a]() -> a<false> {};");
+  verifyFormat("[]() -> a<false> {};");
+  verifyFormat("[]() -> a<false> { ; };");
+  verifyFormat("[]() -> a<false> { ; }();");
+  verifyFormat("auto foo{[]() -> foo<false> { ; }};");
+  verifyFormat("namespace bar {\n"
+               "auto foo{[]() -> foo<false> { ; }};\n"
+               "} // namespace bar");
   verifyFormat("auto aaaaaaaa = [](int i, // break for some reason\n"
                "                   int j) -> int {\n"
                "  return ffffffffffffffffffffffffffffffffffffffffffff(i * j);\n"
                "};");
   verifyFormat(
       "aaaaaaaaaaaaaaaaaaaaaa(\n"
       "    [](aaaaaaaaaaaaaaaaaaaaaaaaaaa &aaa) -> aaaaaaaaaaaaaaaa {\n"
       "      return aaaaaaaaaaaaaaaaa;\n"