[clang-format] Ensure ObjC selectors with 0 args are annotated correctly

[benhamilton]

Summary

Previously, clang-format would incorrectly annotate 0-argument
Objective-C selector names as TT_TrailingAnnotation:

% echo "-(void)foo;" > /tmp/test.m
% ./bin/clang-format -debug /tmp/test.m
Language: Objective-C
----
Line(0, FSC=0): minus[T=68, OC=0] l_paren[T=68, OC=1] void[T=68, OC=2]
r_paren[T=68, OC=6] identifier[T=68, OC=7] semi[T=68, OC=10]
Line(0, FSC=0): eof[T=68, OC=0]
Run 0...
AnnotatedTokens(L=0):
 M=0 C=0 T=ObjCMethodSpecifier S=1 B=0 BK=0 P=0 Name=minus L=1 PPK=2
 FakeLParens= FakeRParens=0 Text='-'
 M=0 C=1 T=Unknown S=1 B=0 BK=0 P=33 Name=l_paren L=3 PPK=2
 FakeLParens= FakeRParens=0 Text='('
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=140 Name=void L=7 PPK=2 FakeLParens=
 FakeRParens=0 Text='void'
 M=0 C=0 T=CastRParen S=0 B=0 BK=0 P=43 Name=r_paren L=8 PPK=2
 FakeLParens= FakeRParens=0 Text=')'
 M=0 C=1 T=TrailingAnnotation S=0 B=0 BK=0 P=120 Name=identifier L=11
 PPK=2 FakeLParens= FakeRParens=0 Text='foo'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=semi L=12 PPK=2 FakeLParens=
 FakeRParens=0 Text=';'

This caused us to incorrectly indent 0-argument wrapped selectors
when Style.IndentWrappedFunctionNames was false, as we thought
the 0-argument ObjC selector name was actually a trailing
annotation (which is always indented).

This diff fixes the issue and adds tests.

Test Plan: New tests added. Confirmed tests failed before diff.

After diff, tests passed. Ran tests with:
% make -j12 FormatTests &&
./tools/clang/unittests/Format/FormatTests

[jolesiak]

unittests/Format/FormatTestObjC.cpp

528

I know that Style.IndentWrappedFunctionNames is false by default, but how about adding

Style.IndentWrappedFunctionNames = false;

before these verifyFormat calls? I feel like that makes it more readable and makes test independent on this Style constant.

[jolesiak]

unittests/Format/FormatTestObjC.cpp

528

This comment is kind of irrelevant as this change is included in D45005.

[jolesiak]

unittests/Format/FormatTestObjC.cpp

528

My bad, this change is NOT included in D45005. (I don't know how to remove inline comment)

[benhamilton]

@jolesiak

[benhamilton]

unittests/Format/FormatTestObjC.cpp

528

Sure, done.

[djasper]

lib/Format/TokenAnnotator.cpp

1350

Isn't it wrong that we detect this as a cast r_paren in the first place?

[benhamilton]

lib/Format/TokenAnnotator.cpp

1350

Fantastic question, I asked myself the same thing.

I tried a few variations on this (leaving it as TT_Unknown, making a new type, etc.) and discovered there is at least one existing place which relies on the TT_CastRParen type as an indicator of ObjC code. Example:

https://github.com/llvm-mirror/clang/blob/e37a191e99773959118155304ec2ed0bc0d591c2/lib/Format/TokenAnnotator.cpp#L394

I can fix those, but if I do so, I think it should be a separate diff. What do you think?

[benhamilton]

lib/Format/TokenAnnotator.cpp

1350

@djasper and I talked about this on Friday and agreed we should follow up separately.

I filed https://bugs.llvm.org/show_bug.cgi?id=36976 to follow up.

[benhamilton]

Any more comments on this one, folks? I'd love to land this fix.

[jolesiak]

lgtm

[djasper]

lib/Format/TokenAnnotator.cpp

1350

You might also want to add a

// FIXME: ...

[benhamilton]

Add FIXME comments.

lib/Format/TokenAnnotator.cpp

bool parseSquare() {
     bool StartsObjCMethodExpr =
         !CppArrayTemplates && Style.isCpp() && !IsCpp11AttributeSpecifier &&
         Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) &&
         !CurrentToken->isOneOf(tok::l_brace, tok::r_square) &&
         (!Parent ||
          Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren,
                          tok::kw_return, tok::kw_throw) ||
          Parent->isUnaryOperator() ||
+         // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
          Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) ||
          getBinOpPrecedence(Parent->Tok.getKind(), true, true) > prec::Unknown);
     bool ColonFound = false;
 
     unsigned BindingIncrease = 1;
     if (Left->isCppStructuredBinding(Style)) {
       Left->Type = TT_StructuredBindingLSquare;
     } else if (Left->is(TT_Unknown)) {
while (CurrentToken) {
           StartsObjCMethodExpr = true;
           // ParameterCount might have been set to 1 before expression was
           // recognized as ObjCMethodExpr (as '1 + number of commas' formula is
           // used for other expression types). Parameter counter has to be,
           // therefore, reset to 0.
           Left->ParameterCount = 0;
           Contexts.back().ColonIsObjCMethodExpr = true;
           if (Parent && Parent->is(tok::r_paren))
+            // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
             Parent->Type = TT_CastRParen;
         }
         ColonFound = true;
       }
       if (CurrentToken->is(tok::comma) && Left->is(TT_ObjCMethodExpr) &&
           !ColonFound)
         Left->Type = TT_ArrayInitializerLSquare;
       FormatToken *Tok = CurrentToken;
