This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Fix indent for selective formatting.
ClosedPublic

Authored by Sedeniono on May 21 2023, 2:32 AM.

Details

Summary

Fixes github issues #59178, #58464 and #62799.

The problem was that the LevelIndentTracker remembered
the indentation level of previous deeper levels when
leaving a scope. Afterwards, when it entered again a
deeper level, it blindly reused the previous
indentation level. In case the --lines option was used
such that the previous deeper level was not formatted,
that previous level was whatever happened to be there
in the source code. The formatter simply believed it.

This is fixed by letting the LevelIndentTracker forget
the previous deeper levels when stepping out of them
(=> the added resize() in LevelIndentTracker::nextLine()).
Note that this resize() existed in the past until LLVM
14.0.6, but was changed in https://reviews.llvm.org/D129064
(#56352, rev. 14c30c7) to fix a crash. Our commit here
essentially reverts that crash fix. The crash no longer
occurs, most likely because of the changes made in
https://reviews.llvm.org/D144296 (#60843).

However, just reverting 14c30c7 is not enough, since
then the LevelIndentTracker would forget the current
indent if it happens to encounter a preprocessor
directive on a lower level. Hence, the resize()
in nextLine() may not be executed when handling
a preprocessor directive.

The new test in
FormatTestSelective.ReformatRegionAdjustsIndent check
what originally triggered this patch.
The new tests in
FormatMacroRegardlessOfPreviousIndent check the
formatting of a preprocessor #define. They ensure
that the content of LevelIndentTracker::IndentForLevel
does not affect PP directives.

The new test FormatTestSelective.DontAssert checks
a case where a previous iteration of the present
patch triggered an assert().

See
https://github.com/llvm/llvm-project/issues/59178#issuecomment-1542637781
and https://reviews.llvm.org/D151047
for some more details.

Diff Detail

Event Timeline

Sedeniono created this revision.May 21 2023, 2:32 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptMay 21 2023, 2:32 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Sedeniono requested review of this revision.May 21 2023, 2:32 AM

The build failures seem to be unrelated to my changes. The builds of other reviews also show them, and the error messages point to things I haven't changed.

MyDeveloperDay accepted this revision.EditedMay 22 2023, 1:42 AM

Thank you for the indepth explaination in https://github.com/llvm/llvm-project/issues/59178, that was really helpful for me trying to understand what the problem was.

I think your explaination is clear, the reason why you are suggesting this change, and as it passed all the tests, hopefully we can apply the "Beyonce Rule" on https://github.com/llvm/llvm-project/issues/56352, I recommend we pull in @curdeius as a reviewer as the original author of D129064: [clang-format] Avoid crash in LevelIndentTracker.

I'm going to accept this, but I would ask one of the others gives it the all clear too, let us know if you have commit access and if not and want one of us to land it we'll need you name and email to attribute the change to you.

This revision is now accepted and ready to land.May 22 2023, 1:42 AM
Sedeniono added a comment.EditedMay 22 2023, 9:26 AM

@MyDeveloperDay Thanks for the positive review.
Regarding https://github.com/llvm/llvm-project/issues/56352 and the Beyoncé Rule, the original fix for the crash actually added a test (see the changes made at that time). The test also still works, since it crashes/debug-asserts with just the reintroduction of the resize(). That caused me to dig some more and I came up with the A.Level = B.Level; part of the fix to fix the crash. The if (!Line.InPPDirective) part was because of another failing test.

Since this is my first submission to llvm, I do not have commit access. So someone else has to commit and push this into main.
Regarding name and mail: If you use "Sedenion" as name and "39583823+Sedeniono@users.noreply.github.com" as mail, I guess github will attribute the commit to my github account?

Also, should it be merged into the LLVM 16.x branch?

Just a side note, please link to commits, or even better the reviews here when talking about old commits. I found it through the github issue, but I think the other way around is better. I was interested in the commit, and only afterwards in what it thought to fix (aka the issue).

owenpan accepted this revision.May 22 2023, 8:39 PM

Regarding name and mail: If you use "Sedenion" as name and "39583823+Sedeniono@users.noreply.github.com" as mail, I guess github will attribute the commit to my github account?

See D150403#4358845.

Also, should it be merged into the LLVM 16.x branch?

+1. @MyDeveloperDay and @HazardyKnusperkeks?

@owenpan Here is the name and mail in the <> format:
Sedenion <39583823+Sedeniono@users.noreply.github.com>

@HazardyKnusperkeks Ok, I will remember it for the next one.

Also, should it be merged into the LLVM 16.x branch?

+1. @MyDeveloperDay and @HazardyKnusperkeks?

Since it's a regression: yes.

This revision was landed with ongoing or failed builds.May 23 2023, 7:40 PM
This revision was automatically updated to reflect the committed changes.

Ok, sorry, the test failures are on me. There is a lesson learned for me: I naively ran only the formatter unit tests, not the whole test suite of llvm. I somehow assumed that my code changes only affected clang-format, since the UnwrappedLineFormatter is in the "Format" folder. I will need some time to investigate.

I think we need to extract the context of the test from RenameTests to ensure we have it covered here. I don't personally normally run the entire LLVM suite.

I think we need to extract the context of the test from RenameTests to ensure we have it covered here. I don't personally normally run the entire LLVM suite.

Dito, and I didn't know (or would have thought about) that clang-tidy uses clang-format code.

Sedeniono added a comment.EditedMay 24 2023, 1:36 PM

Heh, ok, so I wasn't that naive then to not run the tests of everything :-)

I had a look at the issue. The ClangRenameTests first do some replacements, and then call formatAndApplyReplacements() with them, which also formats the lines containing the replacements. With my patch, calling clang-format.exe -style=llvm --lines=13:13 on the following file (line 13 is the OldAlias old_alias;)

#include "header.h"

    namespace x { class Old {}; }
    namespace ns {
    #define REF(alias) alias alias_var;

    #define ALIAS(old) \
      using old##Alias = x::old; \
      REF(old##Alias);

    ALIAS(Old);

    OldAlias old_alias;
    }

triggers the assert in adjustToUnmodifiedLine(). This is what happens in the RenameAliasTest.AliasesInMacros test.

The issue originates already from the line #define REF(alias) alias alias_var;: These are two AnnotatedLine instances at first, namely #define REF(alias) with AnnotatedLine::Level set to 0 and alias alias_var; with AnnotatedLine::Level equals to 1. They get joined eventually. Since my patch sets A.Level = B.Level; in join(), the joined line gets level 1. But the LevelIndentTracker is still in level 0 (i.e. IndentForLevel contains only 1 element).

It is kind of the same problem why I added the if (!Line.InPPDirective) check in nextLine() in the patch: I think, if both AnnotatedLines are PP directives, then join() should not step "into" or "out of" levels. PP-directives are kind-of "temporary insertions". So, replacing in join() the A.Level = B.Level; from my patch with

assert(A.InPPDirective == B.InPPDirective);
if (!A.InPPDirective && !B.InPPDirective)
  A.Level = B.Level;

seems to fix the problem. At least all Google tests (including the ClangRenameTests) pass that didn't fail without the patch. Any opinions on that idea?

I have to think some more about the levels and what it means regarding join(). I also need to get the clang-tidy test to run locally and also add the above example as a formatter test (if possible in a reduced form). Most likely I won't be able to do this until next week, sorry. :-/

To create a new fix, do I understand the guide correctly that I basically execute arc diff --verbatim and mention Depends on D151047 somewhere in the message? Will it then appear here automatically and reopen the review?

To create a new fix, do I understand the guide correctly that I basically execute arc diff --verbatim and mention Depends on D151047 somewhere in the message? Will it then appear here automatically and reopen the review?

I don't use arc, I just upload the diffs manually.

barannikov88 added a subscriber: barannikov88.EditedMay 25 2023, 4:14 AM

To create a new fix, do I understand the guide correctly that I basically execute arc diff --verbatim and mention Depends on D151047 somewhere in the message? Will it then appear here automatically and reopen the review?

If you want to create a new review, use arc diff --create. It will always create a new review and assign it a different number.

If you want to reopen review because the patch was reverted:

  • cherry-pick the reverted commit;
  • do any necessary adjustments for the tests to pass, amend them;
  • If you're using arc, use arc diff --update D151047 as usual. It will update the current review, preserving its number.
  • If you're not using arc, upload the new diff manually via web interface.

(The --update option is not necessary if the commit message contains URL to this review, but it prevents you from accidentally creating a new review.)

I don't remember if the above steps will change the review status to Reopened.
If that doesn't happen, you can reopen the review manually from the menu below (Add Action... -> Reopen Revision).

Sedeniono reopened this revision.Jun 5 2023, 11:30 AM

Ok, here is the 2nd attempt at fixing the issue. The patch is based on the current main branch.

Regarding LineJoiner::join(): Turns out, not setting A.Level but reverting the original crash fix of https://reviews.llvm.org/D129064 no longer produces the crash. From debugging it I guess this is because of https://reviews.llvm.org/D144296 (although I haven't bisected it to confirm this). I am not really sure whether setting A.Level should or should not be done. So I am keeping it as it is.

But therefore note, since https://reviews.llvm.org/D144296 seems to not have been merged into the LLVM 16, the present fix can probably not be merged without further changes.

So, the new patch basically just reverts the original fix of https://reviews.llvm.org/D129064 (i.e. it re-introduces the resize() in LevelIndentTracker::nextLine()). The other changes are just cosmetics and additional tests.

This revision is now accepted and ready to land.Jun 5 2023, 11:30 AM
Sedeniono updated this revision to Diff 528531.Jun 5 2023, 11:30 AM

Fixes github issues #59178, #58464 and #62799.

The problem was that the LevelIndentTracker remembered
the indentation level of previous deeper levels when
leaving a scope. Afterwards, when it entered again a
deeper level, it blindly reused the previous
indentation level. In case the --lines option was used
such that the previous deeper level was not formatted,
that previous level was whatever happened to be there
in the source code. The formatter simply believed it.

This is fixed by letting the LevelIndentTracker forget
the previous deeper levels when stepping out of them
(=> change in LevelIndentTracker::nextLine()).
Note that this used to be the case until LLVM 14.0.6,
but was changed in https://reviews.llvm.org/D129064
(#56352) to fix a crash. Our commit here essentially
reverts that crash fix. It was incorrect/incomplete.

Even with the revert, the crash from
https://reviews.llvm.org/D129064 (#56352) no longer
occurs, most likely because of the changes made in
https://reviews.llvm.org/D144296 (#60843).

The change in adjustToUnmodifiedLine() ensures that
the assert() is only checked if IndentForLevel is
actually accessed.

The new test FormatTestSelective.DontAssert checks
a case where a previous iteration of the present
patch triggered an assert().

The new tests in
FormatMacroRegardlessOfPreviousIndent check the
formatting of a preprocessor #define. They ensure
that the content of LevelIndentTracker::IndentForLevel
does not affect PP directives.

See
https://github.com/llvm/llvm-project/issues/59178#issuecomment-1542637781
for some more details.

This commit is the 2nd try for
https://reviews.llvm.org/D151047

Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2023, 11:30 AM
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

Oh damn, I executed arc diff --update D151047 on the main branch instead of my own local branch... Sorry.
Can I somehow revert that? Or should I simply use the "abandon review" action and create a entirely new one?

barannikov88 added a comment.EditedJun 5 2023, 11:54 AM

Oh damn, I executed arc diff --update D151047 on the main branch instead of my own local branch... Sorry.
Can I somehow revert that? Or should I simply use the "abandon review" action and create a entirely new one?

Just checkout the local branch and rerun this command :)
You will probably need to reset hard the main branch before or after that.

When you are updating the review and arc requests you to write a message, it asks for "what changed since the last revision".
It would usually be something like "Rebase" or "Address review comments", you don't need to repeat commit message there. You will see the message in the History tab.

Sedeniono updated this revision to Diff 528547.Jun 5 2023, 12:51 PM

Next try at getting the 2nd attempt into review. This is again the complete fix, since the previous revision got reverted.

@barannikov88 ok, thanks. I just did that. The latest diff looks right.
I wondered because phabricator suddenly showed changes in ocaml files, which I certainly did not want.

Sedeniono requested review of this revision.Jun 5 2023, 12:59 PM
clang/lib/Format/UnwrappedLineFormatter.cpp
80

Do we need this check? I assume resize has a check if it has to grow or shrink, so we would check twice when the size is to be changed and once if not.

168–169

And then reformat the comment.

clang/unittests/Format/FormatTestSelective.cpp
663

Have you tried to see what is causing this?

Sedeniono updated this revision to Diff 529517.Jun 8 2023, 12:42 AM

Reformatted comment as requested.

Sedeniono marked an inline comment as done.Jun 8 2023, 12:44 AM
Sedeniono added inline comments.
clang/lib/Format/UnwrappedLineFormatter.cpp
80

The Line.Level < IndentForLevel.size() is not necessary because of lines 63+64. resize itself can both grow and shrink the container.
If Line.Level >= IndentForLevel.size() and we omit the Line.Level < IndentForLevel.size() in line 80, then the resize() in line 81 does nothing because the container already has the size Line.Level+1.

I just added the check in line 80 because I thought it made the intention more clear and explicit: Forget indent levels when going to smaller levels.
Still, I can of course remove it again. Should I?

168–169

Done.

clang/unittests/Format/FormatTestSelective.cpp
663

Yes: It is caused by the ContinueFormatting in UnwrappedLineFormatter::format(), see here. If that flag is true, clang-format changes the formatting of lines that are not affected by the --lines option. It more or less stops this at the next right-brace }. I am guessing that #endif should be handled like a right-brace there, to stop the formatting, but I am not sure: The "continue formatting" was introduced in this commit (http://reviews.llvm.org/D14105). I don't fully understand the intention there, but I guess it is intended for situations where clang-format inserts new {...} blocks? So how does this translate to PP directives?

In any case, I think this is an entirely different issue. The main point of these tests is that the #define some line is correct, i.e. that IndentForLevel is not used for the PP directives, i.e. that the PP directives are not affected by the indent of void test().

uabelho removed a subscriber: uabelho.Jun 8 2023, 12:45 AM
Sedeniono marked an inline comment as done.Jun 8 2023, 12:51 AM
Sedeniono added inline comments.
clang/lib/Format/UnwrappedLineFormatter.cpp
168–169

Mh, it says that it couldn't apply the patch. I uploaded a patch containing as only change the reformatted comment, via the Web UI ("Update Diff" at the top right corner). Should I have uploaded a "squashed" diff, containing also ALL changes that I made before (the resize etc)? In other words, is Phabricator expecting individual patches and it merges and accumulates them itself? Or does each new uploaded patch overwrite all previous patches?

barannikov88 added inline comments.Jun 8 2023, 4:47 AM
clang/lib/Format/UnwrappedLineFormatter.cpp
168–169

The latter

Sedeniono updated this revision to Diff 529630.Jun 8 2023, 9:16 AM

Reformatted comment, and submitting it via arc diff --update now.

Sedeniono updated this revision to Diff 529631.Jun 8 2023, 9:24 AM

Next try at getting all changes to phabricator.

Sedeniono marked an inline comment as done.Jun 8 2023, 9:28 AM
Sedeniono added inline comments.
clang/lib/Format/UnwrappedLineFormatter.cpp
168–169

Ok, it took me 2 tries, but it seems that I managed to upload the full list of changes via arc diff now. I had to specify the origin commit explicitly.

HazardyKnusperkeks added inline comments.
clang/lib/Format/UnwrappedLineFormatter.cpp
80

You could put the intention in a comment and add an assert.

clang/unittests/Format/FormatTestSelective.cpp
663

I've seen (and appreciated) that behavior. If you insert for example a } and format that line, following lines will be re-indented, so they are not off.

This revision is now accepted and ready to land.Jun 8 2023, 2:47 PM
Sedeniono marked an inline comment as done.

Updated diff: Captured the intention of "going to lower levels" via a debug assert in nextLine().

Sedeniono marked an inline comment as done.Jun 12 2023, 12:00 PM
Sedeniono added inline comments.
clang/lib/Format/UnwrappedLineFormatter.cpp
80

Ok, I put the check into an assert() instead and uploaded a new diff. Note that asserting for Line.Level < IndentForLevel.size() would be wrong, since it can also be equal at this point. The comment already states the intention, so did not change it.

Sedeniono marked an inline comment as done.Jun 23 2023, 11:10 AM

@owenpan , @MyDeveloperDay Any opinion on the latest changes?
Otherwise, since I do not have commit rights, someone needs to commit the changes to main (my name and mail: Sedenion <39583823+Sedeniono@users.noreply.github.com>). Thanks!

So, the new patch basically just reverts the original fix of https://reviews.llvm.org/D129064 (i.e. it re-introduces the resize() in LevelIndentTracker::nextLine()). The other changes are just cosmetics and additional tests.

Perhaps we should have two separate patches: the first one that just reverts D129064, and the second for the new tests. That would be much easier to follow, IMO.

clang/lib/Format/UnwrappedLineFormatter.cpp
108
Sedeniono updated this revision to Diff 534688.EditedJun 26 2023, 12:02 PM

As suggested by @owenpan, I split the commits differently. The first commit now contains only the fix + corresponding tests. The second commit contains refactorings/asserts.
My previous statement (that the whole fix consists of a revert of D129064) was not entirely correct: The fix reintroduces the resize() that got removed in D129064, but it additionally surrounds it by the if (!Line.InPPDirective). Without that if, the formatting is wrong if some PP directives are encountered. (The FormatMacroRegardlessOfPreviousIndent tests check this.)
Therefore, I haven't added one specific commit to the patch that only reverts D129064.

Also, as @owenpan suggested, I replaced the !Line.First->is(tok::comment) with Line.First->isNot(tok::comment) in adjustToUnmodifiedLine().

I also rebased the patch to the current main branch.

Sedeniono marked 3 inline comments as done.Jun 26 2023, 12:03 PM

If everyone is ok with the changes, could someone commit it for me since I do not have commit rights? Thanks!
Sedenion <39583823+Sedeniono@users.noreply.github.com>

Note: I cannot really make sense of the error reported by the pre merge checks. I suspect a problem in the build system. I can apply the patch locally on the latest main branch without any conflicts.

As suggested by @owenpan, I split the commits differently. The first commit now contains only the fix + corresponding tests. The second commit contains refactorings/asserts.

How do I select/download the two commits? I expected two separate reviews after you split the previous patch.

Sedeniono updated this revision to Diff 539583.Jul 12 2023, 8:37 AM

@owenpan Oh wow, you are right. Phabricator shows that there are two separate commits, under "Revision Contents" > "Commits", and their commit messages. But apparently there is no way to download some proper *.patch file that contains the meta data, only the total diff.

So, I uploaded a new diff here (D151047): It contains the bugfix. I will also edit the summary at the top with the corresponding commit message.

I have submitted the refactoring commit separately here: https://reviews.llvm.org/D155094

Note that both patches (D151047 and D155094) are fortunately independent of each other. I checked that both can be applied to the latest main branch.

Sedeniono retitled this revision from [clang-format] Fix indentation for selective formatting. to [clang-format] Fix indent for selective formatting..Jul 12 2023, 8:38 AM
Sedeniono edited the summary of this revision. (Show Details)
owenpan added inline comments.Jul 12 2023, 5:39 PM
clang/unittests/Format/FormatTestSelective.cpp
641–646

I suppose this would make the purpose of the test more clear. However, this new test (in either version) would pass without the patch, so it doesn't capture the bug mentioned in D151047#4369742?

649

Again, these tests would pass without the patch.

658

It's the default and can be deleted.

664

Typo.

Sedeniono marked 4 inline comments as done.Jul 15 2023, 6:22 AM
Sedeniono added inline comments.
clang/unittests/Format/FormatTestSelective.cpp
641–646

When I originally submitted the fix for the incorrect selective formatting and you committed it to the main branch, some clang rename unit tests failed because they ran into some assert() in LevelIndentTracker (see this post), making a revert necessary. There was no test in the format Google Tests themselves that caught that mistake. Hence I added this test. But this also means that this test passes with and without the current patch. See an earlier comment where I describe the problem in more details.

Note that the change in LineJoiner::join() (which triggered the problem) is no longer in the current version of the patch because of some other unrelated changes that happened in the main branch since then. Again, please compare another earlier comment.

649

As above, these tests fail with the original first submitted and incomplete fix. As far as I can remember, it happens if you forget the if (!Line.InPPDirective) condition in the patch. The result is that the PP directives end up being formatted "randomly". No other tests in the formatting test suite so far checked the selective PP directive formatting (at least I got no test failures with the incomplete patch). So yes, the new tests pass with and without the patch, but they are still worthwhile.
Also compare this earlier comment.

658

I think setting it explicitly makes it clearer which setting is the important one that is under test here.

664

I cannot see any typo.

Sedeniono updated this revision to Diff 540683.Jul 15 2023, 6:37 AM
Sedeniono marked 4 inline comments as done.

Fixed typo in comment.

Sedeniono added inline comments.Jul 15 2023, 6:39 AM
clang/unittests/Format/FormatTestSelective.cpp
664

Oh, ok, ditto, not dito. Fixed.

owenpan added inline comments.Jul 17 2023, 2:31 AM
clang/unittests/Format/FormatTestSelective.cpp
641–646

Then what do you think about changing the test case as suggested?

657
658

Use EXPECT_EQ to make the default explicit.

665–671
681–687
697–703
Sedeniono marked 6 inline comments as done.Jul 17 2023, 11:02 AM
Sedeniono added inline comments.
clang/unittests/Format/FormatTestSelective.cpp
641–646

I now added the hyperlink. Also, added the "format this line only" comment, but on the "}" line, since that is the line that gets formatted. Also changed the length parameter to format() from 1 to 0; this never made any sense to me (a range of 0 length?), but it is done everywhere else, so whatever. Removing the Style.FixNamespaceComments = false; line would be wrong because the value is true for the LLVM style, so I kept it.

Sedeniono marked an inline comment as done.

Implemented review changes as suggested by @owenpan.

If this is finally ok, someone please commit it to main using the following:
Sedenion <39583823+Sedeniono@users.noreply.github.com>
Thanks!

owenpan added inline comments.Jul 17 2023, 5:21 PM
clang/unittests/Format/FormatTestSelective.cpp
643–646

Then I'd remove Style.FixNamespaceComments as a factor by formatting a non-brace line.

owenpan accepted this revision.EditedJul 17 2023, 5:24 PM

LGTM except for D151047#4508359.

Sedeniono added inline comments.Jul 18 2023, 8:20 AM
clang/unittests/Format/FormatTestSelective.cpp
643–646

The problem I observed did not trigger by formatting some arbitrary line. It triggered when formatting specifically the line with the closing }. Formatting any other line makes therefore no sense. I just tried it again.
FixNamespaceComments = false means that the expected result is that the code fragment remains unchanged. With FixNamespaceComments = true, the formatter would change it (by adding the // namespace ns comment after the brace).

If you are still offended by the test, I will give up and will remove it. There are still the clang rename tests after all that execute the particular code path and uncovered the problem originally.

owenpan added inline comments.Jul 18 2023, 12:46 PM
clang/unittests/Format/FormatTestSelective.cpp
643–646

Got it! I didn’t know the assertion was triggered by formatting the closing brace. We should keep the test as is then.

This revision was automatically updated to reflect the committed changes.