[clang-format] Improve detection of ObjC for-in statements

[benhamilton]

Summary

Previously, clang-format would detect the following as an
Objective-C for-in statement:

for (int x = in.value(); ...) {}

because the logic only decided a for-loop was definitely *not*
an Objective-C for-in loop after it saw a semicolon or a colon.

To fix this, I delayed the decision of whether this was a for-in
statement until after we found the matching right-paren, at which
point we know if we've seen a semicolon or not.

Test Plan: New tests added. Ran tests with:

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

[djasper]

lib/Format/TokenAnnotator.cpp

289

There can be only one ObjCForIn token in any for loop, right? If that's the case, can we just remember that in addition to (or instead of) MightBeObjCForRangeLoop? That way, we wouldn't have to re-iterate over all the tokens here, but could just set this directly.

[krasimir]

unittests/Format/FormatTest.cpp

12129

Please also add this instances as formatting tests.

[benhamilton]

• Fix comments from @djasper and @krasimir.

[benhamilton]

One more cleanup

lib/Format/TokenAnnotator.cpp

289

Nice optimization, done.

unittests/Format/FormatTest.cpp

12129

Thanks, tests added.

The new formatting tests revealed a new regression I'd introduced: we were not inserting a space between keyword 'in' and the opening [ of the ObjC message send.

This was because the ObjC message send parsing logic depended on the TT_ObjCForIn keyword being set before parsing the [, but by delaying setting the type until after parsing the for-loop was complete, we broke that logic.

Fixed regression and made sure tests passed.

[krasimir]

unittests/Format/FormatTest.cpp

784

Please move the ObjC-specific instances to FormatTestObjC.cpp.

[benhamilton]

• Move ObjC-specific tests to FormatTestObjC.cpp.

unittests/Format/FormatTest.cpp

784

Done.

lib/Format/TokenAnnotator.cpp

bool parseParens(bool LookForDecls = false) {
     }
 
     bool MightBeFunctionType = !Contexts[Contexts.size() - 2].IsExpression;
     bool ProbablyFunctionType = CurrentToken->isOneOf(tok::star, tok::amp);
     bool HasMultipleLines = false;
     bool HasMultipleParametersOnALine = false;
     bool MightBeObjCForRangeLoop =
         Left->Previous && Left->Previous->is(tok::kw_for);
+    FormatToken *PossibleObjCForInToken = nullptr;
     while (CurrentToken) {
       // LookForDecls is set when "if (" has been seen. Check for
       // 'identifier' '*' 'identifier' followed by not '=' -- this
       // '*' has to be a binary operator but determineStarAmpUsage() will
       // categorize it as an unary operator, so set the right type here.
       if (LookForDecls && CurrentToken->Next) {
         FormatToken *Prev = CurrentToken->getPreviousNonComment();
         if (Prev) {
while (CurrentToken) {
 
         if (!HasMultipleLines)
           Left->PackingKind = PPK_Inconclusive;
         else if (HasMultipleParametersOnALine)
           Left->PackingKind = PPK_BinPacked;
         else
           Left->PackingKind = PPK_OnePerLine;
 
         next();
         return true;
       }
       if (CurrentToken->isOneOf(tok::r_square, tok::r_brace))
         return false;
 
       if (CurrentToken->is(tok::l_brace))
         Left->Type = TT_Unknown; // Not TT_ObjCBlockLParen
       if (CurrentToken->is(tok::comma) && CurrentToken->Next &&
           !CurrentToken->Next->HasUnescapedNewline &&
           !CurrentToken->Next->isTrailingComment())
         HasMultipleParametersOnALine = true;
       if ((CurrentToken->Previous->isOneOf(tok::kw_const, tok::kw_auto) ||
            CurrentToken->Previous->isSimpleTypeSpecifier()) &&
           !CurrentToken->is(tok::l_brace))
         Contexts.back().IsExpression = false;
-      if (CurrentToken->isOneOf(tok::semi, tok::colon))
+      if (CurrentToken->isOneOf(tok::semi, tok::colon)) {
         MightBeObjCForRangeLoop = false;
-      if (MightBeObjCForRangeLoop && CurrentToken->is(Keywords.kw_in))
-        CurrentToken->Type = TT_ObjCForIn;
+        if (PossibleObjCForInToken) {
+          PossibleObjCForInToken->Type = TT_Unknown;
+          PossibleObjCForInToken = nullptr;
+        }
+      }
+      if (MightBeObjCForRangeLoop && CurrentToken->is(Keywords.kw_in)) {
+        PossibleObjCForInToken = CurrentToken;
+        PossibleObjCForInToken->Type = TT_ObjCForIn;
+      }
       // When we discover a 'new', we set CanBeExpression to 'false' in order to
       // parse the type correctly. Reset that after a comma.
       if (CurrentToken->is(tok::comma))
         Contexts.back().CanBeExpression = true;
 
       FormatToken *Tok = CurrentToken;
       if (!consumeToken())
         return false;

unittests/Format/FormatTest.cpp

verifyFormat("for (some_namespace::SomeIterator iter( // force break\n"
                "         aaaaaaaaaa);\n"
                "     iter; ++iter) {\n"
                "}");
   verifyFormat("for (auto aaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
                "         aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);\n"
                "     aaaaaaaaaaaaaaaaaaaaaaaaaaa != bbbbbbbbbbbbbbbbbbbbbbb;\n"
                "     ++aaaaaaaaaaaaaaaaaaaaaaaaaaa) {");
 
+  // These should not be formatted as Objective-C for-in loops.
+  verifyFormat("for (Foo *x = 0; x != in; x++) {\n}");
+  verifyFormat("Foo *x;\nfor (x = 0; x != in; x++) {\n}");
+  verifyFormat("Foo *x;\nfor (x in y) {\n}");
+  verifyFormat("for (const Foo<Bar> &baz = in.value(); !baz.at_end(); ++baz) {\n}");
+
   FormatStyle NoBinPacking = getLLVMStyle();
   NoBinPacking.BinPackParameters = false;
   verifyFormat("for (int aaaaaaaaaaa = 1;\n"
                "     aaaaaaaaaaa <= aaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaa,\n"
                "                                           aaaaaaaaaaaaaaaa,\n"
                "                                           aaaaaaaaaaaaaaaa,\n"
                "                                           aaaaaaaaaaaaaaaa);\n"
                "     aaaaaaaaaaa++, bbbbbbbbbbbbbbbbb++) {\n"
TEST_F(FormatTest, RangeBasedForLoops) {
   verifyFormat("for (auto aaaaaaaaaaaaaaaaaaaaa :\n"
                "     aaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaa, aaaaaaaaaaaaa)) {\n}");
   verifyFormat("for (const aaaaaaaaaaaaaaaaaaaaa &aaaaaaaaa :\n"
                "     aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n}");
   verifyFormat("for (aaaaaaaaa aaaaaaaaaaaaaaaaaaaaa :\n"
                "     aaaaaaaaaaaa.aaaaaaaaaaaa().aaaaaaaaa().a()) {\n}");
 }
 
 TEST_F(FormatTest, ForEachLoops) {
   verifyFormat("void f() {\n"
                "  foreach (Item *item, itemlist) {}\n"
                "  Q_FOREACH (Item *item, itemlist) {}\n"
                "  BOOST_FOREACH (Item *item, itemlist) {}\n"
                "  UNKNOWN_FORACH(Item * item, itemlist) {}\n"
                "}");
 
   // As function-like macros.
EXPECT_EQ(FormatStyle::LK_Cpp,
             guessLanguage("foo.h", "[[using gsl: suppress(\"type\")]];"));
   EXPECT_EQ(
       FormatStyle::LK_Cpp,
       guessLanguage("foo.h",
                     "[[clang::callable_when(\"unconsumed\", \"unknown\")]]"));
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithForIn) {
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+            guessLanguage("foo.h", "for (Foo *x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+            guessLanguage("foo.h", "for (Foo *x in bar) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+            guessLanguage("foo.h", "for (Foo *x in [bar baz]) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+            guessLanguage("foo.h", "for (Foo *x in [bar baz:blech]) {}"));
+  EXPECT_EQ(
+      FormatStyle::LK_ObjC,
+      guessLanguage("foo.h", "for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+            guessLanguage("foo.h", "for (Foo *x in [bar baz:^{[uh oh];}]) {}"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+            guessLanguage("foo.h", "Foo *x; for (x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+            guessLanguage("foo.h", "Foo *x; for (x in y) {}"));
+  EXPECT_EQ(
+      FormatStyle::LK_Cpp,
+      guessLanguage(
+          "foo.h",
+          "for (const Foo<Bar>& baz = in.value(); !baz.at_end(); ++baz) {}"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang

unittests/Format/FormatTestObjC.cpp

verifyFormat("- (void)test {\n"
                "    foo(n);\n"
                "  }\n"
                "}");
   verifyFormat("- (void)test {\n"
                "  for (NSString *n in (__bridge NSArray *)arrayOfStrings) {\n"
                "    foo(n);\n"
                "  }\n"
                "}");
+  verifyFormat("for (Foo *x in bar) {\n}");
+  verifyFormat("for (Foo *x in [bar baz]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:blech]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:^{\n"
+               "       [uh oh];\n"
+               "     }]) {\n}");
 }
 
 TEST_F(FormatTestObjC, ObjCLiterals) {
   verifyFormat("@\"String\"");
   verifyFormat("@1");
   verifyFormat("@+4.8");
   verifyFormat("@-4");
   verifyFormat("@1LL");