[clang-format] Add option for not breaking line before ObjC params

[ghvg1313]

Summary

From clang-format version 3.7.0 and up, , there is no way to keep following format of ObjectiveC block:

- (void)_aMethod
{
    [self.test1 t:self w:self callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {
        u = c;
    }]
}

Regardless of the change in .clang-format configuration file, all parameters will be lined up so that colons will be on the same column, like following:

- (void)_aMethod
{
    [self.test1 t:self
                w:self
         callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {
             u = c;
         }]
}

Considering with ObjectiveC, the first code style is cleaner & more readable for some people, I've added a config option: ObjCDontBreakBeforeNestedBlockParam (boolean) so that if it is enable, the first code style will be favored.

[MyDeveloperDay]

Could you update the ClangStyleOption.rst (using the tool in docs/tools)

and as you are adding a new option would you add a line or two in the docs/ReleaseNotes.rst in the clang-format section

[ghvg1313]

• Update documents

[MyDeveloperDay]

I can tell from https://zed0.co.uk/clang-format-configurator/ that it's not possible to get it to the form you suggested, I'm not opposed to accepting this, I'm just not an ObjectiveC person so I haven't used clang-format on code like this, I just don't want to break others

requesting changes due to removed line?

clang/lib/Format/ContinuationIndenter.cpp

871

I'm a little confused by the code, this feels to me like its covering something more than a specific than just the ObjectiveC case, what is meant here by a nestedblockspecialcase? this feels like we are turning off BreakBeforeParameter by another method?

I get the code below, just wondering why this is also needed.

clang/lib/Format/Format.cpp

796

did you mean to remove this line?

[gribozavr2]

clang/include/clang/Format/Format.h

1651

I don't know much about ClangFormat, but I'd prefer to keep the option name positive ("ObjCBreakBefore...").

[MyDeveloperDay]

clang/include/clang/Format/Format.h

1651

+1 that

[ghvg1313]

clang/lib/Format/ContinuationIndenter.cpp

871

I think you are right, bin packing is already avoided as the comment says so I don't have to do anything extra.

Was trying to handle linebreaking within block as well. Not needed indeed.

[ghvg1313]

• rename option, remove handlers for nested block itself

[MyDeveloperDay]

so now I think this is better because it's encased inside your TT_ObjCMethodExpr method which means it should only impact ObjectiveC

Nit on the documentation
Nit missing parse tests

but otherwise you are pretty close, make those changes and I'll mark accepted

clang/docs/ReleaseNotes.rst

347

Nit: this section should really go into the Format.h and the ClangFormatStyle.rst, for the release note a couple of lines is probably ample.

But ultimately this example will disappear from the release notes (as they move on) and I think this example is good to have in the main documentation.

clang/unittests/Format/FormatTest.cpp

14446 ↗