case tok::colon:
           if (FormatToken *Previous = Tok->getPreviousNonComment())
             Previous->Type = TT_SelectorName;
         }
       } else if (Contexts.back().ColonIsObjCMethodExpr ||
                  Line.startsWith(TT_ObjCMethodSpecifier)) {
         Tok->Type = TT_ObjCMethodExpr;
         const FormatToken *BeforePrevious = Tok->Previous->Previous;
         if (!BeforePrevious ||
+            // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
             !(BeforePrevious->is(TT_CastRParen) ||
               (BeforePrevious->is(TT_ObjCMethodExpr) &&
                BeforePrevious->is(tok::colon))) ||
             BeforePrevious->is(tok::r_square) ||
             Contexts.back().LongestObjCSelectorName == 0) {
           Tok->Previous->Type = TT_SelectorName;
           if (!Contexts.back().FirstObjCSelectorName)
             Contexts.back().FirstObjCSelectorName = Tok->Previous;
if (Current.is(Keywords.kw_instanceof)) {
       if (PreviousNoComment &&
           PreviousNoComment->isOneOf(tok::comma, tok::l_brace))
         Current.Type = TT_DesignatedInitializerPeriod;
       else if (Style.Language == FormatStyle::LK_Java && Current.Previous &&
                Current.Previous->isOneOf(TT_JavaAnnotation,
                                          TT_LeadingJavaAnnotation)) {
         Current.Type = Current.Previous->Type;
       }
+    } else if (Current.isOneOf(tok::identifier, tok::kw_new) &&
+               // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
+               Current.Previous && Current.Previous->is(TT_CastRParen) &&
+               Current.Previous->MatchingParen &&
+               Current.Previous->MatchingParen->Previous &&
+               Current.Previous->MatchingParen->Previous->is(
+                   TT_ObjCMethodSpecifier)) {
+      // This is the first part of an Objective-C selector name. (If there's no
+      // colon after this, this is the only place which annotates the identifier
+      // as a selector.)
+      Current.Type = TT_SelectorName;
     } else if (Current.isOneOf(tok::identifier, tok::kw_const) &&
                Current.Previous &&
                !Current.Previous->isOneOf(tok::equal, tok::at) &&
                Line.MightBeFunctionDecl && Contexts.size() == 1) {
       // Line.MightBeFunctionDecl can only be true after the parentheses of a
       // function declaration have been found.
       Current.Type = TT_TrailingAnnotation;
     } else if ((Style.Language == FormatStyle::LK_Java ||

unittests/Format/FormatTestObjC.cpp

TEST_F(FormatTestObjC, FormatObjCMethodDeclarations) {
   verifyFormat("+ (instancetype)new;\n");
   Style.ColumnLimit = 60;
   verifyFormat("- (instancetype)initXxxxxx:(id<x>)x\n"
                "                         y:(id<yyyyyyyyyyyyyyyyyyyy>)y\n"
                "    NS_DESIGNATED_INITIALIZER;");
   verifyFormat("- (void)drawRectOn:(id)surface\n"
                "            ofSize:(size_t)height\n"
                "                  :(size_t)width;");
+  Style.ColumnLimit = 40;
+  // Make sure selectors with 0, 1, or more arguments are not indented
+  // when IndentWrappedFunctionNames is false.
+  Style.IndentWrappedFunctionNames = false;
+  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"