[clang-format] Fix formatting of the code that follows C# Lambda Expressions

[peterstys]

Summary

The alignment fix introduced by https://reviews.llvm.org/D104388 caused a regression whereby formatting of code that follows the lambda block is incorrect i.e. separate expressions are put on the same line.

For example:

public void Test() {
  while (true) {
    preBindEnumerators.RemoveAll(enumerator => !enumerator.MoveNext());
    CodeThatFollowsLambda();
    IsWellAligned(); 
}

will be formatted as:

public void Test() {
  while (true) {
    preBindEnumerators.RemoveAll(enumerator => !enumerator.MoveNext());
    CodeThatFollowsLambda(); IsWellAligned(); 
}

The problem is that the "Fat arrow" token parsing inside the parseParens() function uses parseStructuralElement() which does not like to be called inside the parenthesis. It seems that code that follows is considered part of the parenthesis block.

As a fix, parseStructuralElement parser inside the parseParens() was replaced with parseChildBlock() if the following token was a "left brace" and nextToken() otherwise. Added few new unit tests to demonstrate the regressions.

[peterstys]

Applied clang-formatting.

[MyDeveloperDay]

LGTM

[MyDeveloperDay]

I tested this on the original code that made me make the original change, and I like your fix much better ;-)

Thank you for this patch, interested on working on other C# clang-format issues?

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

2012

You already tested for C# on line 2004.

2014

Remove braces.

2017

Add braces after else to match if.

[jbcoe]

clang/lib/Format/UnwrappedLineParser.cpp

2011

I think this check for Style.isCSharp() is redundant as it's already checked above.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

2012

Actually, if (Style.BraceWrapping.AfterFunction) will do, i.e., == true is redundant.

[peterstys]

Applied fixes suggested by the reviewers.

[peterstys]

Removed braces.

[peterstys]

In D115738#3193107, @MyDeveloperDay wrote:

I tested this on the original code that made me make the original change, and I like your fix much better ;-)

Thank you for this patch, interested on working on other C# clang-format issues?

Happy to help. My team is using clang-format to format our C# codebase and we have few more issues that we'd like to help fixing.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

1878–1879

Let's fix this too while we are at it. In the future, we may want to factor lines 1862-1874 and 2004-2009.

2013

Remove trailing whitespaces.

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

1878–1879

part of me thinks the MustBreakBefore should be handled separately in TokenAnnotator::mustBreakBefore() and let it do its stuff.

[MyDeveloperDay]

clang/unittests/Format/FormatTestCSharp.cpp

766

Nit: (only my preference) but I don't like the use of RawStrings in these tests (I know others have let them creep in) but I find them more unreadable because we lose the indentation.. I think I'm just so used to the other style that this just crates a little.

Just my personal preference (you can ignore)

[peterstys]

Created tryParseCSharpLambda function to avoid code duplication.

[owenpan]

Thanks for factoring the code.

clang/lib/Format/UnwrappedLineParser.cpp

1859

Early return.

1878–1879

Yes, but we can take care of it in a separate patch.

clang/lib/Format/UnwrappedLineParser.h

141

To keep the naming consistent.

[peterstys]

clang/lib/Format/UnwrappedLineParser.cpp

1878–1879

I used MustBreakBefore = true because the same method was used for the FatArrow processing on the lines 1862-1874 and it did the right thing i.e. added a new line before the block.

If you want me to change the code to use TokenAnnotator::mustBreakBefore() please provide more details on how to do it, or point me to some examples. I failed to find any good references, I'm afraid.

1878–1879

I extracted the C# lambda parsing block into a separate function to avoid repeating the same logic twice. Hope that is alright.

1878–1879

ACK

clang/unittests/Format/FormatTestCSharp.cpp

766

I used the RawStrings as it was very easy to copy snippet of code to a separate file so I could run clang-format on it to check all was working well. I also copied snippets to my local IDE to check if the file was building, or at least structurally okay (which I appreciate is not a requirements as clang-format does not build a complete AST of the code).

For example, I was surprised to see some sample code in unit tests missing braces after catch (Exception) statement (line 694) (which may have been intended). I find it slightly more difficult to read those code snippets if they are decorated with \n.

On the other hand, you're right, raw string break the indentation which is not great. Also, I'd rather follow the leading preferences (and wasn't sure which ones where they before your comment). I'm happy to update this to your style if that is the preference.

[peterstys]

clang/unittests/Format/FormatTestCSharp.cpp

766

I tried to apply your preferred style but after running clang-format on the test file, many files were split which made it look much less readable, in my opinion. Here's an example:

verifyFormat(
    "public class Test\n"
    "{\n"
    "    private static void ComplexLambda(BuildReport protoReport)\n"
    "    {\n"
    "        allSelectedScenes =\n"
    "            "
    "veryVeryLongCollectionNameThatPutsTheLineLenghtAboveTheThresholds.Where("
    "scene => scene.enabled)\n"
    "                .Select(scene => scene.path)\n"
    "                .ToArray();\n"
    "        if (allSelectedScenes.Count == 0)\n"
    "        {\n"
    "            return;\n"
    "        }\n"
    "        Functions();\n"
    "        AreWell();\n"
    "        Aligned();\n"
    "        AfterLambdaBlock();\n"
    "    }\n"
    "}",
    MicrosoftStyle);

I think it makes it look cleaner and more readable if we stick to the raw string literals for my tests. WDYT?

[peterstys]

clang/unittests/Format/FormatTestCSharp.cpp

766

by "many files" I meant "many code snippets".

[owenpan]

Thanks! LGTM other than the extraneous braces on lines 1859-1861. (See here.)

clang/lib/Format/UnwrappedLineParser.cpp

1859

Remove braces.

[peterstys]

This is my first PR into this repo. I'd like to learn more about the process of submitting patches, specifically about:

• Do I need to get LGTM from all the reviewers before I can submit it?
• There are some build failures on the CI, but look unrelated, do I force submit passed them?

Could someone help me with these questions or point me to the relevant documentation. Thanks.

[MyDeveloperDay]

In D115738#3199879, @peterstys wrote:

This is my first PR into this repo. I'd like to learn more about the process of submitting patches, specifically about:

• Do I need to get LGTM from all the reviewers before I can submit it?

No, one is likely enough, but employ some level of respect for others if they are commenting (they may not explicitly say LGTM but may accept)

• There are some build failures on the CI, but look unrelated, do I force submit passed them?

They CI doesn't block commit. just check to ensure its not you, (there is some odd Go issue ATM which I've seen reported elsewhere)

Could someone help me with these questions or point me to the relevant documentation. Thanks.

https://llvm.org/docs/Contributing.html
https://llvm.org/docs/Phabricator.html

[peterstys]

In D115738#3199896, @MyDeveloperDay wrote:

In D115738#3199879, @peterstys wrote:

This is my first PR into this repo. I'd like to learn more about the process of submitting patches, specifically about:

• Do I need to get LGTM from all the reviewers before I can submit it?

No, one is likely enough, but employ some level of respect for others if they are commenting (they may not explicitly say LGTM but may accept)

• There are some build failures on the CI, but look unrelated, do I force submit passed them?

They CI doesn't block commit. just check to ensure its not you, (there is some odd Go issue ATM which I've seen reported elsewhere)

Could someone help me with these questions or point me to the relevant documentation. Thanks.

https://llvm.org/docs/Contributing.html
https://llvm.org/docs/Phabricator.html

Great. Thank you. Very helpful.

Regarding failing CI: it seems that the LLVM_ALL_PROJECTS in CMakeLists.txt miss "bolt" setting (line 67): set(LLVM_ALL_PROJECTS "clang;clang-tools-extra;compiler-rt;cross-project-tests;libc;libclc;libcxx;libcxxabi;libunwind;lld;lldb;mlir;openmp;polly;pstl")

Regarding committing the change: I won't have commit rights. Could someone do it for me?

[MyDeveloperDay]

Thank you for the patch We'll need your name and email address for that, but yes we'll be happy to commit it for you.

If you think you'd like to play some more, then you can apply for commit access. (not sure of the process for that now, but its probably in the documentation somewhere)

[peterstys]

In D115738#3199988, @MyDeveloperDay wrote:

Thank you for the patch We'll need your name and email address for that, but yes we'll be happy to commit it for you.

If you think you'd like to play some more, then you can apply for commit access. (not sure of the process for that now, but its probably in the documentation somewhere)

Peter Stys
[e-mail]

Thank you!

clang/lib/Format/UnwrappedLineParser.h

