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

* 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?)

Author: andrewbranch

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));

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;

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,
         ];
     }
 

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;

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

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;
 `,
     },

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;
 `,
   }

tests/cases/fourslash/moveToNewFile_defaultImport.ts

@@ -12,6 +12,7 @@ verify.moveToNewFile({
 
         "/x.ts":
 `import f from "./a";
+
 const x = f();
 `,
     },

tests/cases/fourslash/moveToNewFile_exportImport.ts

@@ -15,6 +15,7 @@ M;`,
 
         "/O.ts":
 `import { N } from "./a";
+
 export import O = N;
 `,
     },

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();
 `,