[clang-format] Proposal for changes to Objective-C block formatting

[ksuther]

Summary

Changes to clang-format's Objective-C block formatting over the past year have made clang-format's output deviate from what is expected (in my opinion).

There are three changes in particular:

• Method calls with multiple block parameters has each block indented further in
• Nested blocks get indented one level in
• Method calls that end with a block parameter always get forced to a new line and indented

The end of this message has examples of formatting with and without these changes.

This patch undoes one revision which causes methods with multiple block parameters to indent for each parameter (r236598) and adds two new options.
AllowNewlineBeforeBlockParameter makes the change in r235580 optional.
IndentNestedBlocks makes the change in r234304 optional.

Some relevant Bugzilla issues:
https://llvm.org/bugs/show_bug.cgi?id=23585
https://llvm.org/bugs/show_bug.cgi?id=23317

This patch came out of a discussion here https://github.com/square/spacecommander/issues/33, where we're using a custom version of clang-format with these options added. Based on the Bugzilla issues, it seems that we're not alone.

Thanks!

---

Current trunk:

bin/clang-format -style="{BasedOnStyle: WebKit, ColumnLimit: 0, IndentWidth: 4}" ~/clang-format.m

- (void)test
{
    [self block:^(void) {
        doStuff();
    }
        completionHandler:^(void) {
            doStuff();

            [self block:^(void) {
                doStuff();
            }
                completionHandler:^(void) {
                    doStuff();
                }];
        }];

    [[SessionService sharedService] loadWindow:aWindow
                               completionBlock:^(SessionWindow* window) {
                                   if (window) {
                                       [self doStuff];
                                   }
                               }];
}

With this patch applied:

bin/clang-format -style="{BasedOnStyle: WebKit, ColumnLimit: 0, IndentWidth: 4, IndentNestedBlocks: false, AllowNewlineBeforeBlockParameter: false}"~/clang-format.m

- (void)test
{
    [self block:^(void) {
        doStuff();
    } completionHandler:^(void) {
        doStuff();

        [self block:^(void) {
            doStuff();
        } completionHandler:^(void) {
            doStuff();
        }];
    }];

    [[SessionService sharedService] loadWindow:aWindow completionBlock:^(SessionWindow* window) {
        if (window) {
            [self doStuff];
        }
    }];
}

[tonyarnold]

Aside from the lack of tests, and AllowNewlineBeforeBlockParameter being outside of BraceWrapping, this is good! Much closer to how Apple's documentation and Xcode format blocks! 👍

include/clang/Format/Format.h

421

Perhaps this would be better placed within the custom BraceWrapping options? Something simpler like BeforeBlockParameter.

[tonyarnold]

I did find one bug though — if there's any parameters in between the two blocks, they'll end up forcing a newline:

@implementation SomeClass

- (void)test
{
        [sharedController passingTest:^BOOL(id application) {
            return application.thing;
        } withTimeout:kTimeout completionHandler:^(BOOL success, NSError *error) {
            if (success == NO && error != nil)
            {
              // Do the thing
            }
        }];
}

@end

Wrongly becomes:

@implementation SomeClass

- (void)test
{
    [sharedController passingTest:^BOOL(id application) {
        return application.thing;
    } withTimeout:kTimeout
        completionHandler:^(BOOL success, NSError *error) {
            if (success == NO && error != nil)
            {
                // Do the thing
            }
        }];
}

@end

[ksuther]

Thanks for the comments. I've made some changes that eliminates reverting r236598 and instead makes the behavior part of IndentNestedBlocks. That allows the Google Obj-C code style (https://google.github.io/styleguide/objcguide.xml#Blocks) to still work by default. The issue with a parameter between block parameters has also been fixed (as part of AllowNewlineBeforeBlockParameter).

Apologies if I'm missing something obvious, but I don't see how AllowNewlineBeforeBlockParameter belongs in BraceWrapping. Blocks aren't the same as braces, in the original example, this option would be controlling the newline insertion for completionBlock:^(SessionWindow* window) { and not just the opening brace.

[tonyarnold]

Blocks aren't the same as braces

Yeah, good point. I guess I was just seeing the brace in the block, but that's the wrong way to think about it.

[dcasadevall]

Was this lost in the woods? I believe this is a valid change when working in Objective-C

[ksuther]

It seems to have been. I've been using a modified version of clang-format with this change applied (as well as http://reviews.llvm.org/D17922) since I submitted this.

[Myrronth]

