clang-format: [JS] merge import lines.

[mprobst]

Summary

Multiple lines importing from the same URL can be merged:

import {X} from 'a';
import {Y} from 'a';

Merge to:

import {X, Y} from 'a';

This change implements this merge operation. It takes care not to merge in
various corner case situations (default imports, star imports).

[h-joo]

clang/lib/Format/SortJavaScriptImports.cpp

154

Would it be better if this were a named function?

164

nit : It?

clang/unittests/Format/SortImportsTestJS.cpp

333

I'm a bit confused on what this is trying to test. Could you explain?

[mprobst]

address review comments.

[mprobst]

clang/lib/Format/SortJavaScriptImports.cpp

92

FYI I restructured the algorithm a bit here.

clang/unittests/Format/SortImportsTestJS.cpp

333

This makes sure we don't collapse import * as foo imports with import {A} style imports.

[h-joo]

LGTM

[jgorbe]

Hi, one of our tests is failing because of this patch and it looks like an actual bug. clang-format now turns this file:

import './a';
import {bar} from './a';

into this:

barimport './a';

clang/lib/Format/SortJavaScriptImports.cpp

enum ReferenceCategory {
     RELATIVE,        // from './*'
   };
   ReferenceCategory Category = ReferenceCategory::SIDE_EFFECT;
   // The URL imported, e.g. `import .. from 'url';`. Empty for `export {a, b};`.
   StringRef URL;
   // Prefix from "import * as prefix". Empty for symbol imports and `export *`.
   // Implies an empty names list.
   StringRef Prefix;
+  // Default import from "import DefaultName from '...';".
+  StringRef DefaultImport;
   // Symbols from `import {SymbolA, SymbolB, ...} from ...;`.
   SmallVector<JsImportedSymbol, 1> Symbols;
+  // The source location just after { and just before } in the import.
+  // Extracted eagerly to allow modification of Symbols later on.
+  SourceLocation SymbolsStart, SymbolsEnd;
   // Textual position of the import/export, including preceding and trailing
   // comments.
   SourceRange Range;
 };
 
 bool operator<(const JsModuleReference &LHS, const JsModuleReference &RHS) {
   if (LHS.IsExport != RHS.IsExport)
     return LHS.IsExport < RHS.IsExport;
analyze(TokenAnnotator &Annotator,
     SmallVector<unsigned, 16> Indices;
     for (unsigned i = 0, e = References.size(); i != e; ++i)
       Indices.push_back(i);
     llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
       return References[LHSI] < References[RHSI];
     });
     bool ReferencesInOrder = llvm::is_sorted(Indices);
 
+    // Merge module references:
+    // After sorting, find all references that import named symbols from the
+    // same URL and merge their names. E.g.
+    //   import {X} from 'a';
+    //   import {Y} from 'a';
+    // should be rewritten to:
+    //   import {X, Y} from 'a';
+    // Note: this removes merged imports from Indices, but not from References.
+    // The loop below iterates Indices, so things work out.
+    JsModuleReference *PreviousReference = &References[Indices[0]];
+    auto *it = std::next(Indices.begin());
+    bool ImportsMerged = false;
+    while (it != std::end(Indices)) {
+      JsModuleReference *Reference = &References[*it];
+      // Skip:
+      //   import 'foo';
+      //   import * as foo from 'foo'; on either previous or this.
+      //   import Default from 'foo'; on either previous or this.
+      //   mismatching
+      if (Reference->Category == JsModuleReference::SIDE_EFFECT ||
+          !PreviousReference->Prefix.empty() || !Reference->Prefix.empty() ||
+          !PreviousReference->DefaultImport.empty() ||
+          !Reference->DefaultImport.empty() || Reference->Symbols.empty() ||
+          PreviousReference->URL != Reference->URL) {
+        PreviousReference = Reference;
+        ++it;
+        continue;
+      }
+      // Merge symbols from identical imports.
+      PreviousReference->Symbols.append(Reference->Symbols);
+      ImportsMerged = true;
+      // Remove the merged import.
+      it = Indices.erase(it);
+    }
+
     std::string ReferencesText;
     bool SymbolsInOrder = true;
     for (unsigned i = 0, e = Indices.size(); i != e; ++i) {
       JsModuleReference Reference = References[Indices[i]];
       if (appendReference(ReferencesText, Reference))
         SymbolsInOrder = false;
       if (i + 1 < e) {
         // Insert breaks between imports and exports.
         ReferencesText += "\n";
         // Separate imports groups with two line breaks, but keep all exports
         // in a single group.
         if (!Reference.IsExport &&
             (Reference.IsExport != References[Indices[i + 1]].IsExport ||
              Reference.Category != References[Indices[i + 1]].Category))
           ReferencesText += "\n";
       }
     }
 
-    if (ReferencesInOrder && SymbolsInOrder)
+    if (ReferencesInOrder && SymbolsInOrder && !ImportsMerged)
       return {Result, 0};
 
     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
private:
   }
 
   // Appends ``Reference`` to ``Buffer``, returning true if text within the
   // ``Reference`` changed (e.g. symbol order).
   bool appendReference(std::string &Buffer, JsModuleReference &Reference) {
     // Sort the individual symbols within the import.
     // E.g. `import {b, a} from 'x';` -> `import {a, b} from 'x';`
     SmallVector<JsImportedSymbol, 1> Symbols = Reference.Symbols;
-    llvm::stable_sort(
-        Symbols, [&](const JsImportedSymbol &LHS, const JsImportedSymbol &RHS) {
-          return LHS.Symbol.compare_lower(RHS.Symbol) < 0;
-        });
-    if (Symbols == Reference.Symbols) {
-      // No change in symbol order.
+    if (Symbols.empty()) {
+      // No symbol imports, just emit the entire module reference.
       StringRef ReferenceStmt = getSourceText(Reference.Range);
       Buffer += ReferenceStmt;
       return false;
     }
+    llvm::stable_sort(
+        Symbols, [&](const JsImportedSymbol &LHS, const JsImportedSymbol &RHS) {
+          return LHS.Symbol.compare_lower(RHS.Symbol) < 0;
+        });
     // Stitch together the module reference start...
-    SourceLocation SymbolsStart = Reference.Symbols.front().Range.getBegin();
-    SourceLocation SymbolsEnd = Reference.Symbols.back().Range.getEnd();
-    Buffer += getSourceText(Reference.Range.getBegin(), SymbolsStart);
+    Buffer += getSourceText(Reference.Range.getBegin(), Reference.SymbolsStart);
     // ... then the references in order ...
     for (auto I = Symbols.begin(), E = Symbols.end(); I != E; ++I) {
       if (I != Symbols.begin())
         Buffer += ",";
       Buffer += getSourceText(I->Range);
     }
     // ... followed by the module reference end.
-    Buffer += getSourceText(SymbolsEnd, Reference.Range.getEnd());
+    Buffer += getSourceText(Reference.SymbolsEnd, Reference.Range.getEnd());
     return true;
   }
 
   // Parses module references in the given lines. Returns the module references,
   // and a pointer to the first "main code" line if that is adjacent to the
   // affected lines of module references, nullptr otherwise.
   std::pair<SmallVector<JsModuleReference, 16>, AnnotatedLine *>
   parseModuleReferences(const AdditionalKeywords &Keywords,
                         SmallVectorImpl<AnnotatedLine *> &AnnotatedLines) {
     SmallVector<JsModuleReference, 16> References;
     SourceLocation Start;
     AnnotatedLine *FirstNonImportLine = nullptr;
     bool AnyImportAffected = false;
-    for (auto Line : AnnotatedLines) {
+    for (auto *Line : AnnotatedLines) {
       Current = Line->First;
       LineEnd = Line->Last;
       skipComments();
       if (Start.isInvalid() || References.empty())
         // After the first file level comment, consider line comments to be part
         // of the import that immediately follows them by using the previously
         // set Start.
         Start = Line->First->Tok.getLocation();
if (Current->isNot(tok::identifier))
       return false;
     Reference.Prefix = Current->TokenText;
     nextToken();
     return true;
   }
 
   bool parseNamedBindings(const AdditionalKeywords &Keywords,
                           JsModuleReference &Reference) {
+    // eat a potential "import X, " prefix.
     if (Current->is(tok::identifier)) {
+      Reference.DefaultImport = Current->TokenText;
       nextToken();
       if (Current->is(Keywords.kw_from))
         return true;
       if (Current->isNot(tok::comma))
         return false;
       nextToken(); // eat comma.
     }
     if (Current->isNot(tok::l_brace))
       return false;
 
     // {sym as alias, sym2 as ...} from '...';
+    Reference.SymbolsStart = Current->Tok.getEndLoc();
     while (Current->isNot(tok::r_brace)) {
       nextToken();
       if (Current->is(tok::r_brace))
         break;
       if (!Current->isOneOf(tok::identifier, tok::kw_default))
         return false;
 
       JsImportedSymbol Symbol;
while (Current->isNot(tok::r_brace)) {
         nextToken();
       }
       Symbol.Range.setEnd(Current->Tok.getLocation());
       Reference.Symbols.push_back(Symbol);
 
       if (!Current->isOneOf(tok::r_brace, tok::comma))
         return false;
     }
+    Reference.SymbolsEnd = Current->Tok.getLocation();
+    // For named imports with a trailing comma ("import {X,}"), consider the
+    // comma to be the end of the import list, so that it doesn't get removed.
+    if (Current->Previous->is(tok::comma))
+      Reference.SymbolsEnd = Current->Previous->Tok.getLocation();
     nextToken(); // consume r_brace
     return true;
   }
 };
 
 tooling::Replacements sortJavaScriptImports(const FormatStyle &Style,
                                             StringRef Code,
                                             ArrayRef<tooling::Range> Ranges,

clang/unittests/Format/SortImportsTestJS.cpp

 TEST_F(SortImportsTestJS, SortDefaultImports) {
   // Reproduces issue where multi-line import was not parsed correctly.
   verifySort("import {A} from 'a';\n"
              "import {default as B} from 'b';\n",
              "import {default as B} from 'b';\n"
              "import {A} from 'a';\n");
 }
 
+TEST_F(SortImportsTestJS, MergeImports) {
+  // basic operation
+  verifySort("import {X, Y} from 'a';\n"
+             "import {Z} from 'z';\n"
+             "\n"
+             "X + Y + Z;\n",
+             "import {X} from 'a';\n"
+             "import {Z} from 'z';\n"
+             "import {Y} from 'a';\n"
+             "\n"
+             "X + Y + Z;\n");
+
+  // empty imports
+  verifySort("import {A} from 'foo';\n", "import {} from 'foo';\n"
+                                         "import {A} from 'foo';");
+
+  // ignores import *
+  verifySort("import * as foo from 'foo';\n"
+             "import {A} from 'foo';\n",
+             "import   * as foo from 'foo';\n"
+             "import {A} from 'foo';\n");
+
+  // ignores default import
+  verifySort("import X from 'foo';\n"
+             "import {A} from 'foo';\n",
+             "import    X from 'foo';\n"
+             "import {A} from 'foo';\n");
+
+  // keeps comments
+  // known issue: loses the 'also a' comment.
+  verifySort("// a\n"
+             "import {/* x */ X, /* y */ Y} from 'a';\n"
+             "// z\n"
+             "import {Z} from 'z';\n"
+             "\n"
+             "X + Y + Z;\n",
+             "// a\n"
+             "import {/* y */ Y} from 'a';\n"
+             "// z\n"
+             "import {Z} from 'z';\n"
+             "// also a\n"
+             "import {/* x */ X} from 'a';\n"
+             "\n"
+             "X + Y + Z;\n");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang