[clang-format] Always indent wrapped Objective-C selector names

[benhamilton]

Summary

Currently, indentation of Objective-C method names which are wrapped
onto the next line due to a long return type is controlled by the
style option IndentWrappedFunctionNames.

This diff changes the behavior so we always indent wrapped Objective-C
selector names.

NOTE: I partially reverted https://github.com/llvm-mirror/clang/commit/6159c0fbd1876c7f5f984b4830c664cc78f16e2e / rL242484, as it was causing wrapped selectors to be double-indented. Its tests in FormatTestObjC.cpp still pass.

Test Plan: Tests updated. Ran tests with:

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

[benhamilton]

Fix typo

[stephanemoore]

What do you think of the name IndentWrappedObjCMethodSignatures as an alternative to IndentWrappedObjCMethodNames? In Objective-C the method name might be considered the selector whereas I believe that this option pertains to formatting of the method signature?

[benhamilton]

What do you think of the name IndentWrappedObjCMethodSignatures as an alternative to IndentWrappedObjCMethodNames? In Objective-C the method name might be considered the selector whereas I believe that this option pertains to formatting of the method signature?

This is a good suggestion, but since this is a specialization of IndentWrappedFunctionNames, I figured keeping the name of the new option similar to that would be best.

[stephanemoore]

include/clang/Format/Format.h

1154–1163

Do we explicitly want these to be generic with the intent to later reuse the enum for C++ method wrapping style?

[djasper]

Do we have a public style guide that explicitly says to do this?
That's a requirement for new style options as per https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options.

Also, are we sure that somebody wants the other behavior? Does that make sense for ObjC? I'd like to avoid options that nobody actually sets to a different value.

[benhamilton]

include/clang/Format/Format.h

1154–1163

I figured languages like Java and JS might want to customize this.

[benhamilton]

Do we have a public style guide that explicitly says to do this?

Good question! This was the result of internal discussion with the Google Objective-C style guide maintainers based on analysis of ObjC code at Google.

I took a look, and the C++ style guide doesn't seem to explicitly say to align wrapped function names on column 0 (although clang-format does that):

http://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions
http://google.github.io/styleguide/cppguide.html#Function_Calls

I will confirm with the ObjC style guide maintainers whether they want to explicitly list the desired ObjC behavior in the style guide.

[djasper]

Well, I disagree. It says: "If you break after the return type of a function declaration or definition, do not indent."

[benhamilton]

Well, I disagree. It says: "If you break after the return type of a function declaration or definition, do not indent."

Great, thanks for clarifying!

[stephanemoore]

Update:
I am proposing to update the Google Objective-C style guide to explicitly describe the desired indentation behavior.

include/clang/Format/Format.h

1154–1163

Gotcha; sounds reasonable to me.

[jolesiak]

I am proposing to update the Google Objective-C style guide to explicitly describe the desired indentation behavior.

+1

[djasper]

I still really believe that these config options do no make sense and are actively confusing.

I see two options:

• We leave this as is
• We fix this right

The right fix here is (IMO) that a style already is per language and thus already has a member specifying the language. What we have done in the scope of formatting the contents of raw string literals is that you can, in the same configuration file/setting, have different styles based on the language being formatted. Thus, if we encounter a text-formatted proto inside a C++ raw string literal, we switch to the style flags for proto rather than using those for C++. You have these different options in the same style configuration file, in a different section per language.

So, if you look at a config file, you could see how a user sets the existing IndentWrappedFunctionNames to true for ObjC and to false for C++. Now IMO, that should mean that ObjC function names are indented and C++ functions are not, even if the language of the *file* is ObjC. It doesn't require us to repeat these options for each language in the style for each language.

Getting this right will require some refactoring of how a style is passed around and used, but I think it'd be the right thing to do.

Look at it the other way. If we go forward with this patch you can have style configuration files saying:

---

