[clang-format] Fix putting ObjC message arguments in one line for multiline receiver

[jolesiak]

Summary

Reapply reverted changes from D46879.

Currently BreakBeforeParameter is set to true everytime message receiver spans multiple lines, e.g.:

[[object block:^{
  return 42;
}] aa:42 bb:42];

will be formatted:

[[object block:^{
  return 42;
}] aa:42
   bb:42];

even though arguments could fit into one line. This change fixes this behavior.

Test Plan:
make -j12 FormatTests && tools/clang/unittests/Format/FormatTests

[krasimir]

unittests/Format/FormatTestObjC.cpp

815

What's the receiver's scope in this comment referring to?
Also, how would the old test cases be formatted?

[benhamilton]

Please address @krasimir's comments, but looks good to me.

[jolesiak]

unittests/Format/FormatTestObjC.cpp

815

For a receiver: [obj a:42] I meant ] as a token closing the scope.
I'll rephrase the comment to be more precise.

Old tests were introduced in D46879. After this change the formatting will be the same as it was before D46879, i.e. the same as for last test touched in this change:

[[obj aaaaaa:42
           b:42]
    cc:42
     d:42];

even if

[[obj aaaaaa:42
           b:42]
    cc:42 d:42];

satisfies the column limit.

[krasimir]

unittests/Format/FormatTestObjC.cpp

815

Ah, I think get it now: the new code should only apply to after object blocks and not after object receivers? Is this the intention?

[jolesiak]

unittests/Format/FormatTestObjC.cpp

815

The intention is to put arguments into one line if they fit but only in the same line as last character of a receiver expression, e.g.:

[[object block:^{
  return 42;
}] aa:42 bb:42];

instead of

[[object block:^{
  return 42;
}] aa:42
   bb:42];

but not

[[obj a:42]
    a:42 b:42];

I think it gets a little bit too complicated for no reason. Let me revert D46879 and rebase this change.

[jolesiak]

Rebase.

[benhamilton]

Can you update the diff description to reflect that it includes the original change?

[krasimir]

unittests/Format/FormatTestObjC.cpp

815

In that case, would this be allowed?

//      limit:       V
[[obj a:42
      b:42
      c:42
      d:42] e:42 f:42]

[jolesiak]

• Add test

[jolesiak]

unittests/Format/FormatTestObjC.cpp

815

Yes, I added this test.

[jolesiak]

Does it look fine now @krasimir ?

lib/Format/ContinuationIndenter.cpp

unsigned ContinuationIndenter::moveStateToNextToken(LineState &State,
   }
   if (!Current.opensScope() && !Current.closesScope() &&
       !Current.is(TT_PointerOrReference))
     State.LowestLevelOnLine =
         std::min(State.LowestLevelOnLine, Current.NestingLevel);
   if (Current.isMemberAccess())
     State.Stack.back().StartOfFunctionCall =
         !Current.NextOperator ? 0 : State.Column;
-  if (Current.is(TT_SelectorName) &&
-      !State.Stack.back().ObjCSelectorNameFound) {
+  if (Current.is(TT_SelectorName))
     State.Stack.back().ObjCSelectorNameFound = true;
-
-    // Reevaluate whether ObjC message arguments fit into one line.
-    // If a receiver spans multiple lines, e.g.:
-    //   [[object block:^{
-    //     return 42;
-    //   }] a:42 b:42];
-    // BreakBeforeParameter is calculated based on an incorrect assumption
-    // (it is checked whether the whole expression fits into one line without
-    // considering a line break inside a message receiver).
-    if (Current.Previous && Current.Previous->closesScope() &&
-        Current.Previous->MatchingParen &&
-        Current.Previous->MatchingParen->Previous) {
-      const FormatToken &CurrentScopeOpener =
-          *Current.Previous->MatchingParen->Previous;
-      if (CurrentScopeOpener.is(TT_ObjCMethodExpr) &&
-          CurrentScopeOpener.MatchingParen) {
-        int NecessarySpaceInLine =
-            getLengthToMatchingParen(CurrentScopeOpener, State.Stack) +
-            CurrentScopeOpener.TotalLength - Current.TotalLength - 1;
-        if (State.Column + Current.ColumnWidth + NecessarySpaceInLine <=
-            Style.ColumnLimit)
-          State.Stack.back().BreakBeforeParameter = false;
-      }
-    }
-  }
   if (Current.is(TT_CtorInitializerColon) &&
       Style.BreakConstructorInitializers != FormatStyle::BCIS_AfterColon) {
     // Indent 2 from the column, so:
     // SomeClass::SomeClass()
     //     : First(...), ...
     //       Next(...)
     //       ^ line up here.
     State.Stack.back().Indent =
void ContinuationIndenter::moveStatePastScopeCloser(LineState &State) {
   // stacks.
   if (State.Stack.size() > 1 &&
       (Current.isOneOf(tok::r_paren, tok::r_square, TT_TemplateString) ||
        (Current.is(tok::r_brace) && State.NextToken != State.Line->First) ||
        State.NextToken->is(TT_TemplateCloser) ||
        (Current.is(tok::greater) && Current.is(TT_DictLiteral))))
     State.Stack.pop_back();
 
+  // Reevaluate whether ObjC message arguments fit into one line.
+  // If a receiver spans multiple lines, e.g.:
+  //   [[object block:^{
+  //     return 42;
+  //   }] a:42 b:42];
+  // BreakBeforeParameter is calculated based on an incorrect assumption
+  // (it is checked whether the whole expression fits into one line without
+  // considering a line break inside a message receiver).
+  // We check whether arguements fit after receiver scope closer (into the same
+  // line).
+  if (Current.MatchingParen && Current.MatchingParen->Previous) {
+    const FormatToken &CurrentScopeOpener = *Current.MatchingParen->Previous;
+    if (CurrentScopeOpener.is(TT_ObjCMethodExpr) &&
+        CurrentScopeOpener.MatchingParen) {
+      int NecessarySpaceInLine =
+          getLengthToMatchingParen(CurrentScopeOpener, State.Stack) +
+          CurrentScopeOpener.TotalLength - Current.TotalLength - 1;
+      if (State.Column + Current.ColumnWidth + NecessarySpaceInLine <=
+          Style.ColumnLimit)
+        State.Stack.back().BreakBeforeParameter = false;
+    }
+  }
+
   if (Current.is(tok::r_square)) {
     // If this ends the array subscript expr, reset the corresponding value.
     const FormatToken *NextNonComment = Current.getNextNonComment();
     if (NextNonComment && NextNonComment->isNot(tok::l_square))
       State.Stack.back().StartOfArraySubscripts = 0;
   }
 }
 

unittests/Format/FormatTestObjC.cpp

TEST_F(FormatTestObjC, FormatObjCMethodExpr) {
   verifyFormat("[[object block:^{\n"
                "  return 42;\n"
                "}] aaaaaaa:42 b:42];");
   // Arguments just over a column limit.
   verifyFormat("[[object block:^{\n"
                "  return 42;\n"
                "}] aaaaaaa:42\n"
                "        bb:42];");
-  // Non-corner case.
-  verifyFormat("[[object aaa:42\n"
-               "           b:42]\n"
-               "    cc:42 d:42];");
-  // Arguments just fit into one line.
-  verifyFormat("[[object aaa:42\n"
-               "           b:42]\n"
-               "    cccccc:42 d:42];");
-  // Arguments just over a column limit.
-  verifyFormat("[[object aaa:42\n"
+
+  // No line break before closing receiver's scope.
+  verifyFormat("[[obj a:42] a:42\n"
+               "            b:42];\n");
+  verifyFormat("[[obj a:42] a:42\n"
+               "            b:42\n"
+               "            c:42];\n");
+  verifyFormat("[[obj aaaaaa:42\n"
                "           b:42]\n"
-               "    cccccc:42\n"
-               "        dd:42];");
+               "    cc:42\n"
+               "     d:42];");
 
   Style.ColumnLimit = 70;
   verifyFormat(
       "void f() {\n"
       "  popup_wdow_.reset([[RenderWidgetPopupWindow alloc]\n"
       "      iniithContentRect:NSMakRet(origin_global.x, origin_global.y,\n"
       "                                 pos.width(), pos.height())\n"
       "                syeMask:NSBorderlessWindowMask\n"