Update block formatting to behave like Xcode

[KingOfBrian]

Summary

clang-format follows google block formatting style, which is very rare in the iOS world. I added a bool to make the block formatting behave more like Xcode.

[KingOfBrian]

Forgot to add cfe-commits.

[djasper]

include/clang/Format/Format.h

225

I this style documented somewhere? There are many corner cases that you aren't explicitly testing. E.g. what if there are non-block selectors in between? What if the selectors before the first block do not fit on one line?

ObjCXcodeBlockFormat is not a good name. It would be good to have something that actually describes the behavior. Like this it is not really discoverable. And this does not really change the behavior of blocks in general, but the behavior of blocks in method exprs, IIUC.

Also, the comment needs to be quite a bit more elaborate.

lib/Format/ContinuationIndenter.cpp

419

Either remove the comment again or make it more elaborate. As is, it seems more confusing.

857

!Style.ObjCXcodeBlockFormat

unittests/Format/FormatTest.cpp

9803

Again, is there some documentation of what "Apple style" is?

9804

It seems quite weird that this style would only affect ColumnLimit 0. If you'd want to add it, we need to make it work for styles with column limits, too.

9818–9821

Whether Xcode does this or not, my personal opinion is that this is quite terrible. The alignment of a few colons that are many lines a part is next to useless where as this looks very messy, especially if the difference between block selector names is more than 1 character.

9861

What is this testing that's not already tested above?

[KingOfBrian]

include/clang/Format/Format.h

225

I haven't been able to find it documented anywhere, I looked around for a while. But it's been the same formatting in Xcode since blocks were introduced. I'll add some more tests for long selector-parts and see how it behaves.

I'm a bit stumped on the naming for behavior so I opted to name it based on the origin -- similar in approach to allman vs soustroup. I'm not sure there's a name that is discoverable, since the difference between this and the current google style is pretty nuanced. Would changing it to a style enumeration be more acceptable?

I added the prefix ObjC to imply that this only affects the behavior of blocks in method exprs, obviously that was not clear enough.

Also, reading the comment again, it's wrong, sorry about that. The comment explains the inverse of one of the two behavior changes - I'll take another stab at it, but without a good name for the flag, I'm a bit stumped. Let me think about it.

unittests/Format/FormatTest.cpp

9804

Right, let me look into that.

9818–9821

I think Apple developers may have a degree of Stockholm syndrome here, but it is a style that people want at this point. I dislike it personally too, and usually don't do line breaks so it left aligns. If I do, I keep the blocks very short, or break them out into a variable.

Multi-block obj-c can get ugly no matter what way you cut it. For instance, I thought the multi-block google style was so terrible that it was a bug!

9861

Good point, I'll update it.

[djasper]

include/clang/Format/Format.h

225

The fundamental question is, whether we want clang-format to support this style. I don't think it is useful to support it, just because it is what some other tool creates. That would mean that clang-format would have to implement all the flaws of other indentation tools, which we certainly do not want to do.

That being said, if this is widely used and ideally documented somewhere, it might be worth adding. But we certainly need to define it precisely in the comment (especially is there is no other documentation) and we need to make it work with a non-zero column limit.

[KingOfBrian]

include/clang/Format/Format.h

225

After thinking about this some, this does 2 separate style modifications, and if the goal is a discoverable config format, I these two things could be reflected in 2 different configuration options.

The first change is to not do left compression suggested by the google style. Also note that in the google style it states 'inlined blocks may have their segments left-aligned ... This helps when invocations contain multiple inlined blocks'. All this change does is provide the option to not perform this special case formatting (which is allowed by the style guide). What do you think about this being reflected via a 'ObjcLeftAlignMultipleBlocks` option?

The second change is to not force line breaks on multiple-argument ObjCMethodExpr. This only works with zero column limit, I have to hunt down the other code path for a column limit.

Your argument of not re-creating bad styles makes a lot of sense to me. I want to make sure that we agree that the style in the test on line 9855 is worth re-creating. This format is used a lot, although it's not explicitly documented anywhere that I can tell. The other styles are incidental to getting that working, although they have the bonus of matching the current Xcode tool-chain. I'm not going to be disappointed if the other formats don't work, but it's going to be very hard to get adoption in the iOS world if the styles I mentioned above aren't re-creatable. (also -- going to comment on them explicitely incase numbers change)

unittests/Format/FormatTest.cpp

9860

The style above is the block format I'm looking to re-create.

[KingOfBrian]

Here's an update that does two things

