[clang-format] Do not emit replacements if Java imports are OK

[krasimir]

Summary

Currently clang-format would always emit a replacement for a block of Java imports even if it is correctly formatted:

% cat /tmp/Aggregator.java
import X;
% clang-format /tmp/Aggregator.java
import X;
% clang-format -output-replacements-xml /tmp/Aggregator.java
<?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
<replacement offset='0' length='9'>import X;</replacement>
</replacements>
%

This change makes clang-format not emit replacements in this case. Note that
there is logic to not emit replacements in this case for C++.

cfe/trunk/lib/Format/Format.cpp

 // Sorts and deduplicates a block of includes given by 'Imports' based on
 // JavaImportGroups, then adding the necessary replacement to 'Replaces'.
 // Import declarations with the same text will be deduplicated. Between each
 // import group, a newline is inserted, and within each import group, a
 // lexicographic sort based on ASCII value is performed.
 static void sortJavaImports(const FormatStyle &Style,
                             const SmallVectorImpl<JavaImportDirective> &Imports,
                             ArrayRef<tooling::Range> Ranges, StringRef FileName,
+                            StringRef Code,
                             tooling::Replacements &Replaces) {
   unsigned ImportsBeginOffset = Imports.front().Offset;
   unsigned ImportsEndOffset =
       Imports.back().Offset + Imports.back().Text.size();
   unsigned ImportsBlockSize = ImportsEndOffset - ImportsBeginOffset;
   if (!affectsRange(Ranges, ImportsBeginOffset, ImportsEndOffset))
     return;
   SmallVector<unsigned, 16> Indices;
   SmallVector<unsigned, 16> JavaImportGroups;
   for (unsigned i = 0, e = Imports.size(); i != e; ++i) {
     Indices.push_back(i);
     JavaImportGroups.push_back(
         findJavaImportGroup(Style, Imports[i].Identifier));
   }
   llvm::sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
-        // Negating IsStatic to push static imports above non-static imports.
-        return std::make_tuple(!Imports[LHSI].IsStatic, JavaImportGroups[LHSI],
-                               Imports[LHSI].Identifier) <
-               std::make_tuple(!Imports[RHSI].IsStatic, JavaImportGroups[RHSI],
-                               Imports[RHSI].Identifier);
-      });
 
   // Deduplicate imports.
   Indices.erase(std::unique(Indices.begin(), Indices.end(),
                             [&](unsigned LHSI, unsigned RHSI) {
                               return Imports[LHSI].Text == Imports[RHSI].Text;
                             }),
                 Indices.end());
 
for (StringRef CommentLine : Imports[Index].AssociatedCommentLines) {
       result += CommentLine;
       result += "\n";
     }
     result += Imports[Index].Text;
     CurrentIsStatic = Imports[Index].IsStatic;
     CurrentImportGroup = JavaImportGroups[Index];
   }
 
+  // If the imports are out of order, we generate a single replacement fixing
+  // the entire block. Otherwise, no replacement is generated.
+  if (result == Code.substr(Imports.front().Offset, ImportsBlockSize))
+    return;
+
   auto Err = Replaces.add(tooling::Replacement(FileName, Imports.front().Offset,
                                                ImportsBlockSize, 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 (ImportRegex.match(Line, &Matches)) {
       AssociatedCommentLines.push_back(Line);
     }
     Prev = Pos + 1;
     if (Pos == StringRef::npos || Pos + 1 == Code.size())
       break;
     SearchFrom = Pos + 1;
   }
   if (!ImportsInBlock.empty())
-    sortJavaImports(Style, ImportsInBlock, Ranges, FileName, Replaces);
+    sortJavaImports(Style, ImportsInBlock, Ranges, FileName, Code, Replaces);
   return Replaces;
 }
 
 bool isMpegTS(StringRef Code) {
   // MPEG transport streams use the ".ts" file extension. clang-format should
   // not attempt to format those. MPEG TS' frame format starts with 0x47 every
   // 189 bytes - detect that and return.
   return Code.size() > 188 && Code[0] == 0x47 && Code[188] == 0x47;

cfe/trunk/unittests/Format/SortImportsTestJava.cpp

 
 TEST_F(SortImportsTestJava, NoNewlineAtEnd) {
   EXPECT_EQ("import org.a;\n"
             "import org.b;",
             sort("import org.b;\n"
                  "import org.a;"));
 }
 
+TEST_F(SortImportsTestJava, NoReplacementsForValidImports) {
+  // Identical #includes have led to a failure with an unstable sort.
+  std::string Code = "import org.a;\n"
+                     "import org.b;\n";
+  EXPECT_TRUE(
+      sortIncludes(FmtStyle, Code, GetCodeRange(Code), "input.java").empty());
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang