clang-format: [JS] handle "off" in imports

[mprobst]

Summary

Previously, the JavaScript import sorter would ignore `// clang-format
off` and on comments. This change fixes that. It tracks whether
formatting is enabled for a stretch of imports, and then only sorts and
merges the imports where formatting is enabled, in individual chunks.

This means that there's no meaningful total order when module references are mixed
with blocks that have formatting disabled. The alternative approach
would have been to sort all imports that have formatting enabled in one
group. However that raises the question where to insert the
formatting-off block, which can also impact symbol visibility (in
particular for exports). In practice, sorting in chunks probably isn't a
big problem.

This change also simplifies the general algorithm: instead of tracking
indices separately and sorting them, it just sorts the vector of module
references. And instead of attempting to do fine grained tracking of
whether the code changed order, it just prints out the module references
text, and compares that to the previous text. Given that source files
typically have dozens, but not even hundreds of imports, the performance
impact seems negligible.

[h-joo]

clang/lib/Format/SortJavaScriptImports.cpp

252

References seem read-only. Can you make it const Smallvector &?

257

nit : Start

379

I think this has a potential bug that if the user makes

// clang-format off
// clang-format on

Every subsequent import will fail to be sorted.

or maybe something like below

//clang-format off
import {A, B} from "./a"
//clang-format on
//clang-format off

will make the 'should not be sorted' part of the code be sorted.

Even if this is not the case, could you write a test for this one?

clang/unittests/Format/SortImportsTestJS.cpp

405

Do you think it makes sense to add some weird test cases that // clang-format on-off switch, is pointless or ill-formed?
examples :

// this is for the corner-case I mentioned above around the comment token handler
// clang-format off
// clang-format on
import {C} from "./.."
import {B} from "./.."
import {A} from "./.."

// The clang-format off comment might be dropped or not?
// clang-format on
import {C} from "./.."
import {B} from "./.."
import {A} from "./.."
// clang-format off

// This test maybe isn't necessary?
// clang-format off
// clang-format off
import {C} from "./.."
import {B} from "./.."
import {A} from "./.."

...and maybe some other fun examples! :)

[krasimir]

clang/lib/Format/SortJavaScriptImports.cpp

157–158

nit: capitalize, I, E (:

191

note that we generally also support /* clang-format off/on */: https://github.com/llvm/llvm-project/blob/aaf026d9da3885a951dcdc5edd64c8e7d23b6285/clang/lib/Format/Format.cpp#L2390

251

We should pass in and return llvm::SmallVectorImpl<JsModuleReference> (the size version is mainly for local vars). Also, take the arg by const reference.

257

nit: Start

288

use llvm::SmallVectorImpl<JsModuleReference>

clang/unittests/Format/SortImportsTestJS.cpp

362

Was this change intended? It doesn't seem related to this patch.

385

can we have a test that checks that we don't merge import lines in an off section

[krasimir]

@h-joo , seems we had a comment race :D cheers!

[h-joo]

In D101515#2726889, @krasimir wrote:

@h-joo , seems we had a comment race :D cheers!

:D

[mprobst]

PTAL, addressed review comments.

clang/lib/Format/SortJavaScriptImports.cpp

191

We don't in the C++ import sorter. Sigh.

I think the Real Fix (tm) would be folding the clang-format off thing in import sorting with affected lines, wouldn't it? I don't quite know how to untangle this, but it seems suboptimal that each sorter here re-implements the same logic.

251

That gives me:

error: no viable conversion from 'SmallVector<[...], 16>' to 'const SmallVector<[...], (default) CalculateSmallVectorDefaultInlinedElements<T>::value aka 1>'

288

As above, this seems not possible.

379

Nice, good catch! Done.

clang/unittests/Format/SortImportsTestJS.cpp

362

the empty line is intended. Previously this wouldn't end up formatting because the code was trying to be too clever in detecting changes.

[mprobst]

• review comments

[mprobst]

• test for avoiding merges

[mprobst]

added test for merging

[h-joo]

LGTM :)

[krasimir]

clang/lib/Format/SortJavaScriptImports.cpp

257

please fix lint warning on line 257

[mprobst]

• review comments

clang/lib/Format/SortJavaScriptImports.cpp

 #include "SortJavaScriptImports.h"
 #include "TokenAnalyzer.h"
 #include "TokenAnnotator.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TokenKinds.h"
 #include "clang/Format/Format.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Debug.h"
 #include <algorithm>
 #include <string>
 
 #define DEBUG_TYPE "format-formatter"

 //
 // imports and exports support individual symbols, but also a wildcard syntax:
 //   import * as prefix from 'foo';
 //   export * from 'bar';
 //
 // This struct represents both exports and imports to build up the information
 // required for sorting module references.
 struct JsModuleReference {
+  bool FormattingOff = false;
   bool IsExport = false;
   // Module references are sorted into these categories, in order.
   enum ReferenceCategory {
     SIDE_EFFECT,     // "import 'something';"
     ABSOLUTE,        // from 'something'
     RELATIVE_PARENT, // from '../*'
     RELATIVE,        // from './*'
   };
analyze(TokenAnnotator &Annotator,
     SmallVector<JsModuleReference, 16> References;
     AnnotatedLine *FirstNonImportLine;
     std::tie(References, FirstNonImportLine) =
         parseModuleReferences(Keywords, AnnotatedLines);
 
     if (References.empty())
       return {Result, 0};
 
-    SmallVector<unsigned, 16> Indices;
-    for (unsigned i = 0, e = References.size(); i != e; ++i)
-      Indices.push_back(i);
-    llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
-      return References[LHSI] < References[RHSI];
-    });
-    bool ReferencesInOrder = llvm::is_sorted(Indices);
+    // The text range of all parsed imports, to be replaced later.
+    SourceRange InsertionPoint = References[0].Range;
+    InsertionPoint.setEnd(References[References.size() - 1].Range.getEnd());
 
-    mergeModuleReferences(References, Indices);
+    References = sortModuleReferences(References);
 
     std::string ReferencesText;
-    bool SymbolsInOrder = true;
-    for (unsigned i = 0, e = Indices.size(); i != e; ++i) {
-      JsModuleReference Reference = References[Indices[i]];
-      if (appendReference(ReferencesText, Reference))
-        SymbolsInOrder = false;
-      if (i + 1 < e) {
+    for (unsigned I = 0, E = References.size(); I != E; ++I) {
+      JsModuleReference Reference = References[I];
+      appendReference(ReferencesText, Reference);
+      if (I + 1 < E) {
         // Insert breaks between imports and exports.
         ReferencesText += "\n";
         // Separate imports groups with two line breaks, but keep all exports
         // in a single group.
         if (!Reference.IsExport &&
-            (Reference.IsExport != References[Indices[i + 1]].IsExport ||
-             Reference.Category != References[Indices[i + 1]].Category))
+            (Reference.IsExport != References[I + 1].IsExport ||
+             Reference.Category != References[I + 1].Category))
           ReferencesText += "\n";
       }
     }
-    if (ReferencesInOrder && SymbolsInOrder)
+    llvm::StringRef PreviousText = getSourceText(InsertionPoint);
+    if (ReferencesText == PreviousText)
       return {Result, 0};
 
-    SourceRange InsertionPoint = References[0].Range;
-    InsertionPoint.setEnd(References[References.size() - 1].Range.getEnd());
-
     // The loop above might collapse previously existing line breaks between
     // import blocks, and thus shrink the file. SortIncludes must not shrink
     // overall source length as there is currently no re-calculation of ranges
     // after applying source sorting.
     // This loop just backfills trailing spaces after the imports, which are
     // harmless and will be stripped by the subsequent formatting pass.
     // FIXME: A better long term fix is to re-calculate Ranges after sorting.
-    unsigned PreviousSize = getSourceText(InsertionPoint).size();
+    unsigned PreviousSize = PreviousText.size();
     while (ReferencesText.size() < PreviousSize) {
       ReferencesText += " ";
     }
 
     // Separate references from the main code body of the file.
-    if (FirstNonImportLine && FirstNonImportLine->First->NewlinesBefore < 2)
+    if (FirstNonImportLine && FirstNonImportLine->First->NewlinesBefore < 2 &&
+        !(FirstNonImportLine->First->is(tok::comment) &&
+          FirstNonImportLine->First->TokenText.trim() == "// clang-format on"))
       ReferencesText += "\n";
 
     LLVM_DEBUG(llvm::dbgs() << "Replacing imports:\n"
-                            << getSourceText(InsertionPoint) << "\nwith:\n"
+                            << PreviousText << "\nwith:\n"
                             << ReferencesText << "\n");
     auto Err = Result.add(tooling::Replacement(
         Env.getSourceManager(), CharSourceRange::getCharRange(InsertionPoint),
         ReferencesText));
     // FIXME: better error handling. For now, just print error message and skip
     // the replacement for the release version.
     if (Err) {
       llvm::errs() << llvm::toString(std::move(Err)) << "\n";
private:
   }
 
   StringRef getSourceText(SourceLocation Begin, SourceLocation End) {
     const SourceManager &SM = Env.getSourceManager();
     return FileContents.substr(SM.getFileOffset(Begin),
                                SM.getFileOffset(End) - SM.getFileOffset(Begin));
   }
 
+  // Sorts the given module references.
+  // Imports can have formatting disabled (FormattingOff), so the code below
+  // skips runs of "no-formatting" module references, and sorts/merges the
+  // references that have formatting enabled in individual chunks.
+  SmallVector<JsModuleReference, 16>
+  sortModuleReferences(const SmallVector<JsModuleReference, 16> &References) {
+    // Sort module references.
+    // Imports can have formatting disabled (FormattingOff), so the code below
+    // skips runs of "no-formatting" module references, and sorts other
+    // references per group.
+    const auto *Start = References.begin();
+    SmallVector<JsModuleReference, 16> ReferencesSorted;
+    while (Start != References.end()) {
+      while (Start != References.end() && Start->FormattingOff) {
+        // Skip over all imports w/ disabled formatting.
+        ReferencesSorted.push_back(*Start);
+        Start++;
+      }
+      SmallVector<JsModuleReference, 16> SortChunk;
+      while (Start != References.end() && !Start->FormattingOff) {
+        // Skip over all imports w/ disabled formatting.
+        SortChunk.push_back(*Start);
+        Start++;
+      }
+      llvm::stable_sort(SortChunk);
+      mergeModuleReferences(SortChunk);
+      ReferencesSorted.insert(ReferencesSorted.end(), SortChunk.begin(),
+                              SortChunk.end());
+    }
+    return ReferencesSorted;
+  }
+
   // Merge module references.
   // After sorting, find all references that import named symbols from the
   // same URL and merge their names. E.g.
   //   import {X} from 'a';
   //   import {Y} from 'a';
   // should be rewritten to:
   //   import {X, Y} from 'a';
-  // Note: this modifies the passed in ``Indices`` vector (by removing no longer
-  // needed references), but not ``References``.
-  // ``JsModuleReference``s that get merged have the ``SymbolsMerged`` flag
-  // flipped to true.
-  void mergeModuleReferences(SmallVector<JsModuleReference, 16> &References,
-                             SmallVector<unsigned, 16> &Indices) {
-    JsModuleReference *PreviousReference = &References[Indices[0]];
-    auto *It = std::next(Indices.begin());
-    while (It != std::end(Indices)) {
-      JsModuleReference *Reference = &References[*It];
+  // Note: this modifies the passed in ``References`` vector (by removing no
+  // longer needed references).
+  void mergeModuleReferences(SmallVector<JsModuleReference, 16> &References) {
+    if (References.empty())
+      return;
+    JsModuleReference *PreviousReference = References.begin();
+    auto *Reference = std::next(References.begin());
+    while (Reference != References.end()) {
       // Skip:
       //   import 'foo';
       //   import * as foo from 'foo'; on either previous or this.
       //   import Default from 'foo'; on either previous or this.
       //   mismatching
       if (Reference->Category == JsModuleReference::SIDE_EFFECT ||
           PreviousReference->Category == JsModuleReference::SIDE_EFFECT ||
           Reference->IsExport != PreviousReference->IsExport ||
           !PreviousReference->Prefix.empty() || !Reference->Prefix.empty() ||
           !PreviousReference->DefaultImport.empty() ||
           !Reference->DefaultImport.empty() || Reference->Symbols.empty() ||
           PreviousReference->URL != Reference->URL) {
         PreviousReference = Reference;
-        ++It;
+        ++Reference;
         continue;
       }
       // Merge symbols from identical imports.
       PreviousReference->Symbols.append(Reference->Symbols);
       PreviousReference->SymbolsMerged = true;
       // Remove the merged import.
-      It = Indices.erase(It);
+      Reference = References.erase(Reference);
     }
   }
 
-  // Appends ``Reference`` to ``Buffer``, returning true if text within the
-  // ``Reference`` changed (e.g. symbol order).
-  bool appendReference(std::string &Buffer, JsModuleReference &Reference) {
+  // Appends ``Reference`` to ``Buffer``.
+  void appendReference(std::string &Buffer, JsModuleReference &Reference) {
     // Sort the individual symbols within the import.
     // E.g. `import {b, a} from 'x';` -> `import {a, b} from 'x';`
     SmallVector<JsImportedSymbol, 1> Symbols = Reference.Symbols;
     llvm::stable_sort(
         Symbols, [&](const JsImportedSymbol &LHS, const JsImportedSymbol &RHS) {
           return LHS.Symbol.compare_lower(RHS.Symbol) < 0;
         });
     if (!Reference.SymbolsMerged && Symbols == Reference.Symbols) {
       // Symbols didn't change, just emit the entire module reference.
       StringRef ReferenceStmt = getSourceText(Reference.Range);
       Buffer += ReferenceStmt;
-      return false;
+      return;
     }
     // Stitch together the module reference start...
     Buffer += getSourceText(Reference.Range.getBegin(), Reference.SymbolsStart);
     // ... then the references in order ...
     for (auto I = Symbols.begin(), E = Symbols.end(); I != E; ++I) {
       if (I != Symbols.begin())
         Buffer += ",";
       Buffer += getSourceText(I->Range);
     }
     // ... followed by the module reference end.
     Buffer += getSourceText(Reference.SymbolsEnd, Reference.Range.getEnd());
-    return true;
   }
 
   // Parses module references in the given lines. Returns the module references,
   // and a pointer to the first "main code" line if that is adjacent to the
   // affected lines of module references, nullptr otherwise.
   std::pair<SmallVector<JsModuleReference, 16>, AnnotatedLine *>
   parseModuleReferences(const AdditionalKeywords &Keywords,
                         SmallVectorImpl<AnnotatedLine *> &AnnotatedLines) {
     SmallVector<JsModuleReference, 16> References;
     SourceLocation Start;
     AnnotatedLine *FirstNonImportLine = nullptr;
     bool AnyImportAffected = false;
+    bool FormattingOff = false;
     for (auto *Line : AnnotatedLines) {
       Current = Line->First;
       LineEnd = Line->Last;
+      // clang-format comments toggle formatting on/off.
+      // This is tracked in FormattingOff here and on JsModuleReference.
+      while (Current && Current->is(tok::comment)) {
+        StringRef CommentText = Current->TokenText.trim();
+        if (CommentText == "// clang-format off") {
+          FormattingOff = true;
+        } else if (CommentText == "// clang-format on") {
+          FormattingOff = false;
+          // Special case: consider a trailing "clang-format on" line to be part
+          // of the module reference, so that it gets moved around together with
+          // it (as opposed to the next module reference, which might get sorted
+          // around).
+          if (!References.empty()) {
+            References.back().Range.setEnd(Current->Tok.getEndLoc());
+            Start = Current->Tok.getEndLoc().getLocWithOffset(1);
+          }
+        }
+        // Handle all clang-format comments on a line, e.g. for an empty block.
+        Current = Current->Next;
+      }
       skipComments();
       if (Start.isInvalid() || References.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 Start.
         Start = Line->First->Tok.getLocation();
       if (!Current) {
         // Only comments on this line. Could be the first non-import line.
         FirstNonImportLine = Line;
         continue;
       }
       JsModuleReference Reference;
+      Reference.FormattingOff = FormattingOff;
       Reference.Range.setBegin(Start);
       if (!parseModuleReference(Keywords, Reference)) {
         if (!FirstNonImportLine)
           FirstNonImportLine = Line; // if no comment before.
         break;
       }
       FirstNonImportLine = nullptr;
       AnyImportAffected = AnyImportAffected || Line->Affected;
       Reference.Range.setEnd(LineEnd->Tok.getEndLoc());
       LLVM_DEBUG({
         llvm::dbgs() << "JsModuleReference: {"
-                     << "is_export: " << Reference.IsExport
+                     << "formatting_off: " << Reference.FormattingOff
+                     << ", is_export: " << Reference.IsExport
                      << ", cat: " << Reference.Category
                      << ", url: " << Reference.URL
                      << ", prefix: " << Reference.Prefix;
-        for (size_t i = 0; i < Reference.Symbols.size(); ++i)
-          llvm::dbgs() << ", " << Reference.Symbols[i].Symbol << " as "
-                       << Reference.Symbols[i].Alias;
+        for (size_t I = 0; I < Reference.Symbols.size(); ++I)
+          llvm::dbgs() << ", " << Reference.Symbols[I].Symbol << " as "
+                       << Reference.Symbols[I].Alias;
         llvm::dbgs() << ", text: " << getSourceText(Reference.Range);
         llvm::dbgs() << "}\n";
       });
       References.push_back(Reference);
       Start = SourceLocation();
     }
     // Sort imports if any import line was affected.
     if (!AnyImportAffected)

clang/unittests/Format/SortImportsTestJS.cpp

verifySort("// a\n"
              "import {Z} from 'z';\n"
              "// also a\n"
              "import {/* x */ X} from 'a';\n"
              "\n"
              "X + Y + Z;\n");
 
   // do not merge imports and exports
   verifySort("import {A} from 'foo';\n"
-             "export {B} from 'foo';",
+             "\n"
+             "export {B} from 'foo';\n",
              "import {A} from 'foo';\n"
              "export   {B} from 'foo';");
   // do merge exports
   verifySort("export {A, B} from 'foo';\n", "export {A} from 'foo';\n"
                                             "export   {B} from 'foo';");
 
   // do not merge side effect imports with named ones
   verifySort("import './a';\n"
              "\n"
              "import {bar} from './a';\n",
              "import {bar} from './a';\n"
              "import './a';\n");
 }
 
+TEST_F(SortImportsTestJS, RespectsClangFormatOff) {
+  verifySort("// clang-format off\n"
+             "import {B} from './b';\n"
+             "import {A} from './a';\n"
+             "// clang-format on\n",
+             "// clang-format off\n"
+             "import {B} from './b';\n"
+             "import {A} from './a';\n"
+             "// clang-format on\n");
+
+  verifySort("import {A} from './sorted1_a';\n"
+             "import {B} from './sorted1_b';\n"
+             "// clang-format off\n"
+             "import {B} from './unsorted_b';\n"
+             "import {A} from './unsorted_a';\n"
+             "// clang-format on\n"
+             "import {A} from './sorted2_a';\n"
+             "import {B} from './sorted2_b';\n",
+             "import {B} from './sorted1_b';\n"
+             "import {A} from './sorted1_a';\n"
+             "// clang-format off\n"
+             "import {B} from './unsorted_b';\n"
+             "import {A} from './unsorted_a';\n"
+             "// clang-format on\n"
+             "import {B} from './sorted2_b';\n"
+             "import {A} from './sorted2_a';\n");
+
+  // Boundary cases
+  verifySort("// clang-format on\n", "// clang-format on\n");
+  verifySort("// clang-format off\n", "// clang-format off\n");
+  verifySort("// clang-format on\n"
+             "// clang-format off\n",
+             "// clang-format on\n"
+             "// clang-format off\n");
+  verifySort("// clang-format off\n"
+             "// clang-format on\n"
+             "import {A} from './a';\n"
+             "import {B} from './b';\n",
+             "// clang-format off\n"
+             "// clang-format on\n"
+             "import {B} from './b';\n"
+             "import {A} from './a';\n");
+  // section ends with comment
+  verifySort("// clang-format on\n"
+             "import {A} from './a';\n"
+             "import {B} from './b';\n"
+             "import {C} from './c';\n"
+             "\n" // inserted empty line is working as intended: splits imports
+                  // section from main code body
+             "// clang-format off\n",
+             "// clang-format on\n"
+             "import {C} from './c';\n"
+             "import {B} from './b';\n"
+             "import {A} from './a';\n"
+             "// clang-format off\n");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang