[clang-format] Propagate LeadingEmptyLinesAffected when joining lines

[tJener]

Before this commit, when LineJoiner joins a line with affected leading whitespace, it would drop the knowledge of this entirely. However, when the AffectedRangeManager is computing the affected lines, the leading empty whitespace lines are potentially considered for non-first tokens in the AnnotatedLine. This causes a discrepancy in behavior when an AnnotatedLine is put together from joining multiple lines versus when it is not.

We change LineJoiner::join to follow AffectedRangeManager's logic, considering the leading whitespace when determining Affected for a token if the previous token did not have children.

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

Lines 111 to 130 in a63f572

	// Stores whether we need to look at the leading newlines of the next token
	// in order to determine whether it was affected.
	bool IncludeLeadingNewlines = false;
	// Stores whether the first child line of any of this line's tokens is
	// affected.
	bool SomeFirstChildAffected = false;
	assert(Line->First);
	for (FormatToken *Tok = Line->First; Tok; Tok = Tok->Next) {
	// Determine whether 'Tok' was affected.
	if (affectsTokenRange(*Tok, *Tok, IncludeLeadingNewlines))
	SomeTokenAffected = true;
	// Determine whether the first child of 'Tok' was affected.
	if (!Tok->Children.empty() && Tok->Children.front()->Affected)
	SomeFirstChildAffected = true;
	IncludeLeadingNewlines = Tok->Children.empty();
	}

Fixes #138942.

[llvmbot]

@llvm/pr-subscribers-clang-format

Author: Eric Li (tJener)

Changes

Before this commit, when LineJoiner joins a line with affected leading whitespace, it would drop the knowledge of this entirely. However, when the AffectedRangeManager is computing the affected lines, the leading empty whitespace lines are potentially considered for non-first tokens in the AnnotatedLine. This causes a discrepancy in behavior when an AnnotatedLine is put together from joining multiple lines versus when it is not.

We change LineJoiner::join to follow AffectedRangeManager's logic, considering the leading whitespace when determining Affected for a token if the previous token did not have children.

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

Lines 111 to 130 in a63f572

	// Stores whether we need to look at the leading newlines of the next token
	// in order to determine whether it was affected.
	bool IncludeLeadingNewlines = false;
	// Stores whether the first child line of any of this line's tokens is
	// affected.
	bool SomeFirstChildAffected = false;
	assert(Line->First);
	for (FormatToken *Tok = Line->First; Tok; Tok = Tok->Next) {
	// Determine whether 'Tok' was affected.
	if (affectsTokenRange(*Tok, *Tok, IncludeLeadingNewlines))
	SomeTokenAffected = true;
	// Determine whether the first child of 'Tok' was affected.
	if (!Tok->Children.empty() && Tok->Children.front()->Affected)
	SomeFirstChildAffected = true;
	IncludeLeadingNewlines = Tok->Children.empty();
	}

Fixes #138942.

---

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

2 Files Affected:

• (modified) clang/lib/Format/UnwrappedLineFormatter.cpp (+2)
• (modified) clang/unittests/Format/FormatTestSelective.cpp (+2)

diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index f2ed027b2c047..d22bfc61ed736 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -988,6 +988,8 @@ class LineJoiner {
     assert(!B.First->Previous);
     if (B.Affected)
       A.Affected = true;
+    else if (B.LeadingEmptyLinesAffected && A.Last->Children.empty())
+      A.Affected = true;
     A.Last->Next = B.First;
     B.First->Previous = A.Last;
     B.First->CanBreakBefore = true;
diff --git a/clang/unittests/Format/FormatTestSelective.cpp b/clang/unittests/Format/FormatTestSelective.cpp
index 624684c7a079b..3500b7d47e99d 100644
--- a/clang/unittests/Format/FormatTestSelective.cpp
+++ b/clang/unittests/Format/FormatTestSelective.cpp
@@ -41,6 +41,8 @@ TEST_F(FormatTestSelective, RemovesTrailingWhitespaceOfFormattedLine) {
   EXPECT_EQ("int a;", format("int a;         ", 0, 0));
   EXPECT_EQ("int a;\n", format("int a;  \n   \n   \n ", 0, 0));
   EXPECT_EQ("int a;\nint b;    ", format("int a;  \nint b;    ", 0, 0));
+
+  EXPECT_EQ("void f() {}\n", format("void f() {\n  \n}\n", 11, 0));
 }
 
 TEST_F(FormatTestSelective, FormatsCorrectRegionForLeadingWhitespace) {

[HazardyKnusperkeks]

Please wait a few days, or until another approval.

[owenca]

Can you add a test case for A.Last->Children.empty() being false?

[tJener]

I'm not familiar with when the current implementation would merge two lines where !A.Last->Children.empty(). Do you have any suggestions?