[clang-format] Recognize escape sequences when breaking strings

[sstwcw]

Summary

When a string literal is too long to fit within the column limit, the
formatter breaks it into several lines. When breaking a string, the
formatter has always preferred to break it where there is already
whitespace in the string. That is, if the string can be made to fit
in the column limit by being broken at a whitespace character, then it
will not be broken at a location that does not have a whitespace
character. Take for example the string "some text". Even if the
column limit allows for 6 characters on the first line, it will get
broken into "some " and "text". This has always been the case.

However, the program did not consider escape sequences when looking
for whitespace in the string to break it at. Say the original string
has an escape sequence instead of a space like "some\ttext" or
"some\ntext". Before this patch, the program treated the \t or
\n as an ordinary character. It could break the string into
"some\nt" and "ext".

After this patch, the program will treat escape sequences for
whitespace characters as preferred locations to break the string at
just like ordinary unescaped whitespace characters. So the string
"some\ntext" will get broken into "some\n" and "text" when it is
too long to fit in one line, even if the column limit allows
"some\nt" to fit on the first line. And also \n is preferred over
other whitespace. That is, if a string that needs to be broken into
several lines has any newline \n escape sequence in it, and breaking
it at one of the newline escape sequences makes it fit in the column
limit, then it will get broken at one of the newlines.

This patch only affects strings that are too long. A string that
already fits in the column limit still will not get broken, even if it
has \n in it.

[sstwcw]

clang/lib/Format/BreakableToken.cpp

225

Should I change the threshold from 2 to 1?

It is set to 2 because it is the old behavior. There is already a test.

already in the test:
"some"
" tex"
"t"

I expect:
"some"
" "
"text"

[MyDeveloperDay]

is there a git issue for this, I'm struggling to understand the problem we are solving.

[sstwcw]

See the updated summary. There does not seem to be an issue on GitHub when I searched. I simply noticed it when reading the code in order to understand how strings got broken.

[HazardyKnusperkeks]

clang/lib/Format/BreakableToken.cpp

223–226

What if the new line is after the limit?

225

Since it was explicit, I'd keep it that way.

[sstwcw]

clang/lib/Format/BreakableToken.cpp

223–226

Then the loop will exit on line 197. I added a test for it.

[sstwcw]

• Add tests for newline outside of column limit

[MyDeveloperDay]

What would happen in a \r\n example

clang/lib/Format/BreakableToken.cpp

207

Are you testing \r \v \f

[sstwcw]

• Add tests for other escape sequences

[sstwcw]

In D154091#4470158, @MyDeveloperDay wrote:

What would happen in a \r\n example

If the entire \r\n part fits on the first line, the break is after \r\n. If only the part up to \r fits on the first line, the break is between \r and \n.

clang/lib/Format/BreakableToken.cpp

207

I am now.

[MyDeveloperDay]

What would happen for say unicode escapes?

x = "XXXThis is a string \u0041 With stuff after";

would it break after the \u?

x = "XXXThis is a string \u
0041 With stuff after";

[MyDeveloperDay]

clang/lib/Format/BreakableToken.cpp

199

Can we add a unit test for escape sequences > \X which I assume this handles

[sstwcw]

It won't break the string inside an escape sequence. But escape sequences like \040, \x20, and \u0020 don't get recognized as whitespace.

clang/lib/Format/BreakableToken.cpp

199

There are already tests that verify that the string does not get broken inside an escape sequence in BreaksWideAndNSStringLiterals and DoNotBreakStringLiteralsInEscapeSequence. And escape sequences longer than 1 character following the backslash should not begin with one of these letters. So I don't see what tests I should add.

[MyDeveloperDay]

clang/lib/Format/BreakableToken.cpp

199

I was thinking about your tests having a test case which shows it shouldn't break, in case someone add case 'x' into your code, i.e. asserting the negative.

[sstwcw]

• Add tests for non-whitespace sequences

[sstwcw]

I am closing it since no one seems to like it.

