Page MenuHomePhabricator

[clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace
ClosedPublic

Authored by MyDeveloperDay on Jun 14 2021, 6:11 AM.

Details

Summary

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

I believe D44609: [clang-format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style) may be too aggressive with brace wrapping rules which doesn't always apply to Lamdbas

The introduction of BeforeLambdaBody and AllowShortLambdasOnASingleLine has impact on brace handling on other block types, which I suspect we didn't see before as people may not be using the BeforeLambdaBody style

From what I can tell this can be seen by the unit test I change as its not honouring the orginal LLVM brace wrapping style for the Fct() function

I added a unit test from PR50702 and have removed some of the code (which has zero impact on the unit test, which kind of suggests its unnecessary), some additional attempt has been made to try and ensure we'll only break on what is actually a LamdbaLBrace

Diff Detail

Event Timeline

MyDeveloperDay requested review of this revision.Jun 14 2021, 6:11 AM
MyDeveloperDay created this revision.
Wawha added inline comments.Jun 14 2021, 7:51 AM
clang/lib/Format/TokenAnnotator.cpp
3782

Without this code, you should be able to remove functions isAllmanBraceIncludedBreakableLambda(), isItAInlineLambdaAllowed() and isOneChildWithoutMustBreakBefore().

Thank you for the fix.

When I implement that part, I have some difficulty to handle inline lambda and BreakBeforeLambdaBody. At the beginning, I implement the solution without inline lambda option (not yet present). But I have to handle it before merging the patch. And, this modification was added at this moment.

@Wawha do you have a project that perhaps uses this work your did? I would like to ensure I didn't break anything

Remove unused functions

This revision is now accepted and ready to land.Jun 14 2021, 12:54 PM
Wawha added a comment.EditedJun 15 2021, 3:44 AM

@Wawha do you have a project that perhaps uses this work your did? I would like to ensure I didn't break anything

@MyDeveloperDay Good idea!
I just test on my code, and I see 3 types of regressions due to that patch.
This code:

		auto select = [this]() -> const Library::Object*
		{
			return MyAssignment::SelectFromList(this);
		};

Become:

		auto select = [this]() -> const Library::Object* {
			return MyAssignment::SelectFromList(this);
		};

And this code,

	auto Items()
	{
		return iter::imap(
			[this](const std::unique_ptr<ItemBase>& iItem) -> T&
			{
				return *GetItem(*iItem)->Item;
			},
			m_Root->Items());
	}

Become:

	auto Items()
	{
		return iter::imap(
			[this](const std::unique_ptr<ItemBase>& iItem) -> T& {
				return *GetItem(*iItem)->Item;
			},
			m_Root->Items());
	}

And the last case is due to inline lambda. Even if the option is disable, for that code

		auto AllParents() const
		{
			return iter::chain.from_iterable(iter::imap(
				[](auto&& ite) -> auto&
				{
					return ite.second;
				},
				m_Parents));
		}

It's format like this:

		auto AllParents() const
		{
			return iter::chain.from_iterable(iter::imap(
				[](auto&& ite) -> auto& { return ite.second; }, m_Parents));
		}

I'm surprise, because the 3 cases, should be present inside the UnitTest.

Hi,

Due to the pandemic, I wasn't able to look at https://reviews.llvm.org/D44609 which got broken since it was pushed to LLVM. Nevertheless I commented in that post with a rather complete unit tests that should pass if any changes are to be pushed to that part of the code.

Here is the test suite:

noOtherParams([](int x){call();});
paramBeforeLambda(8,[](int x){call();});
paramAfterLambda([](int x){call();},5);
paramBeforeAndAfterLambda(8,[](int x){call();},5);
doubleLambda(8,[](int x){call();},6,[](float f){return f*2;},5);
nestedLambda1([](int x){return [](float f){return f*2;};});
nestedLambda2([](int x){ call([](float f){return f*2;});});
noExceptCall([](int x) noexcept {call();});
mutableCall([](int x) mutable{call();});

funcCallFunc(call(),[](int x){call();});

auto const l=[](char v){if(v)call();};
void f() { return [](char v){ if(v) return v++;}; }

And here is how it should be formatted:

noOtherParams(
        [](int x)
        {
                call();
        });
paramBeforeLambda(8,
        [](int x)
        {
                call();
        });
paramAfterLambda(
        [](int x)
        {
                call();
        },
        5);
paramBeforeAndAfterLambda(8,
        [](int x)
        {
                call();
        },
        5);
doubleLambda(8,
        [](int x)
        {
                call();
        },
        6,
        [](float f)
        {
                return f * 2;
        },
        5);
nestedLambda1(
        [](int x)
        {
                return [](float f)
                {
                        return f * 2;
                };
        });
nestedLambda2(
        [](int x)
        {
                call(
                        [](float f)
                        {
                                return f * 2;
                        });
        });
noExceptCall(
        [](int x) noexcept
        {
                call();
        });
mutableCall(
        [](int x) mutable
        {
                call();
        });

funcCallFunc(call(),
        [](int x)
        {
                call();
        });

auto const l = [](char v)
{
        if (v)
                call();
};
void f()
{
        return [](char v)
        {
                if (v)
                        return v++;
        };
}

I couldn't find the time to test this case with the latest clang-format release, but I guess it's still broken and doesn't match the expected output.
Anyway, this test suite is helpful as it covers most cases, and if someone with enough knowledge of LLVM is willing to add this to the unit tests it would help a lot.

Best regards,
Chris

Wawha added a comment.Jun 15 2021, 6:58 AM

Hi @christophe-calmejane.
I test your code with a old version (few time after the merge of https://reviews.llvm.org/D44609), and the latest version (commit: e0c382a9d5a0), and in both cases I have the same result, which is near your output:

noOtherParams(
	[](int x)
	{
		call();
	});
paramBeforeLambda(
	8,
	[](int x)
	{
		call();
	});
paramAfterLambda(
	[](int x)
	{
		call();
	},
	5);
paramBeforeAndAfterLambda(
	8,
	[](int x)
	{
		call();
	},
	5);
doubleLambda(
	8,
	[](int x)
	{
		call();
	},
	6,
	[](float f)
	{
		return f * 2;
	},
	5);
nestedLambda1(
	[](int x)
	{
		return [](float f)
		{
			return f * 2;
		};
	});
nestedLambda2(
	[](int x)
	{
		call(
			[](float f)
			{
				return f * 2;
			});
	});
noExceptCall(
	[](int x) noexcept
	{
		call();
	});
mutableCall(
	[](int x) mutable
	{
		call();
	});

funcCallFunc(
	call(),
	[](int x)
	{
		call();
	});

auto const l = [](char v)
{
	if(v)
		call();
};
void f()
{
	return [](char v)
	{
		if(v)
			return v++;
	};
}

The only different is for cases with multiples parameters.

paramBeforeLambda(
	8,
	[](int x)
	{
		call();
	}

I though I add multiples cases, but looking at UnitTests with inline lambda "None", I see only few tests, perhaps some are missing.

Hi @christophe-calmejane.
I test your code with a old version (few time after the merge of https://reviews.llvm.org/D44609), and the latest version (commit: e0c382a9d5a0), and in both cases I have the same result, which is near your output:

I though I add multiples cases, but looking at UnitTests with inline lambda "None", I see only few tests, perhaps some are missing.

I posted a fix for the incorrect formatting you have in your output (in your original review post), but like I said back then, I thought the fix was not clean enough to be included. Nevertheless it did fix all cases and formatting was perfect for my whole test suite.
I using this "patched" version since then as it's the only one with the correct lambda formatting. I tried to reapply my patch on a recent version.. of course it failed as there were too many changes in clang-format. I don't have enough knowledge on this project to be able to clearly understand how it works and fix the formatting on the latest version (but I wish I could)... at least not without having to spend too much time on it :(
For the record, my patch over clang-format 7.0 can be found here: https://www.kikisoft.com/Hive/clang-format/

@Wawha your cases could be covered by the following (in mustBreakBefore)

if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace) &&
    Left.isOneOf(tok::star, tok::amp, tok::ampamp)) {
  return true;
}

As I think its the presence of & and * that causes it to not wrap.

By all means if you would prefer to commandeer the patch I'm ok with that. I was just on a bug fixing day. If you'd like to take this over and add the unit tests @christophe-calmejane suggest, I'm happy to be the reviewer

@christophe-calmejane are you able to post your .clang-format styles that you are using, I'm struggling to see where its not matching your style (other than brace styles on the function and argument placing)

Add some of the failing cases identified by @Wawha

Wawha added a comment.Jun 15 2021, 7:47 AM

@Wawha your cases could be covered by the following (in mustBreakBefore)

if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace) &&
    Left.isOneOf(tok::star, tok::amp, tok::ampamp)) {
  return true;
}

As I think its the presence of & and * that causes it to not wrap.

By all means if you would prefer to commandeer the patch I'm ok with that. I was just on a bug fixing day. If you'd like to take this over and add the unit tests @christophe-calmejane suggest, I'm happy to be the reviewer

Indeed, & and * could be the root cause.
About fixing that, I won't have the time in the next days. It will take time for me to re-read and re-understand that code.
But I could find time to test patch or modification in my code to check that there is no regression.

Wawha added a comment.Jun 15 2021, 9:04 AM

Add some of the failing cases identified by @Wawha

Thank for the fix. I test and discover a new case with a regression:

	auto createObj = [this] -> std::unique_ptr<Object>
		{
		  return std::make_unique<Object>();
		};

Like & and * a template return ending with > is not handle.

Add support for TemplateCloser before LambdaBrace

Just asking: Would it be different if there is auto, decltype(auto), decltype(a+b), typename or template as trailing return type?

Indeed, I found another case with a regression:

return iter::chain.from_iterable(
  [](auto&& ite) -> auto&
  {
    return ite.second;
  });

it's format all inline.

And also another different in that case (without lambda):

	ASSERT_NO_THROW(
		{
			iterator += 507408;
		});

is now format like this:

	ASSERT_NO_THROW({ iterator += 507408; });

Part of the problem here is we are not really parsing the Lambda but trying to break based on token annotation only. This can tend to be a bit fragile (normally around corner cases)

The previous approach I think was not constrained enough to just lambdas which is why we had the bleed into other things, also when we added this we didn't really test that it didn't have any impact on anything else.

Actually this has been a problem when adding new options. I keep meaning to extend the LLVMDefaultStyle GNUDefaultStyle MozillaDefaultStyle tests to cover all behaviour (if/else/while/do/for/etc..) but also to find a way that when a new option is added we can validate that non of the other aspects of a style aren't broken when using that. (not an easy problem to fix)

I'm keen for this NOT to break the AfterClass, AfterNamespace issue, (which is a regression)... we can continue to chip away at corner cases or revert the original change completely. (which I also don't want to do)

