[clang-format] Adjust braced list detection (try 2)

[galenelias]

Summary

This is a retry of https://reviews.llvm.org/D114583, which was backed
out for regressions. This fixes
https://github.com/llvm/llvm-project/issues/33891 and
https://github.com/llvm/llvm-project/issues/52911.

Clang Format is detecting a nested scope followed by another open brace
as a braced initializer list due to incorrectly thinking it's matching a
braced initializer at the end of a constructor initializer list which is
followed by the body open brace.

Unfortunately, UnwrappedLineParser isn't doing a very detailed parse, so
it's not super straightforward to distinguish these cases given the
current structure of calculateBraceTypes. My current hypothesis is that
these can be disambiguated by looking at the token preceding the
l_brace, as initializer list parameters will be preceded by an
identifier, but a scope block generally will not (barring the MACRO
wildcard).

To this end, I am adding tracking of the previous token to the LBraceStack
to help scope this particular case.

TokenAnnotatorTests cherry picked from https://reviews.llvm.org/D150452
where @sstwcw also fixed this issue simultaneously.

[galenelias]

Looks like @sstwcw also submitted a fix for the same issue, with a bit of a different approach: https://reviews.llvm.org/D150452

[HazardyKnusperkeks]

Shouldn't it have token annotator tests?

clang/lib/Format/UnwrappedLineParser.cpp

495–496

580–581

[galenelias]

@HazardyKnusperkeks, thanks for the feedback. I added the TokenAnnotatorTests from @sstwcw's review. I also updated the design to use a single stack instead of the two parallel stacks.

[sstwcw]

I would suggest adding a link to the revision on the issue thread for your future bug fixes. So people like me don't try to fix what others have fixed.

[sstwcw]

To this end, I am tracking the previous token type in a stack parallel
to LBraceStack to help scope this particular case.

Maybe keep the description up to date with the code.

[HazardyKnusperkeks]

Please add the full stop to the code comment(s), and then mark review comments as done.

You should also give credit to @sstwcw in the commit message/differential description for the token annotator tests.

In D150403#4339530, @sstwcw wrote:

I would suggest adding a link to the revision on the issue thread for your future bug fixes. So people like me don't try to fix what others have fixed.

That's correct, you could even assign the issue to you (or ask to be done so), to show that someone is working on it.

clang/unittests/Format/TokenAnnotatorTest.cpp

44 ↗

