[clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[jp4a50]

Summary

The previous implementation of the option involved a hack which corrupted the parenthesis state stack. The review for the previous implementation is here: https://reviews.llvm.org/D102706

Specifically, this change fixes the following github issues:

• LambdaBodyIndentation for lambda as argument argument
• clang-format 13.0.0: LambdaBodyIndentation: OuterScope closing brace & parenthesis too indented
• Inconsistent indentation closing bracket of the lambda body
• clang-format LambdaBodyIndentation=OuterScope doesn't work correctly with BraceWrapping.BeforeLambdaBody=1

This is my first contribution to clang/clang-format, so I'd appreciate some guidance.

One thing that I'd specifically like some guidance on is which version to ship these changes in and how. Although I would consider all the changes here to be bugfixes, they do considerably change the behaviour of the OuterScope option in question, so I'm not sure whether these changes are suitable to go into a patch release or if they should only go into the next major release.

[jp4a50]

clang/lib/Format/ContinuationIndenter.cpp

1132

Note that we do not apply "OuterScope" behaviour when the line's indentation level is 0 to avoid placing a lambda's closing brace at 0 indentation like this:

class Foo :
    callback{[] {
}, 
    bar,
    baz} {}

In other words, we generally try to apply this rule at function/block scope but not otherwise.

[jp4a50]

Add new code sample to demonstrate behaviour of "LambdaBodyIndentation: OuterScope"

[MyDeveloperDay]

clang/docs/ClangFormatStyleOptions.rst

3540

This code may come from Format.h

clang/unittests/Format/FormatTest.cpp

21916–21917

Why are you changing existing tests

[owenpan]

The previous implementation of the option involved a hack which corrupted the parenthesis state stack.

Can you link the review (e.g. Dnnnnnn) of the previous implementation in the summary?

Specifically, this change fixes github issues #55708, #53212, #52846 and #59954.

Please link the issues (e.g. https://github.com/llvm/llvm-project/issues/nnnnn).

[jp4a50]

In D146042#4195295, @owenpan wrote:

The previous implementation of the option involved a hack which corrupted the parenthesis state stack.

Can you link the review (e.g. Dnnnnnn) of the previous implementation in the summary?

Specifically, this change fixes github issues #55708, #53212, #52846 and #59954.

Please link the issues (e.g. https://github.com/llvm/llvm-project/issues/nnnnn).

Done! Please see updated description.

To give a little more relevant context, my team were the original authors of this feature/option (primarily for our own benefit) so we are motivated to fix it and get it right this time.

[jp4a50]

clang/docs/ClangFormatStyleOptions.rst

3540

Sorry, I'm not sure I understand.

Do you mean that this code should *also* be added to the corresponding documentation in Format.h? Or do you mean that the documentation in this file is programatically generated from Format.h and I should therefore only put it there? Or something else?

clang/unittests/Format/FormatTest.cpp

21916–21917

Sorry about the rather large diff here.

I have made the following changes to existing tests (all for what I think are good reasons):

• Reduced the column limit and then reduced the verbosity of the code samples in the test to make them more readable. Before the column limit was 100 and the code samples were very long. I found this made them unnecessarily difficult to read, especially because some of the lines of the code sample would wrap before a newline in the code itself, which was confusing.
• Changed a number of tests written in the form EXPECT_EQ(code, format(otherCode)) to simply verifyFormat(code) because the latter is much shorter and easier to read. If you can think of a good reason to favour the former over the latter then let me know but it just seemed like unnecessary duplication to me.
• Some of the tests weren't syntactically valid C++ e.g. the tests at line 21939 which I have changed to have a valid (rather than incomplete) trailing return type.
• The macro test actually captured a bug so I had to change the code sample there to reflect the now fixed behaviour.

I also added one or two more tests at the bottom to capture more complex cases like when more than one argument to a function call is a lambda.

[jp4a50]

clang/docs/ClangFormatStyleOptions.rst

3540

Ahh I've found dump_format_style.py now. Will fix.

[jp4a50]

Update OuterScope docs in Format.h and regenerate public docs.

[jp4a50]

All comments addressed so bump on review please :)

Also, a build has failed but I don't think my changes caused it - it looks like an old version of clang-format is being invoked which doesn't support a recently added option?

[owenpan]

Looks like this patch doesn't put the opening brace at the outerscope for some of the examples from the linked github issues:

$ cat .clang-format
AllowShortLambdasOnASingleLine: None
BraceWrapping:
  BeforeLambdaBody: true
BreakBeforeBraces: Custom
LambdaBodyIndentation: OuterScope
$ cat test.cpp
aaaaaaaaaaaaaaaaaaaa(1,
	             b(
	             	[]
                     {
                       return 0;
                     }));

some_function(a_really_long_name, another_long_name, a_third_long_name,
              [&](LineCallback line_callback)
{
  int a;
  int b;
});
$ clang-format test.cpp
aaaaaaaaaaaaaaaaaaaa(1, b(
                            []
                            {
                              return 0;
                            }));

some_function(a_really_long_name, another_long_name, a_third_long_name,
              [&](LineCallback line_callback)
              {
                int a;
                int b;
              });
$ 

Shouldn't the expected output be the same as the input?

[jp4a50]

In D146042#4204513, @owenpan wrote:

Looks like this patch doesn't put the opening brace at the outerscope for some of the examples from the linked github issues:

$ cat .clang-format
AllowShortLambdasOnASingleLine: None
BraceWrapping:
  BeforeLambdaBody: true
BreakBeforeBraces: Custom
LambdaBodyIndentation: OuterScope
$ cat test.cpp
aaaaaaaaaaaaaaaaaaaa(1,
	             b(
	             	[]
                     {
                       return 0;
                     }));

some_function(a_really_long_name, another_long_name, a_third_long_name,
              [&](LineCallback line_callback)
{
  int a;
  int b;
});
$ clang-format test.cpp
aaaaaaaaaaaaaaaaaaaa(1, b(
                            []
                            {
                              return 0;
                            }));

some_function(a_really_long_name, another_long_name, a_third_long_name,
              [&](LineCallback line_callback)
              {
                int a;
                int b;
              });
$ 

Shouldn't the expected output be the same as the input?

I'm confident that the patch will indent all those examples correctly when they are at block scope which is the only place those snippets will actually be valid code. I added an exception (see comment in ContinuationIndenter.cpp) to the code to disable OuterScope's behaviour when the line's indentation level is 0 (i.e. statements at namespace scope) because otherwise you can end up with things like:

Namespace::Foo::Foo(
  Arg arg1, Arg arg2,
  Arg arg3, Arg arg4)
  : init1{arg1},
    init2{[arg2]() {
  return arg2;
}},
    init3{arg3},
    init4{arg4} {}

The main rationale behind the OuterScope style is that the lambda body will be indented similarly to the code block of an if statement (or for etc) inside function definitions/blocks, but this breaks down when you have something like an constructor initializer which lives outside of a code block. The way I've handled this case isn't particularly elegant, though, so I'd be happy to remove the relevant code from this diff as it's not necessary to fix the issues I've linked - it was an extra bit of behaviour that my team (authors of the original implementation of OuterScope) wanted.

FYI I will be away until next Monday now but thanks for the feedback so far.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

21916–21917

refactoring the tests is one thing, but not at the same time you are modifying how they are formatted. Could it be a separate commit so we can actually see what is changing?

I don't deny what is there before doesn't look 100% correct, but I've always worked on the Beyoncé rule (If you liked it you should have put a test on it), meaning... someone put a test there to assert some form of formatting behaviour, you are now changing that to be your desired behaviour, that break the rule (IMHO)

we have to agree what was there before wasn't correct, but your changes don't give me that confidence,

21918–21927

This is a completely different function what happen to the std::vector<int> argument?

21952

I feel like lots of these test changes could be made on their own, BEFORE changing the code, I would feel more comfortable about that I think

21953

I would think the point here is to test someLongArgumentName not to use a small variable like 'v'