[clang-format] Avoid unnecessarily aggressive line-breaking when using "LambdaBodyIndentation: OuterScope" with argument bin-packing.

[jp4a50]

Summary

The pre-existing logic dictating indentation of function arguments (one or more of which are lambdas) has been heavily tailored towards the default (and historically, the only available) indentation of lambda bodies: "LambdaBodyIndentation: Signature".

Much of that logic has been geared towards avoiding inconsistent, over-indentation of lambdas when passed to functions. See https://reviews.llvm.org/D52676 for context.

When this logic is applied with "LambdaBodyIndentation: OuterScope", it mostly serves to introduce an unnecessary number of line breaks and actually misalign arguments with preceding or proceeding lambdas. This is particularly noticeable when using BinPackArguments: true. The benefits of the pre-existing logic are effectively non-existent when using OuterScope because lambda bodies are *already* consistently and minimally indented by the behaviour of OuterScope.

As such, my aim here is to, for OuterScope only, disable the aforementioned logic.

I'm aware that some of these changes are a little subjective so I am expecting and open to discussion on them! That being said, it's worth bearing in mind that I am acting on behalf of the maintainers/users of the KJ style guide whose preferred style motivated the implementation of OuterScope in the first place. We are attempting to fit to our preferred style (but not overfit to the extent that we contravene behaviour that can be reasonably expected).

[jp4a50]

clang/lib/Format/ContinuationIndenter.cpp

342

AFAICT, this only matters when using OuterScope, but it seems like a sensible exception to make in the general case, so I haven't included a check for OuterScope here.

1089

So I genuinely think that the original implementation here (which I've left intact *except* for when OuterScope is enabled) does not match up to the intention expressed in the comment in line 1018.

It effectively seems to assume that the parenthesis level entry which is second from the top is the 'parent' parenthesis level, but this ignores the fact that there are often "fake parens" entries in the parenthesis state stack which presumably should be ignored (as I have done in my implementation for OuterScope).

As such, there are a lot of cases where (when OuterScope is not enabled) we apply BreakBeforeParameter to the *existing* parenthesis level rather than the parent parenthesis level. So when I tried to "fix" this behaviour in the general case it broke a lot of tests, even though I think the behaviour may be what was originally intended.

Happy to discuss this and be proven wrong, but I thought I'd explain why I've added a special case here for OuterScope in even greater detail than I have in the comment.

[jp4a50]

Hey @MyDeveloperDay @owenpan . I'd appreciate if you guys have time to consider this change. It's the last significant issue my team and I are tracking that's blocking our adoption of clang-format for multiple codebases that use KJ :)

[owenpan]

In D148131#4304960, @jp4a50 wrote:

Hey @MyDeveloperDay @owenpan . I'd appreciate if you guys have time to consider this change. It's the last significant issue my team and I are tracking that's blocking our adoption of clang-format for multiple codebases that use KJ :)

We probably should fix https://github.com/llvm/llvm-project/issues/44486 first.

[jp4a50]

In D148131#4447426, @owenpan wrote:

In D148131#4304960, @jp4a50 wrote:

Hey @MyDeveloperDay @owenpan . I'd appreciate if you guys have time to consider this change. It's the last significant issue my team and I are tracking that's blocking our adoption of clang-format for multiple codebases that use KJ :)

We probably should fix https://github.com/llvm/llvm-project/issues/44486 first.

Hi @owenpan . Thanks for your reply. Sorry that I missed it for a while - I stopped monitoring the review after a while and missed the email notification because I hadn't figured out how to make my LLVM email notifications only send me notifications related to my reviews.

I've had a bit of a look into https://github.com/llvm/llvm-project/issues/44486 and I can see why it's happening. It is caused by the unconditional, aggressive line-breaking introduced by https://reviews.llvm.org/D52676 as mkurdej notes on the github issue.

Unfortunately I think the fact that the changes in https://reviews.llvm.org/D52676 were implemented in the TokenAnnotator (rather than ContinuationIndenter) means this will be a non-trivial fix - I don't think the decision to force these line breaks should have been implemented here as there is not enough context to handle edge cases like the one raised in https://github.com/llvm/llvm-project/issues/44486.

I'll have a go at moving the implementation of this behaviour into ContinuationIndenter to see if it allows me to fix the issue. My rough idea is to get the OptimizingLineFormatter to consider either breaking or not breaking before the first argument, but when we don't break before the first argument, disallow line-breaking before any other arguments (in all cases that we have lambda arguments except when there is a single lambda that is the last argument).

Hopefully that should allow us to maintain the same logic as we currently have but also consider the additional case of having all arguments (including the lambdas) on the same line. I doubt it will be as simple as that but thought I'd give you a heads up.

[jp4a50]

@owenpan I've put up a patch for the bug you mentioned here: https://reviews.llvm.org/D156259

I'll rebase this change on top of that if you like the look of it. It should be fairly trivial to do.

[jp4a50]

Stack diff on top of https://reviews.llvm.org/D156259.

[jp4a50]

Hello @owenpan , @HazardyKnusperkeks . Now that https://reviews.llvm.org/D156259 is merged, please can you take a look at this change?

[HazardyKnusperkeks]

clang/docs/ReleaseNotes.rst

169 ↗

(On Diff #546004)

Owens comment holds, no bug fix in release notes.

clang/lib/Format/ContinuationIndenter.cpp

1080

I don't know if LLVM has any policy in this regard, but I prefer this.

1103

see above

1110

Drop the if, the loop does the checking.

1111–1112

Would be consistent with the code base.

1125

[jp4a50]

Address review comments.

[jp4a50]

All comments addressed.

[HazardyKnusperkeks]

But please wait for other opinions.

[jp4a50]

In D148131#4628719, @HazardyKnusperkeks wrote:

But please wait for other opinions.

OK sure. @owenpan, any thoughts?

[owenpan]

LG with a couple of minor comments.

clang/lib/Format/ContinuationIndenter.cpp

342

To be consistent with line 1116 below.

clang/unittests/Format/FormatTest.cpp

22576

If so, we should put the test case below in a #if 0-#endif block with a FIXME comment?

[jp4a50]

Minor review comments.

[jp4a50]

Thanks all. All comments addressed now. Please merge for me once ready.

[owenpan]

clang/unittests/Format/FormatTest.cpp

22575

This line is too long. Will fix it before merging.

clang/lib/Format/ContinuationIndenter.cpp

if (Previous.opensScope() && Previous.isNot(tok::l_brace) &&
       State.LowestLevelOnLine < State.StartOfLineLevel &&
       State.LowestLevelOnLine < Current.NestingLevel) {
     return false;
   }
   if (Current.isMemberAccess() && CurrentState.ContainsUnwrappedBuilder)
     return false;
 
   // Don't create a 'hanging' indent if there are multiple blocks in a single
-  // statement.
+  // statement and we are aligning lambda blocks to their signatures.
   if (Previous.is(tok::l_brace) && State.Stack.size() > 1 &&
       State.Stack[State.Stack.size() - 2].NestedBlockInlined &&
-      State.Stack[State.Stack.size() - 2].HasMultipleNestedBlocks) {
+      State.Stack[State.Stack.size() - 2].HasMultipleNestedBlocks &&
+      Style.LambdaBodyIndentation == FormatStyle::LBI_Signature) {
     return false;
   }
 
   // Don't break after very short return types (e.g. "void") as that is often
   // unexpected.
   if (Current.is(TT_FunctionDeclarationName) && State.Column < 6) {
     if (Style.AlwaysBreakAfterReturnType == FormatStyle::RTBS_None)
       return false;
   }
 
   // If binary operators are moved to the next line (including commas for some
   // styles of constructor initializers), that's always ok.
   if (!Current.isOneOf(TT_BinaryOperator, tok::comma) &&
+      // Allow breaking opening brace of lambdas (when passed as function
+      // arguments) to a new line when BeforeLambdaBody brace wrapping is
+      // enabled.
+      (!Style.BraceWrapping.BeforeLambdaBody ||
+       Current.isNot(TT_LambdaLBrace)) &&
       CurrentState.NoLineBreakInOperand) {
     return false;
   }
 
   if (Previous.is(tok::l_square) && Previous.is(TT_ObjCMethodExpr))
     return false;
 
   if (Current.is(TT_ConditionalExpr) && Previous.is(tok::r_paren) &&

 
 void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
                                                  unsigned ExtraSpaces) {
   FormatToken &Current = *State.NextToken;
   assert(State.NextToken->Previous);
   const FormatToken &Previous = *State.NextToken->Previous;
   auto &CurrentState = State.Stack.back();
 
-  bool DisallowLineBreaksOnThisLine = Style.isCpp() && [&Current] {
+  bool DisallowLineBreaksOnThisLine =
+      Style.LambdaBodyIndentation == FormatStyle::LBI_Signature &&
+      Style.isCpp() && [&Current] {
         // Deal with lambda arguments in C++. The aim here is to ensure that we
         // don't over-indent lambda function bodies when lambdas are passed as
         // arguments to function calls. We do this by ensuring that either all
         // arguments (including any lambdas) go on the same line as the function
         // call, or we break before the first argument.
         auto PrevNonComment = Current.getPreviousNonComment();
         if (!PrevNonComment || PrevNonComment->isNot(tok::l_paren))
           return false;
         if (Current.isOneOf(tok::comment, tok::l_paren, TT_LambdaLSquare))
           return false;
         auto BlockParameterCount = PrevNonComment->BlockParameterCount;
         if (BlockParameterCount == 0)
           return false;
 
         // Multiple lambdas in the same function call.
         if (BlockParameterCount > 1)
           return true;
 
         // A lambda followed by another arg.
         if (!PrevNonComment->Role)
           return false;
         auto Comma = PrevNonComment->Role->lastComma();
         if (!Comma)
           return false;
         auto Next = Comma->getNextNonComment();
-    return Next && !Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret);
+        return Next &&
+               !Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret);
       }();
 
   if (DisallowLineBreaksOnThisLine)
     State.NoLineBreak = true;
 
   if (Current.is(tok::equal) &&
       (State.Line->First->is(tok::kw_for) || Current.NestingLevel == 0) &&
       CurrentState.VariablePos == 0) {
     CurrentState.VariablePos = State.Column;
bool NestedBlockSpecialCase =
        State.Stack[State.Stack.size() - 2].NestedBlockInlined) ||
       (Style.Language == FormatStyle::LK_ObjC && Current.is(tok::r_brace) &&
        State.Stack.size() > 1 && !Style.ObjCBreakBeforeNestedBlockParam);
   // Do not force parameter break for statements with requires expressions.
   NestedBlockSpecialCase =
       NestedBlockSpecialCase ||
       (Current.MatchingParen &&
        Current.MatchingParen->is(TT_RequiresExpressionLBrace));
-  if (!NestedBlockSpecialCase)
-    for (ParenState &PState : llvm::drop_end(State.Stack))
-      PState.BreakBeforeParameter = true;
+  if (!NestedBlockSpecialCase) {
+    auto ParentLevelIt = std::next(State.Stack.rbegin());
+    if (Style.LambdaBodyIndentation == FormatStyle::LBI_OuterScope &&
+        Current.MatchingParen && Current.MatchingParen->is(TT_LambdaLBrace)) {
+      // If the first character on the new line is a lambda's closing brace, the
+      // stack still contains that lambda's parenthesis. As such, we need to
+      // recurse further down the stack than usual to find the parenthesis level
+      // containing the lambda, which is where we want to set
+      // BreakBeforeParameter.
+      //
+      // We specifically special case "OuterScope"-formatted lambdas here
+      // because, when using that setting, breaking before the parameter
+      // directly following the lambda is particularly unsightly. However, when
+      // "OuterScope" is not set, the logic to find the parent parenthesis level
+      // still appears to be sometimes incorrect. It has not been fixed yet
+      // because it would lead to significant changes in existing behaviour.
+      //
+      // TODO: fix the non-"OuterScope" case too.
+      auto FindCurrentLevel = [&](const auto &It) {
+        return std::find_if(It, State.Stack.rend(), [](const auto &PState) {
+          return PState.Tok != nullptr; // Ignore fake parens.
+        });
+      };
+      auto MaybeIncrement = [&](const auto &It) {
+        return It != State.Stack.rend() ? std::next(It) : It;
+      };
+      auto LambdaLevelIt = FindCurrentLevel(State.Stack.rbegin());
+      auto LevelContainingLambdaIt =
+          FindCurrentLevel(MaybeIncrement(LambdaLevelIt));
+      ParentLevelIt = MaybeIncrement(LevelContainingLambdaIt);
+    }
+    for (auto I = ParentLevelIt, E = State.Stack.rend(); I != E; ++I)
+      I->BreakBeforeParameter = true;
+  }
 
   if (PreviousNonComment &&
       !PreviousNonComment->isOneOf(tok::comma, tok::colon, tok::semi) &&
       ((PreviousNonComment->isNot(TT_TemplateCloser) &&
         !PreviousNonComment->ClosesRequiresClause) ||
        Current.NestingLevel != 0) &&
       !PreviousNonComment->isOneOf(
           TT_BinaryOperator, TT_FunctionAnnotationRParen, TT_JavaAnnotation,
           TT_LeadingJavaAnnotation) &&
-      Current.isNot(TT_BinaryOperator) && !PreviousNonComment->opensScope()) {
+      Current.isNot(TT_BinaryOperator) && !PreviousNonComment->opensScope() &&
+      // We don't want to enforce line breaks for subsequent arguments just
+      // because we have been forced to break before a lambda body.
+      (!Style.BraceWrapping.BeforeLambdaBody ||
+       Current.isNot(TT_LambdaLBrace))) {
     CurrentState.BreakBeforeParameter = true;
   }
 
   // If we break after { or the [ of an array initializer, we should also break
   // before the corresponding } or ].
   if (PreviousNonComment &&
       (PreviousNonComment->isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
        opensProtoMessageField(*PreviousNonComment, Style))) {
     CurrentState.BreakBeforeClosingBrace = true;
   }
 
   if (PreviousNonComment && PreviousNonComment->is(tok::l_paren)) {
     CurrentState.BreakBeforeClosingParen =
         Style.AlignAfterOpenBracket == FormatStyle::BAS_BlockIndent;
   }
 
   if (CurrentState.AvoidBinPacking) {
     // If we are breaking after '(', '{', '<', or this is the break after a ':'
-    // to start a member initializater list in a constructor, this should not
+    // to start a member initializer list in a constructor, this should not
     // be considered bin packing unless the relevant AllowAll option is false or
     // this is a dict/object literal.
     bool PreviousIsBreakingCtorInitializerColon =
         PreviousNonComment && PreviousNonComment->is(TT_CtorInitializerColon) &&
         Style.BreakConstructorInitializers == FormatStyle::BCIS_AfterColon;
     bool AllowAllConstructorInitializersOnNextLine =
         Style.PackConstructorInitializers == FormatStyle::PCIS_NextLine ||
         Style.PackConstructorInitializers == FormatStyle::PCIS_NextLineOnly;

clang/unittests/Format/FormatTest.cpp

verifyFormat("std::sort(v.begin(), v.end(),\n"
                "  return foo.baz < bar.baz;\n"
                "});",
                Style);
   verifyFormat("void test() {\n"
                "  (\n"
                "      []() -> auto {\n"
                "    int b = 32;\n"
                "    return 3;\n"
-               "  },\n"
-               "      foo, bar)\n"
+               "  }, foo, bar)\n"
                "      .foo();\n"
                "}",
                Style);
   verifyFormat("void test() {\n"
                "  ([]() -> auto {\n"
                "    int b = 32;\n"
                "    return 3;\n"
                "  })\n"
verifyFormat("#define A                                                  \\\n"
                Style);
   verifyFormat("void foo() {\n"
                "  aFunction(1, b(c(foo, bar, baz, [](d) {\n"
                "    auto f = e(d);\n"
                "    return f;\n"
                "  })));\n"
                "}",
                Style);
+  verifyFormat("void foo() {\n"
+               "  aFunction(1, b(c(foo, Bar{}, baz, [](d) -> Foo {\n"
+               "    auto f = e(foo, [&] {\n"
+               "      auto g = h();\n"
+               "      return g;\n"
+               "    }, qux, [&] -> Bar {\n"
+               "      auto i = j();\n"
+               "      return i;\n"
+               "    });\n"
+               "    return f;\n"
+               "  })));\n"
+               "}",
+               Style);
+  verifyFormat("Namespace::Foo::Foo(LongClassName bar,\n"
+               "                    AnotherLongClassName baz)\n"
+               "    : baz{baz}, func{[&] {\n"
+               "  auto qux = bar;\n"
+               "  return aFunkyFunctionCall(qux);\n"
+               "}} {}",
+               Style);
   Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  // FIXME: The following test should pass, but fails at the time of writing.
+#if 0
+  // As long as all the non-lambda arguments fit on a single line, AlwaysBreak
+  // doesn't force an initial line break, even if lambdas span multiple lines.
   verifyFormat("void foo() {\n"
                "  aFunction(\n"
-               "      1, b(c(\n"
-               "             [](d) -> Foo {\n"
+               "      [](d) -> Foo {\n"
                "    auto f = e(d);\n"
                "    return f;\n"
-               "  },\n"
-               "             foo, Bar{},\n"
-               "             [] {\n"
+               "  }, foo, Bar{}, [] {\n"
+               "    auto g = h();\n"
+               "    return g;\n"
+               "  }, baz);\n"
+               "}",
+               Style);
+#endif
+  // A long non-lambda argument forces arguments to span multiple lines and thus
+  // forces an initial line break when using AlwaysBreak.
+  verifyFormat("void foo() {\n"
+               "  aFunction(\n"
+               "      1,\n"
+               "      [](d) -> Foo {\n"
+               "    auto f = e(d);\n"
+               "    return f;\n"
+               "  }, foo, Bar{},\n"
+               "      [] {\n"
                "    auto g = h();\n"
                "    return g;\n"
-               "  },\n"
-               "             baz)));\n"
+               "  }, bazzzzz,\n"
+               "      quuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuux);\n"
                "}",
                Style);
+  Style.BinPackArguments = false;
   verifyFormat("void foo() {\n"
-               "  aFunction(1, b(c(foo, Bar{}, baz, [](d) -> Foo {\n"
-               "    auto f = e(\n"
-               "        foo,\n"
-               "        [&] {\n"
-               "      auto g = h();\n"
-               "      return g;\n"
-               "    },\n"
-               "        qux,\n"
-               "        [&] -> Bar {\n"
-               "      auto i = j();\n"
-               "      return i;\n"
-               "    });\n"
-               "    return f;\n"
-               "  })));\n"
+               "  aFunction(\n"
+               "      1,\n"
+               "      [](d) -> Foo {\n"
+               "    auto f = e(d);\n"
+               "    return f;\n"
+               "  },\n"
+               "      foo,\n"
+               "      Bar{},\n"
+               "      [] {\n"
+               "    auto g = h();\n"
+               "    return g;\n"
+               "  },\n"
+               "      bazzzzz,\n"
+               "      quuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuux);\n"
                "}",
                Style);
-  verifyFormat("Namespace::Foo::Foo(\n"
-               "    LongClassName bar, AnotherLongClassName baz)\n"
-               "    : baz{baz}, func{[&] {\n"
-               "  auto qux = bar;\n"
-               "  return aFunkyFunctionCall(qux);\n"
-               "}} {}",
-               Style);
+  Style.BinPackArguments = true;
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.BeforeLambdaBody = true;
   verifyFormat("void foo() {\n"
                "  aFunction(\n"
-               "      1, b(c(foo, Bar{}, baz,\n"
-               "             [](d) -> Foo\n"
+               "      1, b(c(foo, Bar{}, baz, [](d) -> Foo\n"
                "  {\n"
                "    auto f = e(\n"
                "        [&]\n"
                "    {\n"
                "      auto g = h();\n"
                "      return g;\n"
-               "    },\n"
-               "        qux,\n"
-               "        [&] -> Bar\n"
+               "    }, qux, [&] -> Bar\n"
                "    {\n"
                "      auto i = j();\n"
                "      return i;\n"
                "    });\n"
                "    return f;\n"
                "  })));\n"
                "}",
                Style);