1. Splits the behavior changes into 2 bools, since they were controlling distinct formatting behaviors
2. Adds support for non-zero column width for ObjCAvoidLineBreaksForInlineBlocks. Currently there's potentially broken behavior, where the decision to split an objective-c line is determined by the length of the opening [ to the close ]. In the case of in-line blocks, that calculation is deceptive as it includes blocks, which break the line. I added getLengthToLineTerminator that matches the closing ] or the first token with children lines. I only changed this behavior when ObjCAvoidLineBreaksForInlineBlocks is enabled, but it could be argued that this should be default behavior. I opted for compatibility unless you think otherwise.

[tonyarnold]

This looks great, Brian!

Why is formatting within blocks/closures different to what I define for code that's not inside a block?
It really threw me when code within blocks started being wrapped when I'd already set a ColumnLimit of 0.

I'm quite keen to see this patch merged, if only to get familiar formatting back to my blocks in Xcode projects.

[tonyarnold]

@djasper so was the decision here to not merge these options?

I think it's fair that you require a documented style guide somewhere (which Apple don't currently publish), however it would be good to at least have generic options in clang-format that allow us to replicate the unpublished style, even without official support in the tool.

What do we need to do to get support for options like ObjCAvoidLineBreaksForInlineBlocks and ObjCLeftAlignMultipleBlocks into clang-format? There are potentially one or two more options around dictionary/array literal alignment/newlines that I'd like to add — but I'm not going to spend any time drawing up PRs if there's no way they'll ever be merged.

include/clang/Format/Format.h

   /// on a single line.
   ShortFunctionStyle AllowShortFunctionsOnASingleLine;
 
+  /// \brief Compress the selector parts to align left when more than two inline
+  /// blocks are specified. Default is true
+  bool ObjCLeftAlignMultipleBlocks;
+
+  /// \brief Avoid inserting line breaks when calculating the length of lines
+  /// with inline Objective-C blocks.
+  bool ObjCAvoidLineBreaksForInlineBlocks;
+
   /// \brief Add a space after \c @property in Objective-C, i.e. use
   /// <tt>\@property (readonly)</tt> instead of <tt>\@property(readonly)</tt>.
   bool ObjCSpaceAfterProperty;

lib/Format/ContinuationIndenter.cpp

   return End->TotalLength - Tok.TotalLength + 1;
 }
 
+// Returns the length of everything up to the first possible line break after
+// the ), ], } or > matching \c Tok, or the first token with children.
+static unsigned getLengthToLineTerminator(const FormatToken &Tok) {
+  if (!Tok.MatchingParen)
+    return 0;
+  FormatToken *End = Tok.MatchingParen;
+  FormatToken *Cursor = Tok.Next;
+  while (Cursor->Children.size() == 0 && Cursor != End) {
+    Cursor = Cursor->Next;
+  }
+  while (Cursor->Next && !Cursor->Next->CanBreakBefore) {
+    Cursor = Cursor->Next;
+  }
+  return Cursor->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) {

       if (Style.ColumnLimit) {
         // If this '[' opens an ObjC call, determine whether all parameters fit
         // into one line and put one per line if they don't.
-        if (getLengthToMatchingParen(Current) + State.Column >
-            getColumnLimit(State))
+        unsigned length = (Style.ObjCAvoidLineBreaksForInlineBlocks ?
+                               getLengthToLineTerminator(Current) :
+                               getLengthToMatchingParen(Current));
+        if (length + State.Column > getColumnLimit(State))
           BreakBeforeParameter = true;
       } else {
         // For ColumnLimit = 0, we have to figure out whether there is or has to

              Tok && Tok != Current.MatchingParen; Tok = Tok->Next) {
           if (Tok->MustBreakBefore || 
               (Tok->CanBreakBefore && Tok->NewlinesBefore > 0)) {
-            BreakBeforeParameter = true;
+            BreakBeforeParameter = !Style.ObjCAvoidLineBreaksForInlineBlocks;
             break;
           }
         }

lib/Format/Format.cpp

                    Style.KeepEmptyLinesAtTheStartOfBlocks);
     IO.mapOptional("NamespaceIndentation", Style.NamespaceIndentation);
     IO.mapOptional("ObjCBlockIndentWidth", Style.ObjCBlockIndentWidth);
+    IO.mapOptional("ObjCAvoidLineBreaksForInlineBlocks", Style.ObjCAvoidLineBreaksForInlineBlocks);
+    IO.mapOptional("ObjCLeftAlignMultipleBlocks", Style.ObjCLeftAlignMultipleBlocks);
     IO.mapOptional("ObjCSpaceAfterProperty", Style.ObjCSpaceAfterProperty);
     IO.mapOptional("ObjCSpaceBeforeProtocolList",
                    Style.ObjCSpaceBeforeProtocolList);

   LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true;
   LLVMStyle.NamespaceIndentation = FormatStyle::NI_None;
   LLVMStyle.ObjCBlockIndentWidth = 2;
