[clang-format] [PR51640] - New AfterEnum brace wrapping changes have cause C# behaviour to change

[MyDeveloperDay]

Summary

LLVM 13.0.0-rc2 shows change of behaviour in enum and interface BraceWrapping (likely before we simply didn't wrap) but may be related to D99840: [clang-format] Correctly attach enum braces with ShortEnums disabled

Logged as https://bugs.llvm.org/show_bug.cgi?id=51640

This change ensure AfterEnum works for

internal|public|protected|private enum A { in the same way as it works for enum A { in C++

A similar issue was also observed with interface in C#

[MyDeveloperDay]

Remove the unnecessary comment changes

[krasimir]

clang/lib/Format/TokenAnnotator.cpp

3803

nit: the && Style.BraceWrapping.AfterClass is unnecessary since we've ensured that in the parent if. I'd just fold the kw_interface case into the previous if, and then it seems we can further fold the remaining inner if into the parent if, resulting in this whole section (lines 3796--3805) turning into a single large if statement.

[MyDeveloperDay]

Address Nit to reduce if nesting

[----------] Global test environment tear-down
[==========] 814 tests from 25 test suites ran. (41295 ms total)
[  PASSED  ] 814 tests.

clang/lib/Format/TokenAnnotator.cpp

3803

nice catch, will do

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

3788–3790

Nit: can you add a boolean above the outer if so that it can be used in the if below?

Also, Line.First is inconsistent with Line.startsWith elsewhere? I think the latter skips leading comments.

[MyDeveloperDay]

Handle comments before the enum

[owenpan]

LGTM.

clang/lib/Format/TokenAnnotator.cpp

3789

Nit: I think it would be a little more efficient to call FirstNonComment->is instead.

clang/lib/Format/TokenAnnotator.cpp

if ((Right.Previous->is(tok::l_brace) ||
       Right.NestingLevel == 1 && Style.Language == FormatStyle::LK_Proto) {
     // Don't put enums or option definitions onto single lines in protocol
     // buffers.
     return true;
   }
   if (Right.is(TT_InlineASMBrace))
     return Right.HasUnescapedNewline;
 
-  if (isAllmanBrace(Left) || isAllmanBrace(Right))
-    return (Line.startsWith(tok::kw_enum) && Style.BraceWrapping.AfterEnum) ||
-           (Line.startsWith(tok::kw_typedef, tok::kw_enum) &&
-            Style.BraceWrapping.AfterEnum) ||
-           (Line.startsWith(tok::kw_class) && Style.BraceWrapping.AfterClass) ||
+  if (isAllmanBrace(Left) || isAllmanBrace(Right)) {
+    if (Style.BraceWrapping.AfterEnum) {
+      if (Line.startsWith(tok::kw_enum) ||
+          Line.startsWith(tok::kw_typedef, tok::kw_enum))
+        return true;
+      if (Style.isCSharp()) {
+        if (Line.First && Line.First->Next &&
+            Line.First->isOneOf(Keywords.kw_internal, tok::kw_public,
+                                tok::kw_private, tok::kw_protected) &&
+            Line.First->Next->is(tok::kw_enum))
+          return true;
+      }
+    }
+
+    // Ensure BraceWrapping for public interface A {
+    if (Style.BraceWrapping.AfterClass && Style.isCSharp()) {
+      if (Line.First && Line.First->Next &&
+          Line.First->isOneOf(Keywords.kw_internal, tok::kw_public,
+                              tok::kw_private, tok::kw_protected) &&
+          Line.First->Next->is(Keywords.kw_interface))
+        return true;
+      if (Line.startsWith(Keywords.kw_interface) &&
+          Style.BraceWrapping.AfterClass)
+        return true;
+    }
+    return (Line.startsWith(tok::kw_class) && Style.BraceWrapping.AfterClass) ||
            (Line.startsWith(tok::kw_struct) && Style.BraceWrapping.AfterStruct);
+  }
   if (Left.is(TT_ObjCBlockLBrace) &&
       Style.AllowShortBlocksOnASingleLine == FormatStyle::SBS_Never)
     return true;
 
   if (Left.is(TT_LambdaLBrace)) {
     if (IsFunctionArgument(Left) &&
         Style.AllowShortLambdasOnASingleLine == FormatStyle::SLS_Inline)
       return false;

clang/unittests/Format/FormatTestCSharp.cpp

 }
 
 TEST_F(FormatTestCSharp, CSharpRegions) {
   verifyFormat("#region aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaa "
                "aaaaaaaaaaaaaaa long region");
 }
 
 TEST_F(FormatTestCSharp, CSharpKeyWordEscaping) {
-  verifyFormat("public enum var {\n"
+  // AfterEnum is true by default.
+  verifyFormat("public enum var\n"
+               "{\n"
                "    none,\n"
                "    @string,\n"
                "    bool,\n"
                "    @enum\n"
                "}");
 }
 
 TEST_F(FormatTestCSharp, CSharpNullCoalescing) {
class ItemFactory<T, U>
   // This example is in C++!
   verifyFormat(R"(//
 class A {
   int f(int where) {}
 };)",
                getGoogleStyle(FormatStyle::LK_Cpp));
 }
 
+TEST_F(FormatTestCSharp, CSharpAfterEnum) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterEnum = false;
+  Style.AllowShortEnumsOnASingleLine = false;
+
+  verifyFormat("enum MyEnum {\n"
+               "  Foo,\n"
+               "  Bar,\n"
+               "}",
+               Style);
+  verifyFormat("internal enum MyEnum {\n"
+               "  Foo,\n"
+               "  Bar,\n"
+               "}",
+               Style);
+  verifyFormat("public enum MyEnum {\n"
+               "  Foo,\n"
+               "  Bar,\n"
+               "}",
+               Style);
+  verifyFormat("protected enum MyEnum {\n"
+               "  Foo,\n"
+               "  Bar,\n"
+               "}",
+               Style);
+  verifyFormat("private enum MyEnum {\n"
+               "  Foo,\n"
+               "  Bar,\n"
+               "}",
+               Style);
+
+  Style.BraceWrapping.AfterEnum = true;
+  Style.AllowShortEnumsOnASingleLine = false;
+
+  verifyFormat("enum MyEnum\n"
+               "{\n"
+               "  Foo,\n"
+               "  Bar,\n"
+               "}",
+               Style);
+  verifyFormat("internal enum MyEnum\n"
+               "{\n"
+               "  Foo,\n"
+               "  Bar,\n"
+               "}",
+               Style);
+  verifyFormat("public enum MyEnum\n"
+               "{\n"
+               "  Foo,\n"
+               "  Bar,\n"
+               "}",
+               Style);
+  verifyFormat("protected enum MyEnum\n"
+               "{\n"
+               "  Foo,\n"
+               "  Bar,\n"
+               "}",
+               Style);
+  verifyFormat("private enum MyEnum\n"
+               "{\n"
+               "  Foo,\n"
+               "  Bar,\n"
+               "}",
+               Style);
+}
+
+TEST_F(FormatTestCSharp, CSharpAfterClass) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterClass = false;
+
+  verifyFormat("class MyClass {\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("internal class MyClass {\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("public class MyClass {\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("protected class MyClass {\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("private class MyClass {\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+
+  verifyFormat("interface Interface {\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("internal interface Interface {\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("public interface Interface {\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("protected interface Interface {\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("private interface Interface {\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+
+  Style.BraceWrapping.AfterClass = true;
+
+  verifyFormat("class MyClass\n"
+               "{\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("internal class MyClass\n"
+               "{\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("public class MyClass\n"
+               "{\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("protected class MyClass\n"
+               "{\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("private class MyClass\n"
+               "{\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+
+  verifyFormat("interface MyInterface\n"
+               "{\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("internal interface MyInterface\n"
+               "{\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("public interface MyInterface\n"
+               "{\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("protected interface MyInterface\n"
+               "{\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+  verifyFormat("private interface MyInterface\n"
+               "{\n"
+               "  int a;\n"
+               "  int b;\n"
+               "}",
+               Style);
+}
+
 } // namespace format
 } // end namespace clang