This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by denis-fatkulin on Jul 21 2022, 12:42 PM.

Details

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

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 12:42 PM
denis-fatkulin requested review of this revision.Jul 21 2022, 12:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 12:42 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
denis-fatkulin edited the summary of this revision. (Show Details)Jul 21 2022, 12:54 PM
curdeius edited the summary of this revision. (Show Details)Jul 21 2022, 2:45 PM
curdeius edited reviewers, added: HazardyKnusperkeks, owenpan, MyDeveloperDay, curdeius; removed: rsmith.

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

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 requested changes to this revision.Jul 22 2022, 10:00 PM

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
3314

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

This revision now requires changes to proceed.Jul 22 2022, 10:00 PM
denis-fatkulin retitled this revision from [clang-format] FIX: Misformatting lambdas with trailing return type 'auto' in braced lists to [clang-format] FIX: Misannotation 'auto' as trailing return type in lambdas.
denis-fatkulin edited the summary of this revision. (Show Details)

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.

Thanks for the changes.

clang/unittests/Format/TokenAnnotatorTest.cpp
752 ↗(On Diff #447052)

I'd just merge it with the test above.

This revision is now accepted and ready to land.Jul 23 2022, 12:41 PM
owenpan accepted this revision.Jul 23 2022, 8:54 PM
owenpan added inline comments.
clang/unittests/Format/TokenAnnotatorTest.cpp
752 ↗(On Diff #447052)

I'd just merge it with the test above.

+1.

753–756 ↗(On Diff #447052)

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

curdeius accepted this revision.Jul 23 2022, 9:41 PM
curdeius added inline comments.
clang/unittests/Format/TokenAnnotatorTest.cpp
752 ↗(On Diff #447052)
denis-fatkulin updated this revision to Diff 447115.EditedJul 24 2022, 12:59 AM

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

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.

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: fatkulin.denis@huawei.com

Thank you!

PS. Related patch should be merged too. D130417

denis-fatkulin marked 3 inline comments as done.Jul 25 2022, 10:28 PM
This revision was landed with ongoing or failed builds.Jul 27 2022, 12:25 PM
This revision was automatically updated to reflect the committed changes.