Do you mind sharing your modified version? The fork in the discussion you mentioned (https://github.com/square/spacecommander/issues/33) seems to be out-of-date or at least does not have the AllowNewlineBeforeBlockParameter setting.

[ksuther]

My fork of spacecommander has a version of clang-format with the option. It's a couple of years old at this point, but it has been running without any issues.

[Myrronth]

Thanks, I’ll give it a try.

include/clang/Format/Format.h

   /// type.
   bool IndentWrappedFunctionNames;
 
+  /// \brief If true, indent nested blocks to the start of the nested function call
+  bool IndentNestedBlocks;
+
+  /// \brief If true, allow newlines before block parameters when ColumnLimit is 0.
+  bool AllowNewlineBeforeBlockParameter;
+
   /// \brief If true, empty lines at the start of blocks are kept.
   bool KeepEmptyLinesAtTheStartOfBlocks;
 

            IndentCaseLabels == R.IndentCaseLabels &&
            IndentWidth == R.IndentWidth && Language == R.Language &&
            IndentWrappedFunctionNames == R.IndentWrappedFunctionNames &&
+           IndentNestedBlocks == R.IndentNestedBlocks &&
+           AllowNewlineBeforeBlockParameter == R.AllowNewlineBeforeBlockParameter &&
            KeepEmptyLinesAtTheStartOfBlocks ==
                R.KeepEmptyLinesAtTheStartOfBlocks &&
            MacroBlockBegin == R.MacroBlockBegin &&

lib/Format/ContinuationIndenter.cpp

       ((Style.AllowShortFunctionsOnASingleLine != FormatStyle::SFS_All) ||
        Style.BreakConstructorInitializersBeforeComma || Style.ColumnLimit != 0))
     return true;
-  if (Current.is(TT_SelectorName) && State.Stack.back().ObjCSelectorNameFound &&
-      State.Stack.back().BreakBeforeParameter)
-    return true;
 
   unsigned NewLineColumn = getNewLineColumn(State);
   if (Current.isMemberAccess() && Style.ColumnLimit != 0 &&

       State.Stack.back().FirstLessLess == 0)
     return true;
 
+  if (Current.is(TT_SelectorName) && State.Stack.back().ObjCSelectorNameFound &&
+      State.Stack.back().BreakBeforeParameter)
+    return true;
   if (Current.NestingLevel == 0 && !Current.isTrailingComment()) {
     // Always break after "template <...>" and leading annotations. This is only
     // for cases where the entire line does not fit on a single line as a

   unsigned LastSpace = State.Stack.back().LastSpace;
   bool AvoidBinPacking;
   bool BreakBeforeParameter = false;
-  unsigned NestedBlockIndent = std::max(State.Stack.back().StartOfFunctionCall,
-                                        State.Stack.back().NestedBlockIndent);
+  unsigned NestedBlockIndent;
+  if (Style.IndentNestedBlocks) {
+    NestedBlockIndent = std::max(State.Stack.back().StartOfFunctionCall,
+                                 State.Stack.back().NestedBlockIndent);
+  } else {
+    NestedBlockIndent = State.Stack.back().NestedBlockIndent;
+  }
   if (Current.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare)) {
     if (Current.opensBlockOrBlockTypeList(Style)) {
       NewIndent = State.Stack.back().NestedBlockIndent + Style.IndentWidth;

         if (getLengthToMatchingParen(Current) + State.Column >
             getColumnLimit(State))
           BreakBeforeParameter = true;
-      } else {
+      } else if (Style.AllowNewlineBeforeBlockParameter) {
         // For ColumnLimit = 0, we have to figure out whether there is or has to
         // be a line break within this call.
         for (const FormatToken *Tok = &Current;

lib/Format/Format.cpp

     IO.mapOptional("IndentWidth", Style.IndentWidth);
     IO.mapOptional("IndentWrappedFunctionNames",
                    Style.IndentWrappedFunctionNames);
+    IO.mapOptional("IndentNestedBlocks",
+                   Style.IndentNestedBlocks);
+    IO.mapOptional("AllowNewlineBeforeBlockParameter",
+                   Style.AllowNewlineBeforeBlockParameter);
     IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks",
                    Style.KeepEmptyLinesAtTheStartOfBlocks);
     IO.mapOptional("MacroBlockBegin", Style.MacroBlockBegin);

                                  {".*", 1}};
   LLVMStyle.IndentCaseLabels = false;
   LLVMStyle.IndentWrappedFunctionNames = false;
+  LLVMStyle.IndentNestedBlocks = true;
+  LLVMStyle.AllowNewlineBeforeBlockParameter = true;
   LLVMStyle.IndentWidth = 2;
   LLVMStyle.TabWidth = 8;
   LLVMStyle.MaxEmptyLinesToKeep = 1;

unittests/Format/FormatTest.cpp

       "                 aaaaaaaaaaaaaaa | aaaaaaaaaaaaaaa | aaaaaaaaaaaaaaa |\n"
       "                 aaaaaaaaaaaaaaa | aaaaaaaaaaaaaaa];");
 
-  // FIXME: This violates the column limit.
-  verifyFormat(
-      "[aaaaaaaaaaaaaaaaaaaaaaaaa\n"
-      "    aaaaaaaaaaaaaaaaa:aaaaaaaa\n"
-      "                  aaa:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa];",
-      getLLVMStyleWithColumns(60));
-
   // Variadic parameters.
   verifyFormat(
       "NSArray *myStrings = [NSArray stringarray:@\"a\", @\"b\", nil];");