[clang-format] PR48594 BraceWrapping: SplitEmptyRecord ignored for templates

[MyDeveloperDay]

Summary

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

Empty or small templates were not being treated the same way as small classes especially when SplitEmptyRecord was set to true

This revision aims to help this by identifying a case when we should try not to merge the lines together

BraceWrapping:
  AfterStruct: true
  SplitEmptyRecord: true
BreakBeforeBraces: Custom

Input and expected formatted result:

C++
template <class> struct rep
{
};

Actual output:

C++
template <class> struct rep
{};

[JohelEGP]

Thank you.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineFormatter.cpp

252

Why is this not just also in the previous if?
I see that the condition is a bit different, but what would happen if the condition is SplitEmptyRecord && !EmptyBlock?

[JohelEGP]

I have another case:
.clang-format:

BraceWrapping:
  AfterStruct: true
  SplitEmptyRecord: true
BreakBeforeBraces: Custom

Input and expected formatted output:

template <std::regular T> struct rep<std::complex<T>>
{
  using type = T;
};

Actual formatted output:

template <std::regular T> struct rep<std::complex<T>>
{ using type = T; };

[MyDeveloperDay]

Addressing additional usecase found issues in C# tests too

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineFormatter.cpp

252

non empty blocks are handled by the code below.

The reason the condition is different is to do with the fact that the merging is required to ensure concepts are not overly broken apart, this condition purely detects where we want to ensure we split the empty block.

[curdeius]

clang/lib/Format/UnwrappedLineFormatter.cpp

364

Could you please add a comment about this block of code?
Something's like "Don't merge blocks when SplitEmptyRecords is enabled." or whatever you judge more useful/correct.

370

You might want to factor out the check of Previous from this and the next if.

375

You should check the type of Previous before computing PreviousPrevious to avoid the latter when unnecessary.

clang/unittests/Format/FormatTest.cpp

9952

What about testing specializations with empty body?

[MyDeveloperDay]

Address review comments

[curdeius]

LGTM. Thank you!

[leonardchan]

Hi. Up until this point, we've noticed that this patch can produce different formatted files depending on amount of whitespace. For example, given (A):

#include <stdint.h>
namespace fuzzing {}

clang-format with this patch would produced (B):

#include <stdint.h>
namespace fuzzing {
}

but given (C):

#include <stdint.h>
namespace fuzzing {


}

would be formatted to (D):

#include <stdint.h>
namespace fuzzing {

}

The invocation specifically is clang-format --style=google file. Prior to this patch, both inputs (A/C) would give the same output:

#include <stdint.h>
namespace fuzzing {}

Is this unintended behavior and worth looking into? We have tests for generated code that use clang-format to attempt to "unify" expected and generated output for easy comparison. We would expect that extra whitespace not produce different formatted outputs. Thanks.

[curdeius]

That's definitely an unintended behaviour. Please file a bug and possibly mark it as release blocker for LLVM 12. Either a fix will be there soon, or we'll revert.

FYI, simple reproduce:

verifyFormat("#include <stdint.h>\n"
             "namespace rep {}",
             Style);

The problem doesn't happen with #include "stdint.h".

clang/lib/Format/UnwrappedLineFormatter.cpp

374–375

@leonardchan, it seems this is the culprit. the closing > in the #include <...> is treated as if it were a template closer and so it inhibits line merging.

[leonardchan]

In D93839#2523703, @curdeius wrote:

That's definitely an unintended behaviour. Please file a bug and possibly mark it as release blocker for LLVM 12. Either a fix will be there soon, or we'll revert.

FYI, simple reproduce:

verifyFormat("#include <stdint.h>\n"
             "namespace rep {}",
             Style);

The problem doesn't happen with #include "stdint.h".

Thanks for looking into this. I filed https://bugs.llvm.org/show_bug.cgi?id=48891.

clang/lib/Format/UnwrappedLineFormatter.cpp

if (TheLine->Last->is(tok::l_brace) && TheLine->First == TheLine->Last &&
 
       if (Tok && Tok->is(tok::kw_typedef))
         Tok = Tok->getNextNonComment();
       if (Tok && Tok->isOneOf(tok::kw_class, tok::kw_struct, tok::kw_union,
                               tok::kw_extern, Keywords.kw_interface))
         return !Style.BraceWrapping.SplitEmptyRecord && EmptyBlock
                    ? tryMergeSimpleBlock(I, E, Limit)
                    : 0;
+
+      if (Tok && Tok->is(tok::kw_template) &&
+          Style.BraceWrapping.SplitEmptyRecord && EmptyBlock) {
+        return 0;
+      }
     }
 
     // FIXME: TheLine->Level != 0 might or might not be the right check to do.
     // If necessary, change to something smarter.
     bool MergeShortFunctions =
         Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All ||
         (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty &&
          I[1]->First->is(tok::r_brace)) ||
if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() &&
       if (kwId == clang::tok::objc_autoreleasepool ||
           kwId == clang::tok::objc_synchronized)
         return 0;
     }
     // Don't merge block with left brace wrapped after case labels
     if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() &&
         I[-1]->First->isOneOf(tok::kw_case, tok::kw_default))
       return 0;
