[clang-format] PR50727 C# Invoke Lamda Expression indentation incorrect

[MyDeveloperDay]

Summary

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

When processing C# Lambda expression in the indentation can goes a little wrong,
resulting the the closing } being at the wrong indentation level and meaning the remaining part of the file is
incorrectly indented.

This revision tries to address that for C#

This can be a fairly common pattern for when C# wants to peform a UI action from a thread,
and it wants to invoke that action on the main thread

class A
{

    void foo()
    {
    Dispatcher.Invoke(DispatcherPriority.Render, (Action)(() => {
                                                  lock (A)
                                                  {
                                                      if (true)
                                                      {
                                                          A.Remove(item);
    }
}
}));
}

void bar()
{
...
}

[MyDeveloperDay]

I wanted to add some more tests and this really only manifested itself using lock and using although the use of them is not generally broken when not using them in a Lambda expression

[MyDeveloperDay]

ping

[HazardyKnusperkeks]

I have basically no idea about C#, thus not much to say. I think if the old tests pass and the new are as expected it is good.

clang/unittests/Format/FormatTestCSharp.cpp

647

How is that indention calculated? Is this position fix? Or is it always one column less than the (presumably) start of the lambda?

Maybe add a test not with Val, but something else as first argument. Or is this not valid?

As said, basically no idea about C#.

[exv]

I have a few concerns about this implementation:

• If the incorrect formatting is due to lock and using not being recognized correctly, shouldn't we instead fix how those keywords are parsed?
• In the examples of Microsoft style that I've seen, statement lambdas always place the brace on a new line from the arrow. If used as a function argument like this, does that rule no longer apply? I'm having difficulty finding an example of that specific usage in Microsoft's docs.

[exv]

Oops, didn't mean to accept, see my other comment.

[MyDeveloperDay]

lock and using will be considered identifiers where as if and while will be seens as if/while

M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=99 Name=identifier L=66 PPK=2 FakeLParens= FakeRParens=0 II=0x1fdc7a0 Text='lock'

From what I can tell they will be handled by the parsing parseIfElse etc.. what this tends to do is to jump from { to matching } which means it doesn't have to parse whats inside

This is why by detecting the => then parsing the { -> } of the lamdba as just and ordinary block.

I'm not 100% sure of the indentiation, I was assuming clang-format was just doing its thing...to be honest in its current form clang-format is devastating the files I'm looking at.. any level of indentation is better than what I'm getting

Ultimately we probably need to see how it interacts with D102706: [clang-format] Add new LambdaBodyIndentation option

[MyDeveloperDay]

Fix parseBracedList() to do what parseStructuralElement() does (with regard to breaking AfterFunction, I'm not completely convinced AfterFunction should have been reused for that but might be too late to change.

This fixes the somewhat random indentation but also ensue the { is on a newline in Microsoft style like the other lambda in the unit tests, add tests to show that the indentation is being handled by the IndentWidth and that GoogleStyle handles the { positioning correctly.

[MyDeveloperDay]

ping

[curdeius]

I can't help with C# part either, unfortunately.
I'll chime in if I see something strange, but current patch looks okay to me. I let other reviewers (@exv) accept it when they feel it's ok.

clang/lib/Format/UnwrappedLineParser.cpp

1793

Unrelated nit: typo in parseAssignment.

1797

"The*y* always"... ?

1798

curly brace?

[jbcoe]

Some outstanding nits to address but this looks good to me.

With @krasimir I implemented a good fraction of the C# support and am confident that these changes are an improvement.

[exv]

Sorry, been away from my setup lately. I'll approve once I'm back at my PC
hopefully by EOD.

[MyDeveloperDay]

I implemented a good fraction of the C# support and am confident that these changes are an improvement.

Thank you.

I've been very grateful to you all (which is why I included you as reviewers) for filling in the gaps in the C# formatting, but I'm super grateful we've been able to get to the point where its very usable, because I use C# in my personal projects and C#/C++ for my work, but I tend not to push the envelop on the latest language features.

[MyDeveloperDay]

Correct a couple of Nits prior to commit. (just updating patch for completeness)

clang/lib/Format/UnwrappedLineParser.cpp

case tok::identifier: {
           addUnwrappedLine();
           return;
         }
       }
       break;
     }
     case tok::equal:
       // Fat arrows (=>) have tok::TokenKind tok::equal but TokenType
-      // TT_FatArrow. The always start an expression or a child block if
-      // followed by a curly.
+      // 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;
           }

 }
 
 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 parseAssigmentExpression() inside.