clang/lib/Format/BreakableToken.cpp

 getStringSplit(StringRef Text, unsigned UsedColumns, unsigned ColumnLimit,
                unsigned TabWidth, encoding::Encoding Encoding) {
   // FIXME: Reduce unit test case.
   if (Text.empty())
     return BreakableToken::Split(StringRef::npos, 0);
   if (ColumnLimit <= UsedColumns)
     return BreakableToken::Split(StringRef::npos, 0);
   unsigned MaxSplit = ColumnLimit - UsedColumns;
-  StringRef::size_type SpaceOffset = 0;
+  StringRef::size_type NewLine = 0;
+  StringRef::size_type AfterSpace = 0;
   StringRef::size_type SlashOffset = 0;
   StringRef::size_type WordStartOffset = 0;
   StringRef::size_type SplitPoint = 0;
   for (unsigned Chars = 0;;) {
     unsigned Advance;
-    if (Text[0] == '\\') {
+    bool EscapeSequence = Text[0] == '\\';
+    if (EscapeSequence) {
       Advance = encoding::getEscapeSequenceLength(Text);
       Chars += Advance;
     } else {
       Advance = encoding::getCodePointNumBytes(Text[0], Encoding);
       Chars += encoding::columnWidthWithTabs(
           Text.substr(0, Advance), UsedColumns + Chars, TabWidth, Encoding);
     }
 
     if (Chars > MaxSplit || Text.size() <= Advance)
       break;
 
+    if (EscapeSequence && Advance == 2) {
+      switch (Text[1]) {
+      case 'n':
+        NewLine = SplitPoint + 2;
+        break;
+      case 'f':
+      case 'r':
+      case 't':
+      case 'v':
+        AfterSpace = SplitPoint + 2;
+        break;
+      }
+    }
     if (IsBlank(Text[0]))
-      SpaceOffset = SplitPoint;
+      AfterSpace = SplitPoint + 1;
     if (Text[0] == '/')
       SlashOffset = SplitPoint;
     if (Advance == 1 && !isAlphanumeric(Text[0]))
       WordStartOffset = SplitPoint;
 
     SplitPoint += Advance;
     Text = Text.substr(Advance);
   }
 
-  if (SpaceOffset != 0)
-    return BreakableToken::Split(SpaceOffset + 1, 0);
+  if (NewLine != 0)
+    return BreakableToken::Split(NewLine, 0);
+  if (AfterSpace >= 2)
+    return BreakableToken::Split(AfterSpace, 0);
   if (SlashOffset != 0)
     return BreakableToken::Split(SlashOffset + 1, 0);
   if (WordStartOffset != 0)
     return BreakableToken::Split(WordStartOffset + 1, 0);
   if (SplitPoint != 0)
     return BreakableToken::Split(SplitPoint, 0);
   return BreakableToken::Split(StringRef::npos, 0);
 }

clang/unittests/Format/FormatTest.cpp

EXPECT_EQ("\"some\"\n"
             "\" tex\"\n"
             "\" and\"",
             format("\"some tex and\"", getLLVMStyleWithColumns(6)));
   EXPECT_EQ("\"some\"\n"
             "\"/tex\"\n"
             "\"/and\"",
             format("\"some/tex/and\"", getLLVMStyleWithColumns(6)));
 
+  // Escape sequences should be recognized.  When the limit is 13 columns,
+  // another character can fit on the first line between the escape sequence and
+  // the closing quote.  We know the escape sequence is recognized when the
+  // program doesn't put the character following the escape sequence on the
+  // first line.
+  verifyFormat(R"(x = "some\n"
+    "text";)",
+               R"(x = "some\ntext";)", getLLVMStyleWithColumns(12));
+  verifyFormat(R"(x = "some\n"
+    "text";)",
+               R"(x = "some\ntext";)", getLLVMStyleWithColumns(13));
+  verifyFormat(R"(x = "some\n"
+    " text";)",
+               R"(x = "some\n text";)", getLLVMStyleWithColumns(13));
+  verifyFormat(R"(x = "some\t"
+    "text";)",
+               R"(x = "some\ttext";)", getLLVMStyleWithColumns(12));
+  verifyFormat(R"(x = "some\t"
+    "text";)",
+               R"(x = "some\ttext";)", getLLVMStyleWithColumns(13));
+  verifyFormat(R"(x = "some\f"
+    "text";)",
+               R"(x = "some\ftext";)", getLLVMStyleWithColumns(13));
+  verifyFormat(R"(x = "some\r"
+    "text";)",
+               R"(x = "some\rtext";)", getLLVMStyleWithColumns(13));
+  verifyFormat(R"(x = "some\v"
+    "text";)",
+               R"(x = "some\vtext";)", getLLVMStyleWithColumns(13));
+  // A newline outside of the column limit should not cause a break.
+  verifyFormat(R"(x = "something "
+    "so\nshort";)",
+               R"(x = "something so\nshort";)", getLLVMStyleWithColumns(16));
+  verifyFormat(R"(x = "something "
+    "so\nshort";)",
+               R"(x = "something so\nshort";)", getLLVMStyleWithColumns(19));
+  verifyFormat(R"(x = "something so\n"
+    "short";)",
+               R"(x = "something so\nshort";)", getLLVMStyleWithColumns(20));
+  // Escape sequences for characters that are not whitespace should not be
+  // treated as whitespace.
+  verifyFormat(R"(x = "some\at"
+    "ext";)",
+               R"(x = "some\atext";)", getLLVMStyleWithColumns(13));
+  verifyFormat(R"(x = "some\bt"
+    "ext";)",
+               R"(x = "some\btext";)", getLLVMStyleWithColumns(13));
+  verifyFormat(R"(x = "some\et"
+    "ext";)",
+               R"(x = "some\etext";)", getLLVMStyleWithColumns(13));
+  verifyFormat(R"(x = "s\157me"
+    "text";)",
+               R"(x = "s\157metext";)", getLLVMStyleWithColumns(13));
+  verifyFormat(R"(x = "s\x6fme"
+    "text";)",
+               R"(x = "s\x6fmetext";)", getLLVMStyleWithColumns(13));
+  verifyFormat(R"(x = "s\u006fm"
+    "etext";)",
+               R"(x = "s\u006fmetext";)", getLLVMStyleWithColumns(14));
+  verifyFormat(R"(x = "s\U0000006fm"
+    "etext";)",
+               R"(x = "s\U0000006fmetext";)", getLLVMStyleWithColumns(18));
+
   EXPECT_EQ("variable =\n"
             "    \"long string \"\n"
             "    \"literal\";",
             format("variable = \"long string literal\";",
                    getLLVMStyleWithColumns(20)));
 
   EXPECT_EQ("variable = f(\n"
             "    \"long string \"\n"