[clang-format] Fix bug with UT_Always when there is less than one full tab

[guiguiiiiiiii]

Summary

There's a bug in the WhitespaceManager that prevents correct alignment when using tab.
This patch fix a test condition that was triggering the bug and add missing unit tests that would have failed.

The issue can be seen by invoking clang-format on this simple snippet with -style="{BasedOnStyle: llvm, UseTab: Always, AlignConsecutiveAssignments: true, AlignConsecutiveDeclarations: true, TabWidth: 4}"

int a = 42;
int aa = 42;
int aaa = 42;
int aaaa = 42;

Displaying the result in a editor with a tabsize of 4, the first = won't be aligned correctly.

The logic in WhitespaceManager::appendIndentText treats the first tab as a special case, which is what must be done in order to handle tabstop correctly.
However, handling the first tab is done conditionally using the first tab width.
The proposed fix is to handle the first tab independently of its size as the code that follow this test...

Text.append(Spaces / Style.TabWidth, '\t');

...will only work if the '\t' we're adding are positionned on a tabstop (so '\t' is equal to TabWidth in the output, otherwise it will be between 1 and TabWidth - 1)

[klimek]

lib/Format/WhitespaceManager.cpp

678

Why is this not just if (FirstTabWidth <= Spaces) then?

unittests/Format/FormatTest.cpp

9372

Instead of doing the save/restore dance, just put it at the end of a test or create a new test (or alternatively create a new style as copy and then change the settings on that).

[guiguiiiiiiii]

lib/Format/WhitespaceManager.cpp

678

Actually, that was my first way of fixing it.

if (FirstTabWidth <= Spaces && Spaces > 1) // to avoid converting a single space to a tab

Doing so will produce a correct output but introduce what I thought could be an unwanted change : whitespace less than TabWidth might get converted to a tab.
The comment above the test seems to indicate that this behavior was not originally wanted, that's why I changed my mind.

But after thinking it over, I think my fix attempt also introduce an unwanted change. I misinterpreted the original goal of this option.
I went back to the official documentation and it says

Use tabs whenever we need to fill whitespace that spans at least from one tab stop to the next one

This is clearly not what this code is doing. I can land another patch for a more appropriate fix but I wonder if this option should stay like this. Aren't people expecting the formatting to use as much tab as possible with UT_Always ?

Unless I'm mistaken, the following example (TabWidth of 4)

int a = 42;
int aa = 42;
int aaa = 42;
int aaaa = 42;

would become (following what's written in the documentation)

int a    = 42;
int aa   = 42;
int aaa  = 42;
int aaaa = 42; 
// only spaces since there's never enough whitespace to span a full tab stop

And this is what I would expect:

int a	 = 42; // int a\t = 42;
int aa	 = 42; // int aa\t = 42;
int aaa	 = 42; // int aaa\t = 42;
int aaaa = 42; // int aaaa = 42;

unittests/Format/FormatTest.cpp

9372

I guess a new test would be better since the unit tests are really lacking toward using tabs.

[klimek]

lib/Format/WhitespaceManager.cpp

678

The intent of the Always setting was to mirror what the Linux kernel wants, so whatever that is, we should probably follow it :)

[guiguiiiiiiii]

Forgot about this one.
While merging, noticed that it's fixed by https://reviews.llvm.org/D57655. The integrated fix is better as it handle the unlikely but possible tabwidth of 1.
Closing.

lib/Format/WhitespaceManager.cpp

void WhitespaceManager::appendIndentText(std::string &Text,
   switch (Style.UseTab) {
   case FormatStyle::UT_Never:
     Text.append(Spaces, ' ');
     break;
   case FormatStyle::UT_Always: {
     unsigned FirstTabWidth =
         Style.TabWidth - WhitespaceStartColumn % Style.TabWidth;
     // Indent with tabs only when there's at least one full tab.
-    if (FirstTabWidth + Style.TabWidth <= Spaces) {
+    if (Style.TabWidth <= Spaces) {
       Spaces -= FirstTabWidth;
       Text.append("\t");
     }
     Text.append(Spaces / Style.TabWidth, '\t');
     Text.append(Spaces % Style.TabWidth, ' ');
     break;
   }
   case FormatStyle::UT_ForIndentation:

unittests/Format/FormatTest.cpp

EXPECT_EQ("int a   = 5;\n"
             "int oneTwoThree = 123;\n"
             "int oneTwo      = 12;",
             format("int a = 5;\n"
                    "int one = 1;\n"
                    "\n"
                    "int oneTwoThree = 123;\n"
                    "int oneTwo = 12;",
                    Alignment));
+  // Test with use of tabs for alignment
+  FormatStyle::UseTabStyle OldTabStyle = Alignment.UseTab;
+  unsigned OldTabWidth = Alignment.TabWidth;
+  Alignment.UseTab = FormatStyle::UT_Always;
+  Alignment.TabWidth = 4;
+  verifyFormat("int a\t = 42;\n"
+               "int aa   = 42;\n"
+               "int aaa  = 42;\n"
+               "int aaaa = 42;",
+               Alignment);
+  Alignment.UseTab = OldTabStyle;
+  Alignment.TabWidth = OldTabWidth;
   Alignment.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
   verifyFormat("#define A \\\n"
                "  int aaaa       = 12; \\\n"
                "  int b          = 23; \\\n"
                "  int ccc        = 234; \\\n"
                "  int dddddddddd = 2345;",
                Alignment);
   Alignment.AlignEscapedNewlines = FormatStyle::ENAS_Left;
EXPECT_EQ("float a = 5;\n"
             "unsigned oneTwoThree = 123;\n"
             "int      oneTwo = 12;",
             format("float    a = 5;\n"
                    "int one = 1;\n"
                    "\n"
                    "unsigned oneTwoThree = 123;\n"
                    "int oneTwo = 12;",
                    Alignment));
+  // Test with use of tabs for alignment
+  FormatStyle::UseTabStyle OldTabStyle = Alignment.UseTab;
+  unsigned OldTabWidth = Alignment.TabWidth;
+  Alignment.UseTab = FormatStyle::UT_Always;
+  Alignment.TabWidth = 4;
+  verifyFormat("int\t   a = 42;\n"
+               "float  b = 42;\n"
+               "char   c = 42;\n"
+               "double d = 42;",
+               Alignment);
+  Alignment.UseTab = OldTabStyle;
+  Alignment.TabWidth = OldTabWidth;
   // Function prototype alignment
   verifyFormat("int    a();\n"
                "double b();",
                Alignment);
   verifyFormat("int    a(int x);\n"
                "double b();",
                Alignment);
   unsigned OldColumnLimit = Alignment.ColumnLimit;