[clang-format] FIX: Misannotation 'auto' as trailing return type in lambdas

[denis-fatkulin]

Summary

Lambdas with trailing return type 'auto' are annotated incorrectly. It causes a misformatting. The simpliest code to reproduce is:

auto list = {[]() -> auto { return 0; }};

Fixes https://github.com/llvm/llvm-project/issues/54798

[curdeius]

Could you please add full git context?
Was the problem due to misannotation of auto? If so, could you add an annotator test?

[denis-fatkulin]

Could you please add full git context?

I updated the patch with properly git context. Thanks!

Was the problem due to misannotation of auto? If so, could you add an annotator test?

I'm not sure about the questions. I will try to explain my patch purpose. Actually there was two problems:

1. auto wasn't detected as properly type keyword in lambda's trailing return type. So, formatting was completely wrong for this case (fixed in clang/lib/Format/UnwrappedLineParser.cpp)
2. The keyword auto and left brace { was interpreted as declaration auto{}. So, formatting delete a space symbol between them. (fixed in clang/lib/Format/TokenAnnotator.cpp)

Both cases are checked in changes at clang/unittests/Format/FormatTest.cpp and I think the unit test is sufficient.

[HazardyKnusperkeks]

In D130299#3672486, @denis-fatkulin wrote:

Could you please add full git context?

I updated the patch with properly git context. Thanks!

Was the problem due to misannotation of auto? If so, could you add an annotator test?

I'm not sure about the questions. I will try to explain my patch purpose. Actually there was two problems:

1. auto wasn't detected as properly type keyword in lambda's trailing return type. So, formatting was completely wrong for this case (fixed in clang/lib/Format/UnwrappedLineParser.cpp)
2. The keyword auto and left brace { was interpreted as declaration auto{}. So, formatting delete a space symbol between them. (fixed in clang/lib/Format/TokenAnnotator.cpp)

Both cases are checked in changes at clang/unittests/Format/FormatTest.cpp and I think the unit test is sufficient.

The question is, wether auto did get the type kw_auto before your change or not. If not a test for the token annotator would be in place. See e.g. D129946.
And I actually think you should amend the test case from there, because as far as I can see if there was an auto we would stop parsing the lambda and not assign the TT_LambdaLBrace, etc. In fact I can't see the anything related to braced lists in your fixes. The error was primarily an annotation error, with the adding space as a formation error. The fucked up braced list is just a follow up error.

clang/lib/Format/TokenAnnotator.cpp

3314 ↗

(On Diff #446916)

3314 ↗

(On Diff #446916)

Maybe split it up in two changes, and change it to this, because I think we would have the same problem.

[denis-fatkulin]

Patch is splitted, as suggested by @HazardyKnusperkeks

This piece of changes is related to misannotation KW 'auto' as trailing return type in lambdas. Also, appropriate test is added.

[HazardyKnusperkeks]

Thanks for the changes.

clang/unittests/Format/TokenAnnotatorTest.cpp

767

I'd just merge it with the test above.

[owenpan]

clang/unittests/Format/TokenAnnotatorTest.cpp

767

I'd just merge it with the test above.

+1.

768–771

Should we add a test case with an & between auto and {?

[curdeius]

clang/unittests/Format/TokenAnnotatorTest.cpp

767

[denis-fatkulin]

Merrged with previous test case. Cases for auto & and auto * are added also.

[HazardyKnusperkeks]

Do you have commit access, or do you want to apply for it, or do you need someone to push the change(s)? If the latter we need a name and mail for the commit.

[denis-fatkulin]

In D130299#3674586, @HazardyKnusperkeks wrote:

Do you have commit access, or do you want to apply for it, or do you need someone to push the change(s)? If the latter we need a name and mail for the commit.

I haven't access for merging changes to code base. So, I need support from community for doing this.

My name and mail:
Name: Denis Fatkulin
Mail: [e-mail]

Thank you!

PS. Related patch should be merged too. D130417

clang/lib/Format/UnwrappedLineParser.cpp

while (FormatTok->isNot(tok::l_brace)) {
     case tok::l_brace:
       break;
     case tok::l_paren:
       parseParens();
       break;
     case tok::l_square:
       parseSquare();
       break;
+    case tok::kw_auto:
     case tok::kw_class:
     case tok::kw_template:
     case tok::kw_typename:
       assert(FormatTok->Previous);
       if (FormatTok->Previous->is(tok::less))
         InTemplateParameterList = true;
       nextToken();
       break;

clang/unittests/Format/TokenAnnotatorTest.cpp

TEST_F(TokenAnnotatorTest, UnderstandsObjCBlock) {
   EXPECT_TOKEN(Tokens[9], tok::l_brace, TT_ObjCBlockLBrace);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsLambdas) {
   auto Tokens = annotate("[]() constexpr {}");
   ASSERT_EQ(Tokens.size(), 8u) << Tokens;
   EXPECT_TOKEN(Tokens[0], tok::l_square, TT_LambdaLSquare);
   EXPECT_TOKEN(Tokens[5], tok::l_brace, TT_LambdaLBrace);
+
+  Tokens = annotate("[]() -> auto {}");
+  ASSERT_EQ(Tokens.size(), 9u) << Tokens;
+  EXPECT_TOKEN(Tokens[4], tok::arrow, TT_LambdaArrow);
+  EXPECT_TOKEN(Tokens[6], tok::l_brace, TT_LambdaLBrace);
+
+  Tokens = annotate("[]() -> auto & {}");
+  ASSERT_EQ(Tokens.size(), 10u) << Tokens;
+  EXPECT_TOKEN(Tokens[4], tok::arrow, TT_LambdaArrow);
+  EXPECT_TOKEN(Tokens[7], tok::l_brace, TT_LambdaLBrace);
+
+  Tokens = annotate("[]() -> auto * {}");
+  ASSERT_EQ(Tokens.size(), 10u) << Tokens;
+  EXPECT_TOKEN(Tokens[4], tok::arrow, TT_LambdaArrow);
+  EXPECT_TOKEN(Tokens[7], tok::l_brace, TT_LambdaLBrace);
 }
 
 } // namespace
 } // namespace format
 } // namespace clang