Fix emit/formatting issues in refactors (microsoft#39506)

andrewbranch · web-flow · commit b8340969981a · 2020-07-22T10:31:42.000-07:00
* Fix microsoft#37948 * Fix formatter skipping tab/space fixup on comments, handle trailing commas in list closing line count. Fixes microsoft#37944 * Add newline between imports and main body of new file in moveToNewFile Fixes microsoft#37941 * Update baseline (probably broken before?)
diff --git a/src/compiler/emitter.ts b/src/compiler/emitter.ts
@@ -845,6 +845,7 @@ namespace ts {
         let reservedNamesStack: Set<string>[]; // Stack of TempFlags reserved in enclosing name generation scopes.
         let reservedNames: Set<string>; // TempFlags to reserve in nested name generation scopes.
         let preserveSourceNewlines = printerOptions.preserveSourceNewlines; // Can be overridden inside nodes with the `IgnoreSourceNewlines` emit flag.
+        let nextListElementPos: number | undefined; // See comment in `getLeadingLineTerminatorCount`.
 
         let writer: EmitTextWriter;
         let ownWriter: EmitTextWriter; // Reusable `EmitTextWriter` for basic printing.
@@ -4104,6 +4105,7 @@ namespace ts {
                         shouldEmitInterveningComments = mayEmitInterveningComments;
                     }
 
+                    nextListElementPos = child.pos;
                     emit(child);
 
                     if (shouldDecreaseIndentAfterEmit) {
@@ -4312,11 +4314,32 @@ namespace ts {
                 if (firstChild === undefined) {
                     return rangeIsOnSingleLine(parentNode, currentSourceFile!) ? 0 : 1;
                 }
+                if (firstChild.pos === nextListElementPos) {
+                    // If this child starts at the beginning of a list item in a parent list, its leading
+                    // line terminators have already been written as the separating line terminators of the
+                    // parent list. Example:
+                    //
+                    // class Foo {
+                    //   constructor() {}
+                    //   public foo() {}
+                    // }
+                    //
+                    // The outer list is the list of class members, with one line terminator between the
+                    // constructor and the method. The constructor is written, the separating line terminator
+                    // is written, and then we start emitting the method. Its modifiers ([public]) constitute an inner
+                    // list, so we look for its leading line terminators. If we didn't know that we had already
+                    // written a newline as part of the parent list, it would appear that we need to write a
+                    // leading newline to start the modifiers.
+                    return 0;
+                }
                 if (firstChild.kind === SyntaxKind.JsxText) {
                     // JsxText will be written with its leading whitespace, so don't add more manually.
                     return 0;
                 }
-                if (!positionIsSynthesized(parentNode.pos) && !nodeIsSynthesized(firstChild) && (!firstChild.parent || firstChild.parent === parentNode)) {
+                if (!positionIsSynthesized(parentNode.pos) &&
+                    !nodeIsSynthesized(firstChild) &&
+                    (!firstChild.parent || getOriginalNode(firstChild.parent) === getOriginalNode(parentNode as Node))
+                ) {
                     if (preserveSourceNewlines) {
                         return getEffectiveLines(
                             includeComments => getLinesBetweenPositionAndPrecedingNonWhitespaceCharacter(
@@ -4376,9 +4399,10 @@ namespace ts {
                 }
                 if (!positionIsSynthesized(parentNode.pos) && !nodeIsSynthesized(lastChild) && (!lastChild.parent || lastChild.parent === parentNode)) {
                     if (preserveSourceNewlines) {
+                        const end = isNodeArray(children) && !positionIsSynthesized(children.end) ? children.end : lastChild.end;
                         return getEffectiveLines(
                             includeComments => getLinesBetweenPositionAndNextNonWhitespaceCharacter(
-                                lastChild.end,
+                                end,
                                 parentNode.end,
                                 currentSourceFile!,
                                 includeComments));
diff --git a/src/services/formatting/formatting.ts b/src/services/formatting/formatting.ts
@@ -1092,10 +1092,6 @@ namespace ts.formatting {
             const nonWhitespaceColumnInFirstPart =
                 SmartIndenter.findFirstNonWhitespaceCharacterAndColumn(startLinePos, parts[0].pos, sourceFile, options);
 
-            if (indentation === nonWhitespaceColumnInFirstPart.column && !jsxTextStyleIndent) {
-                return;
-            }
-
             let startIndex = 0;
             if (firstLineIsIndented) {
                 startIndex = 1;
diff --git a/src/services/refactors/moveToNewFile.ts b/src/services/refactors/moveToNewFile.ts
@@ -109,7 +109,7 @@ namespace ts.refactor {
 
     function getNewStatementsAndRemoveFromOldFile(
         oldFile: SourceFile, usage: UsageInfo, changes: textChanges.ChangeTracker, toMove: ToMove, program: Program, newModuleName: string, preferences: UserPreferences,
-    ): readonly Statement[] {
+    ) {
         const checker = program.getTypeChecker();
 
         if (!oldFile.externalModuleIndicator && !oldFile.commonJsModuleIndicator) {
@@ -129,9 +129,19 @@ namespace ts.refactor {
 
         updateImportsInOtherFiles(changes, program, oldFile, usage.movedSymbols, newModuleName);
 
+        const imports = getNewFileImportsAndAddExportInOldFile(oldFile, usage.oldImportsNeededByNewFile, usage.newFileImportsFromOldFile, changes, checker, useEs6ModuleSyntax, quotePreference);
+        const body = addExports(oldFile, toMove.all, usage.oldFileImportsFromNewFile, useEs6ModuleSyntax);
+        if (imports.length && body.length) {
+            return [
+                ...imports,
+                SyntaxKind.NewLineTrivia as const,
+                ...body
+            ];
+        }
+
         return [
-            ...getNewFileImportsAndAddExportInOldFile(oldFile, usage.oldImportsNeededByNewFile, usage.newFileImportsFromOldFile, changes, checker, useEs6ModuleSyntax, quotePreference),
-            ...addExports(oldFile, toMove.all, usage.oldFileImportsFromNewFile, useEs6ModuleSyntax),
+            ...imports,
+            ...body,
         ];
     }
 
diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts
@@ -249,7 +249,7 @@ namespace ts.textChanges {
 
     export class ChangeTracker {
         private readonly changes: Change[] = [];
-        private readonly newFiles: { readonly oldFile: SourceFile | undefined, readonly fileName: string, readonly statements: readonly Statement[] }[] = [];
+        private readonly newFiles: { readonly oldFile: SourceFile | undefined, readonly fileName: string, readonly statements: readonly (Statement | SyntaxKind.NewLineTrivia)[] }[] = [];
         private readonly classesWithNodesInsertedAtStart = new Map<string, { readonly node: ClassDeclaration | InterfaceDeclaration | ObjectLiteralExpression, readonly sourceFile: SourceFile }>(); // Set<ClassDeclaration> implemented as Map<node id, ClassDeclaration>
         private readonly deletedNodes: { readonly sourceFile: SourceFile, readonly node: Node | NodeArray<TypeParameterDeclaration> }[] = [];
 
@@ -868,7 +868,7 @@ namespace ts.textChanges {
             return changes;
         }
 
-        public createNewFile(oldFile: SourceFile | undefined, fileName: string, statements: readonly Statement[]): void {
+        public createNewFile(oldFile: SourceFile | undefined, fileName: string, statements: readonly (Statement | SyntaxKind.NewLineTrivia)[]): void {
             this.newFiles.push({ oldFile, fileName, statements });
         }
     }
@@ -922,14 +922,14 @@ namespace ts.textChanges {
             });
         }
 
-        export function newFileChanges(oldFile: SourceFile | undefined, fileName: string, statements: readonly Statement[], newLineCharacter: string, formatContext: formatting.FormatContext): FileTextChanges {
+        export function newFileChanges(oldFile: SourceFile | undefined, fileName: string, statements: readonly (Statement | SyntaxKind.NewLineTrivia)[], newLineCharacter: string, formatContext: formatting.FormatContext): FileTextChanges {
             const text = newFileChangesWorker(oldFile, getScriptKindFromFileName(fileName), statements, newLineCharacter, formatContext);
             return { fileName, textChanges: [createTextChange(createTextSpan(0, 0), text)], isNewFile: true };
         }
 
-        export function newFileChangesWorker(oldFile: SourceFile | undefined, scriptKind: ScriptKind, statements: readonly Statement[], newLineCharacter: string, formatContext: formatting.FormatContext): string {
+        export function newFileChangesWorker(oldFile: SourceFile | undefined, scriptKind: ScriptKind, statements: readonly (Statement | SyntaxKind.NewLineTrivia)[], newLineCharacter: string, formatContext: formatting.FormatContext): string {
             // TODO: this emits the file, parses it back, then formats it that -- may be a less roundabout way to do this
-            const nonFormattedText = statements.map(s => getNonformattedText(s, oldFile, newLineCharacter).text).join(newLineCharacter);
+            const nonFormattedText = statements.map(s => s === SyntaxKind.NewLineTrivia ? "" : getNonformattedText(s, oldFile, newLineCharacter).text).join(newLineCharacter);
             const sourceFile = createSourceFile("any file name", nonFormattedText, ScriptTarget.ESNext, /*setParentNodes*/ true, scriptKind);
             const changes = formatting.formatDocument(sourceFile, formatContext);
             return applyChanges(nonFormattedText, changes) + newLineCharacter;
diff --git a/tests/baselines/reference/textChanges/replaceRangeWithForcedIndentation.js b/tests/baselines/reference/textChanges/replaceRangeWithForcedIndentation.js
@@ -14,6 +14,6 @@ var x = 1; // comment 2
 // comment 3
         public class class1 implements interface1
         {
-        property1: boolean;
+            property1: boolean;
         }
 var a = 4; // comment 7
diff --git a/tests/cases/fourslash/moveToNewFile.ts b/tests/cases/fourslash/moveToNewFile.ts
@@ -23,6 +23,7 @@ a; y;`,
         "/y.ts":
 `import { b } from './other';
 import { p } from './a';
+
 export const y: Date = p + b;
 `,
     },
diff --git a/tests/cases/fourslash/moveToNewFile_cleanUpLastNamedImport.ts b/tests/cases/fourslash/moveToNewFile_cleanUpLastNamedImport.ts
@@ -21,6 +21,7 @@ console.log(defaultExport)
 
     "/bar.ts": 
     `import { Exported } from "./has-exports";
+
 export const bar = (logger: Exported) => 0;
 `,
   }
diff --git a/tests/cases/fourslash/moveToNewFile_defaultImport.ts b/tests/cases/fourslash/moveToNewFile_defaultImport.ts
@@ -12,6 +12,7 @@ verify.moveToNewFile({
 
         "/x.ts":
 `import f from "./a";
+
 const x = f();
 `,
     },
diff --git a/tests/cases/fourslash/moveToNewFile_exportImport.ts b/tests/cases/fourslash/moveToNewFile_exportImport.ts
@@ -15,6 +15,7 @@ M;`,
 
         "/O.ts":
 `import { N } from "./a";
+
 export import O = N;
 `,
     },
diff --git a/tests/cases/fourslash/moveToNewFile_getter.ts b/tests/cases/fourslash/moveToNewFile_getter.ts
@@ -23,6 +23,7 @@ verify.moveToNewFile({
 
         "/g.ts":
 `import { C } from "./a";
+
 export const { g, h: i } = new C();
 `,
 
diff --git a/tests/cases/fourslash/moveToNewFile_importEquals.ts b/tests/cases/fourslash/moveToNewFile_importEquals.ts
@@ -13,6 +13,7 @@ verify.moveToNewFile({
 j;`,
         "/y.ts":
 `import i = require("./i");
+
 const y = i;
 `,
     },
diff --git a/tests/cases/fourslash/moveToNewFile_js.ts b/tests/cases/fourslash/moveToNewFile_js.ts
@@ -26,6 +26,7 @@ a; y; z;`,
         "/y.js":
 `const { b } = require("./other");
 const { p } = require("./a");
+
 const y = p + b;
 exports.y = y;
 const z = 0;
diff --git a/tests/cases/fourslash/moveToNewFile_moveImport.ts b/tests/cases/fourslash/moveToNewFile_moveImport.ts
@@ -13,6 +13,7 @@ verify.moveToNewFile({
 b;`,
         "/l.ts":
 `import { a } from "m";
+
 let l;
 a;
 `,
diff --git a/tests/cases/fourslash/moveToNewFile_moveJsxImport1.ts b/tests/cases/fourslash/moveToNewFile_moveJsxImport1.ts
@@ -15,6 +15,7 @@ verify.moveToNewFile({
 `1;`,
         "/tests/cases/fourslash/newFile.tsx":
 `import React = require('react');
+
 <div />;
 `,
     }
diff --git a/tests/cases/fourslash/moveToNewFile_moveJsxImport2.ts b/tests/cases/fourslash/moveToNewFile_moveJsxImport2.ts
@@ -16,6 +16,7 @@ verify.moveToNewFile({
 <div/>;`,
         "/tests/cases/fourslash/newFile.tsx":
 `import React = require('react');
+
 <div />;
 `,
     }
diff --git a/tests/cases/fourslash/moveToNewFile_moveJsxImport4.ts b/tests/cases/fourslash/moveToNewFile_moveJsxImport4.ts
@@ -20,6 +20,7 @@ React;`,
         // NB: A perfect implementation would not copy over the import
         "/tests/cases/fourslash/F.tsx":
 `import React = require('leftpad');
+
 function F() {
     const React = import("react");
     <div />;
diff --git a/tests/cases/fourslash/moveToNewFile_moveNamedImport.ts b/tests/cases/fourslash/moveToNewFile_moveNamedImport.ts
@@ -12,6 +12,7 @@ verify.moveToNewFile({
 
 "/x.ts":
 `import { foo as oFoo } from './other';
+
 export const x = oFoo();
 `
     },
diff --git a/tests/cases/fourslash/moveToNewFile_moveNamespaceImport.ts b/tests/cases/fourslash/moveToNewFile_moveNamespaceImport.ts
@@ -12,6 +12,7 @@ verify.moveToNewFile({
 
 "/x.ts":
 `import * as o from './other';
+
 export const x = o.foo();
 `
     },
diff --git a/tests/cases/fourslash/moveToNewFile_multiple.ts b/tests/cases/fourslash/moveToNewFile_multiple.ts
@@ -20,6 +20,7 @@ x; y;`,
 
         "/x.ts":
 `import { a, b } from "./a";
+
 export const x = 0;
 a;
 export const y = 1;
diff --git a/tests/cases/fourslash/moveToNewFile_selectionOnName.ts b/tests/cases/fourslash/moveToNewFile_selectionOnName.ts
@@ -14,6 +14,7 @@ export {};
 export function gee() { eff(); }`,
         "/eff.ts":
 `import { gee } from "./a";
+
 export function eff() { gee(); }
 `,
     },
diff --git a/tests/cases/fourslash/moveToNewFile_variableDeclarationWithNoInitializer.ts b/tests/cases/fourslash/moveToNewFile_variableDeclarationWithNoInitializer.ts
@@ -14,6 +14,7 @@ export let x;
 
         "/y.ts":
 `import { x } from "./a";
+
 const y = x;
 `,
     },
diff --git a/tests/cases/fourslash/textChangesIndentStyle.ts b/tests/cases/fourslash/textChangesIndentStyle.ts
@@ -0,0 +1,34 @@
+/// <reference path="fourslash.ts" />
+
+// This fourslash file is indented with tabs.
+
+// @Filename: /a.ts
+//// [|export class Foo {
+//// 	constructor(
+//// 		public readonly a: number,
+//// 
+//// 		/**
+//// 		 * Docs!
+//// 		 */
+//// 		public readonly b: number,
+//// 	) { }
+//// }|]
+
+format.setOption("convertTabsToSpaces", false);
+verify.moveToNewFile({
+  newFileContents: {
+    "/a.ts": "",
+    "/Foo.ts":
+`export class Foo {
+	constructor(
+		public readonly a: number,
+
+		/**
+		 * Docs!
+		 */
+		public readonly b: number
+	) { }
+}
+`
+  }
+});
diff --git a/tests/cases/fourslash/textChangesPreserveNewlines10.ts b/tests/cases/fourslash/textChangesPreserveNewlines10.ts
@@ -0,0 +1,45 @@
+/// <reference path="fourslash.ts" />
+
+// @Filename: /a.ts
+//// [|class Foo {
+//// 
+////     constructor() {
+////     }
+//// 
+////     public runCommand(): void {
+////         let focusedEditor = 1;
+////         // Only if editor text focus (i.e. not if editor has widget focus).
+////         if (focusedEditor) {
+////             return;
+////         }
+////     }
+//// 
+////     public run(): void { }
+//// }|]
+//// export const a = new Foo();
+
+verify.moveToNewFile({
+  newFileContents: {
+    "/a.ts":
+`import { Foo } from "./Foo";
+
+export const a = new Foo();`,
+    "/Foo.ts":
+`export class Foo {
+
+    constructor() {
+    }
+
+    public runCommand(): void {
+        let focusedEditor = 1;
+        // Only if editor text focus (i.e. not if editor has widget focus).
+        if (focusedEditor) {
+            return;
+        }
+    }
+
+    public run(): void { }
+}
+`
+  }
+});

Original file line number	Diff line number	Diff line change
`@@ -249,7 +249,7 @@ namespace ts.textChanges {`
`249`	`249`
`250`	`250`	`export class ChangeTracker {`
`251`	`251`	`private readonly changes: Change[] = [];`
`252`		`- private readonly newFiles: { readonly oldFile: SourceFile \| undefined, readonly fileName: string, readonly statements: readonly Statement[] }[] = [];`
	`252`	`+ private readonly newFiles: { readonly oldFile: SourceFile \| undefined, readonly fileName: string, readonly statements: readonly (Statement \| SyntaxKind.NewLineTrivia)[] }[] = [];`
`253`	`253`	`private readonly classesWithNodesInsertedAtStart = new Map<string, { readonly node: ClassDeclaration \| InterfaceDeclaration \| ObjectLiteralExpression, readonly sourceFile: SourceFile }>(); // Set<ClassDeclaration> implemented as Map<node id, ClassDeclaration>`
`254`	`254`	`private readonly deletedNodes: { readonly sourceFile: SourceFile, readonly node: Node \| NodeArray<TypeParameterDeclaration> }[] = [];`
`255`	`255`
`@@ -868,7 +868,7 @@ namespace ts.textChanges {`
`868`	`868`	`return changes;`
`869`	`869`	`}`
`870`	`870`
`871`		`- public createNewFile(oldFile: SourceFile \| undefined, fileName: string, statements: readonly Statement[]): void {`
	`871`	`+ public createNewFile(oldFile: SourceFile \| undefined, fileName: string, statements: readonly (Statement \| SyntaxKind.NewLineTrivia)[]): void {`
`872`	`872`	`this.newFiles.push({ oldFile, fileName, statements });`
`873`	`873`	`}`
`874`	`874`	`}`
`@@ -922,14 +922,14 @@ namespace ts.textChanges {`
`922`	`922`	`});`
`923`	`923`	`}`
`924`	`924`
`925`		`- export function newFileChanges(oldFile: SourceFile \| undefined, fileName: string, statements: readonly Statement[], newLineCharacter: string, formatContext: formatting.FormatContext): FileTextChanges {`
	`925`	`+ export function newFileChanges(oldFile: SourceFile \| undefined, fileName: string, statements: readonly (Statement \| SyntaxKind.NewLineTrivia)[], newLineCharacter: string, formatContext: formatting.FormatContext): FileTextChanges {`
`926`	`926`	`const text = newFileChangesWorker(oldFile, getScriptKindFromFileName(fileName), statements, newLineCharacter, formatContext);`
`927`	`927`	`return { fileName, textChanges: [createTextChange(createTextSpan(0, 0), text)], isNewFile: true };`
`928`	`928`	`}`
`929`	`929`
`930`		`- export function newFileChangesWorker(oldFile: SourceFile \| undefined, scriptKind: ScriptKind, statements: readonly Statement[], newLineCharacter: string, formatContext: formatting.FormatContext): string {`
	`930`	`+ export function newFileChangesWorker(oldFile: SourceFile \| undefined, scriptKind: ScriptKind, statements: readonly (Statement \| SyntaxKind.NewLineTrivia)[], newLineCharacter: string, formatContext: formatting.FormatContext): string {`
`931`	`931`	`// TODO: this emits the file, parses it back, then formats it that -- may be a less roundabout way to do this`
`932`		`- const nonFormattedText = statements.map(s => getNonformattedText(s, oldFile, newLineCharacter).text).join(newLineCharacter);`
	`932`	`+ const nonFormattedText = statements.map(s => s === SyntaxKind.NewLineTrivia ? "" : getNonformattedText(s, oldFile, newLineCharacter).text).join(newLineCharacter);`
`933`	`933`	`const sourceFile = createSourceFile("any file name", nonFormattedText, ScriptTarget.ESNext, /setParentNodes/ true, scriptKind);`
`934`	`934`	`const changes = formatting.formatDocument(sourceFile, formatContext);`
`935`	`935`	`return applyChanges(nonFormattedText, changes) + newLineCharacter;`
Original file line number	Diff line number	Diff line change
`@@ -14,6 +14,6 @@ var x = 1; // comment 2`
`14`	`14`	`// comment 3`
`15`	`15`	`public class class1 implements interface1`
`16`	`16`	`{`
`17`		`- property1: boolean;`
	`17`	`+ property1: boolean;`
`18`	`18`	`}`
`19`	`19`	`var a = 4; // comment 7`
Original file line number	Diff line number	Diff line change
`@@ -21,6 +21,7 @@ console.log(defaultExport)`
`21`	`21`
`22`	`22`	`"/bar.ts":`
`23`	`23`	`import { Exported } from "./has-exports";
	`24`	`+`
`24`	`25`	`export const bar = (logger: Exported) => 0;`
`25`	`26`	`,
`26`	`27`	`}`