[clang-format] [PR49298] Sort includes pass will sort inside raw strings

[MyDeveloperDay]

Summary

https://bugs.llvm.org/show_bug.cgi?id=49298

clang-format does not respect raw string literals when sorting includes

const char *RawStr = R"(
#include "headerB.h"
#include "headerA.h"
)";

Running clang-format over with SortIncludes enabled transforms this code to:

const char *RawStr = R"(
#include "headerA.h"
#include "headerB.h"
)";

The following code tries to minimize this impact during IncludeSorting, by treating R"( and )" as equivalent of // clang-format off/on

[curdeius]

clang/lib/Format/Format.cpp

2603

"we we"?

2604

This won't be enough for raw literals with custom delimiter e.g. R"xyz(...)xyz".
But well, without any code parsing, this function will always be easily outsmarted I guess.
Maybe we can use regexp here and save the delimiter? (And then check it below)

[HazardyKnusperkeks]

clang/lib/Format/Format.cpp

2604

And then we need to check first for closing, since we can close and reopen on the same line, or not?

[curdeius]

clang/lib/Format/Format.cpp

2604

It's done below anyway (i.e. Trimmed is still the same line), nope?

[HazardyKnusperkeks]

clang/lib/Format/Format.cpp

2604

Yes but if we save the delimiter we will save the new one, before checking for the old one to close.
I was just thinking we must close the old one correctly before "opening" the new one. But after rethinking about it, that shouldn't matter.

[MyDeveloperDay]

Allow for optional Raw String CharSequences

[curdeius]

clang/lib/Format/Format.cpp

2589

A raw string literal is:

prefix(optional) R"d-char-sequence(optional) (r-char-sequence(optional))d-char-sequence(optional)"	

d-char-sequence	-	A sequence of one or more d-chars, at most 16 characters long
d-char	-	A character from the basic source character set (until C++23)basic character set (since C++23), except parentheses, backslash and spaces

So, you missed the digits and the characters: _{}[]#<>%:;.?*+-/^&|~!=,'.
Please add a test case.
Mind the need to escape the square brackets [] and the minus sign - in the regexp (the latter can be put at the beginning or at the end too).

Cf. https://en.cppreference.com/w/cpp/language/string_literal, https://godbolt.org/z/rb61WzMcs

2606–2608

Since CharSequence is empty, you might want to remove the if altogether and create RawStringTermination unconditionally.

Oh, actually, as it's used in the for loop, you *have to* reassign RawStringTermination, otherwise the code like:

R"x(...
...)x"; // RawStringTermination is "x"
R"(...
...); // RawStringTermination would still be "x"
#include <b.h> // not sorted but it should
#include <a.h> // not sorted but it should

will misbehave.
Please create a test case for this.

[MyDeveloperDay]

clang/lib/Format/Format.cpp

2606–2608

Correct I need to remove the if

[MyDeveloperDay]

So, you missed the digits and the characters: _{}[]#<>%:;.?*+-/^&|~!=,'.

The standard is nuts sometimes... let me add that madness in ;-)

[MyDeveloperDay]

Add a suitable amount of C++ madness, (to the limit of what I could coax the regex to accept)

[HazardyKnusperkeks]

clang/unittests/Format/SortIncludesTest.cpp

1162

You should add a #undef X.

[curdeius]

clang/lib/Format/Format.cpp

2590

I'm picky but you missed [ and ], no?

2604

Will this match the first or the last occurrence? If it's the last then it's ok. Otherwise, we should change it. Also, please test something along the lines of:

R"x(...)x" ... on the same line R"y(
#include "..."
)y"

clang/unittests/Format/SortIncludesTest.cpp

1150

Can't | be tested? Same for [ and ]?

[MyDeveloperDay]

clang/lib/Format/Format.cpp

2590

I couldn't get them to work, even if I escaped them.. let me go look at the Regex unit tests

[curdeius]

clang/lib/Format/Format.cpp

2590

You can try putting`] at the beginning (just after the opening[`). It *should* work.

[MyDeveloperDay]

Add support for | but still can't get [] to work in the regex (checked the unit tests for Support/Regex and its not clear if its supported)

[HazardyKnusperkeks]

In D115168#3185199, @MyDeveloperDay wrote:

