[clang-format] Improve ObjC method expressions formatting

[jolesiak]

Summary

1. Fix counting parameters/arguments for ObjC.
2. Fix split priorities for ObjC methods.
3. Fix breaking after square bracket starting ObjC method expression.
4. Fix putting ObjC method arguments into one line when they fit.

Test plan:

make -j12 FormatTests && tools/clang/unittests/Format/FormatTests

[jolesiak]

• Add test

[benhamilton]

It's really hard to understand reviews which change 4 different things.

I hate to ask, but can you split this up into the 4 fixes?

[jolesiak]

In D48352#1145220, @benhamilton wrote:

It's really hard to understand reviews which change 4 different things.

I hate to ask, but can you split this up into the 4 fixes?

Sure, I'll split it tomorrow.

[jolesiak]

Split:
D48716
D48718
D48719
D48720

lib/Format/ContinuationIndenter.cpp

bool ContinuationIndenter::canBreak(const LineState &State) {
   }
 
   // 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) &&
       State.Stack.back().NoLineBreakInOperand)
     return false;
 
+  if (Previous.is(tok::l_square) && Previous.is(TT_ObjCMethodExpr))
+    return false;
+
   return !State.Stack.back().NoLineBreak;
 }
 
 bool ContinuationIndenter::mustBreak(const LineState &State) {
   const FormatToken &Current = *State.NextToken;
   const FormatToken &Previous = *Current.Previous;
   if (Current.MustBreakBefore || Current.is(TT_InlineASMColon))
     return true;
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;
   }
 }
 

lib/Format/FormatToken.h

struct FormatToken {
 
