[clang-format] Fix formatting of if statements with BlockIndent

[gedare]

Summary

A bug with BlockIndent prevents line breaks within if (and else if) clauses.
While fixing this bug, it appears that AlignAfterOpenBracket is not designed
to work with loop and if statements, but AlwaysBreak works on if clauses.
The documentation and tests are not clear on whether or not this is intended.
This patch preserves the AlwaysBreak behavior but does not use BlockIndent on if
clauses.

It may be reasonable to create a new style option for alignment of if (and loop) clauses intentionally so AlwaysBreak and BlockIndent can be enabled or disabled for conditional blocks.

Fixes #54663.

[gedare]

Although I chose to preserve the existing behavior of AlwaysBreak and to mimic it for BlockIndent, I think it would be best to choose a consistent approach and document it. I also have a patch that does not apply the AlignAfterOpenBracket for if clauses, which could be done for only BlockIndent, or it could be done for AlwaysBreak too but that will cause some backward compatibility problems.

[gedare]

Ping.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

25486

Why remove?

[gedare]

Revert deleted line in test.

clang/unittests/Format/FormatTest.cpp

25486

oops. that's a spurious change. I have reverted it.

[MyDeveloperDay]

It looks like this was added in D109557: Adds a BlockIndent option to AlignAfterOpenBracket I'm trying to understand what bad stuff happens if you remove the "if" from

return !(Previous && Previous->is(tok::kw_for));

@csmulhern any thoughts? (if you are still around?)

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

25504

we don't do this.

25505–25510

you can't remove a test and just call it good... the original author put this test in for a reason I assume?

25514

isn't the breaking of if ( and } else if (\n inconsistent?

[gedare]

clang/unittests/Format/FormatTest.cpp

25504

OK, I'll move that other bug up my priority list, as the behavior of this test case involves both. That bug prevents breaking the line after the operand, I think there's some off-by-1 error somewhere in calculating the columns for blockindent breaks. Otherwise, this patch reveals that bug as a regression in this test case.

25505–25510

understood, although it seems that the original author may not have considered if statements as legitimately to be used in BlockIndented formats, since this test case does not block indent. So, the test case will have to change anyway, if the if should be blockindented.

25514

The breaking is dependent on what ends up exceeding the ColumnLimit. since if ( is shorter than } else if ( by about 5 characters, you can get different breaks. As far as I can tell, the behavior of AlwaysBreak and BlockIndent will only break after the opening parens if the first "argument" doesn't fit. From what I understand, the penalties and desirable breaks come into play then, based on breaking around commas or boolean operators. With this patch, the behavior here is now consistent for both AlwaysBreak and BlockIndent.

This behavior is undocumented and I do not know the intention. I would think it is better for the two options to behave consistently, but I'm also willing to treat them differently for backward compatibility while fixing this bug.

[gedare]

I guess since the current behavior does not treat if with block indentation, this bug fix should instead prefer to maintain the current behavior, and if someone wants to have block indentation of if it would need to be added as a new style option. Otherwise, this will introduce regressions like that test case.

[gedare]

Do not apply BlockIndent to if conditionals. Address other comments.

[gedare]

I have fixed the bug in a different way that avoids using BlockIndent on if conditionals. AlwaysBreak will continue to apply to if conditionals by default. I may propose a style option to control that behavior later.

[gedare]

Rebase to main

[gedare]

@MyDeveloperDay ping

I have rebased to main without any problems. This patch fixes the bug.

[owenpan]

clang/lib/Format/ContinuationIndenter.cpp

773–792 ↗

(On Diff #558063)

This is getting out of hand. Consider writing a lambda with early returns.

[owenpan]

clang/unittests/Format/FormatTest.cpp

25513–25523

Please use camel case.

[gedare]

Use cameLCAse in test

[gedare]

clang/lib/Format/ContinuationIndenter.cpp

773–792 ↗

(On Diff #558063)

I would probably not get to refactoring this until January.

clang/lib/Format/TokenAnnotator.cpp

if (Style.AlignAfterOpenBracket != FormatStyle::BAS_BlockIndent ||
       return false;
     }
     auto Next = Right.Next;
     if (Next && Next->is(tok::r_paren))
       Next = Next->Next;
     if (Next && Next->is(tok::l_paren))
       return false;
     const FormatToken *Previous = Right.MatchingParen->Previous;
-    return !(Previous && (Previous->is(tok::kw_for) || Previous->isIf()));
+    return !(Previous && Previous->is(tok::kw_for));
   }
 
   // Allow breaking after a trailing annotation, e.g. after a method
   // declaration.
   if (Left.is(TT_TrailingAnnotation)) {
     return !Right.isOneOf(tok::l_brace, tok::semi, tok::equal, tok::l_paren,
                           tok::less, tok::coloncolon);
   }

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, AlignAfterOpenBracketBlockIndentIfStatement) {
   verifyFormat("if (foo()) {\n"
                "  return;\n"
                "}",
                Style);
 
   verifyFormat("if (quitelongarg !=\n"
                "    (alsolongarg - 1)) { // ABC is a very longgggggggggggg "
                "comment\n"
                "  return;\n"
                "}",
                Style);
 
   Style.AlignAfterOpenBracket = FormatStyle::BAS_BlockIndent;
 
   verifyFormat("if (foo()) {\n"
                "  return;\n"
                "}",
                Style);
 
+  // Treating if clauses as block indents causes a known bug (#54808, #63383)
+  // breaking the following test. It gets formatted instead as:
+  // "if (quitelongarg != (alsolongarg - 1)\n"
+  // ") { // ABC is a very longgggggggggggg comment"
+  // "return;"
+  // "}"
+
+#if 0
   verifyFormat("if (quitelongarg !=\n"
                "    (alsolongarg - 1)) { // ABC is a very longgggggggggggg "
                "comment\n"
                "  return;\n"
                "}",
                Style);
+#endif
+
+  verifyFormat("void foo() {\n"
+               "  if (quitelongname < alsolongname ||\n"
+               "      anotherevenlongername <=\n"
+               "          thisreallyreallyreallyreallyreallyreallylongername ||"
+               "\n"
+               "      othername < thislastname) {\n"
+               "    return;\n"
+               "  } else if (\n"
+               "      quitelongname < alsolongname ||\n"
+               "      anotherevenlongername <=\n"
+               "          thisreallyreallyreallyreallyreallyreallylongername ||"
+               "\n"
+               "      othername < thislastname\n"
+               "  ) {\n"
+               "    return;\n"
+               "  }\n"
+               "}",
+               Style);
+
+  Style.ContinuationIndentWidth = 2;
+  verifyFormat("void foo() {\n"
+               "  if (\n"
+               "    ThisIsRatherALongIfClause && thatIExpectToBeBroken ||\n"
+               "    ontoMultipleLines && whenFormattedCorrectly\n"
+               "  ) {\n"
+               "    if (false) {\n"
+               "    } else if (\n"
+               "      thisIsRatherALongIfClause && thatIExpectToBeBroken ||\n"
+               "      ontoMultipleLines && whenFormattedCorrectly\n"
+               "    ) {\n"
+               "    }\n"
+               "  }\n"
+               "}",
+               Style);
 }
 
 TEST_F(FormatTest, AlignAfterOpenBracketBlockIndentForStatement) {
   auto Style = getLLVMStyle();
 
   verifyFormat("for (int i = 0; i < 5; ++i) {\n"
                "  doSomething();\n"
                "}",