[clang-format] Do not emit replacements while regrouping if Cpp includes are OK

[krasimir]

Summary

Currently clang-format would always emit a replacement for multi-block #include
sections if IBS_Regroup, even if the sections are correct:

% cat ~/test.h
#include <a.h>

#include "b.h"
% bin/clang-format --output-replacements-xml -style=google ~/test.h
<?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
<replacement offset='0' length='30'>#include &lt;a.h>&#10;&#10;#include "b.h"</replacement>
</replacements>
%

This change makes clang-format not emit replacements in this case.
The logic is similar to the one implemented for Java in r354452.

[ioeric]

unittests/Format/SortIncludesTest.cpp

28

As most cases would pass 1, maybe it would make sense to make this a class member that defaults to 1? It can be explicitly set in specific tests when needed.

[krasimir]

• Address review comments

[MyDeveloperDay]

Possible fix for PR43372 (https://bugs.llvm.org/show_bug.cgi?id=43372)

lib/Format/Format.cpp

 // source order.
 // #include directives with the same text will be deduplicated, and only the
 // first #include in the duplicate #includes remains. If the `Cursor` is
 // provided and put on a deleted #include, it will be moved to the remaining
 // #include in the duplicate #includes.
 static void sortCppIncludes(const FormatStyle &Style,
                             const SmallVectorImpl<IncludeDirective> &Includes,
                             ArrayRef<tooling::Range> Ranges, StringRef FileName,
+                            StringRef Code,
                             tooling::Replacements &Replaces, unsigned *Cursor) {
   unsigned IncludesBeginOffset = Includes.front().Offset;
   unsigned IncludesEndOffset =
       Includes.back().Offset + Includes.back().Text.size();
   unsigned IncludesBlockSize = IncludesEndOffset - IncludesBeginOffset;
   if (!affectsRange(Ranges, IncludesBeginOffset, IncludesEndOffset))
     return;
   SmallVector<unsigned, 16> Indices;
Indices.erase(std::unique(Indices.begin(), Indices.end(),
                               return Includes[LHSI].Text == Includes[RHSI].Text;
                             }),
                 Indices.end());
 
   int CurrentCategory = Includes.front().Category;
 
   // If the #includes are out of order, we generate a single replacement fixing
   // the entire block. Otherwise, no replacement is generated.
+  // In case Style.IncldueStyle.IncludeBlocks != IBS_Preserve, this check is not
+  // enough as additional newlines might be added or removed across #include
+  // blocks. This we handle below by generating the updated #imclude blocks and
+  // comparing it to the original.
   if (Indices.size() == Includes.size() &&
       std::is_sorted(Indices.begin(), Indices.end()) &&
       Style.IncludeStyle.IncludeBlocks == tooling::IncludeStyle::IBS_Preserve)
     return;
 
   std::string result;
   for (unsigned Index : Indices) {
     if (!result.empty()) {
       result += "\n";
       if (Style.IncludeStyle.IncludeBlocks ==
               tooling::IncludeStyle::IBS_Regroup &&
           CurrentCategory != Includes[Index].Category)
         result += "\n";
     }
     result += Includes[Index].Text;
     if (Cursor && CursorIndex == Index)
       *Cursor = IncludesBeginOffset + result.size() - CursorToEOLOffset;
     CurrentCategory = Includes[Index].Category;
   }
 
+  // If the #includes are out of order, we generate a single replacement fixing
+  // the entire range of blocks. Otherwise, no replacement is generated.
+  if (result == Code.substr(IncludesBeginOffset, IncludesBlockSize))
+    return;
+
   auto Err = Replaces.add(tooling::Replacement(
       FileName, Includes.front().Offset, IncludesBlockSize, result));
   // FIXME: better error handling. For now, just skip the replacement for the
   // release version.
   if (Err) {
     llvm::errs() << llvm::toString(std::move(Err)) << "\n";
     assert(false);
   }
if (!FormattingOff && !Line.endswith("\\")) {
         StringRef IncludeName = Matches[2];
         int Category = Categories.getIncludePriority(
             IncludeName,
             /*CheckMainHeader=*/!MainIncludeFound && FirstIncludeBlock);
         if (Category == 0)
           MainIncludeFound = true;
         IncludesInBlock.push_back({IncludeName, Line, Prev, Category});
       } else if (!IncludesInBlock.empty() && !EmptyLineSkipped) {
-        sortCppIncludes(Style, IncludesInBlock, Ranges, FileName, Replaces,
-                        Cursor);
+        sortCppIncludes(Style, IncludesInBlock, Ranges, FileName, Code,
+                        Replaces, Cursor);
         IncludesInBlock.clear();
         FirstIncludeBlock = false;
       }
       Prev = Pos + 1;
     }
     if (Pos == StringRef::npos || Pos + 1 == Code.size())
       break;
     SearchFrom = Pos + 1;
   }
-  if (!IncludesInBlock.empty())
-    sortCppIncludes(Style, IncludesInBlock, Ranges, FileName, Replaces, Cursor);
+  if (!IncludesInBlock.empty()) {
+    sortCppIncludes(Style, IncludesInBlock, Ranges, FileName, Code, Replaces,
+                    Cursor);
+  }
   return Replaces;
 }
 
 // Returns group number to use as a first order sort on imports. Gives UINT_MAX
 // if the import does not match any given groups.
 static unsigned findJavaImportGroup(const FormatStyle &Style,
                                     StringRef ImportIdentifier) {
   unsigned LongestMatchIndex = UINT_MAX;

unittests/Format/SortIncludesTest.cpp

 //===- unittest/Format/SortIncludesTest.cpp - Include sort unit tests -----===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "FormatTestUtils.h"
 #include "clang/Format/Format.h"
+#include "llvm/ADT/None.h"
 #include "llvm/Support/Debug.h"
 #include "gtest/gtest.h"
 
 #define DEBUG_TYPE "format-test"
 
 namespace clang {
 namespace format {
 namespace {
 
 class SortIncludesTest : public ::testing::Test {
 protected:
   std::vector<tooling::Range> GetCodeRange(StringRef Code) {
     return std::vector<tooling::Range>(1, tooling::Range(0, Code.size()));
   }
 
   std::string sort(StringRef Code, std::vector<tooling::Range> Ranges,
-                   StringRef FileName = "input.cc") {
+                   StringRef FileName = "input.cc",
+                   unsigned ExpectedNumRanges = 1) {
     auto Replaces = sortIncludes(FmtStyle, Code, Ranges, FileName);
     Ranges = tooling::calculateRangesAfterReplacements(Replaces, Ranges);
+    EXPECT_EQ(ExpectedNumRanges, Replaces.size());
     auto Sorted = applyAllReplacements(Code, Replaces);
     EXPECT_TRUE(static_cast<bool>(Sorted));
     auto Result = applyAllReplacements(
         *Sorted, reformat(FmtStyle, *Sorted, Ranges, FileName));
     EXPECT_TRUE(static_cast<bool>(Result));
     return *Result;
   }
 
-  std::string sort(StringRef Code, StringRef FileName = "input.cpp") {
-    return sort(Code, GetCodeRange(Code), FileName);
+  std::string sort(StringRef Code,
+                   StringRef FileName = "input.cpp",
+                   unsigned ExpectedNumRanges = 1) {
+    return sort(Code, GetCodeRange(Code), FileName, ExpectedNumRanges);
   }
 
   unsigned newCursor(llvm::StringRef Code, unsigned Cursor) {
     sortIncludes(FmtStyle, Code, GetCodeRange(Code), "input.cpp", &Cursor);
     return Cursor;
   }
 
   FormatStyle FmtStyle = getLLVMStyle();
EXPECT_EQ("#include <a>\n"
             "/* clang-format onwards */\n",
             sort("#include <b>\n"
                  "#include <a>\n"
                  "#include <c>\n"
                  "/* clang-format offically */\n"
                  "#include <b>\n"
                  "#include <a>\n"
                  "#include <c>\n"
-                 "/* clang-format onwards */\n"));
+                 "/* clang-format onwards */\n", "input.h", 2));
 }
 
 TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) {
   FmtStyle.SortIncludes = false;
   EXPECT_EQ("#include \"a.h\"\n"
             "#include \"c.h\"\n"
             "#include \"b.h\"\n",
             sort("#include \"a.h\"\n"
                  "#include \"c.h\"\n"
-                 "#include \"b.h\"\n"));
+                 "#include \"b.h\"\n",
+                 "input.h", 0));
 }
 
 TEST_F(SortIncludesTest, MixIncludeAndImport) {
   EXPECT_EQ("#include \"a.h\"\n"
             "#import \"b.h\"\n"
             "#include \"c.h\"\n",
             sort("#include \"a.h\"\n"
                  "#include \"c.h\"\n"

 TEST_F(SortIncludesTest, SortsLocallyInEachBlock) {
   EXPECT_EQ("#include \"a.h\"\n"
             "#include \"c.h\"\n"
             "\n"
             "#include \"b.h\"\n",
             sort("#include \"a.h\"\n"
                  "#include \"c.h\"\n"
                  "\n"
-                 "#include \"b.h\"\n"));
+                 "#include \"b.h\"\n", "input.h", 0));
 }
 
 TEST_F(SortIncludesTest, SortsAllBlocksWhenMerging) {
   Style.IncludeBlocks = tooling::IncludeStyle::IBS_Merge;
   EXPECT_EQ("#include \"a.h\"\n"
             "#include \"b.h\"\n"
             "#include \"c.h\"\n",
             sort("#include \"a.h\"\n"
TEST_F(SortIncludesTest, NegativePriorities) {
 
   // check stable when re-run
   EXPECT_EQ("#include \"important_os_header.h\"\n"
             "#include \"c_main.h\"\n"
             "#include \"a_other.h\"\n",
             sort("#include \"important_os_header.h\"\n"
                  "#include \"c_main.h\"\n"
                  "#include \"a_other.h\"\n",
-                 "c_main.cc"));
+                 "c_main.cc", 0));
 }
 
 TEST_F(SortIncludesTest, PriorityGroupsAreSeparatedWhenRegroupping) {
   Style.IncludeCategories = {{".*important_os_header.*", -1}, {".*", 1}};
   Style.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
 
   EXPECT_EQ("#include \"important_os_header.h\"\n"
             "\n"
EXPECT_EQ("#include \"important_os_header.h\"\n"
             "#include \"c_main.h\"\n"
             "\n"
             "#include \"a_other.h\"\n",
             sort("#include \"important_os_header.h\"\n"
                  "\n"
                  "#include \"c_main.h\"\n"
                  "\n"
                  "#include \"a_other.h\"\n",
-                 "c_main.cc"));
+                 "c_main.cc", 0));
 }
 
 TEST_F(SortIncludesTest, CalculatesCorrectCursorPosition) {
   std::string Code = "#include <ccc>\n"    // Start of line: 0
                      "#include <bbbbbb>\n" // Start of line: 15
                      "#include <a>\n";     // Start of line: 33
   EXPECT_EQ(31u, newCursor(Code, 0));
   EXPECT_EQ(13u, newCursor(Code, 15));

 TEST_F(SortIncludesTest, DoNotSortLikelyXml) {
   EXPECT_EQ("<!--;\n"
             "#include <b>\n"
             "#include <a>\n"
             "-->",
             sort("<!--;\n"
                  "#include <b>\n"
                  "#include <a>\n"
-                 "-->"));
+                 "-->", "input.h", 0));
+}
+
+TEST_F(SortIncludesTest, DoNotOutputReplacementsForSortedBlocksWithRegrouping) {
+  Style.IncludeBlocks = Style.IBS_Regroup;
+  std::string Code = R"(
+#include "b.h"
+
+#include <a.h>
+)";
+  EXPECT_EQ(Code, sort(Code, "input.h", 0));
 }
 
 } // end namespace
 } // end namespace format
 } // end namespace clang