clang-format: Builder closing paren must be a call

[ono]

Summary

We should somehow ensure that builder type call segment is preceded by a
closing parenthesis that comes from a call not a type cast or anything else.

Before:

(aaaa)
    .aaaaaaaaaaaaaaa()
    .aaaaaaaaaaaaaaa()
    .aaaaaaaaaaaaaaa()
    .aaaaaaaaaaaaaaa()
    .aaaaaaaaaaaaaaa();

After:

(aaaa).aaaaaaaaaaaaaaa()
    .aaaaaaaaaaaaaaa()
    .aaaaaaaaaaaaaaa()
    .aaaaaaaaaaaaaaa()
    .aaaaaaaaaaaaaaa();

Some more real life example:

Before:

- (bool)isALabelWithText:(NSString *)anyCaseText {
  return [((UILabel *)self)
              .someLabelText isEqualToString:[anyCaseText toVeryUpperCase]];
}

After:

- (bool)isALabelWithText:(NSString *)anyCaseText {
  return [((UILabel *)self).someLabelText
      isEqualToString:[anyCaseText toVeryUpperCase]];
}

[djasper]

I am not sure this is the right direction. We have actually considered going the other way, i.e. do builder-type wrapping even after the first element, i.e.:

aaaaaaaa
    .aaaaaa()
    .bbbbbb();

There were some edge cases where I got stuck a bit, so I haven't proceeded. But engraining this more seems wrong. The given example seems wrong for other reasons, in particular, we should force wrapping the parameters/selectors if the callee spans multiple lines.

[ono]

In D9469#165347, @djasper wrote:

I am not sure this is the right direction. We have actually considered going the other way, i.e. do builder-type wrapping even after the first element, i.e.:

aaaaaaaa
    .aaaaaa()
    .bbbbbb();

There were some edge cases where I got stuck a bit, so I haven't proceeded. But engraining this more seems wrong. The given example seems wrong for other reasons, in particular, we should force wrapping the parameters/selectors if the callee spans multiple lines.

I may agree with that if it is constrained to something, like either a break if preceded with a real call or it is followed by real call. Current implementation expects (looking at the tests) that element preceding dot (and the break) if the call.

Now you give a sample that actually the stuff following a break and dot is a call. Either or is okay, but not:

aaaaaaaaa
    .bbbbbbbbb
    .cccccccccc

This is just accessing fields, not builder call.

lib/Format/ContinuationIndenter.cpp

while (End->Next && !End->Next->CanBreakBefore) {
     End = End->Next;
   }
   return End->TotalLength - Tok.TotalLength + 1;
 }
 
 // Returns \c true if \c Tok is the "." or "->" of a call and starts the next
 // segment of a builder type call.
 static bool startsSegmentOfBuilderTypeCall(const FormatToken &Tok) {
-  return Tok.isMemberAccess() && Tok.Previous && Tok.Previous->closesScope();
+  if (Tok.isMemberAccess() && Tok.Previous && Tok.Previous->closesScope()) {
+    // Ensure that closing scope was function or method call, instantiation
+    // or indexed access, not eg. a type cast or Objective-C call.
+    const FormatToken *LeftParen = Tok.Previous->MatchingParen;
+    if (!LeftParen)
+      return false;
+    // We expect previous token is an identifier ...
+    const FormatToken *ExpectedIdentifier = LeftParen->getPreviousNonComment();
+    if (!ExpectedIdentifier)
+      return false;
+    // or template closing ...
+    if (ExpectedIdentifier->is(TT_TemplateCloser)) {
+      const FormatToken *TemplateOpener = ExpectedIdentifier->MatchingParen;
+      if (!TemplateOpener)
+        return false;
+      const FormatToken *ExpectedIdentifier = TemplateOpener->getPreviousNonComment();
+      // ... of template preceeded by identifier.
+      return ExpectedIdentifier && ExpectedIdentifier->is(tok::identifier);
+    }
+    return ExpectedIdentifier->is(tok::identifier);
+  }
+  return false;
 }
 
 // Returns \c true if \c Current starts a new parameter.
 static bool startsNextParameter(const FormatToken &Current,
                                 const FormatStyle &Style) {
   const FormatToken &Previous = *Current.Previous;
   if (Current.is(TT_CtorInitializerComma) &&
       Style.BreakConstructorInitializersBeforeComma)

unittests/Format/FormatTest.cpp

verifyFormat("aaaaaaaaaaaaaaa.aaaaaaaaaaaaaaa()\n"
                "    .aaaaaaaaaaaaaaa()\n"
                "    .aaaaaaaaaaaaaaa();");
   verifyFormat("aaaaaaaaaaaaaaa.aaaaaaaaaaaaaaa()\n"
                "    .aaaaaaaaaaaaaaa.aaaaaaaaaaaaaaa()\n"
                "    .aaaaaaaaaaaaaaa();");
   verifyFormat("aaaaaaaaaaaaa->aaaaaaaaaaaaaaaaaaaaaaaa()\n"
                "    ->aaaaaaaaaaaaaae(0)\n"
                "    ->aaaaaaaaaaaaaaa();");
+  verifyFormat("(aaaa).aaaaaaaaaaaaaaa()\n"
+               "    .aaaaaaaaaaaaaaa()\n"
+               "    .aaaaaaaaaaaaaaa()\n"
+               "    .aaaaaaaaaaaaaaa()\n"
+               "    .aaaaaaaaaaaaaaa();");
+  verifyFormat("aaaa()\n"
+               "    .aaaaaaaaaaaaaaa()\n"
+               "    .aaaaaaaaaaaaaaa()\n"
+               "    .aaaaaaaaaaaaaaa()\n"
+               "    .aaaaaaaaaaaaaaa()\n"
+               "    .aaaaaaaaaaaaaaa();");
+  verifyFormat("[aaaaa aaaaaaaaaaaaaaa].aaaaaaaaaaaaaaa()\n"
+               "    .aaaaaaaaaaaaaaa()\n"
+               "    .aaaaaaaaaaaaaaa()\n"
+               "    .aaaaaaaaaaaaaaa();");
 
   // Don't linewrap after very short segments.
   verifyFormat("a().aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa()\n"
                "    .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa()\n"
                "    .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa();");
   verifyFormat("aa().aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa()\n"
                "    .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa()\n"
                "    .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa();");