[clang-format] Fix short block when braking after control statement

[Bouska]

Summary

This patch fixes bug #44192

When clang-format is run with option AllowShortBlocksOnASingleLine, it is expected to either succeed in putting the short block with its control statement on a single line or fail and leave the block as is. When brace wrapping after control statement is activated, if the block + the control statement length is superior to column limit but the block alone is not, clang-format puts the block in two lines: one for the control statement and one for the block. This patch removes this unexpected behaviour. Current unittests are updated to check for this behaviour.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

610

any reason you are changing existing tests?

[Bouska]

clang/unittests/Format/FormatTest.cpp

610

According to the change that added this test (https://reviews.llvm.org/D37140?id=113074), this test was made to check that "the wrapping is forced by exceeding the column limit". This should be check at the limit condition when the control statement + the block exceed the column limit by a few characters. With the current test, the length of the block statement alone is sufficient to force the wrapping, that's why with the current code this test has the expected wrapping.

Even my change is incorrect, with the control statement, the curly braces, the spaces and the statement, it is way above the column limit of 40. I'm going to fix that. Alternatively, I just could add another test with the correct length.

[Bouska]

Set line length to column limit + 1 (41) in tests

[Bouska]

Updated unittest to check under the column limit and over the column limit.

[Bouska]

Ping @MyDeveloperDay @klimek

[MyDeveloperDay]

In D71512#1815512, @Bouska wrote:

Ping @MyDeveloperDay @klimek

clang/lib/Format/UnwrappedLineFormatter.cpp

359

I just can't understand how we are free to just remove a chunk of code. Why is this not being some sort of

if (Style.AllowShortBlocksOnASingleLine)

You are just not giving a convincing argument that this is a safe change without us having to go in a debug the original problem ourselves.

clang/unittests/Format/FormatTest.cpp

686

I'm just not comfortable with changing tests, code can still be written in the way it was before, and I cannot tell what it would now be, I believe tests should be additive

[Bouska]

clang/lib/Format/UnwrappedLineFormatter.cpp

359

This chunk of code only provides a bogus formatting and nothing else. So in order to fix the bug, I need to remove the whole block of code.

I wasn't clear on why this chunk of code created the bug, so let me explained. Let's take an example:

if (true)
{
    doStuff();
}

with Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always and Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never.

When formatting the first line, the control statement @ line 308 is going to fail (because Style.BraceWrapping.AfterControlStatement != FormatStyle::BWACS_Always) but the control statement @ line 322 is going to pass (because the second line is a left brace and the first token of the line is a if). As Style.BraceWrapping.AfterControlStatement == FormatStyle::BWACS_Always, tryMergeSimpleBlock() is going to handle the merging. There is two possible outcomes : either the block is simple and not too long and it can be merge in one line, or it is not and the whole block stays as it is. That line is then considered formated (due to the return).

When formatting the second line, the control statement @ line 332 (because the first and only token of the line is a left brace and the first token of the first line is a if) is going to pass and Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never, so we are going to redo tryMergeSimpleBlock() on the first line which failed the first time! So this is the part I understand, we are doing someting we already did. The part I don't understand is that this time tryMergeSimpleBlock() managed to merge the line (probably because of some changes on the annoted first line). As the merging worked, MergedLines > 0 @ line 340 is true so MergedLines is decremented (as explained by the comment) and we end up with the block merged but without the control statement:

if (true)
{ doStuff(); }

Which is not the expected formating.

I think there might be another bug hidden somewhere in tryMergeSimpleBlock().

[Bouska]

clang/unittests/Format/FormatTest.cpp

686

Ok, I'll keep the original test and add two new ones.

[Bouska]

Do not touch current tests.

[MyDeveloperDay]

This seems to LGTM, in the absence of any other input, I guess we have to rely on the tests.

[Bouska]

@MyDeveloperDay I don't have commit rights, could you land this change for me please?

clang/lib/Format/UnwrappedLineFormatter.cpp

if (I[1]->First->is(tok::l_brace) &&
       // most likely because of BeforeElse/BeforeCatch set to true.
       // If the line length doesn't fit ColumnLimit, leave l_brace on the
       // next line to respect the BWACS_MultiLine.
       return (Style.ColumnLimit == 0 ||
               TheLine->Last->TotalLength <= Style.ColumnLimit)
                  ? 1
                  : 0;
     }
-    // Try to merge either empty or one-line block if is precedeed by control
-    // statement token
-    if (TheLine->First->is(tok::l_brace) && TheLine->First == TheLine->Last &&
-        I != AnnotatedLines.begin() &&
-        I[-1]->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) {
-      unsigned MergedLines = 0;
-      if (Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never) {
-        MergedLines = tryMergeSimpleBlock(I - 1, E, Limit);
-        // If we managed to merge the block, discard the first merged line
-        // since we are merging starting from I.
-        if (MergedLines > 0)
-          --MergedLines;
-      }
-      return MergedLines;
-    }
     // Don't merge block with left brace wrapped after ObjC special blocks
     if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() &&
         I[-1]->First->is(tok::at) && I[-1]->First->Next) {
       tok::ObjCKeywordKind kwId = I[-1]->First->Next->Tok.getObjCKeywordID();
       if (kwId == clang::tok::objc_autoreleasepool ||
           kwId == clang::tok::objc_synchronized)
         return 0;
     }

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, FormatShortBracedStatements) {
   verifyFormat("if CONSTEXPR (true) {}", AllowSimpleBracedStatements);
   verifyFormat("while (true) {}", AllowSimpleBracedStatements);
   verifyFormat("for (;;) {}", AllowSimpleBracedStatements);
   verifyFormat("if (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("if constexpr (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fffffffffffffffffffffff(); }",
+               AllowSimpleBracedStatements);
+  verifyFormat("if (true) {\n"
+               "  ffffffffffffffffffffffff();\n"
+               "}",
+               AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
                "  ffffffffffffffffffffffffffffffffffffffffffffffffffffff();\n"
                "}",
                AllowSimpleBracedStatements);
   verifyFormat("if (true) { //\n"
                "  f();\n"
                "}",
                AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
                "  f();\n"
TEST_F(FormatTest, FormatShortBracedStatements) {
   verifyFormat("if CONSTEXPR (true) {}", AllowSimpleBracedStatements);
   verifyFormat("while (true) {}", AllowSimpleBracedStatements);
   verifyFormat("for (;;) {}", AllowSimpleBracedStatements);
   verifyFormat("if (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("if constexpr (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fffffffffffffffffffffff(); }",
+               AllowSimpleBracedStatements);
+  verifyFormat("if (true)\n"
+               "{\n"
+               "  ffffffffffffffffffffffff();\n"
+               "}",
+               AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
                "{\n"
                "  ffffffffffffffffffffffffffffffffffffffffffffffffffffff();\n"
                "}",
                AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
                "{ //\n"
                "  f();\n"
                "}",
                AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"

 
 TEST_F(FormatTest, ShortBlocksInMacrosDontMergeWithCodeAfterMacro) {
   FormatStyle Style = getLLVMStyleWithColumns(60);
   Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Always;
   Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
   Style.BreakBeforeBraces = FormatStyle::BS_Allman;
   EXPECT_EQ("#define A                                                  \\\n"
             "  if (HANDLEwernufrnuLwrmviferuvnierv)                     \\\n"
-            "  { RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier; }\n"
+            "  {                                                        \\\n"
+            "    RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier;               \\\n"
+            "  }\n"
             "X;",
             format("#define A \\\n"
                    "   if (HANDLEwernufrnuLwrmviferuvnierv) { \\\n"
                    "      RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier; \\\n"
                    "   }\n"
                    "X;",
                    Style));
 }