(On Diff #521828)

To be consistent, and to get a better output when the expect fails.

[galenelias]

Addressed remaining review feedback.

[galenelias]

Thanks for the review and feedback. Sorry about the unfortunate timing between @sstwcw and my fix - we submitted our fixes less than 24 hours apart. I didn't think there would be another simultaneous fix for this 5 year old bug, so didn't even think to link to the review in the GitHub issue. I will definitely do this for any future contributions.

[galenelias]

Thanks @HazardyKnusperkeks! I don't have commit access, so will need someone to land this for me.

[HazardyKnusperkeks]

In D150403#4343708, @galenelias wrote:

Thanks @HazardyKnusperkeks! I don't have commit access, so will need someone to land this for me.

We'll wait a bit, if someone might have a comment. And (at least I) need name and email for the commit.

[galenelias]

In D150403#4347323, @HazardyKnusperkeks wrote:

We'll wait a bit, if someone might have a comment. And (at least I) need name and email for the commit.

Name: Galen Elias
Email: galenelias at gmail dot com

[rymiel]

This patch also fixes https://github.com/llvm/llvm-project/issues/52911 (which is probably a duplicate anyway)

clang/lib/Format/UnwrappedLineParser.cpp

584–585

llvm-project/clang/lib/Format/UnwrappedLineParser.cpp:583:71: warning: '&&' within '||' [-Wlogical-op-parentheses]
          NextTok->is(tok::l_brace) && LBraceStack.back().PrevTok &&
          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
llvm-project/clang/lib/Format/UnwrappedLineParser.cpp:583:71: note: place parentheses around the '&&' expression to silence this warning
          NextTok->is(tok::l_brace) && LBraceStack.back().PrevTok &&
                                                                  ^
          (

1 warning generated.

[owenpan]

In D150403#4350325, @galenelias wrote:

In D150403#4347323, @HazardyKnusperkeks wrote:

We'll wait a bit, if someone might have a comment. And (at least I) need name and email for the commit.

Name: Galen Elias
Email: galenelias at gmail dot com

Can you put your authorship in the 'John Doe <[e-mail]>' format?

In D150403#4358562, @rymiel wrote:

This patch also fixes https://github.com/llvm/llvm-project/issues/52911 (which is probably a duplicate anyway)

@galenelias Can you update the summary?

clang/unittests/Format/TokenAnnotatorTest.cpp

1789–1790 ↗

(On Diff #522245)

[galenelias]

Thanks for the additional review comments. Hopefully everything if fixed.

In D150403#4358845, @owenpan wrote:

Can you put your authorship in the 'John Doe <[e-mail]>' format?

Galen Elias <[e-mail]>

I'm not sure if there is somewhere I should be putting that in the summary or diff itself, or just here in the comments?

[owenpan]

In D150403#4362403, @galenelias wrote:

Galen Elias <[e-mail]>

I'm not sure if there is somewhere I should be putting that in the summary or diff itself, or just here in the comments?

Just here in the comments would be enough.

[owenpan]

This seems to cause a regression. See https://github.com/llvm/llvm-project/issues/64134. @galenelias any idea?

[galenelias]

In D150403#4539874, @owenpan wrote:

This seems to cause a regression. See https://github.com/llvm/llvm-project/issues/64134. @galenelias any idea?

I will take a look. The logic I added is trying to distinguish { } { by looking at the token prior to the first l_brace, and checking if it's an identifier to quantify this as a braced list brace. In this case, it's actually a >, so the code decides it's a scope brace instead.

I could easily patch it to also allow for a >, but don't want this to devolve into a long trail of hacks either. Let me think about it a bit more, and I'll come up with a forward fix.

clang/lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::calculateBraceTypes(bool ExpectClassBody) {
   // definitions, too.
   unsigned StoredPosition = Tokens->getPosition();
   FormatToken *Tok = FormatTok;
   const FormatToken *PrevTok = Tok->Previous;
   // Keep a stack of positions of lbrace tokens. We will
   // update information about whether an lbrace starts a
   // braced init list or a different block during the loop.
   SmallVector<FormatToken *, 8> LBraceStack;
+  // Track the previous token type corresponding to our lbraces // to help
+  // detect brace types
+  SmallVector<tok::TokenKind, 8> PrevTokenKindStack;
   assert(Tok->is(tok::l_brace));
   do {
     // Get next non-comment token.
     FormatToken *NextTok;
     do {
       NextTok = Tokens->getNextToken();
     } while (NextTok->is(tok::comment));
 
case tok::l_brace:
         } else if (PrevTok->is(tok::r_paren)) {
           // `) { }` can only occur in function or method declarations in JS.
           Tok->setBlockKind(BK_Block);
         }
       } else {
         Tok->setBlockKind(BK_Unknown);
       }
       LBraceStack.push_back(Tok);
+      PrevTokenKindStack.push_back(PrevTok ? PrevTok->Tok.getKind()
+                                           : tok::unknown);
       break;
     case tok::r_brace:
       if (LBraceStack.empty())
         break;
       if (LBraceStack.back()->is(BK_Unknown)) {
         bool ProbablyBracedList = false;
         if (Style.Language == FormatStyle::LK_Proto) {
           ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
case tok::r_brace:
 
           // If there is a comma, semicolon or right paren after the closing
           // brace, we assume this is a braced initializer list.
           // FIXME: Some of these do not apply to JS, e.g. "} {" can never be a
           // braced list in JS.
           ProbablyBracedList =
               ProbablyBracedList ||
               NextTok->isOneOf(tok::comma, tok::period, tok::colon,
-                               tok::r_paren, tok::r_square, tok::l_brace,
-                               tok::ellipsis);
+                               tok::r_paren, tok::r_square, tok::ellipsis);
+
+          // Distinguish between braced list in a constructor initializer list
+          // followed by constructor body, or just adjacent blocks
+          ProbablyBracedList = ProbablyBracedList ||
+                               NextTok->is(tok::l_brace) &&
+                                   PrevTokenKindStack.back() == tok::identifier;
 
           ProbablyBracedList =
               ProbablyBracedList ||
               (NextTok->is(tok::identifier) &&
                !PrevTok->isOneOf(tok::semi, tok::r_brace, tok::l_brace));
 
           ProbablyBracedList = ProbablyBracedList ||
                                (NextTok->is(tok::semi) &&
                                 (!ExpectClassBody || LBraceStack.size() != 1));
case tok::r_brace:
           Tok->setBlockKind(BK_BracedInit);
           LBraceStack.back()->setBlockKind(BK_BracedInit);
         } else {
           Tok->setBlockKind(BK_Block);
           LBraceStack.back()->setBlockKind(BK_Block);
         }
       }
       LBraceStack.pop_back();
+      PrevTokenKindStack.pop_back();
       break;
     case tok::identifier:
       if (!Tok->is(TT_StatementMacro))
         break;
       [[fallthrough]];
     case tok::at:
     case tok::semi:
     case tok::kw_if:

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
 
   verifyNoCrash("a<,");
 
   // No braced initializer here.
   verifyFormat("void f() {\n"
                "  struct Dummy {};\n"
                "  f(v);\n"
                "}");
+  verifyFormat("void foo() {\n"
+               "  { // asdf\n"
+               "    { int a; }\n"
+               "  }\n"
+               "  {\n"
+               "    { int b; }\n"
+               "  }\n"
+               "}");
+  verifyFormat("namespace n {\n"
+               "void foo() {\n"
+               "  {\n"
+               "    {\n"
+               "      statement();\n"
+               "      if (false) {\n"
+               "      }\n"
+               "    }\n"
+               "  }\n"
+               "  {}\n"
+               "}\n"
+               "} // namespace n");
 
   // Long lists should be formatted in columns even if they are nested.
   verifyFormat(
       "vector<int> x = function({1, 22, 333, 4444, 55555, 666666, 7777777,\n"
       "                          1, 22, 333, 4444, 55555, 666666, 7777777,\n"
       "                          1, 22, 333, 4444, 55555, 666666, 7777777,\n"
       "                          1, 22, 333, 4444, 55555, 666666, 7777777,\n"
       "                          1, 22, 333, 4444, 55555, 666666, 7777777,\n"