clang-format: [JS] fix an assertion failure caused by shrinking sources.

[mprobst]

Summary

The JavaScript import sorter has a corner condition that can cause the overall
source text length to shrink. This change circumvents the issue by appending
trailing space in the line after the import blocks to match at least the
previous source code length.

This needs a better long term fix, but this fixes the immediate issue.

[djasper]

I think fixing the ranges should actually not be too hard. I'll take a look soon.

lib/Format/SortJavaScriptImports.cpp

182 ↗

(On Diff #59975)

nit: We call it FIXME.

[mprobst]

In D21108#451883, @djasper wrote:

I think fixing the ranges should actually not be too hard. I'll take a look soon.

Yeah, I don't think it's particularly hard, but I wasn't clear on where it'd be done best, and which APIs are public. I think it might make sense to extract a "sort imports, apply the changes, update these ranges, return the code" function altogether, but not sure. Currently we have the tests and the main method follow different code paths, which seems suboptimal.

[mprobst]

In D21108#451883, @djasper wrote:

I think fixing the ranges should actually not be too hard. I'll take a look soon.

Yeah, I don't think it's particularly hard, but I wasn't clear on where it'd be done best, and which APIs are public. I think it might make sense to extract a "sort imports, apply the changes, update these ranges, return the code" function altogether, but not sure. Currently we have the tests and the main method follow different code paths, which seems suboptimal.

cfe/trunk/lib/Format/SortJavaScriptImports.cpp

for (unsigned i = 0, e = Indices.size(); i != e; ++i) {
              Reference.Category != References[Indices[i + 1]].Category))
           ReferencesText += "\n";
       }
     }
 
     if (ReferencesInOrder && SymbolsInOrder)
       return Result;
 
+    SourceRange InsertionPoint = References[0].Range;
+    InsertionPoint.setEnd(References[References.size() - 1].Range.getEnd());
+
+    // The loop above might collapse previously existing line breaks between
+    // import blocks, and thus shrink the file. SortIncludes must not shrink
+    // overall source length as there is currently no re-calculation of ranges
+    // after applying source sorting.
+    // This loop just backfills trailing spaces after the imports, which are
+    // harmless and will be stripped by the subsequent formatting pass.
+    // TODO: A better long term fix is to re-calculate Ranges after sorting.
+    unsigned PreviousSize = getSourceText(InsertionPoint).size();
+    while (ReferencesText.size() < PreviousSize) {
+      ReferencesText += " ";
+    }
+
     // Separate references from the main code body of the file.
     if (FirstNonImportLine && FirstNonImportLine->First->NewlinesBefore < 2)
       ReferencesText += "\n";
 
-    SourceRange InsertionPoint = References[0].Range;
-    InsertionPoint.setEnd(References[References.size() - 1].Range.getEnd());
     DEBUG(llvm::dbgs() << "Replacing imports:\n"
                        << getSourceText(InsertionPoint) << "\nwith:\n"
                        << ReferencesText << "\n");
     Result.insert(tooling::Replacement(
         Env.getSourceManager(), CharSourceRange::getCharRange(InsertionPoint),
         ReferencesText));
 
     return Result;

cfe/trunk/unittests/Format/SortImportsTestJS.cpp

verifySort("import {sym} from 'c';\n"
              "import {sym} from 'c';\n"
              "import {sym} from 'b';\n"
              "import {sym} from 'a';\n"
              "import {sym} from 'c';\n"
              "let x = 1;",
              24, 30);
 }
 
+TEST_F(SortImportsTestJS, SortingCanShrink) {
+  // Sort excluding a suffix.
+  verifySort("import {B} from 'a';\n"
+             "import {A} from 'b';\n"
+             "\n"
+             "1;",
+             "import {A} from 'b';\n"
+             "\n"
+             "import {B} from 'a';\n"
+             "\n"
+             "1;");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang