From 3eea1a9e9a7f575c4469f57e6fb72877e4f13e60 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 14 Sep 2017 16:04:44 -0700 Subject: [PATCH 1/9] Generalize extract method to handle constants as well Major changes: 1) Instead of skipping undesirable scopes, include them and mark them with errors. Constants can be extracted into more scopes. 2) Update the tests to call through the "public" API. This caused some baseline changes. 3) Rename refactoring to "Extract Symbol" for generality. 4) Return a second ApplicableRefactorInfo for constants. Distinguish the two by splitting the action name. --- src/compiler/diagnosticMessages.json | 12 +- src/harness/unittests/extractMethods.ts | 25 +- src/services/refactors/extractMethod.ts | 429 +++++++++++++----- .../reference/extractMethod/extractMethod1.ts | 8 +- .../extractMethod/extractMethod10.ts | 10 +- .../extractMethod/extractMethod11.ts | 10 +- .../extractMethod/extractMethod12.ts | 6 +- .../extractMethod/extractMethod13.ts | 6 +- .../extractMethod/extractMethod14.ts | 14 +- .../extractMethod/extractMethod15.ts | 14 +- .../extractMethod/extractMethod16.ts | 4 +- .../extractMethod/extractMethod17.ts | 8 +- .../extractMethod/extractMethod18.ts | 8 +- .../extractMethod/extractMethod19.ts | 4 +- .../reference/extractMethod/extractMethod2.ts | 8 +- .../extractMethod/extractMethod20.ts | 8 +- .../extractMethod/extractMethod21.ts | 4 +- .../extractMethod/extractMethod22.ts | 4 +- .../extractMethod/extractMethod23.ts | 6 +- .../extractMethod/extractMethod24.ts | 6 +- .../extractMethod/extractMethod25.ts | 4 +- .../extractMethod/extractMethod26.ts | 8 +- .../extractMethod/extractMethod27.ts | 8 +- .../extractMethod/extractMethod28.ts | 8 +- .../extractMethod/extractMethod29.ts | 4 +- .../reference/extractMethod/extractMethod3.ts | 8 +- .../extractMethod/extractMethod30.ts | 4 +- .../extractMethod/extractMethod31.ts | 4 +- .../extractMethod/extractMethod32.ts | 4 +- .../extractMethod/extractMethod33.ts | 4 +- .../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 +- .../extract-method-empty-namespace.ts | 4 +- .../fourslash/extract-method-formatting.ts | 4 +- .../fourslash/extract-method-not-for-empty.ts | 2 +- .../extract-method-not-for-import.ts | 2 +- .../fourslash/extract-method-uniqueName.ts | 4 +- tests/cases/fourslash/extract-method1.ts | 8 +- tests/cases/fourslash/extract-method10.ts | 4 +- tests/cases/fourslash/extract-method11.ts | 4 +- tests/cases/fourslash/extract-method13.ts | 24 +- tests/cases/fourslash/extract-method14.ts | 4 +- tests/cases/fourslash/extract-method15.ts | 4 +- tests/cases/fourslash/extract-method17.ts | 4 +- tests/cases/fourslash/extract-method18.ts | 4 +- tests/cases/fourslash/extract-method19.ts | 4 +- tests/cases/fourslash/extract-method2.ts | 4 +- tests/cases/fourslash/extract-method20.ts | 4 +- tests/cases/fourslash/extract-method21.ts | 10 +- tests/cases/fourslash/extract-method22.ts | 2 +- tests/cases/fourslash/extract-method23.ts | 2 +- tests/cases/fourslash/extract-method24.ts | 4 +- tests/cases/fourslash/extract-method25.ts | 4 +- tests/cases/fourslash/extract-method26.ts | 8 +- tests/cases/fourslash/extract-method3.ts | 6 +- tests/cases/fourslash/extract-method4.ts | 2 +- tests/cases/fourslash/extract-method5.ts | 4 +- tests/cases/fourslash/extract-method6.ts | 4 +- tests/cases/fourslash/extract-method7.ts | 6 +- tests/cases/fourslash/extract-method8.ts | 6 +- tests/cases/fourslash/extract-method9.ts | 4 +- 65 files changed, 539 insertions(+), 305 deletions(-) diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index bf8fcdc4f84b3..573993f1b5e29 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -3703,7 +3703,7 @@ "code": 95002 }, - "Extract function": { + "Extract symbol": { "category": "Message", "code": 95003 }, @@ -3711,5 +3711,15 @@ "Extract to {0}": { "category": "Message", "code": 95004 + }, + + "Extract function": { + "category": "Message", + "code": 95005 + }, + + "Extract constant": { + "category": "Message", + "code": 95006 } } diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts index 190cd1d5be0bc..3a161baa8e730 100644 --- a/src/harness/unittests/extractMethods.ts +++ b/src/harness/unittests/extractMethods.ts @@ -105,7 +105,7 @@ namespace ts { if (!selectionRange) { throw new Error(`Test ${s} does not specify selection range`); } - const result = refactor.extractMethod.getRangeToExtract(file, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); + const result = refactor.extractSymbol.getRangeToExtract(file, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); assert(result.targetRange === undefined, "failure expected"); const sortedErrors = result.errors.map(e => e.messageText).sort(); assert.deepEqual(sortedErrors, expectedErrors.sort(), "unexpected errors"); @@ -119,7 +119,7 @@ namespace ts { if (!selectionRange) { throw new Error(`Test ${s} does not specify selection range`); } - const result = refactor.extractMethod.getRangeToExtract(f, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); + const result = refactor.extractSymbol.getRangeToExtract(f, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); const expectedRange = t.ranges.get("extracted"); if (expectedRange) { let start: number, end: number; @@ -407,7 +407,7 @@ function test(x: number) { testExtractRangeFailed("extractRangeFailed9", `var x = ([#||]1 + 2);`, [ - "Statement or expression expected." + "Cannot extract empty range." ]); testExtractRangeFailed("extract-method-not-for-token-expression-statement", `[#|a|]`, ["Select more than a single identifier."]); @@ -604,7 +604,7 @@ function test(x: number) { // doesn't handle type parameter shadowing. testExtractMethod("extractMethod14", `function F(t1: T) { - function F(t2: T) { + function G(t2: T) { [#|t1.toString(); t2.toString();|] } @@ -612,7 +612,7 @@ function test(x: number) { // Confirm that the constraint is preserved. testExtractMethod("extractMethod15", `function F(t1: T) { - function F(t2: U) { + function G(t2: U) { [#|t2.toString();|] } }`); @@ -799,19 +799,20 @@ function parsePrimaryExpression(): any { newLineCharacter, program, file: sourceFile, - startPosition: -1, + startPosition: selectionRange.start, + endPosition: selectionRange.end, rulesProvider: getRuleProvider() }; - const result = refactor.extractMethod.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); - assert.equal(result.errors, undefined, "expect no errors"); - const results = refactor.extractMethod.getPossibleExtractions(result.targetRange, context); + const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); + assert.equal(rangeToExtract.errors, undefined, "expect no errors"); + const actions = refactor.extractSymbol.getAvailableActions(context)[0].actions; // TODO (acasey): smarter index const data: string[] = []; data.push(`// ==ORIGINAL==`); data.push(sourceFile.text); - for (const r of results) { - const { renameLocation, edits } = refactor.extractMethod.getExtractionAtIndex(result.targetRange, context, results.indexOf(r)); + for (const action of actions) { + const { renameLocation, edits } = refactor.extractSymbol.getEditsForAction(context, action.name); assert.lengthOf(edits, 1); - data.push(`// ==SCOPE::${r.scopeDescription}==`); + data.push(`// ==SCOPE::${action.description}==`); const newText = textChanges.applyChanges(sourceFile.text, edits[0].textChanges); const newTextWithRename = newText.slice(0, renameLocation) + "/*RENAME*/" + newText.slice(renameLocation); data.push(newTextWithRename); diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 481834c5a094b..a1212e7260ce6 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -2,18 +2,21 @@ /// /* @internal */ -namespace ts.refactor.extractMethod { - const extractMethod: Refactor = { - name: "Extract Method", - description: Diagnostics.Extract_function.message, +namespace ts.refactor.extractSymbol { + const extractSymbol: Refactor = { + name: "Extract Symbol", + description: Diagnostics.Extract_symbol.message, getAvailableActions, getEditsForAction, }; - registerRefactor(extractMethod); + registerRefactor(extractSymbol); - /** Compute the associated code actions */ - function getAvailableActions(context: RefactorContext): ApplicableRefactorInfo[] | undefined { + /** + * Compute the associated code actions + * Exported for tests. + */ + export function getAvailableActions(context: RefactorContext): ApplicableRefactorInfo[] | undefined { const rangeToExtract = getRangeToExtract(context.file, { start: context.startPosition, length: getRefactorContextLength(context) }); const targetRange: TargetRange = rangeToExtract.targetRange; @@ -27,54 +30,90 @@ namespace ts.refactor.extractMethod { return undefined; } - const actions: RefactorActionInfo[] = []; - const usedNames: Map = createMap(); + const functionActions: RefactorActionInfo[] = []; + const usedFunctionNames: Map = createMap(); + + const constantActions: RefactorActionInfo[] = []; + const usedConstantNames: Map = createMap(); let i = 0; - for (const { scopeDescription, errors } of extractions) { + for (const extraction of extractions) { // Skip these since we don't have a way to report errors yet - if (errors.length) { - continue; + if (extraction.functionErrors.length === 0) { + // 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_to_0.message, [extraction.functionDescription]); + if (!usedFunctionNames.has(description)) { + usedFunctionNames.set(description, true); + functionActions.push({ + description, + name: `function_scope_${i}` + }); + } } - // 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_to_0.message, [scopeDescription]); - if (!usedNames.has(description)) { - usedNames.set(description, true); - actions.push({ - description, - name: `scope_${i}` - }); + // Skip these since we don't have a way to report errors yet + if (extraction.constantErrors.length === 0) { + // 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_to_0.message, [extraction.constantDescription]); + if (!usedConstantNames.has(description)) { + usedConstantNames.set(description, true); + constantActions.push({ + description, + name: `constant_scope_${i}` + }); + } } + // *do* increment i anyway because we'll look for the i-th scope // later when actually doing the refactoring if the user requests it i++; } - if (actions.length === 0) { - return undefined; + const infos: ApplicableRefactorInfo[] = []; + + if (functionActions.length) { + infos.push({ + name: extractSymbol.name, + description: Diagnostics.Extract_function.message, + actions: functionActions + }); } - return [{ - name: extractMethod.name, - description: extractMethod.description, - inlineable: true, - actions - }]; + if (constantActions.length) { + infos.push({ + name: extractSymbol.name, + description: Diagnostics.Extract_constant.message, + actions: constantActions + }); + } + + return infos.length ? infos : undefined; } - function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined { + /* Exported for tests */ + export function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined { const rangeToExtract = getRangeToExtract(context.file, { start: context.startPosition, length: getRefactorContextLength(context) }); const targetRange: TargetRange = rangeToExtract.targetRange; - const parsedIndexMatch = /^scope_(\d+)$/.exec(actionName); - Debug.assert(!!parsedIndexMatch, "Scope name should have matched the regexp"); - const index = +parsedIndexMatch[1]; - Debug.assert(isFinite(index), "Expected to parse a finite number from the scope index"); + const parsedFunctionIndexMatch = /^function_scope_(\d+)$/.exec(actionName); + if (parsedFunctionIndexMatch) { + const index = +parsedFunctionIndexMatch[1]; + Debug.assert(isFinite(index), "Expected to parse a finite number from the function scope index"); + return getFunctionExtractionAtIndex(targetRange, context, index); + } + + const parsedConstantIndexMatch = /^constant_scope_(\d+)$/.exec(actionName); + if (parsedConstantIndexMatch) { + const index = +parsedConstantIndexMatch[1]; + Debug.assert(isFinite(index), "Expected to parse a finite number from the constant scope index"); + return getConstantExtractionAtIndex(targetRange, context, index); + } - return getExtractionAtIndex(targetRange, context, index); + Debug.fail("Unrecognized action name"); } // Move these into diagnostic messages if they become user-facing @@ -83,7 +122,11 @@ namespace ts.refactor.extractMethod { return { message, code: 0, category: DiagnosticCategory.Message, key: message }; } - export const CannotExtractFunction: DiagnosticMessage = createMessage("Cannot extract function."); + export const CannotExtractRange: DiagnosticMessage = createMessage("Cannot extract range."); + export const CannotExtractImport: DiagnosticMessage = createMessage("Cannot extract import statement."); + export const CannotExtractSuper: DiagnosticMessage = createMessage("Cannot extract super call."); + export const CannotExtractEmpty: DiagnosticMessage = createMessage("Cannot extract empty range."); + export const ExpressionExpected: DiagnosticMessage = createMessage("expression expected."); export const StatementOrExpressionExpected: DiagnosticMessage = createMessage("Statement or expression expected."); export const CannotExtractRangeContainingConditionalBreakOrContinueStatements: DiagnosticMessage = createMessage("Cannot extract range containing conditional break or continue statements."); export const CannotExtractRangeContainingConditionalReturnStatement: DiagnosticMessage = createMessage("Cannot extract range containing conditional return statement."); @@ -91,11 +134,13 @@ namespace ts.refactor.extractMethod { export const CannotExtractRangeThatContainsWritesToReferencesLocatedOutsideOfTheTargetRangeInGenerators: DiagnosticMessage = createMessage("Cannot extract range containing writes to references located outside of the target range in generators."); export const TypeWillNotBeVisibleInTheNewScope = createMessage("Type will not visible in the new scope."); export const FunctionWillNotBeVisibleInTheNewScope = createMessage("Function will not visible in the new scope."); - export const InsufficientSelection = createMessage("Select more than a single identifier."); + export const CannotExtractIdentifier = createMessage("Select more than a single identifier."); export const CannotExtractExportedEntity = createMessage("Cannot extract exported declaration"); export const CannotCombineWritesAndReturns = createMessage("Cannot combine writes and returns"); export const CannotExtractReadonlyPropertyInitializerOutsideConstructor = createMessage("Cannot move initialization of read-only class property outside of the constructor"); export const CannotExtractAmbientBlock = createMessage("Cannot extract code from ambient contexts"); + export const CannotAccessVariablesFromNestedScopes = createMessage("Cannot access variables from nested scopes"); + export const CannotExtractToOtherFunctionLike = createMessage("Cannot extract method to a function-like scope that is not a function"); } enum RangeFacts { @@ -150,7 +195,7 @@ namespace ts.refactor.extractMethod { const { length } = span; if (length === 0) { - return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.StatementOrExpressionExpected)] }; + return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractEmpty)] }; } // Walk up starting from the the start position until we find a non-SourceFile node that subsumes the selected span. @@ -167,7 +212,7 @@ namespace ts.refactor.extractMethod { if (!start || !end) { // cannot find either start or end node - return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractFunction)] }; + return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractRange)] }; } if (start.parent !== end.parent) { @@ -193,13 +238,13 @@ namespace ts.refactor.extractMethod { } else { // start and end nodes belong to different subtrees - return createErrorResult(sourceFile, span.start, length, Messages.CannotExtractFunction); + return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractRange)] }; } } if (start !== end) { // start and end should be statements and parent should be either block or a source file if (!isBlockLike(start.parent)) { - return createErrorResult(sourceFile, span.start, length, Messages.CannotExtractFunction); + return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractRange)] }; } const statements: Statement[] = []; for (const statement of (start.parent).statements) { @@ -216,22 +261,21 @@ namespace ts.refactor.extractMethod { } return { targetRange: { range: statements, facts: rangeFacts, declarations } }; } - else { - // We have a single node (start) - const errors = checkRootNode(start) || checkNode(start); - if (errors) { - return { errors }; - } - return { targetRange: { range: getStatementOrExpressionRange(start), facts: rangeFacts, declarations } }; + + if (isExpressionStatement(start)) { + start = start.expression; } - function createErrorResult(sourceFile: SourceFile, start: number, length: number, message: DiagnosticMessage): RangeToExtract { - return { errors: [createFileDiagnostic(sourceFile, start, length, message)] }; + // We have a single node (start) + const errors = checkRootNode(start) || checkNode(start); + if (errors) { + return { errors }; } + return { targetRange: { range: getStatementOrExpressionRange(start), facts: rangeFacts, declarations } }; function checkRootNode(node: Node): Diagnostic[] | undefined { - if (isIdentifier(isExpressionStatement(node) ? node.expression : node)) { - return [createDiagnosticForNode(node, Messages.InsufficientSelection)]; + if (isIdentifier(node)) { + return [createDiagnosticForNode(node, Messages.CannotExtractIdentifier)]; } return undefined; } @@ -309,7 +353,7 @@ namespace ts.refactor.extractMethod { // Some things can't be extracted in certain situations switch (node.kind) { case SyntaxKind.ImportDeclaration: - (errors || (errors = [])).push(createDiagnosticForNode(node, Messages.CannotExtractFunction)); + (errors || (errors = [])).push(createDiagnosticForNode(node, Messages.CannotExtractImport)); return true; case SyntaxKind.SuperKeyword: // For a super *constructor call*, we have to be extracting the entire class, @@ -318,7 +362,7 @@ namespace ts.refactor.extractMethod { // Super constructor call const containingClass = getContainingClass(node); if (containingClass.pos < span.start || containingClass.end >= (span.start + span.length)) { - (errors || (errors = [])).push(createDiagnosticForNode(node, Messages.CannotExtractFunction)); + (errors || (errors = [])).push(createDiagnosticForNode(node, Messages.CannotExtractSuper)); return true; } } @@ -328,7 +372,7 @@ namespace ts.refactor.extractMethod { break; } - if (!node || isFunctionLike(node) || isClassLike(node)) { + if (!node || isFunctionLikeDeclaration(node) || isClassLike(node)) { switch (node.kind) { case SyntaxKind.FunctionDeclaration: case SyntaxKind.ClassDeclaration: @@ -439,9 +483,8 @@ namespace ts.refactor.extractMethod { return undefined; } - function isValidExtractionTarget(node: Node): node is Scope { - // Note that we don't use isFunctionLike because we don't want to put the extracted closure *inside* a method - return (node.kind === SyntaxKind.FunctionDeclaration) || isSourceFile(node) || isModuleBlock(node) || isClassLike(node); + function isScope(node: Node): node is Scope { + return isFunctionLikeDeclaration(node) || isSourceFile(node) || isModuleBlock(node) || isClassLike(node); } /** @@ -468,14 +511,14 @@ namespace ts.refactor.extractMethod { // * Function declaration // * Class declaration or expression // * Module/namespace or source file - if (current !== start && isValidExtractionTarget(current)) { + if (current !== start && isScope(current)) { (scopes = scopes || []).push(current); } // A function parameter's initializer is actually in the outer scope, not the function declaration if (current && current.parent && current.parent.kind === SyntaxKind.Parameter) { // Skip all the way to the outer scope of the function that declared this parameter - current = findAncestor(current, parent => isFunctionLike(parent)).parent; + current = findAncestor(current, parent => isFunctionLikeDeclaration(parent)).parent; } else { current = current.parent; @@ -485,29 +528,42 @@ namespace ts.refactor.extractMethod { return scopes; } - // exported only for tests - 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?"); + function getFunctionExtractionAtIndex(targetRange: TargetRange, context: RefactorContext, requestedChangesIndex: number): RefactorEditInfo { + const { scopes, readsAndWrites: { target, usagesPerScope, functionErrorsPerScope } } = getPossibleExtractionsWorker(targetRange, context); + Debug.assert(!functionErrorsPerScope[requestedChangesIndex].length, "The extraction went missing? How?"); context.cancellationToken.throwIfCancellationRequested(); return extractFunctionInScope(target, scopes[requestedChangesIndex], usagesPerScope[requestedChangesIndex], targetRange, context); } + function getConstantExtractionAtIndex(targetRange: TargetRange, context: RefactorContext, requestedChangesIndex: number): RefactorEditInfo { + const { scopes, readsAndWrites: { target, usagesPerScope, constantErrorsPerScope } } = getPossibleExtractionsWorker(targetRange, context); + Debug.assert(!constantErrorsPerScope[requestedChangesIndex].length, "The extraction went missing? How?"); + context.cancellationToken.throwIfCancellationRequested(); + Debug.assert(target === targetRange.range); + return extractConstantInScope(target as Expression, scopes[requestedChangesIndex], usagesPerScope[requestedChangesIndex], targetRange.facts, context); + } + interface PossibleExtraction { - readonly scopeDescription: string; - readonly errors: ReadonlyArray; + readonly functionDescription: string; + readonly functionErrors: ReadonlyArray; + readonly constantDescription: string; + readonly constantErrors: 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. */ - // exported only for tests - export function getPossibleExtractions(targetRange: TargetRange, context: RefactorContext): ReadonlyArray | undefined { - const { scopes, readsAndWrites: { errorsPerScope } } = getPossibleExtractionsWorker(targetRange, context); + function getPossibleExtractions(targetRange: TargetRange, context: RefactorContext): ReadonlyArray | undefined { + const { scopes, readsAndWrites: { functionErrorsPerScope, constantErrorsPerScope } } = getPossibleExtractionsWorker(targetRange, context); // Need the inner type annotation to avoid https://github.com/Microsoft/TypeScript/issues/7547 - return scopes.map((scope, i): PossibleExtraction => - ({ scopeDescription: getDescriptionForScope(scope), errors: errorsPerScope[i] })); + const extractions = scopes.map((scope, i): PossibleExtraction => ({ + functionDescription: getDescriptionForFunctionInScope(scope), + functionErrors: functionErrorsPerScope[i], + constantDescription: getDescriptionForConstantInScope(scope), + constantErrors: constantErrorsPerScope[i], + })); + return extractions; } function getPossibleExtractionsWorker(targetRange: TargetRange, context: RefactorContext): { readonly scopes: Scope[], readonly readsAndWrites: ReadsAndWrites } { @@ -533,13 +589,20 @@ namespace ts.refactor.extractMethod { return { scopes, readsAndWrites }; } - function getDescriptionForScope(scope: Scope): string { + function getDescriptionForFunctionInScope(scope: Scope): string { return isFunctionLikeDeclaration(scope) ? `inner function in ${getDescriptionForFunctionLikeDeclaration(scope)}` : isClassLike(scope) ? `method in ${getDescriptionForClassLikeDeclaration(scope)}` : `function in ${getDescriptionForModuleLikeDeclaration(scope)}`; } + function getDescriptionForConstantInScope(scope: Scope): string { + return isFunctionLikeDeclaration(scope) + ? `constant in ${getDescriptionForFunctionLikeDeclaration(scope)}` + : isClassLike(scope) + ? `readonly field in ${getDescriptionForClassLikeDeclaration(scope)}` + : `constant in ${getDescriptionForModuleLikeDeclaration(scope)}`; + } function getDescriptionForFunctionLikeDeclaration(scope: FunctionLikeDeclaration): string { switch (scope.kind) { case SyntaxKind.Constructor: @@ -573,12 +636,12 @@ namespace ts.refactor.extractMethod { : scope.externalModuleIndicator ? "module scope" : "global scope"; } - function getUniqueName(fileText: string): string { - let functionNameText = "newFunction"; - for (let i = 1; fileText.indexOf(functionNameText) !== -1; i++) { - functionNameText = `newFunction_${i}`; + function getUniqueName(baseName: string, fileText: string): string { + let nameText = baseName; + for (let i = 1; fileText.indexOf(nameText) !== -1; i++) { + nameText = `${baseName}_${i}`; } - return functionNameText; + return nameText; } /** @@ -596,7 +659,7 @@ namespace ts.refactor.extractMethod { // Make a unique name for the extracted function const file = scope.getSourceFile(); - const functionNameText = getUniqueName(file.text); + const functionNameText = getUniqueName(isClassLike(scope) ? "newMethod" : "newFunction", file.text); const isJS = isInJavaScriptFile(scope); const functionName = createIdentifier(functionNameText); @@ -688,7 +751,7 @@ namespace ts.refactor.extractMethod { const changeTracker = textChanges.ChangeTracker.fromContext(context); const minInsertionPos = (isReadonlyArray(range.range) ? lastOrUndefined(range.range) : range.range).end; - const nodeToInsertBefore = getNodeToInsertBefore(minInsertionPos, scope); + const nodeToInsertBefore = getNodeToInsertFunctionBefore(minInsertionPos, scope); if (nodeToInsertBefore) { changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newFunction, { suffix: context.newLineCharacter + context.newLineCharacter }); } @@ -774,27 +837,130 @@ namespace ts.refactor.extractMethod { const renameRange = isReadonlyArray(range.range) ? range.range[0] : range.range; const renameFilename = renameRange.getSourceFile().fileName; - const renameLocation = getRenameLocation(edits, renameFilename, functionNameText); + const renameLocation = getRenameLocation(edits, renameFilename, functionNameText, /*isDeclaredBeforeUse*/ false); + return { renameFilename, renameLocation, edits }; + } + + /** + * 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 extractConstantInScope( + node: Expression, + scope: Scope, + { substitutions }: ScopeUsages, + rangeFacts: RangeFacts, + context: RefactorContext): RefactorEditInfo { + + const checker = context.program.getTypeChecker(); + + // Make a unique name for the extracted variable + const file = scope.getSourceFile(); + const localNameText = getUniqueName(isClassLike(scope) ? "newProperty" : "newLocal", file.text); + const isJS = isInJavaScriptFile(scope); + + const variableType = isJS + ? undefined + : checker.typeToTypeNode(checker.getContextualType(node)); + + const initializer = transformConstantInitializer(node, substitutions); + + const changeTracker = textChanges.ChangeTracker.fromContext(context); + + if (isClassLike(scope)) { + // always create private method in TypeScript files + const modifiers: Modifier[] = []; + if (!isJS) { + modifiers.push(createToken(SyntaxKind.PrivateKeyword)); + } + if (rangeFacts & RangeFacts.InStaticRegion) { + modifiers.push(createToken(SyntaxKind.StaticKeyword)); + } + if (!isJS) { + modifiers.push(createToken(SyntaxKind.ReadonlyKeyword)); + } + + const newVariable = createProperty( + /*decorators*/ undefined, + modifiers.length ? modifiers : undefined, + localNameText, + /*questionToken*/ undefined, + variableType, + initializer); + + const localReference = createPropertyAccess( + rangeFacts & RangeFacts.InStaticRegion + ? createIdentifier(scope.name.getText()) + : createThis(), + createIdentifier(localNameText)); + + // Declare + const minInsertionPos = node.end; + const nodeToInsertBefore = getNodeToInsertConstantBefore(minInsertionPos, scope); + changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newVariable, { suffix: context.newLineCharacter + context.newLineCharacter }); + + // Consume + changeTracker.replaceNodeWithNodes(context.file, node, [localReference], { nodeSeparator: context.newLineCharacter }); + } + else { + const newVariable = createVariableStatement( + /*modifiers*/ undefined, + createVariableDeclarationList( + [createVariableDeclaration(localNameText, variableType, initializer)], + NodeFlags.Const)); + + // If the parent is an expression statement, replace the statement with the declaration + if (node.parent.kind === SyntaxKind.ExpressionStatement) { + changeTracker.replaceNodeWithNodes(context.file, node.parent, [newVariable], { nodeSeparator: context.newLineCharacter }); + } + else { + // Declare + const minInsertionPos = node.end; + const nodeToInsertBefore = getNodeToInsertConstantBefore(minInsertionPos, scope); + changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newVariable, { suffix: context.newLineCharacter + context.newLineCharacter }); + + // Consume + const localReference = createIdentifier(localNameText); + changeTracker.replaceNodeWithNodes(context.file, node, [localReference], { nodeSeparator: context.newLineCharacter }); + } + } + + const edits = changeTracker.getChanges(); + + const renameFilename = node.getSourceFile().fileName; + const renameLocation = getRenameLocation(edits, renameFilename, localNameText, /*isDeclaredBeforeUse*/ true); return { renameFilename, renameLocation, edits }; } - function getRenameLocation(edits: ReadonlyArray, renameFilename: string, functionNameText: string): number { + /** + * @return The index of the (only) reference to the extracted symbol. We want the cursor + * to be on the reference, rather than the declaration, because it's closer to where the + * user was before extracting it. + */ + function getRenameLocation(edits: ReadonlyArray, renameFilename: string, functionNameText: string, isDeclaredBeforeUse: boolean): number { let delta = 0; + let lastPos = -1; for (const { fileName, textChanges } of edits) { Debug.assert(fileName === renameFilename); for (const change of textChanges) { const { span, newText } = change; - // 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); if (index !== -1) { - return span.start + delta + index; + lastPos = span.start + delta + index; + + // If the reference comes first, return immediately. + if (!isDeclaredBeforeUse) { + return lastPos; + } } delta += newText.length - span.length; } } - throw new Error(); // Didn't find the text we inserted? + + // If the declaration comes first, return the position of the last occurrence. + Debug.assert(isDeclaredBeforeUse); + Debug.assert(lastPos >= 0); + return lastPos; } function getFirstDeclaration(type: Type): Declaration | undefined { @@ -899,7 +1065,7 @@ namespace ts.refactor.extractMethod { } else { const oldIgnoreReturns = ignoreReturns; - ignoreReturns = ignoreReturns || isFunctionLike(node) || isClassLike(node); + ignoreReturns = ignoreReturns || isFunctionLikeDeclaration(node) || isClassLike(node); const substitution = substitutions.get(getNodeId(node).toString()); const result = substitution || visitEachChild(node, visitor, nullTransformationContext); ignoreReturns = oldIgnoreReturns; @@ -908,8 +1074,19 @@ namespace ts.refactor.extractMethod { } } + function transformConstantInitializer(initializer: Expression, substitutions: ReadonlyMap): Expression { + return substitutions.size + ? visitor(initializer) as Expression + : initializer; + + function visitor(node: Node): VisitResult { + const substitution = substitutions.get(getNodeId(node).toString()); + return substitution || visitEachChild(node, visitor, nullTransformationContext); + } + } + function getStatementsOrClassElements(scope: Scope): ReadonlyArray | ReadonlyArray { - if (isFunctionLike(scope)) { + if (isFunctionLikeDeclaration(scope)) { const body = scope.body; if (isBlock(body)) { return body.statements; @@ -932,9 +1109,23 @@ namespace ts.refactor.extractMethod { * If `scope` contains a function after `minPos`, then return the first such function. * Otherwise, return `undefined`. */ - function getNodeToInsertBefore(minPos: number, scope: Scope): Node | undefined { + function getNodeToInsertFunctionBefore(minPos: number, scope: Scope): Node | undefined { return find(getStatementsOrClassElements(scope), child => - child.pos >= minPos && isFunctionLike(child) && !isConstructorDeclaration(child)); + child.pos >= minPos && isFunctionLikeDeclaration(child) && !isConstructorDeclaration(child)); + } + + function getNodeToInsertConstantBefore(minPos: number, scope: Scope): Node { + const isClassLikeScope = isClassLike(scope); + const children = getStatementsOrClassElements(scope); + let prevChild: Statement | ClassElement | undefined = undefined; + for (const child of children) { + if (child.pos >= minPos || (isClassLikeScope && !isPropertyDeclaration(child))) { + break; + } + prevChild = child; + } + + return prevChild || children[0]; // There must be one - minPos is in one. } function getPropertyAssignmentsForWrites(writes: ReadonlyArray): ShorthandPropertyAssignment[] { @@ -982,7 +1173,8 @@ namespace ts.refactor.extractMethod { interface ReadsAndWrites { readonly target: Expression | Block; readonly usagesPerScope: ReadonlyArray; - readonly errorsPerScope: ReadonlyArray>; + readonly functionErrorsPerScope: ReadonlyArray>; + readonly constantErrorsPerScope: ReadonlyArray>; } function collectReadsAndWrites( targetRange: TargetRange, @@ -995,14 +1187,24 @@ namespace ts.refactor.extractMethod { const allTypeParameterUsages = createMap(); // Key is type ID const usagesPerScope: ScopeUsages[] = []; const substitutionsPerScope: Map[] = []; - const errorsPerScope: Diagnostic[][] = []; + const functionErrorsPerScope: Diagnostic[][] = []; + const constantErrorsPerScope: Diagnostic[][] = []; const visibleDeclarationsInExtractedRange: Symbol[] = []; + const expressionDiagnostics = + isReadonlyArray(targetRange.range) + ? ((start, end) => [createFileDiagnostic(sourceFile, start, end - start, Messages.ExpressionExpected)])(firstOrUndefined(targetRange.range).getStart(), lastOrUndefined(targetRange.range).end) + : []; + // initialize results - for (const _ of scopes) { + for (const scope of scopes) { usagesPerScope.push({ usages: createMap(), typeParameterUsages: createMap(), substitutions: createMap() }); substitutionsPerScope.push(createMap()); - errorsPerScope.push([]); + functionErrorsPerScope.push( + isFunctionLikeDeclaration(scope) && scope.kind !== SyntaxKind.FunctionDeclaration + ? [createDiagnosticForNode(scope, Messages.CannotExtractToOtherFunctionLike)] + : []); + constantErrorsPerScope.push(expressionDiagnostics); } const seenUsages = createMap(); @@ -1054,6 +1256,13 @@ namespace ts.refactor.extractMethod { } for (let i = 0; i < scopes.length; i++) { + if (!isReadonlyArray(targetRange.range)) { + const scopeUsages = usagesPerScope[i]; + if (scopeUsages.usages.size > 0 || scopeUsages.typeParameterUsages.size > 0) { + constantErrorsPerScope[i].push(createDiagnosticForNode(targetRange.range, Messages.CannotAccessVariablesFromNestedScopes)); + } + } + let hasWrite = false; let readonlyClassPropertyWrite: Declaration | undefined = undefined; usagesPerScope[i].usages.forEach(value => { @@ -1068,10 +1277,14 @@ namespace ts.refactor.extractMethod { }); if (hasWrite && !isReadonlyArray(targetRange.range) && isExpression(targetRange.range)) { - errorsPerScope[i].push(createDiagnosticForNode(targetRange.range, Messages.CannotCombineWritesAndReturns)); + const diag = createDiagnosticForNode(targetRange.range, Messages.CannotCombineWritesAndReturns); + functionErrorsPerScope[i].push(diag); + constantErrorsPerScope[i].push(diag); } else if (readonlyClassPropertyWrite && i > 0) { - errorsPerScope[i].push(createDiagnosticForNode(readonlyClassPropertyWrite, Messages.CannotExtractReadonlyPropertyInitializerOutsideConstructor)); + const diag = createDiagnosticForNode(readonlyClassPropertyWrite, Messages.CannotExtractReadonlyPropertyInitializerOutsideConstructor); + functionErrorsPerScope[i].push(diag); + constantErrorsPerScope[i].push(diag); } } @@ -1081,7 +1294,7 @@ namespace ts.refactor.extractMethod { forEachChild(containingLexicalScopeOfExtraction, checkForUsedDeclarations); } - return { target, usagesPerScope, errorsPerScope }; + return { target, usagesPerScope, functionErrorsPerScope, constantErrorsPerScope }; function hasTypeParameters(node: Node) { return isDeclarationWithTypeParameters(node) && @@ -1211,8 +1424,12 @@ namespace ts.refactor.extractMethod { if (targetRange.facts & RangeFacts.IsGenerator && usage === Usage.Write) { // this is write to a reference located outside of the target scope and range is extracted into generator // currently this is unsupported scenario - for (const errors of errorsPerScope) { - errors.push(createDiagnosticForNode(identifier, Messages.CannotExtractRangeThatContainsWritesToReferencesLocatedOutsideOfTheTargetRangeInGenerators)); + const diag = createDiagnosticForNode(identifier, Messages.CannotExtractRangeThatContainsWritesToReferencesLocatedOutsideOfTheTargetRangeInGenerators); + for (const errors of functionErrorsPerScope) { + errors.push(diag); + } + for (const errors of constantErrorsPerScope) { + errors.push(diag); } } for (let i = 0; i < scopes.length; i++) { @@ -1230,7 +1447,9 @@ namespace ts.refactor.extractMethod { // If the symbol is a type parameter that won't be in scope, we'll pass it as a type argument // so there's no problem. if (!(symbol.flags & SymbolFlags.TypeParameter)) { - errorsPerScope[i].push(createDiagnosticForNode(identifier, Messages.TypeWillNotBeVisibleInTheNewScope)); + const diag = createDiagnosticForNode(identifier, Messages.TypeWillNotBeVisibleInTheNewScope); + functionErrorsPerScope[i].push(diag); + constantErrorsPerScope[i].push(diag); } } else { @@ -1250,8 +1469,12 @@ namespace ts.refactor.extractMethod { // Otherwise check and recurse. const sym = checker.getSymbolAtLocation(node); if (sym && visibleDeclarationsInExtractedRange.some(d => d === sym)) { - for (const scope of errorsPerScope) { - scope.push(createDiagnosticForNode(node, Messages.CannotExtractExportedEntity)); + const diag = createDiagnosticForNode(node, Messages.CannotExtractExportedEntity); + for (const errors of functionErrorsPerScope) { + errors.push(diag); + } + for (const errors of constantErrorsPerScope) { + errors.push(diag); } return true; } diff --git a/tests/baselines/reference/extractMethod/extractMethod1.ts b/tests/baselines/reference/extractMethod/extractMethod1.ts index 86c28b5f4d288..74303bbf3e189 100644 --- a/tests/baselines/reference/extractMethod/extractMethod1.ts +++ b/tests/baselines/reference/extractMethod/extractMethod1.ts @@ -14,7 +14,7 @@ namespace A { } } } -// ==SCOPE::inner function in function 'a'== +// ==SCOPE::Extract to inner function in function 'a'== namespace A { let x = 1; function foo() { @@ -34,7 +34,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'B'== +// ==SCOPE::Extract to function in namespace 'B'== namespace A { let x = 1; function foo() { @@ -55,7 +55,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'A'== +// ==SCOPE::Extract to function in namespace 'A'== namespace A { let x = 1; function foo() { @@ -76,7 +76,7 @@ namespace A { return a; } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== namespace A { let x = 1; function foo() { diff --git a/tests/baselines/reference/extractMethod/extractMethod10.ts b/tests/baselines/reference/extractMethod/extractMethod10.ts index e3eb73b661ed1..65fbc9fb4c936 100644 --- a/tests/baselines/reference/extractMethod/extractMethod10.ts +++ b/tests/baselines/reference/extractMethod/extractMethod10.ts @@ -9,22 +9,22 @@ namespace A { } } } -// ==SCOPE::method in class 'C'== +// ==SCOPE::Extract to method in class 'C'== namespace A { export interface I { x: number }; class C { a() { let z = 1; - return this./*RENAME*/newFunction(); + return this./*RENAME*/newMethod(); } - private newFunction() { + private newMethod() { let a1: I = { x: 1 }; return a1.x + 10; } } } -// ==SCOPE::function in namespace 'A'== +// ==SCOPE::Extract to function in namespace 'A'== namespace A { export interface I { x: number }; class C { @@ -39,7 +39,7 @@ namespace A { return a1.x + 10; } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== namespace A { export interface I { x: number }; class C { diff --git a/tests/baselines/reference/extractMethod/extractMethod11.ts b/tests/baselines/reference/extractMethod/extractMethod11.ts index 43fdd75b76f93..55472063055ee 100644 --- a/tests/baselines/reference/extractMethod/extractMethod11.ts +++ b/tests/baselines/reference/extractMethod/extractMethod11.ts @@ -11,18 +11,18 @@ namespace A { } } } -// ==SCOPE::method in class 'C'== +// ==SCOPE::Extract to method in class 'C'== namespace A { let y = 1; class C { a() { let z = 1; var __return: any; - ({ __return, z } = this./*RENAME*/newFunction(z)); + ({ __return, z } = this./*RENAME*/newMethod(z)); return __return; } - private newFunction(z: number) { + private newMethod(z: number) { let a1 = { x: 1 }; y = 10; z = 42; @@ -30,7 +30,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'A'== +// ==SCOPE::Extract to function in namespace 'A'== namespace A { let y = 1; class C { @@ -49,7 +49,7 @@ namespace A { return { __return: a1.x + 10, z }; } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== namespace A { let y = 1; class C { diff --git a/tests/baselines/reference/extractMethod/extractMethod12.ts b/tests/baselines/reference/extractMethod/extractMethod12.ts index 2f3082cf28008..c8f066657a218 100644 --- a/tests/baselines/reference/extractMethod/extractMethod12.ts +++ b/tests/baselines/reference/extractMethod/extractMethod12.ts @@ -13,7 +13,7 @@ namespace A { } } } -// ==SCOPE::method in class 'C'== +// ==SCOPE::Extract to method in class 'C'== namespace A { let y = 1; class C { @@ -21,11 +21,11 @@ namespace A { a() { let z = 1; var __return: any; - ({ __return, z } = this./*RENAME*/newFunction(z)); + ({ __return, z } = this./*RENAME*/newMethod(z)); return __return; } - private newFunction(z: number) { + private newMethod(z: number) { let a1 = { x: 1 }; y = 10; z = 42; diff --git a/tests/baselines/reference/extractMethod/extractMethod13.ts b/tests/baselines/reference/extractMethod/extractMethod13.ts index 121d7eeecfa7e..c40f29711f482 100644 --- a/tests/baselines/reference/extractMethod/extractMethod13.ts +++ b/tests/baselines/reference/extractMethod/extractMethod13.ts @@ -14,7 +14,7 @@ } } } -// ==SCOPE::inner function in function 'F2'== +// ==SCOPE::Extract to inner function in function 'F2'== (u1a: U1a, u1b: U1b) => { function F1(t1a: T1a, t1b: T1b) { (u2a: U2a, u2b: U2b) => { @@ -34,7 +34,7 @@ } } } -// ==SCOPE::inner function in function 'F1'== +// ==SCOPE::Extract to inner function in function 'F1'== (u1a: U1a, u1b: U1b) => { function F1(t1a: T1a, t1b: T1b) { (u2a: U2a, u2b: U2b) => { @@ -54,7 +54,7 @@ } } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== (u1a: U1a, u1b: U1b) => { function F1(t1a: T1a, t1b: T1b) { (u2a: U2a, u2b: U2b) => { diff --git a/tests/baselines/reference/extractMethod/extractMethod14.ts b/tests/baselines/reference/extractMethod/extractMethod14.ts index d3dcded2c42da..943c8250edd44 100644 --- a/tests/baselines/reference/extractMethod/extractMethod14.ts +++ b/tests/baselines/reference/extractMethod/extractMethod14.ts @@ -1,13 +1,13 @@ // ==ORIGINAL== function F(t1: T) { - function F(t2: T) { + function G(t2: T) { t1.toString(); t2.toString(); } } -// ==SCOPE::inner function in function 'F'== +// ==SCOPE::Extract to inner function in function 'G'== function F(t1: T) { - function F(t2: T) { + function G(t2: T) { /*RENAME*/newFunction(); function newFunction() { @@ -16,9 +16,9 @@ function F(t1: T) { } } } -// ==SCOPE::inner function in function 'F'== +// ==SCOPE::Extract to inner function in function 'F'== function F(t1: T) { - function F(t2: T) { + function G(t2: T) { /*RENAME*/newFunction(t2); } @@ -27,9 +27,9 @@ function F(t1: T) { t2.toString(); } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== function F(t1: T) { - function F(t2: T) { + function G(t2: T) { /*RENAME*/newFunction(t1, t2); } } diff --git a/tests/baselines/reference/extractMethod/extractMethod15.ts b/tests/baselines/reference/extractMethod/extractMethod15.ts index 50516445c8762..b6e3e0f04b6a7 100644 --- a/tests/baselines/reference/extractMethod/extractMethod15.ts +++ b/tests/baselines/reference/extractMethod/extractMethod15.ts @@ -1,12 +1,12 @@ // ==ORIGINAL== function F(t1: T) { - function F(t2: U) { + function G(t2: U) { t2.toString(); } } -// ==SCOPE::inner function in function 'F'== +// ==SCOPE::Extract to inner function in function 'G'== function F(t1: T) { - function F(t2: U) { + function G(t2: U) { /*RENAME*/newFunction(); function newFunction() { @@ -14,9 +14,9 @@ function F(t1: T) { } } } -// ==SCOPE::inner function in function 'F'== +// ==SCOPE::Extract to inner function in function 'F'== function F(t1: T) { - function F(t2: U) { + function G(t2: U) { /*RENAME*/newFunction(t2); } @@ -24,9 +24,9 @@ function F(t1: T) { t2.toString(); } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== function F(t1: T) { - function F(t2: U) { + function G(t2: U) { /*RENAME*/newFunction(t2); } } diff --git a/tests/baselines/reference/extractMethod/extractMethod16.ts b/tests/baselines/reference/extractMethod/extractMethod16.ts index e58bbf576c6f8..223307ff349e1 100644 --- a/tests/baselines/reference/extractMethod/extractMethod16.ts +++ b/tests/baselines/reference/extractMethod/extractMethod16.ts @@ -2,7 +2,7 @@ function F() { const array: T[] = []; } -// ==SCOPE::inner function in function 'F'== +// ==SCOPE::Extract to inner function in function 'F'== function F() { const array: T[] = /*RENAME*/newFunction(); @@ -10,7 +10,7 @@ function F() { return []; } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== function F() { const array: T[] = /*RENAME*/newFunction(); } diff --git a/tests/baselines/reference/extractMethod/extractMethod17.ts b/tests/baselines/reference/extractMethod/extractMethod17.ts index f79abcca7924f..e1f20af68671a 100644 --- a/tests/baselines/reference/extractMethod/extractMethod17.ts +++ b/tests/baselines/reference/extractMethod/extractMethod17.ts @@ -4,17 +4,17 @@ class C { t1.toString(); } } -// ==SCOPE::method in class 'C'== +// ==SCOPE::Extract to method in class 'C'== class C { M(t1: T1, t2: T2) { - this./*RENAME*/newFunction(t1); + this./*RENAME*/newMethod(t1); } - private newFunction(t1: T1) { + private newMethod(t1: T1) { t1.toString(); } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== class C { M(t1: T1, t2: T2) { /*RENAME*/newFunction(t1); diff --git a/tests/baselines/reference/extractMethod/extractMethod18.ts b/tests/baselines/reference/extractMethod/extractMethod18.ts index 122eced75d5f2..775a1f3ed15e9 100644 --- a/tests/baselines/reference/extractMethod/extractMethod18.ts +++ b/tests/baselines/reference/extractMethod/extractMethod18.ts @@ -4,17 +4,17 @@ class C { t1.toString(); } } -// ==SCOPE::method in class 'C'== +// ==SCOPE::Extract to method in class 'C'== class C { M(t1: T1, t2: T2) { - this./*RENAME*/newFunction(t1); + this./*RENAME*/newMethod(t1); } - private newFunction(t1: T1) { + private newMethod(t1: T1) { t1.toString(); } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== class C { M(t1: T1, t2: T2) { /*RENAME*/newFunction(t1); diff --git a/tests/baselines/reference/extractMethod/extractMethod19.ts b/tests/baselines/reference/extractMethod/extractMethod19.ts index 61d35f97db5bc..e9107198b186b 100644 --- a/tests/baselines/reference/extractMethod/extractMethod19.ts +++ b/tests/baselines/reference/extractMethod/extractMethod19.ts @@ -2,7 +2,7 @@ function F(v: V) { v.toString(); } -// ==SCOPE::inner function in function 'F'== +// ==SCOPE::Extract to inner function in function 'F'== function F(v: V) { /*RENAME*/newFunction(); @@ -10,7 +10,7 @@ function F(v: V) { v.toString(); } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== function F(v: V) { /*RENAME*/newFunction(v); } diff --git a/tests/baselines/reference/extractMethod/extractMethod2.ts b/tests/baselines/reference/extractMethod/extractMethod2.ts index b83cc6f32c6b1..201dd173adc04 100644 --- a/tests/baselines/reference/extractMethod/extractMethod2.ts +++ b/tests/baselines/reference/extractMethod/extractMethod2.ts @@ -12,7 +12,7 @@ namespace A { } } } -// ==SCOPE::inner function in function 'a'== +// ==SCOPE::Extract to inner function in function 'a'== namespace A { let x = 1; function foo() { @@ -30,7 +30,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'B'== +// ==SCOPE::Extract to function in namespace 'B'== namespace A { let x = 1; function foo() { @@ -48,7 +48,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'A'== +// ==SCOPE::Extract to function in namespace 'A'== namespace A { let x = 1; function foo() { @@ -66,7 +66,7 @@ namespace A { return foo(); } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== namespace A { let x = 1; function foo() { diff --git a/tests/baselines/reference/extractMethod/extractMethod20.ts b/tests/baselines/reference/extractMethod/extractMethod20.ts index 6e0148e099197..723d8a99ca69e 100644 --- a/tests/baselines/reference/extractMethod/extractMethod20.ts +++ b/tests/baselines/reference/extractMethod/extractMethod20.ts @@ -5,18 +5,18 @@ const _ = class { return a1.x + 10; } } -// ==SCOPE::method in anonymous class expression== +// ==SCOPE::Extract to method in anonymous class expression== const _ = class { a() { - return this./*RENAME*/newFunction(); + return this./*RENAME*/newMethod(); } - private newFunction() { + private newMethod() { let a1 = { x: 1 }; return a1.x + 10; } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== const _ = class { a() { return /*RENAME*/newFunction(); diff --git a/tests/baselines/reference/extractMethod/extractMethod21.ts b/tests/baselines/reference/extractMethod/extractMethod21.ts index 2c4ffd1bdb831..b0469d81384e7 100644 --- a/tests/baselines/reference/extractMethod/extractMethod21.ts +++ b/tests/baselines/reference/extractMethod/extractMethod21.ts @@ -4,7 +4,7 @@ function foo() { x++; return; } -// ==SCOPE::inner function in function 'foo'== +// ==SCOPE::Extract to inner function in function 'foo'== function foo() { let x = 10; return /*RENAME*/newFunction(); @@ -14,7 +14,7 @@ function foo() { return; } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== function foo() { let x = 10; x = /*RENAME*/newFunction(x); diff --git a/tests/baselines/reference/extractMethod/extractMethod22.ts b/tests/baselines/reference/extractMethod/extractMethod22.ts index 990bfdf0575b9..09fbc7a5b82d0 100644 --- a/tests/baselines/reference/extractMethod/extractMethod22.ts +++ b/tests/baselines/reference/extractMethod/extractMethod22.ts @@ -6,7 +6,7 @@ function test() { return 1; } } -// ==SCOPE::inner function in function 'test'== +// ==SCOPE::Extract to inner function in function 'test'== function test() { try { } @@ -18,7 +18,7 @@ function test() { return 1; } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== function test() { try { } diff --git a/tests/baselines/reference/extractMethod/extractMethod23.ts b/tests/baselines/reference/extractMethod/extractMethod23.ts index b9bc4264ea625..1a0eaf098fd99 100644 --- a/tests/baselines/reference/extractMethod/extractMethod23.ts +++ b/tests/baselines/reference/extractMethod/extractMethod23.ts @@ -6,7 +6,7 @@ namespace NS { } function M3() { } } -// ==SCOPE::inner function in function 'M2'== +// ==SCOPE::Extract to inner function in function 'M2'== namespace NS { function M1() { } function M2() { @@ -18,7 +18,7 @@ namespace NS { } function M3() { } } -// ==SCOPE::function in namespace 'NS'== +// ==SCOPE::Extract to function in namespace 'NS'== namespace NS { function M1() { } function M2() { @@ -30,7 +30,7 @@ namespace NS { function M3() { } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== namespace NS { function M1() { } function M2() { diff --git a/tests/baselines/reference/extractMethod/extractMethod24.ts b/tests/baselines/reference/extractMethod/extractMethod24.ts index a9dc25d32ea89..7b80180c5d3cb 100644 --- a/tests/baselines/reference/extractMethod/extractMethod24.ts +++ b/tests/baselines/reference/extractMethod/extractMethod24.ts @@ -6,7 +6,7 @@ function Outer() { } function M3() { } } -// ==SCOPE::inner function in function 'M2'== +// ==SCOPE::Extract to inner function in function 'M2'== function Outer() { function M1() { } function M2() { @@ -18,7 +18,7 @@ function Outer() { } function M3() { } } -// ==SCOPE::inner function in function 'Outer'== +// ==SCOPE::Extract to inner function in function 'Outer'== function Outer() { function M1() { } function M2() { @@ -30,7 +30,7 @@ function Outer() { function M3() { } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== function Outer() { function M1() { } function M2() { diff --git a/tests/baselines/reference/extractMethod/extractMethod25.ts b/tests/baselines/reference/extractMethod/extractMethod25.ts index dc376781346c3..3233c4043077d 100644 --- a/tests/baselines/reference/extractMethod/extractMethod25.ts +++ b/tests/baselines/reference/extractMethod/extractMethod25.ts @@ -4,7 +4,7 @@ function M2() { return 1; } function M3() { } -// ==SCOPE::inner function in function 'M2'== +// ==SCOPE::Extract to inner function in function 'M2'== function M1() { } function M2() { return /*RENAME*/newFunction(); @@ -14,7 +14,7 @@ function M2() { } } function M3() { } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== function M1() { } function M2() { return /*RENAME*/newFunction(); diff --git a/tests/baselines/reference/extractMethod/extractMethod26.ts b/tests/baselines/reference/extractMethod/extractMethod26.ts index b49e8ab950838..6f4d85c78018c 100644 --- a/tests/baselines/reference/extractMethod/extractMethod26.ts +++ b/tests/baselines/reference/extractMethod/extractMethod26.ts @@ -6,19 +6,19 @@ class C { } M3() { } } -// ==SCOPE::method in class 'C'== +// ==SCOPE::Extract to method in class 'C'== class C { M1() { } M2() { - return this./*RENAME*/newFunction(); + return this./*RENAME*/newMethod(); } - private newFunction() { + private newMethod() { return 1; } M3() { } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== class C { M1() { } M2() { diff --git a/tests/baselines/reference/extractMethod/extractMethod27.ts b/tests/baselines/reference/extractMethod/extractMethod27.ts index 0ec214bb5ed18..3426c4ec5c306 100644 --- a/tests/baselines/reference/extractMethod/extractMethod27.ts +++ b/tests/baselines/reference/extractMethod/extractMethod27.ts @@ -7,20 +7,20 @@ class C { constructor() { } M3() { } } -// ==SCOPE::method in class 'C'== +// ==SCOPE::Extract to method in class 'C'== class C { M1() { } M2() { - return this./*RENAME*/newFunction(); + return this./*RENAME*/newMethod(); } constructor() { } - private newFunction() { + private newMethod() { return 1; } M3() { } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== class C { M1() { } M2() { diff --git a/tests/baselines/reference/extractMethod/extractMethod28.ts b/tests/baselines/reference/extractMethod/extractMethod28.ts index ab6ce220bd76a..b6e94cb47c590 100644 --- a/tests/baselines/reference/extractMethod/extractMethod28.ts +++ b/tests/baselines/reference/extractMethod/extractMethod28.ts @@ -7,20 +7,20 @@ class C { M3() { } constructor() { } } -// ==SCOPE::method in class 'C'== +// ==SCOPE::Extract to method in class 'C'== class C { M1() { } M2() { - return this./*RENAME*/newFunction(); + return this./*RENAME*/newMethod(); } - private newFunction() { + private newMethod() { return 1; } M3() { } constructor() { } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== class C { M1() { } M2() { diff --git a/tests/baselines/reference/extractMethod/extractMethod29.ts b/tests/baselines/reference/extractMethod/extractMethod29.ts index aa7004d254e87..9baea5870f520 100644 --- a/tests/baselines/reference/extractMethod/extractMethod29.ts +++ b/tests/baselines/reference/extractMethod/extractMethod29.ts @@ -16,7 +16,7 @@ function parseUnaryExpression(operator: string): UnaryExpression { function parsePrimaryExpression(): any { throw "Not implemented"; } -// ==SCOPE::inner function in function 'parseUnaryExpression'== +// ==SCOPE::Extract to inner function in function 'parseUnaryExpression'== interface UnaryExpression { kind: "Unary"; operator: string; @@ -38,7 +38,7 @@ function parseUnaryExpression(operator: string): UnaryExpression { function parsePrimaryExpression(): any { throw "Not implemented"; } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== interface UnaryExpression { kind: "Unary"; operator: string; diff --git a/tests/baselines/reference/extractMethod/extractMethod3.ts b/tests/baselines/reference/extractMethod/extractMethod3.ts index 0af79791fe7cb..b5ae759215794 100644 --- a/tests/baselines/reference/extractMethod/extractMethod3.ts +++ b/tests/baselines/reference/extractMethod/extractMethod3.ts @@ -11,7 +11,7 @@ namespace A { } } } -// ==SCOPE::inner function in function 'a'== +// ==SCOPE::Extract to inner function in function 'a'== namespace A { function foo() { } @@ -28,7 +28,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'B'== +// ==SCOPE::Extract to function in namespace 'B'== namespace A { function foo() { } @@ -45,7 +45,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'A'== +// ==SCOPE::Extract to function in namespace 'A'== namespace A { function foo() { } @@ -62,7 +62,7 @@ namespace A { return foo(); } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== namespace A { function foo() { } diff --git a/tests/baselines/reference/extractMethod/extractMethod30.ts b/tests/baselines/reference/extractMethod/extractMethod30.ts index 67dc1208abcfe..35b86b668ff1b 100644 --- a/tests/baselines/reference/extractMethod/extractMethod30.ts +++ b/tests/baselines/reference/extractMethod/extractMethod30.ts @@ -2,7 +2,7 @@ function F() { let t: T; } -// ==SCOPE::inner function in function 'F'== +// ==SCOPE::Extract to inner function in function 'F'== function F() { /*RENAME*/newFunction(); @@ -10,7 +10,7 @@ function F() { let t: T; } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== function F() { /*RENAME*/newFunction(); } diff --git a/tests/baselines/reference/extractMethod/extractMethod31.ts b/tests/baselines/reference/extractMethod/extractMethod31.ts index 754814dcc266d..7712009182841 100644 --- a/tests/baselines/reference/extractMethod/extractMethod31.ts +++ b/tests/baselines/reference/extractMethod/extractMethod31.ts @@ -10,7 +10,7 @@ namespace N { } } } -// ==SCOPE::function in namespace 'N'== +// ==SCOPE::Extract to function in namespace 'N'== namespace N { export const value = 1; @@ -27,7 +27,7 @@ namespace N { return f; } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== namespace N { export const value = 1; diff --git a/tests/baselines/reference/extractMethod/extractMethod32.ts b/tests/baselines/reference/extractMethod/extractMethod32.ts index b9b870a08fa7a..529b81e8f9b2b 100644 --- a/tests/baselines/reference/extractMethod/extractMethod32.ts +++ b/tests/baselines/reference/extractMethod/extractMethod32.ts @@ -11,7 +11,7 @@ namespace N { } } } -// ==SCOPE::function in namespace 'N'== +// ==SCOPE::Extract to function in namespace 'N'== namespace N { export const value = 1; @@ -28,7 +28,7 @@ namespace N { }; } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== namespace N { export const value = 1; diff --git a/tests/baselines/reference/extractMethod/extractMethod33.ts b/tests/baselines/reference/extractMethod/extractMethod33.ts index 79a6626a535c2..fd716646e6610 100644 --- a/tests/baselines/reference/extractMethod/extractMethod33.ts +++ b/tests/baselines/reference/extractMethod/extractMethod33.ts @@ -2,7 +2,7 @@ function F() { function G() { } } -// ==SCOPE::inner function in function 'F'== +// ==SCOPE::Extract to inner function in function 'F'== function F() { /*RENAME*/newFunction(); @@ -10,7 +10,7 @@ function F() { function G() { } } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== function F() { /*RENAME*/newFunction(); } diff --git a/tests/baselines/reference/extractMethod/extractMethod4.ts b/tests/baselines/reference/extractMethod/extractMethod4.ts index e0a636135d45d..6ef3e843ee128 100644 --- a/tests/baselines/reference/extractMethod/extractMethod4.ts +++ b/tests/baselines/reference/extractMethod/extractMethod4.ts @@ -13,7 +13,7 @@ namespace A { } } } -// ==SCOPE::inner function in function 'a'== +// ==SCOPE::Extract to inner function in function 'a'== namespace A { function foo() { } @@ -32,7 +32,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'B'== +// ==SCOPE::Extract to function in namespace 'B'== namespace A { function foo() { } @@ -51,7 +51,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'A'== +// ==SCOPE::Extract to function in namespace 'A'== namespace A { function foo() { } @@ -70,7 +70,7 @@ namespace A { return foo(); } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== namespace A { function foo() { } diff --git a/tests/baselines/reference/extractMethod/extractMethod5.ts b/tests/baselines/reference/extractMethod/extractMethod5.ts index fc15f6762e568..e2f72721b950d 100644 --- a/tests/baselines/reference/extractMethod/extractMethod5.ts +++ b/tests/baselines/reference/extractMethod/extractMethod5.ts @@ -14,7 +14,7 @@ namespace A { } } } -// ==SCOPE::inner function in function 'a'== +// ==SCOPE::Extract to inner function in function 'a'== namespace A { let x = 1; export function foo() { @@ -34,7 +34,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'B'== +// ==SCOPE::Extract to function in namespace 'B'== namespace A { let x = 1; export function foo() { @@ -55,7 +55,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'A'== +// ==SCOPE::Extract to function in namespace 'A'== namespace A { let x = 1; export function foo() { @@ -76,7 +76,7 @@ namespace A { return a; } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== namespace A { let x = 1; export function foo() { diff --git a/tests/baselines/reference/extractMethod/extractMethod6.ts b/tests/baselines/reference/extractMethod/extractMethod6.ts index 37112b80e9e13..6b4852dc722a6 100644 --- a/tests/baselines/reference/extractMethod/extractMethod6.ts +++ b/tests/baselines/reference/extractMethod/extractMethod6.ts @@ -14,7 +14,7 @@ namespace A { } } } -// ==SCOPE::inner function in function 'a'== +// ==SCOPE::Extract to inner function in function 'a'== namespace A { let x = 1; export function foo() { @@ -34,7 +34,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'B'== +// ==SCOPE::Extract to function in namespace 'B'== namespace A { let x = 1; export function foo() { @@ -56,7 +56,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'A'== +// ==SCOPE::Extract to function in namespace 'A'== namespace A { let x = 1; export function foo() { @@ -78,7 +78,7 @@ namespace A { return { __return: foo(), a }; } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== namespace A { let x = 1; export function foo() { diff --git a/tests/baselines/reference/extractMethod/extractMethod7.ts b/tests/baselines/reference/extractMethod/extractMethod7.ts index 8859b7b4fdd1d..9712cedda8d5d 100644 --- a/tests/baselines/reference/extractMethod/extractMethod7.ts +++ b/tests/baselines/reference/extractMethod/extractMethod7.ts @@ -16,7 +16,7 @@ namespace A { } } } -// ==SCOPE::inner function in function 'a'== +// ==SCOPE::Extract to inner function in function 'a'== namespace A { let x = 1; export namespace C { @@ -38,7 +38,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'B'== +// ==SCOPE::Extract to function in namespace 'B'== namespace A { let x = 1; export namespace C { @@ -62,7 +62,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'A'== +// ==SCOPE::Extract to function in namespace 'A'== namespace A { let x = 1; export namespace C { @@ -86,7 +86,7 @@ namespace A { return { __return: C.foo(), a }; } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== namespace A { let x = 1; export namespace C { diff --git a/tests/baselines/reference/extractMethod/extractMethod8.ts b/tests/baselines/reference/extractMethod/extractMethod8.ts index cb06470d3855f..adb8adbe56ab0 100644 --- a/tests/baselines/reference/extractMethod/extractMethod8.ts +++ b/tests/baselines/reference/extractMethod/extractMethod8.ts @@ -8,7 +8,7 @@ namespace A { } } } -// ==SCOPE::inner function in function 'a'== +// ==SCOPE::Extract to inner function in function 'a'== namespace A { let x = 1; namespace B { @@ -22,7 +22,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'B'== +// ==SCOPE::Extract to function in namespace 'B'== namespace A { let x = 1; namespace B { @@ -36,7 +36,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'A'== +// ==SCOPE::Extract to function in namespace 'A'== namespace A { let x = 1; namespace B { @@ -50,7 +50,7 @@ namespace A { return 1 + a1 + x; } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== namespace A { let x = 1; namespace B { diff --git a/tests/baselines/reference/extractMethod/extractMethod9.ts b/tests/baselines/reference/extractMethod/extractMethod9.ts index 022dab8236397..9c1f81edf452d 100644 --- a/tests/baselines/reference/extractMethod/extractMethod9.ts +++ b/tests/baselines/reference/extractMethod/extractMethod9.ts @@ -8,7 +8,7 @@ namespace A { } } } -// ==SCOPE::inner function in function 'a'== +// ==SCOPE::Extract to inner function in function 'a'== namespace A { export interface I { x: number }; namespace B { @@ -22,7 +22,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'B'== +// ==SCOPE::Extract to function in namespace 'B'== namespace A { export interface I { x: number }; namespace B { @@ -36,7 +36,7 @@ namespace A { } } } -// ==SCOPE::function in namespace 'A'== +// ==SCOPE::Extract to function in namespace 'A'== namespace A { export interface I { x: number }; namespace B { @@ -50,7 +50,7 @@ namespace A { return a1.x + 10; } } -// ==SCOPE::function in global scope== +// ==SCOPE::Extract to function in global scope== namespace A { export interface I { x: number }; namespace B { diff --git a/tests/cases/fourslash/extract-method-empty-namespace.ts b/tests/cases/fourslash/extract-method-empty-namespace.ts index 3a29992350df5..bef4cdd12fe4e 100644 --- a/tests/cases/fourslash/extract-method-empty-namespace.ts +++ b/tests/cases/fourslash/extract-method-empty-namespace.ts @@ -6,8 +6,8 @@ goTo.select('start', 'end') edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_1", + refactorName: "Extract Symbol", + actionName: "function_scope_1", actionDescription: "Extract to function in global scope", newContent: `function f() { /*RENAME*/newFunction(); diff --git a/tests/cases/fourslash/extract-method-formatting.ts b/tests/cases/fourslash/extract-method-formatting.ts index e4193fd8db390..d4c2836e815e6 100644 --- a/tests/cases/fourslash/extract-method-formatting.ts +++ b/tests/cases/fourslash/extract-method-formatting.ts @@ -7,8 +7,8 @@ goTo.select('start', 'end') edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_1", + refactorName: "Extract Symbol", + actionName: "function_scope_1", actionDescription: "Extract to function in global scope", newContent: `function f(x: number): number { return /*RENAME*/newFunction(x); diff --git a/tests/cases/fourslash/extract-method-not-for-empty.ts b/tests/cases/fourslash/extract-method-not-for-empty.ts index 756716441cd6c..253614d316065 100644 --- a/tests/cases/fourslash/extract-method-not-for-empty.ts +++ b/tests/cases/fourslash/extract-method-not-for-empty.ts @@ -3,4 +3,4 @@ ////"/**/foo"; goTo.marker(""); -verify.not.refactorAvailable('Extract Method'); +verify.not.refactorAvailable('Extract Symbol'); diff --git a/tests/cases/fourslash/extract-method-not-for-import.ts b/tests/cases/fourslash/extract-method-not-for-import.ts index a72d793611d62..2852d856264d8 100644 --- a/tests/cases/fourslash/extract-method-not-for-import.ts +++ b/tests/cases/fourslash/extract-method-not-for-import.ts @@ -7,4 +7,4 @@ ////export default function f() {} goTo.marker(""); -verify.not.refactorAvailable('Extract Method'); +verify.not.refactorAvailable('Extract Symbol'); diff --git a/tests/cases/fourslash/extract-method-uniqueName.ts b/tests/cases/fourslash/extract-method-uniqueName.ts index 8f024ad6a4799..4271d9e84d2f2 100644 --- a/tests/cases/fourslash/extract-method-uniqueName.ts +++ b/tests/cases/fourslash/extract-method-uniqueName.ts @@ -7,8 +7,8 @@ // it's omitted right now. goTo.select('start', 'end') edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_0", + refactorName: "Extract Symbol", + actionName: "function_scope_0", actionDescription: "Extract to function in global scope", newContent: `/*RENAME*/newFunction_1(); diff --git a/tests/cases/fourslash/extract-method1.ts b/tests/cases/fourslash/extract-method1.ts index a8d421923b14e..f4693877ed8b9 100644 --- a/tests/cases/fourslash/extract-method1.ts +++ b/tests/cases/fourslash/extract-method1.ts @@ -14,18 +14,18 @@ goTo.select('start', 'end') edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_0", + refactorName: "Extract Symbol", + actionName: "function_scope_1", actionDescription: "Extract to method in class 'Foo'", newContent: `class Foo { someMethod(m: number) { - this./*RENAME*/newFunction(m); + this./*RENAME*/newMethod(m); var q = 10; return q; } - private newFunction(m: number) { + private newMethod(m: number) { var x = m; x = x * 3; var y = 30; diff --git a/tests/cases/fourslash/extract-method10.ts b/tests/cases/fourslash/extract-method10.ts index 215196d6c51dc..ed92e6b06ac8d 100644 --- a/tests/cases/fourslash/extract-method10.ts +++ b/tests/cases/fourslash/extract-method10.ts @@ -5,8 +5,8 @@ goTo.select('1', '2'); edit.applyRefactor({ - refactorName: "Extract Method", - actionName: 'scope_0', + refactorName: "Extract Symbol", + actionName: 'function_scope_0', actionDescription: "Extract to function in module scope", newContent: `export {}; // Make this a module diff --git a/tests/cases/fourslash/extract-method11.ts b/tests/cases/fourslash/extract-method11.ts index 705f2373104de..a7d90f04f42eb 100644 --- a/tests/cases/fourslash/extract-method11.ts +++ b/tests/cases/fourslash/extract-method11.ts @@ -20,9 +20,9 @@ for (const m of ['1', '2', '3', '4', '5']) { goTo.select(m + 'a', m + 'b'); - verify.not.refactorAvailable('Extract Method'); + verify.not.refactorAvailable('Extract Symbol'); } // Verify we can still extract the entire class goTo.select('oka', 'okb'); -verify.refactorAvailable('Extract Method'); +verify.refactorAvailable('Extract Symbol', 'function_scope_0'); diff --git a/tests/cases/fourslash/extract-method13.ts b/tests/cases/fourslash/extract-method13.ts index 409b5f890ad9b..fc0b22f3d48e8 100644 --- a/tests/cases/fourslash/extract-method13.ts +++ b/tests/cases/fourslash/extract-method13.ts @@ -11,16 +11,16 @@ goTo.select('a', 'b'); edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_0", + refactorName: "Extract Symbol", + actionName: "function_scope_0", actionDescription: "Extract to method in class 'C'", newContent: `class C { static j = 1 + 1; - constructor(q: string = C./*RENAME*/newFunction()) { + constructor(q: string = C./*RENAME*/newMethod()) { } - private static newFunction(): string { + private static newMethod(): string { return "hello"; } }` @@ -28,30 +28,30 @@ edit.applyRefactor({ verify.currentFileContentIs(`class C { static j = 1 + 1; - constructor(q: string = C.newFunction()) { + constructor(q: string = C.newMethod()) { } - private static newFunction(): string { + private static newMethod(): string { return "hello"; } }`); goTo.select('c', 'd'); edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_0", + refactorName: "Extract Symbol", + actionName: "function_scope_0", actionDescription: "Extract to method in class 'C'", newContent: `class C { - static j = C./*RENAME*/newFunction_1(); - constructor(q: string = C.newFunction()) { + static j = C./*RENAME*/newMethod_1(); + constructor(q: string = C.newMethod()) { } - private static newFunction_1() { + private static newMethod_1() { return 1 + 1; } - private static newFunction(): string { + private static newMethod(): string { return "hello"; } }` diff --git a/tests/cases/fourslash/extract-method14.ts b/tests/cases/fourslash/extract-method14.ts index e2b58a36450c8..ea051aabbe75b 100644 --- a/tests/cases/fourslash/extract-method14.ts +++ b/tests/cases/fourslash/extract-method14.ts @@ -12,8 +12,8 @@ goTo.select('a', 'b'); edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_1", + refactorName: "Extract Symbol", + actionName: "function_scope_1", actionDescription: "Extract to function in global scope", newContent: `function foo() { diff --git a/tests/cases/fourslash/extract-method15.ts b/tests/cases/fourslash/extract-method15.ts index c3db3186cdf9d..e46ff39ad6a49 100644 --- a/tests/cases/fourslash/extract-method15.ts +++ b/tests/cases/fourslash/extract-method15.ts @@ -10,8 +10,8 @@ goTo.select('a', 'b'); edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_1", + refactorName: "Extract Symbol", + actionName: "function_scope_1", actionDescription: "Extract to function in global scope", newContent: `function foo() { diff --git a/tests/cases/fourslash/extract-method17.ts b/tests/cases/fourslash/extract-method17.ts index ce54896604dd9..467ff91ebb282 100644 --- a/tests/cases/fourslash/extract-method17.ts +++ b/tests/cases/fourslash/extract-method17.ts @@ -6,5 +6,5 @@ //// } goTo.select('start', 'end') -verify.refactorAvailable('Extract Method', 'scope_0'); -verify.not.refactorAvailable('Extract Method', 'scope_1'); +verify.refactorAvailable('Extract Symbol', 'function_scope_0'); +verify.not.refactorAvailable('Extract Symbol', 'function_scope_1'); diff --git a/tests/cases/fourslash/extract-method18.ts b/tests/cases/fourslash/extract-method18.ts index 8ff1cc3028df8..d53e79f493000 100644 --- a/tests/cases/fourslash/extract-method18.ts +++ b/tests/cases/fourslash/extract-method18.ts @@ -10,8 +10,8 @@ goTo.select('a', 'b') edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_1", + refactorName: "Extract Symbol", + actionName: "function_scope_1", actionDescription: "Extract to function in global scope", newContent: `function fn() { diff --git a/tests/cases/fourslash/extract-method19.ts b/tests/cases/fourslash/extract-method19.ts index 56d6b02560fdd..87534a17d4a4a 100644 --- a/tests/cases/fourslash/extract-method19.ts +++ b/tests/cases/fourslash/extract-method19.ts @@ -10,8 +10,8 @@ goTo.select('a', 'b') edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_0", + refactorName: "Extract Symbol", + actionName: "function_scope_0", actionDescription: "Extract to inner function in function 'fn'", newContent: `function fn() { diff --git a/tests/cases/fourslash/extract-method2.ts b/tests/cases/fourslash/extract-method2.ts index 6fbe7394c2f84..7f197d4775b0d 100644 --- a/tests/cases/fourslash/extract-method2.ts +++ b/tests/cases/fourslash/extract-method2.ts @@ -11,8 +11,8 @@ //// } goTo.select('start', 'end') edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_2", + refactorName: "Extract Symbol", + actionName: "function_scope_3", actionDescription: "Extract to function in global scope", newContent: `namespace NS { diff --git a/tests/cases/fourslash/extract-method20.ts b/tests/cases/fourslash/extract-method20.ts index 04990001b5f0e..75927f0fd5c9e 100644 --- a/tests/cases/fourslash/extract-method20.ts +++ b/tests/cases/fourslash/extract-method20.ts @@ -10,5 +10,5 @@ //// } goTo.select('a', 'b') -verify.refactorAvailable('Extract Method', 'scope_0'); -verify.not.refactorAvailable('Extract Method', 'scope_1'); +verify.refactorAvailable('Extract Symbol', 'function_scope_0'); +verify.not.refactorAvailable('Extract Symbol', 'function_scope_1'); diff --git a/tests/cases/fourslash/extract-method21.ts b/tests/cases/fourslash/extract-method21.ts index 8e3baf619496f..cbb1f55e071f0 100644 --- a/tests/cases/fourslash/extract-method21.ts +++ b/tests/cases/fourslash/extract-method21.ts @@ -10,19 +10,19 @@ goTo.select('start', 'end') -verify.refactorAvailable('Extract Method'); +verify.refactorAvailable('Extract Symbol', 'function_scope_1'); edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_0", + refactorName: "Extract Symbol", + actionName: "function_scope_1", actionDescription: "Extract to method in class 'Foo'", newContent: `class Foo { static method() { - return Foo./*RENAME*/newFunction(); + return Foo./*RENAME*/newMethod(); } - private static newFunction() { + private static newMethod() { return 1; } }` diff --git a/tests/cases/fourslash/extract-method22.ts b/tests/cases/fourslash/extract-method22.ts index 7486520e700f7..85f88f3952cf8 100644 --- a/tests/cases/fourslash/extract-method22.ts +++ b/tests/cases/fourslash/extract-method22.ts @@ -7,4 +7,4 @@ //// } goTo.select('start', 'end') -verify.not.refactorAvailable('Extract Method'); +verify.not.refactorAvailable('Extract Symbol'); diff --git a/tests/cases/fourslash/extract-method23.ts b/tests/cases/fourslash/extract-method23.ts index 7da8f175f1353..ad5248a37e295 100644 --- a/tests/cases/fourslash/extract-method23.ts +++ b/tests/cases/fourslash/extract-method23.ts @@ -5,4 +5,4 @@ //// } goTo.select('start', 'end') -verify.not.refactorAvailable('Extract Method'); +verify.not.refactorAvailable('Extract Symbol'); diff --git a/tests/cases/fourslash/extract-method24.ts b/tests/cases/fourslash/extract-method24.ts index 5706c5c7a5453..8c750edd9f906 100644 --- a/tests/cases/fourslash/extract-method24.ts +++ b/tests/cases/fourslash/extract-method24.ts @@ -8,8 +8,8 @@ goTo.select('a', 'b') edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_1", + refactorName: "Extract Symbol", + actionName: "function_scope_1", actionDescription: "Extract to function in global scope", newContent: `function M() { diff --git a/tests/cases/fourslash/extract-method25.ts b/tests/cases/fourslash/extract-method25.ts index 4fb2193adf3bf..a06262ca47510 100644 --- a/tests/cases/fourslash/extract-method25.ts +++ b/tests/cases/fourslash/extract-method25.ts @@ -9,8 +9,8 @@ goTo.select('a', 'b') edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_0", + refactorName: "Extract Symbol", + actionName: "function_scope_0", actionDescription: "Extract to inner function in function 'fn'", newContent: `function fn() { diff --git a/tests/cases/fourslash/extract-method26.ts b/tests/cases/fourslash/extract-method26.ts index 68982eda86cd8..2915118fd8e18 100644 --- a/tests/cases/fourslash/extract-method26.ts +++ b/tests/cases/fourslash/extract-method26.ts @@ -13,17 +13,17 @@ goTo.select('a', 'b') edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_0", + refactorName: "Extract Symbol", + actionName: "function_scope_1", actionDescription: "Extract to method in class 'C'", newContent: `class C { M() { - const q = this./*RENAME*/newFunction(); + const q = this./*RENAME*/newMethod(); q.toString(); } - newFunction() { + newMethod() { return 1 + 2; } }` diff --git a/tests/cases/fourslash/extract-method3.ts b/tests/cases/fourslash/extract-method3.ts index 27a520d0555a4..43402a7ac209f 100644 --- a/tests/cases/fourslash/extract-method3.ts +++ b/tests/cases/fourslash/extract-method3.ts @@ -10,9 +10,9 @@ //// } //// } -// Don't offer to 'extract method' a single identifier +// Don't offer to 'extract symbol' a single identifier goTo.marker('a'); -verify.not.refactorAvailable('Extract Method'); +verify.not.refactorAvailable('Extract Symbol'); goTo.select('a', 'b'); -verify.not.refactorAvailable('Extract Method'); +verify.not.refactorAvailable('Extract Symbol'); diff --git a/tests/cases/fourslash/extract-method4.ts b/tests/cases/fourslash/extract-method4.ts index ec8f39f354189..93eabbc0ae5d6 100644 --- a/tests/cases/fourslash/extract-method4.ts +++ b/tests/cases/fourslash/extract-method4.ts @@ -11,4 +11,4 @@ // Should rewrite to a = newFunc(); function() { return b = c = d; } goTo.select('1', '2'); -verify.not.refactorAvailable('Extract Method'); +verify.not.refactorAvailable('Extract Symbol'); diff --git a/tests/cases/fourslash/extract-method5.ts b/tests/cases/fourslash/extract-method5.ts index b27d9a8209bda..ce164d43c6e3f 100644 --- a/tests/cases/fourslash/extract-method5.ts +++ b/tests/cases/fourslash/extract-method5.ts @@ -10,8 +10,8 @@ goTo.select('start', 'end'); edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_0", + refactorName: "Extract Symbol", + actionName: "function_scope_0", actionDescription: "Extract to inner function in function 'f'", newContent: `function f() { diff --git a/tests/cases/fourslash/extract-method6.ts b/tests/cases/fourslash/extract-method6.ts index f188ab319e9b8..be21505ac1341 100644 --- a/tests/cases/fourslash/extract-method6.ts +++ b/tests/cases/fourslash/extract-method6.ts @@ -10,7 +10,7 @@ //// }/*f1b*/ goTo.select('f1a', 'f1b'); -verify.not.refactorAvailable('Extract Method'); +verify.not.refactorAvailable('Extract Symbol'); goTo.select('g1a', 'g1b'); -verify.not.refactorAvailable('Extract Method'); +verify.not.refactorAvailable('Extract Symbol'); diff --git a/tests/cases/fourslash/extract-method7.ts b/tests/cases/fourslash/extract-method7.ts index 0ce39c5a3095c..2998b6bbee31c 100644 --- a/tests/cases/fourslash/extract-method7.ts +++ b/tests/cases/fourslash/extract-method7.ts @@ -1,15 +1,15 @@ /// // You cannot extract a function initializer into the function's body. -// The innermost scope (scope_0) is the sibling of the function, not the function itself. +// The innermost scope (function_scope_0) is the sibling of the function, not the function itself. //// function fn(x = /*a*/3/*b*/) { //// } goTo.select('a', 'b'); edit.applyRefactor({ - refactorName: "Extract Method", - actionName: "scope_0", + refactorName: "Extract Symbol", + actionName: "function_scope_0", actionDescription: "Extract to function in global scope", newContent: `function fn(x = /*RENAME*/newFunction()) { diff --git a/tests/cases/fourslash/extract-method8.ts b/tests/cases/fourslash/extract-method8.ts index 28068dd7c783b..12d1dab282861 100644 --- a/tests/cases/fourslash/extract-method8.ts +++ b/tests/cases/fourslash/extract-method8.ts @@ -4,14 +4,14 @@ //// namespace ns { //// /*a*/export function fn() { -//// +//// //// } //// fn(); //// /*b*/ //// } goTo.select('a', 'b'); -verify.not.refactorAvailable("Extract Method"); +verify.not.refactorAvailable("Extract Symbol"); edit.deleteAtCaret('export'.length); goTo.select('a', 'b'); -verify.refactorAvailable("Extract Method"); +verify.refactorAvailable("Extract Symbol", 'function_scope_0'); diff --git a/tests/cases/fourslash/extract-method9.ts b/tests/cases/fourslash/extract-method9.ts index f70ef20a87b56..41af6cbf5f45c 100644 --- a/tests/cases/fourslash/extract-method9.ts +++ b/tests/cases/fourslash/extract-method9.ts @@ -1,11 +1,11 @@ /// //// function f() { -//// /*a*/function q() { } +//// /*a*/function q() { } //// q();/*b*/ //// q(); //// } goTo.select('a', 'b'); -verify.not.refactorAvailable("Extract Method"); +verify.not.refactorAvailable("Extract Symbol"); From eb1fb5c1643a830418805f5ba812ec7faa445e99 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Mon, 25 Sep 2017 18:42:09 -0700 Subject: [PATCH 2/9] Rename extractMethod.ts to extractSymbol.ts --- src/services/refactors/{extractMethod.ts => extractSymbol.ts} | 0 src/services/refactors/refactors.ts | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename src/services/refactors/{extractMethod.ts => extractSymbol.ts} (100%) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractSymbol.ts similarity index 100% rename from src/services/refactors/extractMethod.ts rename to src/services/refactors/extractSymbol.ts diff --git a/src/services/refactors/refactors.ts b/src/services/refactors/refactors.ts index 3a33ccc83c2b8..680b7f8b02fbe 100644 --- a/src/services/refactors/refactors.ts +++ b/src/services/refactors/refactors.ts @@ -1,2 +1,2 @@ /// -/// +/// From 2601bbcea721a0d759ecfd929cf13872c9fb2058 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 26 Sep 2017 10:37:25 -0700 Subject: [PATCH 3/9] Add simple tests for Extract Constant --- Jakefile.js | 1 + src/harness/tsconfig.json | 1 + src/harness/unittests/extractConstants.ts | 250 ++++++++++++++++++ src/harness/unittests/extractMethods.ts | 3 +- src/services/refactors/extractSymbol.ts | 28 +- ...ractConstant_BlockScopes_NoDependencies.ts | 14 + .../extractConstant/extractConstant_Class.ts | 16 ++ .../extractConstant_ClassInsertionPosition.ts | 46 ++++ .../extractConstant_ExpressionStatement.ts | 4 + ...tConstant_ExpressionStatementExpression.ts | 4 + .../extractConstant_Function.ts | 16 ++ .../extractConstant/extractConstant_Method.ts | 30 +++ .../extractConstant_Namespace.ts | 16 ++ .../extractConstant_TopLevel.ts | 6 + 14 files changed, 422 insertions(+), 13 deletions(-) create mode 100644 src/harness/unittests/extractConstants.ts create mode 100644 tests/baselines/reference/extractConstant/extractConstant_BlockScopes_NoDependencies.ts create mode 100644 tests/baselines/reference/extractConstant/extractConstant_Class.ts create mode 100644 tests/baselines/reference/extractConstant/extractConstant_ClassInsertionPosition.ts create mode 100644 tests/baselines/reference/extractConstant/extractConstant_ExpressionStatement.ts create mode 100644 tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementExpression.ts create mode 100644 tests/baselines/reference/extractConstant/extractConstant_Function.ts create mode 100644 tests/baselines/reference/extractConstant/extractConstant_Method.ts create mode 100644 tests/baselines/reference/extractConstant/extractConstant_Namespace.ts create mode 100644 tests/baselines/reference/extractConstant/extractConstant_TopLevel.ts diff --git a/Jakefile.js b/Jakefile.js index 7b4991b74b6f7..b1823f4ea0ee0 100644 --- a/Jakefile.js +++ b/Jakefile.js @@ -138,6 +138,7 @@ var harnessSources = harnessCoreSources.concat([ "projectErrors.ts", "matchFiles.ts", "initializeTSConfig.ts", + "extractConstants.ts", "extractMethods.ts", "printer.ts", "textChanges.ts", diff --git a/src/harness/tsconfig.json b/src/harness/tsconfig.json index c6e7813863852..53654f7365f4b 100644 --- a/src/harness/tsconfig.json +++ b/src/harness/tsconfig.json @@ -128,6 +128,7 @@ "./unittests/printer.ts", "./unittests/transform.ts", "./unittests/customTransforms.ts", + "./unittests/extractConstants.ts", "./unittests/extractMethods.ts", "./unittests/textChanges.ts", "./unittests/telemetry.ts", diff --git a/src/harness/unittests/extractConstants.ts b/src/harness/unittests/extractConstants.ts new file mode 100644 index 0000000000000..f5018811b1aeb --- /dev/null +++ b/src/harness/unittests/extractConstants.ts @@ -0,0 +1,250 @@ +/// +/// + +namespace ts { + interface Range { + start: number; + end: number; + name: string; + } + + interface Test { + source: string; + ranges: Map; + } + + // TODO (acasey): share + function extractTest(source: string): Test { + const activeRanges: Range[] = []; + let text = ""; + let lastPos = 0; + let pos = 0; + const ranges = createMap(); + + while (pos < source.length) { + if (source.charCodeAt(pos) === CharacterCodes.openBracket && + (source.charCodeAt(pos + 1) === CharacterCodes.hash || source.charCodeAt(pos + 1) === CharacterCodes.$)) { + const saved = pos; + pos += 2; + const s = pos; + consumeIdentifier(); + const e = pos; + if (source.charCodeAt(pos) === CharacterCodes.bar) { + pos++; + text += source.substring(lastPos, saved); + const name = s === e + ? source.charCodeAt(saved + 1) === CharacterCodes.hash ? "selection" : "extracted" + : source.substring(s, e); + activeRanges.push({ name, start: text.length, end: undefined }); + lastPos = pos; + continue; + } + else { + pos = saved; + } + } + else if (source.charCodeAt(pos) === CharacterCodes.bar && source.charCodeAt(pos + 1) === CharacterCodes.closeBracket) { + text += source.substring(lastPos, pos); + activeRanges[activeRanges.length - 1].end = text.length; + const range = activeRanges.pop(); + if (range.name in ranges) { + throw new Error(`Duplicate name of range ${range.name}`); + } + ranges.set(range.name, range); + pos += 2; + lastPos = pos; + continue; + } + pos++; + } + text += source.substring(lastPos, pos); + + function consumeIdentifier() { + while (isIdentifierPart(source.charCodeAt(pos), ScriptTarget.Latest)) { + pos++; + } + } + return { source: text, ranges }; + } + + // TODO (acasey): share + const newLineCharacter = "\n"; + function getRuleProvider(action?: (opts: FormatCodeSettings) => void) { + const options = { + indentSize: 4, + tabSize: 4, + newLineCharacter, + convertTabsToSpaces: true, + indentStyle: ts.IndentStyle.Smart, + insertSpaceAfterConstructor: false, + insertSpaceAfterCommaDelimiter: true, + insertSpaceAfterSemicolonInForStatements: true, + insertSpaceBeforeAndAfterBinaryOperators: true, + insertSpaceAfterKeywordsInControlFlowStatements: true, + insertSpaceAfterFunctionKeywordForAnonymousFunctions: false, + insertSpaceAfterOpeningAndBeforeClosingNonemptyParenthesis: false, + insertSpaceAfterOpeningAndBeforeClosingNonemptyBrackets: false, + insertSpaceAfterOpeningAndBeforeClosingNonemptyBraces: true, + insertSpaceAfterOpeningAndBeforeClosingTemplateStringBraces: false, + insertSpaceAfterOpeningAndBeforeClosingJsxExpressionBraces: false, + insertSpaceBeforeFunctionParenthesis: false, + placeOpenBraceOnNewLineForFunctions: false, + placeOpenBraceOnNewLineForControlBlocks: false, + }; + if (action) { + action(options); + } + const rulesProvider = new formatting.RulesProvider(); + rulesProvider.ensureUpToDate(options); + return rulesProvider; + } + + describe("extractConstants", () => { + testExtractConstant("extractConstant_TopLevel", + `let x = [#|1|];`); + + testExtractConstant("extractConstant_Namespace", + `namespace N { + let x = [#|1|]; +}`); + + testExtractConstant("extractConstant_Class", + `class C { + x = [#|1|]; +}`); + + testExtractConstant("extractConstant_Method", + `class C { + M() { + let x = [#|1|]; + } +}`); + + testExtractConstant("extractConstant_Function", + `function F() { + let x = [#|1|]; +}`); + + testExtractConstant("extractConstant_ExpressionStatement", + `[#|"hello";|]`); + + testExtractConstant("extractConstant_ExpressionStatementExpression", + `[#|"hello"|];`); + + testExtractConstant("extractConstant_BlockScopes_NoDependencies", + `for (let i = 0; i < 10; i++) { + for (let j = 0; j < 10; j++) { + let x = [#|1|]; + } +}`); + + testExtractConstant("extractConstant_ClassInsertionPosition", + `class C { + a = 1; + b = 2; + M1() { } + M2() { } + M3() { + let x = [#|1|]; + } +}`); + + testExtractConstantFailed("extractConstant_Parameters", + `function F() { + let w = 1; + let x = [#|w + 1|]; +}`); + + testExtractConstantFailed("extractConstant_TypeParameters", + `function F(t: T) { + let x = [#|t + 1|]; +}`); + + testExtractConstantFailed("extractConstant_BlockScopes_Dependencies", + `for (let i = 0; i < 10; i++) { + for (let j = 0; j < 10; j++) { + let x = [#|i + 1|]; + } +}`); + }); + + // TODO (acasey): share? + function testExtractConstant(caption: string, text: string) { + it(caption, () => { + Harness.Baseline.runBaseline(`extractConstant/${caption}.ts`, () => { + const t = extractTest(text); + const selectionRange = t.ranges.get("selection"); + if (!selectionRange) { + throw new Error(`Test ${caption} does not specify selection range`); + } + const f = { + path: "/a.ts", + content: t.source + }; + const host = projectSystem.createServerHost([f, projectSystem.libFile]); + const projectService = projectSystem.createProjectService(host); + projectService.openClientFile(f.path); + const program = projectService.inferredProjects[0].getLanguageService().getProgram(); + const sourceFile = program.getSourceFile(f.path); + const context: RefactorContext = { + cancellationToken: { throwIfCancellationRequested() { }, isCancellationRequested() { return false; } }, + newLineCharacter, + program, + file: sourceFile, + startPosition: selectionRange.start, + endPosition: selectionRange.end, + rulesProvider: getRuleProvider() + }; + const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); + assert.equal(rangeToExtract.errors, undefined, rangeToExtract.errors && "Range error: " + rangeToExtract.errors[0].messageText); + const infos = refactor.extractSymbol.getAvailableActions(context); + const actions = find(infos, info => info.description === Diagnostics.Extract_constant.message).actions; + const data: string[] = []; + data.push(`// ==ORIGINAL==`); + data.push(sourceFile.text); + for (const action of actions) { + const { renameLocation, edits } = refactor.extractSymbol.getEditsForAction(context, action.name); + assert.lengthOf(edits, 1); + data.push(`// ==SCOPE::${action.description}==`); + 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); + }); + }); + } + + // TODO (acasey): share? + function testExtractConstantFailed(caption: string, text: string) { + it(caption, () => { + const t = extractTest(text); + const selectionRange = t.ranges.get("selection"); + if (!selectionRange) { + throw new Error(`Test ${caption} does not specify selection range`); + } + const f = { + path: "/a.ts", + content: t.source + }; + const host = projectSystem.createServerHost([f, projectSystem.libFile]); + const projectService = projectSystem.createProjectService(host); + projectService.openClientFile(f.path); + const program = projectService.inferredProjects[0].getLanguageService().getProgram(); + const sourceFile = program.getSourceFile(f.path); + const context: RefactorContext = { + cancellationToken: { throwIfCancellationRequested() { }, isCancellationRequested() { return false; } }, + newLineCharacter, + program, + file: sourceFile, + startPosition: selectionRange.start, + endPosition: selectionRange.end, + rulesProvider: getRuleProvider() + }; + const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); + assert.isUndefined(rangeToExtract.errors, rangeToExtract.errors && "Range error: " + rangeToExtract.errors[0].messageText); + const infos = refactor.extractSymbol.getAvailableActions(context); + assert.isUndefined(find(infos, info => info.description === Diagnostics.Extract_constant.message)); + }); + } +} diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts index 3a161baa8e730..be5d21a80ae59 100644 --- a/src/harness/unittests/extractMethods.ts +++ b/src/harness/unittests/extractMethods.ts @@ -805,7 +805,8 @@ function parsePrimaryExpression(): any { }; const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); assert.equal(rangeToExtract.errors, undefined, "expect no errors"); - const actions = refactor.extractSymbol.getAvailableActions(context)[0].actions; // TODO (acasey): smarter index + const infos = refactor.extractSymbol.getAvailableActions(context); + const actions = find(infos, info => info.description === Diagnostics.Extract_function.message).actions; const data: string[] = []; data.push(`// ==ORIGINAL==`); data.push(sourceFile.text); diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index a1212e7260ce6..0c3b372631216 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -262,10 +262,6 @@ namespace ts.refactor.extractSymbol { return { targetRange: { range: statements, facts: rangeFacts, declarations } }; } - if (isExpressionStatement(start)) { - start = start.expression; - } - // We have a single node (start) const errors = checkRootNode(start) || checkNode(start); if (errors) { @@ -274,7 +270,7 @@ namespace ts.refactor.extractSymbol { return { targetRange: { range: getStatementOrExpressionRange(start), facts: rangeFacts, declarations } }; function checkRootNode(node: Node): Diagnostic[] | undefined { - if (isIdentifier(node)) { + if (isIdentifier(isExpressionStatement(node) ? node.expression : node)) { return [createDiagnosticForNode(node, Messages.CannotExtractIdentifier)]; } return undefined; @@ -539,8 +535,10 @@ namespace ts.refactor.extractSymbol { const { scopes, readsAndWrites: { target, usagesPerScope, constantErrorsPerScope } } = getPossibleExtractionsWorker(targetRange, context); Debug.assert(!constantErrorsPerScope[requestedChangesIndex].length, "The extraction went missing? How?"); context.cancellationToken.throwIfCancellationRequested(); - Debug.assert(target === targetRange.range); - return extractConstantInScope(target as Expression, scopes[requestedChangesIndex], usagesPerScope[requestedChangesIndex], targetRange.facts, context); + const expression = isExpression(target) + ? target + : (target.statements[0] as ExpressionStatement).expression; + return extractConstantInScope(expression, scopes[requestedChangesIndex], usagesPerScope[requestedChangesIndex], targetRange.facts, context); } interface PossibleExtraction { @@ -1114,18 +1112,24 @@ namespace ts.refactor.extractSymbol { child.pos >= minPos && isFunctionLikeDeclaration(child) && !isConstructorDeclaration(child)); } - function getNodeToInsertConstantBefore(minPos: number, scope: Scope): Node { - const isClassLikeScope = isClassLike(scope); + // TODO (acasey): need to dig into nested statements + function getNodeToInsertConstantBefore(maxPos: number, scope: Scope): Node { const children = getStatementsOrClassElements(scope); + Debug.assert(children.length > 0); // There must be at least one child, since we extracted from one. + + const isClassLikeScope = isClassLike(scope); let prevChild: Statement | ClassElement | undefined = undefined; for (const child of children) { - if (child.pos >= minPos || (isClassLikeScope && !isPropertyDeclaration(child))) { + if (child.pos >= maxPos) { break; } prevChild = child; + if (isClassLikeScope && !isPropertyDeclaration(child)) { + break; + } } - return prevChild || children[0]; // There must be one - minPos is in one. + return prevChild; } function getPropertyAssignmentsForWrites(writes: ReadonlyArray): ShorthandPropertyAssignment[] { @@ -1192,7 +1196,7 @@ namespace ts.refactor.extractSymbol { const visibleDeclarationsInExtractedRange: Symbol[] = []; const expressionDiagnostics = - isReadonlyArray(targetRange.range) + isReadonlyArray(targetRange.range) && !(targetRange.range.length === 1 && isExpressionStatement(targetRange.range[0])) ? ((start, end) => [createFileDiagnostic(sourceFile, start, end - start, Messages.ExpressionExpected)])(firstOrUndefined(targetRange.range).getStart(), lastOrUndefined(targetRange.range).end) : []; diff --git a/tests/baselines/reference/extractConstant/extractConstant_BlockScopes_NoDependencies.ts b/tests/baselines/reference/extractConstant/extractConstant_BlockScopes_NoDependencies.ts new file mode 100644 index 0000000000000..25609a3a8010a --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_BlockScopes_NoDependencies.ts @@ -0,0 +1,14 @@ +// ==ORIGINAL== +for (let i = 0; i < 10; i++) { + for (let j = 0; j < 10; j++) { + let x = 1; + } +} +// ==SCOPE::Extract to constant in global scope== +const newLocal = 1; + +for (let i = 0; i < 10; i++) { + for (let j = 0; j < 10; j++) { + let x = /*RENAME*/newLocal; + } +} \ No newline at end of file diff --git a/tests/baselines/reference/extractConstant/extractConstant_Class.ts b/tests/baselines/reference/extractConstant/extractConstant_Class.ts new file mode 100644 index 0000000000000..eb06cf6c0cf7d --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_Class.ts @@ -0,0 +1,16 @@ +// ==ORIGINAL== +class C { + x = 1; +} +// ==SCOPE::Extract to readonly field in class 'C'== +class C { + private readonly newProperty = 1; + + x = this./*RENAME*/newProperty; +} +// ==SCOPE::Extract to constant in global scope== +const newLocal = 1; + +class C { + x = /*RENAME*/newLocal; +} \ No newline at end of file diff --git a/tests/baselines/reference/extractConstant/extractConstant_ClassInsertionPosition.ts b/tests/baselines/reference/extractConstant/extractConstant_ClassInsertionPosition.ts new file mode 100644 index 0000000000000..e024fda34bcb1 --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_ClassInsertionPosition.ts @@ -0,0 +1,46 @@ +// ==ORIGINAL== +class C { + a = 1; + b = 2; + M1() { } + M2() { } + M3() { + let x = 1; + } +} +// ==SCOPE::Extract to constant in method 'M3== +class C { + a = 1; + b = 2; + M1() { } + M2() { } + M3() { + const newLocal = 1; + + let x = /*RENAME*/newLocal; + } +} +// ==SCOPE::Extract to readonly field in class 'C'== +class C { + a = 1; + b = 2; + private readonly newProperty = 1; + + M1() { } + M2() { } + M3() { + let x = this./*RENAME*/newProperty; + } +} +// ==SCOPE::Extract to constant in global scope== +const newLocal = 1; + +class C { + a = 1; + b = 2; + M1() { } + M2() { } + M3() { + let x = /*RENAME*/newLocal; + } +} \ No newline at end of file diff --git a/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatement.ts b/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatement.ts new file mode 100644 index 0000000000000..6bf35cd17e125 --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatement.ts @@ -0,0 +1,4 @@ +// ==ORIGINAL== +"hello"; +// ==SCOPE::Extract to constant in global scope== +const /*RENAME*/newLocal = "hello"; \ No newline at end of file diff --git a/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementExpression.ts b/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementExpression.ts new file mode 100644 index 0000000000000..6bf35cd17e125 --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementExpression.ts @@ -0,0 +1,4 @@ +// ==ORIGINAL== +"hello"; +// ==SCOPE::Extract to constant in global scope== +const /*RENAME*/newLocal = "hello"; \ No newline at end of file diff --git a/tests/baselines/reference/extractConstant/extractConstant_Function.ts b/tests/baselines/reference/extractConstant/extractConstant_Function.ts new file mode 100644 index 0000000000000..67c8255c4b45d --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_Function.ts @@ -0,0 +1,16 @@ +// ==ORIGINAL== +function F() { + let x = 1; +} +// ==SCOPE::Extract to constant in function 'F'== +function F() { + const newLocal = 1; + + let x = /*RENAME*/newLocal; +} +// ==SCOPE::Extract to constant in global scope== +const newLocal = 1; + +function F() { + let x = /*RENAME*/newLocal; +} \ No newline at end of file diff --git a/tests/baselines/reference/extractConstant/extractConstant_Method.ts b/tests/baselines/reference/extractConstant/extractConstant_Method.ts new file mode 100644 index 0000000000000..1ae4c8b1cb5ac --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_Method.ts @@ -0,0 +1,30 @@ +// ==ORIGINAL== +class C { + M() { + let x = 1; + } +} +// ==SCOPE::Extract to constant in method 'M== +class C { + M() { + const newLocal = 1; + + let x = /*RENAME*/newLocal; + } +} +// ==SCOPE::Extract to readonly field in class 'C'== +class C { + private readonly newProperty = 1; + + M() { + let x = this./*RENAME*/newProperty; + } +} +// ==SCOPE::Extract to constant in global scope== +const newLocal = 1; + +class C { + M() { + let x = /*RENAME*/newLocal; + } +} \ No newline at end of file diff --git a/tests/baselines/reference/extractConstant/extractConstant_Namespace.ts b/tests/baselines/reference/extractConstant/extractConstant_Namespace.ts new file mode 100644 index 0000000000000..8f25847165f8a --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_Namespace.ts @@ -0,0 +1,16 @@ +// ==ORIGINAL== +namespace N { + let x = 1; +} +// ==SCOPE::Extract to constant in namespace 'N'== +namespace N { + const newLocal = 1; + + let x = /*RENAME*/newLocal; +} +// ==SCOPE::Extract to constant in global scope== +const newLocal = 1; + +namespace N { + let x = /*RENAME*/newLocal; +} \ No newline at end of file diff --git a/tests/baselines/reference/extractConstant/extractConstant_TopLevel.ts b/tests/baselines/reference/extractConstant/extractConstant_TopLevel.ts new file mode 100644 index 0000000000000..fb0447583ff8f --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_TopLevel.ts @@ -0,0 +1,6 @@ +// ==ORIGINAL== +let x = 1; +// ==SCOPE::Extract to constant in global scope== +const newLocal = 1; + +let x = /*RENAME*/newLocal; \ No newline at end of file From 52ab05e99da4f4f3d4f481f12f7e069ae099790b Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 26 Sep 2017 16:26:12 -0700 Subject: [PATCH 4/9] Rename extractMethods.ts to extractFunctions.ts for consistency --- Jakefile.js | 2 +- src/harness/tsconfig.json | 2 +- .../unittests/{extractMethods.ts => extractFunctions.ts} | 0 3 files changed, 2 insertions(+), 2 deletions(-) rename src/harness/unittests/{extractMethods.ts => extractFunctions.ts} (100%) diff --git a/Jakefile.js b/Jakefile.js index b1823f4ea0ee0..359549d82486d 100644 --- a/Jakefile.js +++ b/Jakefile.js @@ -139,7 +139,7 @@ var harnessSources = harnessCoreSources.concat([ "matchFiles.ts", "initializeTSConfig.ts", "extractConstants.ts", - "extractMethods.ts", + "extractFunctions.ts", "printer.ts", "textChanges.ts", "telemetry.ts", diff --git a/src/harness/tsconfig.json b/src/harness/tsconfig.json index 53654f7365f4b..cb2cb00b86118 100644 --- a/src/harness/tsconfig.json +++ b/src/harness/tsconfig.json @@ -129,7 +129,7 @@ "./unittests/transform.ts", "./unittests/customTransforms.ts", "./unittests/extractConstants.ts", - "./unittests/extractMethods.ts", + "./unittests/extractFunctions.ts", "./unittests/textChanges.ts", "./unittests/telemetry.ts", "./unittests/languageService.ts", diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractFunctions.ts similarity index 100% rename from src/harness/unittests/extractMethods.ts rename to src/harness/unittests/extractFunctions.ts From 697bce74b86021ccd28445a3af700d230279e16d Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 26 Sep 2017 16:46:32 -0700 Subject: [PATCH 5/9] Split range tests and helpers out of extractFunctions.ts --- Jakefile.js | 2 + src/harness/tsconfig.json | 2 + src/harness/unittests/extractConstants.ts | 176 +------- src/harness/unittests/extractFunctions.ts | 457 +------------------- src/harness/unittests/extractRanges.ts | 319 ++++++++++++++ src/harness/unittests/extractTestHelpers.ts | 177 ++++++++ 6 files changed, 505 insertions(+), 628 deletions(-) create mode 100644 src/harness/unittests/extractRanges.ts create mode 100644 src/harness/unittests/extractTestHelpers.ts diff --git a/Jakefile.js b/Jakefile.js index 359549d82486d..8a4c67ac84bc4 100644 --- a/Jakefile.js +++ b/Jakefile.js @@ -140,6 +140,8 @@ var harnessSources = harnessCoreSources.concat([ "initializeTSConfig.ts", "extractConstants.ts", "extractFunctions.ts", + "extractRanges.ts", + "extractTestHelpers.ts", "printer.ts", "textChanges.ts", "telemetry.ts", diff --git a/src/harness/tsconfig.json b/src/harness/tsconfig.json index cb2cb00b86118..88999b2d979ff 100644 --- a/src/harness/tsconfig.json +++ b/src/harness/tsconfig.json @@ -130,6 +130,8 @@ "./unittests/customTransforms.ts", "./unittests/extractConstants.ts", "./unittests/extractFunctions.ts", + "./unittests/extractRanges.ts", + "./unittests/extractTestHelpers.ts", "./unittests/textChanges.ts", "./unittests/telemetry.ts", "./unittests/languageService.ts", diff --git a/src/harness/unittests/extractConstants.ts b/src/harness/unittests/extractConstants.ts index f5018811b1aeb..fb776eb6af36d 100644 --- a/src/harness/unittests/extractConstants.ts +++ b/src/harness/unittests/extractConstants.ts @@ -1,104 +1,6 @@ -/// -/// +/// namespace ts { - interface Range { - start: number; - end: number; - name: string; - } - - interface Test { - source: string; - ranges: Map; - } - - // TODO (acasey): share - function extractTest(source: string): Test { - const activeRanges: Range[] = []; - let text = ""; - let lastPos = 0; - let pos = 0; - const ranges = createMap(); - - while (pos < source.length) { - if (source.charCodeAt(pos) === CharacterCodes.openBracket && - (source.charCodeAt(pos + 1) === CharacterCodes.hash || source.charCodeAt(pos + 1) === CharacterCodes.$)) { - const saved = pos; - pos += 2; - const s = pos; - consumeIdentifier(); - const e = pos; - if (source.charCodeAt(pos) === CharacterCodes.bar) { - pos++; - text += source.substring(lastPos, saved); - const name = s === e - ? source.charCodeAt(saved + 1) === CharacterCodes.hash ? "selection" : "extracted" - : source.substring(s, e); - activeRanges.push({ name, start: text.length, end: undefined }); - lastPos = pos; - continue; - } - else { - pos = saved; - } - } - else if (source.charCodeAt(pos) === CharacterCodes.bar && source.charCodeAt(pos + 1) === CharacterCodes.closeBracket) { - text += source.substring(lastPos, pos); - activeRanges[activeRanges.length - 1].end = text.length; - const range = activeRanges.pop(); - if (range.name in ranges) { - throw new Error(`Duplicate name of range ${range.name}`); - } - ranges.set(range.name, range); - pos += 2; - lastPos = pos; - continue; - } - pos++; - } - text += source.substring(lastPos, pos); - - function consumeIdentifier() { - while (isIdentifierPart(source.charCodeAt(pos), ScriptTarget.Latest)) { - pos++; - } - } - return { source: text, ranges }; - } - - // TODO (acasey): share - const newLineCharacter = "\n"; - function getRuleProvider(action?: (opts: FormatCodeSettings) => void) { - const options = { - indentSize: 4, - tabSize: 4, - newLineCharacter, - convertTabsToSpaces: true, - indentStyle: ts.IndentStyle.Smart, - insertSpaceAfterConstructor: false, - insertSpaceAfterCommaDelimiter: true, - insertSpaceAfterSemicolonInForStatements: true, - insertSpaceBeforeAndAfterBinaryOperators: true, - insertSpaceAfterKeywordsInControlFlowStatements: true, - insertSpaceAfterFunctionKeywordForAnonymousFunctions: false, - insertSpaceAfterOpeningAndBeforeClosingNonemptyParenthesis: false, - insertSpaceAfterOpeningAndBeforeClosingNonemptyBrackets: false, - insertSpaceAfterOpeningAndBeforeClosingNonemptyBraces: true, - insertSpaceAfterOpeningAndBeforeClosingTemplateStringBraces: false, - insertSpaceAfterOpeningAndBeforeClosingJsxExpressionBraces: false, - insertSpaceBeforeFunctionParenthesis: false, - placeOpenBraceOnNewLineForFunctions: false, - placeOpenBraceOnNewLineForControlBlocks: false, - }; - if (action) { - action(options); - } - const rulesProvider = new formatting.RulesProvider(); - rulesProvider.ensureUpToDate(options); - return rulesProvider; - } - describe("extractConstants", () => { testExtractConstant("extractConstant_TopLevel", `let x = [#|1|];`); @@ -168,83 +70,11 @@ namespace ts { }`); }); - // TODO (acasey): share? function testExtractConstant(caption: string, text: string) { - it(caption, () => { - Harness.Baseline.runBaseline(`extractConstant/${caption}.ts`, () => { - const t = extractTest(text); - const selectionRange = t.ranges.get("selection"); - if (!selectionRange) { - throw new Error(`Test ${caption} does not specify selection range`); - } - const f = { - path: "/a.ts", - content: t.source - }; - const host = projectSystem.createServerHost([f, projectSystem.libFile]); - const projectService = projectSystem.createProjectService(host); - projectService.openClientFile(f.path); - const program = projectService.inferredProjects[0].getLanguageService().getProgram(); - const sourceFile = program.getSourceFile(f.path); - const context: RefactorContext = { - cancellationToken: { throwIfCancellationRequested() { }, isCancellationRequested() { return false; } }, - newLineCharacter, - program, - file: sourceFile, - startPosition: selectionRange.start, - endPosition: selectionRange.end, - rulesProvider: getRuleProvider() - }; - const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); - assert.equal(rangeToExtract.errors, undefined, rangeToExtract.errors && "Range error: " + rangeToExtract.errors[0].messageText); - const infos = refactor.extractSymbol.getAvailableActions(context); - const actions = find(infos, info => info.description === Diagnostics.Extract_constant.message).actions; - const data: string[] = []; - data.push(`// ==ORIGINAL==`); - data.push(sourceFile.text); - for (const action of actions) { - const { renameLocation, edits } = refactor.extractSymbol.getEditsForAction(context, action.name); - assert.lengthOf(edits, 1); - data.push(`// ==SCOPE::${action.description}==`); - 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); - }); - }); + testExtractSymbol(caption, text, "extractConstant", Diagnostics.Extract_constant); } - // TODO (acasey): share? function testExtractConstantFailed(caption: string, text: string) { - it(caption, () => { - const t = extractTest(text); - const selectionRange = t.ranges.get("selection"); - if (!selectionRange) { - throw new Error(`Test ${caption} does not specify selection range`); - } - const f = { - path: "/a.ts", - content: t.source - }; - const host = projectSystem.createServerHost([f, projectSystem.libFile]); - const projectService = projectSystem.createProjectService(host); - projectService.openClientFile(f.path); - const program = projectService.inferredProjects[0].getLanguageService().getProgram(); - const sourceFile = program.getSourceFile(f.path); - const context: RefactorContext = { - cancellationToken: { throwIfCancellationRequested() { }, isCancellationRequested() { return false; } }, - newLineCharacter, - program, - file: sourceFile, - startPosition: selectionRange.start, - endPosition: selectionRange.end, - rulesProvider: getRuleProvider() - }; - const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); - assert.isUndefined(rangeToExtract.errors, rangeToExtract.errors && "Range error: " + rangeToExtract.errors[0].messageText); - const infos = refactor.extractSymbol.getAvailableActions(context); - assert.isUndefined(find(infos, info => info.description === Diagnostics.Extract_constant.message)); - }); + testExtractSymbolFailed(caption, text, Diagnostics.Extract_constant); } } diff --git a/src/harness/unittests/extractFunctions.ts b/src/harness/unittests/extractFunctions.ts index be5d21a80ae59..2497b2eec5116 100644 --- a/src/harness/unittests/extractFunctions.ts +++ b/src/harness/unittests/extractFunctions.ts @@ -1,417 +1,7 @@ -/// -/// +/// namespace ts { - interface Range { - start: number; - end: number; - name: string; - } - - interface Test { - source: string; - ranges: Map; - } - - function extractTest(source: string): Test { - const activeRanges: Range[] = []; - let text = ""; - let lastPos = 0; - let pos = 0; - const ranges = createMap(); - - while (pos < source.length) { - if (source.charCodeAt(pos) === CharacterCodes.openBracket && - (source.charCodeAt(pos + 1) === CharacterCodes.hash || source.charCodeAt(pos + 1) === CharacterCodes.$)) { - const saved = pos; - pos += 2; - const s = pos; - consumeIdentifier(); - const e = pos; - if (source.charCodeAt(pos) === CharacterCodes.bar) { - pos++; - text += source.substring(lastPos, saved); - const name = s === e - ? source.charCodeAt(saved + 1) === CharacterCodes.hash ? "selection" : "extracted" - : source.substring(s, e); - activeRanges.push({ name, start: text.length, end: undefined }); - lastPos = pos; - continue; - } - else { - pos = saved; - } - } - else if (source.charCodeAt(pos) === CharacterCodes.bar && source.charCodeAt(pos + 1) === CharacterCodes.closeBracket) { - text += source.substring(lastPos, pos); - activeRanges[activeRanges.length - 1].end = text.length; - const range = activeRanges.pop(); - if (range.name in ranges) { - throw new Error(`Duplicate name of range ${range.name}`); - } - ranges.set(range.name, range); - pos += 2; - lastPos = pos; - continue; - } - pos++; - } - text += source.substring(lastPos, pos); - - function consumeIdentifier() { - while (isIdentifierPart(source.charCodeAt(pos), ScriptTarget.Latest)) { - pos++; - } - } - return { source: text, ranges }; - } - - const newLineCharacter = "\n"; - function getRuleProvider(action?: (opts: FormatCodeSettings) => void) { - const options = { - indentSize: 4, - tabSize: 4, - newLineCharacter, - convertTabsToSpaces: true, - indentStyle: ts.IndentStyle.Smart, - insertSpaceAfterConstructor: false, - insertSpaceAfterCommaDelimiter: true, - insertSpaceAfterSemicolonInForStatements: true, - insertSpaceBeforeAndAfterBinaryOperators: true, - insertSpaceAfterKeywordsInControlFlowStatements: true, - insertSpaceAfterFunctionKeywordForAnonymousFunctions: false, - insertSpaceAfterOpeningAndBeforeClosingNonemptyParenthesis: false, - insertSpaceAfterOpeningAndBeforeClosingNonemptyBrackets: false, - insertSpaceAfterOpeningAndBeforeClosingNonemptyBraces: true, - insertSpaceAfterOpeningAndBeforeClosingTemplateStringBraces: false, - insertSpaceAfterOpeningAndBeforeClosingJsxExpressionBraces: false, - insertSpaceBeforeFunctionParenthesis: false, - placeOpenBraceOnNewLineForFunctions: false, - placeOpenBraceOnNewLineForControlBlocks: false, - }; - if (action) { - action(options); - } - const rulesProvider = new formatting.RulesProvider(); - rulesProvider.ensureUpToDate(options); - return rulesProvider; - } - - function testExtractRangeFailed(caption: string, s: string, expectedErrors: string[]) { - return it(caption, () => { - const t = extractTest(s); - const file = createSourceFile("a.ts", t.source, ScriptTarget.Latest, /*setParentNodes*/ true); - const selectionRange = t.ranges.get("selection"); - if (!selectionRange) { - throw new Error(`Test ${s} does not specify selection range`); - } - const result = refactor.extractSymbol.getRangeToExtract(file, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); - assert(result.targetRange === undefined, "failure expected"); - const sortedErrors = result.errors.map(e => e.messageText).sort(); - assert.deepEqual(sortedErrors, expectedErrors.sort(), "unexpected errors"); - }); - } - - function testExtractRange(s: string): void { - const t = extractTest(s); - const f = createSourceFile("a.ts", t.source, ScriptTarget.Latest, /*setParentNodes*/ true); - const selectionRange = t.ranges.get("selection"); - if (!selectionRange) { - throw new Error(`Test ${s} does not specify selection range`); - } - const result = refactor.extractSymbol.getRangeToExtract(f, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); - const expectedRange = t.ranges.get("extracted"); - if (expectedRange) { - let start: number, end: number; - if (ts.isArray(result.targetRange.range)) { - start = result.targetRange.range[0].getStart(f); - end = ts.lastOrUndefined(result.targetRange.range).getEnd(); - } - else { - start = result.targetRange.range.getStart(f); - end = result.targetRange.range.getEnd(); - } - assert.equal(start, expectedRange.start, "incorrect start of range"); - assert.equal(end, expectedRange.end, "incorrect end of range"); - } - else { - assert.isTrue(!result.targetRange, `expected range to extract to be undefined`); - } - } - describe("extractMethods", () => { - it("get extract range from selection", () => { - testExtractRange(` - [#| - [$|var x = 1; - var y = 2;|]|] - `); - testExtractRange(` - [#| - var x = 1; - var y = 2|]; - `); - testExtractRange(` - [#|var x = 1|]; - var y = 2; - `); - testExtractRange(` - if ([#|[#extracted|a && b && c && d|]|]) { - } - `); - testExtractRange(` - if [#|(a && b && c && d|]) { - } - `); - testExtractRange(` - if (a && b && c && d) { - [#| [$|var x = 1; - console.log(x);|] |] - } - `); - testExtractRange(` - [#| - if (a) { - return 100; - } |] - `); - testExtractRange(` - function foo() { - [#| [$|if (a) { - } - return 100|] |] - } - `); - testExtractRange(` - [#| - [$|l1: - if (x) { - break l1; - }|]|] - `); - testExtractRange(` - [#| - [$|l2: - { - if (x) { - } - break l2; - }|]|] - `); - testExtractRange(` - while (true) { - [#| if(x) { - } - break; |] - } - `); - testExtractRange(` - while (true) { - [#| if(x) { - } - continue; |] - } - `); - testExtractRange(` - l3: - { - [#| - if (x) { - } - break l3; |] - } - `); - testExtractRange(` - function f() { - while (true) { - [#| - if (x) { - return; - } |] - } - } - `); - testExtractRange(` - function f() { - while (true) { - [#| - [$|if (x) { - } - return;|] - |] - } - } - `); - testExtractRange(` - function f() { - return [#| [$|1 + 2|] |]+ 3; - } - } - `); - testExtractRange(` - function f() { - return [$|1 + [#|2 + 3|]|]; - } - } - `); - testExtractRange(` - function f() { - return [$|1 + 2 + [#|3 + 4|]|]; - } - } - `); - }); - - testExtractRangeFailed("extractRangeFailed1", - ` -namespace A { - function f() { - [#| - let x = 1 - if (x) { - return 10; - } - |] - } -} - `, - [ - "Cannot extract range containing conditional return statement." - ]); - - testExtractRangeFailed("extractRangeFailed2", - ` -namespace A { - function f() { - while (true) { - [#| - let x = 1 - if (x) { - break; - } - |] - } - } -} - `, - [ - "Cannot extract range containing conditional break or continue statements." - ]); - - testExtractRangeFailed("extractRangeFailed3", - ` -namespace A { - function f() { - while (true) { - [#| - let x = 1 - if (x) { - continue; - } - |] - } - } -} - `, - [ - "Cannot extract range containing conditional break or continue statements." - ]); - - testExtractRangeFailed("extractRangeFailed4", - ` -namespace A { - function f() { - l1: { - [#| - let x = 1 - if (x) { - break l1; - } - |] - } - } -} - `, - [ - "Cannot extract range containing labeled break or continue with target outside of the range." - ]); - - testExtractRangeFailed("extractRangeFailed5", - ` -namespace A { - function f() { - [#| - try { - f2() - return 10; - } - catch (e) { - } - |] - } - function f2() { - } -} - `, - [ - "Cannot extract range containing conditional return statement." - ]); - - testExtractRangeFailed("extractRangeFailed6", - ` -namespace A { - function f() { - [#| - try { - f2() - } - catch (e) { - return 10; - } - |] - } - function f2() { - } -} - `, - [ - "Cannot extract range containing conditional return statement." - ]); - - testExtractRangeFailed("extractRangeFailed7", - ` -function test(x: number) { - while (x) { - x--; - [#|break;|] - } -} - `, - [ - "Cannot extract range containing conditional break or continue statements." - ]); - - testExtractRangeFailed("extractRangeFailed8", - ` -function test(x: number) { - switch (x) { - case 1: - [#|break;|] - } -} - `, - [ - "Cannot extract range containing conditional break or continue statements." - ]); - - testExtractRangeFailed("extractRangeFailed9", - `var x = ([#||]1 + 2);`, - [ - "Cannot extract empty range." - ]); - - testExtractRangeFailed("extract-method-not-for-token-expression-statement", `[#|a|]`, ["Select more than a single identifier."]); - testExtractMethod("extractMethod1", `namespace A { let x = 1; @@ -776,50 +366,7 @@ function parsePrimaryExpression(): any { }`); }); - function testExtractMethod(caption: string, text: string) { - it(caption, () => { - Harness.Baseline.runBaseline(`extractMethod/${caption}.ts`, () => { - const t = extractTest(text); - const selectionRange = t.ranges.get("selection"); - if (!selectionRange) { - throw new Error(`Test ${caption} does not specify selection range`); - } - const f = { - path: "/a.ts", - content: t.source - }; - const host = projectSystem.createServerHost([f, projectSystem.libFile]); - const projectService = projectSystem.createProjectService(host); - projectService.openClientFile(f.path); - const program = projectService.inferredProjects[0].getLanguageService().getProgram(); - const sourceFile = program.getSourceFile(f.path); - const context: RefactorContext = { - cancellationToken: { throwIfCancellationRequested() { }, isCancellationRequested() { return false; } }, - newLineCharacter, - program, - file: sourceFile, - startPosition: selectionRange.start, - endPosition: selectionRange.end, - rulesProvider: getRuleProvider() - }; - const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); - assert.equal(rangeToExtract.errors, undefined, "expect no errors"); - const infos = refactor.extractSymbol.getAvailableActions(context); - const actions = find(infos, info => info.description === Diagnostics.Extract_function.message).actions; - const data: string[] = []; - data.push(`// ==ORIGINAL==`); - data.push(sourceFile.text); - for (const action of actions) { - const { renameLocation, edits } = refactor.extractSymbol.getEditsForAction(context, action.name); - assert.lengthOf(edits, 1); - data.push(`// ==SCOPE::${action.description}==`); - 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); - }); - }); + testExtractSymbol(caption, text, "extractMethod", Diagnostics.Extract_function); } } diff --git a/src/harness/unittests/extractRanges.ts b/src/harness/unittests/extractRanges.ts new file mode 100644 index 0000000000000..55535a6dc44de --- /dev/null +++ b/src/harness/unittests/extractRanges.ts @@ -0,0 +1,319 @@ +/// + +namespace ts { + function testExtractRangeFailed(caption: string, s: string, expectedErrors: string[]) { + return it(caption, () => { + const t = extractTest(s); + const file = createSourceFile("a.ts", t.source, ScriptTarget.Latest, /*setParentNodes*/ true); + const selectionRange = t.ranges.get("selection"); + if (!selectionRange) { + throw new Error(`Test ${s} does not specify selection range`); + } + const result = refactor.extractSymbol.getRangeToExtract(file, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); + assert(result.targetRange === undefined, "failure expected"); + const sortedErrors = result.errors.map(e => e.messageText).sort(); + assert.deepEqual(sortedErrors, expectedErrors.sort(), "unexpected errors"); + }); + } + + function testExtractRange(s: string): void { + const t = extractTest(s); + const f = createSourceFile("a.ts", t.source, ScriptTarget.Latest, /*setParentNodes*/ true); + const selectionRange = t.ranges.get("selection"); + if (!selectionRange) { + throw new Error(`Test ${s} does not specify selection range`); + } + const result = refactor.extractSymbol.getRangeToExtract(f, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); + const expectedRange = t.ranges.get("extracted"); + if (expectedRange) { + let start: number, end: number; + if (ts.isArray(result.targetRange.range)) { + start = result.targetRange.range[0].getStart(f); + end = ts.lastOrUndefined(result.targetRange.range).getEnd(); + } + else { + start = result.targetRange.range.getStart(f); + end = result.targetRange.range.getEnd(); + } + assert.equal(start, expectedRange.start, "incorrect start of range"); + assert.equal(end, expectedRange.end, "incorrect end of range"); + } + else { + assert.isTrue(!result.targetRange, `expected range to extract to be undefined`); + } + } + + describe("extractRanges", () => { + it("get extract range from selection", () => { + testExtractRange(` + [#| + [$|var x = 1; + var y = 2;|]|] + `); + testExtractRange(` + [#| + var x = 1; + var y = 2|]; + `); + testExtractRange(` + [#|var x = 1|]; + var y = 2; + `); + testExtractRange(` + if ([#|[#extracted|a && b && c && d|]|]) { + } + `); + testExtractRange(` + if [#|(a && b && c && d|]) { + } + `); + testExtractRange(` + if (a && b && c && d) { + [#| [$|var x = 1; + console.log(x);|] |] + } + `); + testExtractRange(` + [#| + if (a) { + return 100; + } |] + `); + testExtractRange(` + function foo() { + [#| [$|if (a) { + } + return 100|] |] + } + `); + testExtractRange(` + [#| + [$|l1: + if (x) { + break l1; + }|]|] + `); + testExtractRange(` + [#| + [$|l2: + { + if (x) { + } + break l2; + }|]|] + `); + testExtractRange(` + while (true) { + [#| if(x) { + } + break; |] + } + `); + testExtractRange(` + while (true) { + [#| if(x) { + } + continue; |] + } + `); + testExtractRange(` + l3: + { + [#| + if (x) { + } + break l3; |] + } + `); + testExtractRange(` + function f() { + while (true) { + [#| + if (x) { + return; + } |] + } + } + `); + testExtractRange(` + function f() { + while (true) { + [#| + [$|if (x) { + } + return;|] + |] + } + } + `); + testExtractRange(` + function f() { + return [#| [$|1 + 2|] |]+ 3; + } + } + `); + testExtractRange(` + function f() { + return [$|1 + [#|2 + 3|]|]; + } + } + `); + testExtractRange(` + function f() { + return [$|1 + 2 + [#|3 + 4|]|]; + } + } + `); + }); + + testExtractRangeFailed("extractRangeFailed1", + ` +namespace A { +function f() { + [#| + let x = 1 + if (x) { + return 10; + } + |] +} +} + `, + [ + "Cannot extract range containing conditional return statement." + ]); + + testExtractRangeFailed("extractRangeFailed2", + ` +namespace A { +function f() { + while (true) { + [#| + let x = 1 + if (x) { + break; + } + |] + } +} +} + `, + [ + "Cannot extract range containing conditional break or continue statements." + ]); + + testExtractRangeFailed("extractRangeFailed3", + ` +namespace A { +function f() { + while (true) { + [#| + let x = 1 + if (x) { + continue; + } + |] + } +} +} + `, + [ + "Cannot extract range containing conditional break or continue statements." + ]); + + testExtractRangeFailed("extractRangeFailed4", + ` +namespace A { +function f() { + l1: { + [#| + let x = 1 + if (x) { + break l1; + } + |] + } +} +} + `, + [ + "Cannot extract range containing labeled break or continue with target outside of the range." + ]); + + testExtractRangeFailed("extractRangeFailed5", + ` +namespace A { +function f() { + [#| + try { + f2() + return 10; + } + catch (e) { + } + |] +} +function f2() { +} +} + `, + [ + "Cannot extract range containing conditional return statement." + ]); + + testExtractRangeFailed("extractRangeFailed6", + ` +namespace A { +function f() { + [#| + try { + f2() + } + catch (e) { + return 10; + } + |] +} +function f2() { +} +} + `, + [ + "Cannot extract range containing conditional return statement." + ]); + + testExtractRangeFailed("extractRangeFailed7", + ` +function test(x: number) { +while (x) { + x--; + [#|break;|] +} +} + `, + [ + "Cannot extract range containing conditional break or continue statements." + ]); + + testExtractRangeFailed("extractRangeFailed8", + ` +function test(x: number) { +switch (x) { + case 1: + [#|break;|] +} +} + `, + [ + "Cannot extract range containing conditional break or continue statements." + ]); + + testExtractRangeFailed("extractRangeFailed9", + `var x = ([#||]1 + 2);`, + [ + "Cannot extract empty range." + ]); + + testExtractRangeFailed("extract-method-not-for-token-expression-statement", `[#|a|]`, ["Select more than a single identifier."]); + }); +} \ No newline at end of file diff --git a/src/harness/unittests/extractTestHelpers.ts b/src/harness/unittests/extractTestHelpers.ts new file mode 100644 index 0000000000000..e619f9343d891 --- /dev/null +++ b/src/harness/unittests/extractTestHelpers.ts @@ -0,0 +1,177 @@ +/// +/// + +namespace ts { + export interface Range { + start: number; + end: number; + name: string; + } + + export interface Test { + source: string; + ranges: Map; + } + + export function extractTest(source: string): Test { + const activeRanges: Range[] = []; + let text = ""; + let lastPos = 0; + let pos = 0; + const ranges = createMap(); + + while (pos < source.length) { + if (source.charCodeAt(pos) === CharacterCodes.openBracket && + (source.charCodeAt(pos + 1) === CharacterCodes.hash || source.charCodeAt(pos + 1) === CharacterCodes.$)) { + const saved = pos; + pos += 2; + const s = pos; + consumeIdentifier(); + const e = pos; + if (source.charCodeAt(pos) === CharacterCodes.bar) { + pos++; + text += source.substring(lastPos, saved); + const name = s === e + ? source.charCodeAt(saved + 1) === CharacterCodes.hash ? "selection" : "extracted" + : source.substring(s, e); + activeRanges.push({ name, start: text.length, end: undefined }); + lastPos = pos; + continue; + } + else { + pos = saved; + } + } + else if (source.charCodeAt(pos) === CharacterCodes.bar && source.charCodeAt(pos + 1) === CharacterCodes.closeBracket) { + text += source.substring(lastPos, pos); + activeRanges[activeRanges.length - 1].end = text.length; + const range = activeRanges.pop(); + if (range.name in ranges) { + throw new Error(`Duplicate name of range ${range.name}`); + } + ranges.set(range.name, range); + pos += 2; + lastPos = pos; + continue; + } + pos++; + } + text += source.substring(lastPos, pos); + + function consumeIdentifier() { + while (isIdentifierPart(source.charCodeAt(pos), ScriptTarget.Latest)) { + pos++; + } + } + return { source: text, ranges }; + } + + export const newLineCharacter = "\n"; + export function getRuleProvider(action?: (opts: FormatCodeSettings) => void) { + const options = { + indentSize: 4, + tabSize: 4, + newLineCharacter, + convertTabsToSpaces: true, + indentStyle: ts.IndentStyle.Smart, + insertSpaceAfterConstructor: false, + insertSpaceAfterCommaDelimiter: true, + insertSpaceAfterSemicolonInForStatements: true, + insertSpaceBeforeAndAfterBinaryOperators: true, + insertSpaceAfterKeywordsInControlFlowStatements: true, + insertSpaceAfterFunctionKeywordForAnonymousFunctions: false, + insertSpaceAfterOpeningAndBeforeClosingNonemptyParenthesis: false, + insertSpaceAfterOpeningAndBeforeClosingNonemptyBrackets: false, + insertSpaceAfterOpeningAndBeforeClosingNonemptyBraces: true, + insertSpaceAfterOpeningAndBeforeClosingTemplateStringBraces: false, + insertSpaceAfterOpeningAndBeforeClosingJsxExpressionBraces: false, + insertSpaceBeforeFunctionParenthesis: false, + placeOpenBraceOnNewLineForFunctions: false, + placeOpenBraceOnNewLineForControlBlocks: false, + }; + if (action) { + action(options); + } + const rulesProvider = new formatting.RulesProvider(); + rulesProvider.ensureUpToDate(options); + return rulesProvider; + } + + export function testExtractSymbol(caption: string, text: string, baselineFolder: string, description: DiagnosticMessage) { + it(caption, () => { + Harness.Baseline.runBaseline(`${baselineFolder}/${caption}.ts`, () => { + const t = extractTest(text); + const selectionRange = t.ranges.get("selection"); + if (!selectionRange) { + throw new Error(`Test ${caption} does not specify selection range`); + } + const f = { + path: "/a.ts", + content: t.source + }; + const host = projectSystem.createServerHost([f, projectSystem.libFile]); + const projectService = projectSystem.createProjectService(host); + projectService.openClientFile(f.path); + const program = projectService.inferredProjects[0].getLanguageService().getProgram(); + const sourceFile = program.getSourceFile(f.path); + const context: RefactorContext = { + cancellationToken: { throwIfCancellationRequested() { }, isCancellationRequested() { return false; } }, + newLineCharacter, + program, + file: sourceFile, + startPosition: selectionRange.start, + endPosition: selectionRange.end, + rulesProvider: getRuleProvider() + }; + const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); + assert.equal(rangeToExtract.errors, undefined, rangeToExtract.errors && "Range error: " + rangeToExtract.errors[0].messageText); + const infos = refactor.extractSymbol.getAvailableActions(context); + const actions = find(infos, info => info.description === description.message).actions; + const data: string[] = []; + data.push(`// ==ORIGINAL==`); + data.push(sourceFile.text); + for (const action of actions) { + const { renameLocation, edits } = refactor.extractSymbol.getEditsForAction(context, action.name); + assert.lengthOf(edits, 1); + data.push(`// ==SCOPE::${action.description}==`); + 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); + }); + }); + } + + export function testExtractSymbolFailed(caption: string, text: string, description: DiagnosticMessage) { + it(caption, () => { + const t = extractTest(text); + const selectionRange = t.ranges.get("selection"); + if (!selectionRange) { + throw new Error(`Test ${caption} does not specify selection range`); + } + const f = { + path: "/a.ts", + content: t.source + }; + const host = projectSystem.createServerHost([f, projectSystem.libFile]); + const projectService = projectSystem.createProjectService(host); + projectService.openClientFile(f.path); + const program = projectService.inferredProjects[0].getLanguageService().getProgram(); + const sourceFile = program.getSourceFile(f.path); + const context: RefactorContext = { + cancellationToken: { throwIfCancellationRequested() { }, isCancellationRequested() { return false; } }, + newLineCharacter, + program, + file: sourceFile, + startPosition: selectionRange.start, + endPosition: selectionRange.end, + rulesProvider: getRuleProvider() + }; + const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); + assert.isUndefined(rangeToExtract.errors, rangeToExtract.errors && "Range error: " + rangeToExtract.errors[0].messageText); + const infos = refactor.extractSymbol.getAvailableActions(context); + assert.isUndefined(find(infos, info => info.description === description.message)); + }); + } +} \ No newline at end of file From cb6037b563e2908fd212ac5b7e82241c63762a48 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 27 Sep 2017 10:24:52 -0700 Subject: [PATCH 6/9] Forbid extraction of constants to class scopes in JS --- src/harness/unittests/extractConstants.ts | 4 +-- src/services/refactors/extractSymbol.ts | 30 +++++++++++-------- .../extractConstant_Parameters.ts | 12 ++++++++ .../extractConstant_TypeParameters.ts | 10 +++++++ 4 files changed, 42 insertions(+), 14 deletions(-) create mode 100644 tests/baselines/reference/extractConstant/extractConstant_Parameters.ts create mode 100644 tests/baselines/reference/extractConstant/extractConstant_TypeParameters.ts diff --git a/src/harness/unittests/extractConstants.ts b/src/harness/unittests/extractConstants.ts index fb776eb6af36d..e4ec0932a735f 100644 --- a/src/harness/unittests/extractConstants.ts +++ b/src/harness/unittests/extractConstants.ts @@ -51,13 +51,13 @@ namespace ts { } }`); - testExtractConstantFailed("extractConstant_Parameters", + testExtractConstant("extractConstant_Parameters", `function F() { let w = 1; let x = [#|w + 1|]; }`); - testExtractConstantFailed("extractConstant_TypeParameters", + testExtractConstant("extractConstant_TypeParameters", `function F(t: T) { let x = [#|t + 1|]; }`); diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 0c3b372631216..2b7f9e0dcb86f 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -141,6 +141,7 @@ namespace ts.refactor.extractSymbol { export const CannotExtractAmbientBlock = createMessage("Cannot extract code from ambient contexts"); export const CannotAccessVariablesFromNestedScopes = createMessage("Cannot access variables from nested scopes"); export const CannotExtractToOtherFunctionLike = createMessage("Cannot extract method to a function-like scope that is not a function"); + export const CannotExtractToJSClass = createMessage("Cannot extract constant to a class scope in JS"); } enum RangeFacts { @@ -866,21 +867,17 @@ namespace ts.refactor.extractSymbol { const changeTracker = textChanges.ChangeTracker.fromContext(context); if (isClassLike(scope)) { - // always create private method in TypeScript files + Debug.assert(!isJS); // See CannotExtractToJSClass const modifiers: Modifier[] = []; - if (!isJS) { - modifiers.push(createToken(SyntaxKind.PrivateKeyword)); - } + modifiers.push(createToken(SyntaxKind.PrivateKeyword)); if (rangeFacts & RangeFacts.InStaticRegion) { modifiers.push(createToken(SyntaxKind.StaticKeyword)); } - if (!isJS) { - modifiers.push(createToken(SyntaxKind.ReadonlyKeyword)); - } + modifiers.push(createToken(SyntaxKind.ReadonlyKeyword)); const newVariable = createProperty( /*decorators*/ undefined, - modifiers.length ? modifiers : undefined, + modifiers, localNameText, /*questionToken*/ undefined, variableType, @@ -1195,20 +1192,29 @@ namespace ts.refactor.extractSymbol { const constantErrorsPerScope: Diagnostic[][] = []; const visibleDeclarationsInExtractedRange: Symbol[] = []; - const expressionDiagnostics = + const expressionDiagnostic = isReadonlyArray(targetRange.range) && !(targetRange.range.length === 1 && isExpressionStatement(targetRange.range[0])) - ? ((start, end) => [createFileDiagnostic(sourceFile, start, end - start, Messages.ExpressionExpected)])(firstOrUndefined(targetRange.range).getStart(), lastOrUndefined(targetRange.range).end) - : []; + ? ((start, end) => createFileDiagnostic(sourceFile, start, end - start, Messages.ExpressionExpected))(firstOrUndefined(targetRange.range).getStart(), lastOrUndefined(targetRange.range).end) + : undefined; // initialize results for (const scope of scopes) { usagesPerScope.push({ usages: createMap(), typeParameterUsages: createMap(), substitutions: createMap() }); substitutionsPerScope.push(createMap()); + functionErrorsPerScope.push( isFunctionLikeDeclaration(scope) && scope.kind !== SyntaxKind.FunctionDeclaration ? [createDiagnosticForNode(scope, Messages.CannotExtractToOtherFunctionLike)] : []); - constantErrorsPerScope.push(expressionDiagnostics); + + const constantErrors = []; + if (expressionDiagnostic) { + constantErrors.push(expressionDiagnostic); + } + if (isClassLike(scope) && isInJavaScriptFile(scope)) { + constantErrors.push(createDiagnosticForNode(scope, Messages.CannotExtractToJSClass)); + } + constantErrorsPerScope.push(constantErrors); } const seenUsages = createMap(); diff --git a/tests/baselines/reference/extractConstant/extractConstant_Parameters.ts b/tests/baselines/reference/extractConstant/extractConstant_Parameters.ts new file mode 100644 index 0000000000000..e6c9c474fbcb8 --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_Parameters.ts @@ -0,0 +1,12 @@ +// ==ORIGINAL== +function F() { + let w = 1; + let x = w + 1; +} +// ==SCOPE::Extract to constant in function 'F'== +function F() { + let w = 1; + const newLocal = w + 1; + + let x = /*RENAME*/newLocal; +} \ No newline at end of file diff --git a/tests/baselines/reference/extractConstant/extractConstant_TypeParameters.ts b/tests/baselines/reference/extractConstant/extractConstant_TypeParameters.ts new file mode 100644 index 0000000000000..a323e98817651 --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_TypeParameters.ts @@ -0,0 +1,10 @@ +// ==ORIGINAL== +function F(t: T) { + let x = t + 1; +} +// ==SCOPE::Extract to constant in function 'F'== +function F(t: T) { + const newLocal = t + 1; + + let x = /*RENAME*/newLocal; +} \ No newline at end of file From 13e60bc497b494f7505d498f5c45129a5424a3b2 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 27 Sep 2017 10:35:13 -0700 Subject: [PATCH 7/9] Use resources, rather than string literals, in test baselines --- src/harness/unittests/extractRanges.ts | 18 +++++++++--------- src/services/refactors/extractSymbol.ts | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/harness/unittests/extractRanges.ts b/src/harness/unittests/extractRanges.ts index 55535a6dc44de..fcffffeba9dbe 100644 --- a/src/harness/unittests/extractRanges.ts +++ b/src/harness/unittests/extractRanges.ts @@ -180,7 +180,7 @@ function f() { } `, [ - "Cannot extract range containing conditional return statement." + refactor.extractSymbol.Messages.CannotExtractRangeContainingConditionalReturnStatement.message ]); testExtractRangeFailed("extractRangeFailed2", @@ -199,7 +199,7 @@ function f() { } `, [ - "Cannot extract range containing conditional break or continue statements." + refactor.extractSymbol.Messages.CannotExtractRangeContainingConditionalBreakOrContinueStatements.message ]); testExtractRangeFailed("extractRangeFailed3", @@ -218,7 +218,7 @@ function f() { } `, [ - "Cannot extract range containing conditional break or continue statements." + refactor.extractSymbol.Messages.CannotExtractRangeContainingConditionalBreakOrContinueStatements.message ]); testExtractRangeFailed("extractRangeFailed4", @@ -237,7 +237,7 @@ function f() { } `, [ - "Cannot extract range containing labeled break or continue with target outside of the range." + refactor.extractSymbol.Messages.CannotExtractRangeContainingLabeledBreakOrContinueStatementWithTargetOutsideOfTheRange.message ]); testExtractRangeFailed("extractRangeFailed5", @@ -258,7 +258,7 @@ function f2() { } `, [ - "Cannot extract range containing conditional return statement." + refactor.extractSymbol.Messages.CannotExtractRangeContainingConditionalReturnStatement.message ]); testExtractRangeFailed("extractRangeFailed6", @@ -279,7 +279,7 @@ function f2() { } `, [ - "Cannot extract range containing conditional return statement." + refactor.extractSymbol.Messages.CannotExtractRangeContainingConditionalReturnStatement.message ]); testExtractRangeFailed("extractRangeFailed7", @@ -292,7 +292,7 @@ while (x) { } `, [ - "Cannot extract range containing conditional break or continue statements." + refactor.extractSymbol.Messages.CannotExtractRangeContainingConditionalBreakOrContinueStatements.message ]); testExtractRangeFailed("extractRangeFailed8", @@ -305,7 +305,7 @@ switch (x) { } `, [ - "Cannot extract range containing conditional break or continue statements." + refactor.extractSymbol.Messages.CannotExtractRangeContainingConditionalBreakOrContinueStatements.message ]); testExtractRangeFailed("extractRangeFailed9", @@ -314,6 +314,6 @@ switch (x) { "Cannot extract empty range." ]); - testExtractRangeFailed("extract-method-not-for-token-expression-statement", `[#|a|]`, ["Select more than a single identifier."]); + testExtractRangeFailed("extract-method-not-for-token-expression-statement", `[#|a|]`, [refactor.extractSymbol.Messages.CannotExtractIdentifier.message]); }); } \ No newline at end of file diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 2b7f9e0dcb86f..517fd558569e9 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -117,7 +117,7 @@ namespace ts.refactor.extractSymbol { } // Move these into diagnostic messages if they become user-facing - namespace Messages { + export namespace Messages { function createMessage(message: string): DiagnosticMessage { return { message, code: 0, category: DiagnosticCategory.Message, key: message }; } From e6bfce193c7f5e80421835003a40748df5a53634 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 27 Sep 2017 10:40:12 -0700 Subject: [PATCH 8/9] Add additional TODO about insertion positions --- src/services/refactors/extractSymbol.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 517fd558569e9..75b21e809edac 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -1110,6 +1110,7 @@ namespace ts.refactor.extractSymbol { } // TODO (acasey): need to dig into nested statements + // TODO (acasey): don't insert before pinned comments, directives, or triple-slash references function getNodeToInsertConstantBefore(maxPos: number, scope: Scope): Node { const children = getStatementsOrClassElements(scope); Debug.assert(children.length > 0); // There must be at least one child, since we extracted from one. @@ -1126,6 +1127,7 @@ namespace ts.refactor.extractSymbol { } } + Debug.assert(prevChild !== undefined); return prevChild; } From 386e76543aca28eddd98d514c408f4853e441ba2 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 27 Sep 2017 18:08:35 -0700 Subject: [PATCH 9/9] TODOs for repeated substitution --- src/harness/unittests/extractConstants.ts | 7 +++++++ src/harness/unittests/extractFunctions.ts | 7 +++++++ src/services/refactors/extractSymbol.ts | 6 +++--- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/harness/unittests/extractConstants.ts b/src/harness/unittests/extractConstants.ts index e4ec0932a735f..e2c64afb5261f 100644 --- a/src/harness/unittests/extractConstants.ts +++ b/src/harness/unittests/extractConstants.ts @@ -62,6 +62,13 @@ namespace ts { let x = [#|t + 1|]; }`); +// TODO (acasey): handle repeated substitution +// testExtractConstant("extractConstant_RepeatedSubstitution", +// `namespace X { +// export const j = 10; +// export const y = [#|j * j|]; +// }`); + testExtractConstantFailed("extractConstant_BlockScopes_Dependencies", `for (let i = 0; i < 10; i++) { for (let j = 0; j < 10; j++) { diff --git a/src/harness/unittests/extractFunctions.ts b/src/harness/unittests/extractFunctions.ts index 2497b2eec5116..4e7cba9606c2c 100644 --- a/src/harness/unittests/extractFunctions.ts +++ b/src/harness/unittests/extractFunctions.ts @@ -364,6 +364,13 @@ function parsePrimaryExpression(): any { `function F() { [#|function G() { }|] }`); + +// TODO (acasey): handle repeated substitution +// testExtractMethod("extractMethod_RepeatedSubstitution", +// `namespace X { +// export const j = 10; +// export const y = [#|j * j|]; +// }`); }); function testExtractMethod(caption: string, text: string) { diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 75b21e809edac..375acb7e5857d 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -1382,9 +1382,9 @@ namespace ts.refactor.extractSymbol { if (symbolId) { for (let i = 0; i < scopes.length; i++) { // push substitution from map to map to simplify rewriting - const substitition = substitutionsPerScope[i].get(symbolId); - if (substitition) { - usagesPerScope[i].substitutions.set(getNodeId(n).toString(), substitition); + const substitution = substitutionsPerScope[i].get(symbolId); + if (substitution) { + usagesPerScope[i].substitutions.set(getNodeId(n).toString(), substitution); } } }