BasedOnStyle: LLVM
---
Language: Cpp
IndentWrappedObjCMethodNames: Never
IndentWrappedFunctionNames: true
---
Language: ObjC
IndentWrappedObjCMethodNames: Always
IndentWrappedFunctionNames: false
---

I find it very hard to explain what that even means.

[benhamilton]

a user sets the existing IndentWrappedFunctionNames to true for ObjC and to false for C++.
Now IMO, that should mean that ObjC function names are indented and C++ functions are not, even if the language of the *file* is ObjC.
Getting this right will require some refactoring of how a style is passed around and used, but I think it'd be the right thing to do.

This is an interesting proposal, but adding infrastructure to pass around and manage per-language FormatStyle objects to formatter logic seems like a large change to me, and a lot of work to avoid adding a new configuration option.

Without changing the architecture to pass around per-language FormatStyles, we could also unconditionally indent wrapped Objective-C methods in all styles (ignoring IndentWrappedFunctionNames) and make IndentWrappedFunctionNames only apply to C++ functions and methods.

I just did a survey of open-source Objective-C code in use at Google, and it's over 5:1 in favor of indenting wrapped Objective-C methods.

What do you think of that option?

[djasper]

I'd go to great lengths to avoid adding new config options and so I don't think this would be a bad trade-off to make.

Also, note that you might not actually have to change much. A FormatStyle already contains a reference to the FormatStyleSet it was created from and you can get the FormatStyle for a different language through that. It might just be a bit hacky and not easy to understand as is, but maybe there are easy abstractions that we could build around it.

At any rate, your proposal (always indenting for ObjC code) is also fine by me. Do you think that survey Google-style ObjC code is enough? What does Xcode do by default?

[benhamilton]

A FormatStyle already contains a reference to the FormatStyleSet it was created from and you can get the FormatStyle for a different language through that. It might just be a bit hacky and not easy to understand as is, but maybe there are easy abstractions that we could build around it.

Hmm, I didn't know that. That would make a short-term hack easier.

At any rate, your proposal (always indenting for ObjC code) is also fine by me. Do you think that survey Google-style ObjC code is enough? What does Xcode do by default?

Apple SDKs have no line length limit and don't wrap code as far as I can tell, so they don't hit this issue.

Xcode neither indents nor aligns colons by default. It does no code formatting at all as far as I can tell.

I guess the question is, are there other big clients of Objective-C formatting via clang-format besides Google? I looked at the existing public coding styles, and none of them appear to talk about this issue.

[benhamilton]

Actually, a slight correction. By default, Xcode has a "Line wrapping" setting under Preferences -> Text editing which says:

[ X ] Wrap lines to editor width
     Indent wrapped lines by:  [    4 ] Spaces

which suggests people using Xcode may expect indentation on wrapped lines by default.

It also has a "Syntax-aware indenting" feature including "Automatic indent for... :", but the colon indentation only seems to apply for method invocations, not method signatures.

[benhamilton]

By default, Xcode has a "Line wrapping" setting

Ah, that appears to just be a visual thing. Xcode doesn't actually insert newlines into the source code.

[djasper]

Ok, you know the ObjC community much better than I do. If you think that adding the indentation for ObjC irrespective of the option works for most people, I am happy to go with it.

[benhamilton]

