clang-format: [JS] do not wrap ES6 imports/exports.

[mprobst]

Summary

"import ... from '...';" and "export ... from '...';" should be treated the same
as goog.require/provide/module/forwardDeclare calls.

[mprobst]

FYI this should not be submitted yet, there are still style discussions going on.

[djasper]

I am missing a decent explanation here. In contrast to C++ #includes as well as goog.require, etc. The import/export statements actually provide syntactic structure and can name multiple entities. Why would it be a good idea to always write them into a single line?

unittests/Format/FormatTestJS.cpp

867

Really?

[mprobst]

As discussed offline, this matches existing very similar behaviour for destructured goog.require calls:

const {X, Y, Z} = goog.require('a'); // won't ever wrap
import {X, Y, Z} from 'a';  // Shouldn't ever wrap

It also has the nice side effect that it prevents IWYU comments of the form:

import {X, Y, Z} from 'a';  // from //some/location

... from wrapping.

[djasper]

lib/Format/TokenAnnotator.cpp

760

Can you actually have:

export {a as from};

?

763

Not a big fan of the code duplication in this loop. How about doing:

if (Style.Language == FormatStyle::LK_JavaScript &&
    ((Line.First->is(tok::kw_export) && CurrentToken->is(Keywords.kw_from)) ||
     isClosureImportStatement(*CurrentToken)))
  return LT_ImportStatement;

in the lower loop? The ImportStatement variable can likely go away then.

[mprobst]

lib/Format/TokenAnnotator.cpp

760

Good observation. Fixed by specifically looking for from 'some tring';.

763

Merged with the code below. My intent was to not do this check for JavaScript code that doesn't start with import or export, but merging seems fair, too.

[mprobst]

• Address review comments:

[mprobst]

Rebase diff on current master.

[mprobst]

• Address review comments:
• Add one more explanatory comment.

[djasper]

Looks good. Do you have commit access now?

[mprobst]

I've asked for it, but don't have it yet. Feel free to commit on by behalf.

[djasper]

Submitted as r264055.

lib/Format/FormatToken.h