+  // 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();
-        // Fat arrows can be followed by simple expressions or by child blocks
-        // in curly braces.
         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;
         }
       }
     }
     if (Style.Language == FormatStyle::LK_JavaScript) {
       if (FormatTok->is(Keywords.kw_function) ||
           FormatTok->startsSequence(Keywords.kw_async, Keywords.kw_function)) {
case tok::l_brace:
       break;
     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();
+      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 &&

clang/unittests/Format/FormatTestCSharp.cpp

 {
     Action<string> greet = name =>
     {
         string greeting = $"Hello {name}!";
         Console.WriteLine(greeting);
     };
 })",
                MicrosoftStyle);
+
+  verifyFormat("void bar()\n"
+               "{\n"
+               "    Function(Val, (Action)(() =>\n"
+               "                           {\n"
+               "                               lock (mylock)\n"
+               "                               {\n"
+               "                                   if (true)\n"
+               "                                   {\n"
+               "                                       A.Remove(item);\n"
+               "                                   }\n"
+               "                               }\n"
+               "                           }));\n"
+               "}",
+               MicrosoftStyle);
+
+  verifyFormat("void baz()\n"
+               "{\n"
+               "    Function(Val, (Action)(() =>\n"
+               "                           {\n"
+               "                               using (var a = new Lock())\n"
+               "                               {\n"
+               "                                   if (true)\n"
+               "                                   {\n"
+               "                                       A.Remove(item);\n"
+               "                                   }\n"
+               "                               }\n"
+               "                           }));\n"
+               "}",
+               MicrosoftStyle);
+
+  verifyFormat("void baz()\n"
+               "{\n"
+               "    Function(Val, (Action)(() =>\n"
+               "                           {\n"
+               "                               if (true)\n"
+               "                               {\n"
+               "                                   A.Remove(item);\n"
+               "                               }\n"
+               "                           }));\n"
+               "}",
+               MicrosoftStyle);
+
+  verifyFormat("void baz()\n"
+               "{\n"
+               "    Function(Val, (Action)(() =>\n"
+               "                           {\n"
+               "                               do\n"
+               "                               {\n"
+               "                                   A.Remove(item);\n"
+               "                               } while (true)\n"
+               "                           }));\n"
+               "}",
+               MicrosoftStyle);
+
+  verifyFormat("void baz()\n"
+               "{\n"
+               "    Function(Val, (Action)(() =>\n"
+               "                           { A.Remove(item); }));\n"
+               "}",
+               MicrosoftStyle);
+
+  verifyFormat("void bar()\n"
+               "{\n"
+               "    Function(Val, (() =>\n"
+               "                   {\n"
+               "                       lock (mylock)\n"
+               "                       {\n"
+               "                           if (true)\n"
+               "                           {\n"
+               "                               A.Remove(item);\n"
+               "                           }\n"
+               "                       }\n"
+               "                   }));\n"
+               "}",
+               MicrosoftStyle);
+  verifyFormat("void bar()\n"
+               "{\n"
+               "    Function((() =>\n"
+               "              {\n"
+               "                  lock (mylock)\n"
+               "                  {\n"
+               "                      if (true)\n"
+               "                      {\n"
+               "                          A.Remove(item);\n"
+               "                      }\n"
+               "                  }\n"
+               "              }));\n"
+               "}",
+               MicrosoftStyle);
+
+  MicrosoftStyle.IndentWidth = 2;
+  verifyFormat("void bar()\n"
+               "{\n"
+               "  Function((() =>\n"
+               "            {\n"
+               "              lock (mylock)\n"
+               "              {\n"
+               "                if (true)\n"
+               "                {\n"
+               "                  A.Remove(item);\n"
+               "                }\n"
+               "              }\n"
+               "            }));\n"
+               "}",
+               MicrosoftStyle);
+  verifyFormat("void bar() {\n"
+               "  Function((() => {\n"
+               "    lock (mylock) {\n"
+               "      if (true) {\n"
+               "        A.Remove(item);\n"
+               "      }\n"
+               "    }\n"
+               "  }));\n"
+               "}",
+               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.