[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

579–580

[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

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

583–584

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

1806–1807

[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) {
   // parse macros, so this will magically work inside macro
   // 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;
+  struct StackEntry {
+    FormatToken *Tok;
+    const FormatToken *PrevTok;
+  };
+  SmallVector<StackEntry, 8> LBraceStack;
   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:
           Tok->setBlockKind(BK_BracedInit);
         } 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);
+      LBraceStack.push_back({Tok, PrevTok});
       break;
     case tok::r_brace:
       if (LBraceStack.empty())
         break;
-      if (LBraceStack.back()->is(BK_Unknown)) {
+      if (LBraceStack.back().Tok->is(BK_Unknown)) {
         bool ProbablyBracedList = false;
         if (Style.Language == FormatStyle::LK_Proto) {
           ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
         } else {
           // Skip NextTok over preprocessor lines, otherwise we may not
           // properly diagnose the block as a braced intializer
           // if the comma separator appears after the pp directive.
           while (NextTok->is(tok::hash)) {
case tok::r_brace:
           // Try to detect a braced list. Note that regardless how we mark inner
           // braces here, we will overwrite the BlockKind later if we parse a
           // braced list (where all blocks inside are by default braced lists),
           // or when we explicitly detect blocks (for example while parsing
           // lambdas).
 
           // If we already marked the opening brace as braced list, the closing
           // must also be part of it.
-          ProbablyBracedList = LBraceStack.back()->is(TT_BracedListLBrace);
+          ProbablyBracedList = LBraceStack.back().Tok->is(TT_BracedListLBrace);
 
           ProbablyBracedList = ProbablyBracedList ||
                                (Style.isJavaScript() &&
                                 NextTok->isOneOf(Keywords.kw_of, Keywords.kw_in,
                                                  Keywords.kw_as));
           ProbablyBracedList = ProbablyBracedList ||
                                (Style.isCpp() && NextTok->is(tok::l_paren));
 
           // 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) && LBraceStack.back().PrevTok &&
+               LBraceStack.back().PrevTok->is(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));
 
           ProbablyBracedList =
               ProbablyBracedList ||
               (NextTok->isBinaryOperator() && !NextIsObjCMethod);
 
           if (!Style.isCSharp() && NextTok->is(tok::l_square)) {
             // We can have an array subscript after a braced init
             // list, but C++11 attributes are expected after blocks.
             NextTok = Tokens->getNextToken();
             ProbablyBracedList = NextTok->isNot(tok::l_square);
           }
         }
         if (ProbablyBracedList) {
           Tok->setBlockKind(BK_BracedInit);
-          LBraceStack.back()->setBlockKind(BK_BracedInit);
+          LBraceStack.back().Tok->setBlockKind(BK_BracedInit);
         } else {
           Tok->setBlockKind(BK_Block);
-          LBraceStack.back()->setBlockKind(BK_Block);
+          LBraceStack.back().Tok->setBlockKind(BK_Block);
         }
       }
       LBraceStack.pop_back();
       break;
     case tok::identifier:
       if (!Tok->is(TT_StatementMacro))
         break;
       [[fallthrough]];
     case tok::at:
     case tok::semi:
     case tok::kw_if:
     case tok::kw_while:
     case tok::kw_for:
     case tok::kw_switch:
     case tok::kw_try:
     case tok::kw___try:
-      if (!LBraceStack.empty() && LBraceStack.back()->is(BK_Unknown))
-        LBraceStack.back()->setBlockKind(BK_Block);
+      if (!LBraceStack.empty() && LBraceStack.back().Tok->is(BK_Unknown))
+        LBraceStack.back().Tok->setBlockKind(BK_Block);
       break;
     default:
       break;
     }
     PrevTok = Tok;
     Tok = NextTok;
   } while (Tok->isNot(tok::eof) && !LBraceStack.empty());
 
   // Assume other blocks for all unclosed opening braces.
-  for (FormatToken *LBrace : LBraceStack)
-    if (LBrace->is(BK_Unknown))
-      LBrace->setBlockKind(BK_Block);
+  for (const auto &Entry : LBraceStack)
+    if (Entry.Tok->is(BK_Unknown))
+      Entry.Tok->setBlockKind(BK_Block);
 
   FormatTok = Tokens->setPosition(StoredPosition);
 }
 
 template <class T>
 static inline void hash_combine(std::size_t &seed, const T &v) {
   std::hash<T> hasher;
   seed ^= hasher(v) + 0x9e3779b9 + (seed << 6) + (seed >> 2);

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"

clang/unittests/Format/TokenAnnotatorTest.cpp

 };
 
 #define EXPECT_TOKEN_KIND(FormatTok, Kind)                                     \
   EXPECT_EQ((FormatTok)->Tok.getKind(), Kind) << *(FormatTok)
 #define EXPECT_TOKEN_TYPE(FormatTok, Type)                                     \
   EXPECT_EQ((FormatTok)->getType(), Type) << *(FormatTok)
 #define EXPECT_TOKEN_PRECEDENCE(FormatTok, Prec)                               \
   EXPECT_EQ((FormatTok)->getPrecedence(), Prec) << *(FormatTok)
+#define EXPECT_BRACE_KIND(FormatTok, Kind)                                     \
+  EXPECT_EQ(FormatTok->getBlockKind(), Kind) << *(FormatTok)
 #define EXPECT_TOKEN(FormatTok, Kind, Type)                                    \
   do {                                                                         \
     EXPECT_TOKEN_KIND(FormatTok, Kind);                                        \
     EXPECT_TOKEN_TYPE(FormatTok, Type);                                        \
   } while (false)
 
 TEST_F(TokenAnnotatorTest, UnderstandsUsesOfStarAndAmp) {
   auto Tokens = annotate("auto x = [](const decltype(x) &ptr) {};");
TEST_F(TokenAnnotatorTest, UnderstandsLabels) {
   Tokens = annotate("{ x: { break; } }");
   ASSERT_EQ(Tokens.size(), 9u) << Tokens;
   EXPECT_TOKEN(Tokens[2], tok::colon, TT_GotoLabelColon);
   Tokens = annotate("{ case x: { break; } }");
   ASSERT_EQ(Tokens.size(), 10u) << Tokens;
   EXPECT_TOKEN(Tokens[3], tok::colon, TT_CaseLabelColon);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsNestedBlocks) {
+  // The closing braces are not annotated. It doesn't seem to cause a problem.
+  // So we only test for the opening braces.
+  auto Tokens = annotate("{\n"
+                         "  {\n"
+                         "    { int a = 0; }\n"
+                         "  }\n"
+                         "  {}\n"
+                         "}");
+  ASSERT_EQ(Tokens.size(), 14u) << Tokens;
+  EXPECT_BRACE_KIND(Tokens[0], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[1], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[2], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[10], BK_Block);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang