Fix multiline import specifier sorting (#51634)

andrewbranch · web-flow · commit e6d7b526c827 · 2022-11-28T15:35:32.000-08:00
* Fix multiline import specifier sorting

* Update baselines

* Switch to EmitFlag, set hasTrailingComma on original node array

* Update API baseline

* Update baselines
diff --git a/src/compiler/emitter.ts b/src/compiler/emitter.ts
@@ -4862,7 +4862,14 @@ export function createPrinter(printerOptions: PrinterOptions = {}, handlers: Pri
     }
 
     function emitList(parentNode: Node | undefined, children: NodeArray<Node> | undefined, format: ListFormat, parenthesizerRule?: ParenthesizerRuleOrSelector<Node>, start?: number, count?: number) {
-        emitNodeList(emit, parentNode, children, format, parenthesizerRule, start, count);
+        emitNodeList(
+            emit,
+            parentNode,
+            children,
+            format | (parentNode && getEmitFlags(parentNode) & EmitFlags.MultiLine ? ListFormat.PreferNewLine : 0),
+            parenthesizerRule,
+            start,
+            count);
     }
 
     function emitExpressionList(parentNode: Node | undefined, children: NodeArray<Node> | undefined, format: ListFormat, parenthesizerRule?: ParenthesizerRuleOrSelector<Expression>, start?: number, count?: number) {
diff --git a/src/compiler/types.ts b/src/compiler/types.ts
@@ -7490,7 +7490,7 @@ export interface EmitNode {
     helpers?: EmitHelper[];                  // Emit helpers for the node
     startsOnNewLine?: boolean;               // If the node should begin on a new line
     snippetElement?: SnippetElement;         // Snippet element of the node
-    typeNode?: TypeNode;                         // VariableDeclaration type
+    typeNode?: TypeNode;                     // VariableDeclaration type
 }
 
 /** @internal */
@@ -7520,38 +7520,39 @@ export const enum SnippetKind {
 export const enum EmitFlags {
     None = 0,
     SingleLine = 1 << 0,                    // The contents of this node should be emitted on a single line.
-    AdviseOnEmitNode = 1 << 1,              // The printer should invoke the onEmitNode callback when printing this node.
-    NoSubstitution = 1 << 2,                // Disables further substitution of an expression.
-    CapturesThis = 1 << 3,                  // The function captures a lexical `this`
-    NoLeadingSourceMap = 1 << 4,            // Do not emit a leading source map location for this node.
-    NoTrailingSourceMap = 1 << 5,           // Do not emit a trailing source map location for this node.
+    MultiLine = 1 << 1,
+    AdviseOnEmitNode = 1 << 2,              // The printer should invoke the onEmitNode callback when printing this node.
+    NoSubstitution = 1 << 3,                // Disables further substitution of an expression.
+    CapturesThis = 1 << 4,                  // The function captures a lexical `this`
+    NoLeadingSourceMap = 1 << 5,            // Do not emit a leading source map location for this node.
+    NoTrailingSourceMap = 1 << 6,           // Do not emit a trailing source map location for this node.
     NoSourceMap = NoLeadingSourceMap | NoTrailingSourceMap, // Do not emit a source map location for this node.
-    NoNestedSourceMaps = 1 << 6,            // Do not emit source map locations for children of this node.
-    NoTokenLeadingSourceMaps = 1 << 7,      // Do not emit leading source map location for token nodes.
-    NoTokenTrailingSourceMaps = 1 << 8,     // Do not emit trailing source map location for token nodes.
+    NoNestedSourceMaps = 1 << 7,            // Do not emit source map locations for children of this node.
+    NoTokenLeadingSourceMaps = 1 << 8,      // Do not emit leading source map location for token nodes.
+    NoTokenTrailingSourceMaps = 1 << 9,     // Do not emit trailing source map location for token nodes.
     NoTokenSourceMaps = NoTokenLeadingSourceMaps | NoTokenTrailingSourceMaps, // Do not emit source map locations for tokens of this node.
-    NoLeadingComments = 1 << 9,             // Do not emit leading comments for this node.
-    NoTrailingComments = 1 << 10,           // Do not emit trailing comments for this node.
+    NoLeadingComments = 1 << 10,            // Do not emit leading comments for this node.
+    NoTrailingComments = 1 << 11,           // Do not emit trailing comments for this node.
     NoComments = NoLeadingComments | NoTrailingComments, // Do not emit comments for this node.
-    NoNestedComments = 1 << 11,
-    HelperName = 1 << 12,                   // The Identifier refers to an *unscoped* emit helper (one that is emitted at the top of the file)
-    ExportName = 1 << 13,                   // Ensure an export prefix is added for an identifier that points to an exported declaration with a local name (see SymbolFlags.ExportHasLocal).
-    LocalName = 1 << 14,                    // Ensure an export prefix is not added for an identifier that points to an exported declaration.
-    InternalName = 1 << 15,                 // The name is internal to an ES5 class body function.
-    Indented = 1 << 16,                     // Adds an explicit extra indentation level for class and function bodies when printing (used to match old emitter).
-    NoIndentation = 1 << 17,                // Do not indent the node.
-    AsyncFunctionBody = 1 << 18,
-    ReuseTempVariableScope = 1 << 19,       // Reuse the existing temp variable scope during emit.
-    CustomPrologue = 1 << 20,               // Treat the statement as if it were a prologue directive (NOTE: Prologue directives are *not* transformed).
-    NoHoisting = 1 << 21,                   // Do not hoist this declaration in --module system
-    HasEndOfDeclarationMarker = 1 << 22,    // Declaration has an associated NotEmittedStatement to mark the end of the declaration
-    Iterator = 1 << 23,                     // The expression to a `yield*` should be treated as an Iterator when down-leveling, not an Iterable.
-    NoAsciiEscaping = 1 << 24,              // When synthesizing nodes that lack an original node or textSourceNode, we want to write the text on the node with ASCII escaping substitutions.
-    /** @internal */ TypeScriptClassWrapper = 1 << 25, // The node is an IIFE class wrapper created by the ts transform.
-    /** @internal */ NeverApplyImportHelper = 1 << 26, // Indicates the node should never be wrapped with an import star helper (because, for example, it imports tslib itself)
-    /** @internal */ IgnoreSourceNewlines = 1 << 27,   // Overrides `printerOptions.preserveSourceNewlines` to print this node (and all descendants) with default whitespace.
-    /** @internal */ Immutable = 1 << 28,      // Indicates a node is a singleton intended to be reused in multiple locations. Any attempt to make further changes to the node will result in an error.
-    /** @internal */ IndirectCall = 1 << 29,   // Emit CallExpression as an indirect call: `(0, f)()`
+    NoNestedComments = 1 << 12,
+    HelperName = 1 << 13,                   // The Identifier refers to an *unscoped* emit helper (one that is emitted at the top of the file)
+    ExportName = 1 << 14,                   // Ensure an export prefix is added for an identifier that points to an exported declaration with a local name (see SymbolFlags.ExportHasLocal).
+    LocalName = 1 << 15,                    // Ensure an export prefix is not added for an identifier that points to an exported declaration.
+    InternalName = 1 << 16,                 // The name is internal to an ES5 class body function.
+    Indented = 1 << 17,                     // Adds an explicit extra indentation level for class and function bodies when printing (used to match old emitter).
+    NoIndentation = 1 << 18,                // Do not indent the node.
+    AsyncFunctionBody = 1 << 19,
+    ReuseTempVariableScope = 1 << 20,       // Reuse the existing temp variable scope during emit.
+    CustomPrologue = 1 << 21,               // Treat the statement as if it were a prologue directive (NOTE: Prologue directives are *not* transformed).
+    NoHoisting = 1 << 22,                   // Do not hoist this declaration in --module system
+    HasEndOfDeclarationMarker = 1 << 23,    // Declaration has an associated NotEmittedStatement to mark the end of the declaration
+    Iterator = 1 << 24,                     // The expression to a `yield*` should be treated as an Iterator when down-leveling, not an Iterable.
+    NoAsciiEscaping = 1 << 25,              // When synthesizing nodes that lack an original node or textSourceNode, we want to write the text on the node with ASCII escaping substitutions.
+    /** @internal */ TypeScriptClassWrapper = 1 << 26, // The node is an IIFE class wrapper created by the ts transform.
+    /** @internal */ NeverApplyImportHelper = 1 << 27, // Indicates the node should never be wrapped with an import star helper (because, for example, it imports tslib itself)
+    /** @internal */ IgnoreSourceNewlines = 1 << 28,   // Overrides `printerOptions.preserveSourceNewlines` to print this node (and all descendants) with default whitespace.
+    /** @internal */ Immutable = 1 << 29,      // Indicates a node is a singleton intended to be reused in multiple locations. Any attempt to make further changes to the node will result in an error.
+    /** @internal */ IndirectCall = 1 << 30,   // Emit CallExpression as an indirect call: `(0, f)()`
 }
 
 export interface EmitHelperBase {
diff --git a/src/services/organizeImports.ts b/src/services/organizeImports.ts
@@ -7,6 +7,7 @@ import {
     compareValues,
     Comparison,
     createScanner,
+    EmitFlags,
     emptyArray,
     ExportDeclaration,
     ExportSpecifier,
@@ -43,7 +44,9 @@ import {
     NamespaceImport,
     OrganizeImportsMode,
     Program,
+    rangeIsOnSingleLine,
     Scanner,
+    setEmitFlags,
     some,
     SortedReadonlyArray,
     SourceFile,
@@ -79,7 +82,7 @@ export function organizeImports(
     const maybeRemove = shouldRemove ? removeUnusedImports : identity;
     const maybeCoalesce = shouldCombine ? coalesceImports : identity;
     const processImportsOfSameModuleSpecifier = (importGroup: readonly ImportDeclaration[]) => {
-        const processedDeclarations = maybeCoalesce(maybeRemove(importGroup, sourceFile, program));
+        const processedDeclarations = maybeCoalesce(maybeRemove(importGroup, sourceFile, program), sourceFile);
         return shouldSort
             ? stableSort(processedDeclarations, (s1, s2) => compareImportsOrRequireStatements(s1, s2))
             : processedDeclarations;
@@ -296,7 +299,7 @@ function getExternalModuleName(specifier: Expression) {
  *
  * @internal
  */
-export function coalesceImports(importGroup: readonly ImportDeclaration[]) {
+export function coalesceImports(importGroup: readonly ImportDeclaration[], sourceFile?: SourceFile) {
     if (importGroup.length === 0) {
         return importGroup;
     }
@@ -350,7 +353,11 @@ export function coalesceImports(importGroup: readonly ImportDeclaration[]) {
 
         newImportSpecifiers.push(...getNewImportSpecifiers(namedImports));
 
-        const sortedImportSpecifiers = sortSpecifiers(newImportSpecifiers);
+        const sortedImportSpecifiers = factory.createNodeArray(
+            sortSpecifiers(newImportSpecifiers),
+            (namedImports[0]?.importClause!.namedBindings as NamedImports)?.elements.hasTrailingComma
+        );
+
         const importDecl = defaultImports.length > 0
             ? defaultImports[0]
             : namedImports[0];
@@ -363,6 +370,14 @@ export function coalesceImports(importGroup: readonly ImportDeclaration[]) {
                 ? factory.createNamedImports(sortedImportSpecifiers)
                 : factory.updateNamedImports(namedImports[0].importClause!.namedBindings as NamedImports, sortedImportSpecifiers); // TODO: GH#18217
 
+        if (sourceFile &&
+            newNamedImports &&
+            namedImports[0]?.importClause!.namedBindings &&
+            !rangeIsOnSingleLine(namedImports[0].importClause.namedBindings, sourceFile)
+        ) {
+            setEmitFlags(newNamedImports, EmitFlags.MultiLine);
+        }
+
         // Type-only imports are not allowed to mix default, namespace, and named imports in any combination.
         // We could rewrite a default import as a named import (`import { default as name }`), but we currently
         // choose not to as a stylistic preference.
diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts
@@ -7367,33 +7367,34 @@ declare namespace ts {
     enum EmitFlags {
         None = 0,
         SingleLine = 1,
-        AdviseOnEmitNode = 2,
-        NoSubstitution = 4,
-        CapturesThis = 8,
-        NoLeadingSourceMap = 16,
-        NoTrailingSourceMap = 32,
-        NoSourceMap = 48,
-        NoNestedSourceMaps = 64,
-        NoTokenLeadingSourceMaps = 128,
-        NoTokenTrailingSourceMaps = 256,
-        NoTokenSourceMaps = 384,
-        NoLeadingComments = 512,
-        NoTrailingComments = 1024,
-        NoComments = 1536,
-        NoNestedComments = 2048,
-        HelperName = 4096,
-        ExportName = 8192,
-        LocalName = 16384,
-        InternalName = 32768,
-        Indented = 65536,
-        NoIndentation = 131072,
-        AsyncFunctionBody = 262144,
-        ReuseTempVariableScope = 524288,
-        CustomPrologue = 1048576,
-        NoHoisting = 2097152,
-        HasEndOfDeclarationMarker = 4194304,
-        Iterator = 8388608,
-        NoAsciiEscaping = 16777216
+        MultiLine = 2,
+        AdviseOnEmitNode = 4,
+        NoSubstitution = 8,
+        CapturesThis = 16,
+        NoLeadingSourceMap = 32,
+        NoTrailingSourceMap = 64,
+        NoSourceMap = 96,
+        NoNestedSourceMaps = 128,
+        NoTokenLeadingSourceMaps = 256,
+        NoTokenTrailingSourceMaps = 512,
+        NoTokenSourceMaps = 768,
+        NoLeadingComments = 1024,
+        NoTrailingComments = 2048,
+        NoComments = 3072,
+        NoNestedComments = 4096,
+        HelperName = 8192,
+        ExportName = 16384,
+        LocalName = 32768,
+        InternalName = 65536,
+        Indented = 131072,
+        NoIndentation = 262144,
+        AsyncFunctionBody = 524288,
+        ReuseTempVariableScope = 1048576,
+        CustomPrologue = 2097152,
+        NoHoisting = 4194304,
+        HasEndOfDeclarationMarker = 8388608,
+        Iterator = 16777216,
+        NoAsciiEscaping = 33554432
     }
     interface EmitHelperBase {
         readonly name: string;
diff --git a/tests/baselines/reference/api/typescript.d.ts b/tests/baselines/reference/api/typescript.d.ts
@@ -3431,33 +3431,34 @@ declare namespace ts {
     enum EmitFlags {
         None = 0,
         SingleLine = 1,
-        AdviseOnEmitNode = 2,
-        NoSubstitution = 4,
-        CapturesThis = 8,
-        NoLeadingSourceMap = 16,
-        NoTrailingSourceMap = 32,
-        NoSourceMap = 48,
-        NoNestedSourceMaps = 64,
-        NoTokenLeadingSourceMaps = 128,
-        NoTokenTrailingSourceMaps = 256,
-        NoTokenSourceMaps = 384,
-        NoLeadingComments = 512,
-        NoTrailingComments = 1024,
-        NoComments = 1536,
-        NoNestedComments = 2048,
-        HelperName = 4096,
-        ExportName = 8192,
-        LocalName = 16384,
-        InternalName = 32768,
-        Indented = 65536,
-        NoIndentation = 131072,
-        AsyncFunctionBody = 262144,
-        ReuseTempVariableScope = 524288,
-        CustomPrologue = 1048576,
-        NoHoisting = 2097152,
-        HasEndOfDeclarationMarker = 4194304,
-        Iterator = 8388608,
-        NoAsciiEscaping = 16777216
+        MultiLine = 2,
+        AdviseOnEmitNode = 4,
+        NoSubstitution = 8,
+        CapturesThis = 16,
+        NoLeadingSourceMap = 32,
+        NoTrailingSourceMap = 64,
+        NoSourceMap = 96,
+        NoNestedSourceMaps = 128,
+        NoTokenLeadingSourceMaps = 256,
+        NoTokenTrailingSourceMaps = 512,
+        NoTokenSourceMaps = 768,
+        NoLeadingComments = 1024,
+        NoTrailingComments = 2048,
+        NoComments = 3072,
+        NoNestedComments = 4096,
+        HelperName = 8192,
+        ExportName = 16384,
+        LocalName = 32768,
+        InternalName = 65536,
+        Indented = 131072,
+        NoIndentation = 262144,
+        AsyncFunctionBody = 524288,
+        ReuseTempVariableScope = 1048576,
+        CustomPrologue = 2097152,
+        NoHoisting = 4194304,
+        HasEndOfDeclarationMarker = 8388608,
+        Iterator = 16777216,
+        NoAsciiEscaping = 33554432
     }
     interface EmitHelperBase {
         readonly name: string;
diff --git a/tests/cases/fourslash/organizeImports1.ts b/tests/cases/fourslash/organizeImports1.ts
@@ -20,11 +20,18 @@
 
 verify.organizeImports(
 `import {
-    a, b,
-    b as B, c,
-    c as C, d, d as D, e, f,
-    f as F, g,
-    g as G, h, h as H
+    a,
+    b,
+    b as B,
+    c,
+    c as C,
+    d, d as D,
+    e,
+    f,
+    f as F,
+    g,
+    g as G,
+    h, h as H
 } from './foo';
 
 console.log(a, B, b, c, C, d, D);
diff --git a/tests/cases/fourslash/organizeImports13.ts b/tests/cases/fourslash/organizeImports13.ts
@@ -0,0 +1,49 @@
+/// <reference path="fourslash.ts" />
+
+////import {
+////    Type1,
+////    Type2,
+////    func4,
+////    Type3,
+////    Type4,
+////    Type5,
+////    Type7,
+////    Type8,
+////    Type9,
+////    func1,
+////    func2,
+////    Type6,
+////    func3,
+////    func5,
+////    func6,
+////    func7,
+////    func8,
+////    func9,
+////} from "foo";
+////interface Use extends Type1, Type2, Type3, Type4, Type5, Type6, Type7, Type8, Type9 {}
+////console.log(func1, func2, func3, func4, func5, func6, func7, func8, func9);
+
+verify.organizeImports(
+`import {
+    func1,
+    func2,
+    func3,
+    func4,
+    func5,
+    func6,
+    func7,
+    func8,
+    func9,
+    Type1,
+    Type2,
+    Type3,
+    Type4,
+    Type5,
+    Type6,
+    Type7,
+    Type8,
+    Type9,
+} from "foo";
+interface Use extends Type1, Type2, Type3, Type4, Type5, Type6, Type7, Type8, Type9 {}
+console.log(func1, func2, func3, func4, func5, func6, func7, func8, func9);`
+);
diff --git a/tests/cases/fourslash/organizeImports2.ts b/tests/cases/fourslash/organizeImports2.ts
@@ -10,7 +10,10 @@
 //// console.log(Foo, Bar);
 
 verify.organizeImports(
-`import { Bar, Foo } from "foo";
+`import {
+    Bar,
+    Foo
+} from "foo";
 
 console.log(Foo, Bar);`
 ); 
diff --git a/tests/cases/fourslash/organizeImportsNoFormatOptions.ts b/tests/cases/fourslash/organizeImportsNoFormatOptions.ts
@@ -16,7 +16,7 @@ format.setFormatOptions({});
 // Default newline is carriage return.
 // See `getNewLineOrDefaultFromHost()` in services/utilities.ts.
 verify.organizeImports(
-`import {\r\nstat,\r\nstatSync\r\n} from "fs";\r\nexport function fakeFn() {
+`import {\r\nstat,\r\nstatSync,\r\n} from "fs";\r\nexport function fakeFn() {
   stat;
   statSync;
 }`);

Original file line number	Diff line number	Diff line change
`@@ -4862,7 +4862,14 @@ export function createPrinter(printerOptions: PrinterOptions = {}, handlers: Pri`
`4862`	`4862`	`}`
`4863`	`4863`
`4864`	`4864`	`function emitList(parentNode: Node \| undefined, children: NodeArray<Node> \| undefined, format: ListFormat, parenthesizerRule?: ParenthesizerRuleOrSelector<Node>, start?: number, count?: number) {`
`4865`		`- emitNodeList(emit, parentNode, children, format, parenthesizerRule, start, count);`
	`4865`	`+ emitNodeList(`
	`4866`	`+ emit,`
	`4867`	`+ parentNode,`
	`4868`	`+ children,`
	`4869`	`+ format \| (parentNode && getEmitFlags(parentNode) & EmitFlags.MultiLine ? ListFormat.PreferNewLine : 0),`
	`4870`	`+ parenthesizerRule,`
	`4871`	`+ start,`
	`4872`	`+ count);`
`4866`	`4873`	`}`
`4867`	`4874`
`4868`	`4875`	`function emitExpressionList(parentNode: Node \| undefined, children: NodeArray<Node> \| undefined, format: ListFormat, parenthesizerRule?: ParenthesizerRuleOrSelector<Expression>, start?: number, count?: number) {`