[clang-format] [PR43333] Fix C# breaking before function name when using Attributes

[MyDeveloperDay]

Summary

This is a fix for https://bugs.llvm.org/show_bug.cgi?id=43333

This comes with 3 main parts

• C# attributes cause function names on a new line even when AlwaysBreakAfterReturnType is set to None
• Add AlwaysBreakAfterReturnType to None by default in the Microsoft style,
• C# unit tests are not using Microsoft style (which we created to define the default C# style to match a vanilla C# project).

[mitchell-stellar]

LGTM

cfe/trunk/include/clang/Format/Format.h

struct FormatStyle {
   ///    aaaa =                         vs.     aaaa = "bbbb"
   ///        "bbbb"                                    "cccc";
   ///        "cccc";
   /// \endcode
   bool AlwaysBreakBeforeMultilineStrings;
 
   /// Different ways to break after the template declaration.
   enum BreakTemplateDeclarationsStyle {
-      /// Do not force break before declaration.
-      /// ``PenaltyBreakTemplateDeclaration`` is taken into account.
-      /// \code
-      ///    template <typename T> T foo() {
-      ///    }
-      ///    template <typename T> T foo(int aaaaaaaaaaaaaaaaaaaaa,
-      ///                                int bbbbbbbbbbbbbbbbbbbbb) {
-      ///    }
-      /// \endcode
-      BTDS_No,
-      /// Force break after template declaration only when the following
-      /// declaration spans multiple lines.
-      /// \code
-      ///    template <typename T> T foo() {
-      ///    }
-      ///    template <typename T>
-      ///    T foo(int aaaaaaaaaaaaaaaaaaaaa,
-      ///          int bbbbbbbbbbbbbbbbbbbbb) {
-      ///    }
-      /// \endcode
-      BTDS_MultiLine,
-      /// Always break after template declaration.
-      /// \code
-      ///    template <typename T>
-      ///    T foo() {
-      ///    }
-      ///    template <typename T>
-      ///    T foo(int aaaaaaaaaaaaaaaaaaaaa,
-      ///          int bbbbbbbbbbbbbbbbbbbbb) {
-      ///    }
-      /// \endcode
-      BTDS_Yes
   };
 
   /// The template declaration breaking style to use.
   BreakTemplateDeclarationsStyle AlwaysBreakTemplateDeclarations;
 
   /// If ``false``, a function call's arguments will either be all on the
   /// same line or will have one line each.
   /// \code

 /// Returns a format style complying with Webkit's style guide:
 /// http://www.webkit.org/coding/coding-style.html
 FormatStyle getWebKitStyle();
 
 /// Returns a format style complying with GNU Coding Standards:
 /// http://www.gnu.org/prep/standards/standards.html
 FormatStyle getGNUStyle();
 
+/// Returns a format style complying with Microsoft style guide:
+/// https://docs.microsoft.com/en-us/visualstudio/ide/editorconfig-code-style-settings-reference?view=vs-2017
+FormatStyle getMicrosoftStyle(FormatStyle::LanguageKind Language);
+
 /// Returns style indicating formatting should be not applied at all.
 FormatStyle getNoStyle();
 
 /// Gets a predefined style for the specified language by name.
 ///
 /// Currently supported names: LLVM, Google, Chromium, Mozilla. Names are
 /// compared case-insensitively.
 ///

cfe/trunk/lib/Format/ContinuationIndenter.cpp

if (Current.NestingLevel == 0 && !Current.isTrailingComment()) {
     if (Previous.is(TT_FunctionAnnotationRParen))
       return true;
     if (Previous.is(TT_LeadingJavaAnnotation) && Current.isNot(tok::l_paren) &&
         Current.isNot(TT_LeadingJavaAnnotation))
       return true;
   }
 
   // If the return type spans multiple lines, wrap before the function name.
-  if ((Current.is(TT_FunctionDeclarationName) ||
+  if (((Current.is(TT_FunctionDeclarationName) &&
+        // Don't break before a C# function when no break after return type
+        (!Style.isCSharp() ||
+         Style.AlwaysBreakAfterReturnType != FormatStyle::RTBS_None)) ||
        (Current.is(tok::kw_operator) && !Previous.is(tok::coloncolon))) &&
       !Previous.is(tok::kw_template) && State.Stack.back().BreakBeforeParameter)
     return true;
 
   // The following could be precomputed as they do not depend on the state.
   // However, as they should take effect only if the UnwrappedLine does not fit
   // into the ColumnLimit, they are checked here in the ContinuationIndenter.
   if (Style.ColumnLimit != 0 && Previous.BlockKind == BK_Block &&

cfe/trunk/lib/Format/Format.cpp

FormatStyle getGNUStyle() {
   Style.ColumnLimit = 79;
   Style.FixNamespaceComments = false;
   Style.SpaceBeforeParens = FormatStyle::SBPO_Always;
   Style.Standard = FormatStyle::LS_Cpp03;
   return Style;
 }
 
 FormatStyle getMicrosoftStyle(FormatStyle::LanguageKind Language) {
-  FormatStyle Style = getLLVMStyle();
+  FormatStyle Style = getLLVMStyle(Language);
   Style.ColumnLimit = 120;
   Style.TabWidth = 4;
   Style.IndentWidth = 4;
   Style.UseTab = FormatStyle::UT_Never;
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.AfterClass = true;
   Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always;
   Style.BraceWrapping.AfterEnum = true;
   Style.BraceWrapping.AfterFunction = true;
   Style.BraceWrapping.AfterNamespace = true;
   Style.BraceWrapping.AfterObjCDeclaration = true;
   Style.BraceWrapping.AfterStruct = true;
   Style.BraceWrapping.AfterExternBlock = true;
   Style.BraceWrapping.BeforeCatch = true;
   Style.BraceWrapping.BeforeElse = true;
   Style.PenaltyReturnTypeOnItsOwnLine = 1000;
   Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
   Style.AllowShortCaseLabelsOnASingleLine = false;
   Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   Style.AllowShortLoopsOnASingleLine = false;
+  Style.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
+  Style.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   return Style;
 }
 
 FormatStyle getNoStyle() {
   FormatStyle NoStyle = getLLVMStyle();
   NoStyle.DisableFormat = true;
   NoStyle.SortIncludes = false;
   NoStyle.SortUsingDeclarations = false;

cfe/trunk/lib/Format/TokenAnnotator.cpp

if (!AttrTok)
       return false;
 
     // Limit this to being an access modifier that follows.
     if (AttrTok->isOneOf(tok::kw_public, tok::kw_private, tok::kw_protected,
                          tok::kw_class, tok::kw_static, tok::l_square,
                          Keywords.kw_internal)) {
       return true;
     }
+
+    // incase its a [XXX] retval func(....
+    if (AttrTok->Next &&
+        AttrTok->Next->startsSequence(tok::identifier, tok::l_paren))
+      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)) {
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) {
+        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:
         //
         // optional Aaa aaa = 1 [
if (IsCppStructuredBinding) {
         }
       } else if (!CppArrayTemplates && Parent &&
                  Parent->isOneOf(TT_BinaryOperator, TT_TemplateCloser, tok::at,
                                  tok::comma, tok::l_paren, tok::l_square,
                                  tok::question, tok::colon, tok::kw_return,
                                  // Should only be relevant to JavaScript:
                                  tok::kw_default)) {
         Left->Type = TT_ArrayInitializerLSquare;
-      } else if (IsCSharp11AttributeSpecifier) {
-        Left->Type = TT_AttributeSquare;
       } else {
         BindingIncrease = 10;
         Left->Type = TT_ArraySubscriptLSquare;
       }
     }
 
     ScopedContextCreator ContextCreator(*this, tok::l_square, BindingIncrease);
     Contexts.back().IsExpression = true;

cfe/trunk/unittests/Format/FormatTestCSharp.cpp

static std::string format(llvm::StringRef Code, unsigned Offset,
     auto Result = applyAllReplacements(Code, Replaces);
     EXPECT_TRUE(static_cast<bool>(Result));
     LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
     return *Result;
   }
 
   static std::string
   format(llvm::StringRef Code,
-         const FormatStyle &Style = getGoogleStyle(FormatStyle::LK_CSharp)) {
+         const FormatStyle &Style = getMicrosoftStyle(FormatStyle::LK_CSharp)) {
     return format(Code, 0, Code.size(), Style);
   }
 
   static FormatStyle getStyleWithColumns(unsigned ColumnLimit) {
-    FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+    FormatStyle Style = getMicrosoftStyle(FormatStyle::LK_CSharp);
     Style.ColumnLimit = ColumnLimit;
     return Style;
   }
 
   static void verifyFormat(
       llvm::StringRef Code,
-      const FormatStyle &Style = getGoogleStyle(FormatStyle::LK_CSharp)) {
+      const FormatStyle &Style = getMicrosoftStyle(FormatStyle::LK_CSharp)) {
     EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
     EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 };
 
 TEST_F(FormatTestCSharp, CSharpClass) {
-  verifyFormat("public class SomeClass {\n"
-               "  void f() {}\n"
-               "  int g() { return 0; }\n"
-               "  void h() {\n"
-               "    while (true) f();\n"
-               "    for (;;) f();\n"
-               "    if (true) f();\n"
-               "  }\n"
+  verifyFormat("public class SomeClass\n"
+               "{\n"
+               "    void f()\n"
+               "    {\n"
+               "    }\n"
+               "    int g()\n"
+               "    {\n"
+               "        return 0;\n"
+               "    }\n"
+               "    void h()\n"
+               "    {\n"
+               "        while (true)\n"
+               "            f();\n"
+               "        for (;;)\n"
+               "            f();\n"
+               "        if (true)\n"
+               "            f();\n"
+               "    }\n"
                "}");
 }
 
 TEST_F(FormatTestCSharp, AccessModifiers) {
-  verifyFormat("public String toString() {}");
-  verifyFormat("private String toString() {}");
-  verifyFormat("protected String toString() {}");
-  verifyFormat("internal String toString() {}");
-
-  verifyFormat("public override String toString() {}");
-  verifyFormat("private override String toString() {}");
-  verifyFormat("protected override String toString() {}");
-  verifyFormat("internal override String toString() {}");
+  verifyFormat("public String toString()\n"
+               "{\n"
+               "}");
+  verifyFormat("private String toString()\n"
+               "{\n"
+               "}");
+  verifyFormat("protected String toString()\n"
+               "{\n"
+               "}");
+  verifyFormat("internal String toString()\n"
+               "{\n"
+               "}");
+
+  verifyFormat("public override String toString()\n"
+               "{\n"
+               "}");
+  verifyFormat("private override String toString()\n"
+               "{\n"
+               "}");
+  verifyFormat("protected override String toString()\n"
+               "{\n"
+               "}");
+  verifyFormat("internal override String toString()\n"
+               "{\n"
+               "}");
 
-  verifyFormat("internal static String toString() {}");
+  verifyFormat("internal static String toString()\n"
+               "{\n"
+               "}");
 }
 
 TEST_F(FormatTestCSharp, NoStringLiteralBreaks) {
   verifyFormat("foo("
                "\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
                "aaaaaa\");");
 }
 

 }
 
 TEST_F(FormatTestCSharp, CSharpNullConditional) {
   verifyFormat(
       "public Person(string firstName, string lastName, int? age=null)");
 
   verifyFormat("switch(args?.Length)");
 
-  verifyFormat("public static void Main(string[] args) { string dirPath "
-               "= args?[0]; }");
+  verifyFormat("public static void Main(string[] args)\n"
+               "{\n"
+               "    string dirPath = args?[0];\n"
+               "}");
 }
 
 TEST_F(FormatTestCSharp, Attributes) {
   verifyFormat("[STAThread]\n"
-               "static void\n"
-               "Main(string[] args) {}");
+               "static void Main(string[] args)\n"
+               "{\n"
+               "}");
 
   verifyFormat("[TestMethod]\n"
-               "private class Test {}");
+               "private class Test\n"
+               "{\n"
+               "}");
 
   verifyFormat("[TestMethod]\n"
-               "protected class Test {}");
+               "protected class Test\n"
+               "{\n"
+               "}");
 
   verifyFormat("[TestMethod]\n"
-               "internal class Test {}");
+               "internal class Test\n"
+               "{\n"
+               "}");
 
   verifyFormat("[TestMethod]\n"
-               "class Test {}");
+               "class Test\n"
+               "{\n"
+               "}");
 
   verifyFormat("[TestMethod]\n"
                "[DeploymentItem(\"Test.txt\")]\n"
-               "public class Test {}");
+               "public class Test\n"
+               "{\n"
+               "}");
 
   verifyFormat("[System.AttributeUsage(System.AttributeTargets.Method)]\n"
                "[System.Runtime.InteropServices.ComVisible(true)]\n"
-               "public sealed class STAThreadAttribute : Attribute {}");
+               "public sealed class STAThreadAttribute : Attribute\n"
+               "{\n"
+               "}");
 
   verifyFormat("[Verb(\"start\", HelpText = \"Starts the server listening on "
                "provided port\")]\n"
-               "class Test {}");
+               "class Test\n"
+               "{\n"
+               "}");
 
   verifyFormat("[TestMethod]\n"
-               "public string Host {\n  set;\n  get;\n}");
+               "public string Host\n"
+               "{\n"
+               "    set;\n"
+               "    get;\n"
+               "}");
 
   verifyFormat("[TestMethod(\"start\", HelpText = \"Starts the server "
                "listening on provided host\")]\n"
-               "public string Host {\n  set;\n  get;\n}");
+               "public string Host\n"
+               "{\n"
+               "    set;\n"
+               "    get;\n"
+               "}");
 }
 
 TEST_F(FormatTestCSharp, CSharpUsing) {
   FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
   Style.SpaceBeforeParens = FormatStyle::SBPO_Always;
   verifyFormat("public void foo() {\n"
                "  using (StreamWriter sw = new StreamWriter (filenameA)) {}\n"
                "}",

 }
 
 TEST_F(FormatTestCSharp, CSharpNullCoalescing) {
   verifyFormat("var test = ABC ?? DEF");
   verifyFormat("string myname = name ?? \"ABC\";");
   verifyFormat("return _name ?? \"DEF\";");
 }
 
+TEST_F(FormatTestCSharp, AttributesIndentation) {
+  FormatStyle Style = getMicrosoftStyle(FormatStyle::LK_CSharp);
+  Style.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
+
+  verifyFormat("[STAThread]\n"
+               "static void Main(string[] args)\n"
+               "{\n"
+               "}",
+               Style);
+
+  verifyFormat("[STAThread]\n"
+               "void "
+               "veryLooooooooooooooongFunctionName(string[] args)\n"
+               "{\n"
+               "}",
+               Style);
+
+  verifyFormat("[STAThread]\n"
+               "veryLoooooooooooooooooooongReturnType "
+               "veryLooooooooooooooongFunctionName(string[] args)\n"
+               "{\n"
+               "}",
+               Style);
+
+  verifyFormat("[SuppressMessage(\"A\", \"B\", Justification = \"C\")]\n"
+               "public override X Y()\n"
+               "{\n"
+               "}\n",
+               Style);
+
+  verifyFormat("[SuppressMessage]\n"
+               "public X Y()\n"
+               "{\n"
+               "}\n",
+               Style);
+
+  verifyFormat("[SuppressMessage]\n"
+               "public override X Y()\n"
+               "{\n"
+               "}\n",
+               Style);
+
+  verifyFormat("public A(B b) : base(b)\n"
+               "{\n"
+               "    [SuppressMessage]\n"
+               "    public override X Y()\n"
+               "    {\n"
+               "    }\n"
+               "}\n",
+               Style);
+}
+
 } // namespace format
 } // end namespace clang