Add support for | but still can't get [] to work in the regex (checked the unit tests for Support/Regex and its not clear if its supported)

How about "R\"(([A-Za-z0-9_{}#<>%:;.?*+/^&\\$|~!=,'\\-]|]|\[)*)\\(", not putting the [] symbols within the [].

Does it work with std::regex which should also be available?

[MyDeveloperDay]

Thanks @HazardyKnusperkeks you got me over the line, I was able to escape the [ but not the ] so I used your trick there

"R\"(([\\[A-Za-z0-9_{}#<>%:;.?*+/^&\\$|~!=,'\\-]|])*)\\("

Add unit tests to cover those cases too

[curdeius]

LGTM. Great!

clang/lib/Format/Format.cpp

tooling::Replacements sortCppIncludes(const FormatStyle &Style, StringRef Code,
   //
   // FIXME: Do some validation, e.g. edit distance of the base name, to fix
   // cases where the first #include is unlikely to be the main header.
   tooling::IncludeCategoryManager Categories(Style.IncludeStyle, FileName);
   bool FirstIncludeBlock = true;
   bool MainIncludeFound = false;
   bool FormattingOff = false;
 
+  llvm::Regex RawStringRegex(
+      "R\"(([\\[A-Za-z0-9_{}#<>%:;.?*+/^&\\$|~!=,'\\-]|])*)\\(");
+  SmallVector<StringRef, 2> RawStringMatches;
+  std::string RawStringTermination = ")\"";
+
   for (;;) {
     auto Pos = Code.find('\n', SearchFrom);
     StringRef Line =
         Code.substr(Prev, (Pos != StringRef::npos ? Pos : Code.size()) - Prev);
 
     StringRef Trimmed = Line.trim();
+
+    // #includes inside raw string literals need to be ignored.
+    // or we will sort the contents of the string.
+    // Skip past until we think we are at the rawstring literal close.
+    if (RawStringRegex.match(Trimmed, &RawStringMatches)) {
+      std::string CharSequence = RawStringMatches[1].str();
+      RawStringTermination = ")" + CharSequence + "\"";
+      FormattingOff = true;
+    }
+
+    if (Trimmed.contains(RawStringTermination)) {
+      FormattingOff = false;
+    }
+
     if (Trimmed == "// clang-format off" || Trimmed == "/* clang-format off */")
       FormattingOff = true;
     else if (Trimmed == "// clang-format on" ||
              Trimmed == "/* clang-format on */")
       FormattingOff = false;
 
     const bool EmptyLineSkipped =
         Trimmed.empty() &&

clang/unittests/Format/SortIncludesTest.cpp

StringRef Sorted = "#include <a.h>\n"
                      "#include <b.h>\n";
   StringRef Unsorted = "#include <b.h>\n"
                        "#include <a.h>\n";
   EXPECT_EQ(Sorted, sort(Unsorted));
   FmtStyle.DisableFormat = true;
   EXPECT_EQ(Unsorted, sort(Unsorted, "input.cpp", 0));
 }
 
+TEST_F(SortIncludesTest, DisableRawStringLiteralSorting) {
+
+  EXPECT_EQ("const char *t = R\"(\n"
+            "#include <b.h>\n"
+            "#include <a.h>\n"
+            ")\";",
+            sort("const char *t = R\"(\n"
+                 "#include <b.h>\n"
+                 "#include <a.h>\n"
+                 ")\";",
+                 "test.cxx", 0));
+  EXPECT_EQ("const char *t = R\"x(\n"
+            "#include <b.h>\n"
+            "#include <a.h>\n"
+            ")x\";",
+            sort("const char *t = R\"x(\n"
+                 "#include <b.h>\n"
+                 "#include <a.h>\n"
+                 ")x\";",
+                 "test.cxx", 0));
+  EXPECT_EQ("const char *t = R\"xyz(\n"
+            "#include <b.h>\n"
+            "#include <a.h>\n"
+            ")xyz\";",
+            sort("const char *t = R\"xyz(\n"
+                 "#include <b.h>\n"
+                 "#include <a.h>\n"
+                 ")xyz\";",
+                 "test.cxx", 0));
+
+  EXPECT_EQ("#include <a.h>\n"
+            "#include <b.h>\n"
+            "const char *t = R\"(\n"
+            "#include <b.h>\n"
+            "#include <a.h>\n"
+            ")\";\n"
+            "#include <c.h>\n"
+            "#include <d.h>\n"
+            "const char *t = R\"x(\n"
+            "#include <f.h>\n"
+            "#include <e.h>\n"
+            ")x\";\n"
+            "#include <g.h>\n"
+            "#include <h.h>\n"
+            "const char *t = R\"xyz(\n"
+            "#include <j.h>\n"
+            "#include <i.h>\n"
+            ")xyz\";\n"
+            "#include <k.h>\n"
+            "#include <l.h>",
+            sort("#include <b.h>\n"
+                 "#include <a.h>\n"
+                 "const char *t = R\"(\n"
+                 "#include <b.h>\n"
+                 "#include <a.h>\n"
+                 ")\";\n"
+                 "#include <d.h>\n"
+                 "#include <c.h>\n"
+                 "const char *t = R\"x(\n"
+                 "#include <f.h>\n"
+                 "#include <e.h>\n"
+                 ")x\";\n"
+                 "#include <h.h>\n"
+                 "#include <g.h>\n"
+                 "const char *t = R\"xyz(\n"
+                 "#include <j.h>\n"
+                 "#include <i.h>\n"
+                 ")xyz\";\n"
+                 "#include <l.h>\n"
+                 "#include <k.h>",
+                 "test.cc", 4));
+
+  EXPECT_EQ("const char *t = R\"AMZ029amz(\n"
+            "#include <b.h>\n"
+            "#include <a.h>\n"
+            ")AMZ029amz\";",
+            sort("const char *t = R\"AMZ029amz(\n"
+                 "#include <b.h>\n"
+                 "#include <a.h>\n"
+                 ")AMZ029amz\";",
+                 "test.cxx", 0));
+
+  EXPECT_EQ("const char *t = R\"-AMZ029amz(\n"
+            "#include <b.h>\n"
+            "#include <a.h>\n"
+            ")-AMZ029amz\";",
+            sort("const char *t = R\"-AMZ029amz(\n"
+                 "#include <b.h>\n"
+                 "#include <a.h>\n"
+                 ")-AMZ029amz\";",
+                 "test.cxx", 0));
+
+  EXPECT_EQ("const char *t = R\"AMZ029amz-(\n"
+            "#include <b.h>\n"
+            "#include <a.h>\n"
+            ")AMZ029amz-\";",
+            sort("const char *t = R\"AMZ029amz-(\n"
+                 "#include <b.h>\n"
+                 "#include <a.h>\n"
+                 ")AMZ029amz-\";",
+                 "test.cxx", 0));
+
+  EXPECT_EQ("const char *t = R\"AM|029amz-(\n"
+            "#include <b.h>\n"
+            "#include <a.h>\n"
+            ")AM|029amz-\";",
+            sort("const char *t = R\"AM|029amz-(\n"
+                 "#include <b.h>\n"
+                 "#include <a.h>\n"
+                 ")AM|029amz-\";",
+                 "test.cxx", 0));
+
+  EXPECT_EQ("const char *t = R\"AM[029amz-(\n"
+            "#include <b.h>\n"
+            "#include <a.h>\n"
+            ")AM[029amz-\";",
+            sort("const char *t = R\"AM[029amz-(\n"
+                 "#include <b.h>\n"
+                 "#include <a.h>\n"
+                 ")AM[029amz-\";",
+                 "test.cxx", 0));
+
+  EXPECT_EQ("const char *t = R\"AM]029amz-(\n"
+            "#include <b.h>\n"
+            "#include <a.h>\n"
+            ")AM]029amz-\";",
+            sort("const char *t = R\"AM]029amz-(\n"
+                 "#include <b.h>\n"
+                 "#include <a.h>\n"
+                 ")AM]029amz-\";",
+                 "test.cxx", 0));
+
+#define X "AMZ029amz{}+!%*=_:;',.<>|/?#~-$"
+
+  EXPECT_EQ("const char *t = R\"" X "(\n"
+            "#include <b.h>\n"
+            "#include <a.h>\n"
+            ")" X "\";",
+            sort("const char *t = R\"" X "(\n"
+                 "#include <b.h>\n"
+                 "#include <a.h>\n"
+                 ")" X "\";",
+                 "test.cxx", 0));
+
+#undef X
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang