[clang-format] Indent import statements in JavaScript.

[sstwcw]

Summary

[clang-format] Indent import statements in JavaScript.

Take for example this piece of code found at
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import.

for (const link of document.querySelectorAll("nav > a")) {
  link.addEventListener("click", e => {
    e.preventDefault();

    import('/modules/my-module.js')
        .then(module => {
          module.loadPageInto(main);
        })
        .catch(err => {
          main.textContent = err.message;
        });
  });
}

Previously the import line would be unindented, looking like this.

for (const link of document.querySelectorAll("nav > a")) {
  link.addEventListener("click", e => {
    e.preventDefault();

import('/modules/my-module.js')
        .then(module => {
          module.loadPageInto(main);
        })
        .catch(err => {
          main.textContent = err.message;
        });
  });
}

Actually we were going to fix this along with fixing Verilog import
statements. But the patch got too big.

[owenpan]

Please add @krasimir @MyDeveloperDay @curdeius @HazardyKnusperkeks to Reviewers.

[MyDeveloperDay]

Can you please supply full context diff "Context not available."

clang/lib/Format/ContinuationIndenter.cpp

629

!Style.isJavaScript()

clang/lib/Format/UnwrappedLineFormatter.cpp

1436

!Style.isJavaScript()

[sstwcw]

use isJavascript

[MyDeveloperDay]

clang/lib/Format/ContinuationIndenter.cpp

630

can I check? is this case really firing? i mean isn't it

# import

why would javascript be seeing that?

[sstwcw]

clang/lib/Format/ContinuationIndenter.cpp

630

In Javascript import statements also need some special handling. So the line is intentionally labeled as LT_ImportStatement. In AnnotatingParser::parseLine there is this.

if (Style.isJavaScript() && CurrentToken->is(Keywords.kw_import))
  ImportStatement = true;

I don't know why the line type isn't printed in TokenAnnotator::printDebugInfo.

[MyDeveloperDay]

clang/lib/Format/ContinuationIndenter.cpp

632

AfterHash - Could be true
Previous is hash - Can't be true can it?
FirstIndent > 0 - I guess could be true
State.Line->Type being LT_ImportStatement could be true..

true && false && true && true == false right?

Remove !Style.isJavaScript() from this, does your test still pass?

Either add the second test to cover this case or remove the condition, I don't mind either way.

[sstwcw]

remove unnecessary check

[sstwcw]

clang/lib/Format/ContinuationIndenter.cpp

632

I undid the change here.

clang/lib/Format/ContinuationIndenter.cpp

while (Tok && CurrentState.VariablePos >= Tok->ColumnWidth) {
       Tok = Tok->Previous;
     }
     if (Previous.PartOfMultiVariableDeclStmt)
       CurrentState.LastSpace = CurrentState.VariablePos;
   }
 
   unsigned Spaces = Current.SpacesRequiredBefore + ExtraSpaces;
 
-  // Indent preprocessor directives after the hash if required.
+  // Indent preprocessor directives after the hash if required. In Javascript
+  // import statements are indented like normal statements.
   int PPColumnCorrection = 0;
   if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash &&
       Previous.is(tok::hash) && State.FirstIndent > 0 &&
       (State.Line->Type == LT_PreprocessorDirective ||
        State.Line->Type == LT_ImportStatement)) {
     Spaces += State.FirstIndent;
 
     // For preprocessor indent with tabs, State.Column will be 1 because of the
     // hash. This causes second-level indents onward to have an extra space
     // after the tabs. We avoid this misalignment by subtracting 1 from the
     // column value passed to replaceWhitespace().
     if (Style.UseTab != FormatStyle::UT_Never)
       PPColumnCorrection = -1;

clang/lib/Format/UnwrappedLineFormatter.cpp

if (!RootToken.isAccessSpecifier()) {
         break;
       }
     }
   }
 
   if (Newlines)
     Indent = NewlineIndent;
 
-  // Preprocessor directives get indented before the hash only if specified
-  if (Style.IndentPPDirectives != FormatStyle::PPDIS_BeforeHash &&
+  // Preprocessor directives get indented before the hash only if specified. In
+  // Javascript import statements are indented like normal statements.
+  if (!Style.isJavaScript() &&
+      Style.IndentPPDirectives != FormatStyle::PPDIS_BeforeHash &&
       (Line.Type == LT_PreprocessorDirective ||
        Line.Type == LT_ImportStatement))
     Indent = 0;
 
   Whitespaces->replaceWhitespace(RootToken, Newlines, Indent, Indent,
                                  /*IsAligned=*/false,
                                  Line.InPPDirective &&
                                      !RootToken.HasUnescapedNewline);

clang/unittests/Format/FormatTestJS.cpp

TEST_F(FormatTestJS, Modules) {
   verifyFormat("import a, {X, Y} from 'some/module.js';");
   verifyFormat("import {X, Y,} from 'some/module.js';");
   verifyFormat("import {X as myLocalX, Y as myLocalY} from 'some/module.js';");
   // Ensure Automatic Semicolon Insertion does not break on "as\n".
   verifyFormat("import {X as myX} from 'm';", "import {X as\n"
                                               " myX} from 'm';");
   verifyFormat("import * as lib from 'some/module.js';");
   verifyFormat("var x = {import: 1};\nx.import = 2;");
+  // Ensure an import statement inside a block is at the correct level.
+  verifyFormat("function() {\n"
+               "  var x;\n"
+               "  import 'some/module.js';\n"
+               "}");
 
   verifyFormat("export function fn() {\n"
                "  return 'fn';\n"
                "}");
   verifyFormat("export function A() {}\n"
                "export default function B() {}\n"
                "export function C() {}");
   verifyFormat("export default () => {\n"