[clang-format] [PR34574] Handle [[nodiscard]] attribute in class declaration

[MyDeveloperDay]

Summary

https://bugs.llvm.org/show_bug.cgi?id=34574
https://bugs.llvm.org/show_bug.cgi?id=38401

template <typename T>
class [[nodiscard]] result
{
  public:
    result(T&&)
    {
    }
};

formats incorrectly to

template <typename T>
class [[nodiscard]] result{public : result(T &&){}};

[krasimir]

I have an example where this goes wrong in ObjC (modulo indentation):

struct Stuff stuff[] = {                                                                                                                                                                                                                                              
      {key, value},  // comment                                                                                                                                                                                                                                       
      {key, value},  // comment                                                                                                                                                                                                                                       
      {key, value},  // comment                                                                                                                                                                                                                                       
      {key, value},                                                                                                                                                                                                                                       
      {key, value},                                                                                                                                                                                                                                   
      {nil, 0}};

gets formatted to (module indentation):

struct Stuff stuff[] = {                                                                                                                                                                                                                                              
      {key, value},  // comment                                                                                                                                                                                                                                       
      {key, value},  // comment                                                                                                                                                                                                                                       
      {key, value},  // comment                                                                                                                                                                                                                                       
      {key, value},                                                                                                                                                                                                                                       
      {key, value},                                                                                                                                                                                                                                   
      {
        nil, 0
      }
  };

I think we should try harder to detect the beginning of an attribute specifier. I think there may be a helper for that somewhere in clang-format.

[krasimir]

clang/lib/Format/UnwrappedLineParser.cpp

2401–2402

nit: add, e.g., "... or an [[attribute]] ..."

[MyDeveloperDay]

In D79354#2020722, @krasimir wrote:

I have an example where this goes wrong in ObjC (modulo indentation):

I think we should try harder to detect the beginning of an attribute specifier. I think there may be a helper for that somewhere in clang-format.

could you give me your .clang-format settings standard LLVM gives me

struct Stuff stuff[] = {{key, value}, // comment
                        {key, value}, // comment
                        {key, value}, // comment
                        {key, value}, {key, value}, {nil, 0}};

[krasimir]

I'm not familiar with ObjC, but hope this helps:

% cat ~/example.mm   # left as-is by clang-format without D79354                 
(int)f : (int)arg {
  struct A as[] = {{a11, a12},  //
                   {a21, a22},  //
                   {nil, 0}};
  return 0;
}
% bin/clang-format ~/example.mm  # clang-format with D79354
(int)f : (int)arg {
  struct A as[] = {
    {a11, a12},     //
        {a21, a22}, //
    {
      nil, 0
    }
  };
  return 0;
}
%

[MyDeveloperDay]

Reduce false formatting

[krasimir]

clang/lib/Format/UnwrappedLineParser.cpp

2405

Just to make sure I follow -- at this point why can't we check for the more specific TT_AttributeSquare instead of the general tok::l_square (if that's available, not sure when it gets computed) (isOneOf accepts both tok::'s and TT_'s).

Ditto for the new tok::l_square below.

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

2405

You are quite correct I can use TT_AttributeSquare here... let me change that

[MyDeveloperDay]

Change to TT_AttributeSquare

[krasimir]

Thank you! Looks good with a couple of nits.

clang/lib/Format/UnwrappedLineParser.cpp

2402

nit: add a fullstop

2431

slight concern: check if the next token is TT_AttributeSquare and only consume it in that case. Other languages that are using TT_AttributeSquare may have a different syntax for their attributes than [[attribute]], e.g. recently added [[ https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/concepts/attributes/ | C# [attributes] ]].

[MyDeveloperDay]

Address review comments and ensure existence of closing ]

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

2431

I agree, in fact I had the same concern myself, I kind of assumed because we'd of seen

[[ .... ] then it would be obvious that the next would be ]`

But on reflection I think it could potentially be other things too

clang/lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::parseJavaEnumBody() {
   addUnwrappedLine();
 }
 
 void UnwrappedLineParser::parseRecord(bool ParseAsExpr) {
   const FormatToken &InitialToken = *FormatTok;
   nextToken();
 
   // The actual identifier can be a nested name specifier, and in macros
   // it is often token-pasted.
+  // An [[attribute]] can be before the identifier
   while (FormatTok->isOneOf(tok::identifier, tok::coloncolon, tok::hashhash,
                             tok::kw___attribute, tok::kw___declspec,
-                            tok::kw_alignas) ||
+                            tok::kw_alignas, TT_AttributeSquare) ||
          ((Style.Language == FormatStyle::LK_Java ||
            Style.Language == FormatStyle::LK_JavaScript) &&
           FormatTok->isOneOf(tok::period, tok::comma))) {
     if (Style.Language == FormatStyle::LK_JavaScript &&
         FormatTok->isOneOf(Keywords.kw_extends, Keywords.kw_implements)) {
       // JavaScript/TypeScript supports inline object types in
       // extends/implements positions:
       //     class Foo implements {bar: number} { }
       nextToken();
       if (FormatTok->is(tok::l_brace)) {
         tryToParseBracedList();
         continue;
       }
     }
     bool IsNonMacroIdentifier =
         FormatTok->is(tok::identifier) &&
         FormatTok->TokenText != FormatTok->TokenText.upper();
     nextToken();
     // We can have macros or attributes in between 'class' and the class name.
-    if (!IsNonMacroIdentifier && FormatTok->Tok.is(tok::l_paren))
+    if (!IsNonMacroIdentifier) {
+      if (FormatTok->Tok.is(tok::l_paren)) {
         parseParens();
+      } else if (FormatTok->is(TT_AttributeSquare)) {
+        parseSquare();
+        // Consume the closing TT_AttributeSquare.
+        nextToken();
+      }
+    }
   }
 
   // Note that parsing away template declarations here leads to incorrectly
   // accepting function declarations as record declarations.
   // In general, we cannot solve this problem. Consider:
   // class A<int> B() {}
   // which can be a function definition or a class definition when B() is a
   // macro. If we find enough real-world cases where this is a problem, we

clang/unittests/Format/FormatTest.cpp

 TEST_F(FormatTest, UnderstandsSquareAttributes) {
   verifyFormat("SomeType s [[unused]] (InitValue);");
   verifyFormat("SomeType s [[gnu::unused]] (InitValue);");
   verifyFormat("SomeType s [[using gnu: unused]] (InitValue);");
   verifyFormat("[[gsl::suppress(\"clang-tidy-check-name\")]] void f() {}");
   verifyFormat("void f() [[deprecated(\"so sorry\")]];");
   verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
                "    [[unused]] aaaaaaaaaaaaaaaaaaaaaaa(int i);");
+  verifyFormat("[[nodiscard]] bool f() { return false; }");
+  verifyFormat("class [[nodiscard]] f {\npublic:\n  f() {}\n}");
+  verifyFormat("class [[deprecated(\"so sorry\")]] f {\npublic:\n  f() {}\n}");
+  verifyFormat("class [[gnu::unused]] f {\npublic:\n  f() {}\n}");
 
   // Make sure we do not mistake attributes for array subscripts.
   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.