clang-format: fix conflict between FormatStyle::BWACS_MultiLine and BeforeCatch/BeforeElse

[pastey]

Summary

Hi,

Found a bug introduced with BraceWrappingFlags AfterControlStatement MultiLine. This feature conflicts with the existing BeforeCatch and BeforeElse flags.

For example, our team uses BeforeElse.

if (foo ||
    bar) {
  doSomething();
}
else {
  doSomethingElse();
}

If we enable MultiLine (which we'd really love to do) we expect it to work like this:

if (foo ||
    bar)
{
  doSomething();
}
else {
  doSomethingElse();
}

What we actually get is:

if (foo ||
    bar)
{
  doSomething();
}
else
{
  doSomethingElse();
}

I added two tests that show this issue. One for if/else and one for try/catch (which is affected in the same way as if/else).

One more test with ColumnLimit set to 40 is to check that a suggested fix doesn't break existing cases. This test is the copy of an existing test from the same case, but it removes line wrap from the stage.

I suggest the fix, but I'm new to this code, so maybe you could suggest something better.

As far as I understood, the problem is caused by the following code:

CompoundStatementIndenter(UnwrappedLineParser *Parser,
                            const FormatStyle &Style, unsigned &LineLevel)
      : CompoundStatementIndenter(Parser, LineLevel,
                                  Style.BraceWrapping.AfterControlStatement,  // <----- here
                                  Style.BraceWrapping.IndentBraces) {}
  CompoundStatementIndenter(UnwrappedLineParser *Parser, unsigned &LineLevel,
                            bool WrapBrace, bool IndentBrace)
      : LineLevel(LineLevel), OldLineLevel(LineLevel) {
    if (WrapBrace)							      // <----- and here
      Parser->addUnwrappedLine();
    if (IndentBrace)
      ++LineLevel;
  }

Style.BraceWrapping.AfterControlStatement used to be boolean, now it's an enum. MultiLine and Always both turn into true, thus MultiLine leads to a call of addUnwrappedLine().
I tried to place Style.BraceWrapping.AfterControlStatement == FormatStyle::BWACS_Always right in the CompoundStatementIndenter constructor, but that breaks MultiLine.

As I said, I'm new to this code, so maybe my fix is not at the right place, so I would be glad if we find a better fix.

[Bouska]

Your code still breaks MultiLine

[ RUN      ] FormatTest.MultiLineControlStatements
/home/pablomg/dev/llvm-project/clang/unittests/Format/FormatTest.cpp:1562: Failure
      Expected: "try {\n" "  foo();\n" "} catch (\n" "    Exception &bar)\n" "{\n" "  baz();\n" "}"
      Which is: "try {\n  foo();\n} catch (\n    Exception &bar)\n{\n  baz();\n}"
To be equal to: format("try{foo();}catch(Exception&bar){baz();}", Style)
      Which is: "try {\n  foo();\n} catch (Exception\n             &bar) {\n  baz();\n}"
With diff:
@@ -1,7 +1,6 @@
 try {
   foo();
-} catch (
-    Exception &bar)
-{
+} catch (Exception
+             &bar) {
   baz();
 }

[  FAILED  ] FormatTest.MultiLineControlStatements (28 ms)

[Bouska]

So, you are trying to fix the issue at the wrong place. Contrary from what we should expect from a UnwrappedLine, BWACS_Multiline works by always wrapping the brace in UnwrappedLineParser, and afterwards in UnwrappedLineFormatter merging it back to the control statement if it's not in multi-line. The bug is on the control statement @ line 308 on UnwrappedLineFormatter.cpp. That's the block that merge the brace back to its control statement if it is not a multiline and it's executed only with a line with a else or a catch that begins with right curly brace.

[pastey]

Bouska, thank you! Don't know how I missed that failing test. Never commit on friday :)

However, seeing this test result in a browser window gave me that "a-ha". I understood that I had wrong perception of what MultiLine feature is all about. As I wrote before, I would expect it to format code like this:

if (foo ||
    bar)
{
  doSomething();
}
else {
  doSomethingElse();
}

In my head it didn't have anything to do with ColumnLimit – it was all about more than one line in 'if' condition expression.

After I understood that MultiLine is bound to ColumnLimit, I could understand what was going on @ line 308 on UnwrappedLineFormatter.cpp.

Also found that UnwrappedLine actually wraps every line, which is ... confusing.

Uploaded the new diff. Removed my previous "fix" from UnwrappedLineParser.cpp. Added special case for else/catch in UnwrappedLineFormatter.cpp. Tests pass.

[Bouska]

I'm not sure creating a special case for else/catch is the best idea. I would seem better to change the control statement @ line 309 to add it the case where we break before else or catch (add a new || test in the middle condition). I'm really new to this code too, so I don't know if it's better to check that previous line as a right curly brace and that BeforeElse/Catch is true and that first token of the line is else or catch or if just checking that the first token of the line is else or catch (or a mix in between). I'll let a more senior reviewer advise on this.

[pastey]

Thanks for your feedback. Let's see what other reviewers say.

[MyDeveloperDay]

I think generally speaking this looks ok, I'm ok with the extra clause, as long as it's clear what it's doing

clang/lib/Format/UnwrappedLineFormatter.cpp

329

nit: maybe leave a comment explaining the need for the extra clause

[pastey]

@MyDeveloperDay, thanks for looking into this. Added a comment

[pastey]

Guys, sorry for a dumb question – so the fix can be pushed now? If yes, then there's one issue – I don't think I have write access to push the fix – may I ask one of you to push it?

ERROR: Permission to llvm/llvm-project.git denied to pastey12.

clang/lib/Format/UnwrappedLineFormatter.cpp

if (I[1]->First->is(tok::l_brace) &&
                  : 0;
     } else if (I[1]->First->is(tok::l_brace) &&
                TheLine->First->isOneOf(tok::kw_if, tok::kw_while,
                                        tok::kw_for)) {
       return (Style.BraceWrapping.AfterControlStatement ==
               FormatStyle::BWACS_Always)
                  ? tryMergeSimpleBlock(I, E, Limit)
                  : 0;
+    } else if (I[1]->First->is(tok::l_brace) &&
+               TheLine->First->isOneOf(tok::kw_else, tok::kw_catch) &&
+               Style.BraceWrapping.AfterControlStatement ==
+                   FormatStyle::BWACS_MultiLine) {
+      // This case if different from the upper BWACS_MultiLine processing
+      // in that a preceding r_brace is not on the same line as else/catch
+      // 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) {

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, MultiLineControlStatements) {
   EXPECT_EQ("try {\n"
             "  foo();\n"
             "} catch (\n"
             "    Exception &bar)\n"
             "{\n"
             "  baz();\n"
             "}",
             format("try{foo();}catch(Exception&bar){baz();}", Style));
+  Style.ColumnLimit =
+      40; // to concentrate at brace wrapping, not line wrap due to column limit
+  EXPECT_EQ("try {\n"
+            "  foo();\n"
+            "} catch (Exception &bar) {\n"
+            "  baz();\n"
+            "}",
+            format("try{foo();}catch(Exception&bar){baz();}", Style));
+  Style.ColumnLimit =
+      20; // to concentrate at brace wrapping, not line wrap due to column limit
+
+  Style.BraceWrapping.BeforeElse = true;
+  EXPECT_EQ(
+      "if (foo) {\n"
+      "  bar();\n"
+      "}\n"
+      "else if (baz ||\n"
+      "         quux)\n"
+      "{\n"
+      "  foobar();\n"
+      "}\n"
+      "else {\n"
+      "  barbaz();\n"
+      "}",
+      format("if(foo){bar();}else if(baz||quux){foobar();}else{barbaz();}",
+             Style));
+
+  Style.BraceWrapping.BeforeCatch = true;
+  EXPECT_EQ("try {\n"
+            "  foo();\n"
+            "}\n"
+            "catch (...) {\n"
+            "  baz();\n"
+            "}",
+            format("try{foo();}catch(...){baz();}", Style));
 }
 
 //===----------------------------------------------------------------------===//
 // Tests for classes, namespaces, etc.
 //===----------------------------------------------------------------------===//
 
 TEST_F(FormatTest, DoesNotBreakSemiAfterClassDecl) {
   verifyFormat("class A {};");
TEST_F(FormatTest, AmbersandInLamda) {
   // Test case reported in https://bugs.llvm.org/show_bug.cgi?id=41899
   FormatStyle AlignStyle = getLLVMStyle();
   AlignStyle.PointerAlignment = FormatStyle::PAS_Left;
   verifyFormat("auto lambda = [&a = a]() { a = 2; };", AlignStyle);
   AlignStyle.PointerAlignment = FormatStyle::PAS_Right;
   verifyFormat("auto lambda = [&a = a]() { a = 2; };", AlignStyle);
 }
 
- TEST_F(FormatTest, SpacesInConditionalStatement) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInConditionalStatement = true;
   verifyFormat("for ( int i = 0; i; i++ )\n  continue;", Spaces);
   verifyFormat("if ( !a )\n  return;", Spaces);
   verifyFormat("if ( a )\n  return;", Spaces);
   verifyFormat("if constexpr ( a )\n  return;", Spaces);
   verifyFormat("switch ( a )\ncase 1:\n  return;", Spaces);
   verifyFormat("while ( a )\n  return;", Spaces);