likely trying to add add parseXXX() functions in UnwrappedLineParser would likely take much longer. I feel like that might offer the better solution but think I would prefer we used that to replace this and gravitate slowly towards a more correct case

MyDeveloperDay added a comment.EditedJun 16 2021, 12:22 AM

And also another different in that case (without lambda):

	ASSERT_NO_THROW(
		{
			iterator += 507408;
		});

is now format like this:

	ASSERT_NO_THROW({ iterator += 507408; });

I think this shows where your change made an assumption that this was a lambda, but its not necessarily is as there is no TT_LamdbaLBrace, I would argue your change was the regression even if looks nicer (in your view)

AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=identifier L=15 PPK=2 FakeLParens= FakeRParens=0 II=0xad8a50 Text='ASSERT_NO_THROW'
 M=0 C=0 T=Unknown S=0 F=0 B=1 BK=0 P=23 Name=l_paren L=16 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=1 P=140 Name=l_brace L=17 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='{'
 M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=61 Name=r_brace L=39 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='}'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=40 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=semi L=41 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'

For this case:

return iter::chain.from_iterable(
  [](auto&& ite) -> auto&
  {
    return ite.second;
  });

The code has not been classified as a Lambda

AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=return L=6 PPK=2 FakeLParens= FakeRParens=0 II=0x24bf7e8 Text='return'
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=23 Name=identifier L=11 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x24c35c0 Text='iter'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=coloncolon L=13 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='::'
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=520 Name=identifier L=18 PPK=2 FakeLParens= FakeRParens=0 II=0x24c3738 Text='chain'
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=170 Name=period L=19 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='.'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=identifier L=32 PPK=2 FakeLParens= FakeRParens=0 II=0x24c3760 Text='from_iterable'
 M=0 C=0 T=Unknown S=0 F=0 B=1 BK=0 P=23 Name=l_paren L=33 PPK=1 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=ArrayInitializerLSquare S=0 F=0 B=0 BK=0 P=59 Name=l_square L=34 PPK=2 FakeLParens=1/ FakeRParens=0 II=0x0 Text='['
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=79 Name=r_square L=35 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=']'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=l_paren L=36 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=79 Name=auto L=40 PPK=2 FakeLParens= FakeRParens=0 II=0x24bf510 Text='auto'
 M=0 C=1 T=PointerOrReference S=1 F=0 B=0 BK=0 P=250 Name=ampamp L=43 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='&&'
 M=0 C=1 T=StartOfName S=0 F=0 B=0 BK=0 P=260 Name=identifier L=46 PPK=2 FakeLParens= FakeRParens=0 II=0x24c3780 Text='ite'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=63 Name=r_paren L=47 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=1 T=LambdaArrow S=1 F=0 B=0 BK=0 P=150 Name=arrow L=50 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='->'
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=43 Name=auto L=55 PPK=2 FakeLParens= FakeRParens=0 II=0x24bf510 Text='auto'
 M=0 C=1 T=PointerOrReference S=1 F=0 B=0 BK=0 P=230 Name=amp L=57 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='&'
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=1 P=48 Name=l_brace L=59 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='{'
 M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=61 Name=r_brace L=80 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text='}'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=81 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=semi L=82 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'

It doesn't see this

M=0 C=0 T=Unknown S=1 F=0 B=0 BK=1 P=48 Name=l_brace L=59 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='{'

As a LamdaLBrace

Again I would say this was actually a failure elsewhere

Just asking: Would it be different if there is auto, decltype(auto), decltype(a+b), typename or template as trailing return type?

It would be good if you could give a small example, just so I can clarify what you mean,

but I think these could easily be corner cases which at least for a current non parsed based solution would could potentially fill with mustBreak rules..

@christophe-calmejane are you able to post your .clang-format styles that you are using, I'm struggling to see where its not matching your style (other than brace styles on the function and argument placing)

@MyDeveloperDay
Of course, sorry for the delay, here it is: https://github.com/christophe-calmejane/Hive/blob/master/.clang-format

@MyDeveloperDay
Of course, sorry for the delay, here it is: https://github.com/christophe-calmejane/Hive/blob/master/.clang-format

You don't have AllowShortLambdasOnASingleLine set to be false or None

If you did The only difference (with this current patch) would be the positioning of the first arguments on paramBeforeAndAfterLambda and doubleLambda (I'm not sure the reason for why that is)

@MyDeveloperDay
Of course, sorry for the delay, here it is: https://github.com/christophe-calmejane/Hive/blob/master/.clang-format

You don't have AllowShortLambdasOnASingleLine set to be false or None

Just a clarification, this file is meant to be used with clang-format v7.0.0 along with my small patch fixing formatting issues that were not included in the original merge (see the other post). This means it's using the defaults from back then ;)
I never had a chance to upgrade clang-format because lambda formatting was broken (some of my test cases were failing). I will try to build your latest patch and see how it goes. I would expect my project re-formatting to not change much :D
Thanks for your work btw.

If you did The only difference (with this current patch) would be the positioning of the first arguments on paramBeforeAndAfterLambda and doubleLambda (I'm not sure the reason for why that is)

@MyDeveloperDay The reason is the fix I talked about in the other thread (https://reviews.llvm.org/D44609) that I proposed for LLVM 7.0.0. But last time I checked, my patch couldn't be applied anymore due to massive changes in the code. I don't have enough knowledge of clang-format to clearly follow where went the code my patch applies to, I originally fixed the issue blindly by stepping into the code :)
Nevertheless, I think the issue is still present (since my patch was not applied to the base code) and it might still be related to this reason. See my comment that fixed the issue back then (https://reviews.llvm.org/D44609#1255395).

It would be really awesome if this could finally be fixed so we have a consistent formatting for lambdas!