[clang-format] [PR47936] AfterControlStatement: MultiLine breaks AllowShortFunctionsOnASingleLine

[MyDeveloperDay]

Summary

https://bugs.llvm.org/show_bug.cgi?id=47936

Using the MultiLine setting for BraceWrapping.AfterControlStatement appears to disable AllowShortFunctionsOnASingleLine, even in cases without any control statements

[curdeius]

LGTM, but I have one question. You mentioned in the bug ticket comment that "this exposes a greater issue in AllowShortXXX". Have you found other cases that misbehave? If so, then it would probably be good to add them.

[KyrBoh]

In D114521#3151431, @curdeius wrote:

LGTM, but I have one question. You mentioned in the bug ticket comment that "this exposes a greater issue in AllowShortXXX". Have you found other cases that misbehave? If so, then it would probably be good to add them.

There also seems to be a behavior caused by AfterControlStatement = Multiline. This code:

if (condition) { contrinue; }

Is broken up into lines:

if (condition)
{
  continue;
}

I am not sure if this is intended behavior or not.

Because when having this configuration:

BreakBeforeBraces: Custom
BraceWrapping:
  AfterControlStatement: Never # Always works too!
AllowShortBlocksOnASingleLine: Always
AllowShortIfStatementsOnASingleLine: WithoutElse # any value except Never

clang-format would leave that code unchanged.

The same goes to loops (and AllowShortLoopsOnASingleLine)

[MyDeveloperDay]

In D114521#3151431, @curdeius wrote:

LGTM, but I have one question. You mentioned in the bug ticket comment that "this exposes a greater issue in AllowShortXXX". Have you found other cases that misbehave? If so, then it would probably be good to add them.

So when I took a look I think I stumbled on what @KyrBoh said below, but I'm not convinced its related. (so really want to deal with this separately)