• Unconditionally indent wrapped ObjC selector names.
• Partially revert r242484, as it was causing double-indenting (the tests still pass with it reverted, so I suspect it's no longer necessary).

[benhamilton]

Ok, you know the ObjC community much better than I do. If you think that adding the indentation for ObjC irrespective of the option works for most people, I am happy to go with it.

OK, I went ahead and implemented unconditional indenting of wrapped ObjC selectors and removed the configuration setting.

I had to partially revert rL242484; with it, we were getting 8-column indenting, and without it, its tests still pass, so I suspect it's no longer necessary.

[djasper]

unittests/Format/FormatTest.cpp

7666 ↗

(On Diff #141872)

maybe: .. and always indents. ?

[benhamilton]

Update comment.

[benhamilton]

unittests/Format/FormatTest.cpp

7666 ↗

(On Diff #141872)

Sure, done.

include/clang/Format/Format.h

   ///        LoooooooooooooooooooooooooooooooongFunctionDeclaration();
   ///
   ///    false:
   ///    LoooooooooooooooooooooooooooooooooooooooongReturnType
   ///    LoooooooooooooooooooooooooooooooongFunctionDeclaration();
   /// \endcode
   bool IndentWrappedFunctionNames;
 
+  /// \brief The style of indenting long function or method names wrapped
+  /// onto the next line.
+  enum IndentWrappedMethodStyle {
+    /// Automatically determine indenting style.
+    IWM_Auto,
+    /// Always indent wrapped methods names.
+    IWM_Always,
+    /// Never indent wrapped method names.
+    IWM_Never,
+  };
+
+  /// \brief Controls whether Objective-C method definition or
+  /// declaration is indented after wrapping onto the next line.
+  /// \code
+  ///    Always (or Auto, if IndentWrappedFunctionNames is true):
+  ///    - (LoooooooooooooooooooooooooooooooooooooooongReturnType)
+  ///        loooooooooooooooooooooooooooooooongFunctionDeclaration;
+  ///
+  ///    Never (or Auto, if IndentWrappedFunctionNames is false):
+  ///    - (LoooooooooooooooooooooooooooooooooooooooongReturnType)
+  ///    LoooooooooooooooooooooooooooooooongFunctionDeclaration;
+  /// \endcode
+  IndentWrappedMethodStyle IndentWrappedObjCMethodNames;
+
   /// \brief Quotation styles for JavaScript strings. Does not affect template
   /// strings.
   enum JavaScriptQuoteStyle {
     /// Leave string quotes as they are.
     /// \code{.js}
     ///    string1 = "foo";
     ///    string2 = 'bar';
     /// \endcode
return AccessModifierOffset == R.AccessModifierOffset &&
            FixNamespaceComments == R.FixNamespaceComments &&
            ForEachMacros == R.ForEachMacros &&
            IncludeBlocks == R.IncludeBlocks &&
            IncludeCategories == R.IncludeCategories &&
            IndentCaseLabels == R.IndentCaseLabels &&
            IndentPPDirectives == R.IndentPPDirectives &&
            IndentWidth == R.IndentWidth && Language == R.Language &&
            IndentWrappedFunctionNames == R.IndentWrappedFunctionNames &&
+           IndentWrappedObjCMethodNames == R.IndentWrappedObjCMethodNames &&
            JavaScriptQuotes == R.JavaScriptQuotes &&
            JavaScriptWrapImports == R.JavaScriptWrapImports &&
            KeepEmptyLinesAtTheStartOfBlocks ==
                R.KeepEmptyLinesAtTheStartOfBlocks &&
            MacroBlockBegin == R.MacroBlockBegin &&
            MacroBlockEnd == R.MacroBlockEnd &&
            MaxEmptyLinesToKeep == R.MaxEmptyLinesToKeep &&
            NamespaceIndentation == R.NamespaceIndentation &&

lib/Format/ContinuationIndenter.cpp

 #include "clang/Format/Format.h"
 #include "llvm/Support/Debug.h"
 
 #define DEBUG_TYPE "format-indenter"
 
 namespace clang {
 namespace format {
 
+// Returns true if a TT_SelectorName should be indented when wrapped,
+// false otherwise.
+static bool shouldIndentWrappedSelectorName(const FormatStyle &Style) {
+  // TT_SelectorName is used across multiple languages; we only want
+  // Style.IndentWrappedObjCMethodNames to apply to ObjC.
+  if (Style.Language == FormatStyle::LK_ObjC)
+    return Style.IndentWrappedObjCMethodNames == FormatStyle::IWM_Always ||
+           (Style.IndentWrappedObjCMethodNames == FormatStyle::IWM_Auto &&
+            Style.IndentWrappedFunctionNames);
+  else
+    return Style.IndentWrappedFunctionNames;
+}
+
 // Returns the length of everything up to the first possible line break after
 // the ), ], } or > matching \c Tok.
 static unsigned getLengthToMatchingParen(const FormatToken &Tok) {
   if (!Tok.MatchingParen)
     return 0;
   FormatToken *End = Tok.MatchingParen;
   while (End->Next && !End->Next->CanBreakBefore) {
     End = End->Next;
if (NextNonComment->isMemberAccess()) {
     if (State.Stack.back().CallContinuation == 0)
       State.Stack.back().CallContinuation = State.Column;
   } else if (NextNonComment->is(TT_SelectorName)) {
     if (!State.Stack.back().ObjCSelectorNameFound) {
       if (NextNonComment->LongestObjCSelectorName == 0) {
         State.Stack.back().AlignColons = false;
       } else {
         State.Stack.back().ColonPos =
-            (Style.IndentWrappedFunctionNames
+            (shouldIndentWrappedSelectorName(Style)
                  ? std::max(State.Stack.back().Indent,
                             State.FirstIndent + Style.ContinuationIndentWidth)
                  : State.Stack.back().Indent) +
             std::max(NextNonComment->LongestObjCSelectorName,
                      NextNonComment->ColumnWidth);
       }
     } else if (State.Stack.back().AlignColons &&
                State.Stack.back().ColonPos <= NextNonComment->ColumnWidth) {
if ((PreviousNonComment &&
             TT_AttributeParen, TT_AttributeSquare, TT_FunctionAnnotationRParen,
             TT_JavaAnnotation, TT_LeadingJavaAnnotation))) ||
       (!Style.IndentWrappedFunctionNames &&
        NextNonComment->isOneOf(tok::kw_operator, TT_FunctionDeclarationName)))
     return std::max(State.Stack.back().LastSpace, State.Stack.back().Indent);
   if (NextNonComment->is(TT_SelectorName)) {
     if (!State.Stack.back().ObjCSelectorNameFound) {
       unsigned MinIndent =
-          (Style.IndentWrappedFunctionNames
+          (shouldIndentWrappedSelectorName(Style)
                ? std::max(State.Stack.back().Indent,
                           State.FirstIndent + Style.ContinuationIndentWidth)
                : State.Stack.back().Indent);
       if (NextNonComment->LongestObjCSelectorName == 0)
         return MinIndent;
       return MinIndent +
              std::max(NextNonComment->LongestObjCSelectorName,
                       NextNonComment->ColumnWidth) -
if (!Current.opensScope() && !Current.closesScope() &&
       !Current.is(TT_PointerOrReference))
     State.LowestLevelOnLine =
         std::min(State.LowestLevelOnLine, Current.NestingLevel);
   if (Current.isMemberAccess())
     State.Stack.back().StartOfFunctionCall =
         !Current.NextOperator ? 0 : State.Column;
   if (Current.is(TT_SelectorName)) {
     State.Stack.back().ObjCSelectorNameFound = true;
-    if (Style.IndentWrappedFunctionNames) {
+    if (shouldIndentWrappedSelectorName(Style))
       State.Stack.back().Indent =
           State.FirstIndent + Style.ContinuationIndentWidth;
-    }
-  }
   if (Current.is(TT_CtorInitializerColon) &&
       Style.BreakConstructorInitializers != FormatStyle::BCIS_AfterColon) {
     // Indent 2 from the column, so:
     // SomeClass::SomeClass()
     //     : First(...), ...
     //       Next(...)
     //       ^ line up here.
     State.Stack.back().Indent =

lib/Format/Format.cpp

static void enumeration(IO &IO,
     IO.enumCase(Value, "Always", FormatStyle::SBPO_Always);
 
     // For backward compatibility.
     IO.enumCase(Value, "false", FormatStyle::SBPO_Never);
     IO.enumCase(Value, "true", FormatStyle::SBPO_ControlStatements);
   }
 };
 
+template <>
+struct ScalarEnumerationTraits<FormatStyle::IndentWrappedMethodStyle> {
+  static void enumeration(IO &IO,
+                          FormatStyle::IndentWrappedMethodStyle &Value) {
+    IO.enumCase(Value, "Auto", FormatStyle::IWM_Auto);
+    IO.enumCase(Value, "Always", FormatStyle::IWM_Always);
+    IO.enumCase(Value, "Never", FormatStyle::IWM_Never);
+  }
+};
+
 template <> struct MappingTraits<FormatStyle> {
   static void mapping(IO &IO, FormatStyle &Style) {
     // When reading, read the language first, we need it for getPredefinedStyle.
     IO.mapOptional("Language", Style.Language);
 
     if (IO.outputting()) {
       StringRef StylesArray[] = {"LLVM",    "Google", "Chromium",
                                  "Mozilla", "WebKit", "GNU"};
static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("IncludeBlocks", Style.IncludeBlocks);
     IO.mapOptional("IncludeCategories", Style.IncludeCategories);
     IO.mapOptional("IncludeIsMainRegex", Style.IncludeIsMainRegex);
     IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
     IO.mapOptional("IndentPPDirectives", Style.IndentPPDirectives);
     IO.mapOptional("IndentWidth", Style.IndentWidth);
     IO.mapOptional("IndentWrappedFunctionNames",
                    Style.IndentWrappedFunctionNames);
+    IO.mapOptional("IndentWrappedObjCMethodNames",
+                   Style.IndentWrappedObjCMethodNames);
     IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
     IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
     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);
FormatStyle getLLVMStyle() {
   LLVMStyle.IncludeCategories = {{"^\"(llvm|llvm-c|clang|clang-c)/", 2},
                                  {"^(<|\"(gtest|gmock|isl|json)/)", 3},
                                  {".*", 1}};
   LLVMStyle.IncludeIsMainRegex = "(Test)?$";
   LLVMStyle.IncludeBlocks = FormatStyle::IBS_Preserve;
   LLVMStyle.IndentCaseLabels = false;
   LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
   LLVMStyle.IndentWrappedFunctionNames = false;
+  LLVMStyle.IndentWrappedObjCMethodNames = FormatStyle::IWM_Auto;
   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;

unittests/Format/FormatTestObjC.cpp

verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
                "aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a;\n");
   verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
                " aaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a\n"
                "aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a;\n");
   verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
                "aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a\n"
                " aaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a;\n");
 
+  Style.IndentWrappedObjCMethodNames = FormatStyle::IWM_Always;
+  verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
+               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaa;\n");
+  verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
+               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a;\n");
+  verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
+               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a\n"
+               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a;\n");
+  verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
+               "     aaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a\n"
+               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a;\n");
+  verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
+               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a\n"
+               "     aaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a;\n");
+
+  Style.IndentWrappedObjCMethodNames = FormatStyle::IWM_Never;
+  verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
+               "aaaaaaaaaaaaaaaaaaaaaaaaaaaa;\n");
+  verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
+               "aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a;\n");
+  verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
+               "aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a\n"
+               "aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a;\n");
+  verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
+               " aaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a\n"
+               "aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a;\n");
+  verifyFormat("- (aaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
+               "aaaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a\n"
+               " aaaaaaaaaaaaaaaaaaaaaaaaaaa:(int)a;\n");
+
   // Continuation indent width should win over aligning colons if the function
   // name is long.
   Style = getGoogleStyle(FormatStyle::LK_ObjC);
   Style.ColumnLimit = 40;
   Style.IndentWrappedFunctionNames = true;
   verifyFormat("- (void)shortf:(GTMFoo *)theFoo\n"
                "    dontAlignNamef:(NSRect)theRect {\n"
                "}");