This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jp4a50 on Mar 14 2023, 6:26 AM.

Details

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:

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.

Diff Detail

Event Timeline

jp4a50 created this revision.Mar 14 2023, 6:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 6:26 AM
Herald added a subscriber: mgrang. · View Herald Transcript
jp4a50 requested review of this revision.Mar 14 2023, 6:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 6:26 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jp4a50 edited the summary of this revision. (Show Details)Mar 14 2023, 6:34 AM
jp4a50 added a reviewer: HazardyKnusperkeks.
jp4a50 added a project: Restricted Project.
jp4a50 added inline comments.Mar 14 2023, 6:37 AM
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 updated this revision to Diff 505264.Mar 14 2023, 2:28 PM

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

MyDeveloperDay requested changes to this revision.Mar 14 2023, 4:24 PM
MyDeveloperDay added a subscriber: MyDeveloperDay.
MyDeveloperDay added inline comments.
clang/docs/ClangFormatStyleOptions.rst
3568

This code may come from Format.h

clang/unittests/Format/FormatTest.cpp
21958–21959

Why are you changing existing tests

This revision now requires changes to proceed.Mar 14 2023, 4:24 PM

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 edited the summary of this revision. (Show Details)Mar 15 2023, 4:36 AM

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 added inline comments.Mar 15 2023, 4:52 AM
clang/docs/ClangFormatStyleOptions.rst
3568

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
21958–21959

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 edited the summary of this revision. (Show Details)Mar 15 2023, 4:52 AM
jp4a50 added inline comments.Mar 15 2023, 5:21 AM
clang/docs/ClangFormatStyleOptions.rst
3568

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

jp4a50 updated this revision to Diff 505449.Mar 15 2023, 5:27 AM

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

jp4a50 marked an inline comment as done.Mar 15 2023, 5:28 AM

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?

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?

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 added inline comments.Mar 21 2023, 2:44 AM
clang/unittests/Format/FormatTest.cpp
21958–21959

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,

21960–21969

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

22003–22005

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

22004–22005

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

MyDeveloperDay requested changes to this revision.Mar 21 2023, 2:45 AM
This revision now requires changes to proceed.Mar 21 2023, 2:45 AM
jp4a50 added inline comments.Mar 22 2023, 10:33 AM
clang/unittests/Format/FormatTest.cpp
21958–21959

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?

Sure! I assume that this implies two separate Phabricator reviews/diffs? Or is there somehow a way to show two "commits" within a single review? Sorry, I'm not used to using Phrabricator.

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)

The macro example I was referring to as a "bug" actually had a comment below it which seemed to acknowledge an issue with the previous implementation. Now I'm not 100% sure that that comment was referring to the behaviour in the macro example but what does seem clear is that the behaviour shown in the macro test case is not desirable in the case of using OuterScope. I just can't see how that behaviour could be justified as being intentional/desirable. If you wanted, I could reach out to the original author of that test and ask them since they still work at the same company.

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

I'd appreciate if you could elaborate on this - perhaps you're referring mainly to the macro example but I'm not sure.

21960–21969

The changes to shorten various types and variable names was to avoid lines (in the code sample) being broken onto multiple newlines where they weren't before because I changed the column limit from 100 to 60 (so leaving the code sample changed would have led to additional line breaks).

I changed the column limit from 100 to 60 purely because I thought the 100 column limit made the tests unnecessarily verbose - there were lots of characters to read for seemingly no benefit.

clang/unittests/Format/FormatTest.cpp
21958–21959

Each commit gets its own differential, there are no multiple commit reviews like pull requests. You can add a parent or child commit to show the dependency, if any.

jp4a50 updated this revision to Diff 508762.Mar 27 2023, 12:48 PM

Rebase diff on top of D146995.

jp4a50 added inline comments.Mar 27 2023, 12:50 PM
clang/unittests/Format/FormatTest.cpp
21958–21959

Pulled out test refactoring (excluding functional changes) into D146995.

22003–22005

Pulled them out into D146995.

jp4a50 updated this revision to Diff 508765.Mar 27 2023, 12:56 PM

Re-enable OuterScope lambda body indentation of lambdas in namespace scope statements.

I've removed the special-casing that I added for namespace scope statements. If I make those changes at all, I'll raise them as a follow up diff.

clang/unittests/Format/FormatTest.cpp
22004–22005

These changes have been moved to D146995 now, but I'll respond here: the long variable names were (presumably) being used to cause a line break. Since I've reduced the column limit from 100 to 60, v is now sufficient (and less noisy to read).

Could I get a re-review on this one please?

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} {}

Sorry that I missed it.

clang/include/clang/Format/Format.h
2685–2686

We might as well clean it up a bit.

clang/unittests/Format/FormatTest.cpp
22039

Ditto below.

jp4a50 updated this revision to Diff 510901.Apr 4 2023, 12:49 PM

Tidy up docs and tests.

jp4a50 marked 2 inline comments as done.Apr 4 2023, 12:53 PM

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} {}

Sorry that I missed it.

NP - as I say, I've removed this special casing for now since it is controversial to include it in a patch claiming to simply fix bugs.

jp4a50 updated this revision to Diff 511098.Apr 5 2023, 7:34 AM

Add release note.

This revision is now accepted and ready to land.Apr 5 2023, 12:08 PM
jp4a50 added a comment.EditedApr 5 2023, 12:35 PM

Thanks for accepting!

Commit details as follows as per previous diff:

Name: Jon Phillips
Email: jonap2811@gmail.com

If someone could commit for me that would be much appreciated.

Edit: also note that there are 4 issues that I believe we can close when this is committed, as noted in the description.

owenpan accepted this revision.Apr 5 2023, 2:27 PM
This revision was landed with ongoing or failed builds.Apr 5 2023, 2:39 PM
This revision was automatically updated to reflect the committed changes.