private:
   void parseJavaScriptEs6ImportExport();
   void parseStatementMacro();
   void parseCSharpAttribute();
   // Parse a C# generic type constraint: `where T : IComparable<T>`.
   // See:
   // https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/where-generic-type-constraint
   void parseCSharpGenericTypeConstraint();
   bool tryToParseLambda();
+  bool tryParseCSharpLambda();
   bool tryToParseLambdaIntroducer();
   bool tryToParsePropertyAccessor();
   void tryToParseJSFunction();
   bool tryToParseSimpleAttribute();
 
   // Used by addUnwrappedLine to denote whether to keep or remove a level
   // when resetting the line state.
   enum class LineLevel { Remove, Keep };

clang/lib/Format/UnwrappedLineParser.cpp

bool UnwrappedLineParser::tryToParseBracedList() {
   assert(FormatTok->isNot(BK_Unknown));
   if (FormatTok->is(BK_Block))
     return false;
   nextToken();
   parseBracedList();
   return true;
 }
 
-bool UnwrappedLineParser::parseBracedList(bool ContinueOnSemicolons,
-                                          bool IsEnum,
-                                          tok::TokenKind ClosingBraceKind) {
-  bool HasError = false;
-
-  // FIXME: Once we have an expression parser in the UnwrappedLineParser,
-  // replace this by using parseAssignmentExpression() inside.
-  do {
-    if (Style.isCSharp()) {
-      // Fat arrows (=>) have tok::TokenKind tok::equal but TokenType
-      // TT_FatArrow. They always start an expression or a child block if
-      // followed by a curly brace.
-      if (FormatTok->is(TT_FatArrow)) {
-        nextToken();
-        if (FormatTok->is(tok::l_brace)) {
-          // C# may break after => if the next character is a newline.
-          if (Style.isCSharp() && Style.BraceWrapping.AfterFunction == true) {
-            // calling `addUnwrappedLine()` here causes odd parsing errors.
-            FormatTok->MustBreakBefore = true;
-          }
-          parseChildBlock();
-          continue;
-        }
-      }
-    }
+bool UnwrappedLineParser::tryParseCSharpLambda() {
+    if (Style.BraceWrapping.AfterFunction) {
+    return true;
+  return false;
+
+bool UnwrappedLineParser::parseBracedList(bool ContinueOnSemicolons,
+                                          bool IsEnum,
+                                          tok::TokenKind ClosingBraceKind) {
+  bool HasError = false;
+
+  // FIXME: Once we have an expression parser in the UnwrappedLineParser,
+  // replace this by using parseAssignmentExpression() inside.
+  do {
+    if (Style.isCSharp() && FormatTok->is(TT_FatArrow))
+      if (tryParseCSharpLambda())
+        continue;
     if (Style.Language == FormatStyle::LK_JavaScript) {
       if (FormatTok->is(Keywords.kw_function) ||
           FormatTok->startsSequence(Keywords.kw_async, Keywords.kw_function)) {
         tryToParseJSFunction();
         continue;
       }
       if (FormatTok->is(TT_FatArrow)) {
         nextToken();
case tok::at:
       nextToken();
       if (FormatTok->Tok.is(tok::l_brace)) {
         nextToken();
         parseBracedList();
       }
       break;
     case tok::equal:
       if (Style.isCSharp() && FormatTok->is(TT_FatArrow))
-        parseStructuralElement();
+        tryParseCSharpLambda();
       else
         nextToken();
       break;
     case tok::kw_class:
       if (Style.Language == FormatStyle::LK_JavaScript)
         parseRecord(/*ParseAsExpr=*/true);
       else
         nextToken();
       break;
     case tok::identifier:
       if (Style.Language == FormatStyle::LK_JavaScript &&
           (FormatTok->is(Keywords.kw_function) ||
            FormatTok->startsSequence(Keywords.kw_async, Keywords.kw_function)))
         tryToParseJSFunction();
       else
         nextToken();
       break;

clang/unittests/Format/FormatTestCSharp.cpp

verifyFormat("void bar() {\n"
                "        A.Remove(item);\n"
                "      }\n"
                "    }\n"
                "  }));\n"
                "}",
                GoogleStyle);
 }
 
+TEST_F(FormatTestCSharp, CSharpLambdasDontBreakFollowingCodeAlignment) {
+  FormatStyle GoogleStyle = getGoogleStyle(FormatStyle::LK_CSharp);
+  FormatStyle MicrosoftStyle = getMicrosoftStyle(FormatStyle::LK_CSharp);
+
+  verifyFormat(R"(//
+public class Sample {
+  public void Test() {
+    while (true) {
+      preBindEnumerators.RemoveAll(enumerator => !enumerator.MoveNext());
+      CodeThatFollowsLambda();
+      IsWellAligned();
+    }
+  }
+})",
+               GoogleStyle);
+
+  verifyFormat(R"(//
+public class Sample
+{
+    public void Test()
+    {
+        while (true)
+        {
+            preBindEnumerators.RemoveAll(enumerator => !enumerator.MoveNext());
+            CodeThatFollowsLambda();
+            IsWellAligned();
+        }
+    }
+})",
+               MicrosoftStyle);
+}
+
+TEST_F(FormatTestCSharp, CSharpLambdasComplexLambdasDontBreakAlignment) {
+  FormatStyle GoogleStyle = getGoogleStyle(FormatStyle::LK_CSharp);
+  FormatStyle MicrosoftStyle = getMicrosoftStyle(FormatStyle::LK_CSharp);
+
+  verifyFormat(R"(//
+public class Test
+{
+    private static void ComplexLambda(BuildReport protoReport)
+    {
+        allSelectedScenes =
+            veryVeryLongCollectionNameThatPutsTheLineLenghtAboveTheThresholds.Where(scene => scene.enabled)
+                .Select(scene => scene.path)
+                .ToArray();
+        if (allSelectedScenes.Count == 0)
+        {
+            return;
+        }
+        Functions();
+        AreWell();
+        Aligned();
+        AfterLambdaBlock();
+    }
+})",
+               MicrosoftStyle);
+
+  verifyFormat(R"(//
+public class Test {
+  private static void ComplexLambda(BuildReport protoReport) {
+    allSelectedScenes = veryVeryLongCollectionNameThatPutsTheLineLenghtAboveTheThresholds
+                            .Where(scene => scene.enabled)
+                            .Select(scene => scene.path)
+                            .ToArray();
+    if (allSelectedScenes.Count == 0) {
+      return;
+    }
+    Functions();
+    AreWell();
+    Aligned();
+    AfterLambdaBlock();
+  }
+})",
+               GoogleStyle);
+}
+
+TEST_F(FormatTestCSharp, CSharpLambdasMulipleLambdasDontBreakAlignment) {
+  FormatStyle GoogleStyle = getGoogleStyle(FormatStyle::LK_CSharp);
+  FormatStyle MicrosoftStyle = getMicrosoftStyle(FormatStyle::LK_CSharp);
+
+  verifyFormat(R"(//
+public class Test
+{
+    private static void MultipleLambdas(BuildReport protoReport)
+    {
+        allSelectedScenes =
+            veryVeryLongCollectionNameThatPutsTheLineLenghtAboveTheThresholds.Where(scene => scene.enabled)
+                .Select(scene => scene.path)
+                .ToArray();
+        preBindEnumerators.RemoveAll(enumerator => !enumerator.MoveNext());
+        if (allSelectedScenes.Count == 0)
+        {
+            return;
+        }
+        Functions();
+        AreWell();
+        Aligned();
+        AfterLambdaBlock();
+    }
+})",
+               MicrosoftStyle);
+
+  verifyFormat(R"(//
+public class Test {
+  private static void MultipleLambdas(BuildReport protoReport) {
+    allSelectedScenes = veryVeryLongCollectionNameThatPutsTheLineLenghtAboveTheThresholds
+                            .Where(scene => scene.enabled)
+                            .Select(scene => scene.path)
+                            .ToArray();
+    preBindEnumerators.RemoveAll(enumerator => !enumerator.MoveNext());
+    if (allSelectedScenes.Count == 0) {
+      return;
+    }
+    Functions();
+    AreWell();
+    Aligned();
+    AfterLambdaBlock();
+  }
+})",
+               GoogleStyle);
+}
+
 TEST_F(FormatTestCSharp, CSharpObjectInitializers) {
   FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
 
   // Start code fragments with a comment line so that C++ raw string literals
   // as seen are identical to expected formatted code.
 
   verifyFormat(R"(//
 Shape[] shapes = new[] {