[clang-format] Do not merge target-name and : for C# attributes

[jbcoe]

Summary

Re-use token type TT_AttributeColon for C# attribute target colons.

[krasimir]

clang/unittests/Format/FormatTestCSharp.cpp

281

isn't the space after the : a regression?
Looking at
https://docs.microsoft.com/en-us/dotnet/standard/assembly/set-attributes
it looks like such attributes are spelled [assembly:Blah]

How is an example where such an attribute is not the last thing in the file?

I'm not entirely clear on which edit caused this.

[krasimir]

clang/unittests/Format/FormatTestCSharp.cpp

281

The space maybe is coming from
TokenAnnotator.spaceRequiredBetween, which may need to be adjusted for C#.

[jbcoe]

Do not allow spaces around C# attribute colons.

clang/lib/Format/TokenAnnotator.cpp

391

This is needed to avoid

[assembly:InternalsVisibleTo("SomeAssembly, PublicKey=SomePublicKeyThatExceedsTheColumnLimit")]

being classified as a ObjCMethodExpr (as seen when running clang-format with -debug).

The formatting results are currently the same for ObjCMethodExpr and CSharpAttributes.

clang/unittests/Format/FormatTestCSharp.cpp

281

There seems to be some inconsistency between the docs you linked (which has no space) and code that forms part of projects like Roslyn (which has the space).

I'll defer making this change (if we want it until later).

[MyDeveloperDay]

This feels better than merging

I think visual studio tends to create files via the new project wizard that do not have a space before but do have a space after the :

[assembly: ComVisible(false)]

[krasimir]

Thank you for the explanations! Looking good!

clang/lib/Format/FormatTokenLexer.h

private:
   bool tryMergeLessLess();
   bool tryMergeNSStringLiteral();
   bool tryMergeJSPrivateIdentifier();
   bool tryMergeCSharpStringLiteral();
   bool tryMergeCSharpKeywordVariables();
   bool tryMergeCSharpDoubleQuestion();
   bool tryMergeCSharpNullConditional();
   bool tryTransformCSharpForEach();
-  bool tryMergeCSharpAttributeAndTarget();
 
   bool tryMergeTokens(ArrayRef<tok::TokenKind> Kinds, TokenType NewType);
 
   // Returns \c true if \p Tok can only be followed by an operand in JavaScript.
   bool precedesOperand(FormatToken *Tok);
 
   bool canPrecedeRegexLiteral(FormatToken *Prev);
 

clang/lib/Format/FormatTokenLexer.cpp

void FormatTokenLexer::tryMergePreviousTokens() {
   if (tryMerge_TMacro())
     return;
   if (tryMergeConflictMarkers())
     return;
   if (tryMergeLessLess())
     return;
 
   if (Style.isCSharp()) {
-    if (tryMergeCSharpAttributeAndTarget())
-      return;
     if (tryMergeCSharpKeywordVariables())
       return;
     if (tryMergeCSharpStringLiteral())
       return;
     if (tryMergeCSharpDoubleQuestion())
       return;
     if (tryMergeCSharpNullConditional())
       return;

 
 // Valid C# attribute targets:
 // https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/concepts/attributes/#attribute-targets
 const llvm::StringSet<> FormatTokenLexer::CSharpAttributeTargets = {
     "assembly", "module",   "field",  "event", "method",
     "param",    "property", "return", "type",
 };
 
-bool FormatTokenLexer::tryMergeCSharpAttributeAndTarget() {
-  // Treat '[assembly:' and '[field:' as tokens in their own right.
-  if (Tokens.size() < 3)
-    return false;
-
-  auto &SquareBracket = *(Tokens.end() - 3);
-  auto &Target = *(Tokens.end() - 2);
-  auto &Colon = *(Tokens.end() - 1);
-
-  if (!SquareBracket->Tok.is(tok::l_square))
-    return false;
-
-  if (CSharpAttributeTargets.find(Target->TokenText) ==
-      CSharpAttributeTargets.end())
-    return false;
-
-  if (!Colon->Tok.is(tok::colon))
-    return false;
-
-  SquareBracket->TokenText =
-      StringRef(SquareBracket->TokenText.begin(),
-                Colon->TokenText.end() - SquareBracket->TokenText.begin());
-  SquareBracket->ColumnWidth += (Target->ColumnWidth + Colon->ColumnWidth);
-  Tokens.erase(Tokens.end() - 2);
-  Tokens.erase(Tokens.end() - 1);
-  return true;
-}
-
 bool FormatTokenLexer::tryMergeCSharpDoubleQuestion() {
   if (Tokens.size() < 2)
     return false;
   auto &FirstQuestion = *(Tokens.end() - 2);
   auto &SecondQuestion = *(Tokens.end() - 1);
   if (!FirstQuestion->is(tok::question) || !SecondQuestion->is(tok::question))
     return false;
   FirstQuestion->Tok.setKind(tok::question); // no '??' in clang tokens.

clang/lib/Format/TokenAnnotator.cpp

bool isCSharpAttributeSpecifier(const FormatToken &Tok) {
 
     // Move along the tokens inbetween the '[' and ']' e.g. [STAThread].
     while (AttrTok && AttrTok->isNot(tok::r_square)) {
       AttrTok = AttrTok->Next;
     }
 
     if (!AttrTok)
       return false;
     
-    // Move past the end of ']'.
+    // Allow an attribute to be the only content of a file.
     AttrTok = AttrTok->Next;
     if (!AttrTok)
-      return false;
+      return true;
 
     // Limit this to being an access modifier that follows.
     if (AttrTok->isOneOf(tok::kw_public, tok::kw_private, tok::kw_protected,
                          tok::comment, tok::kw_class, tok::kw_static,
                          tok::l_square, Keywords.kw_internal)) {
       return true;
     }
 
bool CppArrayTemplates =
         Style.isCpp() && Parent && Parent->is(TT_TemplateCloser) &&
         (Contexts.back().CanBeExpression || Contexts.back().IsExpression ||
          Contexts.back().InTemplateArgument);
 
     bool IsCpp11AttributeSpecifier = isCpp11AttributeSpecifier(*Left) ||
                                      Contexts.back().InCpp11AttributeSpecifier;
 
     // Treat C# Attributes [STAThread] much like C++ attributes [[...]].
-    bool IsCSharp11AttributeSpecifier =
+    bool IsCSharpAttributeSpecifier =
         isCSharpAttributeSpecifier(*Left) ||
         Contexts.back().InCSharpAttributeSpecifier;
 
     bool InsideInlineASM = Line.startsWith(tok::kw_asm);
     bool IsCppStructuredBinding = Left->isCppStructuredBinding(Style);
     bool StartsObjCMethodExpr =
         !IsCppStructuredBinding && !InsideInlineASM && !CppArrayTemplates &&
         Style.isCpp() && !IsCpp11AttributeSpecifier &&
-        Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) &&
+        !IsCSharpAttributeSpecifier && Contexts.back().CanBeExpression &&
+        Left->isNot(TT_LambdaLSquare) &&
         !CurrentToken->isOneOf(tok::l_brace, tok::r_square) &&
         (!Parent ||
          Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren,
                          tok::kw_return, tok::kw_throw) ||
          Parent->isUnaryOperator() ||
          // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
          Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) ||
          (getBinOpPrecedence(Parent->Tok.getKind(), true, true) >
if (IsCppStructuredBinding) {
         Left->Type = TT_AttributeSquare;
       } else if (Style.Language == FormatStyle::LK_JavaScript && Parent &&
                  Contexts.back().ContextKind == tok::l_brace &&
                  Parent->isOneOf(tok::l_brace, tok::comma)) {
         Left->Type = TT_JsComputedPropertyName;
       } else if (Style.isCpp() && Contexts.back().ContextKind == tok::l_brace &&
                  Parent && Parent->isOneOf(tok::l_brace, tok::comma)) {
         Left->Type = TT_DesignatedInitializerLSquare;
-      } else if (IsCSharp11AttributeSpecifier) {
+      } else if (IsCSharpAttributeSpecifier) {
         Left->Type = TT_AttributeSquare;
       } else if (CurrentToken->is(tok::r_square) && Parent &&
                  Parent->is(TT_TemplateCloser)) {
         Left->Type = TT_ArraySubscriptLSquare;
       } else if (Style.Language == FormatStyle::LK_Proto ||
                  Style.Language == FormatStyle::LK_TextProto) {
         // Square braces in LK_Proto can either be message field attributes:
         //
bool parseSquare() {
     ScopedContextCreator ContextCreator(*this, tok::l_square, BindingIncrease);
     Contexts.back().IsExpression = true;
     if (Style.Language == FormatStyle::LK_JavaScript && Parent &&
         Parent->is(TT_JsTypeColon))
       Contexts.back().IsExpression = false;
 
     Contexts.back().ColonIsObjCMethodExpr = StartsObjCMethodExpr;
     Contexts.back().InCpp11AttributeSpecifier = IsCpp11AttributeSpecifier;
-    Contexts.back().InCSharpAttributeSpecifier = IsCSharp11AttributeSpecifier;
+    Contexts.back().InCSharpAttributeSpecifier = IsCSharpAttributeSpecifier;
 
     while (CurrentToken) {
       if (CurrentToken->is(tok::r_square)) {
         if (IsCpp11AttributeSpecifier)
           CurrentToken->Type = TT_AttributeSquare;
-        if (IsCSharp11AttributeSpecifier)
+        if (IsCSharpAttributeSpecifier)
           CurrentToken->Type = TT_AttributeSquare;
         else if (((CurrentToken->Next &&
                    CurrentToken->Next->is(tok::l_paren)) ||
                   (CurrentToken->Previous &&
                    CurrentToken->Previous->Previous == Left)) &&
                  Left->is(TT_ObjCMethodExpr)) {
           // An ObjC method call is rarely followed by an open parenthesis. It
           // also can't be composed of just one token, unless it's a macro that
case tok::colon:
              Contexts.back().ContextKind == tok::l_brace) || // object type
             (Contexts.size() == 1 &&
              Line.MustBeDeclaration)) { // method/property declaration
           Contexts.back().IsExpression = false;
           Tok->Type = TT_JsTypeColon;
           break;
         }
       } else if (Style.isCSharp()) {
+        if (Contexts.back().InCSharpAttributeSpecifier) {
+          Tok->Type = TT_AttributeColon;
+          break;
+        }
         if (Contexts.back().ContextKind == tok::l_paren) {
           Tok->Type = TT_CSharpNamedArgumentColon;
           break;
         }
       }
       if (Contexts.back().ColonIsDictLiteral ||
           Style.Language == FormatStyle::LK_Proto ||
           Style.Language == FormatStyle::LK_TextProto) {
bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line,
   return false;
 }
 
 bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
                                     const FormatToken &Right) {
   const FormatToken &Left = *Right.Previous;
   // Language-specific stuff.
   if (Style.isCSharp()) {
-    if (Left.is(TT_CSharpNamedArgumentColon) ||
-        Right.is(TT_CSharpNamedArgumentColon))
+    if (Left.isOneOf(TT_CSharpNamedArgumentColon, TT_AttributeColon) ||
+        Right.isOneOf(TT_CSharpNamedArgumentColon, TT_AttributeColon))
       return false;
   } else if (Style.Language == FormatStyle::LK_Java) {
     if (Left.isOneOf(Keywords.kw_throws, Keywords.kw_extends,
                      Keywords.kw_implements))
       return false;
     if (Right.isOneOf(Keywords.kw_throws, Keywords.kw_extends,
                       Keywords.kw_implements))
       return true;

clang/unittests/Format/FormatTestCSharp.cpp

verifyFormat("void MethodA([In][Out] ref double x)\n"
                "{\n"
                "}");
 
   //  Unwrappable lines go on a line of their own.
   // 'target:' is not treated as a label.
   // Modify Style to enforce a column limit.
   FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
   Style.ColumnLimit = 10;
-  verifyFormat(R"([assembly:InternalsVisibleTo(
+  verifyFormat(R"([assembly: InternalsVisibleTo(
     "SomeAssembly, PublicKey=SomePublicKeyThatExceedsTheColumnLimit")])",
                Style);
 }
 
 TEST_F(FormatTestCSharp, CSharpUsing) {
   FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
   Style.SpaceBeforeParens = FormatStyle::SBPO_Always;
   verifyFormat("public void foo () {\n"