Address PR feedback

Eliminate cancellation token

Add organizeImports.ts to tsconfig.json

Simplify ts.OrganizeImports.organizeImports

Simplify sortImports

Semantic change: all invalid module specifiers are now considered to be
equal.

Simplify comparisons using ||

Pull out imports with invalid modules specifiers

...for separate processing.  They are tacked on to the end of the
organized imports in their original order.

Bonus: downstream functions can now assume imports have valid module
specifiers.

Rename baseline folder with leading lowercase

Simplify coalesceImports

Remove some unnecessary null checks

Simplify baseline generation

Author: amcasey

src/compiler/core.ts

@@ -1897,6 +1897,11 @@ namespace ts {
             Comparison.EqualTo;
     }
 
+    /** True is greater than false. */
+    export function compareBooleans(a: boolean, b: boolean): Comparison {
+        return compareValues(a ? 1 : 0, b ? 1 : 0);
+    }
+
     function compareMessageText(text1: string | DiagnosticMessageChain, text2: string | DiagnosticMessageChain): Comparison {
         while (text1 && text2) {
             // We still have both chains.

src/harness/unittests/organizeImports.ts

@@ -54,34 +54,12 @@ namespace ts {
                     `import x from "./lib2";`);
             });
 
-            it("Sort - invalid vs invalid", () => {
-                assertSortsBefore(
-                    // tslint:disable-next-line no-invalid-template-strings
-                    "import y from `${'lib1'}`;",
-                    // tslint:disable-next-line no-invalid-template-strings
-                    "import x from `${'lib2'}`;");
-            });
-
             it("Sort - relative vs non-relative", () => {
                 assertSortsBefore(
                     `import y from "lib";`,
                     `import x from "./lib";`);
             });
 
-            it("Sort - non-relative vs invalid", () => {
-                assertSortsBefore(
-                    `import y from "lib";`,
-                    // tslint:disable-next-line no-invalid-template-strings
-                    "import x from `${'lib'}`;");
-            });
-
-            it("Sort - relative vs invalid", () => {
-                assertSortsBefore(
-                    `import y from "./lib";`,
-                    // tslint:disable-next-line no-invalid-template-strings
-                    "import x from `${'lib'}`;");
-            });
-
             function assertUnaffectedBySort(...importStrings: string[]) {
                 const unsortedImports1 = parseImports(...importStrings);
                 assertListEqual(unsortedImports1, OrganizeImports.sortImports(unsortedImports1));
@@ -282,6 +260,23 @@ import * as NS from "lib";
 NS.F1();
 import D from "lib";
 D();
+`,
+                },
+                libFile);
+
+            testOrganizeImports("MoveToTop_Invalid",
+                {
+                    path: "/test.ts",
+                    content: `
+import { F1, F2 } from "lib";
+F1();
+F2();
+import * as NS from "lib";
+NS.F1();
+import b from ${"`${'lib'}`"};
+import a from ${"`${'lib'}`"};
+import D from "lib";
+D();
 `,
                 },
                 libFile);
@@ -311,7 +306,7 @@ F2();
                 { path: "/lib2.ts", content: "" });
 
             function testOrganizeImports(testName: string, testFile: TestFSWithWatch.FileOrFolder, ...otherFiles: TestFSWithWatch.FileOrFolder[]) {
-                it(testName, () => runBaseline(`OrganizeImports/${testName}.ts`, testFile, ...otherFiles));
+                it(testName, () => runBaseline(`organizeImports/${testName}.ts`, testFile, ...otherFiles));
             }
 
             function runBaseline(baselinePath: string, testFile: TestFSWithWatch.FileOrFolder, ...otherFiles: TestFSWithWatch.FileOrFolder[]) {
@@ -322,15 +317,13 @@ F2();
                 assert.equal(testPath, changes[0].fileName);
 
                 Harness.Baseline.runBaseline(baselinePath, () => {
-                    const data: string[] = [];
-                    data.push(`// ==ORIGINAL==`);
-                    data.push(testContent);
-
-                    data.push(`// ==ORGANIZED==`);
                     const newText = textChanges.applyChanges(testContent, changes[0].textChanges);
-                    data.push(newText);
-
-                    return data.join(newLineCharacter);
+                    return [
+                        "// ==ORIGINAL==",
+                        testContent,
+                        "// ==ORGANIZED==",
+                        newText,
+                    ].join(newLineCharacter);
                 });
             }
 

