clang-format 15 has no effect for simple functions with --lines (works with clang-format 14)

[sean-mcmanus]

Do formatting on the 2nd function only:

int a()
{
return 0;
}

int b()
{
return 0;
}

with a command line like "clang-format test.cpp --style="Mozilla" --lines=5:10" and it works with clang 14. The bug repros with other styles too.

A similar (or the same issue) was reported at #58464 with slightly different repro code, but the root cause could be the same.

[llvmbot]

@llvm/issue-subscribers-clang-format

[Sedeniono]

Since I am also bothered by this bug, I did a git bisect and found that it is a regression introduced in 14c30c7 (github issue #56352). Version 14.0.6 is fine, 15.0.0 is broken.

I do not really know the code base, but I had a closer look. From what I can understand, before commit 14c30c7 the statement IndentForLevel.resize(Line.Level + 1) in LevelIndentTracker::nextLine() caused the LevelIndentTracker to "forget" about previous deeper indentation levels when the formatter leaves block scopes (i.e. goes to lower indentation levels). After the change, IndentForLevel retains the information about previous deeper levels. Since the corresponding elements in IndentForLevel are still there and also are not reset to -1, they are never recomputed.

So, in the example from the OP, the formatter first looks at the return 0 in function a(). The return 0 is on level 1, and has an actual indentation of 0 characters. It is left alone because of the --lines option. Then it continues and eventually looks at the return statement in b(). Since IndentForLevel[Level=1] is still around from the first return 0 and it is also still 0, it simply uses it. So the second return 0 ends up with no indentation.
The same thing happens in #58464, where the LevelIndentTracker remembers the indentation of the void bar() const; and simply reuses it for the auto x = 5; line. I.e. #58464 is a duplicate.

I guess, if formatting the full file (no --lines), the problem does not surface because even though IndentForLevel contains stale data, it happens to be correct since all previous lines were correctly formatted. With --lines this is no longer the case, since the formatting of non-formatted previous lines is basically arbitrary.

To me it seems as if the fix 14c30c7 for the crash in #56352 was incorrect: It removed the IndentForLevel.resize(Line.Level + 1) statement. But why should LevelIndentTracker remember any information about previous deeper levels where the formatter already stepped out? I think the IndentForLevel.resize(Line.Level + 1) should be kept. Instead, the problem in #56352 seems to be that the levels of merged lines (as computed in LineJoiner::join()) are incorrect. Considering the example from #56352, the formatter merges the AnnotatedLine instances for the two closing braces }, where the first inner closing brace has level 1 and the second outer brace has level 0. The resulting merged AnnotatedLine has level 1. Shouldn't this be level 0?

There is one exception, where LevelIndentTracker should actually remember previous deeper indentation levels: When handling a preprocessor directive. These do not really leave a code block but are more of a "temporary insertion".

With my understanding of the issues so far, I created Patch_59178.patch that fixes the issue, adds a unit test and also passes all existing formatter tests. Any opinions on it?
I read the contribution guidelines, but since the code base is completely new, I am unsure whether my understanding and thus my fix is completely off. How is the procedure here? Should I still just submit the patch for review?

[sean-mcmanus]

Cool, that sounds good to me. I don't work on the LLVM code base either, so hopefully someone who does can respond.

[Sedeniono]

I just sent in a fix for review: https://reviews.llvm.org/D151047

[owenca]

/cherry-pick 72ab89e