[clang-format] Improve handling of C# attributes

[jbcoe]

Summary

C# attributes can appear on classes and methods, in which case they should go on their own line, or on method parameters in which case
they should be left inline.

[jbcoe]

clang/lib/Format/TokenAnnotator.cpp

3283

I removed this as it's too aggressive and introduces line breaks when attributes appear on method parameters.

[MyDeveloperDay]

LGTM, thanks

[MyDeveloperDay]

This issue seems to have caused a regression https://bugs.llvm.org/show_bug.cgi?id=50515, I understand we removed the rule because it was too aggressive, now its not aggressive enough.

I'm going to try and find some sort of compromise so we can gravitate to a solution that can work for both.

[MyDeveloperDay]

The second property in a class is putting the property on the same line

[XmlIgnore] 
public string property1 { get; set; }

[XmlIgnore] public string property2{ get; set; }

as are all successive properties

Multiple properites are also not getting broken unless they are the first property

[XmlIgnore] 
[ScriptIgnore] 
public string property1 { get; set; }

[XmlIgnore] [ScriptIgnore] public string property2{ get; set; }

Ideally we want this all the time.

[XmlIgnore] 
[ScriptIgnore] 
public string Url { get; set; }

In the original change @jbcoe I think you were trying to fix the case where void MethodA([In][Out] ref... got broken, I presume into

void MethodA([In]
   [Out] ref

Part of the problem although not a solution was that the [ of the [Out] isn't even actually seen as an TT_AttributeSquare but instead as a TT_ArraySubscriptLSquare

AnnotatedTokens(L=1):
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=void L=4 PPK=2 FakeLParens= FakeRParens=0 II=0x1c0f418 Text='void'
 M=0 C=1 T=StartOfName S=1 F=0 B=0 BK=0 P=1020 Name=identifier L=15 PPK=2 FakeLParens= FakeRParens=0 II=0x1c38cb0 Text='myFunction'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=l_paren L=16 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=AttributeSquare S=0 F=0 B=0 BK=0 P=140 Name=l_square L=17 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='['
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=79 Name=identifier L=19 PPK=2 FakeLParens= FakeRParens=0 II=0x1c38cd0 Text='In'
 M=0 C=0 T=AttributeSquare S=0 F=0 B=0 BK=0 P=63 Name=r_square L=20 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=']'
 M=0 C=0 T=ArraySubscriptLSquare S=0 F=0 B=0 BK=0 P=240 Name=l_square L=21 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='['
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=259 Name=identifier L=24 PPK=2 FakeLParens= FakeRParens=0 II=0x1c38cf0 Text='Out'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=243 Name=r_square L=25 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=']'
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=43 Name=identifier L=27 PPK=2 FakeLParens= FakeRParens=0 II=0x1c38d10 Text='a'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=28 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'

That I think you move the handling of the newlines to the parsing rather than in the canBreakBefore/mustBreakBefore (i.e. you wrap the lines when you see it on the function)

But of course a property isn't a function and so this is why its not being handled correctly. Going from the overly aggressive to the the parsing model regressed the behaviour for properties with attributes (which is very common if you use Newtonsoft.Json)

See D103307: [clang-format] successive C# attributes cause line breaking issues

clang/lib/Format/TokenAnnotator.cpp

if (Left.is(TT_LambdaLBrace)) {
 
     if (Style.AllowShortLambdasOnASingleLine == FormatStyle::SLS_None ||
         Style.AllowShortLambdasOnASingleLine == FormatStyle::SLS_Inline ||
         (!Left.Children.empty() &&
          Style.AllowShortLambdasOnASingleLine == FormatStyle::SLS_Empty))
       return true;
   }
 
-  // Put multiple C# attributes on a new line.
-  if (Style.isCSharp() &&
-      ((Left.is(TT_AttributeSquare) && Left.is(tok::r_square)) ||
-       (Left.is(tok::r_square) && Right.is(TT_AttributeSquare) &&
-        Right.is(tok::l_square))))
-    return true;
-
   // Put multiple Java annotation on a new line.
   if ((Style.Language == FormatStyle::LK_Java ||
        Style.Language == FormatStyle::LK_JavaScript) &&
       Left.is(TT_LeadingJavaAnnotation) &&
       Right.isNot(TT_LeadingJavaAnnotation) && Right.isNot(tok::l_paren) &&
       (Line.Last->is(tok::l_brace) || Style.BreakAfterJavaFieldAnnotations))
     return true;
 

clang/lib/Format/UnwrappedLineParser.h

private:
   void parseRecord(bool ParseAsExpr = false);
   void parseObjCMethod();
   void parseObjCProtocolList();
   void parseObjCUntilAtEnd();
   void parseObjCInterfaceOrImplementation();
   bool parseObjCProtocol();
   void parseJavaScriptEs6ImportExport();
   void parseStatementMacro();
+  void parseCSharpAttribute();
   bool tryToParseLambda();
   bool tryToParseLambdaIntroducer();
   void tryToParseJSFunction();
   void addUnwrappedLine();
   bool eof() const;
   // LevelDifference is the difference of levels after and before the current
   // token. For example:
   // - if the token is '{' and opens a block, LevelDifference is 1.

clang/lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::parseFile() {
   // Here we add this newline before end-of-file comments.
   if (Style.Language == FormatStyle::LK_TextProto &&
       !CommentsBeforeNextToken.empty())
     addUnwrappedLine();
   flushComments(true);
   addUnwrappedLine();
 }
 
+void UnwrappedLineParser::parseCSharpAttribute() {
+  do {
+    switch (FormatTok->Tok.getKind()) {
+    case tok::r_square:
+      nextToken();
+      addUnwrappedLine();
+      return;
+    default:
+      nextToken();
+      break;
+    }
+  } while (!eof());
+}
+
 void UnwrappedLineParser::parseLevel(bool HasOpeningBrace) {
   bool SwitchLabelEncountered = false;
   do {
     tok::TokenKind kind = FormatTok->Tok.getKind();
     if (FormatTok->Type == TT_MacroBlockBegin) {
       kind = tok::l_brace;
     } else if (FormatTok->Type == TT_MacroBlockEnd) {
       kind = tok::r_brace;
case tok::kw_case:
         break;
       }
       if (!SwitchLabelEncountered &&
           (Style.IndentCaseLabels || (Line->InPPDirective && Line->Level == 1)))
         ++Line->Level;
       SwitchLabelEncountered = true;
       parseStructuralElement();
       break;
+    case tok::l_square:
+      if (Style.isCSharp()) {
+        nextToken();
+        parseCSharpAttribute();
+        break;
+      }
+      LLVM_FALLTHROUGH;
     default:
       parseStructuralElement();
       break;
     }
   } while (!eof());
 }
 
 void UnwrappedLineParser::calculateBraceTypes(bool ExpectClassBody) {

clang/unittests/Format/FormatTestCSharp.cpp

verifyFormat("[TestMethod(\"start\", HelpText = \"Starts the server "
                "    get;\n"
                "}");
 
   verifyFormat(
       "[DllImport(\"Hello\", EntryPoint = \"hello_world\")]\n"
       "// The const char* returned by hello_world must not be deleted.\n"
       "private static extern IntPtr HelloFromCpp();)");
 
+  // Class attributes go on their own line and do not affect layout of
+  // interfaces. Line wrapping decisions previously caused each interface to be
+  // on its own line.
+  verifyFormat("[SomeAttribute]\n"
+               "[SomeOtherAttribute]\n"
+               "public class A : IShape, IAnimal, IVehicle\n"
+               "{\n"
+               "    int X;\n"
+               "}");
+
+  // Attributes in a method declaration do not cause line wrapping.
+  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(
     "SomeAssembly, PublicKey=SomePublicKeyThatExceedsTheColumnLimit")])",
                Style);