[clang-format] Fix CompactNamespaces corner case when AllowShortLambdasOnASingleLine/BraceWrapping.BeforeLambdaBody are set

[curdeius]

Summary

In clang-format 12, CompactNamespaces misformatted the code when AllowShortLambdasOnASingleLine is set to false and BraceWrapping.BeforeLambdaBody is true.

Input:

namespace out {
namespace in {
} 
} // namespace out::in

Expected output:

namespace out { namespace in {
}} // namespace out::in

Output from v12:

namespace out {
namespace in {
} 
} // namespace out::in

Config triggering the issue:

---
AllowShortLambdasOnASingleLine: None
BraceWrapping:
  BeforeLambdaBody :    true
BreakBeforeBraces: Custom
CompactNamespaces: true
...

Seems there's a corner case when AllowShortLambdasOnASingleLine is false, and BraceWrapping.BeforeLambdaBody is true, that causes CompactNamespaces to stop working.
The cause was a misannotation of { opening brace after namespace as a lambda opening brace.
The regression was probably introduced with this commit.

Originally contributed by Ahmed Mahdy (@aybassiouny). Thank you!

[Wawha]

Thank you for the fix!

[HazardyKnusperkeks]

Looks good. How about verifyFormat? Would that fail without your patch?

[aybassiouny]

How about verifyFormat? Would that fail without your patch?

@HazardyKnusperkeks sorry, but not sure that I understand your point. Are you suggesting to use verifyFormat in my UT instead of EXPECT_EQ?

[HazardyKnusperkeks]

In D99031#2640425, @aybassiouny wrote:

How about verifyFormat? Would that fail without your patch?

@HazardyKnusperkeks sorry, but not sure that I understand your point. Are you suggesting to use verifyFormat in my UT instead of EXPECT_EQ?

I'm pretty sure @MyDeveloperDay would ask for that. I would maybe use both. But what it really interesting for me if verifyFormat would fail, i.e. would clang-format misformat something previously correctly formatted.

[MyDeveloperDay]

In D99031#2640654, @HazardyKnusperkeks wrote:

In D99031#2640425, @aybassiouny wrote:

How about verifyFormat? Would that fail without your patch?

@HazardyKnusperkeks sorry, but not sure that I understand your point. Are you suggesting to use verifyFormat in my UT instead of EXPECT_EQ?

I'm pretty sure @MyDeveloperDay would ask for that. I would maybe use both. But what it really interesting for me if verifyFormat would fail, i.e. would clang-format misformat something previously correctly formatted.

I would, see D98237: [clang-format] Option for empty lines after an access modifier. as to why.

[aybassiouny]

Sorry for the delay!

After rechecking, turns out AllowShortLambdasOnASingleLine and BeforeLambdaBody both need to be turned on in order for the regression to be expressed, this affects the UT.

Also added verifyFormat check as suggested, it does not fail btw without this patch.

[HazardyKnusperkeks]

LGTM

[curdeius]

In D99031#2697563, @aybassiouny wrote:

After rechecking, turns out AllowShortLambdasOnASingleLine and BeforeLambdaBody both need to be turned on in order for the regression to be expressed, this affects the UT.

Please update the summary with this information.

clang/unittests/Format/FormatTest.cpp

2594

Nit.

[curdeius]

clang/lib/Format/TokenAnnotator.cpp

3501

Please add at least asserts for Tok.Previous and Tok.Previous->Previous.

[aybassiouny]

Add an assert

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

3496–3501

I find this really hard to read. Maybe you have to split the return.

[curdeius]

@aybassiouny, still interested in working on this?

[curdeius]

This bug is not present in clang-format 13 (nor master), but was there in v12.
I'll commandeer this revision to keep the regression tests.

[curdeius]

Keep tests only. Clean up.
Rebase.

[aybassiouny]

Thanks @curdeius for checking this in, apologies for delay on my side. Glad the bug is not there in the new version!

clang/lib/Format/TokenAnnotator.cpp

3501

I almost see no asserts despite similar usage, but sure

clang/lib/Format/TokenAnnotator.cpp

static bool isOneChildWithoutMustBreakBefore(const FormatToken &Tok) {
   while (curElt) {
     if (curElt->MustBreakBefore)
       return false;
     curElt = curElt->Next;
   }
   return true;
 }
 static bool isAllmanLambdaBrace(const FormatToken &Tok) {
+  assert(!Tok.is(tok::l_brace) || !Tok.is(BK_Block) ||
+         !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral) ||
+         (Tok.Previous && Tok.Previous->Previous));
   return (Tok.is(tok::l_brace) && Tok.is(BK_Block) &&
-          !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral));
+          !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral) &&
+          !Tok.Previous->Previous->is(tok::kw_namespace));
 }
 
 static bool isAllmanBraceIncludedBreakableLambda(
     const FormatToken &Tok, FormatStyle::ShortLambdaStyle ShortLambdaOption) {
   if (!isAllmanLambdaBrace(Tok))
     return false;
 
   if (isItAnEmptyLambdaAllowed(Tok, ShortLambdaOption))

clang/unittests/Format/FormatTest.cpp

EXPECT_EQ("namespace out { namespace mid { namespace in {\n"
                    "namespace in {\n"
                    "int i;\n"
                    "} // namespace in\n"
                    "} // namespace mid\n"
                    "} // namespace out",
                    Style));
 }
 
+TEST_F(FormatTest, FormatsCompactNamespacesLambdaRegression) {
+  // Make sure compact namespaces are not confused with lambdas
+  FormatStyle CompactNamespacesStyle{getLLVMStyle()};
+  CompactNamespacesStyle.CompactNamespaces = true;
+  CompactNamespacesStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_None;
+  CompactNamespacesStyle.BreakBeforeBraces = FormatStyle::BS_Custom;
+  CompactNamespacesStyle.BraceWrapping.BeforeLambdaBody = true;
+  verifyFormat("namespace out { namespace in {\n"
+               "}} // namespace out::in",
+               CompactNamespacesStyle);
+  EXPECT_EQ("namespace out { namespace in {\n"
+            "}} // namespace out::in",
+            format("namespace out {\n"
+                   "namespace in {\n"
+                   "} // namespace in\n"
+                   "} // namespace out",
+                   CompactNamespacesStyle));
+}
+
 TEST_F(FormatTest, FormatsExternC) {
   verifyFormat("extern \"C\" {\nint a;");
   verifyFormat("extern \"C\" {}");
   verifyFormat("extern \"C\" {\n"
                "int foo();\n"
                "}");
   verifyFormat("extern \"C\" int foo() {}");
   verifyFormat("extern \"C\" int foo();");