[clang-format] Fix ObjC keywords following try/catch getting split.

[Bigcheese]

Summary

Previously things like:

int main() {
  @try {
  } @catch (NSException *e) {
  }

  @try {
  } @catch (NSException *e) {
  }
}

Would be formatted like:

int main() {
  @try {
  } @catch (NSException *e) {
  }

  @
  try {
  } @catch (NSException *e) {
  }
}

because UnwrappedLineParser::parseTryCatch() would consume the @ as
part of checking for another catch or finally. This patch fixes that
by doing a lookahead.

I'm not super happy about the way the lookhead works, but this is the only thing that worked.

[merge_guards_bot]

Build result: pass - 60659 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

1849

can you use FormatTok->getNextNonComment()?

clang/unittests/Format/FormatTestObjC.cpp

200

Nit: Could you not keep the original test? just add a new testcase? I get uncomfortable about changing tests no matter how trivial

[Bigcheese]

clang/lib/Format/UnwrappedLineParser.cpp

1849

No, Next has not been setup at this point, so it will always return nullptr.

clang/unittests/Format/FormatTestObjC.cpp

200

Is there something special about clang-format that causes this concern? I'm all for testing separate things separately, but the additions to this test are testing the same leaf lines of code.

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

1849

There is something about what we are doing here which doesn't feel correct...If you look in FormatTokenLexer why are we not doing something similar here for @try and @catch as we are doing for @"ABC" (see below tryMergeNSStringLiteral)

The FormatTokenLexer can merge together a number of tokens into a new single token and combined text, after that nothing can split that token, meaning the @ and try will never become separated and will simply be treated as a "@try"

This can be useful because you can set the new setKind() to be "tok::try" after which @try will behave just like a normal try for all rules without the constant need to keep checking if the previous token is an @

bool FormatTokenLexer::tryMergeNSStringLiteral() {
  if (Tokens.size() < 2)
    return false;
  auto &At = *(Tokens.end() - 2);
  auto &String = *(Tokens.end() - 1);
  if (!At->is(tok::at) || !String->is(tok::string_literal))
    return false;
  At->Tok.setKind(tok::string_literal);
  At->TokenText = StringRef(At->TokenText.begin(),
                            String->TokenText.end() - At->TokenText.begin());
  At->ColumnWidth += String->ColumnWidth;
  At->Type = TT_ObjCStringLiteral;
  Tokens.erase(Tokens.end() - 1);
  return true;
}

clang/unittests/Format/FormatTestObjC.cpp

200

as you are scanning back, I'd like to know we haven't broken the previous tests behaviour.

[MyDeveloperDay]

bool FormatTokenLexer::tryMergeAtTry() {
  if (Tokens.size() < 2)
    return false;
  auto &At = *(Tokens.end() - 2);
  auto &Try = *(Tokens.end() - 1);
  if (!At->is(tok::at) || !String->is(tok::try))
    return false;
  At->Tok.setKind(tok::try);
  At->TokenText = StringRef(At->TokenText.begin(),
                            String->TokenText.end() - At->TokenText.begin());
  At->ColumnWidth += String->ColumnWidth;
  Tokens.erase(Tokens.end() - 1);
  return true;
}

bool FormatTokenLexer::tryMergeAtCatch() {
  if (Tokens.size() < 2)
    return false;
  auto &At = *(Tokens.end() - 2);
  auto &Try = *(Tokens.end() - 1);
  if (!At->is(tok::at) || !String->is(tok::catch))
    return false;
  At->Tok.setKind(tok::catch);
  At->TokenText = StringRef(At->TokenText.begin(),
                            String->TokenText.end() - At->TokenText.begin());
  At->ColumnWidth += String->ColumnWidth;
  Tokens.erase(Tokens.end() - 1);
  return true;
}

I think adding these 2 functions could help to always treat @try and @catch the same as you we do try and catch (and any rules they have like BeforeCatch etc..

Did you consider doing that or are you not pursuing this patch any more?

clang/lib/Format/UnwrappedLineParser.cpp

if (FormatTok->is(tok::l_brace)) {
     // If there's none, we try to assume there's a structuralElement
     // and try to continue.
     addUnwrappedLine();
     ++Line->Level;
     parseStructuralElement();
     --Line->Level;
   }
   while (1) {
-    if (FormatTok->is(tok::at))
-      nextToken();
+    unsigned StoredPosition = Tokens->getPosition();
+    if (FormatTok->is(tok::at)) {
+      // Get next non-comment token.
+      do {
+        FormatTok = Tokens->getNextToken();
+      } while (FormatTok->is(tok::comment));
+    }
     if (!(FormatTok->isOneOf(tok::kw_catch, Keywords.kw___except,
                              tok::kw___finally) ||
           ((Style.Language == FormatStyle::LK_Java ||
             Style.Language == FormatStyle::LK_JavaScript) &&
            FormatTok->is(Keywords.kw_finally)) ||
           (FormatTok->Tok.isObjCAtKeyword(tok::objc_catch) ||
-           FormatTok->Tok.isObjCAtKeyword(tok::objc_finally))))
+           FormatTok->Tok.isObjCAtKeyword(tok::objc_finally)))) {
+      // Go back to before the (optional) @.
+      FormatTok = Tokens->setPosition(StoredPosition);
       break;
+    }
+    FormatTok = Tokens->setPosition(StoredPosition);
+    if (FormatTok->is(tok::at))
+      nextToken();
     nextToken();
     while (FormatTok->isNot(tok::l_brace)) {
       if (FormatTok->is(tok::l_paren)) {
         parseParens();
         continue;
       }
       if (FormatTok->isOneOf(tok::semi, tok::r_brace, tok::eof))
         return;

clang/unittests/Format/FormatTestObjC.cpp

 
 TEST_F(FormatTestObjC, FormatObjCTryCatch) {
   verifyFormat("@try {\n"
                "  f();\n"
                "} @catch (NSException e) {\n"
                "  @throw;\n"
                "} @finally {\n"
                "  exit(42);\n"
-               "}");
+               "}\n"
+               "@try {\n"
+               "} @catch (NSException *e) {\n"
+               "}\n"
+               "@ /* adena */ try {\n"
+               "} @catch (NSException *e) {\n"
+               "}\n");
   verifyFormat("DEBUG({\n"
                "  @try {\n"
                "  } @finally {\n"
                "  }\n"
                "});\n");
 }
 
 TEST_F(FormatTestObjC, FormatObjCAutoreleasepool) {