[clang-format/ObjC] Correctly parse Objective-C methods with 'class' in name

[benhamilton]

Summary

Please take a close look at this CL. I haven't touched much of
UnwrappedLineParser before, so I may have gotten things wrong.

Previously, clang-format would incorrectly format the following:

@implementation Foo

- (Class)class {
}

- (void)foo {
}

@end

as:

@implementation Foo

- (Class)class {
}

    - (void)foo {
}

@end

The problem is whenever UnwrappedLineParser::parseStructuralElement()
sees any of the keywords class, struct, or enum, it calls
parseRecord() to parse them as a C/C++ record.

This causes subsequent lines to be parsed incorrectly, which
causes them to be indented incorrectly.

In Objective-C/Objective-C++, these keywords are valid selector
components.

This diff fixes the issue by explicitly handling + and - lines
inside @implementation / @interface / @protocol blocks
and parsing them as Objective-C methods.

Test Plan: New tests added. Ran tests with:

make -j16 FormatTests && ./tools/clang/unittests/Format/FormatTests

[benhamilton]

Format

[jolesiak]

lib/Format/UnwrappedLineParser.cpp

2137

tok::r_brace could be skiped - see comment to line 2143.

2143

We have to add return after addUnwrappedLine as parseBlock does consume tok::r_brace. Without return we will consume tokens after }. This problem will rarely occur as most lines end with tok::semi or tok::r_brace and it will be terminated properly (however maybe not handled properly as we just skip every token in else) by if branch.

Test like:

@implementation Foo
- (foo)foo {
}
@end
@implementation Bar
- (bar)bar {
}
@end

will distinguish version with return from one without. Therefore, I think we should add it.

[benhamilton]

@jolesiak

lib/Format/UnwrappedLineParser.cpp

2137

Done.

2143

Done, test added.

[jolesiak]

LGTM, would be nice though if somebody else took a look (@klimek ?).

[klimek]

LG

lib/Format/UnwrappedLineParser.h

private:
   void parseNew();
   void parseAccessSpecifier();
   bool parseEnum();
   void parseJavaEnumBody();
   // Parses a record (aka class) as a top level element. If ParseAsExpr is true,
   // parses the record as a child block, i.e. if the class declaration is an
   // expression.
   void parseRecord(bool ParseAsExpr = false);
+  void parseObjCMethod();
   void parseObjCProtocolList();
   void parseObjCUntilAtEnd();
   void parseObjCInterfaceOrImplementation();
   bool parseObjCProtocol();
   void parseJavaScriptEs6ImportExport();
   bool tryToParseLambda();
   bool tryToParseLambdaIntroducer();
   void tryToParseJSFunction();

lib/Format/UnwrappedLineParser.cpp

if (ParseAsExpr) {
                  /*MunchSemi=*/false);
     }
   }
   // There is no addUnwrappedLine() here so that we fall through to parsing a
   // structural element afterwards. Thus, in "class A {} n, m;",
   // "} n, m;" will end up in one unwrapped line.
 }
 
+void UnwrappedLineParser::parseObjCMethod() {
+  assert(FormatTok->Tok.isOneOf(tok::l_paren, tok::identifier) &&
+         "'(' or identifier expected.");
+  do {
+    if (FormatTok->Tok.is(tok::semi)) {
+      nextToken();
+      addUnwrappedLine();
+      return;
+    } else if (FormatTok->Tok.is(tok::l_brace)) {
+      parseBlock(/*MustBeDeclaration=*/false);
+      addUnwrappedLine();
+      return;
+    } else {
+      nextToken();
+    }
+  } while (!eof());
+}
+
 void UnwrappedLineParser::parseObjCProtocolList() {
   assert(FormatTok->Tok.is(tok::less) && "'<' expected.");
   do {
     nextToken();
     // Early exit in case someone forgot a close angle.
     if (FormatTok->isOneOf(tok::semi, tok::l_brace) ||
         FormatTok->Tok.isObjCAtKeyword(tok::objc_end))
       return;
do {
     if (FormatTok->is(tok::l_brace)) {
       parseBlock(/*MustBeDeclaration=*/false);
       // In ObjC interfaces, nothing should be following the "}".
       addUnwrappedLine();
     } else if (FormatTok->is(tok::r_brace)) {
       // Ignore stray "}". parseStructuralElement doesn't consume them.
       nextToken();
       addUnwrappedLine();
+    } else if (FormatTok->isOneOf(tok::minus, tok::plus)) {
+      nextToken();
+      parseObjCMethod();
     } else {
       parseStructuralElement();
     }
   } while (!eof());
 }
 
 void UnwrappedLineParser::parseObjCInterfaceOrImplementation() {
   assert(FormatTok->Tok.getObjCKeywordID() == tok::objc_interface ||

unittests/Format/FormatTestObjC.cpp

verifyFormat("@interface Foo () {\n"
                "+ (id)init;\n"
                "@end");
 
   verifyFormat("@interface Foo (HackStuff) <MyProtocol> {\n"
                "  int _i;\n"
                "}\n"
                "+ (id)init;\n"
                "@end");
-
+  verifyFormat("@interface Foo\n"
+               "- (void)foo {\n"
+               "}\n"
+               "@end\n"
+               "@implementation Bar\n"
+               "- (void)bar {\n"
+               "}\n"
+               "@end");
   Style.ColumnLimit = 40;
   verifyFormat("@interface ccccccccccccc () <\n"
                "    ccccccccccccc, ccccccccccccc,\n"
                "    ccccccccccccc, ccccccccccccc> {\n"
                "}");
   verifyFormat("@interface ccccccccccccc (ccccccccccc) <\n"
                "    ccccccccccccc> {\n"
                "}");
TEST_F(FormatTestObjC, ObjCCxxKeywords) {
   verifyFormat("+ (instancetype)myDelete {\n"
                "  return [self delete];\n"
                "}\n");
   verifyFormat("SEL DeleteSelector(void) { return @selector(delete); }\n");
   verifyFormat("SEL MacroSelector(void) { return MACRO(delete); }\n");
   verifyFormat("MACRO(new:)\n");
   verifyFormat("MACRO(delete:)\n");
   verifyFormat("foo = @{MACRO(new:) : MACRO(delete:)}\n");
+  verifyFormat("@implementation Foo\n"
+               "// Testing\n"
+               "- (Class)class {\n"
+               "}\n"
+               "- (void)foo {\n"
+               "}\n"
+               "@end\n");
+  verifyFormat("@implementation Foo\n"
+               "- (Class)class {\n"
+               "}\n"
+               "- (void)foo {\n"
+               "}\n"
+               "@end");
+  verifyFormat("@implementation Foo\n"
+               "+ (Class)class {\n"
+               "}\n"
+               "- (void)foo {\n"
+               "}\n"
+               "@end");
+  verifyFormat("@implementation Foo\n"
+               "- (Class)class:(Class)klass {\n"
+               "}\n"
+               "- (void)foo {\n"
+               "}\n"
+               "@end");
+  verifyFormat("@implementation Foo\n"
+               "+ (Class)class:(Class)klass {\n"
+               "}\n"
+               "- (void)foo {\n"
+               "}\n"
+               "@end");
+
+  verifyFormat("@interface Foo\n"
+               "// Testing\n"
+               "- (Class)class;\n"
+               "- (void)foo;\n"
+               "@end\n");
+  verifyFormat("@interface Foo\n"
+               "- (Class)class;\n"
+               "- (void)foo;\n"
+               "@end");
+  verifyFormat("@interface Foo\n"
+               "+ (Class)class;\n"
+               "- (void)foo;\n"
+               "@end");
+  verifyFormat("@interface Foo\n"
+               "- (Class)class:(Class)klass;\n"
+               "- (void)foo;\n"
+               "@end");
+  verifyFormat("@interface Foo\n"
+               "+ (Class)class:(Class)klass;\n"
+               "- (void)foo;\n"
+               "@end");
 }
 
 TEST_F(FormatTestObjC, ObjCLiterals) {
   verifyFormat("@\"String\"");
   verifyFormat("@1");
   verifyFormat("@+4.8");
   verifyFormat("@-4");
   verifyFormat("@1LL");