[clang-format] Correctly format JavaScript imports with long module paths

[JaredNeil]

Summary

Currently the UnwrappedLineParser fails to correctly unwrap JavaScript imports where the module path is not on the same line as the from keyword.
For example:

import {A} from
'some/path/longer/than/column/limit/module.js';

This causes issues when in the middle a list of imports because the formatter thinks it has reached the end of the imports, and therefore will not sort any imports lower in the list.

The formatter will, however, split the from keyword and the module path if the path exceeds the column limit, which triggers the issue the next time the file is formatted.

[mprobst]

Thanks for the contribution! Two nits.

unittests/Format/SortImportsTestJS.cpp

287 ↗

(On Diff #104971)

Do you need all the other imports? Wouldn't a minimal test be to just have:

import {A} from
'y';
import {A} from 'x';

And then see that 'x' does get sorted before 'y'?

292 ↗

(On Diff #104971)

Could you add a comment that this repro's a specific issue where we didn't parse the import correctly? It's non-obvious otherwise.

[mprobst]

Ping?

[JaredNeil]

unittests/Format/SortImportsTestJS.cpp

287 ↗

(On Diff #104971)

Yeah, that does cover this specific issue.
I guess I put the other ones in there to make sure it parsed correctly no matter where the newline was.

Though after looking at it again, the tests really could be combined into a single test like:

import
{
A
}
from
'y';
import {A} from 'x';

Would that be better or worse than the simple one you suggested?

[mprobst]

unittests/Format/SortImportsTestJS.cpp

287 ↗

(On Diff #104971)

I think that's roughly the same as what I suggested, isn't it? Looks good in any case.

[JaredNeil]

Combined the sorting tests into a single test that covers all of those cases.

[mprobst]

unittests/Format/FormatTestJS.cpp

1459 ↗

(On Diff #106900)

FYI I updated this to have some different whitespace in the original input, so that we can be sure something actually was changed and clang-format didn't bail somehow.

cfe/trunk/lib/Format/UnwrappedLineParser.cpp

return FormatTok->is(tok::identifier) &&
          (FormatTok->Tok.getIdentifierInfo() == nullptr ||
           !FormatTok->isOneOf(
               Keywords.kw_in, Keywords.kw_of, Keywords.kw_as, Keywords.kw_async,
               Keywords.kw_await, Keywords.kw_yield, Keywords.kw_finally,
               Keywords.kw_function, Keywords.kw_import, Keywords.kw_is,
               Keywords.kw_let, Keywords.kw_var, tok::kw_const,
               Keywords.kw_abstract, Keywords.kw_extends, Keywords.kw_implements,
               Keywords.kw_instanceof, Keywords.kw_interface,
-              Keywords.kw_throws));
+              Keywords.kw_throws, Keywords.kw_from));
 }
 
 static bool mustBeJSIdentOrValue(const AdditionalKeywords &Keywords,
                                  const FormatToken *FormatTok) {
   return FormatTok->Tok.isLiteral() ||
          FormatTok->isOneOf(tok::kw_true, tok::kw_false) ||
          mustBeJSIdent(Keywords, FormatTok);
 }

cfe/trunk/unittests/Format/FormatTestJS.cpp

verifyFormat("import {\n"
                "  A,\n"
                "} from 'some/module.js';",
                Style);
   verifyFormat("export {\n"
                "  A,\n"
                "  A,\n"
                "} from 'some/module.js';",
                Style);
+  Style.ColumnLimit = 40;
+  // Using this version of verifyFormat because test::messUp hides the issue.
+  verifyFormat("import {\n"
+               "  A,\n"
+               "} from\n"
+               "    'some/path/longer/than/column/limit/module.js';",
+               " import  {  \n"
+               "    A,  \n"
+               "  }    from\n"
+               "      'some/path/longer/than/column/limit/module.js'  ; ",
+               Style);
 }
 
 TEST_F(FormatTestJS, TemplateStrings) {
   // Keeps any whitespace/indentation within the template string.
   verifyFormat("var x = `hello\n"
             "     ${name}\n"
             "  !`;",
             "var x    =    `hello\n"

cfe/trunk/unittests/Format/SortImportsTestJS.cpp

TEST_F(SortImportsTestJS, SortCaseInsensitive) {
   verifySort("import {aa, Ab, b, Bc} from 'x';\n"
              "\n"
              "1;",
              "import {b, Bc, Ab, aa} from 'x';\n"
              "\n"
              "1;");
 }
 
+TEST_F(SortImportsTestJS, SortMultiLine) {
+  // Reproduces issue where multi-line import was not parsed correctly.
+  verifySort("import {A} from 'a';\n"
+             "import {A} from 'b';\n"
+             "\n"
+             "1;",
+             "import\n"
+             "{\n"
+             "A\n"
+             "}\n"
+             "from\n"
+             "'b';\n"
+             "import {A} from 'a';\n"
+             "\n"
+             "1;");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang