clang-format: [JS] sort ES6 imports.

[mprobst]

Summary

This change automatically sorts ES6 imports and exports into four groups:
absolute imports, parent imports, relative imports, and then exports. Exports
are sorted in the same order, but not grouped further.

[djasper]

Just two high-level comments. Will review in more depth later.

include/clang/Format/Format.h

770

Why is this a separate function as opposed to looking at Style.Language?

lib/Format/Format.cpp

1958

I know this file is already a huge mess, but can you move all of the JS import sorting stuff to a separate file?

[mprobst]

• extract TokenAnalyzer.h and SortJavaScriptImports.h/cpp
• clean up imports

[mprobst]

• extract TokenAnalyzer.h and SortJavaScriptImports.h/cpp
• clean up imports/
• includes

[djasper]

lib/Format/Format.cpp

20

Use clang-format to fix the order :-)

lib/Format/FormatTokenLexer.h

28 ↗

(On Diff #57039)

I believe it is a bit harmful to have that much code in a header. It now gets pulled into multiple translation units and is thus compiled multiple times only for the linker to deduplicate it afterwards. We should move the implementation in FormatTokenLexer.cpp. I am happy to do that as a follow up, though.

lib/Format/SortJavaScriptImports.cpp

54 ↗

(On Diff #57039)

This comment doesn't really explain what it is and it is not obvious to me.

112 ↗

(On Diff #57039)

No braces.

150 ↗

(On Diff #57039)

Add:

// FIXME: Pull this into a common function.

162 ↗

(On Diff #57039)

Is there any chance you are changing the total length of an import block?

226 ↗

(On Diff #57039)

No braces.

291 ↗

(On Diff #57039)

s/TODO(martinprobst)/FIXME/

Also, I'd probably remove the parameter until this is supported. This FIXME is quite hidden but if there is no Cursor in the interface, there is no confusion.

lib/Format/TokenAnalyzer.h

1 ↗

(On Diff #57039)

This isn't quite as bad as the other, but we probably want a dedicated .cpp file, too.

[mprobst]

• extract TokenAnalyzer.h and SortJavaScriptImports.h/cpp
• clean up imports/
• includes
• address review comments
• pull out implementations from header files.

[mprobst]

PTAL.

lib/Format/SortJavaScriptImports.cpp

162 ↗

(On Diff #57039)

No, this only ever monotonously increases the length. I'm aware things fall apart if it doesn't ;-)

226 ↗

(On Diff #57039)

Just can't get used to that, it feels so wrong...

[mprobst]

• extract TokenAnalyzer.h and SortJavaScriptImports.h/cpp
• clean up imports/
• includes
• address review comments
• pull out implementations from header files.
• support side effect imports, keep in relative order at top.

[djasper]

I think, this looks good.

Manuel: Are you fine with this, design-wise?

lib/Format/SortJavaScriptImports.cpp

77 ↗

(On Diff #57138)

NB?

255 ↗

(On Diff #57138)

We probably should really have a variant of AnnotatedLine::startsWith() that can start at an arbitrary token. But we can try that in a follow-up.

[mprobst]

• extract TokenAnalyzer.h and SortJavaScriptImports.h/cpp
• clean up imports/
• includes
• address review comments
• pull out implementations from header files.
• support side effect imports, keep in relative order at top.
• Move getLanguageName to Format.h

[mprobst]

lib/Format/SortJavaScriptImports.cpp

77 ↗

(On Diff #57138)

"Nota bene". Seems to be uncommon in this code base, so removed.

255 ↗

(On Diff #57138)

Maybe that should be a function on FormatToken itself, that searches the linked list rooted at it?

Current->startsSequence(tok::star, Keywords.kw_as, tok::identifier)?

I'll take a stab at that.

[mprobst]

• review comments

[klimek]

include/clang/Format/Format.h

863

s/for debugging/.
Or do you want to claim that we'll arbitrarily change the strings here?

lib/Format/Format.cpp

2285–2293

I'd make this symmetric by pulling out 2 functions instead of falling through into the function.

lib/Format/SortJavaScriptImports.cpp

45 ↗

(On Diff #57440)

As not all of the members will be default initialized I'd prefer to have constructors.

45–46 ↗

(On Diff #57440)

This is really weird - a class called ImportExport that then has a bool IsExport. Please explain in a comment why you're doing that instead of any of:
a) managing a set of imports and exports on their own
b) calling this something else
c) having a class hierarchy

115 ↗

(On Diff #57440)

When I see ImpExp I think "ImplicitExpression". I'd name this ImportExport.
Also, it seems like we now have an uninitialized bool and enum in the struct.

163 ↗

(On Diff #57440)

If that's the case, please add a comment to that end.

207 ↗

(On Diff #57440)

Usually we call Lexer::getSourceText for that (don't we have a Lexer?)

252–253 ↗

(On Diff #57440)

This function is really long - can we perhaps break out smaller syntactic options.

258 ↗

(On Diff #57440)

Please don't put side-effects into an if (). This might be generally easier to read if nextToken() can't fail, and instead creates an invalid token.

[mprobst]

• address review comments

[mprobst]

PTAL.

lib/Format/SortJavaScriptImports.cpp

45–46 ↗

(On Diff #57440)

Discussed offline. I stand by this code. Yes, it has an odd boolean flag, but imports and exports in ES6 are essentially the same syntactical construct, with exports being syntactic sugar for import followed by export.

I've updated the comment on this struct substantially to explain that (I can tell how it's non-obvious).

Note that the boolean is never dispatched on in this code, except for the "empty line before exports" part. Exports are the same as imports wrt sorting them here.

115 ↗

(On Diff #57440)

Now called Reference.

207 ↗

(On Diff #57440)

We don't, FormatTokenLexer doesn't expose the Lexer, and TokenAnalyzer doesn't export FormatTokenLexer.

258 ↗

(On Diff #57440)

Done, much cleaner indeed. LLVM style guide says not to introduce new static globals with constructors, so I'm just using a field. Please advise if there's a better way.

[klimek]

lib/Format/SortJavaScriptImports.cpp

46–47 ↗

(On Diff #57497)

Thanks, this is much better.

84–85 ↗

(On Diff #57497)

Any reason you're not using a SourceRange?

93–96 ↗

(On Diff #57497)

Doesn't that break strict weak ordering requirements?

x = non-side-effect (url: a)
y = side-effect (url: b)
z = non-side-effect (url: c)
now x and y are incomparable:
!(x < y) because x.url < y.url
&& !(y < x) because of y == side-effect
but x and z are comparable:
x < z
breaks transitivity of incomparability (https://en.wikipedia.org/wiki/Weak_ordering#Strict_weak_orderings)

To keep them equal, wouldn't you need:

if (LHS == SIDE_EFFECT && RHS == SIDE_EFFECT)
  return false;

(that would at least seem symmetric)

128 ↗

(On Diff #57497)

It seems like you can pull the loop below into its own function parseModuleReferences or something.

129 ↗

(On Diff #57497)

LastStart is just the start of the currently processed module reference, right? I'd just call it Start then.

135 ↗

(On Diff #57497)

Why are you defining this here, far before its use?

I think this code might be slightly simpler (for me to understand) if you just pulled Reference out of the loop, and got rid of LastStart.

[mprobst]

PTAL.

lib/Format/SortJavaScriptImports.cpp

93–96 ↗

(On Diff #57497)

As discussed offline, the comparison before makes sure that RHS == SIDE_EFFECT here.

Added a comment.

135 ↗

(On Diff #57497)

As discussed offline, moved the declaration a bit down.

I think exposing just a SourceLocation Start makes the algorithm easier to read, otherwise it'd look like a whole JsModuleReference could leave the loop, which is never the case. It explicitly points out we only have state outside of the loop to track the start.

[mprobst]

• address review comments
• - extract parseModuleReferences

[mprobst]

PTAL.

[klimek]

We're getting there. Couple of nits left.

lib/Format/SortJavaScriptImports.cpp

94–97 ↗

(On Diff #57621)

Yea, completely missed that the != above.

128 ↗

(On Diff #57621)

Return by value.

216–217 ↗

(On Diff #57621)

Both of these are used only once, perhaps inline?

229 ↗

(On Diff #57621)

I'd put that down after References.push_back so calculating the Reference is at least a single flow.

[mprobst]

lib/Format/SortJavaScriptImports.cpp

216–217 ↗

(On Diff #57621)

These are setting fields for the parse state (note: this is not a variable declaration).

[mprobst]

• address review comments
• - extract parseModuleReferences
• more review comments

[klimek]

lg

[mprobst]

• correctly insert breaks after import block

[mprobst]

• ranges

[klimek]

lib/Format/SortJavaScriptImports.cpp

160 ↗

(On Diff #57795)

Between categories of imports and imports and exports, right?

170–171 ↗

(On Diff #57795)

Shouldn't we add 2 if NewlinesBefore is 0? Or is that syntactically impossible?

[mprobst]

Thanks for the review, appreciated!

lib/Format/SortJavaScriptImports.cpp

160 ↗

(On Diff #57795)

Added a comment.

170–171 ↗

(On Diff #57795)

The next token is on a new line, so it gets a wrap just from that already. I've added a test though.

include/clang/Format/Format.h

 
 /// \brief Returns the replacements necessary to sort all ``#include`` blocks
 /// that are affected by ``Ranges``.
 tooling::Replacements sortIncludes(const FormatStyle &Style, StringRef Code,
                                    ArrayRef<tooling::Range> Ranges,
                                    StringRef FileName,
                                    unsigned *Cursor = nullptr);
 
+/// \brief Returns the replacements necessary to sort all ``import`` and
+/// ``export`` blocks are affected by ``Ranges``.
+tooling::Replacements sortJavaScriptIncludes(const FormatStyle &Style,
+                                             StringRef Code,
+                                             ArrayRef<tooling::Range> Ranges,
+                                             StringRef FileName,
+                                             unsigned *Cursor = nullptr);
+
 /// \brief Returns the replacements corresponding to applying and formatting
 /// \p Replaces.
 tooling::Replacements formatReplacements(StringRef Code,
                                          const tooling::Replacements &Replaces,
                                          const FormatStyle &Style);
 
 /// \brief Returns the replacements corresponding to applying \p Replaces and
 /// cleaning up the code after that.

 /// \param[in] FS The underlying file system, in which the file resides. By
 /// default, the file system is the real file system.
 ///
 /// \returns FormatStyle as specified by ``StyleName``. If no style could be
 /// determined, the default is LLVM Style (see ``getLLVMStyle()``).
 FormatStyle getStyle(StringRef StyleName, StringRef FileName,
                      StringRef FallbackStyle, vfs::FileSystem *FS = nullptr);
 
 } // end namespace format
 } // end namespace clang
 
 namespace std {
 template <>
 struct is_error_code_enum<clang::format::ParseError> : std::true_type {};
 }
 
 #endif // LLVM_CLANG_FORMAT_FORMAT_H

lib/Format/Format.cpp

 /// split into separate files as we go.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "clang/Format/Format.h"
 #include "AffectedRangeManager.h"
 #include "ContinuationIndenter.h"
 #include "TokenAnnotator.h"
 #include "UnwrappedLineFormatter.h"
 #include "UnwrappedLineParser.h"
 #include "WhitespaceManager.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/STLExtras.h"
private:
   };
 
   // Tokens to be deleted.
   std::set<FormatToken *, FormatTokenLess> DeletedTokens;
   // The line numbers of lines to be deleted.
   std::set<unsigned> DeletedLines;
 };
 
+// An imported symbol in a JavaScript ES6 import/export, possibly aliased.
+struct JsImportedSymbol {
+  StringRef Symbol;
+  StringRef Alias;
+};
+
+struct JsImportExport {
+  bool IsExport;
+  // JS imports are sorted into these categories, in order.
+  enum JsImportCategory {
+    ABSOLUTE,        // from 'something'
+    RELATIVE_PARENT, // from '../*'
+    RELATIVE,        // from './*'
+  };
+  JsImportCategory Category;
+  // Empty for `export {a, b};`.
+  StringRef URL;
+  // Prefix from "import * as prefix". Empty for symbol imports. Implies an
+  // empty names list.
+  StringRef Prefix;
+  // Symbols from `import {SymbolA, SymbolB, ...} from ...;`.
+  SmallVector<JsImportedSymbol, 1> Symbols;
+  // Textual position of the import/export, including preceding and trailing
+  // comments.
+  SourceLocation Start;
+  SourceLocation End;
+};
+
+bool operator<(const JsImportExport &LHS, const JsImportExport &RHS) {
+  if (LHS.IsExport != RHS.IsExport)
+    return LHS.IsExport < RHS.IsExport;
+  if (LHS.Category != RHS.Category)
+    return LHS.Category < RHS.Category;
+  // NB: empty URLs sort *last* (for export {...};).
+  if (LHS.URL.empty() != RHS.URL.empty())
+    return LHS.URL.empty() < RHS.URL.empty();
+  if (LHS.URL != RHS.URL)
+    return LHS.URL < RHS.URL;
+  // NB: '*' imports (with prefix) sort before {a, b, ...} imports.
+  if (LHS.Prefix.empty() != RHS.Prefix.empty())
+    return LHS.Prefix.empty() < RHS.Prefix.empty();
+  if (LHS.Prefix != RHS.Prefix)
+    return LHS.Prefix > RHS.Prefix;
+  return false;
+}
+
+// JavaScriptImportSorter sorts JavaScript ES6 imports and exports. It is
+// implemented as a TokenAnalyzer because ES6 imports have substantial syntactic
+// structure, making it messy to sort them using regular expressions.
+class JavaScriptImportSorter : public TokenAnalyzer {
+public:
+  JavaScriptImportSorter(const Environment &Env, const FormatStyle &Style)
+      : TokenAnalyzer(Env, Style),
+        FileContents(Env.getSourceManager().getBufferData(Env.getFileID())) {}
+
+  tooling::Replacements
+  analyze(TokenAnnotator &Annotator,
+          SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
+          FormatTokenLexer &Tokens, tooling::Replacements &Result) override {
+    AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(),
+                                          AnnotatedLines.end());
+
+    const AdditionalKeywords &Keywords = Tokens.getKeywords();
+
+    SmallVector<JsImportExport, 16> Imports;
+    SourceLocation LastStart;
+    for (auto Line : AnnotatedLines) {
+      if (!Line->Affected)
+        break;
+      Current = Line->First;
+      LineEnd = Line->Last;
+      JsImportExport ImpExp;
+      skipComments();
+      if (LastStart.isInvalid() || Imports.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 LastStart.
+        LastStart = Line->First->Tok.getLocation();
+      }
+      if (!Current)
+        continue;  // Only comments on this line.
+      ImpExp.Start = LastStart;
+      LastStart = SourceLocation();
+      if (!parseImportExport(Keywords, ImpExp))
+        break;
+      ImpExp.End = LineEnd->Tok.getEndLoc();
+      DEBUG({
+        llvm::dbgs() << "Import: {"
+                     << "is_export: " << ImpExp.IsExport
+                     << ", cat: " << ImpExp.Category
+                     << ", url: " << ImpExp.URL
+                     << ", prefix: " << ImpExp.Prefix;
+        for (size_t i = 0; i < ImpExp.Symbols.size(); ++i)
+          llvm::dbgs() << ", " << ImpExp.Symbols[i].Symbol << " as "
+                       << ImpExp.Symbols[i].Alias;
+        llvm::dbgs() << ", text: " << getSourceText(ImpExp.Start, ImpExp.End);
+        llvm::dbgs() << "}\n";
+      });
+      Imports.push_back(ImpExp);
+    }
+
+    if (Imports.empty())
+      return Result;
+
+    SmallVector<unsigned, 16> Indices;
+    for (unsigned i = 0, e = Imports.size(); i != e; ++i)
+      Indices.push_back(i);
+    std::stable_sort(
+        Indices.begin(), Indices.end(), [&](unsigned LHSI, unsigned RHSI) {
+          return Imports[LHSI] < Imports[RHSI];
+        });
+
+    bool OutOfOrder = false;
+    for (unsigned i = 0, e = Indices.size(); i != e; ++i) {
+      if (i != Indices[i]) {
+        OutOfOrder = true;
+        break;
+      }
+    }
+    if (!OutOfOrder)
+      return Result;
+
+    // Replace all existing import/export statements.
+    std::string ImportsText;
+    for (unsigned i = 0, e = Indices.size(); i != e; ++i) {
+      JsImportExport ImpExp = Imports[Indices[i]];
+      StringRef ImportStmt = getSourceText(ImpExp.Start, ImpExp.End);
+      ImportsText += ImportStmt;
+      ImportsText += "\n";
+      // Separate groups outside of exports with two line breaks.
+      if (i + 1 < e && !ImpExp.IsExport &&
+          ImpExp.Category != Imports[Indices[i + 1]].Category)
+        ImportsText += "\n";
+    }
+    SourceLocation InsertionPoint = Imports[0].Start;
+    SourceLocation End = Imports[Imports.size() - 1].End;
+    DEBUG(llvm::dbgs() << "Replacing imports:\n"
+                       << getSourceText(InsertionPoint, End) << "\nwith:\n"
+                       << ImportsText);
+    Result.insert(tooling::Replacement(
+        Env.getSourceManager(),
+        CharSourceRange::getCharRange(InsertionPoint, End), ImportsText));
+
+    return Result;
+  }
+
+private:
+  FormatToken *Current;
+  FormatToken *LineEnd;
+  StringRef FileContents;
+
+  void skipComments() {
+    Current = skipComments(Current);
+  }
+
+  FormatToken *skipComments(FormatToken *Tok) {
+    while (Tok && Tok->is(tok::comment))
+      Tok = Tok->Next;
+    return Tok;
+  }
+
+  bool nextToken() {
+    Current = Current->Next;
+    skipComments();
+    return Current && Current != LineEnd->Next;
+  }
+
+  StringRef getSourceText(SourceLocation Start, SourceLocation End) {
+    const SourceManager &SM = Env.getSourceManager();
+    return FileContents.substr(SM.getFileOffset(Start),
+                               SM.getFileOffset(End) - SM.getFileOffset(Start));
+  }
+
+  bool parseImportExport(const AdditionalKeywords &Keywords,
+                         JsImportExport &ImpExp) {
+    if (!Current || !Current->isOneOf(Keywords.kw_import, tok::kw_export))
+      return false;
+    ImpExp.IsExport = Current->is(tok::kw_export);
+    nextToken();
+
+    if (!parseImportExportSpecifier(Keywords, ImpExp) || !nextToken())
+      return false;
+    if (Current->is(Keywords.kw_from)) {
+      // imports have a 'from' clause, exports might not.
+      if (!nextToken())
+        return false;
+      if (!Current->isStringLiteral())
+        return false;
+      // URL = TokenText without the quotes.
+      ImpExp.URL = Current->TokenText.substr(1, Current->TokenText.size() - 2);
+      if (ImpExp.URL.startswith("..")) {
+        ImpExp.Category = JsImportExport::JsImportCategory::RELATIVE_PARENT;
+      } else if (ImpExp.URL.startswith(".")) {
+        ImpExp.Category = JsImportExport::JsImportCategory::RELATIVE;
+      } else {
+        ImpExp.Category = JsImportExport::JsImportCategory::ABSOLUTE;
+      }
+    } else {
+      // w/o URL groups with "empty".
+      ImpExp.Category = JsImportExport::JsImportCategory::RELATIVE;
+    }
+    return true;
+  }
+
+  bool parseImportExportSpecifier(const AdditionalKeywords &Keywords,
+                                  JsImportExport &ImpExp) {
+    // * as prefix from '...';
+    if (Current->is(tok::star)) {
+      if (!nextToken())
+        return false;
+      if (!Current->is(Keywords.kw_as) || !nextToken())
+        return false;
+      if (!Current->is(tok::identifier))
+        return false;
+      ImpExp.Prefix = Current->TokenText;
+      return true;
+    }
+
+    if (!Current->is(tok::l_brace))
+      return false;
+
+    // {sym as alias, sym2 as ...} from '...';
+    if (!nextToken())
+      return false;
+    while (true) {
+      if (!Current->is(tok::identifier))
+        return false;
+
+      JsImportedSymbol Symbol;
+      Symbol.Symbol = Current->TokenText;
+      nextToken();
+
+      if (Current->is(Keywords.kw_as)) {
+        nextToken();
+        if (!Current->is(tok::identifier))
+          return false;
+        Symbol.Alias = Current->TokenText;
+        nextToken();
+      }
+      ImpExp.Symbols.push_back(Symbol);
+
+      if (Current->is(tok::r_brace))
+        return true;
+      if (!Current->is(tok::comma))
+        return false;
+      nextToken();
+    }
+  }
+};
+
 struct IncludeDirective {
   StringRef Filename;
   StringRef Text;
   unsigned Offset;
   int Category;
 };
 
 } // end anonymous namespace
static void sortIncludes(const FormatStyle &Style,
   assert(result.size() ==
          Includes.back().Offset + Includes.back().Text.size() -
              Includes.front().Offset);
 
   Replaces.insert(tooling::Replacement(FileName, Includes.front().Offset,
                                        result.size(), result));
 }
 
 tooling::Replacements sortIncludes(const FormatStyle &Style, StringRef Code,
                                    ArrayRef<tooling::Range> Ranges,
                                    StringRef FileName, unsigned *Cursor) {
   tooling::Replacements Replaces;
   if (!Style.SortIncludes)
     return Replaces;
+  if (Style.Language == FormatStyle::LanguageKind::LK_JavaScript)
+    return sortJavaScriptIncludes(Style, Code, Ranges, FileName);
 
   unsigned Prev = 0;
   unsigned SearchFrom = 0;
   llvm::Regex IncludeRegex(
       R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))");
   SmallVector<StringRef, 4> Matches;
   SmallVector<IncludeDirective, 16> IncludesInBlock;
 
   // In compiled files, consider the first #include to be the main #include of
if (Pos == StringRef::npos || Pos + 1 == Code.size())
       break;
     SearchFrom = Pos + 1;
   }
   if (!IncludesInBlock.empty())
     sortIncludes(Style, IncludesInBlock, Ranges, FileName, Replaces, Cursor);
   return Replaces;
 }
 
+// Sorts a block of includes given by 'Includes' alphabetically adding the
+// necessary replacement to 'Replaces'. 'Includes' must be in strict source
+// order.
+tooling::Replacements sortJavaScriptIncludes(const FormatStyle &Style,
+                                             StringRef Code,
+                                             ArrayRef<tooling::Range> Ranges,
+                                             StringRef FileName,
+                                             unsigned *Cursor) {
+  std::unique_ptr<Environment> Env =
+      Environment::CreateVirtualEnvironment(Code, FileName, Ranges);
+  JavaScriptImportSorter Sorter(*Env, Style);
+  return Sorter.process();
+}
+
 template <typename T>
 static tooling::Replacements
 processReplacements(T ProcessFunc, StringRef Code,
                     const tooling::Replacements &Replaces,
                     const FormatStyle &Style) {
   if (Replaces.empty())
     return tooling::Replacements();
 

lib/Format/FormatToken.h

AdditionalKeywords(IdentifierTable &IdentTable) {
     kw_override = &IdentTable.get("override");
     kw_in = &IdentTable.get("in");
     kw_of = &IdentTable.get("of");
     kw_CF_ENUM = &IdentTable.get("CF_ENUM");
     kw_CF_OPTIONS = &IdentTable.get("CF_OPTIONS");
     kw_NS_ENUM = &IdentTable.get("NS_ENUM");
     kw_NS_OPTIONS = &IdentTable.get("NS_OPTIONS");
 
+    kw_as = &IdentTable.get("as");
     kw_async = &IdentTable.get("async");
     kw_await = &IdentTable.get("await");
     kw_finally = &IdentTable.get("finally");
     kw_from = &IdentTable.get("from");
     kw_function = &IdentTable.get("function");
     kw_import = &IdentTable.get("import");
     kw_is = &IdentTable.get("is");
     kw_let = &IdentTable.get("let");
struct AdditionalKeywords {
   IdentifierInfo *kw_of;
   IdentifierInfo *kw_CF_ENUM;
   IdentifierInfo *kw_CF_OPTIONS;
   IdentifierInfo *kw_NS_ENUM;
   IdentifierInfo *kw_NS_OPTIONS;
   IdentifierInfo *kw___except;
 
   // JavaScript keywords.
+  IdentifierInfo *kw_as;
   IdentifierInfo *kw_async;
   IdentifierInfo *kw_await;
   IdentifierInfo *kw_finally;
   IdentifierInfo *kw_from;
   IdentifierInfo *kw_function;
   IdentifierInfo *kw_import;
   IdentifierInfo *kw_is;
   IdentifierInfo *kw_let;

unittests/Format/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_unittest(FormatTests
   CleanupTest.cpp
   FormatTest.cpp
   FormatTestJava.cpp
   FormatTestJS.cpp
   FormatTestProto.cpp
   FormatTestSelective.cpp
+  SortImportsTestJS.cpp
   SortIncludesTest.cpp
   )
 
 target_link_libraries(FormatTests
   clangBasic
   clangFormat
   clangFrontend
   clangRewrite
   clangToolingCore
   )

unittests/Format/SortImportsTestJS.cpp

+//===- unittest/Format/SortImportsTestJS.cpp - JS import sort unit tests --===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJS : 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, StringRef FileName = "input.js") {
+    auto Ranges = GetCodeRange(Code);
+    std::string Sorted = applyAllReplacements(
+        Code, sortJavaScriptIncludes(Style, Code, Ranges, FileName));
+    return applyAllReplacements(Sorted,
+                                reformat(Style, Sorted, Ranges, FileName));
+  }
+
+  void verifySort(llvm::StringRef Expected, llvm::StringRef Code) {
+    std::string Result = sort(Code);
+    EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
+  }
+
+  unsigned newCursor(llvm::StringRef Code, unsigned Cursor) {
+    sortJavaScriptIncludes(Style, Code, GetCodeRange(Code), "input.js",
+                           &Cursor);
+    return Cursor;
+  }
+
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+};
+
+TEST_F(SortImportsTestJS, BasicSorting) {
+  verifySort("import {sym} from 'a';\n"
+             "import {sym} from 'b';\n"
+             "import {sym} from 'c';\n"
+             "\n"
+             "let x = 1;",
+             "import {sym} from 'a';\n"
+             "import {sym} from 'c';\n"
+             "import {sym} from 'b';\n"
+             "let x = 1;");
+}
+
+TEST_F(SortImportsTestJS, Comments) {
+  verifySort("/** @fileoverview This is a great file. */\n"
+             "// A very important import follows.\n"
+             "import {sym} from 'a'; /* more comments */\n"
+             "import {sym} from 'b'; // from //foo:bar\n",
+             "/** @fileoverview This is a great file. */\n"
+             "import {sym} from 'b'; // from //foo:bar\n"
+             "// A very important import follows.\n"
+             "import {sym} from 'a'; /* more comments */\n");
+}
+
+TEST_F(SortImportsTestJS, SortStar) {
+  verifySort("import * as foo from 'a';\n"
+             "import {sym} from 'a';\n"
+             "import * as bar from 'b';\n",
+             "import {sym} from 'a';\n"
+             "import * as foo from 'a';\n"
+             "import * as bar from 'b';\n");
+}
+
+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, 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"
+             "\n"
+             "let x = 1;\n",
+             "import {b} from './relative/path/nested';\n"
+             "import {b} from './relative/path';\n"
+             "import {b} from '../parent/nested';\n"
+             "import {b} from '../parent';\n"
+             "import {a} from 'absolute';\n"
+             "let x = 1;\n");
+}
+
+TEST_F(SortImportsTestJS, Exports) {
+  verifySort("import {S} from 'bpath';\n"
+             "\n"
+             "import {T} from './cpath';\n"
+             "\n"
+             "export {A, B} from 'apath';\n"
+             "export {P} from '../parent';\n"
+             "export {R} from './relative';\n"
+             "export {S};\n"
+             "\n"
+             "let x = 1;\n"
+             "export y = 1;\n",
+             "export {R} from './relative';\n"
+             "import {T} from './cpath';\n"
+             "export {S};\n"
+             "export {A, B} from 'apath';\n"
+             "import {S} from 'bpath';\n"
+             "export {P} from '../parent';\n"
+             "let x = 1;\n"
+             "export y = 1;\n");
+}
+
+} // end namespace
+} // end namespace format
+} // end namespace clang