(On Diff #231971)

Nit: you are missing a CHECK_PARSE_BOOL tests

[ghvg1313]

• Move documents from release notes to header and proper rst, register test

[MyDeveloperDay]

LGTM

A little advice as you fix each comment check the "Done" button.

I notice you are a new user, will you need help landing it?

[ghvg1313]

In D70926#1769437, @MyDeveloperDay wrote:

LGTM

A little advice as you fix each comment check the "Done" button.

I notice you are a new user, will you need help landing it?

Thanks for the review!
And yes I do need help landing it, quite new to the community, not sure if I should head to github and dupe the changes or something else...

[ghvg1313]

@MyDeveloperDay Anything I'll need to do on my end to start the landing process?

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

14468 ↗

(On Diff #232177)

This line does not compile, I think whilst we are on the subject it would be better to move this test into

FormatTestObjC.cpp

[ghvg1313]

• Handle parameter break between blocks
• Fix test cases and move them to objcTest
• rebase

[ghvg1313]

clang/lib/Format/ContinuationIndenter.cpp

871

Notice I get this back again, it's actually to avoid parameter break in between blocks, so we don't get inconsistent parameter break between blocks, for example:

- (void)_aMethod
{
    [self.test1 t:self
                    w:self
         callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {
             u = c;
         } x:self callback2:^(typeof(self) self, NSNumber *u, NSNumber *v) {
             u = c;
         }]
}

Originally, the second block will trigger the parameter break in between itself and the previous block.

[MyDeveloperDay]

Thanks for the patch.

[ghvg1313]

In D70926#1790848, @MyDeveloperDay wrote:

Thanks for the patch.

Happy new year!
Could you help landing this diff? I don't know exactly what to do from here

[ghvg1313]

@MyDeveloperDay gentle ping for help merging

[MyDeveloperDay]

I need you to rebase this fix, also you need to clang-format it as well. Format.h/ContinuationIndenter.cpp and FormatTestObjC.cpp all failed clang-format check

[ghvg1313]

• rebase master
• formatting code

[MyDeveloperDay]

clang/docs/ReleaseNotes.rst

343

is this line part of this patch?

[ghvg1313]

clang/docs/ReleaseNotes.rst

343

No, rebased again and got rid of it

[ghvg1313]

• rebase master

clang/docs/ClangFormatStyleOptions.rst

**ObjCBlockIndentWidth** (``unsigned``)
   .. code-block:: objc
 
      ObjCBlockIndentWidth: 4
 
      [operation setCompletionBlock:^{
          [self onOperationDone];
      }];
 
+**ObjCBreakBeforeNestedBlockParam** (``bool``)
+  Break parameters list into lines when there is nested block
+  parameters in a fuction call.
+
+  .. code-block:: c++
+
+    false:
+     - (void)_aMethod
+     {
+         [self.test1 t:self w:self callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {
+             u = c;
+         }]
+     }
+     true:
+     - (void)_aMethod
+     {
+        [self.test1 t:self
+                     w:self
+            callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {
+                 u = c;
+             }]
+     }
+
 **ObjCSpaceAfterProperty** (``bool``)
   Add a space after ``@property`` in Objective-C, i.e. use
   ``@property (readonly)`` instead of ``@property(readonly)``.
 
 **ObjCSpaceBeforeProtocolList** (``bool``)
   Add a space in front of an Objective-C protocol list, i.e. use
   ``Foo <Protocol>`` instead of ``Foo<Protocol>``.
 

clang/docs/ReleaseNotes.rst

- Options ``DeriveLineEnding`` and  ``UseCRLF`` have been added to allow
   the decision as to which sort of line ending to use.
 
 - Option ``SpaceBeforeSquareBrackets`` has been added to insert a space before
   array declarations.
 
   .. code-block:: c++
 
     int a [5];    vs    int a[5];
 
+- Option ``ObjCBreakBeforeNestedBlockParam`` has been added to optionally apply
+  linebreaks for function arguments declarations before nested blocks.
+
 - Clang-format now supports JavaScript null operators.
 
   .. code-block:: c++
 
     const x = foo ?? default;
     const z = foo?.bar?.baz;
 
 libclang
 --------

clang/include/clang/Format/Format.h

   ///    }];
   /// \endcode
   unsigned ObjCBlockIndentWidth;
 
   /// Add a space after ``@property`` in Objective-C, i.e. use
   /// ``@property (readonly)`` instead of ``@property(readonly)``.
   bool ObjCSpaceAfterProperty;
 
+  /// Break parameters list into lines when there is nested block
+  /// parameters in a fuction call.
+  /// \code
+  ///   false:
+  ///    - (void)_aMethod
+  ///    {
+  ///        [self.test1 t:self w:self callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {
+  ///            u = c;
+  ///        }]
+  ///    }
+  ///    true:
+  ///    - (void)_aMethod
+  ///    {
+  ///       [self.test1 t:self
+  ///                    w:self
+  ///           callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {
+  ///                u = c;
+  ///            }]
+  ///    }
+  /// \endcode
+  bool ObjCBreakBeforeNestedBlockParam;
+
   /// Add a space in front of an Objective-C protocol list, i.e. use
   /// ``Foo <Protocol>`` instead of ``Foo<Protocol>``.
   bool ObjCSpaceBeforeProtocolList;
 
   /// The penalty for breaking around an assignment operator.
   unsigned PenaltyBreakAssignment;
 
   /// The penalty for breaking a function call after ``call(``.
return AccessModifierOffset == R.AccessModifierOffset &&
                R.KeepEmptyLinesAtTheStartOfBlocks &&
            MacroBlockBegin == R.MacroBlockBegin &&
            MacroBlockEnd == R.MacroBlockEnd &&
            MaxEmptyLinesToKeep == R.MaxEmptyLinesToKeep &&
            NamespaceIndentation == R.NamespaceIndentation &&
            NamespaceMacros == R.NamespaceMacros &&
            ObjCBinPackProtocolList == R.ObjCBinPackProtocolList &&
            ObjCBlockIndentWidth == R.ObjCBlockIndentWidth &&
+           ObjCBreakBeforeNestedBlockParam == R.ObjCBreakBeforeNestedBlockParam &&
            ObjCSpaceAfterProperty == R.ObjCSpaceAfterProperty &&
            ObjCSpaceBeforeProtocolList == R.ObjCSpaceBeforeProtocolList &&
            PenaltyBreakAssignment == R.PenaltyBreakAssignment &&
            PenaltyBreakBeforeFirstCallParameter ==
                R.PenaltyBreakBeforeFirstCallParameter &&
            PenaltyBreakComment == R.PenaltyBreakComment &&
            PenaltyBreakFirstLessLess == R.PenaltyBreakFirstLessLess &&
            PenaltyBreakString == R.PenaltyBreakString &&

clang/lib/Format/ContinuationIndenter.cpp

if (Current.is(tok::lessless))
     State.Stack.back().LastSpace += 3; // 3 -> width of "<< ".
 
   State.StartOfLineLevel = Current.NestingLevel;
   State.LowestLevelOnLine = Current.NestingLevel;
 
   // Any break on this level means that the parent level has been broken
   // and we need to avoid bin packing there.
   bool NestedBlockSpecialCase =
-      !Style.isCpp() && Current.is(tok::r_brace) && State.Stack.size() > 1 &&
-      State.Stack[State.Stack.size() - 2].NestedBlockInlined;
+      (!Style.isCpp() && Current.is(tok::r_brace) && State.Stack.size() > 1 &&
+      State.Stack[State.Stack.size() - 2].NestedBlockInlined) ||
+      (Style.Language == FormatStyle::LK_ObjC && Current.is(tok::r_brace)
+       && State.Stack.size() > 1 && !Style.ObjCBreakBeforeNestedBlockParam);
   if (!NestedBlockSpecialCase)
     for (unsigned i = 0, e = State.Stack.size() - 1; i != e; ++i)
       State.Stack[i].BreakBeforeParameter = true;
 
   if (PreviousNonComment &&
       !PreviousNonComment->isOneOf(tok::comma, tok::colon, tok::semi) &&
       (PreviousNonComment->isNot(TT_TemplateCloser) ||
        Current.NestingLevel != 0) &&
       !PreviousNonComment->isOneOf(
           TT_BinaryOperator, TT_FunctionAnnotationRParen, TT_JavaAnnotation,
           TT_LeadingJavaAnnotation) &&
       Current.isNot(TT_BinaryOperator) && !PreviousNonComment->opensScope())
AvoidBinPacking =
         (Style.Language == FormatStyle::LK_JavaScript && EndsInComma) ||
         (State.Line->MustBeDeclaration && !BinPackDeclaration) ||
         (!State.Line->MustBeDeclaration && !Style.BinPackArguments) ||
         (Style.ExperimentalAutoDetectBinPacking &&
          (Current.PackingKind == PPK_OnePerLine ||
           (!BinPackInconclusiveFunctions &&
            Current.PackingKind == PPK_Inconclusive)));
 
-    if (Current.is(TT_ObjCMethodExpr) && Current.MatchingParen) {
+    if (Current.is(TT_ObjCMethodExpr) && Current.MatchingParen &&
+        Style.ObjCBreakBeforeNestedBlockParam) {
       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.Stack) + State.Column >
             getColumnLimit(State))
           BreakBeforeParameter = true;
       } else {
         // For ColumnLimit = 0, we have to figure out whether there is or has to

clang/lib/Format/Format.cpp

IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks",
                    Style.KeepEmptyLinesAtTheStartOfBlocks);
     IO.mapOptional("MacroBlockBegin", Style.MacroBlockBegin);
     IO.mapOptional("MacroBlockEnd", Style.MacroBlockEnd);
     IO.mapOptional("MaxEmptyLinesToKeep", Style.MaxEmptyLinesToKeep);
     IO.mapOptional("NamespaceIndentation", Style.NamespaceIndentation);
     IO.mapOptional("NamespaceMacros", Style.NamespaceMacros);
     IO.mapOptional("ObjCBinPackProtocolList", Style.ObjCBinPackProtocolList);
     IO.mapOptional("ObjCBlockIndentWidth", Style.ObjCBlockIndentWidth);
+    IO.mapOptional("ObjCBreakBeforeNestedBlockParam",
+                   Style.ObjCBreakBeforeNestedBlockParam);
     IO.mapOptional("ObjCSpaceAfterProperty", Style.ObjCSpaceAfterProperty);
     IO.mapOptional("ObjCSpaceBeforeProtocolList",
                    Style.ObjCSpaceBeforeProtocolList);
     IO.mapOptional("PenaltyBreakAssignment", Style.PenaltyBreakAssignment);
     IO.mapOptional("PenaltyBreakBeforeFirstCallParameter",
                    Style.PenaltyBreakBeforeFirstCallParameter);
     IO.mapOptional("PenaltyBreakComment", Style.PenaltyBreakComment);
     IO.mapOptional("PenaltyBreakFirstLessLess",
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.IndentWidth = 2;
   LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
   LLVMStyle.JavaScriptWrapImports = true;
   LLVMStyle.TabWidth = 8;
   LLVMStyle.MaxEmptyLinesToKeep = 1;
   LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true;
   LLVMStyle.NamespaceIndentation = FormatStyle::NI_None;
   LLVMStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Auto;
   LLVMStyle.ObjCBlockIndentWidth = 2;
+  LLVMStyle.ObjCBreakBeforeNestedBlockParam = true;
   LLVMStyle.ObjCSpaceAfterProperty = false;
   LLVMStyle.ObjCSpaceBeforeProtocolList = true;
   LLVMStyle.PointerAlignment = FormatStyle::PAS_Right;
   LLVMStyle.SpacesBeforeTrailingComments = 1;
   LLVMStyle.Standard = FormatStyle::LS_Latest;
   LLVMStyle.UseCRLF = false;
   LLVMStyle.UseTab = FormatStyle::UT_Never;
   LLVMStyle.ReflowComments = true;

clang/unittests/Format/FormatTestObjC.cpp

verifyFormat("int f() {\n"
                "}");
   verifyFormat(
       "int f() {\n"
       "  for (auto /**/ const /**/ volatile /**/ && /**/ [elem] : list)\n"
       "    return 0;\n"
       "}");
 }
 
+TEST_F(FormatTestObjC, BreakLineBeforeNestedBlockParam) {
+  Style = getGoogleStyle(FormatStyle::LK_ObjC);
+  Style.ObjCBreakBeforeNestedBlockParam = false;
+  Style.ColumnLimit = 0;
+
+  verifyFormat("[self.test1 t:self callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {\n"
+               "  u = v;\n"
+               "}]");
+
+  verifyFormat("[self.test1 t:self w:self callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {\n"
+               "  u = v;\n"
+               "}]");
+
+  verifyFormat("[self.test1 t:self w:self callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {\n"
+               "  u = c;\n"
+               "} w:self callback2:^(typeof(self) self, NSNumber *a, NSNumber *b, NSNumber *c) {\n"
+               "  b = c;\n"
+               "}]");
+
+  Style.ColumnLimit = 80;
+  verifyFormat("[self.test_method a:self b:self\n"
+               "           callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {\n"
+               "             u = v;\n"
+               "           }]");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang