From c3bdd2a963101d0d79f489b25decae2b1b5f8130 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Fri, 8 Dec 2017 07:07:34 -0800 Subject: [PATCH 1/2] Make ChangeTracker#newLineCharacter public, to avoid having to pass newLineCharacter around as a parameter --- src/services/codefixes/fixAddMissingMember.ts | 35 ++++++++++--------- ...sDoesntImplementInheritedAbstractMember.ts | 8 ++--- .../fixClassIncorrectlyImplementsInterface.ts | 11 +++--- .../fixClassSuperMustPrecedeThisAccess.ts | 14 ++++---- .../fixConstructorForDerivedNeedSuperCall.ts | 12 +++---- src/services/codefixes/importFixes.ts | 3 +- .../refactors/convertFunctionToEs6Class.ts | 3 +- src/services/refactors/extractSymbol.ts | 18 +++++----- src/services/textChanges.ts | 10 +++--- 9 files changed, 58 insertions(+), 56 deletions(-) diff --git a/src/services/codefixes/fixAddMissingMember.ts b/src/services/codefixes/fixAddMissingMember.ts index e20bdf0fb5b86..b2594971a0e52 100644 --- a/src/services/codefixes/fixAddMissingMember.ts +++ b/src/services/codefixes/fixAddMissingMember.ts @@ -21,8 +21,8 @@ namespace ts.codefix { getAllCodeActions: context => { const seenNames = createMap(); return codeFixAll(context, errorCodes, (changes, diag) => { - const { newLineCharacter, program } = context; - const info = getInfo(diag.file!, diag.start!, context.program.getTypeChecker()); + const { program } = context; + const info = getInfo(diag.file!, diag.start!, program.getTypeChecker()); if (!info) return; const { classDeclaration, classDeclarationSourceFile, inJs, makeStatic, token, call } = info; if (!addToSeen(seenNames, token.text)) { @@ -31,15 +31,15 @@ namespace ts.codefix { // Always prefer to add a method declaration if possible. if (call) { - addMethodDeclaration(changes, classDeclarationSourceFile, classDeclaration, token, call, newLineCharacter, makeStatic, inJs); + addMethodDeclaration(changes, classDeclarationSourceFile, classDeclaration, token, call, makeStatic, inJs); } else { if (inJs) { - addMissingMemberInJs(changes, classDeclarationSourceFile, classDeclaration, token.text, makeStatic, newLineCharacter); + addMissingMemberInJs(changes, classDeclarationSourceFile, classDeclaration, token.text, makeStatic); } else { const typeNode = getTypeNode(program.getTypeChecker(), classDeclaration, token); - addPropertyDeclaration(changes, classDeclarationSourceFile, classDeclaration, token.text, typeNode, makeStatic, newLineCharacter); + addPropertyDeclaration(changes, classDeclarationSourceFile, classDeclaration, token.text, typeNode, makeStatic); } } }); @@ -96,13 +96,14 @@ namespace ts.codefix { } function getActionsForAddMissingMemberInJavaScriptFile(context: CodeFixContext, classDeclarationSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, tokenName: string, makeStatic: boolean): CodeFixAction | undefined { - const changes = textChanges.ChangeTracker.with(context, t => addMissingMemberInJs(t, classDeclarationSourceFile, classDeclaration, tokenName, makeStatic, context.newLineCharacter)); + const changes = textChanges.ChangeTracker.with(context, t => addMissingMemberInJs(t, classDeclarationSourceFile, classDeclaration, tokenName, makeStatic)); if (changes.length === 0) return undefined; const description = formatStringFromArgs(getLocaleSpecificMessage(makeStatic ? Diagnostics.Initialize_static_property_0 : Diagnostics.Initialize_property_0_in_the_constructor), [tokenName]); return { description, changes, fixId }; } - function addMissingMemberInJs(changeTracker: textChanges.ChangeTracker, classDeclarationSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, tokenName: string, makeStatic: boolean, newLineCharacter: string): void { + function addMissingMemberInJs(changeTracker: textChanges.ChangeTracker, classDeclarationSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, tokenName: string, makeStatic: boolean): void { + const { newLineCharacter } = changeTracker; if (makeStatic) { if (classDeclaration.kind === SyntaxKind.ClassExpression) { return; @@ -142,13 +143,13 @@ namespace ts.codefix { return typeNode || createKeywordTypeNode(SyntaxKind.AnyKeyword); } - function createAddPropertyDeclarationAction(context: CodeFixContext, classDeclarationSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, makeStatic: boolean, tokenName: string, typeNode: TypeNode): CodeFixAction { + function createAddPropertyDeclarationAction(context: textChanges.TextChangesContext, classDeclarationSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, makeStatic: boolean, tokenName: string, typeNode: TypeNode): CodeFixAction { const description = formatStringFromArgs(getLocaleSpecificMessage(makeStatic ? Diagnostics.Declare_static_property_0 : Diagnostics.Declare_property_0), [tokenName]); - const changes = textChanges.ChangeTracker.with(context, t => addPropertyDeclaration(t, classDeclarationSourceFile, classDeclaration, tokenName, typeNode, makeStatic, context.newLineCharacter)); + const changes = textChanges.ChangeTracker.with(context, t => addPropertyDeclaration(t, classDeclarationSourceFile, classDeclaration, tokenName, typeNode, makeStatic)); return { description, changes, fixId }; } - function addPropertyDeclaration(changeTracker: textChanges.ChangeTracker, classDeclarationSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, tokenName: string, typeNode: TypeNode, makeStatic: boolean, newLineCharacter: string): void { + function addPropertyDeclaration(changeTracker: textChanges.ChangeTracker, classDeclarationSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, tokenName: string, typeNode: TypeNode, makeStatic: boolean): void { const property = createProperty( /*decorators*/ undefined, /*modifiers*/ makeStatic ? [createToken(SyntaxKind.StaticKeyword)] : undefined, @@ -156,10 +157,10 @@ namespace ts.codefix { /*questionToken*/ undefined, typeNode, /*initializer*/ undefined); - changeTracker.insertNodeAtClassStart(classDeclarationSourceFile, classDeclaration, property, newLineCharacter); + changeTracker.insertNodeAtClassStart(classDeclarationSourceFile, classDeclaration, property); } - function createAddIndexSignatureAction(context: CodeFixContext, classDeclarationSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, tokenName: string, typeNode: TypeNode): CodeFixAction { + function createAddIndexSignatureAction(context: textChanges.TextChangesContext, classDeclarationSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, tokenName: string, typeNode: TypeNode): CodeFixAction { // Index signatures cannot have the static modifier. const stringTypeNode = createKeywordTypeNode(SyntaxKind.StringKeyword); const indexingParameter = createParameter( @@ -176,19 +177,19 @@ namespace ts.codefix { [indexingParameter], typeNode); - const changes = textChanges.ChangeTracker.with(context, t => t.insertNodeAtClassStart(classDeclarationSourceFile, classDeclaration, indexSignature, context.newLineCharacter)); + const changes = textChanges.ChangeTracker.with(context, t => t.insertNodeAtClassStart(classDeclarationSourceFile, classDeclaration, indexSignature)); // No fixId here because code-fix-all currently only works on adding individual named properties. return { description: formatStringFromArgs(getLocaleSpecificMessage(Diagnostics.Add_index_signature_for_property_0), [tokenName]), changes, fixId: undefined }; } - function getActionForMethodDeclaration(context: CodeFixContext, classDeclarationSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, token: Identifier, callExpression: CallExpression, makeStatic: boolean, inJs: boolean): CodeFixAction | undefined { + function getActionForMethodDeclaration(context: textChanges.TextChangesContext, classDeclarationSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, token: Identifier, callExpression: CallExpression, makeStatic: boolean, inJs: boolean): CodeFixAction | undefined { const description = formatStringFromArgs(getLocaleSpecificMessage(makeStatic ? Diagnostics.Declare_static_method_0 : Diagnostics.Declare_method_0), [token.text]); - const changes = textChanges.ChangeTracker.with(context, t => addMethodDeclaration(t, classDeclarationSourceFile, classDeclaration, token, callExpression, context.newLineCharacter, makeStatic, inJs)); + const changes = textChanges.ChangeTracker.with(context, t => addMethodDeclaration(t, classDeclarationSourceFile, classDeclaration, token, callExpression, makeStatic, inJs)); return { description, changes, fixId }; } - function addMethodDeclaration(changeTracker: textChanges.ChangeTracker, classDeclarationSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, token: Identifier, callExpression: CallExpression, newLineCharacter: string, makeStatic: boolean, inJs: boolean) { + function addMethodDeclaration(changeTracker: textChanges.ChangeTracker, classDeclarationSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, token: Identifier, callExpression: CallExpression, makeStatic: boolean, inJs: boolean) { const methodDeclaration = createMethodFromCallExpression(callExpression, token.text, inJs, makeStatic); - changeTracker.insertNodeAtClassStart(classDeclarationSourceFile, classDeclaration, methodDeclaration, newLineCharacter); + changeTracker.insertNodeAtClassStart(classDeclarationSourceFile, classDeclaration, methodDeclaration); } } diff --git a/src/services/codefixes/fixClassDoesntImplementInheritedAbstractMember.ts b/src/services/codefixes/fixClassDoesntImplementInheritedAbstractMember.ts index 754735a9d6353..da3685339ab21 100644 --- a/src/services/codefixes/fixClassDoesntImplementInheritedAbstractMember.ts +++ b/src/services/codefixes/fixClassDoesntImplementInheritedAbstractMember.ts @@ -10,12 +10,12 @@ namespace ts.codefix { getCodeActions(context) { const { program, sourceFile, span } = context; const changes = textChanges.ChangeTracker.with(context, t => - addMissingMembers(getClass(sourceFile, span.start), sourceFile, program.getTypeChecker(), context.newLineCharacter, t)); + addMissingMembers(getClass(sourceFile, span.start), sourceFile, program.getTypeChecker(), t)); return changes.length === 0 ? undefined : [{ description: getLocaleSpecificMessage(Diagnostics.Implement_inherited_abstract_class), changes, fixId }]; }, fixIds: [fixId], getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => { - addMissingMembers(getClass(diag.file!, diag.start!), context.sourceFile, context.program.getTypeChecker(), context.newLineCharacter, changes); + addMissingMembers(getClass(diag.file!, diag.start!), context.sourceFile, context.program.getTypeChecker(), changes); }), }); @@ -28,7 +28,7 @@ namespace ts.codefix { return classDeclaration as ClassLikeDeclaration; } - function addMissingMembers(classDeclaration: ClassLikeDeclaration, sourceFile: SourceFile, checker: TypeChecker, newLineCharacter: string, changeTracker: textChanges.ChangeTracker): void { + function addMissingMembers(classDeclaration: ClassLikeDeclaration, sourceFile: SourceFile, checker: TypeChecker, changeTracker: textChanges.ChangeTracker): void { const extendsNode = getClassExtendsHeritageClauseElement(classDeclaration); const instantiatedExtendsType = checker.getTypeAtLocation(extendsNode); @@ -36,7 +36,7 @@ namespace ts.codefix { // so duplicates cannot occur. const abstractAndNonPrivateExtendsSymbols = checker.getPropertiesOfType(instantiatedExtendsType).filter(symbolPointsToNonPrivateAndAbstractMember); - createMissingMemberNodes(classDeclaration, abstractAndNonPrivateExtendsSymbols, checker, member => changeTracker.insertNodeAtClassStart(sourceFile, classDeclaration, member, newLineCharacter)); + createMissingMemberNodes(classDeclaration, abstractAndNonPrivateExtendsSymbols, checker, member => changeTracker.insertNodeAtClassStart(sourceFile, classDeclaration, member)); } function symbolPointsToNonPrivateAndAbstractMember(symbol: Symbol): boolean { diff --git a/src/services/codefixes/fixClassIncorrectlyImplementsInterface.ts b/src/services/codefixes/fixClassIncorrectlyImplementsInterface.ts index 7076f7da34439..f7d5a4b9342c8 100644 --- a/src/services/codefixes/fixClassIncorrectlyImplementsInterface.ts +++ b/src/services/codefixes/fixClassIncorrectlyImplementsInterface.ts @@ -5,11 +5,11 @@ namespace ts.codefix { registerCodeFix({ errorCodes, getCodeActions(context) { - const { newLineCharacter, program, sourceFile, span } = context; + const { program, sourceFile, span } = context; const classDeclaration = getClass(sourceFile, span.start); const checker = program.getTypeChecker(); return mapDefined(getClassImplementsHeritageClauseElements(classDeclaration), implementedTypeNode => { - const changes = textChanges.ChangeTracker.with(context, t => addMissingDeclarations(checker, implementedTypeNode, sourceFile, classDeclaration, newLineCharacter, t)); + const changes = textChanges.ChangeTracker.with(context, t => addMissingDeclarations(checker, implementedTypeNode, sourceFile, classDeclaration, t)); if (changes.length === 0) return undefined; const description = formatStringFromArgs(getLocaleSpecificMessage(Diagnostics.Implement_interface_0), [implementedTypeNode.getText()]); return { description, changes, fixId }; @@ -22,7 +22,7 @@ namespace ts.codefix { const classDeclaration = getClass(diag.file!, diag.start!); if (addToSeen(seenClassDeclarations, getNodeId(classDeclaration))) { for (const implementedTypeNode of getClassImplementsHeritageClauseElements(classDeclaration)) { - addMissingDeclarations(context.program.getTypeChecker(), implementedTypeNode, diag.file!, classDeclaration, context.newLineCharacter, changes); + addMissingDeclarations(context.program.getTypeChecker(), implementedTypeNode, diag.file!, classDeclaration, changes); } } }); @@ -40,7 +40,6 @@ namespace ts.codefix { implementedTypeNode: ExpressionWithTypeArguments, sourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, - newLineCharacter: string, changeTracker: textChanges.ChangeTracker ): void { // Note that this is ultimately derived from a map indexed by symbol names, @@ -58,12 +57,12 @@ namespace ts.codefix { createMissingIndexSignatureDeclaration(implementedType, IndexKind.String); } - createMissingMemberNodes(classDeclaration, nonPrivateMembers, checker, member => changeTracker.insertNodeAtClassStart(sourceFile, classDeclaration, member, newLineCharacter)); + createMissingMemberNodes(classDeclaration, nonPrivateMembers, checker, member => changeTracker.insertNodeAtClassStart(sourceFile, classDeclaration, member)); function createMissingIndexSignatureDeclaration(type: InterfaceType, kind: IndexKind): void { const indexInfoOfKind = checker.getIndexInfoOfType(type, kind); if (indexInfoOfKind) { - changeTracker.insertNodeAtClassStart(sourceFile, classDeclaration, checker.indexInfoToIndexSignatureDeclaration(indexInfoOfKind, kind, classDeclaration), newLineCharacter); + changeTracker.insertNodeAtClassStart(sourceFile, classDeclaration, checker.indexInfoToIndexSignatureDeclaration(indexInfoOfKind, kind, classDeclaration)); } } } diff --git a/src/services/codefixes/fixClassSuperMustPrecedeThisAccess.ts b/src/services/codefixes/fixClassSuperMustPrecedeThisAccess.ts index 9b67c3469cf0b..1595bbf3c1383 100644 --- a/src/services/codefixes/fixClassSuperMustPrecedeThisAccess.ts +++ b/src/services/codefixes/fixClassSuperMustPrecedeThisAccess.ts @@ -5,30 +5,30 @@ namespace ts.codefix { registerCodeFix({ errorCodes, getCodeActions(context) { - const { sourceFile } = context; - const nodes = getNodes(sourceFile, context.span.start); + const { sourceFile, span } = context; + const nodes = getNodes(sourceFile, span.start); if (!nodes) return undefined; const { constructor, superCall } = nodes; - const changes = textChanges.ChangeTracker.with(context, t => doChange(t, sourceFile, constructor, superCall, context.newLineCharacter)); + const changes = textChanges.ChangeTracker.with(context, t => doChange(t, sourceFile, constructor, superCall)); return [{ description: getLocaleSpecificMessage(Diagnostics.Make_super_call_the_first_statement_in_the_constructor), changes, fixId }]; }, fixIds: [fixId], getAllCodeActions(context) { - const { newLineCharacter, sourceFile } = context; + const { sourceFile } = context; const seenClasses = createMap(); // Ensure we only do this once per class. return codeFixAll(context, errorCodes, (changes, diag) => { const nodes = getNodes(diag.file!, diag.start!); if (!nodes) return; const { constructor, superCall } = nodes; if (addToSeen(seenClasses, getNodeId(constructor.parent))) { - doChange(changes, sourceFile, constructor, superCall, newLineCharacter); + doChange(changes, sourceFile, constructor, superCall); } }); }, }); - function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, constructor: ConstructorDeclaration, superCall: ExpressionStatement, newLineCharacter: string): void { - changes.insertNodeAtConstructorStart(sourceFile, constructor, superCall, newLineCharacter); + function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, constructor: ConstructorDeclaration, superCall: ExpressionStatement): void { + changes.insertNodeAtConstructorStart(sourceFile, constructor, superCall); changes.deleteNode(sourceFile, superCall); } diff --git a/src/services/codefixes/fixConstructorForDerivedNeedSuperCall.ts b/src/services/codefixes/fixConstructorForDerivedNeedSuperCall.ts index 4c5b24fb0ec8a..8fe2e8458a86b 100644 --- a/src/services/codefixes/fixConstructorForDerivedNeedSuperCall.ts +++ b/src/services/codefixes/fixConstructorForDerivedNeedSuperCall.ts @@ -5,14 +5,14 @@ namespace ts.codefix { registerCodeFix({ errorCodes, getCodeActions(context) { - const { sourceFile } = context; - const ctr = getNode(sourceFile, context.span.start); - const changes = textChanges.ChangeTracker.with(context, t => doChange(t, sourceFile, ctr, context.newLineCharacter)); + const { sourceFile, span } = context; + const ctr = getNode(sourceFile, span.start); + const changes = textChanges.ChangeTracker.with(context, t => doChange(t, sourceFile, ctr)); return [{ description: getLocaleSpecificMessage(Diagnostics.Add_missing_super_call), changes, fixId }]; }, fixIds: [fixId], getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => - doChange(changes, context.sourceFile, getNode(diag.file, diag.start!), context.newLineCharacter)), + doChange(changes, context.sourceFile, getNode(diag.file, diag.start!))), }); function getNode(sourceFile: SourceFile, pos: number): ConstructorDeclaration { @@ -21,8 +21,8 @@ namespace ts.codefix { return token.parent as ConstructorDeclaration; } - function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, ctr: ConstructorDeclaration, newLineCharacter: string) { + function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, ctr: ConstructorDeclaration) { const superCall = createStatement(createCall(createSuper(), /*typeArguments*/ undefined, /*argumentsArray*/ emptyArray)); - changes.insertNodeAtConstructorStart(sourceFile, ctr, superCall, newLineCharacter); + changes.insertNodeAtConstructorStart(sourceFile, ctr, superCall); } } diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 05a92edf12aff..c399d302ea3cf 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -257,7 +257,7 @@ namespace ts.codefix { } function getCodeActionForNewImport(context: SymbolContext & { kind: ImportKind }, moduleSpecifier: string): ImportCodeAction { - const { kind, sourceFile, newLineCharacter, symbolName } = context; + const { kind, sourceFile, symbolName } = context; const lastImportDeclaration = findLast(sourceFile.statements, isAnyImportSyntax); const moduleSpecifierWithoutQuotes = stripQuotes(moduleSpecifier); @@ -275,6 +275,7 @@ namespace ts.codefix { createExternalModuleReference(quotedModuleSpecifier)); const changes = ChangeTracker.with(context, changeTracker => { + const { newLineCharacter } = changeTracker; if (lastImportDeclaration) { changeTracker.insertNodeAfter(sourceFile, lastImportDeclaration, importDecl, { suffix: newLineCharacter }); } diff --git a/src/services/refactors/convertFunctionToEs6Class.ts b/src/services/refactors/convertFunctionToEs6Class.ts index b66d14ee44961..1d6233a23c511 100644 --- a/src/services/refactors/convertFunctionToEs6Class.ts +++ b/src/services/refactors/convertFunctionToEs6Class.ts @@ -50,7 +50,6 @@ namespace ts.refactor.convertFunctionToES6Class { const { file: sourceFile } = context; const ctorSymbol = getConstructorSymbol(context); - const newLine = context.formatContext.options.newLineCharacter; const deletedNodes: Node[] = []; const deletes: (() => any)[] = []; @@ -88,7 +87,7 @@ namespace ts.refactor.convertFunctionToES6Class { } // Because the preceding node could be touched, we need to insert nodes before delete nodes. - changeTracker.insertNodeAfter(sourceFile, precedingNode, newClassDeclaration, { suffix: newLine }); + changeTracker.insertNodeAfter(sourceFile, precedingNode, newClassDeclaration, { suffix: changeTracker.newLineCharacter }); for (const deleteCallback of deletes) { deleteCallback(); } diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index f1a461b0088fc..4bebaaefd10f0 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -810,13 +810,14 @@ namespace ts.refactor.extractSymbol { const changeTracker = textChanges.ChangeTracker.fromContext(context); const minInsertionPos = (isReadonlyArray(range.range) ? last(range.range) : range.range).end; const nodeToInsertBefore = getNodeToInsertFunctionBefore(minInsertionPos, scope); + const { newLineCharacter } = changeTracker; if (nodeToInsertBefore) { - changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newFunction, { suffix: context.newLineCharacter + context.newLineCharacter }); + changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newFunction, { suffix: newLineCharacter + newLineCharacter }); } else { changeTracker.insertNodeBefore(context.file, scope.getLastToken(), newFunction, { - prefix: isLineBreak(file.text.charCodeAt(scope.getLastToken().pos)) ? context.newLineCharacter : context.newLineCharacter + context.newLineCharacter, - suffix: context.newLineCharacter + prefix: isLineBreak(file.text.charCodeAt(scope.getLastToken().pos)) ? newLineCharacter : newLineCharacter + newLineCharacter, + suffix: newLineCharacter }); } @@ -963,7 +964,7 @@ namespace ts.refactor.extractSymbol { const replacementRange = isReadonlyArray(range.range) ? { pos: first(range.range).getStart(), end: last(range.range).end } : { pos: range.range.getStart(), end: range.range.end }; - changeTracker.replaceRangeWithNodes(context.file, replacementRange, newNodes, { nodeSeparator: context.newLineCharacter }); + changeTracker.replaceRangeWithNodes(context.file, replacementRange, newNodes, { nodeSeparator: newLineCharacter }); const edits = changeTracker.getChanges(); const renameRange = isReadonlyArray(range.range) ? first(range.range) : range.range; @@ -1014,6 +1015,7 @@ namespace ts.refactor.extractSymbol { suppressLeadingAndTrailingTrivia(initializer); const changeTracker = textChanges.ChangeTracker.fromContext(context); + const { newLineCharacter } = changeTracker; if (isClassLike(scope)) { Debug.assert(!isJS); // See CannotExtractToJSClass @@ -1041,7 +1043,7 @@ namespace ts.refactor.extractSymbol { // Declare const maxInsertionPos = node.pos; const nodeToInsertBefore = getNodeToInsertPropertyBefore(maxInsertionPos, scope); - changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newVariable, { suffix: context.newLineCharacter + context.newLineCharacter }); + changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newVariable, { suffix: newLineCharacter + newLineCharacter }); // Consume changeTracker.replaceRange(context.file, { pos: node.getStart(), end: node.end }, localReference); @@ -1084,12 +1086,12 @@ namespace ts.refactor.extractSymbol { // for imports. const insertionPos = getSourceFileImportLocation(file); changeTracker.insertNodeAt(context.file, insertionPos, newVariableStatement, { - prefix: insertionPos === 0 ? undefined : context.newLineCharacter, - suffix: isLineBreak(file.text.charCodeAt(insertionPos)) ? context.newLineCharacter : context.newLineCharacter + context.newLineCharacter + prefix: insertionPos === 0 ? undefined : newLineCharacter, + suffix: isLineBreak(file.text.charCodeAt(insertionPos)) ? newLineCharacter : newLineCharacter + newLineCharacter }); } else { - changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newVariableStatement, { suffix: context.newLineCharacter + context.newLineCharacter }); + changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newVariableStatement, { suffix: newLineCharacter + newLineCharacter }); } // Consume diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index ee4b9091565ed..c5c06f7a890be 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -193,7 +193,7 @@ namespace ts.textChanges { export class ChangeTracker { private changes: Change[] = []; - private readonly newLineCharacter: string; + readonly newLineCharacter: string; private readonly deletedNodesInLists: true[] = []; // Stores ids of nodes in lists that we already deleted. Used to avoid deleting `, ` twice in `a, b`. // Map from class id to nodes to insert at the start private readonly nodesInsertedAtClassStarts = createMap<{ sourceFile: SourceFile, cls: ClassLikeDeclaration, members: ClassElement[] }>(); @@ -351,17 +351,17 @@ namespace ts.textChanges { return this.replaceWithSingle(sourceFile, startPosition, startPosition, newNode, options); } - public insertNodeAtConstructorStart(sourceFile: SourceFile, ctr: ConstructorDeclaration, newStatement: Statement, newLineCharacter: string): void { + public insertNodeAtConstructorStart(sourceFile: SourceFile, ctr: ConstructorDeclaration, newStatement: Statement): void { const firstStatement = firstOrUndefined(ctr.body.statements); if (!firstStatement || !ctr.body.multiLine) { this.replaceNode(sourceFile, ctr.body, createBlock([newStatement, ...ctr.body.statements], /*multiLine*/ true), { useNonAdjustedEndPosition: true }); } else { - this.insertNodeBefore(sourceFile, firstStatement, newStatement, { suffix: newLineCharacter }); + this.insertNodeBefore(sourceFile, firstStatement, newStatement, { suffix: this.newLineCharacter }); } } - public insertNodeAtClassStart(sourceFile: SourceFile, cls: ClassLikeDeclaration, newElement: ClassElement, newLineCharacter: string): void { + public insertNodeAtClassStart(sourceFile: SourceFile, cls: ClassLikeDeclaration, newElement: ClassElement): void { const firstMember = firstOrUndefined(cls.members); if (!firstMember) { const id = getNodeId(cls).toString(); @@ -375,7 +375,7 @@ namespace ts.textChanges { } } else { - this.insertNodeBefore(sourceFile, firstMember, newElement, { suffix: newLineCharacter }); + this.insertNodeBefore(sourceFile, firstMember, newElement, { suffix: this.newLineCharacter }); } } From 9b769ccc61b7916281755df6db67d794a11697f1 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Mon, 11 Dec 2017 13:05:37 -0800 Subject: [PATCH 2/2] Don't require newLineCharacter as input to ChangeTracker methods, and make it private again --- src/compiler/core.ts | 2 + src/harness/unittests/textChanges.ts | 60 ++++---- src/services/codefixes/fixAddMissingMember.ts | 5 +- src/services/codefixes/importFixes.ts | 5 +- .../refactors/convertFunctionToEs6Class.ts | 2 +- src/services/refactors/extractSymbol.ts | 42 ++---- src/services/textChanges.ts | 139 ++++++++++++++---- src/services/utilities.ts | 42 ------ ...ractConstant_VariableList_MultipleLines.js | 2 +- ...ractConstant_VariableList_MultipleLines.ts | 2 +- .../reference/textChanges/insertNodeAfter2.js | 2 +- ... => insertNodeAfterVariableDeclaration.js} | 2 +- ...eAtConstructorStart-block with newline.js} | 1 - ...er3.js => insertNodeAtConstructorStart.js} | 0 ...{insertNodeAt1.js => insertNodeBefore3.js} | 2 +- .../fourslash/codeFixAddMissingMember4.ts | 2 +- .../fourslash/codeFixAddMissingMember6.ts | 2 +- .../codeFixAddMissingMember_all_js.ts | 7 +- .../fourslash/extract-method-uniqueName.ts | 2 - .../importNameCodeFixNewImportAmbient2.ts | 1 + .../importNameCodeFixNewImportFile1.ts | 1 + ...portNameCodeFixNewImportFileAllComments.ts | 1 + .../fourslash/importNameCodeFixShebang.ts | 1 + 23 files changed, 177 insertions(+), 148 deletions(-) rename tests/baselines/reference/textChanges/{insertNodeAt2.js => insertNodeAfterVariableDeclaration.js} (95%) rename tests/baselines/reference/textChanges/{insertNodeAfter3-block with newline.js => insertNodeAtConstructorStart-block with newline.js} (99%) rename tests/baselines/reference/textChanges/{insertNodeAfter3.js => insertNodeAtConstructorStart.js} (100%) rename tests/baselines/reference/textChanges/{insertNodeAt1.js => insertNodeBefore3.js} (100%) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index 6bc8c1f758b7f..8d7c3c9c59ad5 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -288,6 +288,8 @@ namespace ts { return undefined; } + export function findLast(array: ReadonlyArray, predicate: (element: T, index: number) => element is U): U | undefined; + export function findLast(array: ReadonlyArray, predicate: (element: T, index: number) => boolean): T | undefined; export function findLast(array: ReadonlyArray, predicate: (element: T, index: number) => boolean): T | undefined { for (let i = array.length - 1; i >= 0; i--) { const value = array[i]; diff --git a/src/harness/unittests/textChanges.ts b/src/harness/unittests/textChanges.ts index ed571b37399fa..934da4e3ba1e8 100644 --- a/src/harness/unittests/textChanges.ts +++ b/src/harness/unittests/textChanges.ts @@ -103,7 +103,7 @@ namespace M /*body */ createBlock(statements) ); - changeTracker.insertNodeBefore(sourceFile, /*before*/findChild("M2", sourceFile), newFunction, { suffix: newLineCharacter }); + changeTracker.insertNodeBefore(sourceFile, /*before*/findChild("M2", sourceFile), newFunction); // replace statements with return statement const newStatement = createReturn( @@ -129,12 +129,11 @@ function bar() { changeTracker.deleteRange(sourceFile, { pos: text.indexOf("function foo"), end: text.indexOf("function bar") }); }); } - function findVariableStatementContaining(name: string, sourceFile: SourceFile) { - const varDecl = findChild(name, sourceFile); - assert.equal(varDecl.kind, SyntaxKind.VariableDeclaration); - const varStatement = varDecl.parent.parent; - assert.equal(varStatement.kind, SyntaxKind.VariableStatement); - return varStatement; + function findVariableStatementContaining(name: string, sourceFile: SourceFile): VariableStatement { + return cast(findVariableDeclarationContaining(name, sourceFile).parent.parent, isVariableStatement); + } + function findVariableDeclarationContaining(name: string, sourceFile: SourceFile): VariableDeclaration { + return cast(findChild(name, sourceFile), isVariableDeclaration); } { const text = ` @@ -306,11 +305,11 @@ var y; // comment 4 var z = 3; // comment 5 // comment 6 var a = 4; // comment 7`; - runSingleFileTest("insertNodeAt1", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.insertNodeAt(sourceFile, text.indexOf("var y"), createTestClass(), { suffix: newLineCharacter }); + runSingleFileTest("insertNodeBefore3", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { + changeTracker.insertNodeBefore(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass()); }); - runSingleFileTest("insertNodeAt2", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ false, (sourceFile, changeTracker) => { - changeTracker.insertNodeAt(sourceFile, text.indexOf("; // comment 4"), createTestVariableDeclaration("z1")); + runSingleFileTest("insertNodeAfterVariableDeclaration", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ false, (sourceFile, changeTracker) => { + changeTracker.insertNodeAfter(sourceFile, findVariableDeclarationContaining("y", sourceFile), createTestVariableDeclaration("z1")); }); } { @@ -325,23 +324,22 @@ namespace M { var a = 4; // comment 7 }`; runSingleFileTest("insertNodeBefore1", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.insertNodeBefore(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { suffix: newLineCharacter }); + changeTracker.insertNodeBefore(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass()); }); runSingleFileTest("insertNodeBefore2", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.insertNodeBefore(sourceFile, findChild("M", sourceFile), createTestClass(), { suffix: newLineCharacter }); + changeTracker.insertNodeBefore(sourceFile, findChild("M", sourceFile), createTestClass()); }); runSingleFileTest("insertNodeAfter1", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.insertNodeAfter(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass(), { suffix: newLineCharacter }); + changeTracker.insertNodeAfter(sourceFile, findVariableStatementContaining("y", sourceFile), createTestClass()); }); runSingleFileTest("insertNodeAfter2", /*placeOpenBraceOnNewLineForFunctions*/ true, text, /*validateNodes*/ true, (sourceFile, changeTracker) => { - changeTracker.insertNodeAfter(sourceFile, findChild("M", sourceFile), createTestClass(), { prefix: newLineCharacter }); + changeTracker.insertNodeAfter(sourceFile, findChild("M", sourceFile), createTestClass()); }); } - function findOpenBraceForConstructor(sourceFile: SourceFile) { + function findConstructor(sourceFile: SourceFile): ConstructorDeclaration { const classDecl = sourceFile.statements[0]; - const constructorDecl = forEach(classDecl.members, m => m.kind === SyntaxKind.Constructor && (m).body && m); - return constructorDecl.body.getFirstToken(); + return find(classDecl.members, (m): m is ConstructorDeclaration => isConstructorDeclaration(m) && !!m.body)!; } function createTestSuperCall() { const superCall = createCall( @@ -359,8 +357,8 @@ class A { } } `; - runSingleFileTest("insertNodeAfter3", /*placeOpenBraceOnNewLineForFunctions*/ false, text1, /*validateNodes*/ false, (sourceFile, changeTracker) => { - changeTracker.insertNodeAfter(sourceFile, findOpenBraceForConstructor(sourceFile), createTestSuperCall(), { suffix: newLineCharacter }); + runSingleFileTest("insertNodeAtConstructorStart", /*placeOpenBraceOnNewLineForFunctions*/ false, text1, /*validateNodes*/ false, (sourceFile, changeTracker) => { + changeTracker.insertNodeAtConstructorStart(sourceFile, findConstructor(sourceFile), createTestSuperCall()); }); const text2 = ` class A { @@ -370,7 +368,7 @@ class A { } `; runSingleFileTest("insertNodeAfter4", /*placeOpenBraceOnNewLineForFunctions*/ false, text2, /*validateNodes*/ false, (sourceFile, changeTracker) => { - changeTracker.insertNodeAfter(sourceFile, findVariableStatementContaining("x", sourceFile), createTestSuperCall(), { suffix: newLineCharacter }); + changeTracker.insertNodeAfter(sourceFile, findVariableStatementContaining("x", sourceFile), createTestSuperCall()); }); const text3 = ` class A { @@ -379,8 +377,8 @@ class A { } } `; - runSingleFileTest("insertNodeAfter3-block with newline", /*placeOpenBraceOnNewLineForFunctions*/ false, text3, /*validateNodes*/ false, (sourceFile, changeTracker) => { - changeTracker.insertNodeAfter(sourceFile, findOpenBraceForConstructor(sourceFile), createTestSuperCall(), { suffix: newLineCharacter }); + runSingleFileTest("insertNodeAtConstructorStart-block with newline", /*placeOpenBraceOnNewLineForFunctions*/ false, text3, /*validateNodes*/ false, (sourceFile, changeTracker) => { + changeTracker.insertNodeAtConstructorStart(sourceFile, findConstructor(sourceFile), createTestSuperCall()); }); } { @@ -638,7 +636,7 @@ class A { } const insertAfter = findChild("x", sourceFile); for (const newNode of newNodes) { - changeTracker.insertNodeAfter(sourceFile, insertAfter, newNode, { suffix: newLineCharacter }); + changeTracker.insertNodeAfter(sourceFile, insertAfter, newNode); } }); } @@ -649,7 +647,7 @@ class A { } `; runSingleFileTest("insertNodeAfterInClass1", /*placeOpenBraceOnNewLineForFunctions*/ false, text, /*validateNodes*/ false, (sourceFile, changeTracker) => { - changeTracker.insertNodeAfter(sourceFile, findChild("x", sourceFile), createProperty(undefined, undefined, "a", undefined, createKeywordTypeNode(SyntaxKind.BooleanKeyword), undefined), { suffix: newLineCharacter }); + changeTracker.insertNodeAfter(sourceFile, findChild("x", sourceFile), createProperty(undefined, undefined, "a", undefined, createKeywordTypeNode(SyntaxKind.BooleanKeyword), undefined)); }); } { @@ -659,7 +657,7 @@ class A { } `; runSingleFileTest("insertNodeAfterInClass2", /*placeOpenBraceOnNewLineForFunctions*/ false, text, /*validateNodes*/ false, (sourceFile, changeTracker) => { - changeTracker.insertNodeAfter(sourceFile, findChild("x", sourceFile), createProperty(undefined, undefined, "a", undefined, createKeywordTypeNode(SyntaxKind.BooleanKeyword), undefined), { suffix: newLineCharacter }); + changeTracker.insertNodeAfter(sourceFile, findChild("x", sourceFile), createProperty(undefined, undefined, "a", undefined, createKeywordTypeNode(SyntaxKind.BooleanKeyword), undefined)); }); } { @@ -698,7 +696,7 @@ class A { /*questionToken*/ undefined, createKeywordTypeNode(SyntaxKind.AnyKeyword), /*initializer*/ undefined); - changeTracker.insertNodeAfter(sourceFile, findChild("x", sourceFile), newNode, { suffix: newLineCharacter }); + changeTracker.insertNodeAfter(sourceFile, findChild("x", sourceFile), newNode); }); } { @@ -716,7 +714,7 @@ class A { /*questionToken*/ undefined, createKeywordTypeNode(SyntaxKind.AnyKeyword), /*initializer*/ undefined); - changeTracker.insertNodeAfter(sourceFile, findChild("x", sourceFile), newNode, { suffix: newLineCharacter }); + changeTracker.insertNodeAfter(sourceFile, findChild("x", sourceFile), newNode); }); } { @@ -733,7 +731,7 @@ interface A { /*questionToken*/ undefined, createKeywordTypeNode(SyntaxKind.AnyKeyword), /*initializer*/ undefined); - changeTracker.insertNodeAfter(sourceFile, findChild("x", sourceFile), newNode, { suffix: newLineCharacter }); + changeTracker.insertNodeAfter(sourceFile, findChild("x", sourceFile), newNode); }); } { @@ -750,7 +748,7 @@ interface A { /*questionToken*/ undefined, createKeywordTypeNode(SyntaxKind.AnyKeyword), /*initializer*/ undefined); - changeTracker.insertNodeAfter(sourceFile, findChild("x", sourceFile), newNode, { suffix: newLineCharacter }); + changeTracker.insertNodeAfter(sourceFile, findChild("x", sourceFile), newNode); }); } { @@ -759,7 +757,7 @@ let x = foo `; runSingleFileTest("insertNodeInStatementListAfterNodeWithoutSeparator1", /*placeOpenBraceOnNewLineForFunctions*/ false, text, /*validateNodes*/ false, (sourceFile, changeTracker) => { const newNode = createStatement(createParen(createLiteral(1))); - changeTracker.insertNodeAfter(sourceFile, findVariableStatementContaining("x", sourceFile), newNode, { suffix: newLineCharacter }); + changeTracker.insertNodeAfter(sourceFile, findVariableStatementContaining("x", sourceFile), newNode); }); } }); diff --git a/src/services/codefixes/fixAddMissingMember.ts b/src/services/codefixes/fixAddMissingMember.ts index b2594971a0e52..0fc6a430e192a 100644 --- a/src/services/codefixes/fixAddMissingMember.ts +++ b/src/services/codefixes/fixAddMissingMember.ts @@ -103,14 +103,13 @@ namespace ts.codefix { } function addMissingMemberInJs(changeTracker: textChanges.ChangeTracker, classDeclarationSourceFile: SourceFile, classDeclaration: ClassLikeDeclaration, tokenName: string, makeStatic: boolean): void { - const { newLineCharacter } = changeTracker; if (makeStatic) { if (classDeclaration.kind === SyntaxKind.ClassExpression) { return; } const className = classDeclaration.name.getText(); const staticInitialization = initializePropertyToUndefined(createIdentifier(className), tokenName); - changeTracker.insertNodeAfter(classDeclarationSourceFile, classDeclaration, staticInitialization, { prefix: newLineCharacter, suffix: newLineCharacter }); + changeTracker.insertNodeAfter(classDeclarationSourceFile, classDeclaration, staticInitialization); } else { const classConstructor = getFirstConstructorWithBody(classDeclaration); @@ -118,7 +117,7 @@ namespace ts.codefix { return; } const propertyInitialization = initializePropertyToUndefined(createThis(), tokenName); - changeTracker.insertNodeBefore(classDeclarationSourceFile, classConstructor.body.getLastToken(), propertyInitialization, { suffix: newLineCharacter }); + changeTracker.insertNodeAtConstructorEnd(classDeclarationSourceFile, classConstructor, propertyInitialization); } } diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index c399d302ea3cf..0dd7cf2ca2568 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -275,12 +275,11 @@ namespace ts.codefix { createExternalModuleReference(quotedModuleSpecifier)); const changes = ChangeTracker.with(context, changeTracker => { - const { newLineCharacter } = changeTracker; if (lastImportDeclaration) { - changeTracker.insertNodeAfter(sourceFile, lastImportDeclaration, importDecl, { suffix: newLineCharacter }); + changeTracker.insertNodeAfter(sourceFile, lastImportDeclaration, importDecl); } else { - changeTracker.insertNodeAt(sourceFile, getSourceFileImportLocation(sourceFile), importDecl, { suffix: `${newLineCharacter}${newLineCharacter}` }); + changeTracker.insertNodeAtTopOfFile(sourceFile, importDecl); } }); diff --git a/src/services/refactors/convertFunctionToEs6Class.ts b/src/services/refactors/convertFunctionToEs6Class.ts index 1d6233a23c511..cddf40ae01740 100644 --- a/src/services/refactors/convertFunctionToEs6Class.ts +++ b/src/services/refactors/convertFunctionToEs6Class.ts @@ -87,7 +87,7 @@ namespace ts.refactor.convertFunctionToES6Class { } // Because the preceding node could be touched, we need to insert nodes before delete nodes. - changeTracker.insertNodeAfter(sourceFile, precedingNode, newClassDeclaration, { suffix: changeTracker.newLineCharacter }); + changeTracker.insertNodeAfter(sourceFile, precedingNode, newClassDeclaration); for (const deleteCallback of deletes) { deleteCallback(); } diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 4bebaaefd10f0..e1ed7fdaa3c51 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -810,15 +810,11 @@ namespace ts.refactor.extractSymbol { const changeTracker = textChanges.ChangeTracker.fromContext(context); const minInsertionPos = (isReadonlyArray(range.range) ? last(range.range) : range.range).end; const nodeToInsertBefore = getNodeToInsertFunctionBefore(minInsertionPos, scope); - const { newLineCharacter } = changeTracker; if (nodeToInsertBefore) { - changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newFunction, { suffix: newLineCharacter + newLineCharacter }); + changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newFunction, /*blankLineBetween*/ true); } else { - changeTracker.insertNodeBefore(context.file, scope.getLastToken(), newFunction, { - prefix: isLineBreak(file.text.charCodeAt(scope.getLastToken().pos)) ? newLineCharacter : newLineCharacter + newLineCharacter, - suffix: newLineCharacter - }); + changeTracker.insertNodeAtEndOfScope(context.file, scope, newFunction); } const newNodes: Node[] = []; @@ -961,10 +957,12 @@ namespace ts.refactor.extractSymbol { } } - const replacementRange = isReadonlyArray(range.range) - ? { pos: first(range.range).getStart(), end: last(range.range).end } - : { pos: range.range.getStart(), end: range.range.end }; - changeTracker.replaceRangeWithNodes(context.file, replacementRange, newNodes, { nodeSeparator: newLineCharacter }); + if (isReadonlyArray(range.range)) { + changeTracker.replaceNodesWithNodes(context.file, range.range, newNodes); + } + else { + changeTracker.replaceNodeWithNodes(context.file, range.range, newNodes); + } const edits = changeTracker.getChanges(); const renameRange = isReadonlyArray(range.range) ? first(range.range) : range.range; @@ -1015,7 +1013,6 @@ namespace ts.refactor.extractSymbol { suppressLeadingAndTrailingTrivia(initializer); const changeTracker = textChanges.ChangeTracker.fromContext(context); - const { newLineCharacter } = changeTracker; if (isClassLike(scope)) { Debug.assert(!isJS); // See CannotExtractToJSClass @@ -1043,7 +1040,7 @@ namespace ts.refactor.extractSymbol { // Declare const maxInsertionPos = node.pos; const nodeToInsertBefore = getNodeToInsertPropertyBefore(maxInsertionPos, scope); - changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newVariable, { suffix: newLineCharacter + newLineCharacter }); + changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newVariable, /*blankLineBetween*/ true); // Consume changeTracker.replaceRange(context.file, { pos: node.getStart(), end: node.end }, localReference); @@ -1058,8 +1055,8 @@ namespace ts.refactor.extractSymbol { const oldVariableDeclaration = getContainingVariableDeclarationIfInList(node, scope); if (oldVariableDeclaration) { // Declare - // CONSIDER: could detect that each is on a separate line - changeTracker.insertNodeAt(context.file, oldVariableDeclaration.getStart(), newVariableDeclaration, { suffix: ", " }); + // CONSIDER: could detect that each is on a separate line (See `extractConstant_VariableList_MultipleLines` in `extractConstants.ts`) + changeTracker.insertNodeBefore(context.file, oldVariableDeclaration, newVariableDeclaration); // Consume const localReference = createIdentifier(localNameText); @@ -1081,17 +1078,10 @@ namespace ts.refactor.extractSymbol { // Declare const nodeToInsertBefore = getNodeToInsertConstantBefore(node, scope); if (nodeToInsertBefore.pos === 0) { - // If we're at the beginning of the file, we need to take care not to insert before header comments - // (e.g. copyright, triple-slash references). Fortunately, this problem has already been solved - // for imports. - const insertionPos = getSourceFileImportLocation(file); - changeTracker.insertNodeAt(context.file, insertionPos, newVariableStatement, { - prefix: insertionPos === 0 ? undefined : newLineCharacter, - suffix: isLineBreak(file.text.charCodeAt(insertionPos)) ? newLineCharacter : newLineCharacter + newLineCharacter - }); + changeTracker.insertNodeAtTopOfFile(context.file, newVariableStatement); } else { - changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newVariableStatement, { suffix: newLineCharacter + newLineCharacter }); + changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newVariableStatement, /*blankLineBetween*/ true); } // Consume @@ -1288,12 +1278,12 @@ namespace ts.refactor.extractSymbol { * If `scope` contains a function after `minPos`, then return the first such function. * Otherwise, return `undefined`. */ - function getNodeToInsertFunctionBefore(minPos: number, scope: Scope): Node | undefined { + function getNodeToInsertFunctionBefore(minPos: number, scope: Scope): Statement | ClassElement | undefined { return find(getStatementsOrClassElements(scope), child => child.pos >= minPos && isFunctionLikeDeclaration(child) && !isConstructorDeclaration(child)); } - function getNodeToInsertPropertyBefore(maxPos: number, scope: ClassLikeDeclaration): Node { + function getNodeToInsertPropertyBefore(maxPos: number, scope: ClassLikeDeclaration): ClassElement { const members = scope.members; Debug.assert(members.length > 0); // There must be at least one child, since we extracted from one. @@ -1319,7 +1309,7 @@ namespace ts.refactor.extractSymbol { return prevMember; } - function getNodeToInsertConstantBefore(node: Node, scope: Scope): Node { + function getNodeToInsertConstantBefore(node: Node, scope: Scope): Statement { Debug.assert(!isClassLike(scope)); let prevScope: Scope | undefined = undefined; diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index c5c06f7a890be..d9c592488e2c7 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -117,7 +117,7 @@ namespace ts.textChanges { readonly options?: never; } - export interface ChangeMultipleNodesOptions extends ChangeNodeOptions { + interface ChangeMultipleNodesOptions extends ChangeNodeOptions { nodeSeparator: string; } interface ReplaceWithMultipleNodes extends BaseChange { @@ -192,8 +192,8 @@ namespace ts.textChanges { } export class ChangeTracker { - private changes: Change[] = []; - readonly newLineCharacter: string; + private readonly changes: Change[] = []; + private readonly newLineCharacter: string; private readonly deletedNodesInLists: true[] = []; // Stores ids of nodes in lists that we already deleted. Used to avoid deleting `, ` twice in `a, b`. // Map from class id to nodes to insert at the start private readonly nodesInsertedAtClassStarts = createMap<{ sourceFile: SourceFile, cls: ClassLikeDeclaration, members: ClassElement[] }>(); @@ -319,48 +319,74 @@ namespace ts.textChanges { return this; } - public replaceNodeWithNodes(sourceFile: SourceFile, oldNode: Node, newNodes: ReadonlyArray, options: ChangeMultipleNodesOptions) { - const startPosition = getAdjustedStartPosition(sourceFile, oldNode, options, Position.Start); - const endPosition = getAdjustedEndPosition(sourceFile, oldNode, options); - return this.replaceWithMultiple(sourceFile, startPosition, endPosition, newNodes, options); + public replaceNodeWithNodes(sourceFile: SourceFile, oldNode: Node, newNodes: ReadonlyArray): void { + this.replaceWithMultiple(sourceFile, oldNode.getStart(sourceFile), oldNode.getEnd(), newNodes, { nodeSeparator: this.newLineCharacter }); } - public replaceNodesWithNodes(sourceFile: SourceFile, oldNodes: ReadonlyArray, newNodes: ReadonlyArray, options: ChangeMultipleNodesOptions) { - const startPosition = getAdjustedStartPosition(sourceFile, oldNodes[0], options, Position.Start); - const endPosition = getAdjustedEndPosition(sourceFile, lastOrUndefined(oldNodes), options); - return this.replaceWithMultiple(sourceFile, startPosition, endPosition, newNodes, options); + public replaceNodesWithNodes(sourceFile: SourceFile, oldNodes: ReadonlyArray, newNodes: ReadonlyArray): void { + this.replaceWithMultiple(sourceFile, first(oldNodes).getStart(sourceFile), last(oldNodes).getEnd(), newNodes, { nodeSeparator: this.newLineCharacter }); } - public replaceRangeWithNodes(sourceFile: SourceFile, range: TextRange, newNodes: ReadonlyArray, options: ChangeMultipleNodesOptions) { - return this.replaceWithMultiple(sourceFile, range.pos, range.end, newNodes, options); + private insertNodeAt(sourceFile: SourceFile, pos: number, newNode: Node, options: InsertNodeOptions = {}) { + this.changes.push({ kind: ChangeKind.ReplaceWithSingleNode, sourceFile, options, node: newNode, range: { pos, end: pos } }); + return this; } - public replaceNodeRangeWithNodes(sourceFile: SourceFile, startNode: Node, endNode: Node, newNodes: ReadonlyArray, options: ChangeMultipleNodesOptions) { - const startPosition = getAdjustedStartPosition(sourceFile, startNode, options, Position.Start); - const endPosition = getAdjustedEndPosition(sourceFile, endNode, options); - return this.replaceWithMultiple(sourceFile, startPosition, endPosition, newNodes, options); + public insertNodeAtTopOfFile(sourceFile: SourceFile, newNode: Statement): void { + const pos = getInsertionPositionAtSourceFileTop(sourceFile); + this.insertNodeAt(sourceFile, pos, newNode, { + prefix: pos === 0 ? undefined : this.newLineCharacter, + suffix: isLineBreak(sourceFile.text.charCodeAt(pos)) ? this.newLineCharacter : this.newLineCharacter + this.newLineCharacter, + }); } - public insertNodeAt(sourceFile: SourceFile, pos: number, newNode: Node, options: InsertNodeOptions = {}) { - this.changes.push({ kind: ChangeKind.ReplaceWithSingleNode, sourceFile, options, node: newNode, range: { pos, end: pos } }); - return this; + public insertNodeBefore(sourceFile: SourceFile, before: Node, newNode: Node, blankLineBetween = false) { + const startPosition = getAdjustedStartPosition(sourceFile, before, {}, Position.Start); + return this.replaceWithSingle(sourceFile, startPosition, startPosition, newNode, this.getOptionsForInsertNodeBefore(before, blankLineBetween)); } - public insertNodeBefore(sourceFile: SourceFile, before: Node, newNode: Node, options: InsertNodeOptions & ConfigurableStart = {}) { - const startPosition = getAdjustedStartPosition(sourceFile, before, options, Position.Start); - return this.replaceWithSingle(sourceFile, startPosition, startPosition, newNode, options); + private getOptionsForInsertNodeBefore(before: Node, doubleNewlines: boolean): ChangeNodeOptions { + if (isStatement(before) || isClassElement(before)) { + return { suffix: doubleNewlines ? this.newLineCharacter + this.newLineCharacter : this.newLineCharacter }; + } + else if (isVariableDeclaration(before)) { // insert `x = 1, ` into `const x = 1, y = 2; + return { suffix: ", " }; + } + throw Debug.failBadSyntaxKind(before); // We haven't handled this kind of node yet -- add it } public insertNodeAtConstructorStart(sourceFile: SourceFile, ctr: ConstructorDeclaration, newStatement: Statement): void { const firstStatement = firstOrUndefined(ctr.body.statements); if (!firstStatement || !ctr.body.multiLine) { - this.replaceNode(sourceFile, ctr.body, createBlock([newStatement, ...ctr.body.statements], /*multiLine*/ true), { useNonAdjustedEndPosition: true }); + this.replaceConstructorBody(sourceFile, ctr, [newStatement, ...ctr.body.statements]); + } + else { + this.insertNodeBefore(sourceFile, firstStatement, newStatement); + } + } + + public insertNodeAtConstructorEnd(sourceFile: SourceFile, ctr: ConstructorDeclaration, newStatement: Statement): void { + const lastStatement = lastOrUndefined(ctr.body.statements); + if (!lastStatement || !ctr.body.multiLine) { + this.replaceConstructorBody(sourceFile, ctr, [...ctr.body.statements, newStatement]); } else { - this.insertNodeBefore(sourceFile, firstStatement, newStatement, { suffix: this.newLineCharacter }); + this.insertNodeAfter(sourceFile, lastStatement, newStatement); } } + private replaceConstructorBody(sourceFile: SourceFile, ctr: ConstructorDeclaration, statements: ReadonlyArray): void { + this.replaceNode(sourceFile, ctr.body, createBlock(statements, /*multiLine*/ true), { useNonAdjustedEndPosition: true }); + } + + public insertNodeAtEndOfScope(sourceFile: SourceFile, scope: Node, newNode: Node): void { + const startPosition = getAdjustedStartPosition(sourceFile, scope.getLastToken(), {}, Position.Start); + this.replaceWithSingle(sourceFile, startPosition, startPosition, newNode, { + prefix: isLineBreak(sourceFile.text.charCodeAt(scope.getLastToken().pos)) ? this.newLineCharacter : this.newLineCharacter + this.newLineCharacter, + suffix: this.newLineCharacter + }); + } + public insertNodeAtClassStart(sourceFile: SourceFile, cls: ClassLikeDeclaration, newElement: ClassElement): void { const firstMember = firstOrUndefined(cls.members); if (!firstMember) { @@ -375,11 +401,11 @@ namespace ts.textChanges { } } else { - this.insertNodeBefore(sourceFile, firstMember, newElement, { suffix: this.newLineCharacter }); + this.insertNodeBefore(sourceFile, firstMember, newElement); } } - public insertNodeAfter(sourceFile: SourceFile, after: Node, newNode: Node, options: InsertNodeOptions & ConfigurableEnd = {}): this { + public insertNodeAfter(sourceFile: SourceFile, after: Node, newNode: Node): this { if (isStatementButNotDeclaration(after) || after.kind === SyntaxKind.PropertyDeclaration || after.kind === SyntaxKind.PropertySignature || @@ -396,8 +422,21 @@ namespace ts.textChanges { }); } } - const endPosition = getAdjustedEndPosition(sourceFile, after, options); - return this.replaceWithSingle(sourceFile, endPosition, endPosition, newNode, options); + const endPosition = getAdjustedEndPosition(sourceFile, after, {}); + return this.replaceWithSingle(sourceFile, endPosition, endPosition, newNode, this.getInsertNodeAfterOptions(after)); + } + + private getInsertNodeAfterOptions(node: Node): InsertNodeOptions { + if (isClassDeclaration(node) || isModuleDeclaration(node)) { + return { prefix: this.newLineCharacter, suffix: this.newLineCharacter }; + } + else if (isStatement(node) || isClassElement(node) || isTypeElement(node)) { + return { suffix: this.newLineCharacter }; + } + else if (isVariableDeclaration(node)) { + return { prefix: ", " }; + } + throw Debug.failBadSyntaxKind(node); // We haven't handled this kind of node yet -- add it } /** @@ -804,4 +843,46 @@ namespace ts.textChanges { this.lastNonTriviaPosition = 0; } } + + function getInsertionPositionAtSourceFileTop({ text }: SourceFile): number { + const shebang = getShebang(text); + let position = 0; + if (shebang !== undefined) { + position = shebang.length; + advancePastLineBreak(); + } + + // For a source file, it is possible there are detached comments we should not skip + let ranges = getLeadingCommentRanges(text, position); + if (!ranges) return position; + // However we should still skip a pinned comment at the top + if (ranges.length && ranges[0].kind === SyntaxKind.MultiLineCommentTrivia && isPinnedComment(text, ranges[0])) { + position = ranges[0].end; + advancePastLineBreak(); + ranges = ranges.slice(1); + } + // As well as any triple slash references + for (const range of ranges) { + if (range.kind === SyntaxKind.SingleLineCommentTrivia && isRecognizedTripleSlashComment(text, range.pos, range.end)) { + position = range.end; + advancePastLineBreak(); + continue; + } + break; + } + return position; + + function advancePastLineBreak() { + if (position < text.length) { + const charCode = text.charCodeAt(position); + if (isLineBreak(charCode)) { + position++; + + if (position < text.length && charCode === CharacterCodes.carriageReturn && text.charCodeAt(position) === CharacterCodes.lineFeed) { + position++; + } + } + } + } + } } diff --git a/src/services/utilities.ts b/src/services/utilities.ts index 358db5e8b4b99..08ad9c693ae7f 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -1338,48 +1338,6 @@ namespace ts { return position; } - export function getSourceFileImportLocation({ text }: SourceFile) { - const shebang = getShebang(text); - let position = 0; - if (shebang !== undefined) { - position = shebang.length; - advancePastLineBreak(); - } - - // For a source file, it is possible there are detached comments we should not skip - let ranges = getLeadingCommentRanges(text, position); - if (!ranges) return position; - // However we should still skip a pinned comment at the top - if (ranges.length && ranges[0].kind === SyntaxKind.MultiLineCommentTrivia && isPinnedComment(text, ranges[0])) { - position = ranges[0].end; - advancePastLineBreak(); - ranges = ranges.slice(1); - } - // As well as any triple slash references - for (const range of ranges) { - if (range.kind === SyntaxKind.SingleLineCommentTrivia && isRecognizedTripleSlashComment(text, range.pos, range.end)) { - position = range.end; - advancePastLineBreak(); - continue; - } - break; - } - return position; - - function advancePastLineBreak() { - if (position < text.length) { - const charCode = text.charCodeAt(position); - if (isLineBreak(charCode)) { - position++; - - if (position < text.length && charCode === CharacterCodes.carriageReturn && text.charCodeAt(position) === CharacterCodes.lineFeed) { - position++; - } - } - } - } - } - /** * Creates a deep, memberwise clone of a node with no source map location. * diff --git a/tests/baselines/reference/extractConstant/extractConstant_VariableList_MultipleLines.js b/tests/baselines/reference/extractConstant/extractConstant_VariableList_MultipleLines.js index 7112dc6313d1d..70229e77c2599 100644 --- a/tests/baselines/reference/extractConstant/extractConstant_VariableList_MultipleLines.js +++ b/tests/baselines/reference/extractConstant/extractConstant_VariableList_MultipleLines.js @@ -3,4 +3,4 @@ const /*About A*/a = 1, /*About B*/b = /*[#|*/a + 1/*|]*/; // ==SCOPE::Extract to constant in enclosing scope== const /*About A*/a = 1, - /*About B*/newLocal = a + 1, b = /*RENAME*/newLocal; \ No newline at end of file + newLocal = a + 1, /*About B*/b = /*RENAME*/newLocal; \ No newline at end of file diff --git a/tests/baselines/reference/extractConstant/extractConstant_VariableList_MultipleLines.ts b/tests/baselines/reference/extractConstant/extractConstant_VariableList_MultipleLines.ts index 7112dc6313d1d..70229e77c2599 100644 --- a/tests/baselines/reference/extractConstant/extractConstant_VariableList_MultipleLines.ts +++ b/tests/baselines/reference/extractConstant/extractConstant_VariableList_MultipleLines.ts @@ -3,4 +3,4 @@ const /*About A*/a = 1, /*About B*/b = /*[#|*/a + 1/*|]*/; // ==SCOPE::Extract to constant in enclosing scope== const /*About A*/a = 1, - /*About B*/newLocal = a + 1, b = /*RENAME*/newLocal; \ No newline at end of file + newLocal = a + 1, /*About B*/b = /*RENAME*/newLocal; \ No newline at end of file diff --git a/tests/baselines/reference/textChanges/insertNodeAfter2.js b/tests/baselines/reference/textChanges/insertNodeAfter2.js index 7c027e35b50fa..fbd24e2b75e8e 100644 --- a/tests/baselines/reference/textChanges/insertNodeAfter2.js +++ b/tests/baselines/reference/textChanges/insertNodeAfter2.js @@ -23,4 +23,4 @@ namespace M { public class class1 implements interface1 { property1: boolean; -} \ No newline at end of file +} diff --git a/tests/baselines/reference/textChanges/insertNodeAt2.js b/tests/baselines/reference/textChanges/insertNodeAfterVariableDeclaration.js similarity index 95% rename from tests/baselines/reference/textChanges/insertNodeAt2.js rename to tests/baselines/reference/textChanges/insertNodeAfterVariableDeclaration.js index 81c3e2afc3571..3dd0b1882f9bf 100644 --- a/tests/baselines/reference/textChanges/insertNodeAt2.js +++ b/tests/baselines/reference/textChanges/insertNodeAfterVariableDeclaration.js @@ -12,7 +12,7 @@ var a = 4; // comment 7 // comment 1 var x = 1; // comment 2 // comment 3 -var yz1 = { +var y, z1 = { p1: 1 }; // comment 4 var z = 3; // comment 5 diff --git a/tests/baselines/reference/textChanges/insertNodeAfter3-block with newline.js b/tests/baselines/reference/textChanges/insertNodeAtConstructorStart-block with newline.js similarity index 99% rename from tests/baselines/reference/textChanges/insertNodeAfter3-block with newline.js rename to tests/baselines/reference/textChanges/insertNodeAtConstructorStart-block with newline.js index 41bfb72b574c4..d1b7783769f80 100644 --- a/tests/baselines/reference/textChanges/insertNodeAfter3-block with newline.js +++ b/tests/baselines/reference/textChanges/insertNodeAtConstructorStart-block with newline.js @@ -11,6 +11,5 @@ class A { class A { constructor() { super(); - } } diff --git a/tests/baselines/reference/textChanges/insertNodeAfter3.js b/tests/baselines/reference/textChanges/insertNodeAtConstructorStart.js similarity index 100% rename from tests/baselines/reference/textChanges/insertNodeAfter3.js rename to tests/baselines/reference/textChanges/insertNodeAtConstructorStart.js diff --git a/tests/baselines/reference/textChanges/insertNodeAt1.js b/tests/baselines/reference/textChanges/insertNodeBefore3.js similarity index 100% rename from tests/baselines/reference/textChanges/insertNodeAt1.js rename to tests/baselines/reference/textChanges/insertNodeBefore3.js index 9c7ac46f8a8f3..3fa5eb5d66fc8 100644 --- a/tests/baselines/reference/textChanges/insertNodeAt1.js +++ b/tests/baselines/reference/textChanges/insertNodeBefore3.js @@ -11,11 +11,11 @@ var a = 4; // comment 7 // comment 1 var x = 1; // comment 2 -// comment 3 public class class1 implements interface1 { property1: boolean; } +// comment 3 var y; // comment 4 var z = 3; // comment 5 // comment 6 diff --git a/tests/cases/fourslash/codeFixAddMissingMember4.ts b/tests/cases/fourslash/codeFixAddMissingMember4.ts index d5525dc5c3b89..97a17959d269b 100644 --- a/tests/cases/fourslash/codeFixAddMissingMember4.ts +++ b/tests/cases/fourslash/codeFixAddMissingMember4.ts @@ -17,7 +17,7 @@ verify.codeFix({ index: 0, // TODO: GH#18445 newFileContent: `class C { - constructor() { + constructor() {\r this.foo = undefined;\r } method() { diff --git a/tests/cases/fourslash/codeFixAddMissingMember6.ts b/tests/cases/fourslash/codeFixAddMissingMember6.ts index 7eb360ab17039..a04a14c9ce5cd 100644 --- a/tests/cases/fourslash/codeFixAddMissingMember6.ts +++ b/tests/cases/fourslash/codeFixAddMissingMember6.ts @@ -15,7 +15,7 @@ verify.codeFix({ index: 0, // TODO: GH#18445 newFileContent: `class C { - constructor() { + constructor() {\r this.foo = undefined;\r } prop = ()=>{ this.foo === 10 }; diff --git a/tests/cases/fourslash/codeFixAddMissingMember_all_js.ts b/tests/cases/fourslash/codeFixAddMissingMember_all_js.ts index 9c8eb2377e372..1bc0d03c2117c 100644 --- a/tests/cases/fourslash/codeFixAddMissingMember_all_js.ts +++ b/tests/cases/fourslash/codeFixAddMissingMember_all_js.ts @@ -16,13 +16,14 @@ verify.codeFixAll({ fixId: "addMissingMember", newFileContent: - // TODO: GH#18445 GH#20073 + // TODO: GH#18445 `class C { y() {\r throw new Error("Method not implemented.");\r }\r - constructor() {this.x = undefined;\r -} + constructor() {\r + this.x = undefined;\r + } method() { this.x; this.y(); diff --git a/tests/cases/fourslash/extract-method-uniqueName.ts b/tests/cases/fourslash/extract-method-uniqueName.ts index a02b2c5ec9646..ae62f4ba7e7ac 100644 --- a/tests/cases/fourslash/extract-method-uniqueName.ts +++ b/tests/cases/fourslash/extract-method-uniqueName.ts @@ -3,8 +3,6 @@ ////// newFunction /////*start*/1 + 1/*end*/; -// NOTE: '// newFunction' should be included, but due to incorrect handling of trivia, -// it's omitted right now. goTo.select('start', 'end') edit.applyRefactor({ refactorName: "Extract Symbol", diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAmbient2.ts b/tests/cases/fourslash/importNameCodeFixNewImportAmbient2.ts index 64a8c28b3f8f6..1e3cd83e793b3 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportAmbient2.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportAmbient2.ts @@ -15,6 +15,7 @@ verify.importFixAtPosition([ `/*! * I'm a license or something */ + import { f1 } from "ambient-module"; f1();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportFile1.ts b/tests/cases/fourslash/importNameCodeFixNewImportFile1.ts index b98345db9f604..a7c69213ee9bf 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportFile1.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportFile1.ts @@ -12,6 +12,7 @@ verify.importFixAtPosition([ `/// + import { f1 } from "./Module"; f1();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportFileAllComments.ts b/tests/cases/fourslash/importNameCodeFixNewImportFileAllComments.ts index 9dc914ba693c1..34cfac99d008e 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportFileAllComments.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportFileAllComments.ts @@ -24,6 +24,7 @@ verify.importFixAtPosition([ /// /// /// + import { f1 } from "./module"; /** diff --git a/tests/cases/fourslash/importNameCodeFixShebang.ts b/tests/cases/fourslash/importNameCodeFixShebang.ts index 1ef7be11625a4..f15c84402757a 100644 --- a/tests/cases/fourslash/importNameCodeFixShebang.ts +++ b/tests/cases/fourslash/importNameCodeFixShebang.ts @@ -12,6 +12,7 @@ goTo.file("/b.ts"); verify.importFixAtPosition([ `#!/usr/bin/env node + import { foo } from "./a"; foo`,