Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 28 commits into from
Mar 19, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
2b9e4aa
Allow emitter to write multiple newlines in node lists
andrewbranch Feb 6, 2020
c689617
Progress
andrewbranch Feb 7, 2020
6e272f3
Progress
andrewbranch Feb 7, 2020
839fb8e
Fix recomputeIndentation
andrewbranch Feb 7, 2020
0ea8d79
Add tests, fix leading line terminator count
andrewbranch Feb 7, 2020
6ff9c2b
Do a bit less work when `preserveNewlines` is off
andrewbranch Feb 10, 2020
c91efd4
Fix accidental find/replace rename
andrewbranch Feb 11, 2020
e3ef427
Restore some monomorphism
andrewbranch Feb 19, 2020
e535e27
Fix single line writer
andrewbranch Feb 19, 2020
21b0cb8
Fix other writers
andrewbranch Feb 19, 2020
b6cf73d
Merge branch 'master' into bug/27294
andrewbranch Feb 19, 2020
0c536c5
Revert "Fix other writers"
andrewbranch Feb 19, 2020
91eaf80
Revert "Fix single line writer"
andrewbranch Feb 19, 2020
3be2c86
Revert "Restore some monomorphism"
andrewbranch Feb 19, 2020
8ed98bc
Add equal position optimization to getLinesBetweenRangeEndAndRangeStart
andrewbranch Feb 19, 2020
d9c80fd
Add one more test
andrewbranch Feb 19, 2020
af188d4
Actually save the test file
andrewbranch Feb 19, 2020
19a9728
Rename preserveNewlines to preserveSourceNewlines
andrewbranch Feb 28, 2020
131f2bb
Make ignoreSourceNewlines internal
andrewbranch Feb 28, 2020
34147a5
Optimize lines-between functions
andrewbranch Mar 2, 2020
cf96308
Merge branch 'master' into bug/27294
andrewbranch Mar 2, 2020
075c782
Add comment;
andrewbranch Mar 2, 2020
563d223
Fix trailing line terminator count bug for function parameters
andrewbranch Mar 13, 2020
8b61d66
Preserve newlines around parenthesized expressions
andrewbranch Mar 14, 2020
e7c2b28
Merge branch 'master' into bug/27294
andrewbranch Mar 14, 2020
e99d833
Back to speculative microoptimizations, yay
andrewbranch Mar 16, 2020
ac768d0
Don’t call getEffectiveLines during tsc emit at all
andrewbranch Mar 16, 2020
6daa27e
Merge branch 'master' into bug/27294
andrewbranch Mar 16, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
243 changes: 158 additions & 85 deletions src/compiler/emitter.ts

Large diffs are not rendered by default.

6 changes: 6 additions & 0 deletions src/compiler/factoryPublic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3561,6 +3561,12 @@ namespace ts {
return node;
}

/** @internal */
export function ignoreSourceNewlines<T extends Node>(node: T): T {
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
getOrCreateEmitNode(node).flags |= EmitFlags.IgnoreSourceNewlines;
return node;
}

/**
* Gets the constant value to emit for an expression.
*/
Expand Down
30 changes: 24 additions & 6 deletions src/compiler/scanner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
Expand Down
4 changes: 3 additions & 1 deletion src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3781,7 +3781,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;
Expand Down Expand Up @@ -5857,6 +5857,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 {
Expand Down Expand Up @@ -6321,6 +6322,7 @@ namespace ts {
/*@internal*/ writeBundleFileInfo?: boolean;
/*@internal*/ recordInternalSection?: boolean;
/*@internal*/ stripInternal?: boolean;
/*@internal*/ preserveSourceNewlines?: boolean;
/*@internal*/ relativeToBuildInfo?: (path: string) => string;
}

Expand Down
55 changes: 43 additions & 12 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3633,8 +3633,8 @@ namespace ts {
}
}

function writeLine() {
if (!lineStart) {
function writeLine(force?: boolean) {
if (!lineStart || force) {
output += newLine;
lineCount++;
linePos = output.length;
Expand Down Expand Up @@ -3905,12 +3905,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 {
Expand Down Expand Up @@ -4735,32 +4736,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) {
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
while (pos-- > 0) {
if (!isWhiteSpaceLike(sourceFile.text.charCodeAt(pos))) {
return pos;
}
}
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/harness/fourslashImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
10 changes: 5 additions & 5 deletions src/services/formatting/formatting.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[] {
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions src/services/refactors/extractType.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
changes.replaceNode(file, selection, createTypeReferenceNode(name, typeParameters.map(id => createTypeReferenceNode(id.name, /* typeArguments */ undefined))));
}

Expand All @@ -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))));
}

Expand Down
6 changes: 3 additions & 3 deletions src/services/textChanges.ts
Original file line number Diff line number Diff line change
Expand Up @@ -925,7 +925,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) };
}
}
Expand Down Expand Up @@ -1054,8 +1054,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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ var y = {
"typeof":
};
var x = (_a = {
a: a, : .b,
a: a,
: .b,
a: a
},
_a["ss"] = ,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,6 @@ var C2 = /** @class */ (function () {
return C2;
}());
var b = {
x: function () { }, 1: // error
x: function () { },
1: // error
};
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,4 @@ var v = { a
return;

//// [parserErrorRecovery_ObjectLiteral2.js]
var v = { a: a,
"return": };
var v = { a: a, "return": };
Original file line number Diff line number Diff line change
Expand Up @@ -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);`
});
13 changes: 4 additions & 9 deletions tests/cases/fourslash/moveToNewFile_declarationKinds.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 { }
`,
},
});
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,5 @@ verify.codeFix({
export function f() { }
export function g() { }
export function h() { }
export class C {
}`,
export class C { }`,
});
Original file line number Diff line number Diff line change
Expand Up @@ -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 };`,
});
25 changes: 25 additions & 0 deletions tests/cases/fourslash/textChangesPreserveNewlines1.ts
Original file line number Diff line number Diff line change
@@ -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);
}
`
});
Loading