Preserve newlines from original source when printing nodes from TextChanges (#36688)

andrewbranch · web-flow · commit 237ea526f9b1 · 2020-03-19T09:46:00.000-07:00
* Allow emitter to write multiple newlines in node lists * Progress * Progress * Fix recomputeIndentation * Add tests, fix leading line terminator count * Do a bit less work when `preserveNewlines` is off * Fix accidental find/replace rename * Restore some monomorphism * Fix single line writer * Fix other writers * Revert "Fix other writers" This reverts commit 21b0cb8. * Revert "Fix single line writer" This reverts commit e535e27. * Revert "Restore some monomorphism" This reverts commit e3ef427. * Add equal position optimization to getLinesBetweenRangeEndAndRangeStart * Add one more test * Actually save the test file * Rename preserveNewlines to preserveSourceNewlines * Make ignoreSourceNewlines internal * Optimize lines-between functions * Add comment; * Fix trailing line terminator count bug for function parameters * Preserve newlines around parenthesized expressions * Back to speculative microoptimizations, yay * Don’t call getEffectiveLines during tsc emit at all
diff --git a/src/compiler/emitter.ts b/src/compiler/emitter.ts
diff --git a/src/compiler/factoryPublic.ts b/src/compiler/factoryPublic.ts
@@ -3567,6 +3567,12 @@ namespace ts {
         return node;
     }
 
+    /** @internal */
+    export function ignoreSourceNewlines<T extends Node>(node: T): T {
+        getOrCreateEmitNode(node).flags |= EmitFlags.IgnoreSourceNewlines;
+        return node;
+    }
+
     /**
      * Gets the constant value to emit for an expression.
      */
diff --git a/src/compiler/scanner.ts b/src/compiler/scanner.ts
@@ -409,11 +409,20 @@ namespace ts {
     }
 
     /* @internal */
+    export function computeLineAndCharacterOfPosition(lineStarts: readonly number[], position: number): LineAndCharacter {
+        const lineNumber = computeLineOfPosition(lineStarts, position);
+        return {
+            line: lineNumber,
+            character: position - lineStarts[lineNumber]
+        };
+    }
+
     /**
+     * @internal
      * We assume the first line starts at position 0 and 'position' is non-negative.
      */
-    export function computeLineAndCharacterOfPosition(lineStarts: readonly number[], position: number): LineAndCharacter {
-        let lineNumber = binarySearch(lineStarts, position, identity, compareValues);
+    export function computeLineOfPosition(lineStarts: readonly number[], position: number, lowerBound?: number) {
+        let lineNumber = binarySearch(lineStarts, position, identity, compareValues, lowerBound);
         if (lineNumber < 0) {
             // If the actual position was not found,
             // the binary search returns the 2's-complement of the next line start
@@ -425,10 +434,19 @@ namespace ts {
             lineNumber = ~lineNumber - 1;
             Debug.assert(lineNumber !== -1, "position cannot precede the beginning of the file");
         }
-        return {
-            line: lineNumber,
-            character: position - lineStarts[lineNumber]
-        };
+        return lineNumber;
+    }
+
+    /** @internal */
+    export function getLinesBetweenPositions(sourceFile: SourceFileLike, pos1: number, pos2: number) {
+        if (pos1 === pos2) return 0;
+        const lineStarts = getLineStarts(sourceFile);
+        const lower = Math.min(pos1, pos2);
+        const isNegative = lower === pos2;
+        const upper = isNegative ? pos1 : pos2;
+        const lowerLine = computeLineOfPosition(lineStarts, lower);
+        const upperLine = computeLineOfPosition(lineStarts, upper, lowerLine);
+        return isNegative ? lowerLine - upperLine : upperLine - lowerLine;
     }
 
     export function getLineAndCharacterOfPosition(sourceFile: SourceFileLike, position: number): LineAndCharacter {
diff --git a/src/compiler/types.ts b/src/compiler/types.ts
@@ -3784,7 +3784,7 @@ namespace ts {
         writeParameter(text: string): void;
         writeProperty(text: string): void;
         writeSymbol(text: string, symbol: Symbol): void;
-        writeLine(): void;
+        writeLine(force?: boolean): void;
         increaseIndent(): void;
         decreaseIndent(): void;
         clear(): void;
@@ -5860,6 +5860,7 @@ namespace ts {
         NoAsciiEscaping = 1 << 24,              // When synthesizing nodes that lack an original node or textSourceNode, we want to write the text on the node with ASCII escaping substitutions.
         /*@internal*/ TypeScriptClassWrapper = 1 << 25, // The node is an IIFE class wrapper created by the ts transform.
         /*@internal*/ NeverApplyImportHelper = 1 << 26, // Indicates the node should never be wrapped with an import star helper (because, for example, it imports tslib itself)
+        /*@internal*/ IgnoreSourceNewlines = 1 << 27,   // Overrides `printerOptions.preserveSourceNewlines` to print this node (and all descendants) with default whitespace.
     }
 
     export interface EmitHelper {
@@ -6324,6 +6325,7 @@ namespace ts {
         /*@internal*/ writeBundleFileInfo?: boolean;
         /*@internal*/ recordInternalSection?: boolean;
         /*@internal*/ stripInternal?: boolean;
+        /*@internal*/ preserveSourceNewlines?: boolean;
         /*@internal*/ relativeToBuildInfo?: (path: string) => string;
     }
 
diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts
@@ -3633,8 +3633,8 @@ namespace ts {
             }
         }
 
-        function writeLine() {
-            if (!lineStart) {
+        function writeLine(force?: boolean) {
+            if (!lineStart || force) {
                 output += newLine;
                 lineCount++;
                 linePos = output.length;
@@ -3913,12 +3913,13 @@ namespace ts {
         }
     }
 
-    export function getLineOfLocalPosition(currentSourceFile: SourceFile, pos: number) {
-        return getLineAndCharacterOfPosition(currentSourceFile, pos).line;
+    export function getLineOfLocalPosition(sourceFile: SourceFile, pos: number) {
+        const lineStarts = getLineStarts(sourceFile);
+        return computeLineOfPosition(lineStarts, pos);
     }
 
     export function getLineOfLocalPositionFromLineMap(lineMap: readonly number[], pos: number) {
-        return computeLineAndCharacterOfPosition(lineMap, pos).line;
+        return computeLineOfPosition(lineMap, pos);
     }
 
     export function getFirstConstructorWithBody(node: ClassLikeDeclaration): ConstructorDeclaration & { body: FunctionBody } | undefined {
@@ -4743,32 +4744,62 @@ namespace ts {
     }
 
     export function rangeStartPositionsAreOnSameLine(range1: TextRange, range2: TextRange, sourceFile: SourceFile) {
-        return positionsAreOnSameLine(getStartPositionOfRange(range1, sourceFile), getStartPositionOfRange(range2, sourceFile), sourceFile);
+        return positionsAreOnSameLine(
+            getStartPositionOfRange(range1, sourceFile, /*includeComments*/ false),
+            getStartPositionOfRange(range2, sourceFile, /*includeComments*/ false),
+            sourceFile);
     }
 
     export function rangeEndPositionsAreOnSameLine(range1: TextRange, range2: TextRange, sourceFile: SourceFile) {
         return positionsAreOnSameLine(range1.end, range2.end, sourceFile);
     }
 
     export function rangeStartIsOnSameLineAsRangeEnd(range1: TextRange, range2: TextRange, sourceFile: SourceFile) {
-        return positionsAreOnSameLine(getStartPositionOfRange(range1, sourceFile), range2.end, sourceFile);
+        return positionsAreOnSameLine(getStartPositionOfRange(range1, sourceFile, /*includeComments*/ false), range2.end, sourceFile);
     }
 
     export function rangeEndIsOnSameLineAsRangeStart(range1: TextRange, range2: TextRange, sourceFile: SourceFile) {
-        return positionsAreOnSameLine(range1.end, getStartPositionOfRange(range2, sourceFile), sourceFile);
+        return positionsAreOnSameLine(range1.end, getStartPositionOfRange(range2, sourceFile, /*includeComments*/ false), sourceFile);
+    }
+
+    export function getLinesBetweenRangeEndAndRangeStart(range1: TextRange, range2: TextRange, sourceFile: SourceFile, includeSecondRangeComments: boolean) {
+        const range2Start = getStartPositionOfRange(range2, sourceFile, includeSecondRangeComments);
+        return getLinesBetweenPositions(sourceFile, range1.end, range2Start);
+    }
+
+    export function getLinesBetweenRangeEndPositions(range1: TextRange, range2: TextRange, sourceFile: SourceFile) {
+        return getLinesBetweenPositions(sourceFile, range1.end, range2.end);
     }
 
     export function isNodeArrayMultiLine(list: NodeArray<Node>, sourceFile: SourceFile): boolean {
         return !positionsAreOnSameLine(list.pos, list.end, sourceFile);
     }
 
     export function positionsAreOnSameLine(pos1: number, pos2: number, sourceFile: SourceFile) {
-        return pos1 === pos2 ||
-            getLineOfLocalPosition(sourceFile, pos1) === getLineOfLocalPosition(sourceFile, pos2);
+        return getLinesBetweenPositions(sourceFile, pos1, pos2) === 0;
+    }
+
+    export function getStartPositionOfRange(range: TextRange, sourceFile: SourceFile, includeComments: boolean) {
+        return positionIsSynthesized(range.pos) ? -1 : skipTrivia(sourceFile.text, range.pos, /*stopAfterLineBreak*/ false, includeComments);
     }
 
-    export function getStartPositionOfRange(range: TextRange, sourceFile: SourceFile) {
-        return positionIsSynthesized(range.pos) ? -1 : skipTrivia(sourceFile.text, range.pos);
+    export function getLinesBetweenPositionAndPrecedingNonWhitespaceCharacter(pos: number, sourceFile: SourceFile, includeComments?: boolean) {
+        const startPos = skipTrivia(sourceFile.text, pos, /*stopAfterLineBreak*/ false, includeComments);
+        const prevPos = getPreviousNonWhitespacePosition(startPos, sourceFile);
+        return getLinesBetweenPositions(sourceFile, prevPos || 0, startPos);
+    }
+
+    export function getLinesBetweenPositionAndNextNonWhitespaceCharacter(pos: number, sourceFile: SourceFile, includeComments?: boolean) {
+        const nextPos = skipTrivia(sourceFile.text, pos, /*stopAfterLineBreak*/ false, includeComments);
+        return getLinesBetweenPositions(sourceFile, pos, nextPos);
+    }
+
+    function getPreviousNonWhitespacePosition(pos: number, sourceFile: SourceFile) {
+        while (pos-- > 0) {
+            if (!isWhiteSpaceLike(sourceFile.text.charCodeAt(pos))) {
+                return pos;
+            }
+        }
     }
 
     /**
diff --git a/src/harness/fourslashImpl.ts b/src/harness/fourslashImpl.ts
@@ -3306,7 +3306,7 @@ namespace FourSlash {
         }
 
         public moveToNewFile(options: FourSlashInterface.MoveToNewFileOptions): void {
-            assert(this.getRanges().length === 1);
+            assert(this.getRanges().length === 1, "Must have exactly one fourslash range (source enclosed between '[|' and '|]' delimiters) in the source file");
             const range = this.getRanges()[0];
             const refactor = ts.find(this.getApplicableRefactors(range, { allowTextChangesInNewFiles: true }), r => r.name === "Move to a new file")!;
             assert(refactor.actions.length === 1);
diff --git a/src/services/formatting/formatting.ts b/src/services/formatting/formatting.ts
@@ -70,7 +70,7 @@ namespace ts.formatting {
          * Formatter calls this function when rule adds or deletes new lines from the text
          * so indentation scope can adjust values of indentation and delta.
          */
-        recomputeIndentation(lineAddedByFormatting: boolean): void;
+        recomputeIndentation(lineAddedByFormatting: boolean, parent: Node): void;
     }
 
     export function formatOnEnter(position: number, sourceFile: SourceFile, formatContext: FormatContext): TextChange[] {
@@ -567,8 +567,8 @@ namespace ts.formatting {
                     !suppressDelta && shouldAddDelta(line, kind, container) ? indentation + getDelta(container) : indentation,
                 getIndentation: () => indentation,
                 getDelta,
-                recomputeIndentation: lineAdded => {
-                    if (node.parent && SmartIndenter.shouldIndentChildNode(options, node.parent, node, sourceFile)) {
+                recomputeIndentation: (lineAdded, parent) => {
+                    if (SmartIndenter.shouldIndentChildNode(options, parent, node, sourceFile)) {
                         indentation += lineAdded ? options.indentSize! : -options.indentSize!;
                         delta = SmartIndenter.shouldIndentChildNode(options, node) ? options.indentSize! : 0;
                     }
@@ -996,15 +996,15 @@ namespace ts.formatting {
                             // Handle the case where the next line is moved to be the end of this line.
                             // In this case we don't indent the next line in the next pass.
                             if (currentParent.getStart(sourceFile) === currentItem.pos) {
-                                dynamicIndentation.recomputeIndentation(/*lineAddedByFormatting*/ false);
+                                dynamicIndentation.recomputeIndentation(/*lineAddedByFormatting*/ false, contextNode);
                             }
                             break;
                         case LineAction.LineAdded:
                             // Handle the case where token2 is moved to the new line.
                             // In this case we indent token2 in the next pass but we set
                             // sameLineIndent flag to notify the indenter that the indentation is within the line.
                             if (currentParent.getStart(sourceFile) === currentItem.pos) {
-                                dynamicIndentation.recomputeIndentation(/*lineAddedByFormatting*/ true);
+                                dynamicIndentation.recomputeIndentation(/*lineAddedByFormatting*/ true, contextNode);
                             }
                             break;
                         default:
diff --git a/src/services/refactors/extractType.ts b/src/services/refactors/extractType.ts
@@ -159,7 +159,7 @@ namespace ts.refactor {
             typeParameters.map(id => updateTypeParameterDeclaration(id, id.name, id.constraint, /* defaultType */ undefined)),
             selection
         );
-        changes.insertNodeBefore(file, firstStatement, newTypeNode, /* blankLineBetween */ true);
+        changes.insertNodeBefore(file, firstStatement, ignoreSourceNewlines(newTypeNode), /* blankLineBetween */ true);
         changes.replaceNode(file, selection, createTypeReferenceNode(name, typeParameters.map(id => createTypeReferenceNode(id.name, /* typeArguments */ undefined))));
     }
 
@@ -174,7 +174,7 @@ namespace ts.refactor {
             /* heritageClauses */ undefined,
             typeElements
         );
-        changes.insertNodeBefore(file, firstStatement, newTypeNode, /* blankLineBetween */ true);
+        changes.insertNodeBefore(file, firstStatement, ignoreSourceNewlines(newTypeNode), /* blankLineBetween */ true);
         changes.replaceNode(file, selection, createTypeReferenceNode(name, typeParameters.map(id => createTypeReferenceNode(id.name, /* typeArguments */ undefined))));
     }
 
diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts
@@ -935,7 +935,7 @@ namespace ts.textChanges {
         export function getNonformattedText(node: Node, sourceFile: SourceFile | undefined, newLineCharacter: string): { text: string, node: Node } {
             const writer = createWriter(newLineCharacter);
             const newLine = newLineCharacter === "\n" ? NewLineKind.LineFeed : NewLineKind.CarriageReturnLineFeed;
-            createPrinter({ newLine, neverAsciiEscape: true }, writer).writeNode(EmitHint.Unspecified, node, sourceFile, writer);
+            createPrinter({ newLine, neverAsciiEscape: true, preserveSourceNewlines: true }, writer).writeNode(EmitHint.Unspecified, node, sourceFile, writer);
             return { text: writer.getText(), node: assignPositionsToNode(node) };
         }
     }
@@ -1064,8 +1064,8 @@ namespace ts.textChanges {
             writer.writeSymbol(s, sym);
             setLastNonTriviaPosition(s, /*force*/ false);
         }
-        function writeLine(): void {
-            writer.writeLine();
+        function writeLine(force?: boolean): void {
+            writer.writeLine(force);
         }
         function increaseIndent(): void {
             writer.increaseIndent();
diff --git a/tests/baselines/reference/objectLiteralShorthandPropertiesErrorFromNotUsingIdentifier.js b/tests/baselines/reference/objectLiteralShorthandPropertiesErrorFromNotUsingIdentifier.js
@@ -35,7 +35,8 @@ var y = {
     "typeof": 
 };
 var x = (_a = {
-        a: a, : .b,
+        a: a,
+        : .b,
         a: a
     },
     _a["ss"] = ,
diff --git a/tests/baselines/reference/objectLiteralShorthandPropertiesErrorWithModule.js b/tests/baselines/reference/objectLiteralShorthandPropertiesErrorWithModule.js
@@ -25,7 +25,8 @@ var n;
 (function (n) {
     var z = 10000;
     n.y = {
-        m: m, : .x // error
+        m: m,
+        : .x // error
     };
 })(n || (n = {}));
 m.y.x;
diff --git a/tests/baselines/reference/objectTypesWithOptionalProperties2.js b/tests/baselines/reference/objectTypesWithOptionalProperties2.js
@@ -42,5 +42,6 @@ var C2 = /** @class */ (function () {
     return C2;
 }());
 var b = {
-    x: function () { }, 1:  // error
+    x: function () { },
+    1:  // error
 };
diff --git a/tests/baselines/reference/parserErrorRecovery_ObjectLiteral2.js b/tests/baselines/reference/parserErrorRecovery_ObjectLiteral2.js
@@ -3,5 +3,4 @@ var v = { a
 return;
 
 //// [parserErrorRecovery_ObjectLiteral2.js]
-var v = { a: a,
-    "return":  };
+var v = { a: a, "return":  };
diff --git a/tests/cases/fourslash/extract-const-callback-function-this3.ts b/tests/cases/fourslash/extract-const-callback-function-this3.ts
@@ -10,8 +10,6 @@ edit.applyRefactor({
     actionDescription: "Extract to constant in enclosing scope",
     newContent:
 `declare function fWithThis(fn: (this: { a: string }, a: string) => string): void;
-const newLocal = function(this: {
-    a: string;
-}, a: string): string { return this.a; };
+const newLocal = function(this: { a: string; }, a: string): string { return this.a; };
 fWithThis(/*RENAME*/newLocal);`
 });
diff --git a/tests/cases/fourslash/moveToNewFile_declarationKinds.ts b/tests/cases/fourslash/moveToNewFile_declarationKinds.ts
@@ -24,16 +24,11 @@ type U = T; type V = I;`,
         "/x.ts":
 `export const x = 0;
 export function f() { }
-export class C {
-}
-export enum E {
-}
-export namespace N {
-    export const x = 0;
-}
+export class C { }
+export enum E { }
+export namespace N { export const x = 0; }
 export type T = number;
-export interface I {
-}
+export interface I { }
 `,
     },
 });
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_object.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_object.ts
@@ -22,6 +22,5 @@ verify.codeFix({
 export function f() { }
 export function g() { }
 export function h() { }
-export class C {
-}`,
+export class C { }`,
 });
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_expressionToDeclaration.ts b/tests/cases/fourslash/refactorConvertToEs6Module_expressionToDeclaration.ts
@@ -15,8 +15,6 @@ verify.codeFix({
 `var C = {};
 console.log(C);
 export async function* f(p) { p; }
-const _C = class C extends D {
-    m() { }
-};
+const _C = class C extends D { m() { } };
 export { _C as C };`,
 });
diff --git a/tests/cases/fourslash/textChangesPreserveNewlines1.ts b/tests/cases/fourslash/textChangesPreserveNewlines1.ts
@@ -0,0 +1,25 @@
+/// <reference path="fourslash.ts" />
+
+//// /*1*/console.log(1);
+//// 
+//// console.log(2);
+//// 
+//// console.log(3);/*2*/
+
+goTo.select("1", "2");
+edit.applyRefactor({
+  refactorName: "Extract Symbol",
+  actionName: "function_scope_0",
+  actionDescription: "Extract to function in global scope",
+  newContent:
+`/*RENAME*/newFunction();
+
+function newFunction() {
+    console.log(1);
+
+    console.log(2);
+
+    console.log(3);
+}
+`
+});
diff --git a/tests/cases/fourslash/textChangesPreserveNewlines2.ts b/tests/cases/fourslash/textChangesPreserveNewlines2.ts
diff --git a/tests/cases/fourslash/textChangesPreserveNewlines3.ts b/tests/cases/fourslash/textChangesPreserveNewlines3.ts
diff --git a/tests/cases/fourslash/textChangesPreserveNewlines4.ts b/tests/cases/fourslash/textChangesPreserveNewlines4.ts
diff --git a/tests/cases/fourslash/textChangesPreserveNewlines5.ts b/tests/cases/fourslash/textChangesPreserveNewlines5.ts
diff --git a/tests/cases/fourslash/textChangesPreserveNewlines6.ts b/tests/cases/fourslash/textChangesPreserveNewlines6.ts
diff --git a/tests/cases/fourslash/textChangesPreserveNewlines7.ts b/tests/cases/fourslash/textChangesPreserveNewlines7.ts

Original file line number	Diff line number	Diff line change
`@@ -3306,7 +3306,7 @@ namespace FourSlash {`
`3306`	`3306`	`}`
`3307`	`3307`
`3308`	`3308`	`public moveToNewFile(options: FourSlashInterface.MoveToNewFileOptions): void {`
`3309`		`- assert(this.getRanges().length === 1);`
	`3309`	`+ assert(this.getRanges().length === 1, "Must have exactly one fourslash range (source enclosed between '[\|' and '\|]' delimiters) in the source file");`
`3310`	`3310`	`const range = this.getRanges()[0];`
`3311`	`3311`	`const refactor = ts.find(this.getApplicableRefactors(range, { allowTextChangesInNewFiles: true }), r => r.name === "Move to a new file")!;`
`3312`	`3312`	`assert(refactor.actions.length === 1);`
Original file line number	Diff line number	Diff line change
`@@ -935,7 +935,7 @@ namespace ts.textChanges {`
`935`	`935`	`export function getNonformattedText(node: Node, sourceFile: SourceFile \| undefined, newLineCharacter: string): { text: string, node: Node } {`
`936`	`936`	`const writer = createWriter(newLineCharacter);`
`937`	`937`	`const newLine = newLineCharacter === "\n" ? NewLineKind.LineFeed : NewLineKind.CarriageReturnLineFeed;`
`938`		`- createPrinter({ newLine, neverAsciiEscape: true }, writer).writeNode(EmitHint.Unspecified, node, sourceFile, writer);`
	`938`	`+ createPrinter({ newLine, neverAsciiEscape: true, preserveSourceNewlines: true }, writer).writeNode(EmitHint.Unspecified, node, sourceFile, writer);`
`939`	`939`	`return { text: writer.getText(), node: assignPositionsToNode(node) };`
`940`	`940`	`}`
`941`	`941`	`}`
`@@ -1064,8 +1064,8 @@ namespace ts.textChanges {`
`1064`	`1064`	`writer.writeSymbol(s, sym);`
`1065`	`1065`	`setLastNonTriviaPosition(s, /force/ false);`
`1066`	`1066`	`}`
`1067`		`- function writeLine(): void {`
`1068`		`- writer.writeLine();`
	`1067`	`+ function writeLine(force?: boolean): void {`
	`1068`	`+ writer.writeLine(force);`
`1069`	`1069`	`}`
`1070`	`1070`	`function increaseIndent(): void {`
`1071`	`1071`	`writer.increaseIndent();`