   /// If this is the first ObjC selector name in an ObjC method
   /// definition or call, this contains the length of the longest name.
   ///
   /// This being set to 0 means that the selectors should not be colon-aligned,
   /// e.g. because several of them are block-type.
   unsigned LongestObjCSelectorName = 0;
 
-  /// How many parts ObjC selector have (i.e. how many parameters method
-  /// has).
+  /// If this is the first ObjC selector name in an ObjC method
+  /// definition or call, this contains the number of parts that whole selector
+  /// consist of.
   unsigned ObjCSelectorNameParts = 0;
 
+  /// What is the 0-based index of the parameter/argument. For ObjC it is set
+  /// for the selector name.
+  /// For now calculated only for ObjC.
+  unsigned ParameterIndex = 0;
+
   /// Stores the number of required fake parentheses and the
   /// corresponding operator precedence.
   ///
   /// If multiple fake parentheses start at a token, this vector stores them in
   /// reverse order, i.e. inner fake parenthesis first.
   SmallVector<prec::Level, 4> FakeLParens;
   /// Insert this many fake ) after this token for correct indentation.
   unsigned FakeRParens = 0;

lib/Format/TokenAnnotator.cpp

while (CurrentToken) {
           // determineStarAmpUsage() thinks that '*' '[' is allocating an
           // array of pointers, but if '[' starts a selector then '*' is a
           // binary operator.
           if (Parent && Parent->is(TT_PointerOrReference))
             Parent->Type = TT_BinaryOperator;
         }
         Left->MatchingParen = CurrentToken;
         CurrentToken->MatchingParen = Left;
+        // FirstObjCSelectorName is set when a colon is found. This does
+        // not work, however, when method has no parameters.
+        // Here, we set FirstObjCSelectorName when the end of the expression is
+        // reached, in case it was not set already.
+        if (!Contexts.back().FirstObjCSelectorName) {
+            FormatToken* Previous = CurrentToken->getPreviousNonComment();
+            if (Previous && Previous->is(TT_SelectorName)) {
+              Previous->ObjCSelectorNameParts = 1;
+              Contexts.back().FirstObjCSelectorName = Previous;
+            }
+        } else {
+          Left->ParameterCount =
+              Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts;
+        }
+
         if (Contexts.back().FirstObjCSelectorName) {
           Contexts.back().FirstObjCSelectorName->LongestObjCSelectorName =
               Contexts.back().LongestObjCSelectorName;
-          Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts =
-              Left->ParameterCount;
           if (Left->BlockParameterCount > 1)
             Contexts.back().FirstObjCSelectorName->LongestObjCSelectorName = 0;
         }
         next();
         return true;
       }
       if (CurrentToken->isOneOf(tok::r_paren, tok::r_brace))
         return false;
       if (CurrentToken->is(tok::colon)) {
         if (IsCpp11AttributeSpecifier &&
             CurrentToken->endsSequence(tok::colon, tok::identifier,
                                        tok::kw_using)) {
           // Remember that this is a [[using ns: foo]] C++ attribute, so we
           // don't add a space before the colon (unlike other colons).
           CurrentToken->Type = TT_AttributeColon;
         } else if (Left->isOneOf(TT_ArraySubscriptLSquare,
                                  TT_DesignatedInitializerLSquare)) {
           Left->Type = TT_ObjCMethodExpr;
           StartsObjCMethodExpr = true;
-          // ParameterCount might have been set to 1 before expression was
-          // recognized as ObjCMethodExpr (as '1 + number of commas' formula is
-          // used for other expression types). Parameter counter has to be,
-          // therefore, reset to 0.
-          Left->ParameterCount = 0;
           Contexts.back().ColonIsObjCMethodExpr = true;
           if (Parent && Parent->is(tok::r_paren))
             // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
             Parent->Type = TT_CastRParen;
         }
         ColonFound = true;
       }
       if (CurrentToken->is(tok::comma) && Left->is(TT_ObjCMethodExpr) &&
if (CurrentToken) {
         if (!consumeToken())
           return false;
       }
     }
     return true;
   }
 
   void updateParameterCount(FormatToken *Left, FormatToken *Current) {
+    // For ObjC methods number of parameters is calculated differently as
+    // method declarations have a different structure (parameters are not inside
+    // parenthesis scope).
     if (Current->is(tok::l_brace) && Current->BlockKind == BK_Block)
       ++Left->BlockParameterCount;
-    if (Left->Type == TT_ObjCMethodExpr) {
-      if (Current->is(tok::colon))
-        ++Left->ParameterCount;
-    } else if (Current->is(tok::comma)) {
+    if (Current->is(tok::comma)) {
       ++Left->ParameterCount;
       if (!Left->Role)
         Left->Role.reset(new CommaSeparatedList(Style));
       Left->Role->CommaFound(Current);
     } else if (Left->ParameterCount == 0 && Current->isNot(tok::comment)) {
       Left->ParameterCount = 1;
     }
   }
case tok::colon:
             Contexts.back().LongestObjCSelectorName == 0) {
           Tok->Previous->Type = TT_SelectorName;
           if (!Contexts.back().FirstObjCSelectorName)
             Contexts.back().FirstObjCSelectorName = Tok->Previous;
           else if (Tok->Previous->ColumnWidth >
                    Contexts.back().LongestObjCSelectorName)
             Contexts.back().LongestObjCSelectorName =
                 Tok->Previous->ColumnWidth;
+
+          Tok->Previous->ParameterIndex =
+              Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts;
+          ++Contexts.back().FirstObjCSelectorName->ObjCSelectorNameParts;
         }
       } else if (Contexts.back().ColonIsForRangeExpr) {
         Tok->Type = TT_RangeBasedForLoopColon;
       } else if (CurrentToken && CurrentToken->is(tok::numeric_constant)) {
         Tok->Type = TT_BitFieldColon;
       } else if (Contexts.size() == 1 &&
                  !Line.First->isOneOf(tok::kw_enum, tok::kw_case)) {
         if (Tok->getPreviousNonComment()->isOneOf(tok::r_paren,
if (Current->MustBreakBefore || Prev->Children.size() > 1 ||
       Current->TotalLength = Prev->TotalLength + Style.ColumnLimit;
     else
       Current->TotalLength = Prev->TotalLength + Current->ColumnWidth +
                              ChildSize + Current->SpacesRequiredBefore;
 
     if (Current->is(TT_CtorInitializerColon))
       InFunctionDecl = false;
 
+    // Reduce penalty for aligning ObjC method arguments using the colon
+    // alignment as this is a canonical way (still prefer fitting everything
+    // into one line if possible) and trying to fit a whole epression into one
+    // line should not force other line breaks (e.g. when ObjC method
+    // expression is a part of other expression).
+    if (Style.Language == FormatStyle::LK_ObjC &&
+        Current->is(TT_SelectorName) && Current->ParameterIndex > 0) {
+      if (Current->ParameterIndex == 1) {
+        Current->SplitPenalty = 5 * Current->BindingStrength +
+                                splitPenalty(Line, *Current, InFunctionDecl);
+      } else {
+        Current->SplitPenalty = splitPenalty(Line, *Current, InFunctionDecl);
+      }
+    } else {
       // FIXME: Only calculate this if CanBreakBefore is true once static
       // initializers etc. are sorted out.
       // FIXME: Move magic numbers to a better place.
       Current->SplitPenalty = 20 * Current->BindingStrength +
                               splitPenalty(Line, *Current, InFunctionDecl);
+    }
 
     Current = Current->Next;
   }
 
   calculateUnbreakableTailLengths(Line);
   unsigned IndentLevel = Line.Level;
   for (Current = Line.First; Current != nullptr; Current = Current->Next) {
     if (Current->Role)

unittests/Format/FormatTestObjC.cpp

TEST_F(FormatTestObjC, FormatObjCMethodExpr) {
   verifyFormat("arr[[self indexForFoo:a]];");
   verifyFormat("throw [self errorFor:a];");
   verifyFormat("@throw [self errorFor:a];");
 
   verifyFormat("[(id)foo bar:(id)baz quux:(id)snorf];");
   verifyFormat("[(id)foo bar:(id) ? baz : quux];");
   verifyFormat("4 > 4 ? (id)a : (id)baz;");
 
+  unsigned PreviousColumnLimit = Style.ColumnLimit;
+  Style.ColumnLimit = 50;
+  // Instead of:
+  // bool a =
+  //     ([object a:42] == 0 || [object a:42
+  //                                    b:42] == 0);
+  verifyFormat("bool a = ([object a:42] == 0 ||\n"
+               "          [object a:42 b:42] == 0);");
+  Style.ColumnLimit = PreviousColumnLimit;
+  verifyFormat("bool a = ([aaaaaaaa aaaaa] == aaaaaaaaaaaaaaaaa ||\n"
+               "          [aaaaaaaa aaaaa] == aaaaaaaaaaaaaaaaaaaa);");
+
   // This tests that the formatter doesn't break after "backing" but before ":",
   // which would be at 80 columns.
   verifyFormat(
       "void f() {\n"
       "  if ((self = [super initWithContentRect:contentRect\n"
       "                               styleMask:styleMask ?: otherMask\n"
       "                                 backing:NSBackingStoreBuffered\n"
       "                                   defer:YES]))");
TEST_F(FormatTestObjC, FormatObjCMethodExpr) {
 
   // Variadic parameters.
   verifyFormat(
       "NSArray *myStrings = [NSArray stringarray:@\"a\", @\"b\", nil];");
   verifyFormat(
       "[self aaaaaaaaaaaaa:aaaaaaaaaaaaaaa, aaaaaaaaaaaaaaa, aaaaaaaaaaaaaaa,\n"
       "                    aaaaaaaaaaaaaaa, aaaaaaaaaaaaaaa, aaaaaaaaaaaaaaa,\n"
       "                    aaaaaaaaaaaaaaa, aaaaaaaaaaaaaaa];");
+
   verifyFormat("[self // break\n"
                "      a:a\n"
                "    aaa:aaa];");
-  verifyFormat("bool a = ([aaaaaaaa aaaaa] == aaaaaaaaaaaaaaaaa ||\n"
-               "          [aaaaaaaa aaaaa] == aaaaaaaaaaaaaaaaaaaa);");
 
   // Formats pair-parameters.
   verifyFormat("[I drawRectOn:surface ofSize:aa:bbb atOrigin:cc:dd];");
   verifyFormat("[I drawRectOn:surface //\n"
                "       ofSize:aa:bbb\n"
                "     atOrigin:cc:dd];");
 
   // Inline block as a first argument.
verifyFormat("[object\n"
                "    secondBlock:^{\n"
                "      a = 42;\n"
                "    }];");
 
   // Space between cast rparen and selector name component.
   verifyFormat("[((Foo *)foo) bar];");
   verifyFormat("[((Foo *)foo) bar:1 blech:2];");
 
+  Style.ColumnLimit = 20;
+  verifyFormat("aaaaa = [a aa:aa\n"
+               "           aa:aa];");
+
+  // Message receiver taking multiple lines.
+  // Non-corner case.
+  verifyFormat("[[object block:^{\n"
+               "  return 42;\n"
+               "}] a:42 b:42];");
+  // Arguments just fit into one line.
+  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];");
+  // Arguments just fit into one line.
+  Style.ColumnLimit = 23;
+  verifyFormat("[[obj a:42\n"
+               "      b:42\n"
+               "      c:42\n"
+               "      d:42] e:42 f:42];");
+
+  // Arguments do not fit into one line with a receiver.
+  Style.ColumnLimit = 20;
+  verifyFormat("[[obj a:42] a:42\n"
+               "            b:42];");
+  verifyFormat("[[obj a:42] a:42\n"
+               "            b:42\n"
+               "            c:42];");
+  verifyFormat("[[obj aaaaaa:42\n"
+               "           b:42]\n"
+               "    cc:42\n"
+               "     d:42];");
+
+  // Avoid breaking receiver expression.
+  Style.ColumnLimit = 30;
+  verifyFormat("fooooooo =\n"
+               "    [[obj fooo] aaa:42\n"
+               "                aaa:42];");
+  verifyFormat("[[[obj foo] bar] aa:42\n"
+               "                 bb:42\n"
+               "                 cc: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"
       "                  bking:NSBackingStoreBuffered\n"