[clang-format] Refine ObjC guesser to handle child lines of child lines

[benhamilton]

Summary

This fixes an issue brought up by djasper@ in his review of D44790. We
handled top-level child lines, but if those child lines themselves
had child lines, we didn't handle them.

Rather than use recursion (which could blow out the stack), I use a
DenseSet to hold the set of lines we haven't yet checked (since order
doesn't matter), and update the set to add the children of each
line as we check it.

Test Plan: New tests added. Confirmed tests failed before fix

and passed after fix.

[benhamilton]

Update from correct base revision

[djasper]

lib/Format/Format.cpp

1525–1542

Wouldn't it be much easier to call this function recursively for Children instead of using the lambda as well as this additional set? Lines and their children should form a tree, I think, so you should never see the same line again during recursion.

[benhamilton]

lib/Format/Format.cpp

1525–1542

I'm less worried about cycles and more worried about a maliciously deeply nested set of children blowing out the stack and causing a crash.

It seemed safer to use the heap here to avoid that scenario (I don't know of any other way to avoid it, since we don't know the stack size and we don't control it.)

[benhamilton]

lib/Format/Format.cpp

1525–1542

djasper@ informed me that clang-format is already full of places which rely on the stack for recursion on user input, so that window has already been broken. :)

I'll happily go ahead and change this.

[benhamilton]

• Use recursion. Split lambda out into its own static method since recursion on lambdas is quite ugly until C++14

[benhamilton]

• Remove stray semicolon.

[djasper]

lib/Format/Format.cpp

1449

I would not create a second function here. Just iterate over the tokens here and call guessIsObjC recursively with Line->Children. That means we need one less for loop overall, I think (I might be missing something).

1455

Convention would be lineContainsObjCCode.

[benhamilton]

• Simplify recursion. Remove static method LineContainsObjCCode().

lib/Format/Format.cpp

1449

Good call, done.

I don't know why I didn't do that first! For some reason, my brain thought Line.Children was an incompatible type..

1455

Removed.

[djasper]

Looks good.

lib/Format/Format.cpp

analyze(TokenAnnotator &Annotator,
     return {Result, 0};
   }
 
   bool isObjC() { return IsObjC; }
 
 private:
   static bool guessIsObjC(const SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
                           const AdditionalKeywords &Keywords) {
+    for (auto Line : AnnotatedLines)
+      if (LineContainsObjCCode(*Line, Keywords))
+        return true;
+    return false;
+  }
+
+  static bool LineContainsObjCCode(const AnnotatedLine &Line,
+                                   const AdditionalKeywords &Keywords) {
     // Keep this array sorted, since we are binary searching over it.
     static constexpr llvm::StringLiteral FoundationIdentifiers[] = {
         "CGFloat",
         "CGPoint",
         "CGPointMake",
         "CGPointZero",
         "CGRect",
         "CGRectEdge",
static constexpr llvm::StringLiteral FoundationIdentifiers[] = {
         "NSURLComponents",
         "NSURLQueryItem",
         "NSUUID",
         "NSValue",
         "UIImage",
         "UIView",
     };
 
-    auto LineContainsObjCCode = [&Keywords](const AnnotatedLine &Line) {
-      for (const FormatToken *FormatTok = Line.First; FormatTok;
-           FormatTok = FormatTok->Next) {
-        if ((FormatTok->Previous && FormatTok->Previous->is(tok::at) &&
-             (FormatTok->isObjCAtKeyword(tok::objc_interface) ||
-              FormatTok->isObjCAtKeyword(tok::objc_implementation) ||
-              FormatTok->isObjCAtKeyword(tok::objc_protocol) ||
-              FormatTok->isObjCAtKeyword(tok::objc_end) ||
-              FormatTok->isOneOf(tok::numeric_constant, tok::l_square,
-                                 tok::l_brace))) ||
-            (FormatTok->Tok.isAnyIdentifier() &&
-             std::binary_search(std::begin(FoundationIdentifiers),
-                                std::end(FoundationIdentifiers),
-                                FormatTok->TokenText)) ||
-            FormatTok->is(TT_ObjCStringLiteral) ||
-            FormatTok->isOneOf(Keywords.kw_NS_ENUM, Keywords.kw_NS_OPTIONS,
-                               TT_ObjCBlockLBrace, TT_ObjCBlockLParen,
-                               TT_ObjCDecl, TT_ObjCForIn, TT_ObjCMethodExpr,
-                               TT_ObjCMethodSpecifier, TT_ObjCProperty)) {
-          return true;
-        }
-      }
-      return false;
-    };
-    for (auto Line : AnnotatedLines) {
-      if (LineContainsObjCCode(*Line))
-        return true;
-      for (auto ChildLine : Line->Children) {
-        if (LineContainsObjCCode(*ChildLine))
+      for (auto ChildLine : Line.Children)
+        if (LineContainsObjCCode(*ChildLine, Keywords))
           return true;
-      }
-    }
     return false;
-  }
+  };
 
   bool IsObjC;
 };
 
 struct IncludeDirective {
   StringRef Filename;
   StringRef Text;
   unsigned Offset;

unittests/Format/FormatTest.cpp

EXPECT_EQ(
       guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);"));
 }
 
 TEST_F(FormatTest, GuessLanguageWithChildLines) {
   EXPECT_EQ(FormatStyle::LK_Cpp,
             guessLanguage("foo.h", "#define FOO ({ std::string s; })"));
   EXPECT_EQ(FormatStyle::LK_ObjC,
             guessLanguage("foo.h", "#define FOO ({ NSString *s; })"));
+  EXPECT_EQ(
+      FormatStyle::LK_Cpp,
+      guessLanguage("foo.h", "#define FOO ({ foo(); ({ std::string s; }) })"));
+  EXPECT_EQ(
+      FormatStyle::LK_ObjC,
+      guessLanguage("foo.h", "#define FOO ({ foo(); ({ NSString *s; }) })"));
 }
 
 } // end namespace
 } // end namespace format
 } // end namespace clang