[clang-format] Don't detect C++11 attribute specifiers as ObjC

[benhamilton]

Summary

Previously, clang-format would detect C++11 and C++17 attribute
specifiers like the following as Objective-C method invocations:

[[noreturn]];
[[clang::fallthrough]];
[[noreturn, deprecated("so sorry")]];
[[using gsl: suppress("type")]];

To fix this, I ported part of the logic from
tools/clang/lib/Parse/ParseTentative.cpp into TokenAnnotator.cpp so we
can explicitly parse and identify C++11 attribute specifiers.

This allows the guessLanguage() and getStyle() APIs to correctly
guess files containing the C++11 attribute specifiers as C++,
not Objective-C.

Test Plan: New tests added. Ran tests with:

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

[aaron.ballman]

lib/Format/TokenAnnotator.cpp

346

C can use this syntax as well with -fdouble-square-bracket-attributes, which would be good to also support.

[benhamilton]

lib/Format/TokenAnnotator.cpp

346

Ah, good to know. As far as I know, clang-format doesn't actually distinguish between C and C++ (or Objective-C and Objective-C++).

In this case, isCpp() returns true for all of C, C++, Objective-C, and Objective-C++:

https://github.com/llvm-mirror/clang/blob/master/include/clang/Format/Format.h#L1229

[jolesiak]

lib/Format/TokenAnnotator.cpp

323

I feel like it would be clearer (and more consistent) if we used const& here and just make a copy inside function body.
I see no benefit of passing nullptr based on usage presented in this patch.

325

For consistency reasons I wouldn't use one line ifs. Let's keep existing format with line break and without braces.

337

Is second part of this condition necessary?

345

Pointer should be to const type, but same as above, I feel like const& would be a better choice.

351

How about:

bool NamespaceAllowed;
if (AttributeToken->startsSequence(tok::kw_using, tok::identifier,
                                   tok::colon)) {
  // C++17 '[[using namespace: foo, bar(baz, blech)]]'
  AttributeToken = AttributeToken->Next->Next->Next;
  NamespaceAllowed = false;
} else {
  // C++11 '[[namespace::foo, namespace::bar(baz, blech)]]'
  NamespaceAllowed = true;
}
while (AttributeToken) {
  AttributeToken =
      parseCpp11Attribute(AttributeToken, NamespaceAllowed);
  if (!AttributeToken || AttributeToken->isNot(tok::comma)) break;
  AttributeToken = AttributeToken->Next;
}

[benhamilton]

Fixes from @jolesiak

[benhamilton]

Thanks very much for the code review. I fixed those issues.

lib/Format/TokenAnnotator.cpp

323

Fixed.

325

Fixed. (My mistake was using git-clang-format without passing --style llvm — for some reason that's applying a different style.)

337

Not needed. Removed.

345

Fixed.

351

Fixed.

[djasper]

lib/Format/TokenAnnotator.cpp

352

Just join this if with the previous one.

357

Replace lines 355-365 with:

bool IsUsingNamespace = AttrTok && AttrTok->startsSequence( ... );
if (IsUsingNamespace)
  AttrTok = AttrTok->Next->Next->Next;

374

Why not replace these three lines with:

return AttrTok && AttrTok->startsSequence(..);

400

This seems weird. I think if we return true, we should have consumed everything up to the closing r_square.

unittests/Format/FormatTest.cpp

12083

You are not adding any test (AFAICT) that tests that you have correctly set TT_AttributeSpecifier or that you are making any formatting decisions based on it. If you can't test it, remove that part of this patch.

[krasimir]

lib/Format/TokenAnnotator.cpp

323

Please inline this into the other function: it's used only once and its name and arguments aren't clear outside of that context.

unittests/Format/FormatTest.cpp

12083

This will also require adding tests about not messing up the formatting inside/around attribute specifiers too much. At least take these examples and copy them to the C++/ObjC formatting unittests.

[benhamilton]

• Fix comments from @krasimir and @djasper.

[benhamilton]

lib/Format/TokenAnnotator.cpp

323

OK, made this a lambda.

352

Fixed.

357

Fixed.

374

Fixed.

400

That's super useful feedback, I wasn't sure what the contract of this method was.

Fixed to consume everything including the closing r_square. (I actually had to consume that too, or parsing was left with an un-parsed value and parsing would fail.)

unittests/Format/FormatTest.cpp

6068–6069

I couldn't figure out why this breaks before the [ in [[unused]], when the identical test using __attribute__((unused)) above does *not* break before the __attribute__.

I ran with -debug and the two seem fairly similar. Can anyone help shed light on this?

With __attribute__((unused))

P8067

With [[unused]]

P8068

12083

Added formatting tests and logic to use this.

Renamed to TT_AttributeSquare to match TT_AttributeParen.

12083

I don't think we have any tests for square-bracket attribute specifier formatting (only __attribute__((foo))) today. My guess is it's pretty broken.

I've added more tests.

[krasimir]

unittests/Format/FormatTest.cpp

6068–6069

The block of a-s appears to be longer here than in the __attribute__ example. That might be a reason.

[benhamilton]

I made it longer so the two lines (attribute((foo)) vs. [[foo]]) had
the same length.

If I shorten it much more, everything goes on one line.

[djasper]

lib/Format/TokenAnnotator.cpp

329

Can you make this:

// C++17 '[[using <namespace>: foo, bar(baz, blech)]]'

To make clear that we are not looking for kw_namespace here?

332

No braces.

336

Do you actually need to put the return type here? I would have thought that it can be deduced as you pass back a const FormatToken* from a codepath and nullptr from all the others.

342

No braces.

350

Sorry that I have missed this before, I thought this was in a different file. Can you try to avoid iterating trying to count or match parentheses inside any of parseSquare/parseParen/parseAngle. We avoided that AFAICT for everything else and I don't think it's necessary here. Especially as you are not actually moving the token position forward, it's too easy to create a quadratic algorithm here.

Also: Do you actually have a test case for the the parentheses case? This thing could use a lot more comments...

366

No braces for single statement ifs. Don't use "else" after "return".

402

No braces for single-statement ifs.

403

What happens if you don't assign this type here? Which formatting decision is based on it?

unittests/Format/FormatTest.cpp

6064

If this is meant to contrast a TT_AttributeColon from a different colon, that doesn't work. "::" is it's own token type coloncolon.

[benhamilton]

Refactor to avoid matching open and close parens. Fix @djasper comments.

[benhamilton]

Great, I refactored it to avoid parsing the matching parens, so there's no more danger of O(N^2) parsing behavior.

I had to add a new InCpp11AttributeSpecifier boolean to Context so when parsing the inner [] we didn't treat it as an ObjC message send or an array subscript.

lib/Format/TokenAnnotator.cpp

329

Ah, good call. Done.

332

Done.

336

Looks like we don't. Fixed.

342

Done.

350

Fixed. There is a test case for the parentheses case.

I definitely see from your other reviews that it's important to structure code the way you describe.

In this scenario, we technically have to deal with all of these (http://en.cppreference.com/w/cpp/language/attributes):

void f() {
  int y[3];
  int i [[cats::meow([[]])]]; // OK
  int i [[cats::meow(foo:bar)]]; // OK
  int i [[cats::meow(foo::bar)]]; // OK
}

I was able to fudge it and assume nobody will actually use attribute parameters containing colons in the arguments, which allowed me to avoid looking for closing parens.

366

Fixed.

402

Fixed.

403

Added a comment to explain:

// Remember that this is a [[using ns: foo]] C++ attribute, so we don't                                                                                                                     
// add a space before the colon (unlike other colons).

There is a formatting test for this.

unittests/Format/FormatTest.cpp

6064

It's not, I just wanted to make sure we had a test with a namespace.

[djasper]

Ok, looks good.

lib/Format/FormatToken.h

 
 namespace clang {
 namespace format {
 
 #define LIST_TOKEN_TYPES                                                       \
   TYPE(ArrayInitializerLSquare)                                                \
   TYPE(ArraySubscriptLSquare)                                                  \
   TYPE(AttributeParen)                                                         \
+  TYPE(AttributeSpecifier)                                                     \
   TYPE(BinaryOperator)                                                         \
   TYPE(BitFieldColon)                                                          \
   TYPE(BlockComment)                                                           \
   TYPE(CastRParen)                                                             \
   TYPE(ConditionalExpr)                                                        \
   TYPE(ConflictAlternative)                                                    \
   TYPE(ConflictEnd)                                                            \
   TYPE(ConflictStart)                                                          \

lib/Format/TokenAnnotator.cpp

while (CurrentToken) {
         return false;
       updateParameterCount(Left, Tok);
       if (CurrentToken && CurrentToken->HasUnescapedNewline)
         HasMultipleLines = true;
     }
     return false;
   }
 
+  const FormatToken *parseCpp11Attribute(const FormatToken &Tok,
+                                         bool NamespaceAllowed) {
+    if (!Tok.isOneOf(tok::identifier, tok::ellipsis))
+      return nullptr;
+    const FormatToken *AttrTok = Tok.Next;
+    if (!AttrTok)
+      return nullptr;
+    if (NamespaceAllowed &&
+        AttrTok->startsSequence(tok::coloncolon, tok::identifier)) {
+      AttrTok = AttrTok->Next->Next;
+    }
+    if (!AttrTok)
+      return nullptr;
+    if (AttrTok->is(tok::l_paren)) {
+      const FormatToken *ParamToken = AttrTok->Next;
+      while (ParamToken && ParamToken->isNot(tok::r_paren))
+        ParamToken = ParamToken->Next;
+      if (!ParamToken)
+        return nullptr;
+      AttrTok = ParamToken->Next;
+    }
+    return AttrTok;
+  }
+
+  // Look for [[ ... ]] which is a valid C++11 attribute specifier but
+  // never a valid Objective-C or Objective-C++ method invocation.
+  bool parseCpp11AttributeSpecifier(const FormatToken &Tok) {
+    if (!Style.isCpp())
+      return false;
+    if (!Tok.startsSequence(tok::l_square, tok::l_square))
+      return false;
+    const FormatToken *AttrTok = Tok.Next->Next;
+    if (!AttrTok)
+      return false;
+    bool NamespaceAllowed;
+    // C++17 '[[using namespace: foo, bar(baz, blech)]]'
+    if (AttrTok->startsSequence(tok::kw_using, tok::identifier, tok::colon)) {
+      AttrTok = AttrTok->Next->Next->Next;
+      NamespaceAllowed = false;
+    } else {
+      // C++11 '[[namespace::foo, namespace::bar(baz, blech)]]'
+      NamespaceAllowed = true;
+    }
+    while (AttrTok) {
+      AttrTok = parseCpp11Attribute(*AttrTok, NamespaceAllowed);
+      if (!AttrTok || AttrTok->isNot(tok::comma))
+        break;
+      AttrTok = AttrTok->Next;
+    }
+    if (!AttrTok)
+      return false;
+    return AttrTok->startsSequence(tok::r_square, tok::r_square);
+  }
+
   bool parseSquare() {
     if (!CurrentToken)
       return false;
 
     // A '[' could be an index subscript (after an identifier or after
     // ')' or ']'), it could be the start of an Objective-C method
-    // expression, or it could the start of an Objective-C array literal.
+    // expression, it could the start of an Objective-C array literal,
+    // or it could be a C++ attribute specifier [[foo::bar]].
     FormatToken *Left = CurrentToken->Previous;
     Left->ParentBracket = Contexts.back().ContextKind;
     FormatToken *Parent = Left->getPreviousNonComment();
 
     // Cases where '>' is followed by '['.
     // In C++, this can happen either in array of templates (foo<int>[10])
     // or when array is a nested template type (unique_ptr<type1<type2>[]>).
     bool CppArrayTemplates =
         Style.isCpp() && Parent && Parent->is(TT_TemplateCloser) &&
         (Contexts.back().CanBeExpression || Contexts.back().IsExpression ||
          Contexts.back().InTemplateArgument);
 
+    if (parseCpp11AttributeSpecifier(*Left)) {
+      CurrentToken->Type = TT_AttributeSpecifier;
+      next();
+      return true;
+    }
+
     bool StartsObjCMethodExpr =
         !CppArrayTemplates && Style.isCpp() &&
         Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) &&
         CurrentToken->isNot(tok::l_brace) &&
         (!Parent ||
          Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren,
                          tok::kw_return, tok::kw_throw) ||
          Parent->isUnaryOperator() ||
          Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) ||

unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsAttributes) {
   AfterType.AlwaysBreakAfterReturnType = FormatStyle::RTBS_AllDefinitions;
   verifyFormat("__attribute__((nodebug)) void\n"
                "foo() {}\n",
                AfterType);
 }
 
 TEST_F(FormatTest, UnderstandsEllipsis) {
   verifyFormat("int printf(const char *fmt, ...);");
   verifyFormat("template <class... Ts> void Foo(Ts... ts) { Foo(ts...); }");
   verifyFormat("template <class... Ts> void Foo(Ts *... ts) {}");
 
   FormatStyle PointersLeft = getLLVMStyle();
   PointersLeft.PointerAlignment = FormatStyle::PAS_Left;
   verifyFormat("template <class... Ts> void Foo(Ts*... ts) {}", PointersLeft);
 }
 
 TEST_F(FormatTest, AdaptivelyFormatsPointersAndReferences) {
   EXPECT_EQ("int *a;\n"
             "int *a;\n"
             "int *a;",
             format("int *a;\n"
                    "int* a;\n"
TEST_F(FormatTest, FileAndCode) {
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.m", ""));
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.mm", ""));
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", ""));
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.h", "@interface Foo\n@end\n"));
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo", ""));
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo", "@interface Foo\n@end\n"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithCpp11AttributeSpecifiers) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[noreturn]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+            guessLanguage("foo.h", "array[[calculator getIndex]];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+            guessLanguage("foo.h", "[[noreturn, deprecated(\"so sorry\")]];"));
+  EXPECT_EQ(
+      FormatStyle::LK_Cpp,
+      guessLanguage("foo.h", "[[noreturn, deprecated(\"gone, sorry\")]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+            guessLanguage("foo.h", "[[noreturn foo] bar];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+            guessLanguage("foo.h", "[[clang::fallthrough]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+            guessLanguage("foo.h", "[[clang:fallthrough] foo];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+            guessLanguage("foo.h", "[[gsl::suppress(\"type\")]];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+            guessLanguage("foo.h", "[[using clang: fallthrough]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+            guessLanguage("foo.h", "[[abusing clang:fallthrough] bar];"));
+  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, ...]]"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang