[clang-format] Fix alignment in #else preprocessor blocks

[mitchell-stellar]

Summary

Fixes https://github.com/llvm/llvm-project/issues/36070

clang-format makes multiple passes when #if/#else preprocessor blocks are found. It will make one pass for normal code and code in the #if block, and then it will make another pass for just the code in #else blocks. This often results in invalid alignment inside the else blocks because they do not have any scope or indentAndNestingLevel context from their surrounding tokens/lines.

This patch remedies that by caching any initial indentAndNestingLevel from a second pass and not breaking/returning early when a scope change is detected.

[HazardyKnusperkeks]

Please reupload the diff with the context.

clang/unittests/Format/FormatTest.cpp

5785

There should be a verifyFormat variant fitting for you.

[mitchell-stellar]

Added context and use verifyFormat().

[HazardyKnusperkeks]

Looks good, please wait for other reviewers.

clang/unittests/Format/FormatTest.cpp

5747

Why limit the columns?

[MyDeveloperDay]

Should we test nested if/else/endif

[mitchell-stellar]

Do not limit columns and added nested test case.

[MyDeveloperDay]

LGTM

[owenpan]

I'll revert this patch due to the regression https://github.com/llvm/llvm-project/issues/61498 unless any of you (and @rymiel ) objects.

[mitchell-stellar]

After reviewing the GitHub issue, it looks like the IndentAndNestingLevel is sticky over the entire scope for #else blocks, so the regression only occurs within #else blocks in the same scope. The bug I fixed affected alignment in all #else blocks, regardless of scope.

I believe that if you choose to revert this @owenpan, you will see more bad alignments throughout #else blocks in an entire file. If you keep the fix, you will see bad alignment in #else blocks within the current scope. Pick your poison I guess. I personally prefer better alignment in separate #else blocks over the entire file, which is what prompted my bugfix in the first place. On those grounds I object to reverting the fix.

[mitchell-stellar]

I retract my objection. I forgot that from clang-format's perspective, #else blocks in separate functions have the same indent and nesting level, and thus the same "scope", so the alignment issues may persist there. You will see misalignment in #else blocks over an entire source file whether you keep or revert this fix (i.e. pick your poison.) I suppose based on precedent we keep the misalignments I wanted to fix since I introduced misalignments in other cases.

[owenpan]

See D150057.

clang/lib/Format/WhitespaceManager.cpp

static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
   unsigned EndOfSequence = 0;
 
   // Measure the scope level (i.e. depth of (), [], {}) of the first token, and
   // abort when we hit any token in a higher scope than the starting one.
   auto IndentAndNestingLevel = StartAt < Changes.size()
                                    ? Changes[StartAt].indentAndNestingLevel()
                                    : std::tuple<unsigned, unsigned, unsigned>();
 
+  // Keep track if the first token has a non-zero indent and nesting level.
+  // This can happen when aligning the contents of "#else" preprocessor blocks,
+  // which is done separately.
+  bool HasInitialIndentAndNesting =
+      StartAt == 0 &&
+      IndentAndNestingLevel > std::tuple<unsigned, unsigned, unsigned>();
+
   // Keep track of the number of commas before the matching tokens, we will only
   // align a sequence of matching tokens if they are preceded by the same number
   // of commas.
   unsigned CommasBeforeLastMatch = 0;
   unsigned CommasBeforeMatch = 0;
 
   // Whether a matching token has been found on the current line.
   bool FoundMatchOnLine = false;
auto AlignCurrentSequence = [&] {
     WidthAnchor = 0;
     WidthRight = 0;
     StartOfSequence = 0;
     EndOfSequence = 0;
   };
 
   unsigned i = StartAt;
   for (unsigned e = Changes.size(); i != e; ++i) {
-    if (Changes[i].indentAndNestingLevel() < IndentAndNestingLevel)
+    if (Changes[i].indentAndNestingLevel() < IndentAndNestingLevel) {
+      if (!HasInitialIndentAndNesting)
         break;
+      // The contents of preprocessor blocks are aligned separately.
+      // If the initial preprocessor block is indented or nested (e.g. it's in
+      // a function), do not align and exit after finishing this scope block.
+      // Instead, align, and then lower the baseline indent and nesting level
+      // in order to continue aligning subsequent blocks.
+      EndOfSequence = i;
+      AlignCurrentSequence();
+      IndentAndNestingLevel =
+          Changes[i].indentAndNestingLevel(); // new baseline
+    }
 
     if (Changes[i].NewlinesBefore != 0) {
       CommasBeforeMatch = 0;
       EndOfSequence = i;
 
       // Whether to break the alignment sequence because of an empty line.
       bool EmptyLineBreak =
           (Changes[i].NewlinesBefore > 1) && !ACS.AcrossEmptyLines;

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, IndentPreprocessorDirectives) {
   // Test single comment before preprocessor
   verifyFormat("// Comment\n"
                "\n"
                "#if 1\n"
                "#endif",
                Style);
 }
 
+TEST_F(FormatTest, FormatAlignInsidePreprocessorElseBlock) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AlignConsecutiveAssignments.Enabled = true;
+  Style.AlignConsecutiveDeclarations.Enabled = true;
+
+  // Test with just #if blocks.
+  verifyFormat("void f1() {\n"
+               "#if 1\n"
+               "  int foo    = 1;\n"
+               "  int foobar = 2;\n"
+               "#endif\n"
+               "}\n"
+               "#if 1\n"
+               "int baz = 3;\n"
+               "#endif\n"
+               "void f2() {\n"
+               "#if 1\n"
+               "  char *foobarbaz = \"foobarbaz\";\n"
+               "  int   quux      = 4;\n"
+               "}",
+               Style);
+
+  // Test with just #else blocks.
+  verifyFormat("void f1() {\n"
+               "#if 1\n"
+               "#else\n"
+               "  int foo    = 1;\n"
+               "  int foobar = 2;\n"
+               "#endif\n"
+               "}\n"
+               "#if 1\n"
+               "#else\n"
+               "int baz = 3;\n"
+               "#endif\n"
+               "void f2() {\n"
+               "#if 1\n"
+               "#else\n"
+               "  char *foobarbaz = \"foobarbaz\";\n"
+               "  int   quux      = 4;\n"
+               "}",
+               Style);
+
+  // Test with a mix of #if and #else blocks.
+  verifyFormat("void f1() {\n"
+               "#if 1\n"
+               "#else\n"
+               "  int foo    = 1;\n"
+               "  int foobar = 2;\n"
+               "#endif\n"
+               "}\n"
+               "#if 1\n"
+               "int baz = 3;\n"
+               "#endif\n"
+               "void f2() {\n"
+               "#if 1\n"
+               "#else\n"
+               "  // prevent alignment with #else in f1\n"
+               "  char *foobarbaz = \"foobarbaz\";\n"
+               "  int   quux      = 4;\n"
+               "}",
+               Style);
+
+  // Test with nested #if and #else blocks.
+  verifyFormat("void f1() {\n"
+               "#if 1\n"
+               "#else\n"
+               "#if 2\n"
+               "#else\n"
+               "  int foo    = 1;\n"
+               "  int foobar = 2;\n"
+               "#endif\n"
+               "#endif\n"
+               "}\n"
+               "#if 1\n"
+               "#else\n"
+               "#if 2\n"
+               "int baz = 3;\n"
+               "#endif\n"
+               "#endif\n"
+               "void f2() {\n"
+               "#if 1\n"
+               "#if 2\n"
+               "#else\n"
+               "  // prevent alignment with #else in f1\n"
+               "  char *foobarbaz = \"foobarbaz\";\n"
+               "  int   quux      = 4;\n"
+               "#endif\n"
+               "#endif\n"
+               "}",
+               Style);
+}
+
 TEST_F(FormatTest, FormatHashIfNotAtStartOfLine) {
   verifyFormat("{\n  { a #c; }\n}");
 }
 
 TEST_F(FormatTest, FormatUnbalancedStructuralElements) {
   EXPECT_EQ("#define A \\\n  {       \\\n    {\nint i;",
             format("#define A { {\nint i;", getLLVMStyleWithColumns(11)));
   EXPECT_EQ("#define A \\\n  }       \\\n  }\nint i;",