[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

527

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

527

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

[jolesiak]

unittests/Format/FormatTestObjC.cpp

527

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

527

Sure, done.

[djasper]

lib/Format/TokenAnnotator.cpp

1347

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

[benhamilton]

lib/Format/TokenAnnotator.cpp

1347

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

1347

@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

1347

You might also want to add a

// FIXME: ...

[benhamilton]

Add FIXME comments.

lib/Format/TokenAnnotator.cpp

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) &&
+               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"