What it seems is "AllowShortIfStatementsOnASingleLine" ONLY seems to work when there are no {} (this seems to be by design https://clang.llvm.org/docs/ClangFormatStyleOptions.html)

i.e. for

BasedOnStyle: LLVM
AllowShortIfStatementsOnASingleLine: AllIfsAndElse
ColumnLimit: 80

I get

class Foo {
  foo() {
    if (x) bar();
    if (x) {
      bar();
    }
  }
};

So I think that was just me not understanding that limitation, (although I agree it doesn't seem to follow other options of a similar name)

[curdeius]

LGTM.
@KyrBoh, could you create a bug report for the problem you described, please?

[MyDeveloperDay]

However if we add

AllowShortBlocksOnASingleLine : Always

then it will be

class Foo {
  foo() {
    if (x) bar();
    if (x) { bar(); }
  }
};

but if we set

BasedOnStyle: LLVM
AllowShortIfStatementsOnASingleLine: Never
ColumnLimit: 80
AllowShortBlocksOnASingleLine : Always

we get

class Foo {
  foo() {
    if (x)
      bar();
    if (x) {
      bar();
    }
  }
};

which feels like if(x){ bar(); } is primarily controlled by AllowShortBlocksOnASingleLine i.e. setting it to Never will override the AllowShortIfStatementsOnASingleLine setting for if statements with braces.

However if AllowShortBlocksOnASingleLine is Always then if AllowShortIfStatementsOnASingleLine is set to Never then it wins.

[KyrBoh]

@MyDeveloperDay

What it seems is "AllowShortIfStatementsOnASingleLine" ONLY seems to work when there are no {}

AFAIR, there are 2 settings (in case of control statements):

• AllowShortBlocksOnASingleLine --> whether to treat (curly-)braced blocks as a single statement (i.e. allow them be one-liners)
• AllowShortIfStatementsOnASingleLine --> this actually tells whether control statements *can* be one-liners

So these two should be considered in pair. BTW, the documentation for AllowShortIfStatementsOnASingleLine seems to be a bit misleading

EDIT, nevermind: seen the coment above.

@curdeius
Alright, I'll create a report https://bugs.llvm.org/show_bug.cgi?id=52598

[MyDeveloperDay]

@curdeius
Alright, I'll create a report https://bugs.llvm.org/show_bug.cgi?id=52598

That is related. But I'd rather handle it separately because the fix for the function and the fix for a while or if will need to be different.

It really comes down to this line..

if (Line.First == Line.Last && Line.First->isNot(TT_FunctionLBrace)

in the if case and in the while case the { isn't labeled like it is for the function

M=0 C=0 T=Unknown S=1 F=0 B=0 BK=1 P=0 Name=l_brace L=1 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='{'

To be honest this is something we should think about, if every { and } every (,) and [,] got given a Type labels this would be super good at disambiguating them

This is not something new, we've been doing it with the ')' of a cstyle cast for years CastRParen

M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=int L=3 PPK=2 FakeLParens=2/ FakeRParens=0 II=0x25a969b1660 Text='int'
M=0 C=1 T=StartOfName S=1 F=0 B=0 BK=0 P=220 Name=identifier L=5 PPK=2 FakeLParens= FakeRParens=0 II=0x25a969b8768 Text='a'
M=0 C=0 T=BinaryOperator S=1 F=0 B=0 BK=0 P=22 Name=equal L=7 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='='
M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=22 Name=l_paren L=9 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=59 Name=int L=12 PPK=2 FakeLParens= FakeRParens=0 II=0x25a969b1660 Text='int'
M=0 C=0 T=CastRParen S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=13 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'

but in this case, the real problem here is that Multiline just too lax, basically Multiline doesn't in my view seem to be doing what it expects

From this is seems it should ONLY be returning 0 (meaning don't join the line), if the "clause" in the (if,for,while) contains multiple conditions that have been broken over several lines.

I just don't think this is sufficiently constrained.

// Don't merge a trailing multi-line control statement block like:
// } else if (foo &&
//            bar)
// { <-- current Line
//   baz();
// }
if (Line.First == Line.Last  &&
    Style.BraceWrapping.AfterControlStatement ==
        FormatStyle::BWACS_MultiLine)
  return 0;

[MyDeveloperDay]

I think the problem here is encapsulated by adding the following tests

 Style.BraceWrapping.AfterFunction = true;
 Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
 Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_AllIfsAndElse;
 Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Always;
 Style.AllowShortLoopsOnASingleLine = true;
 Style.ColumnLimit = 80;

 Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Never;
 verifyFormat("void shortfunction() { bar(); }", Style);
 verifyFormat("if (x) { bar(); }", Style);
 verifyFormat("for (;;) { bar(); }", Style);
 verifyFormat("while (true) { bar(); }", Style);
 Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_MultiLine;
 verifyFormat("void shortfunction() { bar(); }", Style);
 verifyFormat("if (x) { bar(); }", Style);
>>> FAILS  ^^^^^^^^^^^^^^^^^^^^^^^^^^^

 verifyFormat("for (;;) { bar(); }", Style);
>>> FAILS  ^^^^^^^^^^^^^^^^^^^^^^^^^^^

 verifyFormat("while (true) { bar(); }", Style);
>>> FAILS  ^^^^^^^^^^^^^^^^^^^^^^^^^^^

 Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Never;
 Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
 Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Never;
 Style.AllowShortLoopsOnASingleLine = false;
 verifyFormat("void shortfunction()\n"
              "{\n"
              "  foo();\n"
              "}",
              Style);
 verifyFormat("if (x) {\n"
              "  foo();\n"
              "}",
              Style);
 verifyFormat("for (;;) {\n"
              "  foo();\n"
              "}",
              Style);
 verifyFormat("while (true) {\n"
              "  foo();\n"
              "}",
              Style);

 Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_MultiLine;
 verifyFormat("void shortfunction()\n"
              "{\n"
              "  foo();\n"
              "}",
              Style);
 verifyFormat("if (x) {\n"
              "  foo();\n"
              "}",
              Style);
 verifyFormat("for (;;) {\n"
              "  foo();\n"
              "}",
              Style);
 verifyFormat("while (true) {\n"
              "  foo();\n"
              "}",
              Style);

[MyDeveloperDay]

The if/for/while case is way more involved and requires a change in a separate piece of code, I'm going to land this and we can handle 52598 separately

clang/lib/Format/UnwrappedLineFormatter.cpp

if (Line.Last->is(tok::l_brace)) {
           return 0;
 
         // Don't merge a trailing multi-line control statement block like:
         // } else if (foo &&
         //            bar)
         // { <-- current Line
         //   baz();
         // }
-        if (Line.First == Line.Last &&
+        if (Line.First == Line.Last && Line.First->isNot(TT_FunctionLBrace) &&
             Style.BraceWrapping.AfterControlStatement ==
                 FormatStyle::BWACS_MultiLine)
           return 0;
 
         return 2;
       }
     } else if (I[1]->First->is(tok::l_brace)) {
       if (I[1]->Last->is(TT_LineComment))

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, MultiLineControlStatements) {
   Style.BraceWrapping.BeforeCatch = true;
   EXPECT_EQ("try {\n"
             "  foo();\n"
             "}\n"
             "catch (...) {\n"
             "  baz();\n"
             "}",
             format("try{foo();}catch(...){baz();}", Style));
+
+  Style.BraceWrapping.AfterFunction = true;
+  Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_MultiLine;
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
+  Style.ColumnLimit = 80;
+  verifyFormat("void shortfunction() { bar(); }", Style);
+
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
+  verifyFormat("void shortfunction()\n"
+               "{\n"
+               "  bar();\n"
+               "}",
+               Style);
 }
 
 TEST_F(FormatTest, BeforeWhile) {
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeBraces = FormatStyle::BraceBreakingStyle::BS_Custom;
 
   verifyFormat("do {\n"
                "  foo();\n"