From 3137cc4be85c902aac060f6ad2b41e3fd74102bc Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Tue, 13 Mar 2018 15:05:59 -0700 Subject: [PATCH 1/7] replaceNode: Always use non-adjusted end --- src/harness/unittests/textChanges.ts | 10 ++----- src/services/codefixes/convertToEs6Module.ts | 2 +- .../codefixes/fixStrictClassInitialization.ts | 27 +++++++++---------- src/services/textChanges.ts | 9 +++---- .../reference/textChanges/replaceNode1.js | 20 -------------- .../reference/textChanges/replaceNode2.js | 1 + .../reference/textChanges/replaceNode5.js | 21 --------------- .../fourslash/codeFixUseDefaultImport_all.ts | 5 ++-- 8 files changed, 23 insertions(+), 72 deletions(-) delete mode 100644 tests/baselines/reference/textChanges/replaceNode1.js delete mode 100644 tests/baselines/reference/textChanges/replaceNode5.js diff --git a/src/harness/unittests/textChanges.ts b/src/harness/unittests/textChanges.ts index df940fa8b7411..283ffda092b79 100644 --- a/src/harness/unittests/textChanges.ts +++ b/src/harness/unittests/textChanges.ts @@ -255,20 +255,14 @@ var y = 2; // comment 4 var z = 3; // comment 5 // comment 6 var a = 4; // comment 7`; - runSingleFileTest("replaceNode1", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { suffix: newLineCharacter }); - }); runSingleFileTest("replaceNode2", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { useNonAdjustedStartPosition: true, suffix: newLineCharacter, prefix: newLineCharacter }); }); runSingleFileTest("replaceNode3", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { useNonAdjustedEndPosition: true, suffix: newLineCharacter }); + changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { suffix: newLineCharacter }); }); runSingleFileTest("replaceNode4", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { useNonAdjustedStartPosition: true, useNonAdjustedEndPosition: true }); - }); - runSingleFileTest("replaceNode5", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.replaceNode(sourceFile, findVariableStatementContaining("x", sourceFile), createTestClass(), { useNonAdjustedStartPosition: true, useNonAdjustedEndPosition: true }); + changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { useNonAdjustedStartPosition: true }); }); } { diff --git a/src/services/codefixes/convertToEs6Module.ts b/src/services/codefixes/convertToEs6Module.ts index fcde80e81fbf0..05e074ee68755 100644 --- a/src/services/codefixes/convertToEs6Module.ts +++ b/src/services/codefixes/convertToEs6Module.ts @@ -260,7 +260,7 @@ namespace ts.codefix { changes.replaceNodeWithNodes(sourceFile, statement, newNodes); } else { - changes.replaceNode(sourceFile, statement, convertExportsDotXEquals(text, right), { useNonAdjustedEndPosition: true }); + changes.replaceNode(sourceFile, statement, convertExportsDotXEquals(text, right)); } } diff --git a/src/services/codefixes/fixStrictClassInitialization.ts b/src/services/codefixes/fixStrictClassInitialization.ts index 1199e6a059dcb..a5420f2c1815a 100644 --- a/src/services/codefixes/fixStrictClassInitialization.ts +++ b/src/services/codefixes/fixStrictClassInitialization.ts @@ -10,27 +10,24 @@ namespace ts.codefix { const propertyDeclaration = getPropertyDeclaration(context.sourceFile, context.span.start); if (!propertyDeclaration) return; - const newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options); const result = [ getActionForAddMissingUndefinedType(context, propertyDeclaration), - getActionForAddMissingDefiniteAssignmentAssertion(context, propertyDeclaration, newLineCharacter) + getActionForAddMissingDefiniteAssignmentAssertion(context, propertyDeclaration) ]; - append(result, getActionForAddMissingInitializer(context, propertyDeclaration, newLineCharacter)); + append(result, getActionForAddMissingInitializer(context, propertyDeclaration)); return result; }, fixIds: [fixIdAddDefiniteAssignmentAssertions, fixIdAddUndefinedType, fixIdAddInitializer], getAllCodeActions: context => { - const newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options); - return codeFixAll(context, errorCodes, (changes, diag) => { const propertyDeclaration = getPropertyDeclaration(diag.file, diag.start); if (!propertyDeclaration) return; switch (context.fixId) { case fixIdAddDefiniteAssignmentAssertions: - addDefiniteAssignmentAssertion(changes, diag.file, propertyDeclaration, newLineCharacter); + addDefiniteAssignmentAssertion(changes, diag.file, propertyDeclaration); break; case fixIdAddUndefinedType: addUndefinedType(changes, diag.file, propertyDeclaration); @@ -40,7 +37,7 @@ namespace ts.codefix { const initializer = getInitializer(checker, propertyDeclaration); if (!initializer) return; - addInitializer(changes, diag.file, propertyDeclaration, initializer, newLineCharacter); + addInitializer(changes, diag.file, propertyDeclaration, initializer); break; default: Debug.fail(JSON.stringify(context.fixId)); @@ -54,13 +51,13 @@ namespace ts.codefix { return isIdentifier(token) ? cast(token.parent, isPropertyDeclaration) : undefined; } - function getActionForAddMissingDefiniteAssignmentAssertion (context: CodeFixContext, propertyDeclaration: PropertyDeclaration, newLineCharacter: string): CodeFixAction { + function getActionForAddMissingDefiniteAssignmentAssertion (context: CodeFixContext, propertyDeclaration: PropertyDeclaration): CodeFixAction { const description = formatStringFromArgs(getLocaleSpecificMessage(Diagnostics.Add_definite_assignment_assertion_to_property_0), [propertyDeclaration.getText()]); - const changes = textChanges.ChangeTracker.with(context, t => addDefiniteAssignmentAssertion(t, context.sourceFile, propertyDeclaration, newLineCharacter)); + const changes = textChanges.ChangeTracker.with(context, t => addDefiniteAssignmentAssertion(t, context.sourceFile, propertyDeclaration)); return { description, changes, fixId: fixIdAddDefiniteAssignmentAssertions }; } - function addDefiniteAssignmentAssertion(changeTracker: textChanges.ChangeTracker, propertyDeclarationSourceFile: SourceFile, propertyDeclaration: PropertyDeclaration, newLineCharacter: string): void { + function addDefiniteAssignmentAssertion(changeTracker: textChanges.ChangeTracker, propertyDeclarationSourceFile: SourceFile, propertyDeclaration: PropertyDeclaration): void { const property = updateProperty( propertyDeclaration, propertyDeclaration.decorators, @@ -70,7 +67,7 @@ namespace ts.codefix { propertyDeclaration.type, propertyDeclaration.initializer ); - changeTracker.replaceNode(propertyDeclarationSourceFile, propertyDeclaration, property, { suffix: newLineCharacter }); + changeTracker.replaceNode(propertyDeclarationSourceFile, propertyDeclaration, property); } function getActionForAddMissingUndefinedType (context: CodeFixContext, propertyDeclaration: PropertyDeclaration): CodeFixAction { @@ -85,17 +82,17 @@ namespace ts.codefix { changeTracker.replaceNode(propertyDeclarationSourceFile, propertyDeclaration.type, createUnionTypeNode(types)); } - function getActionForAddMissingInitializer (context: CodeFixContext, propertyDeclaration: PropertyDeclaration, newLineCharacter: string): CodeFixAction | undefined { + function getActionForAddMissingInitializer (context: CodeFixContext, propertyDeclaration: PropertyDeclaration): CodeFixAction | undefined { const checker = context.program.getTypeChecker(); const initializer = getInitializer(checker, propertyDeclaration); if (!initializer) return undefined; const description = formatStringFromArgs(getLocaleSpecificMessage(Diagnostics.Add_initializer_to_property_0), [propertyDeclaration.name.getText()]); - const changes = textChanges.ChangeTracker.with(context, t => addInitializer(t, context.sourceFile, propertyDeclaration, initializer, newLineCharacter)); + const changes = textChanges.ChangeTracker.with(context, t => addInitializer(t, context.sourceFile, propertyDeclaration, initializer)); return { description, changes, fixId: fixIdAddInitializer }; } - function addInitializer (changeTracker: textChanges.ChangeTracker, propertyDeclarationSourceFile: SourceFile, propertyDeclaration: PropertyDeclaration, initializer: Expression, newLineCharacter: string): void { + function addInitializer (changeTracker: textChanges.ChangeTracker, propertyDeclarationSourceFile: SourceFile, propertyDeclaration: PropertyDeclaration, initializer: Expression): void { const property = updateProperty( propertyDeclaration, propertyDeclaration.decorators, @@ -105,7 +102,7 @@ namespace ts.codefix { propertyDeclaration.type, initializer ); - changeTracker.replaceNode(propertyDeclarationSourceFile, propertyDeclaration, property, { suffix: newLineCharacter }); + changeTracker.replaceNode(propertyDeclarationSourceFile, propertyDeclaration, property); } function getInitializer(checker: TypeChecker, propertyDeclaration: PropertyDeclaration): Expression | undefined { diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 850c26efd0729..acc087cf651ab 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -292,10 +292,9 @@ namespace ts.textChanges { } // TODO (https://github.com/Microsoft/TypeScript/issues/21246): default should probably be useNonAdjustedPositions - public replaceNode(sourceFile: SourceFile, oldNode: Node, newNode: Node, options: ChangeNodeOptions = {}) { + public replaceNode(sourceFile: SourceFile, oldNode: Node, newNode: Node, options: InsertNodeOptions & ConfigurableStart = {}) { const pos = getAdjustedStartPosition(sourceFile, oldNode, options, Position.Start); - const end = getAdjustedEndPosition(sourceFile, oldNode, options); - return this.replaceRange(sourceFile, { pos, end }, newNode, options); + return this.replaceRange(sourceFile, { pos, end: oldNode.end }, newNode, options); } // TODO (https://github.com/Microsoft/TypeScript/issues/21246): default should probably be useNonAdjustedPositions @@ -398,7 +397,7 @@ namespace ts.textChanges { } private replaceConstructorBody(sourceFile: SourceFile, ctr: ConstructorDeclaration, statements: ReadonlyArray): void { - this.replaceNode(sourceFile, ctr.body, createBlock(statements, /*multiLine*/ true), { useNonAdjustedEndPosition: true }); + this.replaceNode(sourceFile, ctr.body, createBlock(statements, /*multiLine*/ true)); } public insertNodeAtEndOfScope(sourceFile: SourceFile, scope: Node, newNode: Node): void { @@ -613,7 +612,7 @@ namespace ts.textChanges { const newCls = cls.kind === SyntaxKind.ClassDeclaration ? updateClassDeclaration(cls, cls.decorators, cls.modifiers, cls.name, cls.typeParameters, cls.heritageClauses, members) : updateClassExpression(cls, cls.modifiers, cls.name, cls.typeParameters, cls.heritageClauses, members); - this.replaceNode(sourceFile, cls, newCls, { useNonAdjustedEndPosition: true }); + this.replaceNode(sourceFile, cls, newCls); }); } diff --git a/tests/baselines/reference/textChanges/replaceNode1.js b/tests/baselines/reference/textChanges/replaceNode1.js deleted file mode 100644 index 8511d510385ce..0000000000000 --- a/tests/baselines/reference/textChanges/replaceNode1.js +++ /dev/null @@ -1,20 +0,0 @@ -===ORIGINAL=== - -// comment 1 -var x = 1; // comment 2 -// comment 3 -var y = 2; // comment 4 -var z = 3; // comment 5 -// comment 6 -var a = 4; // comment 7 -===MODIFIED=== - -// comment 1 -var x = 1; // comment 2 -public class class1 implements interface1 -{ - property1: boolean; -} -var z = 3; // comment 5 -// comment 6 -var a = 4; // comment 7 \ No newline at end of file diff --git a/tests/baselines/reference/textChanges/replaceNode2.js b/tests/baselines/reference/textChanges/replaceNode2.js index f474aaa327ba5..bf66d7e8bab29 100644 --- a/tests/baselines/reference/textChanges/replaceNode2.js +++ b/tests/baselines/reference/textChanges/replaceNode2.js @@ -17,6 +17,7 @@ public class class1 implements interface1 { property1: boolean; } + // comment 4 var z = 3; // comment 5 // comment 6 var a = 4; // comment 7 \ No newline at end of file diff --git a/tests/baselines/reference/textChanges/replaceNode5.js b/tests/baselines/reference/textChanges/replaceNode5.js deleted file mode 100644 index 9fc6d7777c99f..0000000000000 --- a/tests/baselines/reference/textChanges/replaceNode5.js +++ /dev/null @@ -1,21 +0,0 @@ -===ORIGINAL=== - -// comment 1 -var x = 1; // comment 2 -// comment 3 -var y = 2; // comment 4 -var z = 3; // comment 5 -// comment 6 -var a = 4; // comment 7 -===MODIFIED=== - -// comment 1 -public class class1 implements interface1 -{ - property1: boolean; -} // comment 2 -// comment 3 -var y = 2; // comment 4 -var z = 3; // comment 5 -// comment 6 -var a = 4; // comment 7 \ No newline at end of file diff --git a/tests/cases/fourslash/codeFixUseDefaultImport_all.ts b/tests/cases/fourslash/codeFixUseDefaultImport_all.ts index 0d02eb9b38996..d3f82d7ed020b 100644 --- a/tests/cases/fourslash/codeFixUseDefaultImport_all.ts +++ b/tests/cases/fourslash/codeFixUseDefaultImport_all.ts @@ -13,6 +13,7 @@ goTo.file("/b.ts"); verify.codeFixAll({ fixId: "useDefaultImport", - // TODO: GH#22337 - newFileContent: `import a1 from "./a";import a2 from "./a";`, + newFileContent: +`import a1 from "./a"; +import a2 from "./a";`, }); From 6e60ee603f6019ccee8c8030f55addb30305410e Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Wed, 14 Mar 2018 11:47:18 -0700 Subject: [PATCH 2/7] Never adjust start position either --- src/harness/unittests/textChanges.ts | 4 +-- .../codefixes/disableJsDiagnostics.ts | 25 +++---------- src/services/textChanges.ts | 36 +++++++++++++++---- .../reference/textChanges/replaceNode3.js | 1 + 4 files changed, 38 insertions(+), 28 deletions(-) diff --git a/src/harness/unittests/textChanges.ts b/src/harness/unittests/textChanges.ts index 283ffda092b79..e3e3b0f2be436 100644 --- a/src/harness/unittests/textChanges.ts +++ b/src/harness/unittests/textChanges.ts @@ -256,13 +256,13 @@ var z = 3; // comment 5 // comment 6 var a = 4; // comment 7`; runSingleFileTest("replaceNode2", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { useNonAdjustedStartPosition: true, suffix: newLineCharacter, prefix: newLineCharacter }); + changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { suffix: newLineCharacter, prefix: newLineCharacter }); }); runSingleFileTest("replaceNode3", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { suffix: newLineCharacter }); }); runSingleFileTest("replaceNode4", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { useNonAdjustedStartPosition: true }); + changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass()); }); } { diff --git a/src/services/codefixes/disableJsDiagnostics.ts b/src/services/codefixes/disableJsDiagnostics.ts index 57778d7352607..8ee89ecbf25cf 100644 --- a/src/services/codefixes/disableJsDiagnostics.ts +++ b/src/services/codefixes/disableJsDiagnostics.ts @@ -27,7 +27,7 @@ namespace ts.codefix { fixId: undefined, }]; - if (isValidSuppressLocation(sourceFile, span.start)) { + if (isValidLocationToAddComment(sourceFile, span.start)) { fixes.unshift({ description: getLocaleSpecificMessage(Diagnostics.Ignore_this_error_message), changes: textChanges.ChangeTracker.with(context, t => makeChange(t, sourceFile, span.start)), @@ -41,37 +41,22 @@ namespace ts.codefix { getAllCodeActions: context => { const seenLines = createMap(); return codeFixAll(context, errorCodes, (changes, diag) => { - if (isValidSuppressLocation(diag.file!, diag.start!)) { + if (isValidLocationToAddComment(diag.file!, diag.start!)) { makeChange(changes, diag.file!, diag.start!, seenLines); } }); }, }); - function isValidSuppressLocation(sourceFile: SourceFile, position: number) { + export function isValidLocationToAddComment(sourceFile: SourceFile, position: number) { return !isInComment(sourceFile, position) && !isInString(sourceFile, position) && !isInTemplateString(sourceFile, position); } function makeChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, position: number, seenLines?: Map) { const { line: lineNumber } = getLineAndCharacterOfPosition(sourceFile, position); - // Only need to add `// @ts-ignore` for a line once. - if (seenLines && !addToSeen(seenLines, lineNumber)) { - return; + if (!seenLines || addToSeen(seenLines, lineNumber)) { + changes.insertCommentBeforeLine(sourceFile, lineNumber, position, " @ts-ignore"); } - - const lineStartPosition = getStartPositionOfLine(lineNumber, sourceFile); - const startPosition = getFirstNonSpaceCharacterPosition(sourceFile.text, lineStartPosition); - - // First try to see if we can put the '// @ts-ignore' on the previous line. - // We need to make sure that we are not in the middle of a string literal or a comment. - // If so, we do not want to separate the node from its comment if we can. - // Otherwise, add an extra new line immediately before the error span. - const insertAtLineStart = isValidSuppressLocation(sourceFile, startPosition); - - const token = getTouchingToken(sourceFile, insertAtLineStart ? startPosition : position, /*includeJsDocComment*/ false); - const clone = setStartsOnNewLine(getSynthesizedDeepClone(token), true); - addSyntheticLeadingComment(clone, SyntaxKind.SingleLineCommentTrivia, " @ts-ignore"); - changes.replaceNode(sourceFile, token, clone, { preserveLeadingWhitespace: true, prefix: insertAtLineStart ? undefined : changes.newLineCharacter }); } } diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index acc087cf651ab..6a3398a7ca514 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -103,10 +103,11 @@ namespace ts.textChanges { enum ChangeKind { Remove, ReplaceWithSingleNode, - ReplaceWithMultipleNodes + ReplaceWithMultipleNodes, + Text, } - type Change = ReplaceWithSingleNode | ReplaceWithMultipleNodes | RemoveNode; + type Change = ReplaceWithSingleNode | ReplaceWithMultipleNodes | RemoveNode | ChangeText; interface BaseChange { readonly sourceFile: SourceFile; @@ -134,6 +135,11 @@ namespace ts.textChanges { readonly options?: ChangeNodeOptions; } + interface ChangeText extends BaseChange { + readonly kind: ChangeKind.Text; + readonly text: string; + } + export function getSeparatorCharacter(separator: Token) { return tokenToString(separator.kind); } @@ -291,10 +297,8 @@ namespace ts.textChanges { return this; } - // TODO (https://github.com/Microsoft/TypeScript/issues/21246): default should probably be useNonAdjustedPositions - public replaceNode(sourceFile: SourceFile, oldNode: Node, newNode: Node, options: InsertNodeOptions & ConfigurableStart = {}) { - const pos = getAdjustedStartPosition(sourceFile, oldNode, options, Position.Start); - return this.replaceRange(sourceFile, { pos, end: oldNode.end }, newNode, options); + public replaceNode(sourceFile: SourceFile, oldNode: Node, newNode: Node, options: InsertNodeOptions = {}) { + return this.replaceRange(sourceFile, { pos: oldNode.getStart(sourceFile), end: oldNode.end }, newNode, options); } // TODO (https://github.com/Microsoft/TypeScript/issues/21246): default should probably be useNonAdjustedPositions @@ -348,6 +352,23 @@ namespace ts.textChanges { this.replaceRange(sourceFile, { pos, end: pos }, createToken(modifier), { suffix: " " }); } + public insertCommentBeforeLine(sourceFile: SourceFile, lineNumber: number, position: number, commentText: string) { + const lineStartPosition = getStartPositionOfLine(lineNumber, sourceFile); + const startPosition = getFirstNonSpaceCharacterPosition(sourceFile.text, lineStartPosition); + // First try to see if we can put the comment on the previous line. + // We need to make sure that we are not in the middle of a string literal or a comment. + // If so, we do not want to separate the node from its comment if we can. + // Otherwise, add an extra new line immediately before the error span. + const insertAtLineStart = codefix.isValidLocationToAddComment(sourceFile, startPosition); + const token = getTouchingToken(sourceFile, insertAtLineStart ? startPosition : position, /*includeJsDocComment*/ false); + const text = `${insertAtLineStart ? "" : this.newLineCharacter}${sourceFile.text.slice(lineStartPosition, startPosition)}//${commentText}${this.newLineCharacter}`; + this.insertText(sourceFile, token.getStart(sourceFile), text); + } + + public insertText(sourceFile: SourceFile, pos: number, text: string) { + this.changes.push({ kind: ChangeKind.Text, sourceFile, range: { pos, end: pos }, text }); + } + /** Prefer this over replacing a node with another that has a type annotation, as it avoids reformatting the other parts of the node. */ public insertTypeAnnotation(sourceFile: SourceFile, node: TypeAnnotatable, type: TypeNode): void { const end = (isFunctionLike(node) @@ -651,6 +672,9 @@ namespace ts.textChanges { if (change.kind === ChangeKind.Remove) { return ""; } + if (change.kind === ChangeKind.Text) { + return change.text; + } const { options = {}, range: { pos } } = change; const format = (n: Node) => getFormattedTextOfNode(n, sourceFile, pos, options, newLineCharacter, formatContext, validate); diff --git a/tests/baselines/reference/textChanges/replaceNode3.js b/tests/baselines/reference/textChanges/replaceNode3.js index 9f321a9743183..4294b9147d7ae 100644 --- a/tests/baselines/reference/textChanges/replaceNode3.js +++ b/tests/baselines/reference/textChanges/replaceNode3.js @@ -11,6 +11,7 @@ var a = 4; // comment 7 // comment 1 var x = 1; // comment 2 +// comment 3 public class class1 implements interface1 { property1: boolean; From 8319a163ff2500be53ad01794c017fc7e86a2604 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Wed, 14 Mar 2018 11:57:02 -0700 Subject: [PATCH 3/7] Fix excess property checks, remove unnecessary arguments --- .../codefixes/fixExtendsInterfaceBecomesImplements.ts | 2 +- src/services/codefixes/fixForgottenThisPropertyAccess.ts | 2 +- src/services/codefixes/fixUnusedIdentifier.ts | 2 +- src/services/codefixes/useDefaultImport.ts | 2 +- src/services/refactors/extractSymbol.ts | 8 ++++---- src/services/textChanges.ts | 2 +- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/services/codefixes/fixExtendsInterfaceBecomesImplements.ts b/src/services/codefixes/fixExtendsInterfaceBecomesImplements.ts index 9b775bef622ea..92d652994898c 100644 --- a/src/services/codefixes/fixExtendsInterfaceBecomesImplements.ts +++ b/src/services/codefixes/fixExtendsInterfaceBecomesImplements.ts @@ -27,7 +27,7 @@ namespace ts.codefix { } function doChanges(changes: textChanges.ChangeTracker, sourceFile: SourceFile, extendsToken: Node, heritageClauses: ReadonlyArray): void { - changes.replaceNode(sourceFile, extendsToken, createToken(SyntaxKind.ImplementsKeyword), textChanges.useNonAdjustedPositions); + changes.replaceNode(sourceFile, extendsToken, createToken(SyntaxKind.ImplementsKeyword)); // If there is already an implements clause, replace the implements keyword with a comma. if (heritageClauses.length === 2 && diff --git a/src/services/codefixes/fixForgottenThisPropertyAccess.ts b/src/services/codefixes/fixForgottenThisPropertyAccess.ts index e71c06399c2df..10933b031e5db 100644 --- a/src/services/codefixes/fixForgottenThisPropertyAccess.ts +++ b/src/services/codefixes/fixForgottenThisPropertyAccess.ts @@ -30,6 +30,6 @@ namespace ts.codefix { } // TODO (https://github.com/Microsoft/TypeScript/issues/21246): use shared helper suppressLeadingAndTrailingTrivia(token); - changes.replaceNode(sourceFile, token, createPropertyAccess(createThis(), token), textChanges.useNonAdjustedPositions); + changes.replaceNode(sourceFile, token, createPropertyAccess(createThis(), token)); } } diff --git a/src/services/codefixes/fixUnusedIdentifier.ts b/src/services/codefixes/fixUnusedIdentifier.ts index ef65e49fc2776..3de5e7e36b060 100644 --- a/src/services/codefixes/fixUnusedIdentifier.ts +++ b/src/services/codefixes/fixUnusedIdentifier.ts @@ -165,7 +165,7 @@ namespace ts.codefix { // and trailing trivia will remain. suppressLeadingAndTrailingTrivia(newFunction); - changes.replaceNode(sourceFile, oldFunction, newFunction, textChanges.useNonAdjustedPositions); + changes.replaceNode(sourceFile, oldFunction, newFunction); } else { changes.deleteNodeInList(sourceFile, parent); diff --git a/src/services/codefixes/useDefaultImport.ts b/src/services/codefixes/useDefaultImport.ts index 909601ab3068b..bd0797953b96a 100644 --- a/src/services/codefixes/useDefaultImport.ts +++ b/src/services/codefixes/useDefaultImport.ts @@ -38,6 +38,6 @@ namespace ts.codefix { } function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, info: Info): void { - changes.replaceNode(sourceFile, info.importNode, makeImportDeclaration(info.name, /*namedImports*/ undefined, info.moduleSpecifier), textChanges.useNonAdjustedPositions); + changes.replaceNode(sourceFile, info.importNode, makeImportDeclaration(info.name, /*namedImports*/ undefined, info.moduleSpecifier)); } } diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 2d76ce1843c08..7144265d11a5f 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -1053,7 +1053,7 @@ namespace ts.refactor.extractSymbol { changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newVariable, /*blankLineBetween*/ true); // Consume - changeTracker.replaceNode(context.file, node, localReference, textChanges.useNonAdjustedPositions); + changeTracker.replaceNode(context.file, node, localReference); } else { const newVariableDeclaration = createVariableDeclaration(localNameText, variableType, initializer); @@ -1070,7 +1070,7 @@ namespace ts.refactor.extractSymbol { // Consume const localReference = createIdentifier(localNameText); - changeTracker.replaceNode(context.file, node, localReference, textChanges.useNonAdjustedPositions); + changeTracker.replaceNode(context.file, node, localReference); } else if (node.parent.kind === SyntaxKind.ExpressionStatement && scope === findAncestor(node, isScope)) { // If the parent is an expression statement and the target scope is the immediately enclosing one, @@ -1078,7 +1078,7 @@ namespace ts.refactor.extractSymbol { const newVariableStatement = createVariableStatement( /*modifiers*/ undefined, createVariableDeclarationList([newVariableDeclaration], NodeFlags.Const)); - changeTracker.replaceNode(context.file, node.parent, newVariableStatement, textChanges.useNonAdjustedPositions); + changeTracker.replaceNode(context.file, node.parent, newVariableStatement); } else { const newVariableStatement = createVariableStatement( @@ -1101,7 +1101,7 @@ namespace ts.refactor.extractSymbol { } else { const localReference = createIdentifier(localNameText); - changeTracker.replaceNode(context.file, node, localReference, textChanges.useNonAdjustedPositions); + changeTracker.replaceNode(context.file, node, localReference); } } } diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 6a3398a7ca514..47e3be046c1ff 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -70,7 +70,7 @@ namespace ts.textChanges { * By default when removing nodes we adjust start and end positions to respect specification of the trivia above. * If pos\end should be interpreted literally 'useNonAdjustedStartPosition' or 'useNonAdjustedEndPosition' should be set to true */ - export type ConfigurableStartEnd = ConfigurableStart & ConfigurableEnd; + export interface ConfigurableStartEnd extends ConfigurableStart, ConfigurableEnd {} export const useNonAdjustedPositions: ConfigurableStartEnd = { useNonAdjustedStartPosition: true, From 57a4a926b9e32d813bf7d37f50e82d71e30fe8d3 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Wed, 14 Mar 2018 11:58:08 -0700 Subject: [PATCH 4/7] Make 'insertText' and 'newLineCharacter' private --- src/services/textChanges.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 47e3be046c1ff..73ae2b304d5bb 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -224,7 +224,7 @@ namespace ts.textChanges { } /** Public for tests only. Other callers should use `ChangeTracker.with`. */ - constructor(public readonly newLineCharacter: string, private readonly formatContext: formatting.FormatContext) {} + constructor(private readonly newLineCharacter: string, private readonly formatContext: formatting.FormatContext) {} public deleteRange(sourceFile: SourceFile, range: TextRange) { this.changes.push({ kind: ChangeKind.Remove, sourceFile, range }); @@ -352,7 +352,7 @@ namespace ts.textChanges { this.replaceRange(sourceFile, { pos, end: pos }, createToken(modifier), { suffix: " " }); } - public insertCommentBeforeLine(sourceFile: SourceFile, lineNumber: number, position: number, commentText: string) { + public insertCommentBeforeLine(sourceFile: SourceFile, lineNumber: number, position: number, commentText: string): void { const lineStartPosition = getStartPositionOfLine(lineNumber, sourceFile); const startPosition = getFirstNonSpaceCharacterPosition(sourceFile.text, lineStartPosition); // First try to see if we can put the comment on the previous line. @@ -365,7 +365,7 @@ namespace ts.textChanges { this.insertText(sourceFile, token.getStart(sourceFile), text); } - public insertText(sourceFile: SourceFile, pos: number, text: string) { + private insertText(sourceFile: SourceFile, pos: number, text: string): void { this.changes.push({ kind: ChangeKind.Text, sourceFile, range: { pos, end: pos }, text }); } From 98e2665901accc30c0970e2a83b493f9b7eb0ef0 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Fri, 16 Mar 2018 11:09:54 -0700 Subject: [PATCH 5/7] Use replaceNode in one more place now that it doesn't affect comments --- src/services/codefixes/fixInvalidImportSyntax.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/services/codefixes/fixInvalidImportSyntax.ts b/src/services/codefixes/fixInvalidImportSyntax.ts index 0939475b8350c..fd66a9db37cda 100644 --- a/src/services/codefixes/fixInvalidImportSyntax.ts +++ b/src/services/codefixes/fixInvalidImportSyntax.ts @@ -42,8 +42,7 @@ namespace ts.codefix { } function createAction(context: CodeFixContext, sourceFile: SourceFile, node: Node, replacement: Node): CodeAction { - // TODO: GH#21246 Should be able to use `replaceNode`, but be sure to preserve comments (see `codeFixCalledES2015Import11.ts`) - const changes = textChanges.ChangeTracker.with(context, t => t.replaceRange(sourceFile, { pos: node.getStart(), end: node.end }, replacement)); + const changes = textChanges.ChangeTracker.with(context, t => t.replaceNode(sourceFile, node, replacement)); return { description: formatStringFromArgs(getLocaleSpecificMessage(Diagnostics.Replace_import_with_0), [changes[0].textChanges[0].newText]), changes, From 9ac77abe2c9d38c3d22238b3792463154c5f16f6 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Tue, 20 Mar 2018 13:53:07 -0700 Subject: [PATCH 6/7] Update replaceNodeRange too --- src/harness/unittests/textChanges.ts | 6 +++--- src/services/textChanges.ts | 6 ++---- tests/baselines/reference/textChanges/extractMethodLike.js | 5 +++++ tests/baselines/reference/textChanges/replaceNodeRange1.js | 2 ++ tests/baselines/reference/textChanges/replaceNodeRange2.js | 1 + tests/baselines/reference/textChanges/replaceNodeRange3.js | 1 + 6 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/harness/unittests/textChanges.ts b/src/harness/unittests/textChanges.ts index e3e3b0f2be436..11a34f5abcacd 100644 --- a/src/harness/unittests/textChanges.ts +++ b/src/harness/unittests/textChanges.ts @@ -278,13 +278,13 @@ var a = 4; // comment 7`; changeTracker.replaceNodeRange(sourceFile, findVariableStatementContaining("y", sourceFile), findVariableStatementContaining("z", sourceFile), createTestClass(), { suffix: newLineCharacter }); }); runSingleFileTest("replaceNodeRange2", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.replaceNodeRange(sourceFile, findVariableStatementContaining("y", sourceFile), findVariableStatementContaining("z", sourceFile), createTestClass(), { useNonAdjustedStartPosition: true, suffix: newLineCharacter, prefix: newLineCharacter }); + changeTracker.replaceNodeRange(sourceFile, findVariableStatementContaining("y", sourceFile), findVariableStatementContaining("z", sourceFile), createTestClass(), { suffix: newLineCharacter, prefix: newLineCharacter }); }); runSingleFileTest("replaceNodeRange3", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.replaceNodeRange(sourceFile, findVariableStatementContaining("y", sourceFile), findVariableStatementContaining("z", sourceFile), createTestClass(), { useNonAdjustedEndPosition: true, suffix: newLineCharacter }); + changeTracker.replaceNodeRange(sourceFile, findVariableStatementContaining("y", sourceFile), findVariableStatementContaining("z", sourceFile), createTestClass(), { suffix: newLineCharacter }); }); runSingleFileTest("replaceNodeRange4", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.replaceNodeRange(sourceFile, findVariableStatementContaining("y", sourceFile), findVariableStatementContaining("z", sourceFile), createTestClass(), { useNonAdjustedStartPosition: true, useNonAdjustedEndPosition: true }); + changeTracker.replaceNodeRange(sourceFile, findVariableStatementContaining("y", sourceFile), findVariableStatementContaining("z", sourceFile), createTestClass()); }); } { diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 73ae2b304d5bb..31cd2a6c1f0ed 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -302,10 +302,8 @@ namespace ts.textChanges { } // TODO (https://github.com/Microsoft/TypeScript/issues/21246): default should probably be useNonAdjustedPositions - public replaceNodeRange(sourceFile: SourceFile, startNode: Node, endNode: Node, newNode: Node, options: ChangeNodeOptions = {}) { - const pos = getAdjustedStartPosition(sourceFile, startNode, options, Position.Start); - const end = getAdjustedEndPosition(sourceFile, endNode, options); - return this.replaceRange(sourceFile, { pos, end }, newNode, options); + public replaceNodeRange(sourceFile: SourceFile, startNode: Node, endNode: Node, newNode: Node, options: InsertNodeOptions = {}) { + this.replaceRange(sourceFile, { pos: startNode.getStart(sourceFile), end: endNode.end }, newNode, options); } public replaceRangeWithNodes(sourceFile: SourceFile, range: TextRange, newNodes: ReadonlyArray, options: ChangeNodeOptions = useNonAdjustedPositions) { diff --git a/tests/baselines/reference/textChanges/extractMethodLike.js b/tests/baselines/reference/textChanges/extractMethodLike.js index a566f76b8ed80..5279540d7c464 100644 --- a/tests/baselines/reference/textChanges/extractMethodLike.js +++ b/tests/baselines/reference/textChanges/extractMethodLike.js @@ -43,7 +43,12 @@ namespace M // comment 1 const x = 1; + /** + * comment 2 line 1 + * comment 2 line 2 + */ return bar(); + } } } \ No newline at end of file diff --git a/tests/baselines/reference/textChanges/replaceNodeRange1.js b/tests/baselines/reference/textChanges/replaceNodeRange1.js index 5fc16960a8a02..ae03829528f04 100644 --- a/tests/baselines/reference/textChanges/replaceNodeRange1.js +++ b/tests/baselines/reference/textChanges/replaceNodeRange1.js @@ -11,9 +11,11 @@ var a = 4; // comment 7 // comment 1 var x = 1; // comment 2 +// comment 3 public class class1 implements interface1 { property1: boolean; } + // comment 5 // comment 6 var a = 4; // comment 7 \ No newline at end of file diff --git a/tests/baselines/reference/textChanges/replaceNodeRange2.js b/tests/baselines/reference/textChanges/replaceNodeRange2.js index ebd2664474062..537ed3cfac229 100644 --- a/tests/baselines/reference/textChanges/replaceNodeRange2.js +++ b/tests/baselines/reference/textChanges/replaceNodeRange2.js @@ -17,5 +17,6 @@ public class class1 implements interface1 { property1: boolean; } + // comment 5 // comment 6 var a = 4; // comment 7 \ No newline at end of file diff --git a/tests/baselines/reference/textChanges/replaceNodeRange3.js b/tests/baselines/reference/textChanges/replaceNodeRange3.js index a20dddc53fe70..ae03829528f04 100644 --- a/tests/baselines/reference/textChanges/replaceNodeRange3.js +++ b/tests/baselines/reference/textChanges/replaceNodeRange3.js @@ -11,6 +11,7 @@ var a = 4; // comment 7 // comment 1 var x = 1; // comment 2 +// comment 3 public class class1 implements interface1 { property1: boolean; From 558e9a5b91aae5663fdb261cbd2a4c0cad46d4ef Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Thu, 22 Mar 2018 15:45:55 -0700 Subject: [PATCH 7/7] Always ask for ChangeNodeOptions --- src/harness/unittests/textChanges.ts | 18 ++++++---- src/services/textChanges.ts | 36 +++++++++---------- .../textChanges/extractMethodLike.js | 5 --- .../reference/textChanges/replaceNode1.js | 20 +++++++++++ .../reference/textChanges/replaceNode2.js | 1 - .../reference/textChanges/replaceNode3.js | 1 - .../reference/textChanges/replaceNode5.js | 21 +++++++++++ .../textChanges/replaceNodeRange1.js | 2 -- .../textChanges/replaceNodeRange2.js | 1 - .../textChanges/replaceNodeRange3.js | 1 - 10 files changed, 71 insertions(+), 35 deletions(-) create mode 100644 tests/baselines/reference/textChanges/replaceNode1.js create mode 100644 tests/baselines/reference/textChanges/replaceNode5.js diff --git a/src/harness/unittests/textChanges.ts b/src/harness/unittests/textChanges.ts index 11a34f5abcacd..df940fa8b7411 100644 --- a/src/harness/unittests/textChanges.ts +++ b/src/harness/unittests/textChanges.ts @@ -255,14 +255,20 @@ var y = 2; // comment 4 var z = 3; // comment 5 // comment 6 var a = 4; // comment 7`; + runSingleFileTest("replaceNode1", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { + changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { suffix: newLineCharacter }); + }); runSingleFileTest("replaceNode2", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { suffix: newLineCharacter, prefix: newLineCharacter }); + changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { useNonAdjustedStartPosition: true, suffix: newLineCharacter, prefix: newLineCharacter }); }); runSingleFileTest("replaceNode3", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { suffix: newLineCharacter }); + changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { useNonAdjustedEndPosition: true, suffix: newLineCharacter }); }); runSingleFileTest("replaceNode4", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass()); + changeTracker.replaceNode(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { useNonAdjustedStartPosition: true, useNonAdjustedEndPosition: true }); + }); + runSingleFileTest("replaceNode5", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { + changeTracker.replaceNode(sourceFile, findVariableStatementContaining("x", sourceFile), createTestClass(), { useNonAdjustedStartPosition: true, useNonAdjustedEndPosition: true }); }); } { @@ -278,13 +284,13 @@ var a = 4; // comment 7`; changeTracker.replaceNodeRange(sourceFile, findVariableStatementContaining("y", sourceFile), findVariableStatementContaining("z", sourceFile), createTestClass(), { suffix: newLineCharacter }); }); runSingleFileTest("replaceNodeRange2", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.replaceNodeRange(sourceFile, findVariableStatementContaining("y", sourceFile), findVariableStatementContaining("z", sourceFile), createTestClass(), { suffix: newLineCharacter, prefix: newLineCharacter }); + changeTracker.replaceNodeRange(sourceFile, findVariableStatementContaining("y", sourceFile), findVariableStatementContaining("z", sourceFile), createTestClass(), { useNonAdjustedStartPosition: true, suffix: newLineCharacter, prefix: newLineCharacter }); }); runSingleFileTest("replaceNodeRange3", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.replaceNodeRange(sourceFile, findVariableStatementContaining("y", sourceFile), findVariableStatementContaining("z", sourceFile), createTestClass(), { suffix: newLineCharacter }); + changeTracker.replaceNodeRange(sourceFile, findVariableStatementContaining("y", sourceFile), findVariableStatementContaining("z", sourceFile), createTestClass(), { useNonAdjustedEndPosition: true, suffix: newLineCharacter }); }); runSingleFileTest("replaceNodeRange4", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.replaceNodeRange(sourceFile, findVariableStatementContaining("y", sourceFile), findVariableStatementContaining("z", sourceFile), createTestClass()); + changeTracker.replaceNodeRange(sourceFile, findVariableStatementContaining("y", sourceFile), findVariableStatementContaining("z", sourceFile), createTestClass(), { useNonAdjustedStartPosition: true, useNonAdjustedEndPosition: true }); }); } { diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index bdae4d6f68e71..a267123f719a8 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -114,13 +114,15 @@ namespace ts.textChanges { readonly range: TextRange; } - export interface ChangeNodeOptions extends ConfigurableStartEnd, InsertNodeOptions { + interface FormatNodeOptions extends InsertNodeOptions { readonly useIndentationFromFile?: boolean; } + + export interface ChangeNodeOptions extends ConfigurableStartEnd, FormatNodeOptions {} interface ReplaceWithSingleNode extends BaseChange { readonly kind: ChangeKind.ReplaceWithSingleNode; readonly node: Node; - readonly options?: ChangeNodeOptions; + readonly options?: InsertNodeOptions; } interface RemoveNode extends BaseChange { @@ -132,7 +134,7 @@ namespace ts.textChanges { interface ReplaceWithMultipleNodes extends BaseChange { readonly kind: ChangeKind.ReplaceWithMultipleNodes; readonly nodes: ReadonlyArray; - readonly options?: ChangeNodeOptions; + readonly options?: FormatNodeOptions; } interface ChangeText extends BaseChange { @@ -140,6 +142,10 @@ namespace ts.textChanges { readonly text: string; } + function getAdjustedRange(sourceFile: SourceFile, startNode: Node, endNode: Node, options: ConfigurableStartEnd): TextRange { + return { pos: getAdjustedStartPosition(sourceFile, startNode, options, Position.Start), end: getAdjustedEndPosition(sourceFile, endNode, options) }; + } + function getAdjustedStartPosition(sourceFile: SourceFile, node: Node, options: ConfigurableStart, position: Position) { if (options.useNonAdjustedStartPosition) { return node.getStart(sourceFile); @@ -288,36 +294,30 @@ namespace ts.textChanges { return this; } - // TODO (https://github.com/Microsoft/TypeScript/issues/21246): default should probably be useNonAdjustedPositions - public replaceRange(sourceFile: SourceFile, range: TextRange, newNode: Node, options: ChangeNodeOptions = {}) { + public replaceRange(sourceFile: SourceFile, range: TextRange, newNode: Node, options: InsertNodeOptions = {}) { this.changes.push({ kind: ChangeKind.ReplaceWithSingleNode, sourceFile, range, options, node: newNode }); return this; } - public replaceNode(sourceFile: SourceFile, oldNode: Node, newNode: Node, options: InsertNodeOptions = {}) { - return this.replaceRange(sourceFile, { pos: oldNode.getStart(sourceFile), end: oldNode.end }, newNode, options); + public replaceNode(sourceFile: SourceFile, oldNode: Node, newNode: Node, options: ChangeNodeOptions = useNonAdjustedPositions) { + return this.replaceRange(sourceFile, getAdjustedRange(sourceFile, oldNode, oldNode, options), newNode, options); } - // TODO (https://github.com/Microsoft/TypeScript/issues/21246): default should probably be useNonAdjustedPositions - public replaceNodeRange(sourceFile: SourceFile, startNode: Node, endNode: Node, newNode: Node, options: InsertNodeOptions = {}) { - this.replaceRange(sourceFile, { pos: startNode.getStart(sourceFile), end: endNode.end }, newNode, options); + public replaceNodeRange(sourceFile: SourceFile, startNode: Node, endNode: Node, newNode: Node, options: ChangeNodeOptions = useNonAdjustedPositions) { + this.replaceRange(sourceFile, getAdjustedRange(sourceFile, startNode, endNode, options), newNode, options); } - public replaceRangeWithNodes(sourceFile: SourceFile, range: TextRange, newNodes: ReadonlyArray, options: ChangeNodeOptions = useNonAdjustedPositions) { + private replaceRangeWithNodes(sourceFile: SourceFile, range: TextRange, newNodes: ReadonlyArray, options: InsertNodeOptions = {}) { this.changes.push({ kind: ChangeKind.ReplaceWithMultipleNodes, sourceFile, range, options, nodes: newNodes }); return this; } public replaceNodeWithNodes(sourceFile: SourceFile, oldNode: Node, newNodes: ReadonlyArray, options: ChangeNodeOptions = useNonAdjustedPositions) { - const pos = getAdjustedStartPosition(sourceFile, oldNode, options, Position.Start); - const end = getAdjustedEndPosition(sourceFile, oldNode, options); - return this.replaceRangeWithNodes(sourceFile, { pos, end }, newNodes, options); + return this.replaceRangeWithNodes(sourceFile, getAdjustedRange(sourceFile, oldNode, oldNode, options), newNodes, options); } public replaceNodeRangeWithNodes(sourceFile: SourceFile, startNode: Node, endNode: Node, newNodes: ReadonlyArray, options: ChangeNodeOptions = useNonAdjustedPositions) { - const pos = getAdjustedStartPosition(sourceFile, startNode, options, Position.Start); - const end = getAdjustedEndPosition(sourceFile, endNode, options); - return this.replaceRangeWithNodes(sourceFile, { pos, end }, newNodes, options); + return this.replaceRangeWithNodes(sourceFile, getAdjustedRange(sourceFile, startNode, endNode, options), newNodes, options); } private insertNodeAt(sourceFile: SourceFile, pos: number, newNode: Node, options: InsertNodeOptions = {}) { @@ -682,7 +682,7 @@ namespace ts.textChanges { } /** Note: this may mutate `nodeIn`. */ - function getFormattedTextOfNode(nodeIn: Node, sourceFile: SourceFile, pos: number, options: ChangeNodeOptions, newLineCharacter: string, formatContext: formatting.FormatContext, validate: ValidateNonFormattedText): string { + function getFormattedTextOfNode(nodeIn: Node, sourceFile: SourceFile, pos: number, options: FormatNodeOptions, newLineCharacter: string, formatContext: formatting.FormatContext, validate: ValidateNonFormattedText): string { const { node, text } = getNonformattedText(nodeIn, sourceFile, newLineCharacter); if (validate) validate(node, text); const { options: formatOptions } = formatContext; diff --git a/tests/baselines/reference/textChanges/extractMethodLike.js b/tests/baselines/reference/textChanges/extractMethodLike.js index 5279540d7c464..a566f76b8ed80 100644 --- a/tests/baselines/reference/textChanges/extractMethodLike.js +++ b/tests/baselines/reference/textChanges/extractMethodLike.js @@ -43,12 +43,7 @@ namespace M // comment 1 const x = 1; - /** - * comment 2 line 1 - * comment 2 line 2 - */ return bar(); - } } } \ No newline at end of file diff --git a/tests/baselines/reference/textChanges/replaceNode1.js b/tests/baselines/reference/textChanges/replaceNode1.js new file mode 100644 index 0000000000000..8511d510385ce --- /dev/null +++ b/tests/baselines/reference/textChanges/replaceNode1.js @@ -0,0 +1,20 @@ +===ORIGINAL=== + +// comment 1 +var x = 1; // comment 2 +// comment 3 +var y = 2; // comment 4 +var z = 3; // comment 5 +// comment 6 +var a = 4; // comment 7 +===MODIFIED=== + +// comment 1 +var x = 1; // comment 2 +public class class1 implements interface1 +{ + property1: boolean; +} +var z = 3; // comment 5 +// comment 6 +var a = 4; // comment 7 \ No newline at end of file diff --git a/tests/baselines/reference/textChanges/replaceNode2.js b/tests/baselines/reference/textChanges/replaceNode2.js index bf66d7e8bab29..f474aaa327ba5 100644 --- a/tests/baselines/reference/textChanges/replaceNode2.js +++ b/tests/baselines/reference/textChanges/replaceNode2.js @@ -17,7 +17,6 @@ public class class1 implements interface1 { property1: boolean; } - // comment 4 var z = 3; // comment 5 // comment 6 var a = 4; // comment 7 \ No newline at end of file diff --git a/tests/baselines/reference/textChanges/replaceNode3.js b/tests/baselines/reference/textChanges/replaceNode3.js index 4294b9147d7ae..9f321a9743183 100644 --- a/tests/baselines/reference/textChanges/replaceNode3.js +++ b/tests/baselines/reference/textChanges/replaceNode3.js @@ -11,7 +11,6 @@ var a = 4; // comment 7 // comment 1 var x = 1; // comment 2 -// comment 3 public class class1 implements interface1 { property1: boolean; diff --git a/tests/baselines/reference/textChanges/replaceNode5.js b/tests/baselines/reference/textChanges/replaceNode5.js new file mode 100644 index 0000000000000..9fc6d7777c99f --- /dev/null +++ b/tests/baselines/reference/textChanges/replaceNode5.js @@ -0,0 +1,21 @@ +===ORIGINAL=== + +// comment 1 +var x = 1; // comment 2 +// comment 3 +var y = 2; // comment 4 +var z = 3; // comment 5 +// comment 6 +var a = 4; // comment 7 +===MODIFIED=== + +// comment 1 +public class class1 implements interface1 +{ + property1: boolean; +} // comment 2 +// comment 3 +var y = 2; // comment 4 +var z = 3; // comment 5 +// comment 6 +var a = 4; // comment 7 \ No newline at end of file diff --git a/tests/baselines/reference/textChanges/replaceNodeRange1.js b/tests/baselines/reference/textChanges/replaceNodeRange1.js index ae03829528f04..5fc16960a8a02 100644 --- a/tests/baselines/reference/textChanges/replaceNodeRange1.js +++ b/tests/baselines/reference/textChanges/replaceNodeRange1.js @@ -11,11 +11,9 @@ var a = 4; // comment 7 // comment 1 var x = 1; // comment 2 -// comment 3 public class class1 implements interface1 { property1: boolean; } - // comment 5 // comment 6 var a = 4; // comment 7 \ No newline at end of file diff --git a/tests/baselines/reference/textChanges/replaceNodeRange2.js b/tests/baselines/reference/textChanges/replaceNodeRange2.js index 537ed3cfac229..ebd2664474062 100644 --- a/tests/baselines/reference/textChanges/replaceNodeRange2.js +++ b/tests/baselines/reference/textChanges/replaceNodeRange2.js @@ -17,6 +17,5 @@ public class class1 implements interface1 { property1: boolean; } - // comment 5 // comment 6 var a = 4; // comment 7 \ No newline at end of file diff --git a/tests/baselines/reference/textChanges/replaceNodeRange3.js b/tests/baselines/reference/textChanges/replaceNodeRange3.js index ae03829528f04..a20dddc53fe70 100644 --- a/tests/baselines/reference/textChanges/replaceNodeRange3.js +++ b/tests/baselines/reference/textChanges/replaceNodeRange3.js @@ -11,7 +11,6 @@ var a = 4; // comment 7 // comment 1 var x = 1; // comment 2 -// comment 3 public class class1 implements interface1 { property1: boolean;