Insert auto-imports in sorted order (#39394)

* Sort auto-imports

* Avoid re-checking sort all the time

* Add comment

Author: andrewbranch

src/compiler/core.ts

@@ -819,6 +819,18 @@ namespace ts {
         return deduplicateSorted(sort(array, comparer), equalityComparer || comparer || compareStringsCaseSensitive as any as Comparer<T>);
     }
 
+    export function arrayIsSorted<T>(array: readonly T[], comparer: Comparer<T>) {
+        if (array.length < 2) return true;
+        let prevElement = array[0];
+        for (const element of array.slice(1)) {
+            if (comparer(prevElement, element) === Comparison.GreaterThan) {
+                return false;
+            }
+            prevElement = element;
+        }
+        return true;
+    }
+
     export function arrayIsEqualTo<T>(array1: readonly T[] | undefined, array2: readonly T[] | undefined, equalityComparer: (a: T, b: T, index: number) => boolean = equateValues): boolean {
         if (!array1 || !array2) {
             return array1 === array2;

src/compiler/types.ts

@@ -4336,6 +4336,9 @@ namespace ts {
     /* @internal */
     export type AnyImportOrRequire = AnyImportSyntax | RequireVariableDeclaration;
 
+    /* @internal */
+    export type AnyImportOrRequireStatement = AnyImportSyntax | RequireVariableStatement;
+
 
     /* @internal */
     export type AnyImportOrReExport = AnyImportSyntax | ExportDeclaration;
@@ -4357,8 +4360,17 @@ namespace ts {
 
     /* @internal */
     export interface RequireVariableDeclaration extends VariableDeclaration {
+        readonly initializer: RequireOrImportCall;
+    }
 
-        initializer: RequireOrImportCall;
+    /* @internal */
+    export interface RequireVariableStatement extends VariableStatement {
+        readonly declarationList: RequireVariableDeclarationList;
+    }
+
+    /* @internal */
+    export interface RequireVariableDeclarationList extends VariableDeclarationList {
+        readonly declarations: NodeArray<RequireVariableDeclaration>;
     }
 
     /* @internal */

src/compiler/utilities.ts

@@ -1934,8 +1934,10 @@ namespace ts {
         return isVariableDeclaration(node) && !!node.initializer && isRequireCall(node.initializer, requireStringLiteralLikeArgument);
     }
 
-    export function isRequireVariableDeclarationStatement(node: Node, requireStringLiteralLikeArgument = true): node is VariableStatement {
-        return isVariableStatement(node) && every(node.declarationList.declarations, decl => isRequireVariableDeclaration(decl, requireStringLiteralLikeArgument));
+    export function isRequireVariableStatement(node: Node, requireStringLiteralLikeArgument = true): node is RequireVariableStatement {
+        return isVariableStatement(node)
+            && node.declarationList.declarations.length > 0
+            && every(node.declarationList.declarations, decl => isRequireVariableDeclaration(decl, requireStringLiteralLikeArgument));
     }
 
     export function isSingleOrDoubleQuote(charCode: number) {

src/services/codefixes/importFixes.ts

@@ -138,7 +138,7 @@ namespace ts.codefix {
                 doAddExistingFix(changeTracker, sourceFile, importClauseOrBindingPattern, defaultImport, namedImports, canUseTypeOnlyImport);
             });
 
-            let newDeclarations: Statement | readonly Statement[] | undefined;
+            let newDeclarations: AnyImportOrRequireStatement | readonly AnyImportOrRequireStatement[] | undefined;
             newImports.forEach(({ useRequire, ...imports }, moduleSpecifier) => {
                 const getDeclarations = useRequire ? getNewRequires : getNewImports;
                 newDeclarations = combine(newDeclarations, getDeclarations(moduleSpecifier, quotePreference, imports));
@@ -671,15 +671,35 @@ namespace ts.codefix {
         }
 
         if (namedImports.length) {
-            const specifiers = namedImports.map(name => factory.createImportSpecifier(/*propertyName*/ undefined, factory.createIdentifier(name)));
-            if (clause.namedBindings && cast(clause.namedBindings, isNamedImports).elements.length) {
-                for (const spec of specifiers) {
-                    changes.insertNodeInListAfter(sourceFile, last(cast(clause.namedBindings, isNamedImports).elements), spec);
+            const existingSpecifiers = clause.namedBindings && cast(clause.namedBindings, isNamedImports).elements;
+            const newSpecifiers = stableSort(
+                namedImports.map(name => factory.createImportSpecifier(/*propertyName*/ undefined, factory.createIdentifier(name))),
+                OrganizeImports.compareImportOrExportSpecifiers);
+
+            if (existingSpecifiers?.length && OrganizeImports.importSpecifiersAreSorted(existingSpecifiers)) {
+                for (const spec of newSpecifiers) {
+                    const insertionIndex = OrganizeImports.getImportSpecifierInsertionIndex(existingSpecifiers, spec);
+                    const prevSpecifier = (clause.namedBindings as NamedImports).elements[insertionIndex - 1];
+                    if (prevSpecifier) {
+                        changes.insertNodeInListAfter(sourceFile, prevSpecifier, spec);
+                    }
+                    else {
+                        changes.insertNodeBefore(
+                            sourceFile,
+                            existingSpecifiers[0],
+                            spec,
+                            !positionsAreOnSameLine(existingSpecifiers[0].getStart(), clause.parent.getStart(), sourceFile));
+                    }
+                }
+            }
+            else if (existingSpecifiers?.length) {
+                for (const spec of newSpecifiers) {
+                    changes.insertNodeAtEndOfList(sourceFile, existingSpecifiers, spec);
                 }
             }
             else {
-                if (specifiers.length) {
-                    const namedImports = factory.createNamedImports(specifiers);
+                if (newSpecifiers.length) {
+                    const namedImports = factory.createNamedImports(newSpecifiers);
                     if (clause.namedBindings) {
                         changes.replaceNode(sourceFile, clause.namedBindings, namedImports);
                     }
@@ -727,9 +747,9 @@ namespace ts.codefix {
             readonly name: string;
         };
     }
-    function getNewImports(moduleSpecifier: string, quotePreference: QuotePreference, imports: ImportsCollection): Statement | readonly Statement[] {
+    function getNewImports(moduleSpecifier: string, quotePreference: QuotePreference, imports: ImportsCollection): AnyImportSyntax | readonly AnyImportSyntax[] {
         const quotedModuleSpecifier = makeStringLiteral(moduleSpecifier, quotePreference);
-        let statements: Statement | readonly Statement[] | undefined;
+        let statements: AnyImportSyntax | readonly AnyImportSyntax[] | undefined;
         if (imports.defaultImport !== undefined || imports.namedImports?.length) {
             statements = combine(statements, makeImport(
                 imports.defaultImport === undefined ? undefined : factory.createIdentifier(imports.defaultImport),
@@ -756,9 +776,9 @@ namespace ts.codefix {
         return Debug.checkDefined(statements);
     }
 
-    function getNewRequires(moduleSpecifier: string, quotePreference: QuotePreference, imports: ImportsCollection): Statement | readonly Statement[] {
+    function getNewRequires(moduleSpecifier: string, quotePreference: QuotePreference, imports: ImportsCollection): RequireVariableStatement | readonly RequireVariableStatement[] {
         const quotedModuleSpecifier = makeStringLiteral(moduleSpecifier, quotePreference);
-        let statements: Statement | readonly Statement[] | undefined;
+        let statements: RequireVariableStatement | readonly RequireVariableStatement[] | undefined;
         // const { default: foo, bar, etc } = require('./mod');
         if (imports.defaultImport || imports.namedImports?.length) {
             const bindingElements = imports.namedImports?.map(name => factory.createBindingElement(/*dotDotDotToken*/ undefined, /*propertyName*/ undefined, name)) || [];
@@ -776,7 +796,7 @@ namespace ts.codefix {
         return Debug.checkDefined(statements);
     }
 
-    function createConstEqualsRequireDeclaration(name: string | ObjectBindingPattern, quotedModuleSpecifier: StringLiteral): VariableStatement {
+    function createConstEqualsRequireDeclaration(name: string | ObjectBindingPattern, quotedModuleSpecifier: StringLiteral): RequireVariableStatement {
         return factory.createVariableStatement(
             /*modifiers*/ undefined,
             factory.createVariableDeclarationList([
@@ -785,7 +805,7 @@ namespace ts.codefix {
                     /*exclamationToken*/ undefined,
                     /*type*/ undefined,
                     factory.createCallExpression(factory.createIdentifier("require"), /*typeArguments*/ undefined, [quotedModuleSpecifier]))],
-                NodeFlags.Const));
+                NodeFlags.Const)) as RequireVariableStatement;
     }
 
     function symbolHasMeaning({ declarations }: Symbol, meaning: SemanticMeaning): boolean {

src/services/organizeImports.ts

@@ -17,7 +17,9 @@ namespace ts.OrganizeImports {
 
         const changeTracker = textChanges.ChangeTracker.fromContext({ host, formatContext, preferences });
 
-        const coalesceAndOrganizeImports = (importGroup: readonly ImportDeclaration[]) => coalesceImports(removeUnusedImports(importGroup, sourceFile, program));
+        const coalesceAndOrganizeImports = (importGroup: readonly ImportDeclaration[]) => stableSort(
+            coalesceImports(removeUnusedImports(importGroup, sourceFile, program)),
+            (s1, s2) => compareImportsOrRequireStatements(s1, s2));
 
         // All of the old ImportDeclarations in the file, in syntactic order.
         const topLevelImportDecls = sourceFile.statements.filter(isImportDeclaration);
@@ -55,7 +57,7 @@ namespace ts.OrganizeImports {
             suppressLeadingTrivia(oldImportDecls[0]);
 
             const oldImportGroups = group(oldImportDecls, importDecl => getExternalModuleName(importDecl.moduleSpecifier!)!);
-            const sortedImportGroups = stableSort(oldImportGroups, (group1, group2) => compareModuleSpecifiers(group1[0].moduleSpecifier!, group2[0].moduleSpecifier!));
+            const sortedImportGroups = stableSort(oldImportGroups, (group1, group2) => compareModuleSpecifiers(group1[0].moduleSpecifier, group2[0].moduleSpecifier));
             const newImportDecls = flatMap(sortedImportGroups, importGroup =>
                 getExternalModuleName(importGroup[0].moduleSpecifier!)
                     ? coalesce(importGroup)
@@ -395,15 +397,18 @@ namespace ts.OrganizeImports {
     }
 
     function sortSpecifiers<T extends ImportOrExportSpecifier>(specifiers: readonly T[]) {
-        return stableSort(specifiers, (s1, s2) =>
-            compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name) ||
-            compareIdentifiers(s1.name, s2.name));
+        return stableSort(specifiers, compareImportOrExportSpecifiers);
+    }
+
+    export function compareImportOrExportSpecifiers<T extends ImportOrExportSpecifier>(s1: T, s2: T) {
+        return compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name)
+            || compareIdentifiers(s1.name, s2.name);
     }
 
     /* internal */ // Exported for testing
-    export function compareModuleSpecifiers(m1: Expression, m2: Expression) {
-        const name1 = getExternalModuleName(m1);
-        const name2 = getExternalModuleName(m2);
+    export function compareModuleSpecifiers(m1: Expression | undefined, m2: Expression | undefined) {
+        const name1 = m1 === undefined ? undefined : getExternalModuleName(m1);
+        const name2 = m2 === undefined ? undefined : getExternalModuleName(m2);
         return compareBooleans(name1 === undefined, name2 === undefined) ||
             compareBooleans(isExternalModuleNameRelative(name1!), isExternalModuleNameRelative(name2!)) ||
             compareStringsCaseInsensitive(name1!, name2!);
@@ -412,4 +417,63 @@ namespace ts.OrganizeImports {
     function compareIdentifiers(s1: Identifier, s2: Identifier) {
         return compareStringsCaseInsensitive(s1.text, s2.text);
     }
+
+    function getModuleSpecifierExpression(declaration: AnyImportOrRequireStatement): Expression | undefined {
+        switch (declaration.kind) {
+            case SyntaxKind.ImportEqualsDeclaration:
+                return tryCast(declaration.moduleReference, isExternalModuleReference)?.expression;
+            case SyntaxKind.ImportDeclaration:
+                return declaration.moduleSpecifier;
+            case SyntaxKind.VariableStatement:
+                return declaration.declarationList.declarations[0].initializer.arguments[0];
+        }
+    }
+
+    export function importsAreSorted(imports: readonly AnyImportOrRequireStatement[]): imports is SortedReadonlyArray<AnyImportOrRequireStatement> {
+        return arrayIsSorted(imports, compareImportsOrRequireStatements);
+    }
+
+    export function importSpecifiersAreSorted(imports: readonly ImportSpecifier[]): imports is SortedReadonlyArray<ImportSpecifier> {
+        return arrayIsSorted(imports, compareImportOrExportSpecifiers);
+    }
+
+    export function getImportDeclarationInsertionIndex(sortedImports: SortedReadonlyArray<AnyImportOrRequireStatement>, newImport: AnyImportOrRequireStatement) {
+        const index = binarySearch(sortedImports, newImport, identity, compareImportsOrRequireStatements);
+        return index < 0 ? ~index : index;
+    }
+
+    export function getImportSpecifierInsertionIndex(sortedImports: SortedReadonlyArray<ImportSpecifier>, newImport: ImportSpecifier) {
+        const index = binarySearch(sortedImports, newImport, identity, compareImportOrExportSpecifiers);
+        return index < 0 ? ~index : index;
+    }
+
+    export function compareImportsOrRequireStatements(s1: AnyImportOrRequireStatement, s2: AnyImportOrRequireStatement) {
+        return compareModuleSpecifiers(getModuleSpecifierExpression(s1), getModuleSpecifierExpression(s2)) || compareImportKind(s1, s2);
+    }
+
+    function compareImportKind(s1: AnyImportOrRequireStatement, s2: AnyImportOrRequireStatement) {
+        return compareValues(getImportKindOrder(s1), getImportKindOrder(s2));
+    }
+
+    // 1. Side-effect imports
+    // 2. Type-only imports
+    // 3. Namespace imports
+    // 4. Default imports
+    // 5. Named imports
+    // 6. ImportEqualsDeclarations
+    // 7. Require variable statements
+    function getImportKindOrder(s1: AnyImportOrRequireStatement) {
+        switch (s1.kind) {
+            case SyntaxKind.ImportDeclaration:
+                if (!s1.importClause) return 0;
+                if (s1.importClause.isTypeOnly) return 1;
+                if (s1.importClause.namedBindings?.kind === SyntaxKind.NamespaceImport) return 2;
+                if (s1.importClause.name) return 3;
+                return 4;
+            case SyntaxKind.ImportEqualsDeclaration:
+                return 5;
+            case SyntaxKind.VariableStatement:
+                return 6;
+        }
+    }
 }

src/services/refactors/moveToNewFile.ts

@@ -269,7 +269,7 @@ namespace ts.refactor {
         | ImportEqualsDeclaration
         | VariableStatement;
 
-    function createOldFileImportsFromNewFile(newFileNeedExport: ReadonlySymbolSet, newFileNameWithExtension: string, useEs6Imports: boolean, quotePreference: QuotePreference): Statement | undefined {
+    function createOldFileImportsFromNewFile(newFileNeedExport: ReadonlySymbolSet, newFileNameWithExtension: string, useEs6Imports: boolean, quotePreference: QuotePreference): AnyImportOrRequireStatement | undefined {
         let defaultImport: Identifier | undefined;
         const imports: string[] = [];
         newFileNeedExport.forEach(symbol => {
@@ -283,7 +283,7 @@ namespace ts.refactor {
         return makeImportOrRequire(defaultImport, imports, newFileNameWithExtension, useEs6Imports, quotePreference);
     }
 
-    function makeImportOrRequire(defaultImport: Identifier | undefined, imports: readonly string[], path: string, useEs6Imports: boolean, quotePreference: QuotePreference): Statement | undefined {
+    function makeImportOrRequire(defaultImport: Identifier | undefined, imports: readonly string[], path: string, useEs6Imports: boolean, quotePreference: QuotePreference): AnyImportOrRequireStatement | undefined {
         path = ensurePathIsNonModuleName(path);
         if (useEs6Imports) {
             const specifiers = imports.map(i => factory.createImportSpecifier(/*propertyName*/ undefined, factory.createIdentifier(i)));
@@ -293,7 +293,7 @@ namespace ts.refactor {
             Debug.assert(!defaultImport, "No default import should exist"); // If there's a default export, it should have been an es6 module.
             const bindingElements = imports.map(i => factory.createBindingElement(/*dotDotDotToken*/ undefined, /*propertyName*/ undefined, i));
             return bindingElements.length
-                ? makeVariableStatement(factory.createObjectBindingPattern(bindingElements), /*type*/ undefined, createRequireCall(factory.createStringLiteral(path)))
+                ? makeVariableStatement(factory.createObjectBindingPattern(bindingElements), /*type*/ undefined, createRequireCall(factory.createStringLiteral(path))) as RequireVariableStatement
                 : undefined;
         }
     }

src/services/textChanges.ts

@@ -472,9 +472,9 @@ namespace ts.textChanges {
             this.insertNodesAt(sourceFile, start, typeParameters, { prefix: "<", suffix: ">" });
         }
 
-        private getOptionsForInsertNodeBefore(before: Node, inserted: Node, doubleNewlines: boolean): InsertNodeOptions {
+        private getOptionsForInsertNodeBefore(before: Node, inserted: Node, blankLineBetween: boolean): InsertNodeOptions {
             if (isStatement(before) || isClassElement(before)) {
-                return { suffix: doubleNewlines ? this.newLineCharacter + this.newLineCharacter : this.newLineCharacter };
+                return { suffix: blankLineBetween ? this.newLineCharacter + this.newLineCharacter : this.newLineCharacter };
             }
             else if (isVariableDeclaration(before)) { // insert `x = 1, ` into `const x = 1, y = 2;
                 return { suffix: ", " };
@@ -485,6 +485,9 @@ namespace ts.textChanges {
             else if (isStringLiteral(before) && isImportDeclaration(before.parent) || isNamedImports(before)) {
                 return { suffix: ", " };
             }
+            else if (isImportSpecifier(before)) {
+                return { suffix: "," + (blankLineBetween ? this.newLineCharacter : " ") };
+            }
             return Debug.failBadSyntaxKind(before); // We haven't handled this kind of node yet -- add it
         }