This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Indent import statements in JavaScript.
ClosedPublic

Authored by sstwcw on Mar 17 2022, 5:26 AM.

Details

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.

Diff Detail

Event Timeline

sstwcw created this revision.Mar 17 2022, 5:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2022, 5:26 AM
sstwcw requested review of this revision.Mar 17 2022, 5:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2022, 5:26 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MyDeveloperDay edited reviewers, added: MyDeveloperDay, curdeius, HazardyKnusperkeks, owenpan, krasimir; removed: Restricted Project.Mar 18 2022, 1:29 AM
MyDeveloperDay edited the summary of this revision. (Show Details)
MyDeveloperDay requested changes to this revision.Mar 18 2022, 1:32 AM

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

clang/lib/Format/ContinuationIndenter.cpp
629 ↗(On Diff #416147)

!Style.isJavaScript()

clang/lib/Format/UnwrappedLineFormatter.cpp
1436

!Style.isJavaScript()

This revision now requires changes to proceed.Mar 18 2022, 1:32 AM
sstwcw updated this revision to Diff 416456.Mar 18 2022, 4:12 AM

use isJavascript

sstwcw marked 2 inline comments as done.Mar 18 2022, 4:13 AM
MyDeveloperDay added inline comments.Mar 18 2022, 4:20 AM
clang/lib/Format/ContinuationIndenter.cpp
631 ↗(On Diff #416456)

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

# import

why would javascript be seeing that?

sstwcw marked an inline comment as done.Mar 18 2022, 4:58 AM
sstwcw added inline comments.
clang/lib/Format/ContinuationIndenter.cpp
631 ↗(On Diff #416456)

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 added inline comments.Mar 18 2022, 9:08 AM
clang/lib/Format/ContinuationIndenter.cpp
633 ↗(On Diff #416456)

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 updated this revision to Diff 417125.Mar 21 2022, 4:03 PM
sstwcw marked an inline comment as done.

remove unnecessary check

sstwcw marked an inline comment as done.Mar 21 2022, 4:04 PM
sstwcw added inline comments.
clang/lib/Format/ContinuationIndenter.cpp
633 ↗(On Diff #416456)

I undid the change here.

sstwcw updated this revision to Diff 417128.Mar 21 2022, 4:05 PM
sstwcw marked an inline comment as done.
MyDeveloperDay accepted this revision.Mar 26 2022, 7:07 AM
This revision is now accepted and ready to land.Mar 26 2022, 7:07 AM
curdeius accepted this revision.Mar 26 2022, 9:19 AM
This revision was landed with ongoing or failed builds.Mar 30 2022, 4:20 PM
This revision was automatically updated to reflect the committed changes.