+  LLVMStyle.ObjCLeftAlignMultipleBlocks = true;
+  LLVMStyle.ObjCAvoidLineBreaksForInlineBlocks = false;
   LLVMStyle.ObjCSpaceAfterProperty = false;
   LLVMStyle.ObjCSpaceBeforeProtocolList = true;
   LLVMStyle.PointerAlignment = FormatStyle::PAS_Right;

lib/Format/TokenAnnotator.cpp

         if (Contexts.back().FirstObjCSelectorName) {
           Contexts.back().FirstObjCSelectorName->LongestObjCSelectorName =
               Contexts.back().LongestObjCSelectorName;
-          if (Left->BlockParameterCount > 1)
+          // Compress blocks if there are multiple block arguments
+          if (Left->BlockParameterCount > 1 &&
+              Style.ObjCLeftAlignMultipleBlocks)
             Contexts.back().FirstObjCSelectorName->LongestObjCSelectorName = 0;
         }
         next();

unittests/Format/FormatTest.cpp

                "    [self onOperationDone];\n"
                "}];",
                FourIndent);
+
+  FormatStyle AvoidBreaks = getLLVMStyle();
+  AvoidBreaks.ObjCAvoidLineBreaksForInlineBlocks = true;
+  verifyFormat("[UIView animateWithDuration:0 animations:^{ // This does not change behavior\n"
+               "  view.center = CGPointZero;\n"
+               "} completion:^(BOOL finished) {\n"
+               "  NSLog(complete);\n"
+               "}];",
+               AvoidBreaks);
+  verifyFormat("[UIView animateWithDuration:0 animations:^{\n"
+               "  view.center = CGPointZero;\n"
+               "} completion:^(BOOL finished) {\n"
+               "  NSLog(complete);\n"
+               "}];",
+               AvoidBreaks);
+  verifyFormat("[UIView animateWithDuration:0 // Break\n"
+               "    animations:^{\n"
+               "      view.center = CGPointZero;\n"
+               "    }\n"
+               "    completion:^(BOOL finished) {\n"
+               "      NSLog(complete);\n"
+               "    }];",
+               AvoidBreaks);
+}
+
+TEST_F(FormatTest, FormatsBlocksLeftAligned) {
+  FormatStyle DisableLeftAlignedStyle = getLLVMStyle();
+  DisableLeftAlignedStyle.ColumnLimit = 0;
+  DisableLeftAlignedStyle.IndentWidth = 4;
+  DisableLeftAlignedStyle.ObjCBlockIndentWidth = 4;
+  DisableLeftAlignedStyle.ObjCLeftAlignMultipleBlocks = false;
+
+  verifyFormat("[myObject doSomethingWith:arg1 // Comment\n"
+               "               firstBlock:^(Foo *a) {\n"
+               "                   // ...\n"
+               "                   int i;\n"
+               "               }\n"
+               "              secondBlock:^(Bar *b) {\n"
+               "                  // ...\n"
+               "                  int i;\n"
+               "              }\n"
+               "               thirdBlock:^Foo(Bar *b) {\n"
+               "                   // ...\n"
+               "                   int i;\n"
+               "               }];",
+               DisableLeftAlignedStyle);
+  verifyFormat("[UIView animateWithDuration:0\n"
+               "                 animations:^{\n"
+               "                     view.center = CGPointZero;\n"
+               "                 }\n"
+               "                 completion:^(BOOL finished) {\n"
+               "                     NSLog(complete);\n"
+               "                 }];",
+               DisableLeftAlignedStyle);
+
 }
 
 TEST_F(FormatTest, FormatsBlocksWithZeroColumnWidth) {

             "  int i;\n"
             "};",
             format("void   (^largeBlock)(void) = ^{ int   i; };", ZeroColumn));
+
+  ZeroColumn.ObjCAvoidLineBreaksForInlineBlocks = true;
+  verifyFormat("[UIView animateWithDuration:0 animations:^{ // This does not change behavior\n"
+               "  view.center = CGPointZero;\n"
+               "} completion:^(BOOL finished) {\n"
+               "  NSLog(complete);\n"
+               "}];",
+               ZeroColumn);
+  verifyFormat("[UIView animateWithDuration:0 animations:^{\n"
+               "  view.center = CGPointZero;\n"
+               "} completion:^(BOOL finished) {\n"
+               "  NSLog(complete);\n"
+               "}];",
+               ZeroColumn);
+
+  verifyFormat("[UIView animateWithDuration:0 // Break\n"
+               "    animations:^{\n"
+               "      view.center = CGPointZero;\n"
+               "    }\n"
+               "    completion:^(BOOL finished) {\n"
+               "      NSLog(complete);\n"
+               "    }];",
+               ZeroColumn);
 }
 
 TEST_F(FormatTest, SupportsCRLF) {