+
+    // Don't merge an empty template class or struct if SplitEmptyRecords
+    // is defined.
+    if (Style.BraceWrapping.SplitEmptyRecord &&
+        TheLine->Last->is(tok::l_brace) && I != AnnotatedLines.begin() &&
+        I[-1]->Last) {
+      const FormatToken *Previous = I[-1]->Last;
+      if (Previous) {
+        if (Previous->is(tok::comment))
+          Previous = Previous->getPreviousNonComment();
+        if (Previous) {
+          if (Previous->is(tok::greater))
+            return 0;
+          if (Previous->is(tok::identifier)) {
+            const FormatToken *PreviousPrevious =
+                Previous->getPreviousNonComment();
+            if (PreviousPrevious &&
+                PreviousPrevious->isOneOf(tok::kw_class, tok::kw_struct))
+              return 0;
+          }
+        }
+      }
+    }
+
     // Try to merge a block with left brace wrapped that wasn't yet covered
     if (TheLine->Last->is(tok::l_brace)) {
       return !Style.BraceWrapping.AfterFunction ||
                      (I[1]->First->is(tok::r_brace) &&
                       !Style.BraceWrapping.SplitEmptyRecord)
                  ? tryMergeSimpleBlock(I, E, Limit)
                  : 0;
     }

clang/unittests/Format/FormatTest.cpp

verifyFormat("class Foo\n"
                "{\n"
                "  Foo();\n"
                "};",
                Style);
   verifyFormat("typedef class Foo\n"
                "{\n"
                "} Foo_t;",
                Style);
+
+  Style.BraceWrapping.SplitEmptyRecord = true;
+  Style.BraceWrapping.AfterStruct = true;
+  verifyFormat("class rep\n"
+               "{\n"
+               "};",
+               Style);
+  verifyFormat("struct rep\n"
+               "{\n"
+               "};",
+               Style);
+  verifyFormat("template <typename T> class rep\n"
+               "{\n"
+               "};",
+               Style);
+  verifyFormat("template <typename T> struct rep\n"
+               "{\n"
+               "};",
+               Style);
+  verifyFormat("class rep\n"
+               "{\n"
+               "  int x;\n"
+               "};",
+               Style);
+  verifyFormat("struct rep\n"
+               "{\n"
+               "  int x;\n"
+               "};",
+               Style);
+  verifyFormat("template <typename T> class rep\n"
+               "{\n"
+               "  int x;\n"
+               "};",
+               Style);
+  verifyFormat("template <typename T> struct rep\n"
+               "{\n"
+               "  int x;\n"
+               "};",
+               Style);
+  verifyFormat("template <typename T> class rep // Foo\n"
+               "{\n"
+               "  int x;\n"
+               "};",
+               Style);
+  verifyFormat("template <typename T> struct rep // Bar\n"
+               "{\n"
+               "  int x;\n"
+               "};",
+               Style);
+
+  verifyFormat("template <typename T> class rep<T>\n"
+               "{\n"
+               "  int x;\n"
+               "};",
+               Style);
+
+  verifyFormat("template <typename T> class rep<std::complex<T>>\n"
+               "{\n"
+               "  int x;\n"
+               "};",
+               Style);
+  verifyFormat("template <typename T> class rep<std::complex<T>>\n"
+               "{\n"
+               "};",
+               Style);
 }
 
 TEST_F(FormatTest, SplitEmptyStruct) {
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.AfterStruct = true;
   Style.BraceWrapping.SplitEmptyRecord = false;
 

clang/unittests/Format/FormatTestCSharp.cpp

 if (someThings[i][j][k].Contains(myThing)) {
 })",
                Style);
 }
 
 TEST_F(FormatTestCSharp, CSharpGenericTypeConstraints) {
   FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
 
-  verifyFormat(R"(//
-class ItemFactory<T>
-    where T : new() {})",
+  EXPECT_TRUE(Style.BraceWrapping.SplitEmptyRecord);
+
+  verifyFormat("class ItemFactory<T>\n"
+               "    where T : new() {\n"
+               "}",
                Style);
 
-  verifyFormat(R"(//
-class Dictionary<TKey, TVal>
-    where TKey : IComparable<TKey>
-    where TVal : IMyInterface {
-  public void MyMethod<T>(T t)
-      where T : IMyInterface {
-    doThing();
-  }
-})",
+  verifyFormat("class Dictionary<TKey, TVal>\n"
+               "    where TKey : IComparable<TKey>\n"
+               "    where TVal : IMyInterface {\n"
+               "  public void MyMethod<T>(T t)\n"
+               "      where T : IMyInterface {\n"
+               "    doThing();\n"
+               "  }\n"
+               "}",
                Style);
 
-  verifyFormat(R"(//
-class ItemFactory<T>
-    where T : new(), IAnInterface<T>, IAnotherInterface<T>, IAnotherInterfaceStill<T> {})",
+  verifyFormat("class ItemFactory<T>\n"
+               "    where T : new(), IAnInterface<T>, IAnotherInterface<T>, "
+               "IAnotherInterfaceStill<T> {\n"
+               "}",
                Style);
 
   Style.ColumnLimit = 50; // Force lines to be wrapped.
   verifyFormat(R"(//
 class ItemFactory<T, U>
     where T : new(),
               IAnInterface<T>,
               IAnotherInterface<T, U>,
-              IAnotherInterfaceStill<T, U> {})",
+              IAnotherInterfaceStill<T, U> {
+})",
                Style);
 
   // In other languages `where` can be used as a normal identifier.
   // This example is in C++!
   verifyFormat(R"(//
 class A {
   int f(int where) {}
 };)",
                getGoogleStyle(FormatStyle::LK_Cpp));
 }
 
 } // namespace format
 } // end namespace clang