[clang-format] Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[jp4a50]

Summary

Fixes a long-standing bug: https://github.com/llvm/llvm-project/issues/44486.

The original diff that introduced the bug implemented behaviour that pushed the first argument to a function onto a new line under certain circumstances relating passing lambdas as arguments.

This behaviour was implemented in TokenAnnotator::mustBreakBefore() which meant the code lacked the necessary context to figure out whether subsequent arguments might be able to all fit on one line. As such, I've moved the implementation to ContinuationIndenter and, instead of forcing a line break at the first argument in all cases, we now allow the OptimizingLineFormatter to consider placing the first argument on the same line as the function call but don't allow further line breaks in this case.

The end result is that either the first argument must go on a new line (as before) or all arguments must be put on the current line.

[HazardyKnusperkeks]

clang/lib/Format/ContinuationIndenter.cpp

263

Was this uninitialized?

657

Can drop outer parens.

664–666

Could move this out of the lambda.

669

This can be merged with !Current.is(tok::comment).

[jp4a50]

Address minor review comments.

[jp4a50]

clang/lib/Format/ContinuationIndenter.cpp

263

It's a new member variable I added as part of this diff.

664–666

Not sure exactly what you're referring to here. Initialization of PrevNonComment?

[jp4a50]

Format files.

[HazardyKnusperkeks]

clang/lib/Format/ContinuationIndenter.cpp

263

Sorry, I looked for that, but somehow missed it.

664–666

Not sure exactly what you're referring to here. Initialization of PrevNonComment?

No everything in the if before you go into the PrevNonComment. It is highlighted if you hover the comment.

[jp4a50]

clang/lib/Format/ContinuationIndenter.cpp

664–666

Ah, I see, thanks. Technically I could move that outside the lambda. I'm just not sure how it's better? Are you making the suggestion because it would make it clearer that this logic only applies to cpp/objc and/or because it means we could avoid capturing Style?

[HazardyKnusperkeks]

clang/lib/Format/ContinuationIndenter.cpp

664–666

Can do how you want, I think avoid capturing Style would be nice. And you wouldn't search for PrevNonComment if you don't need to. This could also be done be reordering the statements and a new return.

[jp4a50]

Refactor DisallowLineBreaksOnThisLine lambda.

[jp4a50]

All comments addressed. PTAL and let me know if there are any other blockers for merge.

clang/lib/Format/ContinuationIndenter.cpp

664–666

I kept all conditions inside the lambda in the end since moving any out would have made the DisallowLineBreaksOnThisLine not reflect all conditions and I wanted to keep that.

Instead I've changed all conditions to the style of early return for consistency and ordered them in the way that I think makes them most readable.

[jp4a50]

Formatting.

[HazardyKnusperkeks]

clang/lib/Format/ContinuationIndenter.cpp

657–666

I'd go for this.
But that's not necessary.

659

lambdas

[jp4a50]

clang/lib/Format/ContinuationIndenter.cpp

657–666

Ah, yes, I see what you mean, now! Easy enough change to make so I'll do it since there's a typo to fix as well anyway.

[jp4a50]

More CR feedback.

[jp4a50]

Format files.

[jp4a50]

@HazardyKnusperkeks could you merge this for me assuming the build is green please? I don't have merge rights. Thanks.

[HazardyKnusperkeks]

In D156259#4558570, @jp4a50 wrote:

@HazardyKnusperkeks could you merge this for me assuming the build is green please? I don't have merge rights. Thanks.

That will have to wait, but if no one else stepped in, I'll do it in ~2 weeks.

[owenpan]

clang/docs/ReleaseNotes.rst

167–168 ↗

(On Diff #546972)

Please delete it. We only update the release note for new options.

clang/lib/Format/ContinuationIndenter.cpp

658–659

667–669

676

// Multiple lambdas in the same function call.

679

// A lambda followed by another arg.

686–692

[jp4a50]

Minor refactoring and comments.

[jp4a50]

Minor refactor.

[jp4a50]

Addressed all comments. Please let me know if there's anything else required before merging.

[owenpan]

In D156259#4607177, @jp4a50 wrote:

Addressed all comments. Please let me know if there's anything else required before merging.

There are still a couple of comments unaddressed plus another that asked for changing !is() to isNot(). :)

[jp4a50]

Minor refactoring.

[jp4a50]

@owenpan right you are! Missed those somehow. Made a further two changes. Hope I haven't missed anything else!

clang/lib/Format/ContinuationIndenter.h

struct LineState {
 
   /// The lowest \c NestingLevel on the current line.
   unsigned LowestLevelOnLine;
 
   /// The start column of the string literal, if we're in a string
   /// literal sequence, 0 otherwise.
   unsigned StartOfStringLiteral;
 
+  /// Disallow line breaks for this line.
+  bool NoLineBreak;
+
   /// A stack keeping track of properties applying to parenthesis
   /// levels.
   SmallVector<ParenState> Stack;
 
   /// Ignore the stack of \c ParenStates for state comparison.
   ///
   /// In long and deeply nested unwrapped lines, the current algorithm can
   /// be insufficient for finding the best formatting with a reasonable amount

clang/lib/Format/ContinuationIndenter.cpp

LineState ContinuationIndenter::getInitialState(unsigned FirstIndent,
   }
   State.Line = Line;
   State.NextToken = Line->First;
   State.Stack.push_back(ParenState(/*Tok=*/nullptr, FirstIndent, FirstIndent,
                                    /*AvoidBinPacking=*/false,
                                    /*NoLineBreak=*/false));
   State.NoContinuation = false;
   State.StartOfStringLiteral = 0;
+  State.NoLineBreak = false;
   State.StartOfLineLevel = 0;
   State.LowestLevelOnLine = 0;
   State.IgnoreStackForComparison = false;
 
   if (Style.Language == FormatStyle::LK_TextProto) {
     // We need this in order to deal with the bin packing of text fields at
     // global scope.
     auto &CurrentState = State.Stack.back();
if (Current.is(TT_ConditionalExpr) && Previous.is(tok::r_paren) &&
       Previous.MatchingParen && Previous.MatchingParen->Previous &&
       Previous.MatchingParen->Previous->MatchingParen &&
       Previous.MatchingParen->Previous->MatchingParen->is(TT_LambdaLBrace)) {
     // We have a lambda within a conditional expression, allow breaking here.
     assert(Previous.MatchingParen->Previous->is(tok::r_brace));
     return true;
   }
 
-  return !CurrentState.NoLineBreak;
+  return !State.NoLineBreak && !CurrentState.NoLineBreak;
 }
 
 bool ContinuationIndenter::mustBreak(const LineState &State) {
   const FormatToken &Current = *State.NextToken;
   const FormatToken &Previous = *Current.Previous;
   const auto &CurrentState = State.Stack.back();
   if (Style.BraceWrapping.BeforeLambdaBody && Current.CanBreakBefore &&
       Current.is(TT_LambdaLBrace) && Previous.isNot(TT_LineComment)) {

 
 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] {
+    // 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);
+  }();
+
+  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;
     // Move over * and & if they are bound to the variable name.
     const FormatToken *Tok = &Previous;
     while (Tok && CurrentState.VariablePos >= Tok->ColumnWidth) {
       CurrentState.VariablePos -= Tok->ColumnWidth;
       if (Tok->SpacesRequiredBefore != 0)
         break;
       Tok = Tok->Previous;
     }

clang/lib/Format/TokenAnnotator.cpp

if ((Style.Language == FormatStyle::LK_Proto ||
     //            v  v
     // sub: { ... } key: value
     // If there was a comment between `}` an `key` above, then `key` would be
     // put on a new line anyways.
     if (Left.isOneOf(tok::r_brace, tok::greater, tok::r_square))
       return true;
   }
 
-  // Deal with lambda arguments in C++ - we want consistent line breaks whether
-  // they happen to be at arg0, arg1 or argN. The selection is a bit nuanced
-  // as aggressive line breaks are placed when the lambda is not the last arg.
-  if ((Style.Language == FormatStyle::LK_Cpp ||
-       Style.Language == FormatStyle::LK_ObjC) &&
-      Left.is(tok::l_paren) && Left.BlockParameterCount > 0 &&
-      !Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) {
-    // Multiple lambdas in the same function call force line breaks.
-    if (Left.BlockParameterCount > 1)
-      return true;
-
-    // A lambda followed by another arg forces a line break.
-    if (!Left.Role)
-      return false;
-    auto Comma = Left.Role->lastComma();
-    if (!Comma)
-      return false;
-    auto Next = Comma->getNextNonComment();
-    if (!Next)
-      return false;
-    if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret))
-      return true;
-  }
-
   return false;
 }
 
 bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
                                     const FormatToken &Right) const {
   const FormatToken &Left = *Right.Previous;
   // Language-specific stuff.
   if (Style.isCSharp()) {

clang/unittests/Format/FormatTest.cpp

verifyFormat("auto L = []<int... T>(T...) {\n"
                "};");
   verifyFormat("auto L = []<Foo... T>(T...) {\n"
                "  {\n"
                "    f();\n"
                "    g();\n"
                "  }\n"
                "};");
 
-  // Multiple lambdas in the same parentheses change indentation rules. These
-  // lambdas are forced to start on new lines.
+  // Lambdas that fit on a single line within an argument list are not forced
+  // onto new lines.
+  verifyFormat("SomeFunction([] {});");
+  verifyFormat("SomeFunction(0, [] {});");
+  verifyFormat("SomeFunction([] {}, 0);");
+  verifyFormat("SomeFunction(0, [] {}, 0);");
+  verifyFormat("SomeFunction([] { return 0; }, 0);");
+  verifyFormat("SomeFunction(a, [] { return 0; }, b);");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; });");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  verifyFormat("auto loooooooooooooooooooooooooooong =\n"
+               "    SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  // Exceeded column limit. We need to break.
+  verifyFormat("auto loooooooooooooooooooooooooooongName = SomeFunction(\n"
+               "    [] { return anotherLooooooooooonoooooooongName; }, [] { "
+               "return 0; }, b);");
+
+  // Multiple multi-line lambdas in the same parentheses change indentation
+  // rules. These lambdas are always forced to start on new lines.
   verifyFormat("SomeFunction(\n"
                "    []() {\n"
                "      //\n"
                "    },\n"
                "    []() {\n"
                "      //\n"
                "    });");
 
-  // A lambda passed as arg0 is always pushed to the next line.
+  // A multi-line lambda passed as arg0 is always pushed to the next line.
   verifyFormat("SomeFunction(\n"
                "    [this] {\n"
                "      //\n"
                "    },\n"
                "    1);\n");
 
   // A multi-line lambda passed as arg1 forces arg0 to be pushed out, just like
   // the arg0 case above.