[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

12137

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

12137

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

778

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

[benhamilton]

• Move ObjC-specific tests to FormatTestObjC.cpp.

unittests/Format/FormatTest.cpp

778

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;
+      }
+      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

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, ObjCForInLoop) {
+  verifyFormat("for (Foo *x = 0; x != in; x++) {\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}");
+  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}");
+}
+
 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"
                "}");
 
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