From b899d1015b51fec85acaa7318234ee1f4ad9d489 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Thu, 24 Aug 2017 11:40:25 -0700 Subject: [PATCH 1/7] extractMethod: Support renameLocation --- src/harness/fourslash.ts | 37 ++- src/harness/unittests/extractMethods.ts | 7 +- src/server/client.ts | 4 +- .../refactors/convertFunctionToEs6Class.ts | 4 +- src/services/refactors/extractMethod.ts | 235 ++++++++++-------- src/services/types.ts | 9 +- tests/cases/fourslash/extract-method1.ts | 8 +- tests/cases/fourslash/extract-method10.ts | 7 + tests/cases/fourslash/extract-method13.ts | 20 +- tests/cases/fourslash/extract-method14.ts | 8 +- tests/cases/fourslash/extract-method15.ts | 9 +- tests/cases/fourslash/extract-method18.ts | 9 +- tests/cases/fourslash/extract-method19.ts | 9 +- tests/cases/fourslash/extract-method2.ts | 8 +- tests/cases/fourslash/extract-method21.ts | 10 +- tests/cases/fourslash/extract-method24.ts | 9 +- tests/cases/fourslash/extract-method25.ts | 9 +- tests/cases/fourslash/extract-method5.ts | 8 +- tests/cases/fourslash/extract-method7.ts | 7 +- tests/cases/fourslash/fourslash.ts | 2 +- 20 files changed, 245 insertions(+), 174 deletions(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 3010fc5353345..05ba3354c3735 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2754,11 +2754,11 @@ namespace FourSlash { } } - private getSelection() { - return ({ + private getSelection(): ts.TextRange { + return { pos: this.currentCaretPosition, end: this.selectionEnd === -1 ? this.currentCaretPosition : this.selectionEnd - }); + }; } public verifyRefactorAvailable(negative: boolean, name: string, actionName?: string) { @@ -2799,7 +2799,7 @@ namespace FourSlash { } } - public applyRefactor({ refactorName, actionName, actionDescription }: FourSlashInterface.ApplyRefactorOptions) { + public applyRefactor({ refactorName, actionName, actionDescription, newContent: newContentWithRenameMarker }: FourSlashInterface.ApplyRefactorOptions) { const range = this.getSelection(); const refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, range); const refactor = refactors.find(r => r.name === refactorName); @@ -2819,6 +2819,34 @@ namespace FourSlash { for (const edit of editInfo.edits) { this.applyEdits(edit.fileName, edit.textChanges, /*isFormattingEdit*/ false); } + + const { renamePosition, newContent } = parseNewContent(); + + this.verifyCurrentFileContent(newContent); + + if (renamePosition === undefined) { + if (editInfo.renameLocation !== undefined) { + this.raiseError(`Did not expect a rename location, got ${editInfo.renameLocation}`); + } + } + else { + // TODO: test editInfo.renameFilename too + if (renamePosition !== editInfo.renameLocation) { + this.raiseError(`Expected rename position of ${renamePosition}, but got ${editInfo.renameLocation}`); + } + } + + function parseNewContent(): { renamePosition: number | undefined, newContent: string } { + const renamePosition = newContentWithRenameMarker.indexOf("/*RENAME*/"); + if (renamePosition === -1) { + return { renamePosition: undefined, newContent: newContentWithRenameMarker }; + } + else { + const newContent = newContentWithRenameMarker.slice(0, renamePosition) + newContentWithRenameMarker.slice(renamePosition + "/*RENAME*/".length); + return { renamePosition, newContent }; + } + } + } public verifyFileAfterApplyingRefactorAtMarker( @@ -4313,5 +4341,6 @@ namespace FourSlashInterface { refactorName: string; actionName: string; actionDescription: string; + newContent: string; } } diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts index d096cd3d375d9..9980184053f85 100644 --- a/src/harness/unittests/extractMethods.ts +++ b/src/harness/unittests/extractMethods.ts @@ -224,7 +224,7 @@ namespace ts { testExtractRange(` function f() { while (true) { - [#| + [#| if (x) { return; } |] @@ -234,7 +234,7 @@ namespace ts { testExtractRange(` function f() { while (true) { - [#| + [#| [$|if (x) { } return;|] @@ -580,7 +580,8 @@ namespace A { data.push(`==ORIGINAL==`); data.push(sourceFile.text); for (const r of results) { - const changes = refactor.extractMethod.getPossibleExtractions(result.targetRange, context, results.indexOf(r))[0].changes; + // TODO: GH#18048: Test rename location too + const changes = refactor.extractMethod.getPossibleExtractionAtIndex(result.targetRange, context, results.indexOf(r)).edits; data.push(`==SCOPE::${r.scopeDescription}==`); data.push(textChanges.applyChanges(sourceFile.text, changes[0].textChanges)); } diff --git a/src/server/client.ts b/src/server/client.ts index 4acd862a89a24..0ffac42dae4d4 100644 --- a/src/server/client.ts +++ b/src/server/client.ts @@ -586,9 +586,7 @@ namespace ts.server { const response = this.processResponse(request); if (!response.body) { - return { - edits: [] - }; + return { edits: [], renameFilename: undefined, renameLocation: undefined }; } const edits: FileTextChanges[] = this.convertCodeEditsToTextChanges(response.body.edits); diff --git a/src/services/refactors/convertFunctionToEs6Class.ts b/src/services/refactors/convertFunctionToEs6Class.ts index bf0e4e22658b7..f929f265dc572 100644 --- a/src/services/refactors/convertFunctionToEs6Class.ts +++ b/src/services/refactors/convertFunctionToEs6Class.ts @@ -97,7 +97,9 @@ namespace ts.refactor.convertFunctionToES6Class { } return { - edits: changeTracker.getChanges() + edits: changeTracker.getChanges(), + renameFilename: undefined, + renameLocation: undefined, }; function deleteNode(node: Node, inList = false) { diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 820212011a964..c267162f5924e 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -31,16 +31,16 @@ namespace ts.refactor.extractMethod { const usedNames: Map = createMap(); let i = 0; - for (const extr of extractions) { + for (const { scopeDescription, errors } of extractions) { // Skip these since we don't have a way to report errors yet - if (extr.errors && extr.errors.length) { + if (errors.length) { continue; } // Don't issue refactorings with duplicated names. // Scopes come back in "innermost first" order, so extractions will // preferentially go into nearer scopes - const description = formatStringFromArgs(Diagnostics.Extract_function_into_0.message, [extr.scopeDescription]); + const description = formatStringFromArgs(Diagnostics.Extract_function_into_0.message, [scopeDescription]); if (!usedNames.has(description)) { usedNames.set(description, true); actions.push({ @@ -75,10 +75,7 @@ namespace ts.refactor.extractMethod { const index = +parsedIndexMatch[1]; Debug.assert(isFinite(index), "Expected to parse a finite number from the scope index"); - const extractions = getPossibleExtractions(targetRange, context, index); - // Scope is no longer valid from when the user issued the refactor (??) - Debug.assert(extractions !== undefined, "The extraction went missing? How?"); - return ({ edits: extractions[0].changes }); + return getPossibleExtractionAtIndex(targetRange, context, index); } // Move these into diagnostic messages if they become user-facing @@ -102,7 +99,7 @@ namespace ts.refactor.extractMethod { export const CannotExtractAmbientBlock = createMessage("Cannot extract code from ambient contexts"); } - export enum RangeFacts { + enum RangeFacts { None = 0, HasReturn = 1 << 0, IsGenerator = 1 << 1, @@ -117,7 +114,7 @@ namespace ts.refactor.extractMethod { /** * Represents an expression or a list of statements that should be extracted with some extra information */ - export interface TargetRange { + interface TargetRange { readonly range: Expression | Statement[]; readonly facts: RangeFacts; /** @@ -130,7 +127,7 @@ namespace ts.refactor.extractMethod { /** * Result of 'getRangeToExtract' operation: contains either a range or a list of errors */ - export type RangeToExtract = { + type RangeToExtract = { readonly targetRange?: never; readonly errors: ReadonlyArray; } | { @@ -141,18 +138,7 @@ namespace ts.refactor.extractMethod { /* * Scopes that can store newly extracted method */ - export type Scope = FunctionLikeDeclaration | SourceFile | ModuleBlock | ClassLikeDeclaration; - - /** - * Result of 'extractRange' operation for a specific scope. - * Stores either a list of changes that should be applied to extract a range or a list of errors - */ - export interface ExtractResultForScope { - readonly scope: Scope; - readonly scopeDescription: string; - readonly changes?: FileTextChanges[]; - readonly errors?: Diagnostic[]; - } + type Scope = FunctionLikeDeclaration | SourceFile | ModuleBlock | ClassLikeDeclaration; /** * getRangeToExtract takes a span inside a text file and returns either an expression or an array @@ -160,6 +146,7 @@ namespace ts.refactor.extractMethod { * process may fail, in which case a set of errors is returned instead (these are currently * not shown to the user, but can be used by us diagnostically) */ + // exported only for tests export function getRangeToExtract(sourceFile: SourceFile, span: TextSpan): RangeToExtract { const length = span.length || 0; // Walk up starting from the the start position until we find a non-SourceFile node that subsumes the selected span. @@ -472,7 +459,7 @@ namespace ts.refactor.extractMethod { * you may be able to extract into a class method *or* local closure *or* namespace function, * depending on what's in the extracted body. */ - export function collectEnclosingScopes(range: TargetRange): Scope[] | undefined { + function collectEnclosingScopes(range: TargetRange): Scope[] | undefined { let current: Node = isReadonlyArray(range.range) ? firstOrUndefined(range.range) : range.range; if (range.facts & RangeFacts.UsesThis) { // if range uses this as keyword or as type inside the class then it can only be extracted to a method of the containing class @@ -508,12 +495,34 @@ namespace ts.refactor.extractMethod { return scopes; } + // exported only for tests + export function getPossibleExtractionAtIndex(targetRange: TargetRange, context: RefactorContext, requestedChangesIndex: number): RefactorEditInfo { + const { scopes, readsAndWrites: { target, usagesPerScope, errorsPerScope } } = getPossibleExtractionsWorker(targetRange, context); + Debug.assert(!errorsPerScope[requestedChangesIndex].length, "The extraction went missing? How?"); + context.cancellationToken.throwIfCancellationRequested(); + return extractFunctionInScope(target, scopes[requestedChangesIndex], usagesPerScope[requestedChangesIndex], targetRange, context); + } + + interface PossibleExtraction { + readonly scopeDescription: string; + readonly errors: ReadonlyArray; + } /** * Given a piece of text to extract ('targetRange'), computes a list of possible extractions. * Each returned ExtractResultForScope corresponds to a possible target scope and is either a set of changes * or an error explaining why we can't extract into that scope. */ - export function getPossibleExtractions(targetRange: TargetRange, context: RefactorContext, requestedChangesIndex: number = undefined): ReadonlyArray | undefined { + // exported only for tests + export function getPossibleExtractions(targetRange: TargetRange, context: RefactorContext): ReadonlyArray | undefined { + const { scopes, readsAndWrites: { errorsPerScope } } = getPossibleExtractionsWorker(targetRange, context); + // Need the inner type annotation to avoid https://github.com/Microsoft/TypeScript/issues/7547 + return scopes.map((scope, i): PossibleExtraction => { + const errors = errorsPerScope[i]; + return { scopeDescription: getDescriptionForScope(scope), errors }; + }); + } + + function getPossibleExtractionsWorker(targetRange: TargetRange, context: RefactorContext): { readonly scopes: Scope[], readonly readsAndWrites: ReadsAndWrites } { const { file: sourceFile } = context; if (targetRange === undefined) { @@ -526,34 +535,13 @@ namespace ts.refactor.extractMethod { } const enclosingTextRange = getEnclosingTextRange(targetRange, sourceFile); - const { target, usagesPerScope, errorsPerScope } = collectReadsAndWrites( + const readsAndWrites = collectReadsAndWrites( targetRange, scopes, enclosingTextRange, sourceFile, context.program.getTypeChecker()); - - context.cancellationToken.throwIfCancellationRequested(); - - if (requestedChangesIndex !== undefined) { - if (errorsPerScope[requestedChangesIndex].length) { - return undefined; - } - return [extractFunctionInScope(target, scopes[requestedChangesIndex], usagesPerScope[requestedChangesIndex], targetRange, context)]; - } - else { - return scopes.map((scope, i) => { - const errors = errorsPerScope[i]; - if (errors.length) { - return { - scope, - scopeDescription: getDescriptionForScope(scope), - errors - }; - } - return { scope, scopeDescription: getDescriptionForScope(scope) }; - }); - } + return { scopes, readsAndWrites }; } function getDescriptionForScope(scope: Scope) { @@ -607,12 +595,16 @@ namespace ts.refactor.extractMethod { return functionNameText; } - export function extractFunctionInScope( + /** + * Result of 'extractRange' operation for a specific scope. + * Stores either a list of changes that should be applied to extract a range or a list of errors + */ + function extractFunctionInScope( node: Statement | Expression | Block, scope: Scope, { usages: usagesInScope, substitutions }: ScopeUsages, range: TargetRange, - context: RefactorContext): ExtractResultForScope { + context: RefactorContext): RefactorEditInfo { const checker = context.program.getTypeChecker(); @@ -621,8 +613,7 @@ namespace ts.refactor.extractMethod { const functionNameText: string = getUniqueName(n => !file.identifiers.has(n)); const isJS = isInJavaScriptFile(scope); - const functionName = createIdentifier(functionNameText as string); - const functionReference = createIdentifier(functionNameText as string); + const functionName = createIdentifier(functionNameText); let returnType: TypeNode = undefined; const parameters: ParameterDeclaration[] = []; @@ -659,7 +650,7 @@ namespace ts.refactor.extractMethod { returnType = checker.typeToTypeNode(contextualType); } - const { body, returnValueProperty } = transformFunctionBody(node); + const { body, returnValueProperty } = transformFunctionBody(node, writes, substitutions, !!(range.facts & RangeFacts.HasReturn)); let newFunction: MethodDeclaration | FunctionDeclaration; if (isClassLike(scope)) { @@ -702,14 +693,15 @@ namespace ts.refactor.extractMethod { const newNodes: Node[] = []; // replace range with function call - let call: Expression = createCall( - isClassLike(scope) ? createPropertyAccess(range.facts & RangeFacts.InStaticRegion ? createIdentifier(scope.name.getText()) : createThis(), functionReference) : functionReference, - /*typeArguments*/ undefined, - callArguments); + let { calledLhsLength, called } = getCalledExpression(scope, range, functionNameText); + + let call: Expression = createCall(called, /*typeArguments*/ undefined, callArguments); if (range.facts & RangeFacts.IsGenerator) { call = createYield(createToken(SyntaxKind.AsteriskToken), call); + calledLhsLength += "yield* ".length; } if (range.facts & RangeFacts.IsAsyncFunction) { + calledLhsLength += "await ".length; call = createAwait(call); } @@ -768,71 +760,91 @@ namespace ts.refactor.extractMethod { changeTracker.replaceNodeWithNodes(context.file, range.range, newNodes, { nodeSeparator: context.newLineCharacter }); } + const edits = changeTracker.getChanges(); + const renameRange = isReadonlyArray(range.range) ? range.range[0] : range.range; + const returnOffset = range.facts & RangeFacts.HasReturn ? "return ".length : 0; return { - scope, - scopeDescription: getDescriptionForScope(scope), - changes: changeTracker.getChanges() + // TODO: GH#18048 + renameFilename: writes ? undefined : renameRange.getSourceFile().fileName, + renameLocation: writes ? undefined : renameRange.getStart() + calledLhsLength + returnOffset, + edits, }; + } - function getPropertyAssignmentsForWrites(writes: UsageEntry[]) { - return writes.map(w => createShorthandPropertyAssignment(w.symbol.name)); + function getCalledExpression(scope: Node, range: TargetRange, functionNameText: string): { called: Expression, calledLhsLength: number } { + const functionReference = createIdentifier(functionNameText); + if (isClassLike(scope)) { + let calledLhsLength: number, lhs: Expression; + if (range.facts & RangeFacts.InStaticRegion) { + const text = scope.name.text; + calledLhsLength = text.length + 1; // +1 for the "." + lhs = createIdentifier(text); + } + else { + calledLhsLength = "this.".length; + lhs = createThis(); + } + return { called: createPropertyAccess(lhs, functionReference), calledLhsLength }; } - - function generateReturnValueProperty() { - return "__return"; + else { + return { called: functionReference, calledLhsLength: 0 }; } + } - function transformFunctionBody(body: Node) { - if (isBlock(body) && !writes && substitutions.size === 0) { - // already block, no writes to propagate back, no substitutions - can use node as is - return { body: createBlock(body.statements, /*multLine*/ true), returnValueProperty: undefined }; - } - let returnValueProperty: string; - const statements = createNodeArray(isBlock(body) ? body.statements.slice(0) : [isStatement(body) ? body : createReturn(body)]); - // rewrite body if either there are writes that should be propagated back via return statements or there are substitutions - if (writes || substitutions.size) { - const rewrittenStatements = visitNodes(statements, visitor).slice(); - if (writes && !(range.facts & RangeFacts.HasReturn) && isStatement(body)) { - // add return at the end to propagate writes back in case if control flow falls out of the function body - // it is ok to know that range has at least one return since it we only allow unconditional returns - const assignments = getPropertyAssignmentsForWrites(writes); - if (assignments.length === 1) { - rewrittenStatements.push(createReturn(assignments[0].name)); - } - else { - rewrittenStatements.push(createReturn(createObjectLiteral(assignments))); - } + function transformFunctionBody(body: Node, writes: ReadonlyArray, substitutions: ReadonlyMap, hasReturn: boolean): { body: Block, returnValueProperty: string } { + if (isBlock(body) && !writes && substitutions.size === 0) { + // already block, no writes to propagate back, no substitutions - can use node as is + return { body: createBlock(body.statements, /*multLine*/ true), returnValueProperty: undefined }; + } + let returnValueProperty: string; + const statements = createNodeArray(isBlock(body) ? body.statements.slice(0) : [isStatement(body) ? body : createReturn(body)]); + // rewrite body if either there are writes that should be propagated back via return statements or there are substitutions + if (writes || substitutions.size) { + const rewrittenStatements = visitNodes(statements, visitor).slice(); + if (writes && !hasReturn && isStatement(body)) { + // add return at the end to propagate writes back in case if control flow falls out of the function body + // it is ok to know that range has at least one return since it we only allow unconditional returns + const assignments = getPropertyAssignmentsForWrites(writes); + if (assignments.length === 1) { + rewrittenStatements.push(createReturn(assignments[0].name)); + } + else { + rewrittenStatements.push(createReturn(createObjectLiteral(assignments))); } - return { body: createBlock(rewrittenStatements, /*multiLine*/ true), returnValueProperty }; - } - else { - return { body: createBlock(statements, /*multiLine*/ true), returnValueProperty: undefined }; } + return { body: createBlock(rewrittenStatements, /*multiLine*/ true), returnValueProperty }; + } + else { + return { body: createBlock(statements, /*multiLine*/ true), returnValueProperty: undefined }; + } - function visitor(node: Node): VisitResult { - if (node.kind === SyntaxKind.ReturnStatement && writes) { - const assignments: ObjectLiteralElementLike[] = getPropertyAssignmentsForWrites(writes); - if ((node).expression) { - if (!returnValueProperty) { - returnValueProperty = generateReturnValueProperty(); - } - assignments.unshift(createPropertyAssignment(returnValueProperty, visitNode((node).expression, visitor))); - } - if (assignments.length === 1) { - return createReturn(assignments[0].name as Expression); - } - else { - return createReturn(createObjectLiteral(assignments)); + function visitor(node: Node): VisitResult { + if (node.kind === SyntaxKind.ReturnStatement && writes) { + const assignments: ObjectLiteralElementLike[] = getPropertyAssignmentsForWrites(writes); + if ((node).expression) { + if (!returnValueProperty) { + returnValueProperty = "__return"; } + assignments.unshift(createPropertyAssignment(returnValueProperty, visitNode((node).expression, visitor))); + } + if (assignments.length === 1) { + return createReturn(assignments[0].name as Expression); } else { - const substitution = substitutions.get(getNodeId(node).toString()); - return substitution || visitEachChild(node, visitor, nullTransformationContext); + return createReturn(createObjectLiteral(assignments)); } } + else { + const substitution = substitutions.get(getNodeId(node).toString()); + return substitution || visitEachChild(node, visitor, nullTransformationContext); + } } } + function getPropertyAssignmentsForWrites(writes: ReadonlyArray): ShorthandPropertyAssignment[] { + return writes.map(w => createShorthandPropertyAssignment(w.symbol.name)); + } + function isReadonlyArray(v: any): v is ReadonlyArray { return isArray(v); } @@ -859,23 +871,28 @@ namespace ts.refactor.extractMethod { Write = 2 } - export interface UsageEntry { + interface UsageEntry { readonly usage: Usage; readonly symbol: Symbol; readonly node: Node; } - export interface ScopeUsages { - usages: Map; - substitutions: Map; + interface ScopeUsages { + readonly usages: Map; + readonly substitutions: Map; } + interface ReadsAndWrites { + readonly target: Expression | Block; + readonly usagesPerScope: ReadonlyArray; + readonly errorsPerScope: ReadonlyArray; + } function collectReadsAndWrites( targetRange: TargetRange, scopes: Scope[], enclosingTextRange: TextRange, sourceFile: SourceFile, - checker: TypeChecker) { + checker: TypeChecker): ReadsAndWrites { const usagesPerScope: ScopeUsages[] = []; const substitutionsPerScope: Map[] = []; diff --git a/src/services/types.ts b/src/services/types.ts index 609eaf11de507..f5cec754a6318 100644 --- a/src/services/types.ts +++ b/src/services/types.ts @@ -412,12 +412,11 @@ namespace ts { * A set of edits to make in response to a refactor action, plus an optional * location where renaming should be invoked from */ - export type RefactorEditInfo = { + export interface RefactorEditInfo { edits: FileTextChanges[]; - renameFilename?: string; - renameLocation?: number; - }; - + renameFilename: string | undefined; + renameLocation: number | undefined; + } export interface TextInsertion { newText: string; diff --git a/tests/cases/fourslash/extract-method1.ts b/tests/cases/fourslash/extract-method1.ts index ff061295c8c6b..faf98c9c0ea30 100644 --- a/tests/cases/fourslash/extract-method1.ts +++ b/tests/cases/fourslash/extract-method1.ts @@ -17,11 +17,10 @@ edit.applyRefactor({ refactorName: "Extract Method", actionName: "scope_0", actionDescription: "Extract function into class 'Foo'", -}); -verify.currentFileContentIs( + newContent: `class Foo { someMethod(m: number) { - this.newFunction(m); + this./*RENAME*/newFunction(m); var q = 10; return q; } @@ -33,4 +32,5 @@ verify.currentFileContentIs( var z = y + x; console.log(z); } -}`); +}` +}); diff --git a/tests/cases/fourslash/extract-method10.ts b/tests/cases/fourslash/extract-method10.ts index ffbee7350e259..3094b86ac6e7e 100644 --- a/tests/cases/fourslash/extract-method10.ts +++ b/tests/cases/fourslash/extract-method10.ts @@ -8,4 +8,11 @@ edit.applyRefactor({ refactorName: "Extract Method", actionName: 'scope_0', actionDescription: "Extract function into module scope", + newContent: +`export {}; // Make this a module +(x => x)(/*RENAME*/newFunction())(1); +function newFunction(): (x: any) => any { + return x => x; +} +` }); diff --git a/tests/cases/fourslash/extract-method13.ts b/tests/cases/fourslash/extract-method13.ts index 14a146a80c509..26759d012df39 100644 --- a/tests/cases/fourslash/extract-method13.ts +++ b/tests/cases/fourslash/extract-method13.ts @@ -14,6 +14,16 @@ edit.applyRefactor({ refactorName: "Extract Method", actionName: "scope_0", actionDescription: "Extract function into class 'C'", + newContent: +`class C { + static j = 100; + constructor(q: string = C./*RENAME*/newFunction()) { + } + + private static newFunction(): string { + return "hello"; + } +}` }); goTo.select('c', 'd'); @@ -21,10 +31,9 @@ edit.applyRefactor({ refactorName: "Extract Method", actionName: "scope_0", actionDescription: "Extract function into class 'C'", -}); - -verify.currentFileContentIs(`class C { - static j = C.newFunction_1(); + newContent: +`class C { + static j = C./*RENAME*/newFunction_1(); constructor(q: string = C.newFunction()) { } @@ -35,4 +44,5 @@ verify.currentFileContentIs(`class C { private static newFunction_1() { return 100; } -}`); \ No newline at end of file +}` +}); diff --git a/tests/cases/fourslash/extract-method14.ts b/tests/cases/fourslash/extract-method14.ts index 696bb664bd358..8892d8974a71a 100644 --- a/tests/cases/fourslash/extract-method14.ts +++ b/tests/cases/fourslash/extract-method14.ts @@ -15,8 +15,9 @@ edit.applyRefactor({ refactorName: "Extract Method", actionName: "scope_1", actionDescription: "Extract function into global scope", -}); -verify.currentFileContentIs(`function foo() { + newContent: +// TODO: GH#18048 +`function foo() { var i = 10; var __return: any; ({ __return, i } = newFunction(i)); @@ -25,4 +26,5 @@ verify.currentFileContentIs(`function foo() { function newFunction(i) { return { __return: i++, i }; } -`); \ No newline at end of file +` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/extract-method15.ts b/tests/cases/fourslash/extract-method15.ts index 93aa357cfee2e..3602d90f40094 100644 --- a/tests/cases/fourslash/extract-method15.ts +++ b/tests/cases/fourslash/extract-method15.ts @@ -13,9 +13,9 @@ edit.applyRefactor({ refactorName: "Extract Method", actionName: "scope_1", actionDescription: "Extract function into global scope", -}); - -verify.currentFileContentIs(`function foo() { + newContent: +// TODO: GH#18048 +`function foo() { var i = 10; i = newFunction(i); } @@ -23,4 +23,5 @@ function newFunction(i: number) { i++; return i; } -`); +` +}); diff --git a/tests/cases/fourslash/extract-method18.ts b/tests/cases/fourslash/extract-method18.ts index d99d14bac73f6..4a76f3247a9b4 100644 --- a/tests/cases/fourslash/extract-method18.ts +++ b/tests/cases/fourslash/extract-method18.ts @@ -13,12 +13,13 @@ edit.applyRefactor({ refactorName: "Extract Method", actionName: "scope_1", actionDescription: "Extract function into global scope", -}); -verify.currentFileContentIs(`function fn() { + newContent: +`function fn() { const x = { m: 1 }; - newFunction(x); + /*RENAME*/newFunction(x); } function newFunction(x: { m: number; }) { x.m = 3; } -`); +` +}); diff --git a/tests/cases/fourslash/extract-method19.ts b/tests/cases/fourslash/extract-method19.ts index e4fb3e6e1110e..1904332f05939 100644 --- a/tests/cases/fourslash/extract-method19.ts +++ b/tests/cases/fourslash/extract-method19.ts @@ -13,13 +13,14 @@ edit.applyRefactor({ refactorName: "Extract Method", actionName: "scope_0", actionDescription: "Extract function into function 'fn'", -}); -verify.currentFileContentIs(`function fn() { - newFunction_1(); + newContent: +`function fn() { + /*RENAME*/newFunction_1(); function newFunction_1() { console.log("hi"); } } -function newFunction() { }`); +function newFunction() { }` +}); diff --git a/tests/cases/fourslash/extract-method2.ts b/tests/cases/fourslash/extract-method2.ts index 508836c419922..0839f6ad426ba 100644 --- a/tests/cases/fourslash/extract-method2.ts +++ b/tests/cases/fourslash/extract-method2.ts @@ -14,18 +14,18 @@ edit.applyRefactor({ refactorName: "Extract Method", actionName: "scope_2", actionDescription: "Extract function into global scope", -}); -verify.currentFileContentIs( + newContent: `namespace NS { class Q { foo() { console.log('100'); const m = 10, j = "hello", k = {x: "what"}; - const q = newFunction(m, j, k); + const q = /*RENAME*/newFunction(m, j, k); } } } function newFunction(m: number, j: string, k: { x: string; }) { return m + j + k; } -`); +` +}); diff --git a/tests/cases/fourslash/extract-method21.ts b/tests/cases/fourslash/extract-method21.ts index c32df5b59798e..f392e6876d16f 100644 --- a/tests/cases/fourslash/extract-method21.ts +++ b/tests/cases/fourslash/extract-method21.ts @@ -16,14 +16,14 @@ edit.applyRefactor({ refactorName: "Extract Method", actionName: "scope_0", actionDescription: "Extract function into class 'Foo'", -}); - -verify.currentFileContentIs(`class Foo { + newContent: +`class Foo { static method() { - return Foo.newFunction(); + return Foo./*RENAME*/newFunction(); } private static newFunction() { return 1; } -}`); \ No newline at end of file +}` +}); diff --git a/tests/cases/fourslash/extract-method24.ts b/tests/cases/fourslash/extract-method24.ts index 615cb2ac4d2f8..3d14126b4d1e4 100644 --- a/tests/cases/fourslash/extract-method24.ts +++ b/tests/cases/fourslash/extract-method24.ts @@ -11,13 +11,14 @@ edit.applyRefactor({ refactorName: "Extract Method", actionName: "scope_1", actionDescription: "Extract function into global scope", -}); -verify.currentFileContentIs(`function M() { + newContent: +`function M() { let a = [1,2,3]; let x = 0; - console.log(newFunction(a, x)); + console.log(/*RENAME*/newFunction(a, x)); } function newFunction(a: number[], x: number): any { return a[x]; } -`); \ No newline at end of file +` +}); diff --git a/tests/cases/fourslash/extract-method25.ts b/tests/cases/fourslash/extract-method25.ts index 8585dd06fd4ea..c88b05a778706 100644 --- a/tests/cases/fourslash/extract-method25.ts +++ b/tests/cases/fourslash/extract-method25.ts @@ -12,12 +12,13 @@ edit.applyRefactor({ refactorName: "Extract Method", actionName: "scope_0", actionDescription: "Extract function into function 'fn'", -}); -verify.currentFileContentIs(`function fn() { - var q = newFunction() + newContent: +`function fn() { + var q = /*RENAME*/newFunction() q[0]++ function newFunction() { return [0]; } -}`); +}` +}); diff --git a/tests/cases/fourslash/extract-method5.ts b/tests/cases/fourslash/extract-method5.ts index 10294298b085b..99c85410702e7 100644 --- a/tests/cases/fourslash/extract-method5.ts +++ b/tests/cases/fourslash/extract-method5.ts @@ -13,12 +13,12 @@ edit.applyRefactor({ refactorName: "Extract Method", actionName: "scope_0", actionDescription: "Extract function into function 'f'", -}); -verify.currentFileContentIs( + newContent: `function f() { - var x: 1 | 2 | 3 = newFunction(); + var x: 1 | 2 | 3 = /*RENAME*/newFunction(); function newFunction(): 1 | 2 | 3 { return 2; } -}`); \ No newline at end of file +}` +}); diff --git a/tests/cases/fourslash/extract-method7.ts b/tests/cases/fourslash/extract-method7.ts index 95c9cbe9897ec..8ef8eb38d3fb8 100644 --- a/tests/cases/fourslash/extract-method7.ts +++ b/tests/cases/fourslash/extract-method7.ts @@ -11,10 +11,11 @@ edit.applyRefactor({ refactorName: "Extract Method", actionName: "scope_0", actionDescription: "Extract function into global scope", -}); -verify.currentFileContentIs(`function fn(x = newFunction()) { + newContent: +`function fn(x = /*RENAME*/newFunction()) { } function newFunction() { return 3; } -`); +` +}); diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index 652ed4812c04a..92bdac4416175 100644 --- a/tests/cases/fourslash/fourslash.ts +++ b/tests/cases/fourslash/fourslash.ts @@ -310,7 +310,7 @@ declare namespace FourSlashInterface { enableFormatting(): void; disableFormatting(): void; - applyRefactor(options: { refactorName: string, actionName: string, actionDescription: string }): void; + applyRefactor(options: { refactorName: string, actionName: string, actionDescription: string, newContent: string }): void; } class debug { printCurrentParameterHelp(): void; From 36d59aff5181b75ab23c46df6f4d4922726ad5f8 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Fri, 25 Aug 2017 14:45:45 -0700 Subject: [PATCH 2/7] Add tslint disable --- src/services/refactors/extractMethod.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index c267162f5924e..615e920c10c80 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -693,7 +693,7 @@ namespace ts.refactor.extractMethod { const newNodes: Node[] = []; // replace range with function call - let { calledLhsLength, called } = getCalledExpression(scope, range, functionNameText); + let { calledLhsLength, called } = getCalledExpression(scope, range, functionNameText); // tslint:disable-line prefer-const let call: Expression = createCall(called, /*typeArguments*/ undefined, callArguments); if (range.facts & RangeFacts.IsGenerator) { From 0aa0ebfe1670b7a016153038df0a95746dc17daf Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Tue, 5 Sep 2017 11:55:06 -0700 Subject: [PATCH 3/7] Properly analyze list of changes to always get a correct rename location --- src/harness/unittests/extractMethods.ts | 8 +-- .../refactors/convertFunctionToEs6Class.ts | 2 +- src/services/refactors/extractMethod.ts | 54 +++++++++---------- .../reference/extractMethod/extractMethod1.ts | 8 +-- .../extractMethod/extractMethod10.ts | 6 +-- .../extractMethod/extractMethod11.ts | 6 +-- .../extractMethod/extractMethod12.ts | 2 +- .../extractMethod/extractMethod13.ts | 6 +-- .../extractMethod/extractMethod14.ts | 6 +-- .../extractMethod/extractMethod15.ts | 6 +-- .../extractMethod/extractMethod16.ts | 4 +- .../extractMethod/extractMethod17.ts | 4 +- .../extractMethod/extractMethod18.ts | 4 +- .../extractMethod/extractMethod19.ts | 4 +- .../reference/extractMethod/extractMethod2.ts | 8 +-- .../extractMethod/extractMethod20.ts | 4 +- .../reference/extractMethod/extractMethod3.ts | 8 +-- .../reference/extractMethod/extractMethod4.ts | 8 +-- .../reference/extractMethod/extractMethod5.ts | 8 +-- .../reference/extractMethod/extractMethod6.ts | 8 +-- .../reference/extractMethod/extractMethod7.ts | 8 +-- .../reference/extractMethod/extractMethod8.ts | 8 +-- .../reference/extractMethod/extractMethod9.ts | 8 +-- tests/cases/fourslash/extract-method14.ts | 3 +- tests/cases/fourslash/extract-method15.ts | 3 +- 25 files changed, 97 insertions(+), 97 deletions(-) diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts index 56915277ad848..22c8d6c03a77c 100644 --- a/src/harness/unittests/extractMethods.ts +++ b/src/harness/unittests/extractMethods.ts @@ -649,10 +649,12 @@ namespace A { data.push(`// ==ORIGINAL==`); data.push(sourceFile.text); for (const r of results) { - // TODO: GH#18048: Test rename location too - const changes = refactor.extractMethod.getPossibleExtractionAtIndex(result.targetRange, context, results.indexOf(r)).edits; + const { renameLocation, edits } = refactor.extractMethod.getPossibleExtractionAtIndex(result.targetRange, context, results.indexOf(r)); + Debug.assert(edits.length === 1); data.push(`// ==SCOPE::${r.scopeDescription}==`); - data.push(textChanges.applyChanges(sourceFile.text, changes[0].textChanges)); + const newText = textChanges.applyChanges(sourceFile.text, edits[0].textChanges); + const newTextWithRename = newText.slice(0, renameLocation) + "/*RENAME*/" + newText.slice(renameLocation); + data.push(newTextWithRename); } return data.join(newLineCharacter); }); diff --git a/src/services/refactors/convertFunctionToEs6Class.ts b/src/services/refactors/convertFunctionToEs6Class.ts index f929f265dc572..b294062176a39 100644 --- a/src/services/refactors/convertFunctionToEs6Class.ts +++ b/src/services/refactors/convertFunctionToEs6Class.ts @@ -63,7 +63,7 @@ namespace ts.refactor.convertFunctionToES6Class { } const ctorDeclaration = ctorSymbol.valueDeclaration; - const changeTracker = textChanges.ChangeTracker.fromCodeFixContext(context as { newLineCharacter: string, rulesProvider: formatting.RulesProvider }); + const changeTracker = textChanges.ChangeTracker.fromCodeFixContext(context); let precedingNode: Node; let newClassDeclaration: ClassDeclaration; diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 2c30ec489df32..6c1f6d749fb71 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -516,10 +516,8 @@ namespace ts.refactor.extractMethod { export function getPossibleExtractions(targetRange: TargetRange, context: RefactorContext): ReadonlyArray | undefined { const { scopes, readsAndWrites: { errorsPerScope } } = getPossibleExtractionsWorker(targetRange, context); // Need the inner type annotation to avoid https://github.com/Microsoft/TypeScript/issues/7547 - return scopes.map((scope, i): PossibleExtraction => { - const errors = errorsPerScope[i]; - return { scopeDescription: getDescriptionForScope(scope), errors }; - }); + return scopes.map((scope, i): PossibleExtraction => + ({ scopeDescription: getDescriptionForScope(scope), errors: errorsPerScope[i] })); } function getPossibleExtractionsWorker(targetRange: TargetRange, context: RefactorContext): { readonly scopes: Scope[], readonly readsAndWrites: ReadsAndWrites } { @@ -705,17 +703,15 @@ namespace ts.refactor.extractMethod { const newNodes: Node[] = []; // replace range with function call - let { calledLhsLength, called } = getCalledExpression(scope, range, functionNameText); // tslint:disable-line prefer-const + let called = getCalledExpression(scope, range, functionNameText); // tslint:disable-line prefer-const let call: Expression = createCall(called, callTypeArguments, // Note that no attempt is made to take advantage of type argument inference callArguments); if (range.facts & RangeFacts.IsGenerator) { call = createYield(createToken(SyntaxKind.AsteriskToken), call); - calledLhsLength += "yield* ".length; } if (range.facts & RangeFacts.IsAsyncFunction) { - calledLhsLength += "await ".length; call = createAwait(call); } @@ -776,13 +772,26 @@ namespace ts.refactor.extractMethod { const edits = changeTracker.getChanges(); const renameRange = isReadonlyArray(range.range) ? range.range[0] : range.range; - const returnOffset = range.facts & RangeFacts.HasReturn ? "return ".length : 0; - return { - // TODO: GH#18048 - renameFilename: writes ? undefined : renameRange.getSourceFile().fileName, - renameLocation: writes ? undefined : renameRange.getStart() + calledLhsLength + returnOffset, - edits, - }; + + const renameFilename = renameRange.getSourceFile().fileName; + const renameLocation = getRenameLocation(edits, renameFilename, functionNameText); + return { renameFilename, renameLocation, edits }; + } + + function getRenameLocation(edits: ReadonlyArray, renameFilename: string, functionNameText: string): number { + let delta = 0; + for (const { fileName, textChanges } of edits) { + Debug.assert(fileName === renameFilename); + for (const change of textChanges) { + const { span, newText } = change; + const index = newText.indexOf(functionNameText); + if (index !== -1) { + return span.start + delta + index; + } + delta += newText.length - span.length; + } + } + throw new Error(); // Didn't find the text we inserted? } function getFirstDeclaration(type: Type): Declaration | undefined { @@ -830,23 +839,14 @@ namespace ts.refactor.extractMethod { return type1.id - type2.id; } - function getCalledExpression(scope: Node, range: TargetRange, functionNameText: string): { called: Expression, calledLhsLength: number } { + function getCalledExpression(scope: Node, range: TargetRange, functionNameText: string): Expression { const functionReference = createIdentifier(functionNameText); if (isClassLike(scope)) { - let calledLhsLength: number, lhs: Expression; - if (range.facts & RangeFacts.InStaticRegion) { - const text = scope.name.text; - calledLhsLength = text.length + 1; // +1 for the "." - lhs = createIdentifier(text); - } - else { - calledLhsLength = "this.".length; - lhs = createThis(); - } - return { called: createPropertyAccess(lhs, functionReference), calledLhsLength }; + const lhs = range.facts & RangeFacts.InStaticRegion ? createIdentifier(scope.name.text) : createThis(); + return createPropertyAccess(lhs, functionReference); } else { - return { called: functionReference, calledLhsLength: 0 }; + return functionReference; } } diff --git a/tests/baselines/reference/extractMethod/extractMethod1.ts b/tests/baselines/reference/extractMethod/extractMethod1.ts index bfe80cb6c9bf6..9ae0f78f2057e 100644 --- a/tests/baselines/reference/extractMethod/extractMethod1.ts +++ b/tests/baselines/reference/extractMethod/extractMethod1.ts @@ -23,7 +23,7 @@ namespace A { function a() { let a = 1; - newFunction(); + /*RENAME*/newFunction(); function newFunction() { let y = 5; @@ -43,7 +43,7 @@ namespace A { function a() { let a = 1; - a = newFunction(a); + a = /*RENAME*/newFunction(a); } function newFunction(a: number) { @@ -64,7 +64,7 @@ namespace A { function a() { let a = 1; - a = newFunction(a); + a = /*RENAME*/newFunction(a); } } @@ -85,7 +85,7 @@ namespace A { function a() { let a = 1; - a = newFunction(x, a, foo); + a = /*RENAME*/newFunction(x, a, foo); } } } diff --git a/tests/baselines/reference/extractMethod/extractMethod10.ts b/tests/baselines/reference/extractMethod/extractMethod10.ts index 97bd5cbd5036b..c218e2e964acb 100644 --- a/tests/baselines/reference/extractMethod/extractMethod10.ts +++ b/tests/baselines/reference/extractMethod/extractMethod10.ts @@ -15,7 +15,7 @@ namespace A { class C { a() { let z = 1; - return this.newFunction(); + return this./*RENAME*/newFunction(); } private newFunction() { @@ -30,7 +30,7 @@ namespace A { class C { a() { let z = 1; - return newFunction(); + return /*RENAME*/newFunction(); } } @@ -45,7 +45,7 @@ namespace A { class C { a() { let z = 1; - return newFunction(); + return /*RENAME*/newFunction(); } } } diff --git a/tests/baselines/reference/extractMethod/extractMethod11.ts b/tests/baselines/reference/extractMethod/extractMethod11.ts index 4999df4a70642..47ed083f911fc 100644 --- a/tests/baselines/reference/extractMethod/extractMethod11.ts +++ b/tests/baselines/reference/extractMethod/extractMethod11.ts @@ -18,7 +18,7 @@ namespace A { a() { let z = 1; var __return: any; - ({ __return, z } = this.newFunction(z)); + ({ __return, z } = this./*RENAME*/newFunction(z)); return __return; } @@ -37,7 +37,7 @@ namespace A { a() { let z = 1; var __return: any; - ({ __return, z } = newFunction(z)); + ({ __return, z } = /*RENAME*/newFunction(z)); return __return; } } @@ -56,7 +56,7 @@ namespace A { a() { let z = 1; var __return: any; - ({ __return, y, z } = newFunction(y, z)); + ({ __return, y, z } = /*RENAME*/newFunction(y, z)); return __return; } } diff --git a/tests/baselines/reference/extractMethod/extractMethod12.ts b/tests/baselines/reference/extractMethod/extractMethod12.ts index 2b95a545ce1e2..83ff5afb80705 100644 --- a/tests/baselines/reference/extractMethod/extractMethod12.ts +++ b/tests/baselines/reference/extractMethod/extractMethod12.ts @@ -21,7 +21,7 @@ namespace A { a() { let z = 1; var __return: any; - ({ __return, z } = this.newFunction(z)); + ({ __return, z } = this./*RENAME*/newFunction(z)); return __return; } diff --git a/tests/baselines/reference/extractMethod/extractMethod13.ts b/tests/baselines/reference/extractMethod/extractMethod13.ts index 8f76dea88aa0d..d15a9e6f7fcbc 100644 --- a/tests/baselines/reference/extractMethod/extractMethod13.ts +++ b/tests/baselines/reference/extractMethod/extractMethod13.ts @@ -20,7 +20,7 @@ (u2a: U2a, u2b: U2b) => { function F2(t2a: T2a, t2b: T2b) { (u3a: U3a, u3b: U3b) => { - newFunction(u3a); + /*RENAME*/newFunction(u3a); } function newFunction(u3a: U3a) { @@ -40,7 +40,7 @@ (u2a: U2a, u2b: U2b) => { function F2(t2a: T2a, t2b: T2b) { (u3a: U3a, u3b: U3b) => { - newFunction(t2a, u2a, u3a); + /*RENAME*/newFunction(t2a, u2a, u3a); } } } @@ -60,7 +60,7 @@ (u2a: U2a, u2b: U2b) => { function F2(t2a: T2a, t2b: T2b) { (u3a: U3a, u3b: U3b) => { - newFunction(t1a, t2a, u1a, u2a, u3a); + /*RENAME*/newFunction(t1a, t2a, u1a, u2a, u3a); } } } diff --git a/tests/baselines/reference/extractMethod/extractMethod14.ts b/tests/baselines/reference/extractMethod/extractMethod14.ts index 38e4c380ebc37..4539bbda10f68 100644 --- a/tests/baselines/reference/extractMethod/extractMethod14.ts +++ b/tests/baselines/reference/extractMethod/extractMethod14.ts @@ -8,7 +8,7 @@ function F(t1: T) { // ==SCOPE::function 'F'== function F(t1: T) { function F(t2: T) { - newFunction(); + /*RENAME*/newFunction(); function newFunction() { t1.toString(); @@ -19,7 +19,7 @@ function F(t1: T) { // ==SCOPE::function 'F'== function F(t1: T) { function F(t2: T) { - newFunction(t2); + /*RENAME*/newFunction(t2); } function newFunction(t2: T) { @@ -30,7 +30,7 @@ function F(t1: T) { // ==SCOPE::global scope== function F(t1: T) { function F(t2: T) { - newFunction(t1, t2); + /*RENAME*/newFunction(t1, t2); } } function newFunction(t1: T, t2: T) { diff --git a/tests/baselines/reference/extractMethod/extractMethod15.ts b/tests/baselines/reference/extractMethod/extractMethod15.ts index ba5b9b3ad0143..b41b45f626cc7 100644 --- a/tests/baselines/reference/extractMethod/extractMethod15.ts +++ b/tests/baselines/reference/extractMethod/extractMethod15.ts @@ -7,7 +7,7 @@ function F(t1: T) { // ==SCOPE::function 'F'== function F(t1: T) { function F(t2: U) { - newFunction(); + /*RENAME*/newFunction(); function newFunction() { t2.toString(); @@ -17,7 +17,7 @@ function F(t1: T) { // ==SCOPE::function 'F'== function F(t1: T) { function F(t2: U) { - newFunction(t2); + /*RENAME*/newFunction(t2); } function newFunction(t2: U) { @@ -27,7 +27,7 @@ function F(t1: T) { // ==SCOPE::global scope== function F(t1: T) { function F(t2: U) { - newFunction(t2); + /*RENAME*/newFunction(t2); } } function newFunction(t2: U) { diff --git a/tests/baselines/reference/extractMethod/extractMethod16.ts b/tests/baselines/reference/extractMethod/extractMethod16.ts index 1ce88b9314528..0f6a783be0f2d 100644 --- a/tests/baselines/reference/extractMethod/extractMethod16.ts +++ b/tests/baselines/reference/extractMethod/extractMethod16.ts @@ -4,7 +4,7 @@ function F() { } // ==SCOPE::function 'F'== function F() { - const array: T[] = newFunction(); + const array: T[] = /*RENAME*/newFunction(); function newFunction(): T[] { return []; @@ -12,7 +12,7 @@ function F() { } // ==SCOPE::global scope== function F() { - const array: T[] = newFunction(); + const array: T[] = /*RENAME*/newFunction(); } function newFunction(): T[] { return []; diff --git a/tests/baselines/reference/extractMethod/extractMethod17.ts b/tests/baselines/reference/extractMethod/extractMethod17.ts index d800c79f3bdfc..b0279030d2a72 100644 --- a/tests/baselines/reference/extractMethod/extractMethod17.ts +++ b/tests/baselines/reference/extractMethod/extractMethod17.ts @@ -7,7 +7,7 @@ class C { // ==SCOPE::class 'C'== class C { M(t1: T1, t2: T2) { - this.newFunction(t1); + this./*RENAME*/newFunction(t1); } private newFunction(t1: T1) { @@ -17,7 +17,7 @@ class C { // ==SCOPE::global scope== class C { M(t1: T1, t2: T2) { - newFunction(t1); + /*RENAME*/newFunction(t1); } } function newFunction(t1: T1) { diff --git a/tests/baselines/reference/extractMethod/extractMethod18.ts b/tests/baselines/reference/extractMethod/extractMethod18.ts index ce6a90c3790c8..2a03fe5875c99 100644 --- a/tests/baselines/reference/extractMethod/extractMethod18.ts +++ b/tests/baselines/reference/extractMethod/extractMethod18.ts @@ -7,7 +7,7 @@ class C { // ==SCOPE::class 'C'== class C { M(t1: T1, t2: T2) { - this.newFunction(t1); + this./*RENAME*/newFunction(t1); } private newFunction(t1: T1) { @@ -17,7 +17,7 @@ class C { // ==SCOPE::global scope== class C { M(t1: T1, t2: T2) { - newFunction(t1); + /*RENAME*/newFunction(t1); } } function newFunction(t1: T1) { diff --git a/tests/baselines/reference/extractMethod/extractMethod19.ts b/tests/baselines/reference/extractMethod/extractMethod19.ts index 1bf25550172f3..62a251e5b8a5f 100644 --- a/tests/baselines/reference/extractMethod/extractMethod19.ts +++ b/tests/baselines/reference/extractMethod/extractMethod19.ts @@ -4,7 +4,7 @@ function F(v: V) { } // ==SCOPE::function 'F'== function F(v: V) { - newFunction(); + /*RENAME*/newFunction(); function newFunction() { v.toString(); @@ -12,7 +12,7 @@ function F(v: V) { } // ==SCOPE::global scope== function F(v: V) { - newFunction(v); + /*RENAME*/newFunction(v); } function newFunction(v: V) { v.toString(); diff --git a/tests/baselines/reference/extractMethod/extractMethod2.ts b/tests/baselines/reference/extractMethod/extractMethod2.ts index 3b61a98d0011d..789afb1f6f154 100644 --- a/tests/baselines/reference/extractMethod/extractMethod2.ts +++ b/tests/baselines/reference/extractMethod/extractMethod2.ts @@ -20,7 +20,7 @@ namespace A { namespace B { function a() { - return newFunction(); + return /*RENAME*/newFunction(); function newFunction() { let y = 5; @@ -38,7 +38,7 @@ namespace A { namespace B { function a() { - return newFunction(); + return /*RENAME*/newFunction(); } function newFunction() { @@ -56,7 +56,7 @@ namespace A { namespace B { function a() { - return newFunction(); + return /*RENAME*/newFunction(); } } @@ -74,7 +74,7 @@ namespace A { namespace B { function a() { - return newFunction(x, foo); + return /*RENAME*/newFunction(x, foo); } } } diff --git a/tests/baselines/reference/extractMethod/extractMethod20.ts b/tests/baselines/reference/extractMethod/extractMethod20.ts index 7c35caee30f79..1731109b919df 100644 --- a/tests/baselines/reference/extractMethod/extractMethod20.ts +++ b/tests/baselines/reference/extractMethod/extractMethod20.ts @@ -8,7 +8,7 @@ const _ = class { // ==SCOPE::anonymous class expression== const _ = class { a() { - return this.newFunction(); + return this./*RENAME*/newFunction(); } private newFunction() { @@ -19,7 +19,7 @@ const _ = class { // ==SCOPE::global scope== const _ = class { a() { - return newFunction(); + return /*RENAME*/newFunction(); } } function newFunction() { diff --git a/tests/baselines/reference/extractMethod/extractMethod3.ts b/tests/baselines/reference/extractMethod/extractMethod3.ts index 0837fbfc10dbe..2c45b2fae64ac 100644 --- a/tests/baselines/reference/extractMethod/extractMethod3.ts +++ b/tests/baselines/reference/extractMethod/extractMethod3.ts @@ -18,7 +18,7 @@ namespace A { namespace B { function* a(z: number) { - return yield* newFunction(); + return yield* /*RENAME*/newFunction(); function* newFunction() { let y = 5; @@ -35,7 +35,7 @@ namespace A { namespace B { function* a(z: number) { - return yield* newFunction(z); + return yield* /*RENAME*/newFunction(z); } function* newFunction(z: number) { @@ -52,7 +52,7 @@ namespace A { namespace B { function* a(z: number) { - return yield* newFunction(z); + return yield* /*RENAME*/newFunction(z); } } @@ -69,7 +69,7 @@ namespace A { namespace B { function* a(z: number) { - return yield* newFunction(z, foo); + return yield* /*RENAME*/newFunction(z, foo); } } } diff --git a/tests/baselines/reference/extractMethod/extractMethod4.ts b/tests/baselines/reference/extractMethod/extractMethod4.ts index 107f99e669b8d..8947927603568 100644 --- a/tests/baselines/reference/extractMethod/extractMethod4.ts +++ b/tests/baselines/reference/extractMethod/extractMethod4.ts @@ -20,7 +20,7 @@ namespace A { namespace B { async function a(z: number, z1: any) { - return await newFunction(); + return await /*RENAME*/newFunction(); async function newFunction() { let y = 5; @@ -39,7 +39,7 @@ namespace A { namespace B { async function a(z: number, z1: any) { - return await newFunction(z, z1); + return await /*RENAME*/newFunction(z, z1); } async function newFunction(z: number, z1: any) { @@ -58,7 +58,7 @@ namespace A { namespace B { async function a(z: number, z1: any) { - return await newFunction(z, z1); + return await /*RENAME*/newFunction(z, z1); } } @@ -77,7 +77,7 @@ namespace A { namespace B { async function a(z: number, z1: any) { - return await newFunction(z, z1, foo); + return await /*RENAME*/newFunction(z, z1, foo); } } } diff --git a/tests/baselines/reference/extractMethod/extractMethod5.ts b/tests/baselines/reference/extractMethod/extractMethod5.ts index 77b2f7b554523..807b973a42ca0 100644 --- a/tests/baselines/reference/extractMethod/extractMethod5.ts +++ b/tests/baselines/reference/extractMethod/extractMethod5.ts @@ -23,7 +23,7 @@ namespace A { function a() { let a = 1; - newFunction(); + /*RENAME*/newFunction(); function newFunction() { let y = 5; @@ -43,7 +43,7 @@ namespace A { function a() { let a = 1; - a = newFunction(a); + a = /*RENAME*/newFunction(a); } function newFunction(a: number) { @@ -64,7 +64,7 @@ namespace A { function a() { let a = 1; - a = newFunction(a); + a = /*RENAME*/newFunction(a); } } @@ -85,7 +85,7 @@ namespace A { function a() { let a = 1; - a = newFunction(x, a); + a = /*RENAME*/newFunction(x, a); } } } diff --git a/tests/baselines/reference/extractMethod/extractMethod6.ts b/tests/baselines/reference/extractMethod/extractMethod6.ts index 3c8c0a99b7095..335eed9fef6f6 100644 --- a/tests/baselines/reference/extractMethod/extractMethod6.ts +++ b/tests/baselines/reference/extractMethod/extractMethod6.ts @@ -23,7 +23,7 @@ namespace A { function a() { let a = 1; - return newFunction(); + return /*RENAME*/newFunction(); function newFunction() { let y = 5; @@ -44,7 +44,7 @@ namespace A { let a = 1; var __return: any; - ({ __return, a } = newFunction(a)); + ({ __return, a } = /*RENAME*/newFunction(a)); return __return; } @@ -66,7 +66,7 @@ namespace A { let a = 1; var __return: any; - ({ __return, a } = newFunction(a)); + ({ __return, a } = /*RENAME*/newFunction(a)); return __return; } } @@ -88,7 +88,7 @@ namespace A { let a = 1; var __return: any; - ({ __return, a } = newFunction(x, a)); + ({ __return, a } = /*RENAME*/newFunction(x, a)); return __return; } } diff --git a/tests/baselines/reference/extractMethod/extractMethod7.ts b/tests/baselines/reference/extractMethod/extractMethod7.ts index 3f1e25991bd6f..31c2e4e469385 100644 --- a/tests/baselines/reference/extractMethod/extractMethod7.ts +++ b/tests/baselines/reference/extractMethod/extractMethod7.ts @@ -27,7 +27,7 @@ namespace A { function a() { let a = 1; - return newFunction(); + return /*RENAME*/newFunction(); function newFunction() { let y = 5; @@ -50,7 +50,7 @@ namespace A { let a = 1; var __return: any; - ({ __return, a } = newFunction(a)); + ({ __return, a } = /*RENAME*/newFunction(a)); return __return; } @@ -74,7 +74,7 @@ namespace A { let a = 1; var __return: any; - ({ __return, a } = newFunction(a)); + ({ __return, a } = /*RENAME*/newFunction(a)); return __return; } } @@ -98,7 +98,7 @@ namespace A { let a = 1; var __return: any; - ({ __return, a } = newFunction(x, a)); + ({ __return, a } = /*RENAME*/newFunction(x, a)); return __return; } } diff --git a/tests/baselines/reference/extractMethod/extractMethod8.ts b/tests/baselines/reference/extractMethod/extractMethod8.ts index e6d933ec309d0..23b8629b58f15 100644 --- a/tests/baselines/reference/extractMethod/extractMethod8.ts +++ b/tests/baselines/reference/extractMethod/extractMethod8.ts @@ -14,7 +14,7 @@ namespace A { namespace B { function a() { let a1 = 1; - return newFunction() + 100; + return /*RENAME*/newFunction() + 100; function newFunction() { return 1 + a1 + x; @@ -28,7 +28,7 @@ namespace A { namespace B { function a() { let a1 = 1; - return newFunction(a1) + 100; + return /*RENAME*/newFunction(a1) + 100; } function newFunction(a1: number) { @@ -42,7 +42,7 @@ namespace A { namespace B { function a() { let a1 = 1; - return newFunction(a1) + 100; + return /*RENAME*/newFunction(a1) + 100; } } @@ -56,7 +56,7 @@ namespace A { namespace B { function a() { let a1 = 1; - return newFunction(a1, x) + 100; + return /*RENAME*/newFunction(a1, x) + 100; } } } diff --git a/tests/baselines/reference/extractMethod/extractMethod9.ts b/tests/baselines/reference/extractMethod/extractMethod9.ts index 6d0672f11c150..ab82ae9c0ec3f 100644 --- a/tests/baselines/reference/extractMethod/extractMethod9.ts +++ b/tests/baselines/reference/extractMethod/extractMethod9.ts @@ -13,7 +13,7 @@ namespace A { export interface I { x: number }; namespace B { function a() { - return newFunction(); + return /*RENAME*/newFunction(); function newFunction() { let a1: I = { x: 1 }; @@ -27,7 +27,7 @@ namespace A { export interface I { x: number }; namespace B { function a() { - return newFunction(); + return /*RENAME*/newFunction(); } function newFunction() { @@ -41,7 +41,7 @@ namespace A { export interface I { x: number }; namespace B { function a() { - return newFunction(); + return /*RENAME*/newFunction(); } } @@ -55,7 +55,7 @@ namespace A { export interface I { x: number }; namespace B { function a() { - return newFunction(); + return /*RENAME*/newFunction(); } } } diff --git a/tests/cases/fourslash/extract-method14.ts b/tests/cases/fourslash/extract-method14.ts index 8892d8974a71a..f1d30aac43b38 100644 --- a/tests/cases/fourslash/extract-method14.ts +++ b/tests/cases/fourslash/extract-method14.ts @@ -16,11 +16,10 @@ edit.applyRefactor({ actionName: "scope_1", actionDescription: "Extract function into global scope", newContent: -// TODO: GH#18048 `function foo() { var i = 10; var __return: any; - ({ __return, i } = newFunction(i)); + ({ __return, i } = n/*RENAME*/ewFunction(i)); return __return; } function newFunction(i) { diff --git a/tests/cases/fourslash/extract-method15.ts b/tests/cases/fourslash/extract-method15.ts index 3602d90f40094..eb63c65e0966d 100644 --- a/tests/cases/fourslash/extract-method15.ts +++ b/tests/cases/fourslash/extract-method15.ts @@ -14,10 +14,9 @@ edit.applyRefactor({ actionName: "scope_1", actionDescription: "Extract function into global scope", newContent: -// TODO: GH#18048 `function foo() { var i = 10; - i = newFunction(i); + i = /*RENAME*/newFunction(i); } function newFunction(i: number) { i++; From f592be8f83b0619652087329553c07589f31a1c7 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Fri, 8 Sep 2017 13:07:02 -0700 Subject: [PATCH 4/7] Update test --- tests/baselines/reference/extractMethod/extractMethod22.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/baselines/reference/extractMethod/extractMethod22.ts b/tests/baselines/reference/extractMethod/extractMethod22.ts index 7603c01681ff2..9b85b89559d83 100644 --- a/tests/baselines/reference/extractMethod/extractMethod22.ts +++ b/tests/baselines/reference/extractMethod/extractMethod22.ts @@ -11,7 +11,7 @@ function test() { try { } finally { - return newFunction(); + return /*RENAME*/newFunction(); } function newFunction() { @@ -23,7 +23,7 @@ function test() { try { } finally { - return newFunction(); + return /*RENAME*/newFunction(); } } function newFunction() { From a150596b36c1b7ac8410c10b82dcd007a7cd1991 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Fri, 8 Sep 2017 15:01:50 -0700 Subject: [PATCH 5/7] Ensure name is really unique --- src/services/refactors/extractMethod.ts | 17 ++++++++-------- .../fourslash/extract-method-uniqueName.ts | 20 +++++++++++++++++++ 2 files changed, 28 insertions(+), 9 deletions(-) create mode 100644 tests/cases/fourslash/extract-method-uniqueName.ts diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 538160bb21187..eab6bc3b6c2cd 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -568,14 +568,10 @@ namespace ts.refactor.extractMethod { } } - function getUniqueName(isNameOkay: (name: string) => boolean) { + function getUniqueName(fileText: string): string { let functionNameText = "newFunction"; - if (isNameOkay(functionNameText)) { - return functionNameText; - } - let i = 1; - while (!isNameOkay(functionNameText = `newFunction_${i}`)) { - i++; + for (let i = 1; fileText.indexOf(functionNameText) !== -1; i++) { + functionNameText = `newFunction_${i}`; } return functionNameText; } @@ -595,7 +591,7 @@ namespace ts.refactor.extractMethod { // Make a unique name for the extracted function const file = scope.getSourceFile(); - const functionNameText: string = getUniqueName(n => !file.identifiers.has(n)); + const functionNameText = getUniqueName(file.text); const isJS = isInJavaScriptFile(scope); const functionName = createIdentifier(functionNameText); @@ -691,7 +687,7 @@ namespace ts.refactor.extractMethod { const newNodes: Node[] = []; // replace range with function call - let called = getCalledExpression(scope, range, functionNameText); // tslint:disable-line prefer-const + const called = getCalledExpression(scope, range, functionNameText); let call: Expression = createCall(called, callTypeArguments, // Note that no attempt is made to take advantage of type argument inference @@ -776,6 +772,9 @@ namespace ts.refactor.extractMethod { Debug.assert(fileName === renameFilename); for (const change of textChanges) { const { span, newText } = change; + // Note: We are assuming that the call expression comes before the function declaration, + // because we want the new cursor to be on the call expression, + // which is closer to where the user was before extracting the function. const index = newText.indexOf(functionNameText); if (index !== -1) { return span.start + delta + index; diff --git a/tests/cases/fourslash/extract-method-uniqueName.ts b/tests/cases/fourslash/extract-method-uniqueName.ts new file mode 100644 index 0000000000000..d0da7f70c548b --- /dev/null +++ b/tests/cases/fourslash/extract-method-uniqueName.ts @@ -0,0 +1,20 @@ +/// + +////// newFunction +/////*start*/1 + 1/*end*/; + +goTo.select('start', 'end') +edit.applyRefactor({ + refactorName: "Extract Method", + actionName: "scope_0", + actionDescription: "Extract function into global scope", + newContent: +`// newFunction +/*RENAME*/newFunction_1(); + +function newFunction_1() { + // newFunction + 1 + 1; +} +` +}); From 6953143b7372bdd90a6a566251d4d529b9387d0d Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Fri, 8 Sep 2017 15:03:43 -0700 Subject: [PATCH 6/7] Improvements to test code --- src/harness/fourslash.ts | 3 ++- src/harness/unittests/extractMethods.ts | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 5dbe2563bb51f..d89a4677ef90a 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2834,7 +2834,8 @@ namespace FourSlash { } } else { - // TODO: test editInfo.renameFilename too + // TODO: test editInfo.renameFilename value + assert.isDefined(editInfo.renameFilename); if (renamePosition !== editInfo.renameLocation) { this.raiseError(`Expected rename position of ${renamePosition}, but got ${editInfo.renameLocation}`); } diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts index 3d2776ef2893c..29ba97c62ab1a 100644 --- a/src/harness/unittests/extractMethods.ts +++ b/src/harness/unittests/extractMethods.ts @@ -692,7 +692,7 @@ function test(x: number) { data.push(sourceFile.text); for (const r of results) { const { renameLocation, edits } = refactor.extractMethod.getPossibleExtractionAtIndex(result.targetRange, context, results.indexOf(r)); - Debug.assert(edits.length === 1); + assert.lengthOf(edits, 1); data.push(`// ==SCOPE::${r.scopeDescription}==`); const newText = textChanges.applyChanges(sourceFile.text, edits[0].textChanges); const newTextWithRename = newText.slice(0, renameLocation) + "/*RENAME*/" + newText.slice(renameLocation); From 6303626ca0812005242629ca355198e4b309ab59 Mon Sep 17 00:00:00 2001 From: andy-ms Date: Fri, 8 Sep 2017 15:56:54 -0700 Subject: [PATCH 7/7] Respond to PR comments --- src/harness/unittests/extractMethods.ts | 2 +- src/services/refactors/extractMethod.ts | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts index 29ba97c62ab1a..95023bf465649 100644 --- a/src/harness/unittests/extractMethods.ts +++ b/src/harness/unittests/extractMethods.ts @@ -691,7 +691,7 @@ function test(x: number) { data.push(`// ==ORIGINAL==`); data.push(sourceFile.text); for (const r of results) { - const { renameLocation, edits } = refactor.extractMethod.getPossibleExtractionAtIndex(result.targetRange, context, results.indexOf(r)); + const { renameLocation, edits } = refactor.extractMethod.getExtractionAtIndex(result.targetRange, context, results.indexOf(r)); assert.lengthOf(edits, 1); data.push(`// ==SCOPE::${r.scopeDescription}==`); const newText = textChanges.applyChanges(sourceFile.text, edits[0].textChanges); diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index eab6bc3b6c2cd..2abc0cfcbbff8 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -75,7 +75,7 @@ namespace ts.refactor.extractMethod { const index = +parsedIndexMatch[1]; Debug.assert(isFinite(index), "Expected to parse a finite number from the scope index"); - return getPossibleExtractionAtIndex(targetRange, context, index); + return getExtractionAtIndex(targetRange, context, index); } // Move these into diagnostic messages if they become user-facing @@ -482,7 +482,7 @@ namespace ts.refactor.extractMethod { } // exported only for tests - export function getPossibleExtractionAtIndex(targetRange: TargetRange, context: RefactorContext, requestedChangesIndex: number): RefactorEditInfo { + export function getExtractionAtIndex(targetRange: TargetRange, context: RefactorContext, requestedChangesIndex: number): RefactorEditInfo { const { scopes, readsAndWrites: { target, usagesPerScope, errorsPerScope } } = getPossibleExtractionsWorker(targetRange, context); Debug.assert(!errorsPerScope[requestedChangesIndex].length, "The extraction went missing? How?"); context.cancellationToken.throwIfCancellationRequested(); @@ -689,7 +689,8 @@ namespace ts.refactor.extractMethod { // replace range with function call const called = getCalledExpression(scope, range, functionNameText); - let call: Expression = createCall(called, + let call: Expression = createCall( + called, callTypeArguments, // Note that no attempt is made to take advantage of type argument inference callArguments); if (range.facts & RangeFacts.IsGenerator) { @@ -772,7 +773,7 @@ namespace ts.refactor.extractMethod { Debug.assert(fileName === renameFilename); for (const change of textChanges) { const { span, newText } = change; - // Note: We are assuming that the call expression comes before the function declaration, + // TODO(acasey): We are assuming that the call expression comes before the function declaration, // because we want the new cursor to be on the call expression, // which is closer to where the user was before extracting the function. const index = newText.indexOf(functionNameText); @@ -927,7 +928,7 @@ namespace ts.refactor.extractMethod { readonly node: Node; } - export interface ScopeUsages { + interface ScopeUsages { readonly usages: Map; readonly typeParameterUsages: Map; // Key is type ID readonly substitutions: Map;