AdditionalKeywords(IdentifierTable &IdentTable) {
     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_finally = &IdentTable.get("finally");
     kw_function = &IdentTable.get("function");
+    kw_from = &IdentTable.get("from");
     kw_import = &IdentTable.get("import");
     kw_is = &IdentTable.get("is");
     kw_let = &IdentTable.get("let");
     kw_var = &IdentTable.get("var");
 
     kw_abstract = &IdentTable.get("abstract");
     kw_assert = &IdentTable.get("assert");
     kw_extends = &IdentTable.get("extends");
struct AdditionalKeywords {
   IdentifierInfo *kw_CF_OPTIONS;
   IdentifierInfo *kw_NS_ENUM;
   IdentifierInfo *kw_NS_OPTIONS;
   IdentifierInfo *kw___except;
 
   // JavaScript keywords.
   IdentifierInfo *kw_finally;
   IdentifierInfo *kw_function;
+  IdentifierInfo *kw_from;
   IdentifierInfo *kw_import;
   IdentifierInfo *kw_is;
   IdentifierInfo *kw_let;
   IdentifierInfo *kw_var;
 
   // Java keywords.
   IdentifierInfo *kw_abstract;
   IdentifierInfo *kw_assert;

lib/Format/TokenAnnotator.cpp

LineType parseLine() {
     // and should not be line-wrapped.
     if (Style.Language == FormatStyle::LK_Proto && Line.Level == 0 &&
         CurrentToken->is(Keywords.kw_option)) {
       next();
       if (CurrentToken && CurrentToken->is(tok::identifier))
         return LT_ImportStatement;
     }
 
+    if (Style.Language == FormatStyle::LK_JavaScript) {
+      // import {...} from '...';
+      if (CurrentToken->is(Keywords.kw_import))
+        return LT_ImportStatement;
+      if (CurrentToken->is(tok::kw_export)) {
+        // Find the 'from' part of export {...} from '...';
+        // The difference here is that "export {...};" should not be treated as
+        // an export.
+        while (CurrentToken) {
+          if (CurrentToken->is(tok::semi))
+            return LT_Other;
+          if (CurrentToken->is(Keywords.kw_from))
+            return LT_ImportStatement;
+          if (!consumeToken())
+            return LT_Invalid;
+        }
+      }
+    }
+
     bool KeywordVirtualFound = false;
     bool ImportStatement = false;
     while (CurrentToken) {
       if (CurrentToken->is(tok::kw_virtual))
         KeywordVirtualFound = true;
-      if (isImportStatement(*CurrentToken))
+      if (isClosureImportStatement(*CurrentToken))
         ImportStatement = true;
       if (!consumeToken())
         return LT_Invalid;
     }
     if (KeywordVirtualFound)
       return LT_VirtualFunctionDecl;
     if (ImportStatement)
       return LT_ImportStatement;
 
     if (Line.startsWith(TT_ObjCMethodSpecifier)) {
       if (Contexts.back().FirstObjCSelectorName)
         Contexts.back().FirstObjCSelectorName->LongestObjCSelectorName =
             Contexts.back().LongestObjCSelectorName;
       return LT_ObjCMethodDecl;
     }
 
     return LT_Other;
   }
 
 private:
-  bool isImportStatement(const FormatToken &Tok) {
+  bool isClosureImportStatement(const FormatToken &Tok) {
     // FIXME: Closure-library specific stuff should not be hard-coded but be
     // configurable.
-    return Style.Language == FormatStyle::LK_JavaScript &&
-           Tok.TokenText == "goog" && Tok.Next && Tok.Next->is(tok::period) &&
+    if (Style.Language != FormatStyle::LK_JavaScript)
+      return false;
+    return Tok.TokenText == "goog" && Tok.Next && Tok.Next->is(tok::period) &&
            Tok.Next->Next && (Tok.Next->Next->TokenText == "module" ||
                               Tok.Next->Next->TokenText == "provide" ||
                               Tok.Next->Next->TokenText == "require" ||
                               Tok.Next->Next->TokenText == "setTestOnly") &&
            Tok.Next->Next->Next && Tok.Next->Next->Next->is(tok::l_paren);
   }
 
   void resetTokenMetadata(FormatToken *Token) {

unittests/Format/FormatTestJS.cpp

TEST_F(FormatTestJS, MetadataAnnotations) {
   verifyFormat("class X {}\n"
                "class Y {}");
 }
 
 TEST_F(FormatTestJS, Modules) {
   verifyFormat("import SomeThing from 'some/module.js';");
   verifyFormat("import {X, Y} from 'some/module.js';");
   verifyFormat("import a, {X, Y} from 'some/module.js';");
-  verifyFormat("import {\n"
-               "  VeryLongImportsAreAnnoying,\n"
-               "  VeryLongImportsAreAnnoying,\n"
-               "  VeryLongImportsAreAnnoying,\n"
-               "  VeryLongImportsAreAnnoying\n"
+  verifyFormat("import {VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying,"
+               " VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying"
                "} from 'some/module.js';");
-  verifyFormat("import {\n"
-               "  X,\n"
-               "  Y,\n"
-               "} from 'some/module.js';");
-  verifyFormat("import {\n"
-               "  X,\n"
-               "  Y,\n"
-               "} from 'some/long/module.js';",
-               getGoogleJSStyleWithColumns(20));
+  verifyFormat("import {X, Y,} from 'some/module.js';");
   verifyFormat("import {X as myLocalX, Y as myLocalY} from 'some/module.js';");
   verifyFormat("import * as lib from 'some/module.js';");
   verifyFormat("var x = {import: 1};\nx.import = 2;");
 
   verifyFormat("export function fn() {\n"
                "  return 'fn';\n"
                "}");
   verifyFormat("export function A() {}\n"
                "export default function B() {}\n"
                "export function C() {}");
   verifyFormat("export const x = 12;");
   verifyFormat("export default class X {}");
   verifyFormat("export {X, Y} from 'some/module.js';");
-  verifyFormat("export {\n"
-               "  X,\n"
-               "  Y,\n"
-               "} from 'some/module.js';");
+  verifyFormat("export {X, Y,} from 'some/module.js';");
+  verifyFormat("export {SomeVeryLongExport as X, "
+               "SomeOtherVeryLongExport as Y} from 'some/module.js';");
+  // export without 'from' is wrapped.
+  verifyFormat("export let someRatherLongVariableName =\n"
+               "    someSurprisinglyLongVariable + someOtherRatherLongVar;");
   verifyFormat("export class C {\n"
                "  x: number;\n"
                "  y: string;\n"
                "}");
   verifyFormat("export class X { y: number; }");
   verifyFormat("export abstract class X { y: number; }");
   verifyFormat("export default class X { y: number }");
   verifyFormat("export default function() {\n  return 1;\n}");