[clang-format] Improve detection of Objective-C block types

[benhamilton]

Summary

Previously, clang-format would detect the following as an
Objective-C block type:

FOO(^);

when it actually must be a C or C++ macro dealing with an XOR
statement or an XOR operator overload.

According to the Clang Block Language Spec:

https://clang.llvm.org/docs/BlockLanguageSpec.html

block types are of the form:

int (^)(char, float)

and block variables of block type are of the form:

void (^blockReturningVoidWithVoidArgument)(void);
int (^blockReturningIntWithIntAndCharArguments)(int, char);
void (^arrayOfTenBlocksReturningVoidWithIntArgument[10])(int);

This tightens up the detection so we don't unnecessarily detect
C macros which pass in the XOR operator.

Depends On D43904

Test Plan: New tests added. Ran tests with:

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

[djasper]

Would it be enough to only add the block type case? With the macro false positive, there won't be an open paren after the closing paren, right?

lib/Format/TokenAnnotator.cpp

153

This seems suspect. Does it have to be a numeric_constant?

unittests/Format/FormatTest.cpp

12133

I am somewhat skeptical about all these tests now being solely about guessLanguage. It might be the best choice for some of them, but also, there might be other things we do to detect ObjC at some point and then all of these tests become meaningless.

Does your change create a different formatting here?

[krasimir]

unittests/Format/FormatTest.cpp

12131

Please also add formatting tests. This might require changes to the formatting logic since I'd intuitively expect `int(^)(char) to be formatted as int (^)(char)`.

[benhamilton]

• Fix comments from @djasper and @krasimir.

[benhamilton]

Would it be enough to only add the block type case? With the macro false positive, there won't be an open paren after the closing paren, right?

Are you asking to remove the block variable cases? I was concerned those would no longer be handled correctly if we don't explicitly check for them.

What change are you suggesting if we remove the block variable cases to handle those?

lib/Format/TokenAnnotator.cpp

153

Probably not, any constexpr would do, I suspect. What's the best way to parse that?

unittests/Format/FormatTest.cpp

12131

Added formatting tests. Tests passed with no changes to the formatting logic, and your intuition was correct.

12133

I hear you. I just want to make sure the false positive case doesn't regress, and we don't lose the functionality of detecting ObjC for block types.

Added formatting tests.

[krasimir]

unittests/Format/FormatTest.cpp

7692

This should be in FormatTestObjC.cpp.

[djasper]

lib/Format/TokenAnnotator.cpp

153

I think this is the same answer for both of your questions. If what you are trying to prevent "FOO(^)" to be parsed as a block, wouldn't it be enough to look for whether there is a "(" after the ")" or even only after "(^)", everything else is already correct IIUC? That would get you out of need to parse the specifics here, which will be hard.

Or thinking about it another way. Previously, every "(^" would be parsed as an ObjC block. There seems to be only a really rare corner case in which it isn't (macros). Thus, I'd just try to detect that corner case. Instead you are completely inverting the defaults (defaulting to "^" is not a block) and then try to exactly parse ObjC where there might be many cases and edge cases that you won't even think of now.

[benhamilton]

lib/Format/TokenAnnotator.cpp

153

Hmm. Well, it's not just FOO(^); that isn't a block:

#define FOO(X) operator X

SomeType FOO(^)(int x, const SomeType& y) { ... }

Obviously we can't get this perfect without a pre-processor, but it seems like our best bet is to only assign mark TT_ObjCBlockLParen when we are sure the syntax is a valid block type or block variable.

[benhamilton]

lib/Format/TokenAnnotator.cpp

153

I tried the suggestion to only treat (^)( as a block type, but it appears this is the primary place where we set TT_ObjCBlockLParen, so I think we really do need to handle the other cases here.

[djasper]

lib/Format/TokenAnnotator.cpp

153

I don't follow your logic. I'd like you to slowly change this as opposed to completely going the opposite way.

So currently, the only know real-live problem is "FOO(^);". So address this somehow, but still default/error to recognizing too much stuff as a block.

Have you actually seen

SomeType FOO(^)(int x, const SomeType& y) { ... }

in real code?

[benhamilton]

lib/Format/TokenAnnotator.cpp

153

I haven't seen that example, but I have seen FOO(^, OtherParam);.

I'm trying to change the parser to handle this now without explicitly parsing all block types, but it's tricky. The following should be ObjC block types:

(^)(int, char);
(^foo)(int, char);
(^foo[10])(int, char);
(^foo[kNum])(int, char);
(^foo[(kNum + kOtherNum)])(int, char);

but the following should not:

FOO(^);
FOO(^, int, char);

I'll make it work, it's just easy to make a mistake.

[djasper]

Right. So the difference is that for blocks, there is always a "(" after the "(^.....)". That should be easy to parse, no?

[benhamilton]

• Simplify logic and assume we have an ObjC block whenever we see rparen at the end of (^...)(.

[benhamilton]

Right. So the difference is that for blocks, there is always a "(" after the "(^.....)". That should be easy to parse, no?

Yep, I've updated this diff to implement that as you recommended. I just had to handle nested parens (I couldn't find an existing way to deal with these so I wrote my own—let me know if I missed something.)

[benhamilton]

Fix comment.

[djasper]

lib/Format/TokenAnnotator.cpp

150

No. Don't implement yet another parenthesis counting. This function already iterates over all tokens until the closing paren. Just store a pointer to the caret here and mark it (or unmark it) when encountering the closing brace (line 271). There is already very similar logic there to set TT_FunctionTypeLParen (which is actually doing the exact same parsing now that I think of it).

[benhamilton]

• Greatly clean up ObjC block type logic by re-using TT_FunctionTypeLParen type logic.

[benhamilton]

lib/Format/TokenAnnotator.cpp

150

Oh! Yes, re-using the logic for TT_FunctionTypeLParen is clearly the right thing to do, since it has exactly the same syntax (just * or & instead of ^).

Done, code is much cleaner now.

[djasper]

Nice.. Removed a lot of complexity :). Let's see whether this heuristic is good enough.

lib/Format/TokenAnnotator.cpp

218–219

I'd suggest to put a comment here saying that this is for both ObjC blocks and Function types, because they look very similar in nature (maybe giving examples) and then not actually rename the variables. To me, the long names make the code harder to read.

But if you feel strongly the other way, I'd be ok with it.

[benhamilton]

• Restore short functionn type variable names and add clarifying comment.

[benhamilton]

lib/Format/TokenAnnotator.cpp

218–219

Restored old names and added comment.

lib/Format/TokenAnnotator.cpp

bool parseParens(bool LookForDecls = false) {
     Left->ParentBracket = Contexts.back().ContextKind;
     ScopedContextCreator ContextCreator(*this, tok::l_paren, 1);
 
     // FIXME: This is a bit of a hack. Do better.
     Contexts.back().ColonIsForRangeExpr =
         Contexts.size() == 2 && Contexts[0].ColonIsForRangeExpr;
 
     bool StartsObjCMethodExpr = false;
-    if (CurrentToken->is(tok::caret)) {
-      // (^ can start a block type.
-      Left->Type = TT_ObjCBlockLParen;
-    } else if (FormatToken *MaybeSel = Left->Previous) {
+    if (FormatToken *MaybeSel = Left->Previous) {
       // @selector( starts a selector.
       if (MaybeSel->isObjCAtKeyword(tok::objc_selector) && MaybeSel->Previous &&
           MaybeSel->Previous->is(tok::at)) {
         StartsObjCMethodExpr = true;
       }
     }
 
     if (Left->is(TT_OverloadedOperatorLParen)) {
       Contexts.back().IsExpression = false;
     } else if (Style.Language == FormatStyle::LK_JavaScript &&
                (Line.startsWith(Keywords.kw_type, tok::identifier) ||
                 Line.startsWith(tok::kw_export, Keywords.kw_type,
                                 tok::identifier))) {
       // type X = (...);
       // export type X = (...);
       Contexts.back().IsExpression = false;
     } else if (Left->Previous &&
if (Left->is(TT_OverloadedOperatorLParen)) {
       Contexts.back().IsExpression = !IsForOrCatch;
     }
 
     if (StartsObjCMethodExpr) {
       Contexts.back().ColonIsObjCMethodExpr = true;
       Left->Type = TT_ObjCMethodExpr;
     }
 
+    // MightBeFunctionType and ProbablyFunctionType are used for
+    // function pointer and reference types as well as Objective-C
+    // block types:
+    //
+    // void (*FunctionPointer)(void);
+    // void (&FunctionReference)(void);
+    // void (^ObjCBlock)(void);
     bool MightBeFunctionType = !Contexts[Contexts.size() - 2].IsExpression;
-    bool ProbablyFunctionType = CurrentToken->isOneOf(tok::star, tok::amp);
+    bool ProbablyFunctionType =
+        CurrentToken->isOneOf(tok::star, tok::amp, tok::caret);
     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
while (CurrentToken) {
       if (CurrentToken->is(tok::comma))
         MightBeFunctionType = false;
       if (CurrentToken->Previous->is(TT_BinaryOperator))
         Contexts.back().IsExpression = true;
       if (CurrentToken->is(tok::r_paren)) {
         if (MightBeFunctionType && ProbablyFunctionType && CurrentToken->Next &&
             (CurrentToken->Next->is(tok::l_paren) ||
              (CurrentToken->Next->is(tok::l_square) && Line.MustBeDeclaration)))
-          Left->Type = TT_FunctionTypeLParen;
+          Left->Type = Left->Next->is(tok::caret) ? TT_ObjCBlockLParen
+                                                  : TT_FunctionTypeLParen;
         Left->MatchingParen = CurrentToken;
         CurrentToken->MatchingParen = Left;
 
         if (CurrentToken->Next && CurrentToken->Next->is(tok::l_brace) &&
             Left->Previous && Left->Previous->is(tok::l_paren)) {
           // Detect the case where macros are used to generate lambdas or
           // function bodies, e.g.:
           //   auto my_lambda = MARCO((Type *type, int i) { .. body .. });

unittests/Format/FormatTest.cpp

TEST_F(FormatTest, FormatForObjectiveCMethodDecls) {
 
   // If there's no return type (very rare in practice!), LLVM and Google style
   // agree.
   verifyFormat("- foo;");
   verifyFormat("- foo:(int)f;");
   verifyGoogleFormat("- foo:(int)foo;");
 }
 
 
 TEST_F(FormatTest, BreaksStringLiterals) {
   EXPECT_EQ("\"some text \"\n"
             "\"other\";",
             format("\"some text other\";", getLLVMStyleWithColumns(12)));
   EXPECT_EQ("\"some text \"\n"
             "\"other\";",
             format("\\\n\"some text other\";", getLLVMStyleWithColumns(12)));
   EXPECT_EQ(
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, GuessLanguageWithCaret) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^, Bar);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+            guessLanguage("foo.h", "int(^)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+            guessLanguage("foo.h", "int(^foo)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+            guessLanguage("foo.h", "int(^foo[10])(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+            guessLanguage("foo.h", "int(^foo[kNumEntries])(char, float);"));
+  EXPECT_EQ(
+      FormatStyle::LK_ObjC,
+      guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang

unittests/Format/FormatTestObjC.cpp

TEST_F(FormatTestObjC, ObjCAt) {
 
   EXPECT_EQ("@interface", format("@ interface"));
 
   // The precise formatting of this doesn't matter, nobody writes code like
   // this.
   verifyFormat("@ /*foo*/ interface");
 }
 
+TEST_F(FormatTestObjC, ObjCBlockTypesAndVariables) {
+  verifyFormat("void DoStuffWithBlockType(int (^)(char));");
+  verifyFormat("int (^foo)(char, float);");
+  verifyFormat("int (^foo[10])(char, float);");
+  verifyFormat("int (^foo[kNumEntries])(char, float);");
+  verifyFormat("int (^foo[kNumEntries + 10])(char, float);");
+  verifyFormat("int (^foo[(kNumEntries + 10)])(char, float);");
+}
+
 TEST_F(FormatTestObjC, ObjCSnippets) {
   verifyFormat("@autoreleasepool {\n"
                "  foo();\n"
                "}");
   verifyFormat("@class Foo, Bar;");
   verifyFormat("@compatibility_alias AliasName ExistingClass;");
   verifyFormat("@dynamic textColor;");
   verifyFormat("char *buf1 = @encode(int *);");