Page MenuHomePhabricator

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

Authored by MyDeveloperDay on Jun 16 2021, 8:12 AM.

Details

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()
{
...
}

Diff Detail

Event Timeline

MyDeveloperDay requested review of this revision.Jun 16 2021, 8:12 AM
MyDeveloperDay created this revision.

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

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 accepted this revision.Jun 23 2021, 11:53 AM

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.
This revision is now accepted and ready to land.Jun 23 2021, 11:53 AM
exv requested changes to this revision.Jun 23 2021, 11:54 AM

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

This revision now requires changes to proceed.Jun 23 2021, 11:54 AM

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 requested review of this revision.Jun 23 2021, 12:59 PM

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.

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 accepted this revision.Jun 29 2021, 1:12 PM
jbcoe added a subscriber: krasimir.

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.

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

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.

exv accepted this revision.Jun 30 2021, 10:47 AM
This revision is now accepted and ready to land.Jun 30 2021, 10:47 AM
MyDeveloperDay marked 3 inline comments as done.

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