Fix newline issues when adding multiple imports (#38119)

andrewbranch · web-flow · commit 9569e8aaa499 · 2020-04-23T11:59:38.000-07:00
* Add new import declarations in a single TextChanges call

* Refactor
diff --git a/src/compiler/core.ts b/src/compiler/core.ts
@@ -844,6 +844,28 @@ namespace ts {
         return to;
     }
 
+    /**
+     * Combines two arrays, values, or undefineds into the smallest container that can accommodate the resulting set:
+     *
+     * ```
+     * undefined -> undefined -> undefined
+     * T -> undefined -> T
+     * T -> T -> T[]
+     * T[] -> undefined -> T[] (no-op)
+     * T[] -> T -> T[]         (append)
+     * T[] -> T[] -> T[]       (concatenate)
+     * ```
+     */
+    export function combine<T>(xs: T | readonly T[] | undefined, ys: T | readonly T[] | undefined): T | readonly T[] | undefined;
+    export function combine<T>(xs: T | T[] | undefined, ys: T | T[] | undefined): T | T[] | undefined;
+    export function combine<T>(xs: T | T[] | undefined, ys: T | T[] | undefined) {
+        if (xs === undefined) return ys;
+        if (ys === undefined) return xs;
+        if (isArray(xs)) return isArray(ys) ? concatenate(xs, ys) : append(xs, ys);
+        if (isArray(ys)) return append(ys, xs);
+        return [xs, ys];
+    }
+
     /**
      * Gets the actual offset into an array for a relative offset. Negative offsets indicate a
      * position offset from the end of the array.
diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts
@@ -45,7 +45,6 @@ namespace ts.codefix {
         // Keys are import clause node IDs.
         const addToExisting = createMap<{ readonly importClauseOrBindingPattern: ImportClause | ObjectBindingPattern, defaultImport: string | undefined; readonly namedImports: string[], canUseTypeOnlyImport: boolean }>();
         const newImports = createMap<Mutable<ImportsCollection & { useRequire: boolean }>>();
-        let lastModuleSpecifier: string | undefined;
         return { addImportFromDiagnostic, addImportFromExportedSymbol, writeFixes };
 
         function addImportFromDiagnostic(diagnostic: DiagnosticWithLocation, context: CodeFixContextBase) {
@@ -97,7 +96,6 @@ namespace ts.codefix {
                     let entry = newImports.get(moduleSpecifier);
                     if (!entry) {
                         newImports.set(moduleSpecifier, entry = { namedImports: [], namespaceLikeImport: undefined, typeOnly, useRequire });
-                        lastModuleSpecifier = moduleSpecifier;
                     }
                     else {
                         // An import clause can only be type-only if every import fix contributing to it can be type-only.
@@ -135,10 +133,15 @@ namespace ts.codefix {
             addToExisting.forEach(({ importClauseOrBindingPattern, defaultImport, namedImports, canUseTypeOnlyImport }) => {
                 doAddExistingFix(changeTracker, sourceFile, importClauseOrBindingPattern, defaultImport, namedImports, canUseTypeOnlyImport);
             });
+
+            let newDeclarations: Statement | readonly Statement[] | undefined;
             newImports.forEach(({ useRequire, ...imports }, moduleSpecifier) => {
-                const addDeclarations = useRequire ? addNewRequires : addNewImports;
-                addDeclarations(changeTracker, sourceFile, moduleSpecifier, quotePreference, imports, /*blankLineBetween*/ lastModuleSpecifier === moduleSpecifier);
+                const getDeclarations = useRequire ? getNewRequires : getNewImports;
+                newDeclarations = combine(newDeclarations, getDeclarations(moduleSpecifier, quotePreference, imports));
             });
+            if (newDeclarations) {
+                insertImports(changeTracker, sourceFile, newDeclarations, /*blankLineBetween*/ true);
+            }
         }
     }
 
@@ -631,11 +634,11 @@ namespace ts.codefix {
             }
             case ImportFixKind.AddNew: {
                 const { importKind, moduleSpecifier, typeOnly, useRequire } = fix;
-                const addDeclarations = useRequire ? addNewRequires : addNewImports;
+                const getDeclarations = useRequire ? getNewRequires : getNewImports;
                 const importsCollection = importKind === ImportKind.Default ? { defaultImport: symbolName, typeOnly } :
                     importKind === ImportKind.Named ? { namedImports: [symbolName], typeOnly } :
                     { namespaceLikeImport: { importKind, name: symbolName }, typeOnly };
-                addDeclarations(changes, sourceFile, moduleSpecifier, quotePreference, importsCollection, /*blankLineBetween*/ true);
+                insertImports(changes, sourceFile, getDeclarations(moduleSpecifier, quotePreference, importsCollection), /*blankLineBetween*/ true);
                 return [importKind === ImportKind.Default ? Diagnostics.Import_default_0_from_module_1 : Diagnostics.Import_0_from_module_1, symbolName, moduleSpecifier];
             }
             default:
@@ -717,13 +720,13 @@ namespace ts.codefix {
             readonly name: string;
         };
     }
-    function addNewImports(changes: textChanges.ChangeTracker, sourceFile: SourceFile, moduleSpecifier: string, quotePreference: QuotePreference, imports: ImportsCollection, blankLineBetween: boolean): void {
+    function getNewImports(moduleSpecifier: string, quotePreference: QuotePreference, imports: ImportsCollection): Statement | readonly Statement[] {
         const quotedModuleSpecifier = makeStringLiteral(moduleSpecifier, quotePreference);
+        let statements: Statement | readonly Statement[] | undefined;
         if (imports.defaultImport !== undefined || imports.namedImports?.length) {
-            insertImport(changes, sourceFile,
-                makeImport(
-                    imports.defaultImport === undefined ? undefined : createIdentifier(imports.defaultImport),
-                    imports.namedImports?.map(n => createImportSpecifier(/*propertyName*/ undefined, createIdentifier(n))), moduleSpecifier, quotePreference, imports.typeOnly), /*blankLineBetween*/ blankLineBetween);
+            statements = combine(statements, makeImport(
+                imports.defaultImport === undefined ? undefined : createIdentifier(imports.defaultImport),
+                imports.namedImports?.map(n => createImportSpecifier(/*propertyName*/ undefined, createIdentifier(n))), moduleSpecifier, quotePreference, imports.typeOnly));
         }
         const { namespaceLikeImport, typeOnly } = imports;
         if (namespaceLikeImport) {
@@ -741,26 +744,29 @@ namespace ts.codefix {
                         createNamespaceImport(createIdentifier(namespaceLikeImport.name)),
                         typeOnly),
                     quotedModuleSpecifier);
-            insertImport(changes, sourceFile, declaration, /*blankLineBetween*/ blankLineBetween);
+            statements = combine(statements, declaration);
         }
+        return Debug.checkDefined(statements);
     }
 
-    function addNewRequires(changes: textChanges.ChangeTracker, sourceFile: SourceFile, moduleSpecifier: string, quotePreference: QuotePreference, imports: ImportsCollection, blankLineBetween: boolean) {
+    function getNewRequires(moduleSpecifier: string, quotePreference: QuotePreference, imports: ImportsCollection): Statement | readonly Statement[] {
         const quotedModuleSpecifier = makeStringLiteral(moduleSpecifier, quotePreference);
+        let statements: Statement | readonly Statement[] | undefined;
         // const { default: foo, bar, etc } = require('./mod');
         if (imports.defaultImport || imports.namedImports?.length) {
             const bindingElements = imports.namedImports?.map(name => createBindingElement(/*dotDotDotToken*/ undefined, /*propertyName*/ undefined, name)) || [];
             if (imports.defaultImport) {
                 bindingElements.unshift(createBindingElement(/*dotDotDotToken*/ undefined, "default", imports.defaultImport));
             }
             const declaration = createConstEqualsRequireDeclaration(createObjectBindingPattern(bindingElements), quotedModuleSpecifier);
-            insertImport(changes, sourceFile, declaration, blankLineBetween);
+            statements = combine(statements, declaration);
         }
         // const foo = require('./mod');
         if (imports.namespaceLikeImport) {
             const declaration = createConstEqualsRequireDeclaration(imports.namespaceLikeImport.name, quotedModuleSpecifier);
-            insertImport(changes, sourceFile, declaration, blankLineBetween);
+            statements = combine(statements, declaration);
         }
+        return Debug.checkDefined(statements);
     }
 
     function createConstEqualsRequireDeclaration(name: string | ObjectBindingPattern, quotedModuleSpecifier: StringLiteral): VariableStatement {
diff --git a/src/services/refactors/moveToNewFile.ts b/src/services/refactors/moveToNewFile.ts
@@ -121,7 +121,7 @@ namespace ts.refactor {
         const quotePreference = getQuotePreference(oldFile, preferences);
         const importsFromNewFile = createOldFileImportsFromNewFile(usage.oldFileImportsFromNewFile, newModuleName, useEs6ModuleSyntax, quotePreference);
         if (importsFromNewFile) {
-            insertImport(changes, oldFile, importsFromNewFile, /*blankLineBetween*/ true);
+            insertImports(changes, oldFile, importsFromNewFile, /*blankLineBetween*/ true);
         }
 
         deleteUnusedOldImports(oldFile, toMove.all, changes, usage.unusedImportsFromOldFile, checker);
diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts
@@ -349,11 +349,25 @@ namespace ts.textChanges {
         }
 
         public insertNodeAtTopOfFile(sourceFile: SourceFile, newNode: Statement, blankLineBetween: boolean): void {
+            this.insertAtTopOfFile(sourceFile, newNode, blankLineBetween);
+        }
+
+        public insertNodesAtTopOfFile(sourceFile: SourceFile, newNodes: readonly Statement[], blankLineBetween: boolean): void {
+            this.insertAtTopOfFile(sourceFile, newNodes, blankLineBetween);
+        }
+
+        private insertAtTopOfFile(sourceFile: SourceFile, insert: Statement | readonly Statement[], blankLineBetween: boolean): void {
             const pos = getInsertionPositionAtSourceFileTop(sourceFile);
-            this.insertNodeAt(sourceFile, pos, newNode, {
+            const options = {
                 prefix: pos === 0 ? undefined : this.newLineCharacter,
                 suffix: (isLineBreak(sourceFile.text.charCodeAt(pos)) ? "" : this.newLineCharacter) + (blankLineBetween ? this.newLineCharacter : ""),
-            });
+            };
+            if (isArray(insert)) {
+                this.insertNodesAt(sourceFile, pos, insert, options);
+            }
+            else {
+                this.insertNodeAt(sourceFile, pos, insert, options);
+            }
         }
 
         public insertFirstParameter(sourceFile: SourceFile, parameters: NodeArray<ParameterDeclaration>, newParam: ParameterDeclaration): void {
diff --git a/src/services/utilities.ts b/src/services/utilities.ts
@@ -1871,14 +1871,23 @@ namespace ts {
         return node.modifiers && find(node.modifiers, m => m.kind === kind);
     }
 
-    export function insertImport(changes: textChanges.ChangeTracker, sourceFile: SourceFile, importDecl: Statement, blankLineBetween: boolean): void {
-        const importKindPredicate = importDecl.kind === SyntaxKind.VariableStatement ? isRequireVariableDeclarationStatement : isAnyImportSyntax;
+    export function insertImports(changes: textChanges.ChangeTracker, sourceFile: SourceFile, imports: Statement | readonly Statement[], blankLineBetween: boolean): void {
+        const decl = isArray(imports) ? imports[0] : imports;
+        const importKindPredicate = decl.kind === SyntaxKind.VariableStatement ? isRequireVariableDeclarationStatement : isAnyImportSyntax;
         const lastImportDeclaration = findLast(sourceFile.statements, statement => importKindPredicate(statement));
         if (lastImportDeclaration) {
-            changes.insertNodeAfter(sourceFile, lastImportDeclaration, importDecl);
+            if (isArray(imports)) {
+                changes.insertNodesAfter(sourceFile, lastImportDeclaration, imports);
+            }
+            else {
+                changes.insertNodeAfter(sourceFile, lastImportDeclaration, imports);
+            }
+        }
+        else if (isArray(imports)) {
+            changes.insertNodesAtTopOfFile(sourceFile, imports, blankLineBetween);
         }
         else {
-            changes.insertNodeAtTopOfFile(sourceFile, importDecl, blankLineBetween);
+            changes.insertNodeAtTopOfFile(sourceFile, imports, blankLineBetween);
         }
     }
 
diff --git a/tests/cases/fourslash/importNameCodeFix_all2.ts b/tests/cases/fourslash/importNameCodeFix_all2.ts
@@ -0,0 +1,23 @@
+/// <reference path="fourslash.ts" />
+
+// @Filename: /path.ts
+////export declare function join(): void;
+
+// @Filename: /os.ts
+////export declare function homedir(): void;
+
+// @Filename: /index.ts
+////
+////join();
+////homedir();
+
+goTo.file("/index.ts");
+verify.codeFixAll({
+  fixId: "fixMissingImport",
+  fixAllDescription: "Add all missing imports",
+  newFileContent: `import { join } from "./path";
+import { homedir } from "./os";
+
+join();
+homedir();`
+});
diff --git a/tests/cases/fourslash/importNameCodeFix_exportEquals.ts b/tests/cases/fourslash/importNameCodeFix_exportEquals.ts
@@ -17,7 +17,6 @@ verify.codeFixAll({
     fixAllDescription: "Add all missing imports",
     newFileContent:
 `import { b } from "./a";
-
 import a = require("./a");
 
 a;

Original file line number	Diff line number	Diff line change
`@@ -121,7 +121,7 @@ namespace ts.refactor {`
`121`	`121`	`const quotePreference = getQuotePreference(oldFile, preferences);`
`122`	`122`	`const importsFromNewFile = createOldFileImportsFromNewFile(usage.oldFileImportsFromNewFile, newModuleName, useEs6ModuleSyntax, quotePreference);`
`123`	`123`	`if (importsFromNewFile) {`
`124`		`- insertImport(changes, oldFile, importsFromNewFile, /blankLineBetween/ true);`
	`124`	`+ insertImports(changes, oldFile, importsFromNewFile, /blankLineBetween/ true);`
`125`	`125`	`}`
`126`	`126`
`127`	`127`	`deleteUnusedOldImports(oldFile, toMove.all, changes, usage.unusedImportsFromOldFile, checker);`