[clang-format] Add test case for issue 63170

[paulkirth]

Summary

After https://reviews.llvm.org/D151954 we've noticed some issues w/
clang-format behavior, as outlined in
https://github.com/llvm/llvm-project/issues/63170.

Valid C/C++ files, that were previously accepted, are now rejected by
clang-format, emitting the message:

"The new replacement overlaps with an existing replacement."

This reverts commit 4b9764959dc4b8783e18747c1742ab164e4bc4ee and
d2627cf88d2553a4c2e850430bdb908a4b7d2e52, which depends on it.

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please try to ensure all code changes have a unit test (unless this is an NFC or refactoring, adding documentation etc..)

Add your unit tests in clang/unittests/Format and you can build with ninja FormatTests. We recommend using the verifyFormat(xxx) format of unit tests rather than EXPECT_EQ as this will ensure you change is tolerant to random whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can happen if you are trying to improve the annotation phase to ensure we are correctly identifying the type of a token, please add a token annotator test in TokenAnnotatorTest.cpp

[owenpan]

Can you add the test to clang/unittests/Format/FormatTest.cpp instead? Please also include the full diff. See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface.

[paulkirth]

In D152473#4407832, @owenpan wrote:

Can you add the test to clang/unittests/Format/FormatTest.cpp instead? Please also include the full diff. See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface.

I can try to add this to unit tests, but I used arcanist so this is the full context, since I only made a lit test.

[MyDeveloperDay]

I presume the test fail, we don't check in tests that just fail without a fix, or the build would be stale.

[paulkirth]

The point of this patch was to show a regression. I'm not trying to fix anything per se. I'd like https://reviews.llvm.org/D151954 and anything that depends on it reverted, since it breaks clang-format. My understanding was that @owenpan was reluctant to revert since they couldn't reproduce the issue, but now that they can, I don't see why this can't go in (with a modified summary) after the revert (or as part of it).

I can of course revert those patches here, but it didn't make sense to me to do that, since it wouldn't show the regression.

[paulkirth]

Revert patches introducing regression & add unit test

[paulkirth]

Rebase and update summary

[phosek]

LGTM

[MyDeveloperDay]

Did this need an immediate revert? was the unit tests failing?

clang/test/Format/overlapping-lines.cpp

2

please remove this.

[MyDeveloperDay]

In D152473#4409146, @paulkirth wrote:

The point of this patch was to show a regression. I'm not trying to fix anything per se. I'd like https://reviews.llvm.org/D151954 and anything that depends on it reverted, since it breaks clang-format.

I think its respectful to wait a little for the code owners to give you a LG unless the patch is breaking the unit tests.

[MyDeveloperDay]

In D152473#4409652, @phosek wrote:

LGTM

@phosek can I ask you review https://llvm.org/docs/CodeReview.html

When providing an unqualified LGTM (approval to commit), it is the responsibility of the reviewer to have reviewed all of the discussion and feedback from all reviewers ensuring that all feedback has been addressed and that all other reviewers will almost surely be satisfied with the patch being approved. If unsure, the reviewer should provide a qualified approval, (e.g., “LGTM, but please wait for @someone, @someone_else”). You may also do this if you are fairly certain that a particular community member will wish to review, even if that person hasn’t done so yet.

