clang-format: [JS] Sort imported symbols.

[mprobst]

Summary

E.g. sort import {b, a} from 'x'; into import {a, b} from 'x';.

[mprobst]

• extract method for readability

[djasper]

In general, this is lacking test cases for imports that are wrapped over multiple lines to start with. Should probably add those both for the old and for the new behavior.

lib/Format/Format.cpp

1229–1230

I think it's not even worth storing this variable now.

lib/Format/SortJavaScriptImports.cpp

46

Maybe add a comment on why you are ignoring 'Range' for comparison.

195

nit: no braces

259

I think it's safe to pass the symbols as const reference here.

[mprobst]

Done regarding the tests. We do have an unrelated issue with formatting import {x as foo}, will send a fix later.

[mprobst]

• multiline tests, review comments

[mprobst]

Done regarding the tests. We do have an unrelated issue with formatting import {x as foo}, will send a fix later.

[djasper]

lib/Format/SortJavaScriptImports.cpp

274

It's an implementation detail that the iterator of a SmallVector is a JsImportedSymbol*. Use auto. Also, I find this more readable:

for (auto I = Symbols.begin(), E = Symbols.end(); I != E; ++I) {
  if (I != Symbols.begin())
    Buffer += ",";
  Buffer += getSourceText(I->Range);
}

lib/Format/Format.cpp

 #include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/YAMLTraits.h"
+#include <algorithm>
 #include <memory>
 #include <queue>
 #include <string>
 
 #define DEBUG_TYPE "format-formatter"
 
 using clang::format::FormatStyle;
 
