[clang-format] Don't detect call to ObjC class method as C++11 attribute specifier

[rkgibson2]

Summary

Previously, clang-format detected something like the following as a C++11 attribute specifier.

@[[NSArray class]]

instead of an array with an Objective-C method call inside. In general, when the attribute specifier checking runs, if it sees 2 identifiers in a row, it decides that the square brackets represent an Objective-C method call. However, here, class is tokenized as a keyword instead of an identifier, so this check fails.

To fix this, the attribute specifier first checks whether the first square bracket has an "@" before it. If it does, then that square bracket is not the start of a attribute specifier because it is an Objective-C array literal. (The assumption is that @[[.*]] is not valid C/C++.)

[benhamilton]

Thanks for the fix. One question: how does the real Clang parser deal with this case? Is it something that's actually ambiguous in the ObjC++ grammar, I wonder?

[benhamilton]

clang/lib/Format/TokenAnnotator.cpp

400–413 ↗

(On Diff #209477)

Maybe we should check the token before AttrTok to see if it's tok::at, rather than checking for an identifier followed by tok::kw_class?

I don't think there's any valid C++11 attribute specifier sequence of @[[]].

[rkgibson2]

In D64632#1582793, @benhamilton wrote:

Thanks for the fix. One question: how does the real Clang parser deal with this case? Is it something that's actually ambiguous in the ObjC++ grammar, I wonder?

I am not sure about real Clang. I actually don't know much about how it works. I just ran into this bug and wanted to see if it was fixable. My guess is the @ sign marks the bracket as an array literal. In clang-format, isCpp11AttributeSpecifier is checked before checking whether the previous token is @.

clang/lib/Format/TokenAnnotator.cpp

400–413 ↗

(On Diff #209477)

Ok, sure. I considered that at first but I'm not familiar with C++11 enough to say. I thought it wasn't valid, but I wasn't sure.

[rkgibson2]

Respond to comments and change detection method

[aaron.ballman]

clang/lib/Format/TokenAnnotator.cpp

389 ↗

(On Diff #209496)

Clang has a feature flag to enable support for double-square bracket attributes in more than just C++ mode, and this is enabled by default in C2x mode. This check for isCpp() makes me suspect we may be doing the wrong thing here.

[rkgibson2]

Update comment

[benhamilton]

Thanks!

[benhamilton]

clang/unittests/Format/FormatTest.cpp

7030–7032 ↗

(On Diff #209504)

Consider adding another test for a method besides +class.

[rkgibson2]

Add more tests

[rkgibson2]

I don't think I have commit access. Could someone land this for me?

[benhamilton]

Submitted as r366267. Thanks!

clang/lib/Format/TokenAnnotator.cpp

389 ↗

(On Diff #209496)

Good point. I filed https://bugs.llvm.org/show_bug.cgi?id=42645 to revisit this.

cfe/trunk/lib/Format/TokenAnnotator.cpp

if (AttrTok->isOneOf(tok::kw_public, tok::kw_private, tok::kw_protected,
       return true;
     }
     return false;
   }
 
   bool isCpp11AttributeSpecifier(const FormatToken &Tok) {
     if (!Style.isCpp() || !Tok.startsSequence(tok::l_square, tok::l_square))
       return false;
+    // The first square bracket is part of an ObjC array literal
+    if (Tok.Previous && Tok.Previous->is(tok::at)) {
+      return false;
+    }
     const FormatToken *AttrTok = Tok.Next->Next;
     if (!AttrTok)
       return false;
     // C++17 '[[using ns: foo, bar(baz, blech)]]'
     // We assume nobody will name an ObjC variable 'using'.
     if (AttrTok->startsSequence(tok::kw_using, tok::identifier, tok::colon))
       return true;
     if (AttrTok->isNot(tok::identifier))
       return false;
     while (AttrTok && !AttrTok->startsSequence(tok::r_square, tok::r_square)) {
       // ObjC message send. We assume nobody will use : in a C++11 attribute
       // specifier parameter, although this is technically valid:
-      // [[foo(:)]]
+      // [[foo(:)]].
       if (AttrTok->is(tok::colon) ||
           AttrTok->startsSequence(tok::identifier, tok::identifier) ||
           AttrTok->startsSequence(tok::r_paren, tok::identifier))
         return false;
       if (AttrTok->is(tok::ellipsis))
         return true;
       AttrTok = AttrTok->Next;
     }

cfe/trunk/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsSquareAttributes) {
   verifyFormat("int a() {}\n"
                "[[unused]] int b() {}\n");
   verifyFormat("NSArray *arr;\n"
                "arr[[Foo() bar]];");
 
   // On the other hand, we still need to correctly find array subscripts.
   verifyFormat("int a = std::vector<int>{1, 2, 3}[0];");
 
+  // Make sure that we do not mistake Objective-C method inside array literals
+  // as attributes, even if those method names are also keywords.
+  verifyFormat("@[ [foo bar] ];");
+  verifyFormat("@[ [NSArray class] ];");
+  verifyFormat("@[ [foo enum] ];");
+
   // Make sure we do not parse attributes as lambda introducers.
   FormatStyle MultiLineFunctions = getLLVMStyle();
   MultiLineFunctions.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
   verifyFormat("[[unused]] int b() {\n"
                "  return 42;\n"
                "}\n",
                MultiLineFunctions);
 }