src/services/organizeImports.ts

@@ -3,56 +3,44 @@ namespace ts.OrganizeImports {
     export function organizeImports(
         sourceFile: SourceFile,
         formatContext: formatting.FormatContext,
-        host: LanguageServiceHost,
-        cancellationToken: CancellationToken) {
+        host: LanguageServiceHost) {
 
-        // All of the (old) ImportDeclarations in the file, in syntactic order.
-        const oldImportDecls: ImportDeclaration[] = [];
+        // TODO (https://github.com/Microsoft/TypeScript/issues/10020): sort *within* ambient modules (find using isAmbientModule)
 
-        forEachChild(sourceFile, node => {
-            cancellationToken.throwIfCancellationRequested();
-            if (isImportDeclaration(node)) {
-                oldImportDecls.push(node);
-            }
-            // TODO (https://github.com/Microsoft/TypeScript/issues/10020): sort *within* ambient modules (find using isAmbientModule)
-        });
+        // All of the old ImportDeclarations in the file, in syntactic order.
+        const oldImportDecls = sourceFile.statements.filter(isImportDeclaration);
 
         if (oldImportDecls.length === 0) {
             return [];
         }
 
-        const usedImportDecls = removeUnusedImports(oldImportDecls);
-        cancellationToken.throwIfCancellationRequested();
-        const sortedImportDecls = sortImports(usedImportDecls);
-        cancellationToken.throwIfCancellationRequested();
-        const coalescedImportDecls = coalesceImports(sortedImportDecls);
-        cancellationToken.throwIfCancellationRequested();
+        const oldValidImportDecls = oldImportDecls.filter(importDecl => getExternalModuleName(importDecl.moduleSpecifier));
+        const oldInvalidImportDecls = oldImportDecls.filter(importDecl => !getExternalModuleName(importDecl.moduleSpecifier));
 
-        // All of the (new) ImportDeclarations in the file, in sorted order.
-        const newImportDecls = coalescedImportDecls;
+        // All of the new ImportDeclarations in the file, in sorted order.
+        const newImportDecls = coalesceImports(sortImports(removeUnusedImports(oldValidImportDecls))).concat(oldInvalidImportDecls);
 
         const changeTracker = textChanges.ChangeTracker.fromContext({ host, formatContext });
 
-        // NB: Stopping before i === 0
-        for (let i = oldImportDecls.length - 1; i > 0; i--) {
-            changeTracker.deleteNode(sourceFile, oldImportDecls[i]);
-        }
-
+        // Delete or replace the first import.
         if (newImportDecls.length === 0) {
             changeTracker.deleteNode(sourceFile, oldImportDecls[0]);
         }
         else {
-            // Delete the surrounding trivia because it will have been retained in newImportDecls.
-            const replaceOptions = {
+            // Note: Delete the surrounding trivia because it will have been retained in newImportDecls.
+            changeTracker.replaceNodeWithNodes(sourceFile, oldImportDecls[0], newImportDecls, {
                 useNonAdjustedStartPosition: false,
                 useNonAdjustedEndPosition: false,
                 suffix: getNewLineOrDefaultFromHost(host, formatContext.options),
-             };
-            changeTracker.replaceNodeWithNodes(sourceFile, oldImportDecls[0], newImportDecls, replaceOptions);
+            });
         }
 
-        const changes = changeTracker.getChanges();
-        return changes;
+        // Delete any subsequent imports.
+        for (let i = 1; i < oldImportDecls.length; i++) {
+            changeTracker.deleteNode(sourceFile, oldImportDecls[i]);
+        }
+
+        return changeTracker.getChanges();
     }
 
     function removeUnusedImports(oldImports: ReadonlyArray<ImportDeclaration>) {
@@ -61,51 +49,15 @@ namespace ts.OrganizeImports {
 
     /* @internal */ // Internal for testing
     export function sortImports(oldImports: ReadonlyArray<ImportDeclaration>) {
-        if (oldImports.length < 2) {
-            return oldImports;
-        }
+        return stableSort(oldImports, (import1, import2) => {
+            const name1 = getExternalModuleName(import1.moduleSpecifier);
+            const name2 = getExternalModuleName(import2.moduleSpecifier);
+            Debug.assert(name1 !== undefined);
+            Debug.assert(name2 !== undefined);
+            return compareBooleans(isExternalModuleNameRelative(name1), isExternalModuleNameRelative(name2)) ||
+                compareStringsCaseSensitive(name1, name2);
 
-        // NB: declaration order determines sort order
-        const enum ModuleNameKind {
-            NonRelative,
-            Relative,
-            Invalid,
-        }
-
-        const importRecords = oldImports.map(createImportRecord);
-
-        const sortedRecords = stableSort(importRecords, (import1, import2) => {
-            const { name: name1, kind: kind1 } = import1;
-            const { name: name2, kind: kind2 } = import2;
-
-            if (kind1 !== kind2) {
-                return kind1 < kind2
-                    ? Comparison.LessThan
-                    : Comparison.GreaterThan;
-            }
-
-            // Note that we're using simple equality, retaining case-sensitivity.
-            if (name1 !== name2) {
-                return name1 < name2
-                    ? Comparison.LessThan
-                    : Comparison.GreaterThan;
-            }
-
-            return Comparison.EqualTo;
         });
-
-        return sortedRecords.map(r => r.importDeclaration);
-
-        function createImportRecord(importDeclaration: ImportDeclaration) {
-            const specifier = importDeclaration.moduleSpecifier;
-            const name = getExternalModuleName(specifier);
-            if (name) {
-                const isRelative = isExternalModuleNameRelative(name);
-                return { importDeclaration, name, kind: isRelative ? ModuleNameKind.Relative : ModuleNameKind.NonRelative };
-            }
-
-            return { importDeclaration, name: specifier.getText(), kind: ModuleNameKind.Invalid };
-        }
     }
 
     function getExternalModuleName(specifier: Expression) {
@@ -123,11 +75,13 @@ namespace ts.OrganizeImports {
         const groups: ImportDeclaration[][] = [];
 
         let groupName: string | undefined = getExternalModuleName(sortedImports[0].moduleSpecifier);
+        Debug.assert(groupName !== undefined);
         let group: ImportDeclaration[] = [];
 
         for (const importDeclaration of sortedImports) {
             const moduleName = getExternalModuleName(importDeclaration.moduleSpecifier);
-            if (moduleName && moduleName === groupName) {
+            Debug.assert(moduleName !== undefined);
+            if (moduleName === groupName) {
                 group.push(importDeclaration);
             }
             else if (group.length) {
@@ -159,38 +113,10 @@ namespace ts.OrganizeImports {
         const groupedImports = groupSortedImports(sortedImports);
         for (const importGroup of groupedImports) {
 
-            let seenImportWithoutClause = false;
-
-            const defaultImports: Identifier[] = [];
-            const namespaceImports: NamespaceImport[] = [];
-            const namedImports: NamedImports[] = [];
-
-            for (const importDeclaration of importGroup) {
-                if (importDeclaration.importClause === undefined) {
-                    // Only the first such import is interesting - the others are redundant.
-                    // Note: Unfortunately, we will lose trivia that was on this node.
-                    if (!seenImportWithoutClause) {
-                        coalescedImports.push(importDeclaration);
-                    }
-
-                    seenImportWithoutClause = true;
-                    continue;
-                }
-
-                const { name, namedBindings } = importDeclaration.importClause;
-
-                if (name) {
-                    defaultImports.push(name);
-                }
+            const { importWithoutClause, defaultImports, namespaceImports, namedImports } = getImportParts(importGroup);
 
-                if (namedBindings) {
-                    if (isNamespaceImport(namedBindings)) {
-                        namespaceImports.push(namedBindings);
-                    }
-                    else {
-                        namedImports.push(namedBindings);
-                    }
-                }
+            if (importWithoutClause) {
+                coalescedImports.push(importWithoutClause);
             }
 
             // Normally, we don't combine default and namespace imports, but it would be silly to
@@ -204,8 +130,6 @@ namespace ts.OrganizeImports {
                 continue;
             }
 
-            // For convenience, we cheat and do a little sorting during coalescing.
-            // Seems reasonable since we're restructuring so much anyway.
             const sortedNamespaceImports = stableSort(namespaceImports, (n1, n2) => compareIdentifiers(n1.name, n2.name));
 
             for (const namespaceImport of sortedNamespaceImports) {
@@ -218,7 +142,7 @@ namespace ts.OrganizeImports {
                 continue;
             }
 
-            let newDefaultImport: Identifier = undefined;
+            let newDefaultImport: Identifier | undefined;
             const newImportSpecifiers: ImportSpecifier[] = [];
             if (defaultImports.length === 1) {
                 newDefaultImport = defaultImports[0];
@@ -230,18 +154,11 @@ namespace ts.OrganizeImports {
                 }
             }
 
-            for (const namedImport of namedImports) {
-                for (const specifier of namedImport.elements) {
-                    newImportSpecifiers.push(specifier);
-                }
-            }
+            newImportSpecifiers.push(...flatMap(namedImports, n => n.elements));
 
-            const sortedImportSpecifiers = stableSort(newImportSpecifiers, (s1, s2) => {
-                const nameComparison = compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name);
-                return nameComparison !== Comparison.EqualTo
-                    ? nameComparison
-                    : compareIdentifiers(s1.name, s2.name);
-            });
+            const sortedImportSpecifiers = stableSort(newImportSpecifiers, (s1, s2) =>
+                compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name) ||
+                compareIdentifiers(s1.name, s2.name));
 
             const importClause = defaultImports.length > 0
                 ? defaultImports[0].parent as ImportClause
@@ -259,19 +176,46 @@ namespace ts.OrganizeImports {
 
         return coalescedImports;
 
-        // `undefined` is the min value.
-        function compareIdentifiers(s1: Identifier | undefined, s2: Identifier | undefined) {
-            return s1 === undefined
-                ? s2 === undefined
-                    ? Comparison.EqualTo
-                    : Comparison.LessThan
-                : s2 === undefined
-                    ? Comparison.GreaterThan
-                    : s1.text < s2.text
-                        ? Comparison.LessThan
-                        : s1.text > s2.text
-                            ? Comparison.GreaterThan
-                            : Comparison.EqualTo;
+        function getImportParts(importGroup: ReadonlyArray<ImportDeclaration>) {
+            let importWithoutClause: ImportDeclaration | undefined;
+            const defaultImports: Identifier[] = [];
+            const namespaceImports: NamespaceImport[] = [];
+            const namedImports: NamedImports[] = [];
+
+            for (const importDeclaration of importGroup) {
+                if (importDeclaration.importClause === undefined) {
+                    // Only the first such import is interesting - the others are redundant.
+                    // Note: Unfortunately, we will lose trivia that was on this node.
+                    importWithoutClause = importWithoutClause || importDeclaration;
+                    continue;
+                }
+
+                const { name, namedBindings } = importDeclaration.importClause;
+
+                if (name) {
+                    defaultImports.push(name);
+                }
+
+                if (namedBindings) {
+                    if (isNamespaceImport(namedBindings)) {
+                        namespaceImports.push(namedBindings);
+                    }
+                    else {
+                        namedImports.push(namedBindings);
+                    }
+                }
+            }
+
+            return {
+                importWithoutClause,
+                defaultImports,
+                namespaceImports,
+                namedImports,
+            }
+        }
+
+        function compareIdentifiers(s1: Identifier, s2: Identifier) {
+            return compareStringsCaseSensitive(s1.text, s2.text);
         }
 
         function updateImportDeclarationAndClause(