static void sortCppIncludes(const FormatStyle &Style,
   std::stable_sort(
       Indices.begin(), Indices.end(), [&](unsigned LHSI, unsigned RHSI) {
         return std::tie(Includes[LHSI].Category, Includes[LHSI].Filename) <
                std::tie(Includes[RHSI].Category, Includes[RHSI].Filename);
       });
 
   // If the #includes are out of order, we generate a single replacement fixing
   // the entire block. Otherwise, no replacement is generated.
-  bool OutOfOrder = false;
-  for (unsigned i = 1, e = Indices.size(); i != e; ++i) {
-    if (Indices[i] != i) {
-      OutOfOrder = true;
-      break;
-    }
-  }
-  if (!OutOfOrder)
+  bool InOrder = std::is_sorted(Indices.begin(), Indices.end());
+  if (InOrder)
     return;
 
   std::string result;
   bool CursorMoved = false;
   for (unsigned Index : Indices) {
     if (!result.empty())
       result += "\n";
     result += Includes[Index].Text;

lib/Format/SortJavaScriptImports.cpp

 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Format/Format.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Debug.h"
+#include <algorithm>
 #include <string>
 
 #define DEBUG_TYPE "format-formatter"
 
 namespace clang {
 namespace format {
 
 class FormatTokenLexer;
 
 using clang::format::FormatStyle;
 
 // An imported symbol in a JavaScript ES6 import/export, possibly aliased.
 struct JsImportedSymbol {
   StringRef Symbol;
   StringRef Alias;
+  SourceRange Range;
+
+  bool operator==(const JsImportedSymbol &RHS) const {
+    return Symbol == RHS.Symbol && Alias == RHS.Alias;
+  }
 };
 
 // An ES6 module reference.
 //
 // ES6 implements a module system, where individual modules (~= source files)
 // can reference other modules, either importing symbols from them, or exporting
 // symbols from them:
 //   import {foo} from 'foo';
analyze(TokenAnnotator &Annotator,
 
     SmallVector<unsigned, 16> Indices;
     for (unsigned i = 0, e = References.size(); i != e; ++i)
       Indices.push_back(i);
     std::stable_sort(Indices.begin(), Indices.end(),
                      [&](unsigned LHSI, unsigned RHSI) {
                        return References[LHSI] < References[RHSI];
                      });
-    // FIXME: Pull this into a common function.
-    bool OutOfOrder = false;
-    for (unsigned i = 0, e = Indices.size(); i != e; ++i) {
-      if (i != Indices[i]) {
-        OutOfOrder = true;
-        break;
-      }
-    }
-    if (!OutOfOrder)
-      return Result;
+    bool ReferencesInOrder = std::is_sorted(Indices.begin(), Indices.end());
 
-    // Replace all existing import/export statements.
     std::string ReferencesText;
+    bool SymbolsInOrder = true;
     for (unsigned i = 0, e = Indices.size(); i != e; ++i) {
       JsModuleReference Reference = References[Indices[i]];
+      // 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;
+      std::stable_sort(Symbols.begin(), Symbols.end(),
+                       [&](JsImportedSymbol LHS, JsImportedSymbol RHS) {
+                         return LHS.Symbol < RHS.Symbol;
+                       });
+      if (Symbols == Reference.Symbols) {
+        // No change in symbol order.
         StringRef ReferenceStmt = getSourceText(Reference.Range);
         ReferencesText += ReferenceStmt;
+      } else {
+        // Stitch together the module reference start...
+        SymbolsInOrder = false;
+        SourceLocation SymbolsStart =
+            Reference.Symbols.front().Range.getBegin();
+        SourceLocation SymbolsEnd = Reference.Symbols.back().Range.getEnd();
+        ReferencesText +=
+            getSourceText(Reference.Range.getBegin(), SymbolsStart);
+        // ... then the references in order ...
+        for (JsImportedSymbol *I = Symbols.begin(), *E = Symbols.end();
+             I != E;) {
+          ReferencesText += getSourceText(I->Range);
+          if (++I != E)
+            ReferencesText += ",";
+        }
+        // ... followed by the module reference end.
+        ReferencesText += getSourceText(SymbolsEnd, Reference.Range.getEnd());
+      }
       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) {
+      return Result;
+    }
+
     // 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"
if (!Current || Current == LineEnd->Next) {
       // Set the current token to an invalid token, so that further parsing on
       // this line fails.
       invalidToken.Tok.setKind(tok::unknown);
       Current = &invalidToken;
     }
   }
 
   StringRef getSourceText(SourceRange Range) {
+    return getSourceText(Range.getBegin(), Range.getEnd());
+  }
+
+  StringRef getSourceText(SourceLocation Begin, SourceLocation End) {
     const SourceManager &SM = Env.getSourceManager();
-    return FileContents.substr(SM.getFileOffset(Range.getBegin()),
-                               SM.getFileOffset(Range.getEnd()) -
-                                   SM.getFileOffset(Range.getBegin()));
+    return FileContents.substr(SM.getFileOffset(Begin),
+                               SM.getFileOffset(End) - SM.getFileOffset(Begin));
   }
 
   // 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;
     bool FoundLines = false;
     AnnotatedLine *FirstNonImportLine = nullptr;
     for (auto Line : AnnotatedLines) {
       if (!Line->Affected) {
         // Only sort the first contiguous block of affected lines.
         if (FoundLines)
           break;
         else
           continue;
       }
       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)
         continue; // Only comments on this line.
       FoundLines = true;
       JsModuleReference Reference;
       Reference.Range.setBegin(Start);
bool parseNamedBindings(const AdditionalKeywords &Keywords,
     // {sym as alias, sym2 as ...} from '...';
     nextToken();
     while (true) {
       if (Current->isNot(tok::identifier))
         return false;
 
       JsImportedSymbol Symbol;
       Symbol.Symbol = Current->TokenText;
+      // Make sure to include any preceding comments.
+      Symbol.Range.setBegin(
+          Current->getPreviousNonComment()->Next->WhitespaceRange.getBegin());
       nextToken();
 
       if (Current->is(Keywords.kw_as)) {
         nextToken();
         if (Current->isNot(tok::identifier))
           return false;
         Symbol.Alias = Current->TokenText;
         nextToken();
       }
+      Symbol.Range.setEnd(Current->Tok.getLocation());
       Reference.Symbols.push_back(Symbol);
 
       if (Current->is(tok::r_brace))
         return true;
       if (Current->isNot(tok::comma))
         return false;
       nextToken();
     }

unittests/Format/SortImportsTestJS.cpp

 
 TEST_F(SortImportsTestJS, AliasesSymbols) {
   verifySort("import {sym1 as alias1} from 'b';\n"
              "import {sym2 as alias2, sym3 as alias3} from 'c';\n",
              "import {sym2 as alias2, sym3 as alias3} from 'c';\n"
              "import {sym1 as alias1} from 'b';\n");
 }
 
+TEST_F(SortImportsTestJS, SortSymbols) {
+  verifySort("import {sym1, sym2 as a, sym3} from 'b';\n",
+             "import {sym2 as a, sym1, sym3} from 'b';\n");
+  verifySort("import {sym1 /* important! */, /*!*/ sym2 as a} from 'b';\n",
+             "import {/*!*/ sym2 as a, sym1 /* important! */} from 'b';\n");
+}
+
 TEST_F(SortImportsTestJS, GroupImports) {
   verifySort("import {a} from 'absolute';\n"
              "\n"
              "import {b} from '../parent';\n"
              "import {b} from '../parent/nested';\n"
              "\n"
              "import {b} from './relative/path';\n"
              "import {b} from './relative/path/nested';\n"