Whilst I understand you migth "like" the revert, I would ask that the when you do this, you say `LGTM but please wait for the code owners".

[paulkirth]

In D152473#4409975, @MyDeveloperDay wrote:

In D152473#4409146, @paulkirth wrote:

The point of this patch was to show a regression. I'm not trying to fix anything per se. I'd like https://reviews.llvm.org/D151954 and anything that depends on it reverted, since it breaks clang-format.

I think its respectful to wait a little for the code owners to give you a LG unless the patch is breaking the unit tests.

So first, no one here is trying to be disrespectful, we're all here to make the compiler and surrounding tools better for everyone. That said, it's also normally respectful to revert changes when others report that your patch has a serious regression. https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy

I'm not sure how common this is, but in this case we found a test case that was missing from the unit tests, otherwise the patches that were reverted would never have landed. So, while I understand your point, reverting here is in line w/ the communities policies. If there is a different interpretation I should have on the policy, I'm happy to hear it, but as I read it, our response here is in line w/ our typical developer expectations. My apologies if any of this comes off as rude (being nuanced in text is hard) but I'm trying to be very clear about my interpretation and rationale.

As a last point we've had our CI broken by this since the initial patch landed, and it's been several days since we first reported the issue. We're certainly not trying to be discourteous w/ our actions, but it I also don't think it's reasonable to expect that when community members report a serious regression, file an issue, and provide a reproducer that they be expected to wait days for a revert or forward fix.

[phosek]

I understand the concerns and I apologize if my LGTM came out as disrespectful, but there has been an issue reported with the original change over two days ago, including a reproducer, and given that this issue is affecting clang-format users, I think it's entirely reasonable to go ahead and revert the change in accordance with the Patch reversion policy, and then take as much time as needed to address issue rather than trying to rush a fix forward. In fact, I'd argue that both 4b9764959dc4b8783e18747c1742ab164e4bc4ee and d2627cf88d2553a4c2e850430bdb908a4b7d2e52 should have been reverted right away immediately after the issue was reported without any need for code review as is customary in LLVM.

[MyDeveloperDay]

My additional concern is, is the original patch is the root cause of the regression?, so I’m struggling to understand why this in particular is being reverted or are we just going backwards through all commits?

[paulkirth]

In D152473#4410331, @MyDeveloperDay wrote:

My additional concern is, is the original patch is the root cause of the regression?, so I’m struggling to understand why this in particular is being reverted or are we just going backwards through all commits?

I'm not sure I follow, can you elaborate on what you mean here? Reverting only 4b9764959dc4b8783e18747c1742ab164e4bc4ee will allow the new test added here to pass, but tests added in d2627cf88d2553a4c2e850430bdb908a4b7d2e52 will fail, since they require the behavior added in 4b9764959dc4b8783e18747c1742ab164e4bc4ee. Reverting them both is required to keep tests passing.

clang/test/Format/overlapping-lines.cpp

2

removed in https://reviews.llvm.org/rGa28a46665529992cefb78049da79f2125f6b6c2d

[MyDeveloperDay]

Part of my concern was that the reversion removed a unit test, which effectively regressed a different fix, I'm not comfortable with that.

I think we can resolve the original problem with the following fix.

diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 33be74dfe1b9..4876d8cdcf0c 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -1489,6 +1489,7 @@ void UnwrappedLineFormatter::formatFirstToken(

   // Insert or remove empty line after access specifiers.
   if (PreviousLine && PreviousLine->First->isAccessSpecifier() &&
+      !RootToken.Finalized &&
       (!PreviousLine->InPPDirective || !RootToken.HasUnescapedNewline)) {
     // EmptyLineBeforeAccessModifier is handling the case when two access
     // modifiers follow each other.

clang/unittests/Format/FormatTest.cpp

12859 ↗

(On Diff #530067)

removing a test, only hides another regression, this should have been left really, and a fix worked.

[owenpan]

In D152473#4410030, @paulkirth wrote:

In D152473#4409975, @MyDeveloperDay wrote:

In D152473#4409146, @paulkirth wrote:

The point of this patch was to show a regression. I'm not trying to fix anything per se. I'd like https://reviews.llvm.org/D151954 and anything that depends on it reverted, since it breaks clang-format.

I think its respectful to wait a little for the code owners to give you a LG unless the patch is breaking the unit tests.

So first, no one here is trying to be disrespectful, we're all here to make the compiler and surrounding tools better for everyone. That said, it's also normally respectful to revert changes when others report that your patch has a serious regression. https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy

I'm not sure how common this is, but in this case we found a test case that was missing from the unit tests, otherwise the patches that were reverted would never have landed. So, while I understand your point, reverting here is in line w/ the communities policies. If there is a different interpretation I should have on the policy, I'm happy to hear it, but as I read it, our response here is in line w/ our typical developer expectations. My apologies if any of this comes off as rude (being nuanced in text is hard) but I'm trying to be very clear about my interpretation and rationale.

Please note the following from the patch reversion policy you linked above:

If you are asked to revert by another contributor, please revert and discuss the merits of the request offline (unless doing so would further destabilize tip of tree).

and

Reverts should be reasonably timely. ... If you are unsure, we encourage you to reply to the commit thread, give the author a bit to respond, and then proceed with the revert if the author doesn’t seem to be actively responding.

The revert 94e75469597f was wrong as it destabilized the tip of the tree by breaking clang-format unit tests. (See D151954#4404731.) Then another revert (i.e. this patch) accompanied with a test case irrelevant to D152305 that it was reverting was rushed in in order to "fix" the unit tests that 94e75469597f had broken. Also, the regression was triggered by trailing whitespaces in your files. Although reverting D151954 solves the regression for you, it would leave other users in a limb as a few patches prior to and depending on it might have to be reverted as well. Maybe you could have temporarily worked around the regression (e.g. by removing the trailing spaces) while we were investigating the issue?

As a last point we've had our CI broken by this since the initial patch landed, and it's been several days since we first reported the issue. We're certainly not trying to be discourteous w/ our actions, but it I also don't think it's reasonable to expect that when community members report a serious regression, file an issue, and provide a reproducer that they be expected to wait days for a revert or forward fix.

Please review the timestamps. You first reported the issue in https://reviews.llvm.org/D151954#4403921, and the revert 94e75469597f occurred less than 6 hours later.

Anyway, I've relanded D152305 using exactly the same diff and am working on a better solution than D151954. I will add you to the review list to make sure that it won't break your CI.

clang/test/Format/overlapping-lines.cpp

+// RUN: grep -Ev "// *[A-Z-]+:" %s | clang-format --style=Google 2>&1 | FileCheck %s 
+// CHECK-NOT: The new replacement overlaps with an existing replacement.
+
+#ifdef 
+    
+
+#else 
+#endif