[clang-format] Adjust braced list detection

[cpplearner]

Summary

This avoids mishandling nested compound statements that are followed by another compound statement.

Fixes https://llvm.org/PR38314 and https://llvm.org/PR48305.

[curdeius]

Thanks for working on this.
First of all, the test seems ok for https://llvm.org/PR48305, but I'd like to see a test case for https://llvm.org/PR38314 (since you wrote in the description that this bug gets fixed as well).
And of course, we need to make the failing tests pass! :)

[MyDeveloperDay]

FormatsBracedListsInColumnLayout is actually failing in its own test... it now produces

void foo() {
  { // asdf
    { int a; }
  }
  {
    { int b; }
  }
}

rather than previously

void foo() {
  {// asdf
   {int a;
}
}
{
  { int b; }
}
}

[MyDeveloperDay]

The other failure is

-class C extends {} {}
+class C extends {}
+{}

I'm slightly struggling to work out in both cases if its the test that is wrong @cpplearner what do you think?

[MyDeveloperDay]

For your test I think its your test that is wrong, looking at the simpler versions (using clang-format-12)

void foo() {
  {
    { int a; }
  }
}

void foo() {
  { int a; }
}

[cpplearner]

Fixed tests.

Since in JavaScript, the thing after extends must be an expression, and the thing after implements must be an object type, I decided to parse both by directly calling parseBracedList, instead of going through tryToParseBracedList.

[MyDeveloperDay]

I ran the examples from https://bugs.llvm.org/show_bug.cgi?id=38314 and its so much better than the previous state!

Thanks for this patch, LGTM

namespace n {
void foo() {
    {
        {
            {
                statement();
                if (false) {
                }
            }
        }
        {}
    }
}
}  // namespace n

void foo() {
    {
        {
            statement();
            if (false) {
            }
        }
    }
    {}
}  // namespace n

[HazardyKnusperkeks]

That will be a pain in the ass to merge with my current changes, but looks good.

In D114583#3153780, @curdeius wrote:

First of all, the test seems ok for https://llvm.org/PR48305, but I'd like to see a test case for https://llvm.org/PR38314 (since you wrote in the description that this bug gets fixed as well).

But I second that.

[HazardyKnusperkeks]

Thanks for the update.

(You forgot to add the context, but since it was already accepted it's okay.)

[curdeius]

Thanks! Do you need someone to land this on your behalf? If yes, don't forget to send here your name and email.

[cpplearner]

Yes, I need someone to commit it on my behalf. My name and email address: Tan S. B. <[e-mail]>

[krasimir]

It appears that this regressed the formatting of initializer lists in some cases:

# Before
% cat ~/test.cc
class A {
  A() : a{} {}

  A(int b) : b(b) {}

  A(int a, int b) : a(a), bs{{bs...}} {
    f();
  }

  int a, b;
};

# After
% build/bin/clang-format -style=google ~/test.cc
class A {
  A() : a {}
  {}

  A(int b) : b(b) {}

  A(int a, int b) : a(a), bs {
    { bs... }
  }
  { f(); }

  int a, b;
};

@cpplearner could you please take a look?

[MyDeveloperDay]

We might want to revert this until we get a solution to avoid flip flopping peoples formats, but this to me comes down to what we've been saying before about using the TT_ types to labels the types of () {} []

I think in hindsight adding l_brace here just too generic (we need to know if its a "BracedInitializer {" or just a scope "{"

(NextTok->isOneOf(tok::l_brace, tok::identifier)

[MyDeveloperDay]

This is having an impact on code in flang, I don't deny it might be better, but it may not be what is wanted.

-  explicit Expr(const Scalar<Result> &x) : u{Constant<Result>{x}} {}
-  explicit Expr(Scalar<Result> &&x) : u{Constant<Result>{std::move(x)}} {}
+  explicit Expr(const Scalar<Result> &x) : u {
+    Constant<Result> { x }
+  }
+  {}
+  explicit Expr(Scalar<Result> &&x) : u {
+    Constant<Result> { std::move(x) }
+  }
+  {}

clang/lib/Format/UnwrappedLineParser.cpp

case tok::r_brace:
                                   NextTok->OriginalColumn == 0;
 
           // If there is a comma, semicolon or right paren after the closing
           // brace, we assume this is a braced initializer 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).
-          // FIXME: Some of these do not apply to JS, e.g. "} {" can never be a
-          // braced list in JS.
           ProbablyBracedList =
               (Style.Language == FormatStyle::LK_JavaScript &&
                NextTok->isOneOf(Keywords.kw_of, Keywords.kw_in,
                                 Keywords.kw_as)) ||
               (Style.isCpp() && NextTok->is(tok::l_paren)) ||
               NextTok->isOneOf(tok::comma, tok::period, tok::colon,
-                               tok::r_paren, tok::r_square, tok::l_brace,
-                               tok::ellipsis) ||
-              (NextTok->is(tok::identifier) &&
+                               tok::r_paren, tok::r_square, tok::ellipsis) ||
+              (NextTok->isOneOf(tok::l_brace, tok::identifier) &&
                !PrevTok->isOneOf(tok::semi, tok::r_brace, tok::l_brace)) ||
               (NextTok->is(tok::semi) &&
                (!ExpectClassBody || LBraceStack.size() != 1)) ||
               (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();
while (FormatTok->isOneOf(tok::identifier, tok::coloncolon, tok::hashhash,
           FormatTok->isOneOf(tok::period, tok::comma))) {
     if (Style.Language == FormatStyle::LK_JavaScript &&
         FormatTok->isOneOf(Keywords.kw_extends, Keywords.kw_implements)) {
       // JavaScript/TypeScript supports inline object types in
       // extends/implements positions:
       //     class Foo implements {bar: number} { }
       nextToken();
       if (FormatTok->is(tok::l_brace)) {
-        tryToParseBracedList();
+        parseBracedList();
         continue;
       }
     }
     bool IsNonMacroIdentifier =
         FormatTok->is(tok::identifier) &&
         FormatTok->TokenText != FormatTok->TokenText.upper();
     nextToken();
     // We can have macros or attributes in between 'class' and the class name.

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"
       "                          1, 22, 333, 4444, 55555, 666666, 7777777});");