[clang-format] Don't apply severe penalty if no possible column formats

[jamesg-nz]

If there are possible column formats, but they weren't selected because they don't fit within remaining characters for the current path then applying severe penalty to induce column layout by selection of a different path seems fair.

But if due to style configuration or what the input code is, there are no possible column formats, different paths aren't going to have column layouts. Seems wrong to apply the severe penalty to induce column layouts if there are none available.

It just causes selection of sub-optimal paths, e.g. get bad formatting when brace initializers are used inside lambda bodies.

Fixes #56350

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang-format

Author: James Grant (jamesg-nz)

Changes

If there are possible column formats, but they weren't selected because they don't fit within remaining characters for the current path then applying severe penalty to induce column layout by selection of a different path seems fair.

But if due to style configuration or what the input code is, there are no possible column formats, different paths aren't going to have column layouts. Seems wrong to apply the severe penalty to induce column layouts if there are none available.

It just causes selection of sub-optimal paths, e.g. get bad formatting when brace initializers are used inside lambda bodies.

Fixes #56350

---

Full diff: https://github.com/llvm/llvm-project/pull/76675.diff

2 Files Affected:

• (modified) clang/lib/Format/FormatToken.cpp (+2-2)
• (modified) clang/unittests/Format/FormatTest.cpp (+15)

diff --git a/clang/lib/Format/FormatToken.cpp b/clang/lib/Format/FormatToken.cpp
index 7a2df8c53952f9..b791c5a26bbe3a 100644
--- a/clang/lib/Format/FormatToken.cpp
+++ b/clang/lib/Format/FormatToken.cpp
@@ -113,8 +113,8 @@ unsigned CommaSeparatedList::formatAfterToken(LineState &State,
   if (!State.NextToken || !State.NextToken->Previous)
     return 0;
 
-  if (Formats.size() == 1)
-    return 0; // Handled by formatFromToken
+  if (Formats.size() <= 1)
+    return 0; // Handled by formatFromToken (1) or avoid severe penalty (0).
 
   // Ensure that we start on the opening brace.
   const FormatToken *LBrace =
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 881993ede17c3d..b7350b2fe66d9b 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -13875,6 +13875,21 @@ TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
                getLLVMStyleWithColumns(35));
   verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaa, {},\n"
                "                               aaaaaaaaaaaaaaaaaaaaaaa);");
+
+  // No possible column formats, don't want the optimal paths penalized.
+  verifyFormat(
+      "waarudo::unit desk = {\n"
+      "    .s = \"desk\", .p = p, .b = [] { return w::r{3, 10} * w::m; }};");
+  verifyFormat("SomeType something1([](const Input &i) -> Output { return "
+               "Output{1, 2}; },\n"
+               "                    [](const Input &i) -> Output { return "
+               "Output{1, 2}; });");
+  FormatStyle NoBinPacking = getLLVMStyle();
+  NoBinPacking.BinPackParameters = false;
+  verifyFormat("waarudo::unit desk = {\n"
+               "    .s = \"desk\", .p = p, .b = [] { return w::r{3, 10, 1, 1, "
+               "1, 1} * w::m; }};",
+               NoBinPacking);
 }
 
 TEST_F(FormatTest, PullTrivialFunctionDefinitionsIntoSingleLine) {

[jamesg-nz]

Running debug build clang-format with -debug parameter against code provided in #56350 (and its duplicate #58469) you see Penalty for line: 10000. So pretty obvious the severe penalty is responsible.

Thoughts on this change? Is the removal of the severe penalty being applied too broadly? ... or is it correct/OK?

[HazardyKnusperkeks]

I think that's not the right way to fix the issue.

Why are the 2 lines formatted differently? It seems to me that this fixes the symptom, not the cause.

[jamesg-nz]

I think that's not the right way to fix the issue.

Why are the 2 lines formatted differently? It seems to me that this fixes the symptom, not the cause.

Because for the line where brackets are used it meets the condition in the next if statement down:

llvm-project/clang/lib/Format/FormatToken.cpp

Lines 122 to 125 in 7e405eb

	if (!LBrace || !LBrace->isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
	LBrace->is(BK_Block) || LBrace->is(TT_DictLiteral) ||
	LBrace->Next->is(TT_DesignatedInitializerPeriod)) {
	return 0;

... and a penalty of zero is returned. Similar exclusion exists for precomputing the column formats:

llvm-project/clang/lib/Format/FormatToken.cpp

Lines 189 to 192 in 7e405eb

	// FIXME: At some point we might want to do this for other lists, too.
	if (!Token->MatchingParen ||
	!Token->isOneOf(tok::l_brace, TT_ArrayInitializerLSquare)) {
	return;

But this ultimately ends up simply as a performance optimization as they're never considered later in any case.

Whereas if the braces are used instead of brackets it does not meet the not a left brace condition, and gets to the severe penalty return:

llvm-project/clang/lib/Format/FormatToken.cpp

Lines 136 to 140 in 7e405eb

	// If no ColumnFormat can be used, the braced list would generally be
	// bin-packed. Add a severe penalty to this so that column layouts are
	// preferred if possible.
	if (!Format)
	return 10000;

This is because suitable column formats are not found, as there was less than 5 commas found in the list (there's other conditions dependent on style configuration further up) when precomputing the column formats, and so none are generated:

llvm-project/clang/lib/Format/FormatToken.cpp

Lines 271 to 274 in 7e405eb

	// Don't use column layout for lists with few elements and in presence of
	// separating comments.
	if (Commas.size() < 5 || HasSeparatingComment)
	return;

Seems a little odd to return severe penalty if no column formats are available as opposed to having them, but none fit right now.

But I'm thinking maybe better to somehow add a condition that if the left brace appears within a short lambda body that'll end up on a single line then a penalty of zero is also returned. More/better targeted change.

Ultimately I believe comes down to finding a good justification to return zero penalty when the braces are used inside lambda bodies. Maybe affects other short functions too like inlines - need to check.

[HazardyKnusperkeks]

I think that's not the right way to fix the issue.
Why are the 2 lines formatted differently? It seems to me that this fixes the symptom, not the cause.

Because for the line where brackets are used it meets the condition in the next if statement down:

llvm-project/clang/lib/Format/FormatToken.cpp

Lines 122 to 125 in 7e405eb

	if (!LBrace || !LBrace->isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
	LBrace->is(BK_Block) || LBrace->is(TT_DictLiteral) ||
	LBrace->Next->is(TT_DesignatedInitializerPeriod)) {
	return 0;

... and a penalty of zero is returned. Similar exclusion exists for precomputing the column formats:

llvm-project/clang/lib/Format/FormatToken.cpp

Lines 189 to 192 in 7e405eb

	// FIXME: At some point we might want to do this for other lists, too.
	if (!Token->MatchingParen ||
	!Token->isOneOf(tok::l_brace, TT_ArrayInitializerLSquare)) {
	return;

But this ultimately ends up simply as a performance optimization as they're never considered later in any case.

Whereas if the braces are used instead of brackets it does not meet the not a left brace condition, and gets to the severe penalty return:

llvm-project/clang/lib/Format/FormatToken.cpp

Lines 136 to 140 in 7e405eb

	// If no ColumnFormat can be used, the braced list would generally be
	// bin-packed. Add a severe penalty to this so that column layouts are
	// preferred if possible.
	if (!Format)
	return 10000;

This is because suitable column formats are not found, as there was less than 5 commas found in the list (there's other conditions dependent on style configuration further up) when precomputing the column formats, and so none are generated:

llvm-project/clang/lib/Format/FormatToken.cpp

Lines 271 to 274 in 7e405eb

	// Don't use column layout for lists with few elements and in presence of
	// separating comments.
	if (Commas.size() < 5 || HasSeparatingComment)
	return;

Seems a little odd to return severe penalty if no column formats are available as opposed to having them, but none fit right now.

But I'm thinking maybe better to somehow add a condition that if the left brace appears within a short lambda body that'll end up on a single line then a penalty of zero is also returned. More/better targeted change.

Ultimately I believe comes down to finding a good justification to return zero penalty when the braces are used inside lambda bodies. Maybe affects other short functions too like inlines - need to check.

Only now I see the difference between the two lines...

[HazardyKnusperkeks]

As far as I'm concerned it looks good. But I need a second opinion. @mydeveloperday @owenca @rymiel