From be27a1afeca9f5505297f0e1575bd7071172d6af Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Fri, 30 Jun 2017 13:23:27 -0700 Subject: [PATCH 01/55] Rebased EM WIP --- Jakefile.js | 1 + src/compiler/checker.ts | 7 +- src/compiler/types.ts | 3 +- src/harness/fourslash.ts | 54 ++ src/harness/unittests/extractMethods.ts | 590 ++++++++++++ src/services/codefixes/importFixes.ts | 2 +- src/services/refactors/extractMethod.ts | 847 ++++++++++++++++++ src/services/refactors/refactors.ts | 1 + src/services/textChanges.ts | 59 +- src/services/types.ts | 1 - .../reference/extractMethod/extractMethod1.js | 98 ++ .../extractMethod/extractMethod10.js | 70 ++ .../extractMethod/extractMethod11.js | 86 ++ .../extractMethod/extractMethod12.js | 36 + .../reference/extractMethod/extractMethod2.js | 85 ++ .../reference/extractMethod/extractMethod3.js | 80 ++ .../reference/extractMethod/extractMethod4.js | 90 ++ .../reference/extractMethod/extractMethod5.js | 98 ++ .../reference/extractMethod/extractMethod6.js | 101 +++ .../reference/extractMethod/extractMethod7.js | 111 +++ .../reference/extractMethod/extractMethod8.js | 57 ++ .../reference/extractMethod/extractMethod9.js | 65 ++ tests/baselines/reference/extractMethod1.js | 98 ++ tests/baselines/reference/extractMethod10.js | 70 ++ tests/baselines/reference/extractMethod11.js | 86 ++ tests/baselines/reference/extractMethod12.js | 36 + tests/baselines/reference/extractMethod2.js | 85 ++ tests/baselines/reference/extractMethod3.js | 80 ++ tests/baselines/reference/extractMethod4.js | 90 ++ tests/baselines/reference/extractMethod5.js | 98 ++ tests/baselines/reference/extractMethod6.js | 101 +++ tests/baselines/reference/extractMethod7.js | 111 +++ tests/baselines/reference/extractMethod8.js | 57 ++ tests/baselines/reference/extractMethod9.js | 65 ++ tests/cases/fourslash/extractMethod1.ts | 17 + tests/cases/fourslash/fourslash.ts | 5 + 36 files changed, 3517 insertions(+), 24 deletions(-) create mode 100644 src/harness/unittests/extractMethods.ts create mode 100644 src/services/refactors/extractMethod.ts create mode 100644 tests/baselines/reference/extractMethod/extractMethod1.js create mode 100644 tests/baselines/reference/extractMethod/extractMethod10.js create mode 100644 tests/baselines/reference/extractMethod/extractMethod11.js create mode 100644 tests/baselines/reference/extractMethod/extractMethod12.js create mode 100644 tests/baselines/reference/extractMethod/extractMethod2.js create mode 100644 tests/baselines/reference/extractMethod/extractMethod3.js create mode 100644 tests/baselines/reference/extractMethod/extractMethod4.js create mode 100644 tests/baselines/reference/extractMethod/extractMethod5.js create mode 100644 tests/baselines/reference/extractMethod/extractMethod6.js create mode 100644 tests/baselines/reference/extractMethod/extractMethod7.js create mode 100644 tests/baselines/reference/extractMethod/extractMethod8.js create mode 100644 tests/baselines/reference/extractMethod/extractMethod9.js create mode 100644 tests/baselines/reference/extractMethod1.js create mode 100644 tests/baselines/reference/extractMethod10.js create mode 100644 tests/baselines/reference/extractMethod11.js create mode 100644 tests/baselines/reference/extractMethod12.js create mode 100644 tests/baselines/reference/extractMethod2.js create mode 100644 tests/baselines/reference/extractMethod3.js create mode 100644 tests/baselines/reference/extractMethod4.js create mode 100644 tests/baselines/reference/extractMethod5.js create mode 100644 tests/baselines/reference/extractMethod6.js create mode 100644 tests/baselines/reference/extractMethod7.js create mode 100644 tests/baselines/reference/extractMethod8.js create mode 100644 tests/baselines/reference/extractMethod9.js create mode 100644 tests/cases/fourslash/extractMethod1.ts diff --git a/Jakefile.js b/Jakefile.js index 58779a3d8dc90..c6906526b8ac4 100644 --- a/Jakefile.js +++ b/Jakefile.js @@ -133,6 +133,7 @@ var harnessSources = harnessCoreSources.concat([ "projectErrors.ts", "matchFiles.ts", "initializeTSConfig.ts", + "extractMethods.ts", "printer.ts", "textChanges.ts", "telemetry.ts", diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 0b15527c0622b..1e395ec9dabeb 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -222,11 +222,10 @@ namespace ts { getSuggestionForNonexistentProperty, getSuggestionForNonexistentSymbol, getBaseConstraintOfType, - getJsxNamespace, - resolveNameAtLocation(location: Node, name: string, meaning: SymbolFlags): Symbol | undefined { - location = getParseTreeNode(location); - return resolveName(location, name, meaning, /*nameNotFoundMessage*/ undefined, name); + resolveName(name, location, meaning) { + return resolveName(location, name, meaning, /*nameNotFoundMessage*/ undefined, /*nameArg*/ undefined); }, + getJsxNamespace, }; const tupleTypes: GenericType[] = []; diff --git a/src/compiler/types.ts b/src/compiler/types.ts index e2e14c9fd6b3b..022c426d72500 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -2645,9 +2645,8 @@ namespace ts { * Does not include properties of primitive types. */ /* @internal */ getAllPossiblePropertiesOfType(type: Type): Symbol[]; - + /* @internal */ resolveName(name: string, location: Node, meaning: SymbolFlags): Symbol; /* @internal */ getJsxNamespace(): string; - /* @internal */ resolveNameAtLocation(location: Node, name: string, meaning: SymbolFlags): Symbol | undefined; } export enum NodeBuilderFlags { diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 3e01504b48435..b7352c8229c9a 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -187,6 +187,9 @@ namespace FourSlash { // The current caret position in the active file public currentCaretPosition = 0; + // The position of the end of the current selection, or -1 if nothing is selected + public selectionEnd = -1; + public lastKnownMarker = ""; // The file that's currently 'opened' @@ -433,11 +436,19 @@ namespace FourSlash { public goToPosition(pos: number) { this.currentCaretPosition = pos; + this.selectionEnd = -1; + } + + public select(startMarker: string, endMarker: string) { + const start = this.getMarkerByName(startMarker), end = this.getMarkerByName(endMarker); + this.goToPosition(start.position); + this.selectionEnd = end.position; } public moveCaretRight(count = 1) { this.currentCaretPosition += count; this.currentCaretPosition = Math.min(this.currentCaretPosition, this.getFileContent(this.activeFile.fileName).length); + this.selectionEnd = -1; } // Opens a file given its 0-based index or fileName @@ -2748,6 +2759,25 @@ namespace FourSlash { } } + public verifyRefactorAvailable(negative: boolean, name?: string) { + const selection = { + pos: this.currentCaretPosition, + end: this.selectionEnd === -1 ? this.currentCaretPosition : this.selectionEnd + }; + + let refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, selection); + if (name) { + refactors = refactors.filter(r => r.name === name); + } + const isAvailable = refactors.length > 0; + if (negative && isAvailable) { + this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected no refactor but found some.`); + } + if (!negative && !isAvailable) { + this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected a refactor but found none.`); + } + } + public verifyApplicableRefactorAvailableForRange(negative: boolean) { const ranges = this.getRanges(); if (!(ranges && ranges.length === 1)) { @@ -2764,6 +2794,18 @@ namespace FourSlash { } } + public applyRefactor(name: string, index?: number) { + const range = { pos: this.currentCaretPosition, end: this.selectionEnd === -1 ? this.currentCaretPosition : this.selectionEnd }; + const refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, range); + const refactor = ts.find(refactors, r => r.name === name); + if (!refactor) { + this.raiseError(`The expected refactor: ${name} is not available at the marker location.`); + } + + const actions = this.languageService.getRefactorCodeActions(this.activeFile.fileName, this.formatCodeSettings, range, name); + this.applyCodeAction(this.activeFile.fileName, actions, index || 0); + } + public verifyFileAfterApplyingRefactorAtMarker( markerName: string, expectedContent: string, @@ -3505,6 +3547,10 @@ namespace FourSlashInterface { public file(indexOrName: any, content?: string, scriptKindName?: string): void { this.state.openFile(indexOrName, content, scriptKindName); } + + public select(startMarker: string, endMarker: string) { + this.state.select(startMarker, endMarker); + } } export class VerifyNegatable { @@ -3626,6 +3672,10 @@ namespace FourSlashInterface { public applicableRefactorAvailableForRange() { this.state.verifyApplicableRefactorAvailableForRange(this.negative); } + + public refactorAvailable(name?: string) { + this.state.verifyRefactorAvailable(this.negative, name); + } } export class Verify extends VerifyNegatable { @@ -4017,6 +4067,10 @@ namespace FourSlashInterface { public disableFormatting() { this.state.enableFormatting = false; } + + public applyRefactor(name: string, index?: number) { + this.state.applyRefactor(name, index); + } } export class Debug { diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts new file mode 100644 index 0000000000000..1923d1cc7a623 --- /dev/null +++ b/src/harness/unittests/extractMethods.ts @@ -0,0 +1,590 @@ +/// +/// + +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.extractMethod.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.extractMethod.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." + ]); + + testExtractMethod("extractMethod1", + `namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + let a = 1; + [#| + let y = 5; + let z = x; + a = y; + foo();|] + } + } +}`); + testExtractMethod("extractMethod2", + `namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + [#| + let y = 5; + let z = x; + return foo();|] + } + } +}`); + testExtractMethod("extractMethod3", + `namespace A { + function foo() { + } + namespace B { + function* a(z: number) { + [#| + let y = 5; + yield z; + return foo();|] + } + } +}`); + testExtractMethod("extractMethod4", + `namespace A { + function foo() { + } + namespace B { + async function a(z: number, z1: any) { + [#| + let y = 5; + if (z) { + await z1; + } + return foo();|] + } + } +}`); + testExtractMethod("extractMethod5", + `namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + [#| + let y = 5; + let z = x; + a = y; + foo();|] + } + } +}`); + testExtractMethod("extractMethod6", + `namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + [#| + let y = 5; + let z = x; + a = y; + return foo();|] + } + } +}`); + testExtractMethod("extractMethod7", + `namespace A { + let x = 1; + export namespace C { + export function foo() { + } + } + namespace B { + function a() { + let a = 1; + [#| + let y = 5; + let z = x; + a = y; + return C.foo();|] + } + } +}`); + testExtractMethod("extractMethod8", + `namespace A { + let x = 1; + namespace B { + function a() { + let a1 = 1; + return 1 + [#|a1 + x|] + 100; + } + } +}`); + testExtractMethod("extractMethod9", + `namespace A { + export interface I { x: number }; + namespace B { + function a() { + [#|let a1: I = { x: 1 }; + return a1.x + 10;|] + } + } +}`); + testExtractMethod("extractMethod10", + `namespace A { + export interface I { x: number }; + class C { + a() { + let z = 1; + [#|let a1: I = { x: 1 }; + return a1.x + 10;|] + } + } +}`); + testExtractMethod("extractMethod11", + `namespace A { + let y = 1; + class C { + a() { + let z = 1; + [#|let a1 = { x: 1 }; + y = 10; + z = 42; + return a1.x + 10;|] + } + } +}`); + testExtractMethod("extractMethod12", + `namespace A { + let y = 1; + class C { + b() {} + a() { + let z = 1; + [#|let a1 = { x: 1 }; + y = 10; + z = 42; + this.b(); + return a1.x + 10;|] + } + } +}`); + }); + + + function testExtractMethod(caption: string, text: string) { + it(caption, () => { + Harness.Baseline.runBaseline(`extractMethod/${caption}.js`, () => { + 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]); + 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: -1, + 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.extractRange(result.targetRange, sourceFile, context); + const data: string[] = []; + data.push(`==ORIGINAL==`); + data.push(sourceFile.text); + for (const r of results) { + data.push(`==SCOPE::${r.scopeDescription}==`); + data.push(textChanges.applyChanges(sourceFile.text, r.changes[0].textChanges)); + } + return data.join(newLineCharacter); + }); + }); + } +} diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index c2d26da4392d1..972b84ecc5bd5 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -147,7 +147,7 @@ namespace ts.codefix { } else if (isJsxOpeningLikeElement(token.parent) && token.parent.tagName === token) { // The error wasn't for the symbolAtLocation, it was for the JSX tag itself, which needs access to e.g. `React`. - symbol = checker.getAliasedSymbol(checker.resolveNameAtLocation(token, checker.getJsxNamespace(), SymbolFlags.Value)); + symbol = checker.getAliasedSymbol(checker.resolveName(checker.getJsxNamespace(), token.parent.tagName, SymbolFlags.Value)); symbolName = symbol.name; } else { diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts new file mode 100644 index 0000000000000..8aae08a1a0456 --- /dev/null +++ b/src/services/refactors/extractMethod.ts @@ -0,0 +1,847 @@ +/// +/// + +/* @internal */ +namespace ts.refactor.extractMethod { + export const name = 'extract_method'; + const extractMethod: Refactor = { + name: "Extract method", + description: Diagnostics.Convert_function_to_an_ES2015_class.message, + getAvailableActions, + getEditsForAction, + }; + + registerRefactor(extractMethod); + + /** Compute the associated code actions */ + function getAvailableActions(context: RefactorContext): ApplicableRefactorInfo[] | undefined { + const rangeToExtract = getRangeToExtract(context.file, { start: context.startPosition, length: context.endPosition - context.startPosition }); + const targetRange: TargetRange = rangeToExtract.targetRange; + const extractions = extractRange(targetRange, context.file, context); + + if (extractions.length === 0) { + // No extractions possible + return undefined; + } + + + const actions: RefactorActionInfo[] = []; + let i = 0; + for (const extr of extractions) { + actions.push({ + description: extr.scopeDescription, + name: `scope_${i}` + }); + i++; + } + + return [{ + name, + description: "Extract method/function", + inlineable: true, + actions + }]; + } + + function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined { + const rangeToExtract = getRangeToExtract(context.file, { start: context.startPosition, length: context.endPosition - context.startPosition }); + const targetRange: TargetRange = rangeToExtract.targetRange; + const extractions = extractRange(targetRange, context.file, context); + + let i = 0; + for (const extr of extractions) { + const name = `scope_${i}`; + if (name === actionName) { + return ({ + edits: extr.changes + }); + } + i++; + } + // ? + return undefined; + } + +/* + function isApplicable(context: RefactorContext): boolean { + // Must have something selected + if (context.endPosition === undefined || context.endPosition <= context.startPosition) { + return false; + } + return true; + } +*/ + + // TODO: put into diagnostic messages + namespace Messages { + function m(message: string): DiagnosticMessage { + return { message, code: 0, category: DiagnosticCategory.Message, key: message }; + } + + // TODO provide more information in errors + export const CannotExtractFunction: DiagnosticMessage = m("Cannot extract function."); + export const StatementOrExpressionExpected: DiagnosticMessage = m("Statement or expression expected."); + export const CannotExtractRangeContainingConditionalBreakOrContinueStatements: DiagnosticMessage = m("Cannot extract range containing conditional break or continue statements."); + export const CannotExtractRangeContainingConditionalReturnStatement: DiagnosticMessage = m("Cannot extract range containing conditional return statement."); + export const CannotExtractRangeContainingLabeledBreakOrContinueStatementWithTargetOutsideOfTheRange: DiagnosticMessage = m("Cannot extract range containing labeled break or continue with target outside of the range."); + export const CannotExtractRangeThatContainsWritesToReferencesLocatedOutsideOfTheTargetRangeInGenerators: DiagnosticMessage = m("Cannot extract range containing writes to references located outside of the target range in generators."); + export const TypeWillNotBeVisibleInTheNewScope = m("Type will not visible in the new scope."); + export const FunctionWillNotBeVisibleInTheNewScope = m("Function will not visible in the new scope."); + } + + export enum RangeFacts { + None = 0, + HasReturn = 1 << 0, + IsGenerator = 1 << 1, + IsAsyncFunction = 1 << 2, + UsesThis = 1 << 3 + } + + /** + * Represents an expression or a list of statements that should be extracted with some extra information + */ + export interface TargetRange { + readonly range: Expression | Statement[]; + readonly facts: RangeFacts; + } + + /** + * Result of 'getRangeToExtract' operation: contains either a range or a list of errors + */ + export interface RangeToExtract { + readonly targetRange?: TargetRange; + readonly errors?: Diagnostic[]; + } + + /* + * Scopes that can store newly extracted method + */ + export type Scope = FunctionLikeDeclaration | SourceFile | ModuleBlock | ClassLikeDeclaration; + + + /** + * Result of 'extractRange' operation for a specific scope. + * Stores either a list of changes that should be applied to extract a range or a list of errors + */ + export interface ExtractResultForScope { + readonly scope: Scope; + readonly scopeDescription: string; + readonly changes?: FileTextChanges[]; + readonly errors?: Diagnostic[]; + } + + export function getRangeToExtract(sourceFile: SourceFile, span: TextSpan): RangeToExtract { + let start = getParentNodeInSpan(getTokenAtPosition(sourceFile, span.start, /*includeJsDocComment*/ false), sourceFile, span); + let end = getParentNodeInSpan(findTokenOnLeftOfPosition(sourceFile, textSpanEnd(span)), sourceFile, span); + + let facts = RangeFacts.None; + + if (!start || !end) { + // cannot find either start or end node + return { errors: [createFileDiagnostic(sourceFile, span.start, span.length, Messages.CannotExtractFunction)] }; + } + if (start.parent !== end.parent) { + // handle cases like 1 + [2 + 3] + 4 + // user selection is marked with []. + // in this case 2 + 3 does not belong to the same tree node + // instead the shape of the tree looks like this: + // + + // / \ + // + 4 + // / \ + // + 3 + // / \ + // 1 2 + // in this case there is no such one node that covers ends of selection and is located inside the selection + // to handle this we check if both start and end of the selection belong to some binary operation + // and start node is parented by the parent of the end node (because binary operators are left associative) + // if this is the case - expand the selection to the entire parent of end node (in this case it will be [1 + 2 + 3] + 4) + const startParent = skipParentheses(start.parent); + const endParent = skipParentheses(end.parent); + if (isBinaryExpression(startParent) && isBinaryExpression(endParent) && isNodeDescendantOf(startParent, endParent)) { + start = end = endParent; + } + else { + // start and end nodes belong to different subtrees + return { errors: [createFileDiagnostic(sourceFile, span.start, span.length, Messages.CannotExtractFunction)] }; + } + } + if (start !== end) { + // start and end should be statements and parent should be either block or a source file + if (!isBlockLike(start.parent)) { + return { errors: [createFileDiagnostic(sourceFile, span.start, span.length, Messages.CannotExtractFunction)] }; + } + const statements: Statement[] = []; + for (const n of (start.parent).statements) { + if (n === start || statements.length) { + const errors = checkNode(n); + if (errors) { + return { errors }; + } + statements.push(n); + } + if (n === end) { + break; + } + } + return { targetRange: { range: statements, facts } }; + } + else { + const errors = checkNode(start); + if (errors) { + return { errors }; + } + const range = isStatement(start) + ? [start] + : start; + return { targetRange: { range, facts } }; + } + + function checkNode(n: Node): Diagnostic[] { + const enum PermittedJumps { + None = 0, + Break = 1 << 0, + Continue = 1 << 1, + Return = 1 << 2 + } + if (!isStatement(n) && !isExpression(n)) { + return [createDiagnosticForNode(n, Messages.StatementOrExpressionExpected)]; + } + + let errors: Diagnostic[]; + let permittedJumps = PermittedJumps.Return; + let seenLabels: string[]; + + visit(n); + + return errors; + + function visit(n: Node) { + if (errors) { + // already found an error - can stop now + return true; + } + if (!n || isFunctionLike(n) || isClassLike(n)) { + // do not dive into functions or classes + return false; + } + const savedPermittedJumps = permittedJumps; + if (n.parent) { + switch (n.parent.kind) { + case SyntaxKind.IfStatement: + if ((n.parent).thenStatement === n || (n.parent).elseStatement === n) { + // forbid all jumps inside thenStatement or elseStatement + permittedJumps = PermittedJumps.None; + } + break; + case SyntaxKind.TryStatement: + if ((n.parent).tryBlock === n) { + // forbid all jumps inside try blocks + permittedJumps = PermittedJumps.None; + } + else if ((n.parent).finallyBlock === n) { + // allow unconditional returns from finally blocks + permittedJumps = PermittedJumps.Return; + } + break; + case SyntaxKind.CatchClause: + if ((n.parent).block === n) { + // forbid all jumps inside the block of catch clause + permittedJumps = PermittedJumps.None; + } + break; + case SyntaxKind.CaseClause: + if ((n).expression !== n) { + // allow unlabeled break inside case clauses + permittedJumps |= PermittedJumps.Break; + } + break; + default: + if (isIterationStatement(n.parent, /*lookInLabeledStatements*/ false)) { + if ((n.parent).statement === n) { + // allow unlabeled break/continue inside loops + permittedJumps |= PermittedJumps.Break | PermittedJumps.Continue; + } + } + break; + } + } + switch (n.kind) { + case SyntaxKind.ThisType: + case SyntaxKind.ThisKeyword: + facts |= RangeFacts.UsesThis; + break; + case SyntaxKind.LabeledStatement: + { + const label = (n).label; + (seenLabels || (seenLabels = [])).push(label.text); + forEachChild(n, visit); + seenLabels.pop(); + break; + } + case SyntaxKind.BreakStatement: + case SyntaxKind.ContinueStatement: + { + const label = (n).label; + if (label) { + if (!contains(seenLabels, label.text)) { + // attempts to jump to label that is not in range to be extracted + (errors || (errors = [])).push(createDiagnosticForNode(n, Messages.CannotExtractRangeContainingLabeledBreakOrContinueStatementWithTargetOutsideOfTheRange)); + } + } + else { + if (!(permittedJumps & (SyntaxKind.BreakStatement ? PermittedJumps.Break : PermittedJumps.Continue))) { + // attempt to break or continue in a forbidden context + (errors || (errors = [])).push(createDiagnosticForNode(n, Messages.CannotExtractRangeContainingConditionalBreakOrContinueStatements)); + } + } + break; + } + case SyntaxKind.AwaitExpression: + facts |= RangeFacts.IsAsyncFunction; + break; + case SyntaxKind.YieldExpression: + facts |= RangeFacts.IsGenerator; + break; + case SyntaxKind.ReturnStatement: + if (permittedJumps & PermittedJumps.Return) { + facts |= RangeFacts.HasReturn; + } + else { + (errors || (errors = [])).push(createDiagnosticForNode(n, Messages.CannotExtractRangeContainingConditionalReturnStatement)); + } + break; + default: + forEachChild(n, visit); + break; + } + + permittedJumps = savedPermittedJumps; + } + } + } + + export function collectEnclosingScopes(range: TargetRange) { + let current: Node = isArray(range.range) ? firstOrUndefined(range.range) : range.range; + // 2. collect enclosing scopes + if (range.facts & RangeFacts.UsesThis) { + // if range uses this as keyword or as type inside the class then it can only be extracted to a method of the containing class + const containingClass = getContainingClass(current); + if (containingClass) { + return [containingClass]; + } + } + + const scopes: Scope[] = []; + while (current) { + // We want to find the nearest parent where we can place an "equivalent" sibling to the node we're extracting out of. + // Walk up to the closest parent of a place where we can logically put a sibling: + // * Function declaration + // * Class declaration or expression + // * Module or source file + // Note that we don't use isFunctionLike because we don't want to put the extracted closure *inside* a method + if ((current.kind === SyntaxKind.FunctionDeclaration) || isSourceFile(current) || isModuleBlock(current) || isClassLike(current)) { + scopes.push(current as FunctionLikeDeclaration); + } + current = current.parent; + } + return scopes; + } + + export function extractRange(targetRange: TargetRange, sourceFile: SourceFile, context: RefactorContext): ReadonlyArray { + const scopes = collectEnclosingScopes(targetRange); + if (scopes.length === 0) { + return []; + } + const enclosingTextRange = getEnclosingTextRange(targetRange, sourceFile); + const { target, usagesPerScope, errorsPerScope } = collectReadsAndWrites( + targetRange, + scopes, + enclosingTextRange, + sourceFile, + context.program.getTypeChecker()); + + context.cancellationToken.throwIfCancellationRequested(); + return scopes.map((scope, i) => { + const errors = errorsPerScope[i]; + if (errors.length) { + return { + scope, + scopeDescription: getDescriptionForScope(scope), + errors + }; + } + return extractFunctionInScope(target, scope, usagesPerScope[i], targetRange, context); + }); + } + + function getDescriptionForScope(s: Scope) { + if (isFunctionLike(s)) { + switch (s.kind) { + case SyntaxKind.Constructor: + return "constructor"; + case SyntaxKind.FunctionExpression: + return s.name + ? `function expression ${s.name.getText()}` + : "anonymous function expression"; + case SyntaxKind.FunctionDeclaration: + return `function ${s.name.getText()}`; + case SyntaxKind.ArrowFunction: + return "arrow function"; + case SyntaxKind.MethodDeclaration: + return `method ${s.name.getText()}`; + case SyntaxKind.GetAccessor: + return `get ${s.name.getText()}`; + case SyntaxKind.SetAccessor: + return `set ${s.name.getText()}`; + } + } + else if (isModuleBlock(s)) { + return `namespace ${s.parent.name.getText()}`; + } + else if (isClassLike(s)) { + return s.kind === SyntaxKind.ClassDeclaration + ? `class ${s.name.text}` + : s.name.text + ? `class expression ${s.name.text}` + : "anonymous class expression"; + } + else if (isSourceFile(s)) { + return `file '${s.fileName}'`; + } + else { + return "unknown"; + } + } + + export function extractFunctionInScope( + node: Node, + scope: Scope, + { usages: usagesInScope, substitutions }: ScopeUsages, + range: TargetRange, + context: RefactorContext): ExtractResultForScope { + + const checker = context.program.getTypeChecker(); + + const changeTracker = textChanges.ChangeTracker.fromRefactorContext(context); + // TODO: analyze types of usages and introduce type parameters + // TODO: generate unique function name + + const functionName = createIdentifier("newFunction"); + // TODO: derive type parameters from parameter types + const typeParameters: TypeParameterDeclaration[] = undefined; + // TODO: use real type? + const returnType: TypeNode = undefined; + const parameters: ParameterDeclaration[] = []; + const callArguments: Identifier[] = []; + let writes: UsageEntry[]; + usagesInScope.forEach((value, key) => { + const paramDecl = createParameter( + /*decorators*/ undefined, + /*modifiers*/ undefined, + /*dotDotDotToken*/ undefined, + /*name*/ key, + /*questionToken*/ undefined, + createTypeReferenceNode(checker.typeToString(checker.getTypeOfSymbolAtLocation(value.symbol, value.node)), []) + ); + parameters.push(paramDecl); + if (value.usage === Usage.Write) { + (writes || (writes = [])).push(value); + } + callArguments.push(createIdentifier(key)); + }); + + const { body, returnValueProperty } = transformFunctionBody(node); + let newFunction: MethodDeclaration | FunctionDeclaration; + + if (isClassLike(scope)) { + // always create private method + const modifiers: Modifier[] = [createToken(SyntaxKind.PrivateKeyword)]; + if (range.facts & RangeFacts.IsAsyncFunction) { + modifiers.push(createToken(SyntaxKind.AsyncKeyword)); + } + newFunction = createMethod( + /*decorators*/ undefined, + modifiers, + range.facts & RangeFacts.IsGenerator ? createToken(SyntaxKind.AsteriskToken) : undefined, + functionName, + /*questionToken*/ undefined, + typeParameters, + parameters, + returnType, + body + ); + } + else { + newFunction = createFunctionDeclaration( + /*decorators*/ undefined, + range.facts & RangeFacts.IsAsyncFunction ? [createToken(SyntaxKind.AsyncKeyword)] : undefined, + range.facts & RangeFacts.IsGenerator ? createToken(SyntaxKind.AsteriskToken) : undefined, + functionName, + typeParameters, + parameters, + returnType, + body + ); + } + // insert function at the end of the scope + changeTracker.insertNodeBefore(context.file, scope.getLastToken(), newFunction, { prefix: context.newLineCharacter, suffix: context.newLineCharacter }); + + const newNodes: Node[] = []; + // replace range with function call + let call: Expression = createCall( + isClassLike(scope) ? createPropertyAccess(createThis(), functionName) : functionName, + /*typeArguments*/ undefined, + callArguments); + if (range.facts & RangeFacts.IsGenerator) { + call = createYield(createToken(SyntaxKind.AsteriskToken), call); + } + if (range.facts & RangeFacts.IsAsyncFunction) { + call = createAwait(call); + } + + if (writes) { + if (returnValueProperty) { + // has both writes and return, need to create variable declaration to hold return value; + newNodes.push(createVariableStatement( + /*modifiers*/ undefined, + [createVariableDeclaration(returnValueProperty, createKeywordTypeNode(SyntaxKind.AnyKeyword))] + )); + } + + const assignments = getPropertyAssignmentsForWrites(writes); + if (returnValueProperty) { + assignments.push(createShorthandPropertyAssignment(returnValueProperty)); + } + // propagate writes back + newNodes.push(createStatement(createBinary(createObjectLiteral(assignments), SyntaxKind.EqualsToken, call))); + if (returnValueProperty) { + newNodes.push(createReturn(createIdentifier(returnValueProperty))); + } + } + else { + if (range.facts & RangeFacts.HasReturn) { + newNodes.push(createReturn(call)); + } + else if (isArray(range.range)) { + newNodes.push(createStatement(call)); + } + else { + newNodes.push(call); + } + } + changeTracker.replaceNodes( + context.file, + range.range, + newNodes, + { + nodesSeparator: context.newLineCharacter, + suffix: isArray(range.range) ? context.newLineCharacter : undefined // insert newline only when replacing statements + }); + + return { + scope, + scopeDescription: getDescriptionForScope(scope), + changes: changeTracker.getChanges() + }; + + function getPropertyAssignmentsForWrites(writes: UsageEntry[]) { + return writes.map(w => createShorthandPropertyAssignment(w.symbol.name)); + } + + function generateReturnValueProperty() { + // TODO: generate unique property name + return "__return"; + } + + function transformFunctionBody(n: Node) { + if (isBlock(n) && !writes && substitutions.size === 0) { + // already block, no writes to propagate back, no substitutions - can use node as is + return { body: n, returnValueProperty: undefined }; + } + let returnValueProperty: string; + const statements = createNodeArray(isBlock(n) ? n.statements.slice(0) : [isStatement(n) ? n : createStatement(n)]); + // rewrite body if either there are writes that should be propagated back via return statements or there are substitutions + if (writes || substitutions.size) { + const rewrittenStatements = visitNodes(statements, visitor); + if (writes && !(range.facts & RangeFacts.HasReturn)) { + // add return at the end to propagate writes back in case if control flow falls out of the function body + // it is ok to know that range has at least one return since it we only allow unconditional returns + rewrittenStatements.push(createReturn(createObjectLiteral(getPropertyAssignmentsForWrites(writes)))); + } + return { body: createBlock(rewrittenStatements), returnValueProperty: returnValueProperty }; + } + else { + return { body: createBlock(statements), returnValueProperty: undefined }; + } + + function visitor(node: Node): VisitResult { + if (node.kind === SyntaxKind.ReturnStatement && writes) { + const assignments: ObjectLiteralElementLike[] = getPropertyAssignmentsForWrites(writes); + if ((node).expression) { + if (!returnValueProperty) { + returnValueProperty = generateReturnValueProperty(); + } + assignments.push(createPropertyAssignment(returnValueProperty, visitNode((node).expression, visitor))); + } + return createReturn(createObjectLiteral(assignments)); + } + else { + const substitution = substitutions.get(getNodeId(node).toString()); + return substitution || visitEachChild(node, visitor, nullTransformationContext); + } + } + } + } + + const nullTransformationContext: TransformationContext = { + enableEmitNotification: noop, + enableSubstitution: noop, + endLexicalEnvironment: () => undefined, + getCompilerOptions: notImplemented, + getEmitHost: notImplemented, + getEmitResolver: notImplemented, + hoistFunctionDeclaration: noop, + hoistVariableDeclaration: noop, + isEmitNotificationEnabled: notImplemented, + isSubstitutionEnabled: notImplemented, + onEmitNode: noop, + onSubstituteNode: notImplemented, + readEmitHelpers: notImplemented, + requestEmitHelper: noop, + resumeLexicalEnvironment: noop, + startLexicalEnvironment: noop, + suspendLexicalEnvironment: noop + }; + + + function isModuleBlock(n: Node): n is ModuleBlock { + return n.kind === SyntaxKind.ModuleBlock; + } + + function isReadonlyArray(v: any): v is ReadonlyArray { + return isArray(v); + } + + function getEnclosingTextRange(targetRange: TargetRange, sourceFile: SourceFile): TextRange { + return isReadonlyArray(targetRange.range) + ? { pos: targetRange.range[0].getStart(sourceFile), end: targetRange.range[targetRange.range.length - 1].getEnd() } + : targetRange.range; + } + + const enum Usage { + // value should be passed to extracted method + Read = 1, + // value should be passed to extracted method and propagated back + Write = 2 + } + + interface UsageEntry { + readonly usage: Usage; + readonly symbol: Symbol; + readonly node: Node; + } + + interface ScopeUsages { + usages: Map; + substitutions: Map; + } + + function collectReadsAndWrites( + targetRange: TargetRange, + scopes: Scope[], + enclosingTextRange: TextRange, + sourceFile: SourceFile, + checker: TypeChecker) { + + const usagesPerScope: ScopeUsages[] = []; + const substitutionsPerScope: Map[] = []; + const errorsPerScope: Diagnostic[][] = []; + + // initialize results + for (const _ of scopes) { + usagesPerScope.push({ usages: createMap(), substitutions: createMap() }); + substitutionsPerScope.push(createMap()); + errorsPerScope.push([]); + } + const seenUsages = createMap(); + + let valueUsage = Usage.Read; + + const target = isReadonlyArray(targetRange.range) ? createBlock(targetRange.range) : targetRange.range; + + forEachChild(target, collectUsages); + + return { target, usagesPerScope, errorsPerScope }; + + function collectUsages(n: Node) { + if (isAssignmentExpression(n)) { + const savedValueUsage = valueUsage; + // use 'write' as default usage for values + valueUsage = Usage.Write; + collectUsages(n.left); + valueUsage = savedValueUsage; + + collectUsages(n.right); + } + else if (isUnaryExpressionWithWrite(n)) { + const savedValueUsage = valueUsage; + valueUsage = Usage.Write; + collectUsages(n.operand); + valueUsage = savedValueUsage; + } + else if (isIdentifier(n)) { + if (!n.parent) { + return; + } + if (isQualifiedName(n.parent) && n !== n.parent.left) { + return; + } + if ((isPropertyAccessExpression(n.parent) || isElementAccessExpression(n.parent)) && n !== n.parent.expression) { + return; + } + recordUsage(n, valueUsage, /*isTypeNode*/ isPartOfTypeNode(n)); + } + else { + forEachChild(n, collectUsages); + } + } + + function recordUsage(n: Identifier, usage: Usage, isTypeNode: boolean) { + const symbolId = recordUsagebySymbol(n, usage, isTypeNode); + 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); + } + } + } + } + + function recordUsagebySymbol(n: Identifier, usage: Usage, isTypeName: boolean) { + const symbol = checker.getSymbolAtLocation(n); + if (!symbol) { + // cannot find symbol - do nothing + return undefined; + } + const symbolId = getSymbolId(symbol).toString(); + const lastUsage = seenUsages.get(symbolId); + // there are two kinds of value usages + // - reads - if range contains a read from the value located outside of the range then value should be passed as a parameter + // - writes - if range contains a write to a value located outside the range the value should be passed as a parameter and + // returned as a return value + // 'write' case is a superset of 'read' so if we already have processed 'write' of some symbol there is not need to handle 'read' + // since all information is already recorded + if (lastUsage && lastUsage >= usage) { + return symbolId; + } + + seenUsages.set(symbolId, usage); + if (lastUsage) { + // if we get here this means that we are trying to handle 'write' and 'read' was already processed + // walk scopes and update existing records. + for (const perScope of usagesPerScope) { + const prevEntry = perScope.usages.get(n.text); + if (prevEntry) { + perScope.usages.set(n.text, { usage, symbol, node: n }); + } + } + return symbolId; + } + // find first declaration in this file + const declInFile = find(symbol.getDeclarations(), d => d.getSourceFile() === sourceFile); + if (!declInFile) { + return undefined; + } + if (rangeContainsRange(enclosingTextRange, declInFile)) { + // declaration is located in range to be extracted - do nothing + return undefined; + } + 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(n, Messages.CannotExtractRangeThatContainsWritesToReferencesLocatedOutsideOfTheTargetRangeInGenerators)); + } + } + for (let i = 0; i < scopes.length; i++) { + const scope = scopes[i]; + const resolvedSymbol = checker.resolveName(symbol.name, scope, symbol.flags); + if (resolvedSymbol === symbol) { + continue; + } + if (!substitutionsPerScope[i].has(symbolId)) { + const substitution = tryReplaceWithQualifiedNameOrPropertyAccess(symbol.exportSymbol || symbol, scope, isTypeName); + if (substitution) { + substitutionsPerScope[i].set(symbolId, substitution); + } + else if (isTypeName) { + errorsPerScope[i].push(createDiagnosticForNode(n, Messages.TypeWillNotBeVisibleInTheNewScope)); + } + else { + usagesPerScope[i].usages.set(n.text, { usage, symbol, node: n }); + } + } + } + return symbolId; + } + + function tryReplaceWithQualifiedNameOrPropertyAccess(s: Symbol, scopeDecl: Node, isTypeNode: boolean): PropertyAccessExpression | EntityName { + if (!s) { + return undefined; + } + if (s.getDeclarations().some(d => d.parent === scopeDecl)) { + return createIdentifier(s.name); + } + const prefix = tryReplaceWithQualifiedNameOrPropertyAccess(s.parent, scopeDecl, isTypeNode); + if (prefix === undefined) { + return undefined; + } + return isTypeNode ? createQualifiedName(prefix, createIdentifier(s.name)) : createPropertyAccess(prefix, s.name); + } + + function isUnaryExpressionWithWrite(n: Node): n is PrefixUnaryExpression | PostfixUnaryExpression { + switch (n.kind) { + case SyntaxKind.PostfixUnaryExpression: + return true; + case SyntaxKind.PrefixUnaryExpression: + return (n).operator === SyntaxKind.PlusPlusToken || + (n).operator === SyntaxKind.MinusMinusToken; + default: + return false; + } + } + } + + function getParentNodeInSpan(n: Node, file: SourceFile, span: TextSpan): Node { + while (n) { + if (!n.parent) { + return undefined; + } + if (isSourceFile(n.parent) || !spanContainsNode(span, n.parent, file)) { + return n; + } + + n = n.parent; + } + } + + function spanContainsNode(span: TextSpan, node: Node, file: SourceFile): boolean { + return textSpanContainsPosition(span, node.getStart(file)) && + node.getEnd() <= textSpanEnd(span); + } + + function isBlockLike(n: Node): n is BlockLike { + switch (n.kind) { + case SyntaxKind.Block: + case SyntaxKind.SourceFile: + case SyntaxKind.ModuleBlock: + case SyntaxKind.CaseClause: + return true; + default: + return false; + } + } +} diff --git a/src/services/refactors/refactors.ts b/src/services/refactors/refactors.ts index 4c4ecb33416b1..3a33ccc83c2b8 100644 --- a/src/services/refactors/refactors.ts +++ b/src/services/refactors/refactors.ts @@ -1 +1,2 @@ /// +/// diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index db949ef63cfbc..71bc6d9b4926c 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -83,14 +83,18 @@ namespace ts.textChanges { delta?: number; } + export interface ChangeNodesOptions { + nodesSeparator?: string; + } + export type ChangeNodeOptions = ConfigurableStartEnd & InsertNodeOptions; interface Change { readonly sourceFile: SourceFile; readonly range: TextRange; readonly useIndentationFromFile?: boolean; - readonly node?: Node; - readonly options?: ChangeNodeOptions; + readonly node?: Node | Node[]; + readonly options?: ChangeNodeOptions & ChangeNodesOptions; } export function getSeparatorCharacter(separator: Token) { @@ -161,6 +165,10 @@ namespace ts.textChanges { return new ChangeTracker(context.newLineCharacter === "\n" ? NewLineKind.LineFeed : NewLineKind.CarriageReturnLineFeed, context.rulesProvider); } + public static fromRefactorContext(context: RefactorContext) { + return new ChangeTracker(context.newLineCharacter === "\n" ? NewLineKind.LineFeed : NewLineKind.CarriageReturnLineFeed, context.rulesProvider); + } + constructor( private readonly newLine: NewLineKind, private readonly rulesProvider: formatting.RulesProvider, @@ -241,6 +249,13 @@ namespace ts.textChanges { return this; } + public replaceNodes(sourceFile: SourceFile, oldNodes: Node | Node[], newNodes: Node | Node[], options: ChangeNodeOptions & ChangeNodesOptions = {}) { + const startPosition = getAdjustedStartPosition(sourceFile, isArray(oldNodes) ? oldNodes[0] : oldNodes, options, Position.Start); + const endPosition = getAdjustedEndPosition(sourceFile, isArray(oldNodes) ? lastOrUndefined(oldNodes) : oldNodes, options); + this.changes.push({ sourceFile, options, useIndentationFromFile: true, node: newNodes, range: { pos: startPosition, end: endPosition } }); + return this; + } + public insertNodeAt(sourceFile: SourceFile, pos: number, newNode: Node, options: InsertNodeOptions = {}) { this.changes.push({ sourceFile, options, node: newNode, range: { pos: pos, end: pos } }); return this; @@ -451,33 +466,45 @@ namespace ts.textChanges { return ""; } const options = change.options || {}; - const nonFormattedText = getNonformattedText(change.node, sourceFile, this.newLine); + let text: string; + const pos = change.range.pos; + const posStartsLine = getLineStartPositionForPosition(pos, sourceFile) === pos; + if (isArray(change.node)) { + const parts = change.node.map(n => this.getFormattedTextOfNode(n, sourceFile, pos, change.useIndentationFromFile, options)); + text = parts.join(change.options.nodesSeparator); + } + else { + text = this.getFormattedTextOfNode(change.node, sourceFile, pos, change.useIndentationFromFile, options); + } + // strip initial indentation (spaces or tabs) if text will be inserted in the middle of the line + // however keep indentation if it is was forced + text = posStartsLine || change.options.indentation !== undefined ? text : text.replace(/^\s+/, ""); + return (options.prefix || "") + text + (options.suffix || ""); + } + + private getFormattedTextOfNode(node: Node, sourceFile: SourceFile, pos: number, useIndentationFromFile: boolean, options: ChangeNodeOptions) { + const nonFormattedText = getNonformattedText(node, sourceFile, this.newLine); if (this.validator) { this.validator(nonFormattedText); } const formatOptions = this.rulesProvider.getFormatOptions(); - const pos = change.range.pos; const posStartsLine = getLineStartPositionForPosition(pos, sourceFile) === pos; const initialIndentation = - change.options.indentation !== undefined - ? change.options.indentation - : change.useIndentationFromFile - ? formatting.SmartIndenter.getIndentation(change.range.pos, sourceFile, formatOptions, posStartsLine || (change.options.prefix === this.newLineCharacter)) + options.indentation !== undefined + ? options.indentation + : useIndentationFromFile + ? formatting.SmartIndenter.getIndentation(pos, sourceFile, formatOptions, posStartsLine || (options.prefix === this.newLineCharacter)) : 0; const delta = - change.options.delta !== undefined - ? change.options.delta - : formatting.SmartIndenter.shouldIndentChildNode(change.node) + options.delta !== undefined + ? options.delta + : formatting.SmartIndenter.shouldIndentChildNode(node) ? formatOptions.indentSize : 0; - let text = applyFormatting(nonFormattedText, sourceFile, initialIndentation, delta, this.rulesProvider); - // strip initial indentation (spaces or tabs) if text will be inserted in the middle of the line - // however keep indentation if it is was forced - text = posStartsLine || change.options.indentation !== undefined ? text : text.replace(/^\s+/, ""); - return (options.prefix || "") + text + (options.suffix || ""); + return applyFormatting(nonFormattedText, sourceFile, initialIndentation, delta, this.rulesProvider); } private static normalize(changes: Change[]): Change[] { diff --git a/src/services/types.ts b/src/services/types.ts index 447029a2f668f..8862bd14e1473 100644 --- a/src/services/types.ts +++ b/src/services/types.ts @@ -265,7 +265,6 @@ namespace ts { isValidBraceCompletionAtPosition(fileName: string, position: number, openingBrace: number): boolean; getCodeFixesAtPosition(fileName: string, start: number, end: number, errorCodes: number[], formatOptions: FormatCodeSettings): CodeAction[]; - getApplicableRefactors(fileName: string, positionOrRaneg: number | TextRange): ApplicableRefactorInfo[]; getEditsForRefactor(fileName: string, formatOptions: FormatCodeSettings, positionOrRange: number | TextRange, refactorName: string, actionName: string): RefactorEditInfo | undefined; diff --git a/tests/baselines/reference/extractMethod/extractMethod1.js b/tests/baselines/reference/extractMethod/extractMethod1.js new file mode 100644 index 0000000000000..699916b0dc229 --- /dev/null +++ b/tests/baselines/reference/extractMethod/extractMethod1.js @@ -0,0 +1,98 @@ +==ORIGINAL== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + let a = 1; + + let y = 5; + let z = x; + a = y; + foo(); + } + } +} +==SCOPE::function a== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + let a = 1; + + newFunction(); + + function newFunction() { + let y = 5; + let z = x; + a = y; + foo(); + } + } + } +} +==SCOPE::namespace B== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + let a = 1; + + ({ a } = newFunction(a)); + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + foo(); + return { a }; + } + } +} +==SCOPE::namespace A== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + let a = 1; + + ({ a } = newFunction(a)); + } + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + foo(); + return { a }; + } +} +==SCOPE::file '/a.ts'== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + let a = 1; + + ({ a } = newFunction(x, a, foo)); + } + } +} +function newFunction(x: any, a: any, foo: any) { + let y = 5; + let z = x; + a = y; + foo(); + return { a }; +} diff --git a/tests/baselines/reference/extractMethod/extractMethod10.js b/tests/baselines/reference/extractMethod/extractMethod10.js new file mode 100644 index 0000000000000..e416a66e262ac --- /dev/null +++ b/tests/baselines/reference/extractMethod/extractMethod10.js @@ -0,0 +1,70 @@ +==ORIGINAL== +namespace A { + export interface I { x: number }; + class C { + a() { + let z = 1; + let a1: I = { x: 1 }; + return a1.x + 10; + } + } +} +==SCOPE::method a== +namespace A { + export interface I { x: number }; + class C { + a() { + let z = 1; + return newFunction(); + + function newFunction() { + let a1: I = { x: 1 }; + return a1.x + 10; + } + } + } +} +==SCOPE::class C== +namespace A { + export interface I { x: number }; + class C { + a() { + let z = 1; + return this.newFunction(); + } + + private newFunction() { + let a1: I = { x: 1 }; + return a1.x + 10; + } + } +} +==SCOPE::namespace A== +namespace A { + export interface I { x: number }; + class C { + a() { + let z = 1; + return newFunction(); + } + } + + function newFunction() { + let a1: I = { x: 1 }; + return a1.x + 10; + } +} +==SCOPE::file '/a.ts'== +namespace A { + export interface I { x: number }; + class C { + a() { + let z = 1; + return newFunction(); + } + } +} +function newFunction() { + let a1: A.I = { x: 1 }; + return a1.x + 10; +} diff --git a/tests/baselines/reference/extractMethod/extractMethod11.js b/tests/baselines/reference/extractMethod/extractMethod11.js new file mode 100644 index 0000000000000..4cbd31d445321 --- /dev/null +++ b/tests/baselines/reference/extractMethod/extractMethod11.js @@ -0,0 +1,86 @@ +==ORIGINAL== +namespace A { + let y = 1; + class C { + a() { + let z = 1; + let a1 = { x: 1 }; + y = 10; + z = 42; + return a1.x + 10; + } + } +} +==SCOPE::method a== +namespace A { + let y = 1; + class C { + a() { + let z = 1; + return newFunction(); + + function newFunction() { + let a1 = { x: 1 }; + y = 10; + z = 42; + return a1.x + 10; + } + } + } +} +==SCOPE::class C== +namespace A { + let y = 1; + class C { + a() { + let z = 1; + var __return: any; + ({ z, __return } = this.newFunction(z)); + return __return; + } + + private newFunction(z: any) { + let a1 = { x: 1 }; + y = 10; + z = 42; + return { z, __return: a1.x + 10 }; + } + } +} +==SCOPE::namespace A== +namespace A { + let y = 1; + class C { + a() { + let z = 1; + var __return: any; + ({ z, __return } = newFunction(z)); + return __return; + } + } + + function newFunction(z: any) { + let a1 = { x: 1 }; + y = 10; + z = 42; + return { z, __return: a1.x + 10 }; + } +} +==SCOPE::file '/a.ts'== +namespace A { + let y = 1; + class C { + a() { + let z = 1; + var __return: any; + ({ y, z, __return } = newFunction(y, z)); + return __return; + } + } +} +function newFunction(y: any, z: any) { + let a1 = { x: 1 }; + y = 10; + z = 42; + return { y, z, __return: a1.x + 10 }; +} diff --git a/tests/baselines/reference/extractMethod/extractMethod12.js b/tests/baselines/reference/extractMethod/extractMethod12.js new file mode 100644 index 0000000000000..da0788a937fcf --- /dev/null +++ b/tests/baselines/reference/extractMethod/extractMethod12.js @@ -0,0 +1,36 @@ +==ORIGINAL== +namespace A { + let y = 1; + class C { + b() {} + a() { + let z = 1; + let a1 = { x: 1 }; + y = 10; + z = 42; + this.b(); + return a1.x + 10; + } + } +} +==SCOPE::class C== +namespace A { + let y = 1; + class C { + b() {} + a() { + let z = 1; + var __return: any; + ({ z, __return } = this.newFunction(z)); + return __return; + } + + private newFunction(z: any) { + let a1 = { x: 1 }; + y = 10; + z = 42; + this.b(); + return { z, __return: a1.x + 10 }; + } + } +} \ No newline at end of file diff --git a/tests/baselines/reference/extractMethod/extractMethod2.js b/tests/baselines/reference/extractMethod/extractMethod2.js new file mode 100644 index 0000000000000..a89fe52f2fb54 --- /dev/null +++ b/tests/baselines/reference/extractMethod/extractMethod2.js @@ -0,0 +1,85 @@ +==ORIGINAL== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + + let y = 5; + let z = x; + return foo(); + } + } +} +==SCOPE::function a== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + + return newFunction(); + + function newFunction() { + let y = 5; + let z = x; + return foo(); + } + } + } +} +==SCOPE::namespace B== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + + return newFunction(); + } + + function newFunction() { + let y = 5; + let z = x; + return foo(); + } + } +} +==SCOPE::namespace A== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + + return newFunction(); + } + } + + function newFunction() { + let y = 5; + let z = x; + return foo(); + } +} +==SCOPE::file '/a.ts'== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + + return newFunction(x, foo); + } + } +} +function newFunction(x: any, foo: any) { + let y = 5; + let z = x; + return foo(); +} diff --git a/tests/baselines/reference/extractMethod/extractMethod3.js b/tests/baselines/reference/extractMethod/extractMethod3.js new file mode 100644 index 0000000000000..847e196130ce5 --- /dev/null +++ b/tests/baselines/reference/extractMethod/extractMethod3.js @@ -0,0 +1,80 @@ +==ORIGINAL== +namespace A { + function foo() { + } + namespace B { + function* a(z: number) { + + let y = 5; + yield z; + return foo(); + } + } +} +==SCOPE::function a== +namespace A { + function foo() { + } + namespace B { + function* a(z: number) { + + return yield* newFunction(); + + function* newFunction() { + let y = 5; + yield z; + return foo(); + } + } + } +} +==SCOPE::namespace B== +namespace A { + function foo() { + } + namespace B { + function* a(z: number) { + + return yield* newFunction(z); + } + + function* newFunction(z: any) { + let y = 5; + yield z; + return foo(); + } + } +} +==SCOPE::namespace A== +namespace A { + function foo() { + } + namespace B { + function* a(z: number) { + + return yield* newFunction(z); + } + } + + function* newFunction(z: any) { + let y = 5; + yield z; + return foo(); + } +} +==SCOPE::file '/a.ts'== +namespace A { + function foo() { + } + namespace B { + function* a(z: number) { + + return yield* newFunction(z, foo); + } + } +} +function* newFunction(z: any, foo: any) { + let y = 5; + yield z; + return foo(); +} diff --git a/tests/baselines/reference/extractMethod/extractMethod4.js b/tests/baselines/reference/extractMethod/extractMethod4.js new file mode 100644 index 0000000000000..25f410eeecbde --- /dev/null +++ b/tests/baselines/reference/extractMethod/extractMethod4.js @@ -0,0 +1,90 @@ +==ORIGINAL== +namespace A { + function foo() { + } + namespace B { + async function a(z: number, z1: any) { + + let y = 5; + if (z) { + await z1; + } + return foo(); + } + } +} +==SCOPE::function a== +namespace A { + function foo() { + } + namespace B { + async function a(z: number, z1: any) { + + return await newFunction(); + + async function newFunction() { + let y = 5; + if (z) { + await z1; + } + return foo(); + } + } + } +} +==SCOPE::namespace B== +namespace A { + function foo() { + } + namespace B { + async function a(z: number, z1: any) { + + return await newFunction(z, z1); + } + + async function newFunction(z: any, z1: any) { + let y = 5; + if (z) { + await z1; + } + return foo(); + } + } +} +==SCOPE::namespace A== +namespace A { + function foo() { + } + namespace B { + async function a(z: number, z1: any) { + + return await newFunction(z, z1); + } + } + + async function newFunction(z: any, z1: any) { + let y = 5; + if (z) { + await z1; + } + return foo(); + } +} +==SCOPE::file '/a.ts'== +namespace A { + function foo() { + } + namespace B { + async function a(z: number, z1: any) { + + return await newFunction(z, z1, foo); + } + } +} +async function newFunction(z: any, z1: any, foo: any) { + let y = 5; + if (z) { + await z1; + } + return foo(); +} diff --git a/tests/baselines/reference/extractMethod/extractMethod5.js b/tests/baselines/reference/extractMethod/extractMethod5.js new file mode 100644 index 0000000000000..135c4ed5f621c --- /dev/null +++ b/tests/baselines/reference/extractMethod/extractMethod5.js @@ -0,0 +1,98 @@ +==ORIGINAL== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + let y = 5; + let z = x; + a = y; + foo(); + } + } +} +==SCOPE::function a== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + newFunction(); + + function newFunction() { + let y = 5; + let z = x; + a = y; + foo(); + } + } + } +} +==SCOPE::namespace B== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + ({ a } = newFunction(a)); + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + foo(); + return { a }; + } + } +} +==SCOPE::namespace A== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + ({ a } = newFunction(a)); + } + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + foo(); + return { a }; + } +} +==SCOPE::file '/a.ts'== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + ({ a } = newFunction(x, a)); + } + } +} +function newFunction(x: any, a: any) { + let y = 5; + let z = x; + a = y; + A.foo(); + return { a }; +} diff --git a/tests/baselines/reference/extractMethod/extractMethod6.js b/tests/baselines/reference/extractMethod/extractMethod6.js new file mode 100644 index 0000000000000..94fcc8e6e310d --- /dev/null +++ b/tests/baselines/reference/extractMethod/extractMethod6.js @@ -0,0 +1,101 @@ +==ORIGINAL== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + let y = 5; + let z = x; + a = y; + return foo(); + } + } +} +==SCOPE::function a== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + return newFunction(); + + function newFunction() { + let y = 5; + let z = x; + a = y; + return foo(); + } + } + } +} +==SCOPE::namespace B== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + var __return: any; + ({ a, __return } = newFunction(a)); + return __return; + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + return { a, __return: foo() }; + } + } +} +==SCOPE::namespace A== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + var __return: any; + ({ a, __return } = newFunction(a)); + return __return; + } + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + return { a, __return: foo() }; + } +} +==SCOPE::file '/a.ts'== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + var __return: any; + ({ a, __return } = newFunction(x, a)); + return __return; + } + } +} +function newFunction(x: any, a: any) { + let y = 5; + let z = x; + a = y; + return { a, __return: A.foo() }; +} diff --git a/tests/baselines/reference/extractMethod/extractMethod7.js b/tests/baselines/reference/extractMethod/extractMethod7.js new file mode 100644 index 0000000000000..c7c3cc8d77a7c --- /dev/null +++ b/tests/baselines/reference/extractMethod/extractMethod7.js @@ -0,0 +1,111 @@ +==ORIGINAL== +namespace A { + let x = 1; + export namespace C { + export function foo() { + } + } + namespace B { + function a() { + let a = 1; + + let y = 5; + let z = x; + a = y; + return C.foo(); + } + } +} +==SCOPE::function a== +namespace A { + let x = 1; + export namespace C { + export function foo() { + } + } + namespace B { + function a() { + let a = 1; + + return newFunction(); + + function newFunction() { + let y = 5; + let z = x; + a = y; + return C.foo(); + } + } + } +} +==SCOPE::namespace B== +namespace A { + let x = 1; + export namespace C { + export function foo() { + } + } + namespace B { + function a() { + let a = 1; + + var __return: any; + ({ a, __return } = newFunction(a)); + return __return; + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + return { a, __return: C.foo() }; + } + } +} +==SCOPE::namespace A== +namespace A { + let x = 1; + export namespace C { + export function foo() { + } + } + namespace B { + function a() { + let a = 1; + + var __return: any; + ({ a, __return } = newFunction(a)); + return __return; + } + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + return { a, __return: C.foo() }; + } +} +==SCOPE::file '/a.ts'== +namespace A { + let x = 1; + export namespace C { + export function foo() { + } + } + namespace B { + function a() { + let a = 1; + + var __return: any; + ({ a, __return } = newFunction(x, a)); + return __return; + } + } +} +function newFunction(x: any, a: any) { + let y = 5; + let z = x; + a = y; + return { a, __return: A.C.foo() }; +} diff --git a/tests/baselines/reference/extractMethod/extractMethod8.js b/tests/baselines/reference/extractMethod/extractMethod8.js new file mode 100644 index 0000000000000..f030b0ea4afac --- /dev/null +++ b/tests/baselines/reference/extractMethod/extractMethod8.js @@ -0,0 +1,57 @@ +==ORIGINAL== +namespace A { + let x = 1; + namespace B { + function a() { + let a1 = 1; + return 1 + a1 + x + 100; + } + } +} +==SCOPE::function a== +namespace A { + let x = 1; + namespace B { + function a() { + let a1 = 1; + return newFunction() + 100; + + function newFunction() { 1 + a1 + x; } + } + } +} +==SCOPE::namespace B== +namespace A { + let x = 1; + namespace B { + function a() { + let a1 = 1; + return newFunction(a1) + 100; + } + + function newFunction(a1: any) { 1 + a1 + x; } + } +} +==SCOPE::namespace A== +namespace A { + let x = 1; + namespace B { + function a() { + let a1 = 1; + return newFunction(a1) + 100; + } + } + + function newFunction(a1: any) { 1 + a1 + x; } +} +==SCOPE::file '/a.ts'== +namespace A { + let x = 1; + namespace B { + function a() { + let a1 = 1; + return newFunction(a1, x) + 100; + } + } +} +function newFunction(a1: any, x: any) { 1 + a1 + x; } diff --git a/tests/baselines/reference/extractMethod/extractMethod9.js b/tests/baselines/reference/extractMethod/extractMethod9.js new file mode 100644 index 0000000000000..fcc5dcd23de07 --- /dev/null +++ b/tests/baselines/reference/extractMethod/extractMethod9.js @@ -0,0 +1,65 @@ +==ORIGINAL== +namespace A { + export interface I { x: number }; + namespace B { + function a() { + let a1: I = { x: 1 }; + return a1.x + 10; + } + } +} +==SCOPE::function a== +namespace A { + export interface I { x: number }; + namespace B { + function a() { + return newFunction(); + + function newFunction() { + let a1: I = { x: 1 }; + return a1.x + 10; + } + } + } +} +==SCOPE::namespace B== +namespace A { + export interface I { x: number }; + namespace B { + function a() { + return newFunction(); + } + + function newFunction() { + let a1: I = { x: 1 }; + return a1.x + 10; + } + } +} +==SCOPE::namespace A== +namespace A { + export interface I { x: number }; + namespace B { + function a() { + return newFunction(); + } + } + + function newFunction() { + let a1: I = { x: 1 }; + return a1.x + 10; + } +} +==SCOPE::file '/a.ts'== +namespace A { + export interface I { x: number }; + namespace B { + function a() { + return newFunction(); + } + } +} +function newFunction() { + let a1: A.I = { x: 1 }; + return a1.x + 10; +} diff --git a/tests/baselines/reference/extractMethod1.js b/tests/baselines/reference/extractMethod1.js new file mode 100644 index 0000000000000..699916b0dc229 --- /dev/null +++ b/tests/baselines/reference/extractMethod1.js @@ -0,0 +1,98 @@ +==ORIGINAL== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + let a = 1; + + let y = 5; + let z = x; + a = y; + foo(); + } + } +} +==SCOPE::function a== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + let a = 1; + + newFunction(); + + function newFunction() { + let y = 5; + let z = x; + a = y; + foo(); + } + } + } +} +==SCOPE::namespace B== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + let a = 1; + + ({ a } = newFunction(a)); + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + foo(); + return { a }; + } + } +} +==SCOPE::namespace A== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + let a = 1; + + ({ a } = newFunction(a)); + } + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + foo(); + return { a }; + } +} +==SCOPE::file '/a.ts'== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + let a = 1; + + ({ a } = newFunction(x, a, foo)); + } + } +} +function newFunction(x: any, a: any, foo: any) { + let y = 5; + let z = x; + a = y; + foo(); + return { a }; +} diff --git a/tests/baselines/reference/extractMethod10.js b/tests/baselines/reference/extractMethod10.js new file mode 100644 index 0000000000000..e416a66e262ac --- /dev/null +++ b/tests/baselines/reference/extractMethod10.js @@ -0,0 +1,70 @@ +==ORIGINAL== +namespace A { + export interface I { x: number }; + class C { + a() { + let z = 1; + let a1: I = { x: 1 }; + return a1.x + 10; + } + } +} +==SCOPE::method a== +namespace A { + export interface I { x: number }; + class C { + a() { + let z = 1; + return newFunction(); + + function newFunction() { + let a1: I = { x: 1 }; + return a1.x + 10; + } + } + } +} +==SCOPE::class C== +namespace A { + export interface I { x: number }; + class C { + a() { + let z = 1; + return this.newFunction(); + } + + private newFunction() { + let a1: I = { x: 1 }; + return a1.x + 10; + } + } +} +==SCOPE::namespace A== +namespace A { + export interface I { x: number }; + class C { + a() { + let z = 1; + return newFunction(); + } + } + + function newFunction() { + let a1: I = { x: 1 }; + return a1.x + 10; + } +} +==SCOPE::file '/a.ts'== +namespace A { + export interface I { x: number }; + class C { + a() { + let z = 1; + return newFunction(); + } + } +} +function newFunction() { + let a1: A.I = { x: 1 }; + return a1.x + 10; +} diff --git a/tests/baselines/reference/extractMethod11.js b/tests/baselines/reference/extractMethod11.js new file mode 100644 index 0000000000000..4cbd31d445321 --- /dev/null +++ b/tests/baselines/reference/extractMethod11.js @@ -0,0 +1,86 @@ +==ORIGINAL== +namespace A { + let y = 1; + class C { + a() { + let z = 1; + let a1 = { x: 1 }; + y = 10; + z = 42; + return a1.x + 10; + } + } +} +==SCOPE::method a== +namespace A { + let y = 1; + class C { + a() { + let z = 1; + return newFunction(); + + function newFunction() { + let a1 = { x: 1 }; + y = 10; + z = 42; + return a1.x + 10; + } + } + } +} +==SCOPE::class C== +namespace A { + let y = 1; + class C { + a() { + let z = 1; + var __return: any; + ({ z, __return } = this.newFunction(z)); + return __return; + } + + private newFunction(z: any) { + let a1 = { x: 1 }; + y = 10; + z = 42; + return { z, __return: a1.x + 10 }; + } + } +} +==SCOPE::namespace A== +namespace A { + let y = 1; + class C { + a() { + let z = 1; + var __return: any; + ({ z, __return } = newFunction(z)); + return __return; + } + } + + function newFunction(z: any) { + let a1 = { x: 1 }; + y = 10; + z = 42; + return { z, __return: a1.x + 10 }; + } +} +==SCOPE::file '/a.ts'== +namespace A { + let y = 1; + class C { + a() { + let z = 1; + var __return: any; + ({ y, z, __return } = newFunction(y, z)); + return __return; + } + } +} +function newFunction(y: any, z: any) { + let a1 = { x: 1 }; + y = 10; + z = 42; + return { y, z, __return: a1.x + 10 }; +} diff --git a/tests/baselines/reference/extractMethod12.js b/tests/baselines/reference/extractMethod12.js new file mode 100644 index 0000000000000..da0788a937fcf --- /dev/null +++ b/tests/baselines/reference/extractMethod12.js @@ -0,0 +1,36 @@ +==ORIGINAL== +namespace A { + let y = 1; + class C { + b() {} + a() { + let z = 1; + let a1 = { x: 1 }; + y = 10; + z = 42; + this.b(); + return a1.x + 10; + } + } +} +==SCOPE::class C== +namespace A { + let y = 1; + class C { + b() {} + a() { + let z = 1; + var __return: any; + ({ z, __return } = this.newFunction(z)); + return __return; + } + + private newFunction(z: any) { + let a1 = { x: 1 }; + y = 10; + z = 42; + this.b(); + return { z, __return: a1.x + 10 }; + } + } +} \ No newline at end of file diff --git a/tests/baselines/reference/extractMethod2.js b/tests/baselines/reference/extractMethod2.js new file mode 100644 index 0000000000000..a89fe52f2fb54 --- /dev/null +++ b/tests/baselines/reference/extractMethod2.js @@ -0,0 +1,85 @@ +==ORIGINAL== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + + let y = 5; + let z = x; + return foo(); + } + } +} +==SCOPE::function a== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + + return newFunction(); + + function newFunction() { + let y = 5; + let z = x; + return foo(); + } + } + } +} +==SCOPE::namespace B== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + + return newFunction(); + } + + function newFunction() { + let y = 5; + let z = x; + return foo(); + } + } +} +==SCOPE::namespace A== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + + return newFunction(); + } + } + + function newFunction() { + let y = 5; + let z = x; + return foo(); + } +} +==SCOPE::file '/a.ts'== +namespace A { + let x = 1; + function foo() { + } + namespace B { + function a() { + + return newFunction(x, foo); + } + } +} +function newFunction(x: any, foo: any) { + let y = 5; + let z = x; + return foo(); +} diff --git a/tests/baselines/reference/extractMethod3.js b/tests/baselines/reference/extractMethod3.js new file mode 100644 index 0000000000000..847e196130ce5 --- /dev/null +++ b/tests/baselines/reference/extractMethod3.js @@ -0,0 +1,80 @@ +==ORIGINAL== +namespace A { + function foo() { + } + namespace B { + function* a(z: number) { + + let y = 5; + yield z; + return foo(); + } + } +} +==SCOPE::function a== +namespace A { + function foo() { + } + namespace B { + function* a(z: number) { + + return yield* newFunction(); + + function* newFunction() { + let y = 5; + yield z; + return foo(); + } + } + } +} +==SCOPE::namespace B== +namespace A { + function foo() { + } + namespace B { + function* a(z: number) { + + return yield* newFunction(z); + } + + function* newFunction(z: any) { + let y = 5; + yield z; + return foo(); + } + } +} +==SCOPE::namespace A== +namespace A { + function foo() { + } + namespace B { + function* a(z: number) { + + return yield* newFunction(z); + } + } + + function* newFunction(z: any) { + let y = 5; + yield z; + return foo(); + } +} +==SCOPE::file '/a.ts'== +namespace A { + function foo() { + } + namespace B { + function* a(z: number) { + + return yield* newFunction(z, foo); + } + } +} +function* newFunction(z: any, foo: any) { + let y = 5; + yield z; + return foo(); +} diff --git a/tests/baselines/reference/extractMethod4.js b/tests/baselines/reference/extractMethod4.js new file mode 100644 index 0000000000000..25f410eeecbde --- /dev/null +++ b/tests/baselines/reference/extractMethod4.js @@ -0,0 +1,90 @@ +==ORIGINAL== +namespace A { + function foo() { + } + namespace B { + async function a(z: number, z1: any) { + + let y = 5; + if (z) { + await z1; + } + return foo(); + } + } +} +==SCOPE::function a== +namespace A { + function foo() { + } + namespace B { + async function a(z: number, z1: any) { + + return await newFunction(); + + async function newFunction() { + let y = 5; + if (z) { + await z1; + } + return foo(); + } + } + } +} +==SCOPE::namespace B== +namespace A { + function foo() { + } + namespace B { + async function a(z: number, z1: any) { + + return await newFunction(z, z1); + } + + async function newFunction(z: any, z1: any) { + let y = 5; + if (z) { + await z1; + } + return foo(); + } + } +} +==SCOPE::namespace A== +namespace A { + function foo() { + } + namespace B { + async function a(z: number, z1: any) { + + return await newFunction(z, z1); + } + } + + async function newFunction(z: any, z1: any) { + let y = 5; + if (z) { + await z1; + } + return foo(); + } +} +==SCOPE::file '/a.ts'== +namespace A { + function foo() { + } + namespace B { + async function a(z: number, z1: any) { + + return await newFunction(z, z1, foo); + } + } +} +async function newFunction(z: any, z1: any, foo: any) { + let y = 5; + if (z) { + await z1; + } + return foo(); +} diff --git a/tests/baselines/reference/extractMethod5.js b/tests/baselines/reference/extractMethod5.js new file mode 100644 index 0000000000000..135c4ed5f621c --- /dev/null +++ b/tests/baselines/reference/extractMethod5.js @@ -0,0 +1,98 @@ +==ORIGINAL== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + let y = 5; + let z = x; + a = y; + foo(); + } + } +} +==SCOPE::function a== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + newFunction(); + + function newFunction() { + let y = 5; + let z = x; + a = y; + foo(); + } + } + } +} +==SCOPE::namespace B== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + ({ a } = newFunction(a)); + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + foo(); + return { a }; + } + } +} +==SCOPE::namespace A== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + ({ a } = newFunction(a)); + } + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + foo(); + return { a }; + } +} +==SCOPE::file '/a.ts'== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + ({ a } = newFunction(x, a)); + } + } +} +function newFunction(x: any, a: any) { + let y = 5; + let z = x; + a = y; + A.foo(); + return { a }; +} diff --git a/tests/baselines/reference/extractMethod6.js b/tests/baselines/reference/extractMethod6.js new file mode 100644 index 0000000000000..94fcc8e6e310d --- /dev/null +++ b/tests/baselines/reference/extractMethod6.js @@ -0,0 +1,101 @@ +==ORIGINAL== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + let y = 5; + let z = x; + a = y; + return foo(); + } + } +} +==SCOPE::function a== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + return newFunction(); + + function newFunction() { + let y = 5; + let z = x; + a = y; + return foo(); + } + } + } +} +==SCOPE::namespace B== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + var __return: any; + ({ a, __return } = newFunction(a)); + return __return; + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + return { a, __return: foo() }; + } + } +} +==SCOPE::namespace A== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + var __return: any; + ({ a, __return } = newFunction(a)); + return __return; + } + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + return { a, __return: foo() }; + } +} +==SCOPE::file '/a.ts'== +namespace A { + let x = 1; + export function foo() { + } + namespace B { + function a() { + let a = 1; + + var __return: any; + ({ a, __return } = newFunction(x, a)); + return __return; + } + } +} +function newFunction(x: any, a: any) { + let y = 5; + let z = x; + a = y; + return { a, __return: A.foo() }; +} diff --git a/tests/baselines/reference/extractMethod7.js b/tests/baselines/reference/extractMethod7.js new file mode 100644 index 0000000000000..c7c3cc8d77a7c --- /dev/null +++ b/tests/baselines/reference/extractMethod7.js @@ -0,0 +1,111 @@ +==ORIGINAL== +namespace A { + let x = 1; + export namespace C { + export function foo() { + } + } + namespace B { + function a() { + let a = 1; + + let y = 5; + let z = x; + a = y; + return C.foo(); + } + } +} +==SCOPE::function a== +namespace A { + let x = 1; + export namespace C { + export function foo() { + } + } + namespace B { + function a() { + let a = 1; + + return newFunction(); + + function newFunction() { + let y = 5; + let z = x; + a = y; + return C.foo(); + } + } + } +} +==SCOPE::namespace B== +namespace A { + let x = 1; + export namespace C { + export function foo() { + } + } + namespace B { + function a() { + let a = 1; + + var __return: any; + ({ a, __return } = newFunction(a)); + return __return; + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + return { a, __return: C.foo() }; + } + } +} +==SCOPE::namespace A== +namespace A { + let x = 1; + export namespace C { + export function foo() { + } + } + namespace B { + function a() { + let a = 1; + + var __return: any; + ({ a, __return } = newFunction(a)); + return __return; + } + } + + function newFunction(a: any) { + let y = 5; + let z = x; + a = y; + return { a, __return: C.foo() }; + } +} +==SCOPE::file '/a.ts'== +namespace A { + let x = 1; + export namespace C { + export function foo() { + } + } + namespace B { + function a() { + let a = 1; + + var __return: any; + ({ a, __return } = newFunction(x, a)); + return __return; + } + } +} +function newFunction(x: any, a: any) { + let y = 5; + let z = x; + a = y; + return { a, __return: A.C.foo() }; +} diff --git a/tests/baselines/reference/extractMethod8.js b/tests/baselines/reference/extractMethod8.js new file mode 100644 index 0000000000000..f030b0ea4afac --- /dev/null +++ b/tests/baselines/reference/extractMethod8.js @@ -0,0 +1,57 @@ +==ORIGINAL== +namespace A { + let x = 1; + namespace B { + function a() { + let a1 = 1; + return 1 + a1 + x + 100; + } + } +} +==SCOPE::function a== +namespace A { + let x = 1; + namespace B { + function a() { + let a1 = 1; + return newFunction() + 100; + + function newFunction() { 1 + a1 + x; } + } + } +} +==SCOPE::namespace B== +namespace A { + let x = 1; + namespace B { + function a() { + let a1 = 1; + return newFunction(a1) + 100; + } + + function newFunction(a1: any) { 1 + a1 + x; } + } +} +==SCOPE::namespace A== +namespace A { + let x = 1; + namespace B { + function a() { + let a1 = 1; + return newFunction(a1) + 100; + } + } + + function newFunction(a1: any) { 1 + a1 + x; } +} +==SCOPE::file '/a.ts'== +namespace A { + let x = 1; + namespace B { + function a() { + let a1 = 1; + return newFunction(a1, x) + 100; + } + } +} +function newFunction(a1: any, x: any) { 1 + a1 + x; } diff --git a/tests/baselines/reference/extractMethod9.js b/tests/baselines/reference/extractMethod9.js new file mode 100644 index 0000000000000..fcc5dcd23de07 --- /dev/null +++ b/tests/baselines/reference/extractMethod9.js @@ -0,0 +1,65 @@ +==ORIGINAL== +namespace A { + export interface I { x: number }; + namespace B { + function a() { + let a1: I = { x: 1 }; + return a1.x + 10; + } + } +} +==SCOPE::function a== +namespace A { + export interface I { x: number }; + namespace B { + function a() { + return newFunction(); + + function newFunction() { + let a1: I = { x: 1 }; + return a1.x + 10; + } + } + } +} +==SCOPE::namespace B== +namespace A { + export interface I { x: number }; + namespace B { + function a() { + return newFunction(); + } + + function newFunction() { + let a1: I = { x: 1 }; + return a1.x + 10; + } + } +} +==SCOPE::namespace A== +namespace A { + export interface I { x: number }; + namespace B { + function a() { + return newFunction(); + } + } + + function newFunction() { + let a1: I = { x: 1 }; + return a1.x + 10; + } +} +==SCOPE::file '/a.ts'== +namespace A { + export interface I { x: number }; + namespace B { + function a() { + return newFunction(); + } + } +} +function newFunction() { + let a1: A.I = { x: 1 }; + return a1.x + 10; +} diff --git a/tests/cases/fourslash/extractMethod1.ts b/tests/cases/fourslash/extractMethod1.ts new file mode 100644 index 0000000000000..af589aa5e34cb --- /dev/null +++ b/tests/cases/fourslash/extractMethod1.ts @@ -0,0 +1,17 @@ +/// + +//// class Foo { +//// someMethod(m: number) { +//// /*start*/var x = m; +//// x = x * 3; +//// var y = 30; +//// var z = y + x;/*end*/ +//// return z; +//// } +//// } + +goTo.select('start', 'end') +verify.refactorAvailable('Extract method'); +debug.printCurrentFileState(); +edit.applyRefactor('Extract method', 1); +debug.printCurrentFileState(); diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index 767701a2af66e..41f2fcc8cc175 100644 --- a/tests/cases/fourslash/fourslash.ts +++ b/tests/cases/fourslash/fourslash.ts @@ -129,6 +129,7 @@ declare namespace FourSlashInterface { position(position: number, fileName?: string): any; file(index: number, content?: string, scriptKindName?: string): any; file(name: string, content?: string, scriptKindName?: string): any; + select(startMarker: string, endMarker: string): void; } class verifyNegatable { private negative; @@ -155,6 +156,8 @@ declare namespace FourSlashInterface { applicableRefactorAvailableAtMarker(markerName: string): void; codeFixDiagnosticsAvailableAtMarkers(markerNames: string[], diagnosticCode?: number): void; applicableRefactorAvailableForRange(): void; + + refactorAvailable(name?: string); } class verify extends verifyNegatable { assertHasRanges(ranges: Range[]): void; @@ -303,6 +306,8 @@ declare namespace FourSlashInterface { moveLeft(count?: number): void; enableFormatting(): void; disableFormatting(): void; + + applyRefactor(name: string, index?: number): void; } class debug { printCurrentParameterHelp(): void; From e79825704f18354bb8e6f926b3f0819569dd5eb7 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 5 Jul 2017 14:19:31 -0700 Subject: [PATCH 02/55] Change refactor api in fourslash --- src/harness/fourslash.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index b7352c8229c9a..87ae1df1825b5 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2794,16 +2794,18 @@ namespace FourSlash { } } - public applyRefactor(name: string, index?: number) { + public applyRefactor(refactorName: string, actionName: string) { const range = { pos: this.currentCaretPosition, end: this.selectionEnd === -1 ? this.currentCaretPosition : this.selectionEnd }; const refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, range); - const refactor = ts.find(refactors, r => r.name === name); + const refactor = ts.find(refactors, r => r.name === refactorName); if (!refactor) { - this.raiseError(`The expected refactor: ${name} is not available at the marker location.`); + this.raiseError(`The expected refactor: ${refactorName} is not available at the marker location.`); } - const actions = this.languageService.getRefactorCodeActions(this.activeFile.fileName, this.formatCodeSettings, range, name); - this.applyCodeAction(this.activeFile.fileName, actions, index || 0); + const editInfo = this.languageService.getEditsForRefactor(this.activeFile.fileName, this.formatCodeSettings, range, refactorName, actionName); + for (const edit of editInfo.edits) { + this.applyEdits(edit.fileName, edit.textChanges); + } } public verifyFileAfterApplyingRefactorAtMarker( @@ -4068,8 +4070,8 @@ namespace FourSlashInterface { this.state.enableFormatting = false; } - public applyRefactor(name: string, index?: number) { - this.state.applyRefactor(name, index); + public applyRefactor(refactorName: string, actionName: string) { + this.state.applyRefactor(refactorName, actionName); } } From bf176a613b04abe214d3110028cb6c19f00ff664 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 5 Jul 2017 14:26:58 -0700 Subject: [PATCH 03/55] Add comments --- src/services/refactors/extractMethod.ts | 74 +++++++++++++++---------- 1 file changed, 45 insertions(+), 29 deletions(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 8aae08a1a0456..bd3a525fb368d 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -5,15 +5,15 @@ namespace ts.refactor.extractMethod { export const name = 'extract_method'; const extractMethod: Refactor = { - name: "Extract method", - description: Diagnostics.Convert_function_to_an_ES2015_class.message, + name: "Extract Method", + description: Diagnostics.Extract_function.message, getAvailableActions, getEditsForAction, }; registerRefactor(extractMethod); - /** Compute the associated code actions */ + /** Compute the associated code actions */ function getAvailableActions(context: RefactorContext): ApplicableRefactorInfo[] | undefined { const rangeToExtract = getRangeToExtract(context.file, { start: context.startPosition, length: context.endPosition - context.startPosition }); const targetRange: TargetRange = rangeToExtract.targetRange; @@ -29,15 +29,15 @@ namespace ts.refactor.extractMethod { let i = 0; for (const extr of extractions) { actions.push({ - description: extr.scopeDescription, + description: formatMessage(extr.scopeDescription, Diagnostics.Extract_function_into_0), name: `scope_${i}` }); i++; } return [{ - name, - description: "Extract method/function", + name: extractMethod.name, + description: extractMethod.description, inlineable: true, actions }]; @@ -62,23 +62,12 @@ namespace ts.refactor.extractMethod { return undefined; } -/* - function isApplicable(context: RefactorContext): boolean { - // Must have something selected - if (context.endPosition === undefined || context.endPosition <= context.startPosition) { - return false; - } - return true; - } -*/ - - // TODO: put into diagnostic messages + // Move these into diagnostic messages if they become user-facing namespace Messages { function m(message: string): DiagnosticMessage { return { message, code: 0, category: DiagnosticCategory.Message, key: message }; } - // TODO provide more information in errors export const CannotExtractFunction: DiagnosticMessage = m("Cannot extract function."); export const StatementOrExpressionExpected: DiagnosticMessage = m("Statement or expression expected."); export const CannotExtractRangeContainingConditionalBreakOrContinueStatements: DiagnosticMessage = m("Cannot extract range containing conditional break or continue statements."); @@ -130,16 +119,28 @@ namespace ts.refactor.extractMethod { readonly errors?: Diagnostic[]; } + /** + * getRangeToExtract takes a span inside a text file and returns either an expression or an array + * of statements representing the minimum set of nodes needed to extract the entire span. This + * process may fail, in which case a set of errors is returned instead (these are currently + * not shown to the user, but can be used by us diagnostically) + */ export function getRangeToExtract(sourceFile: SourceFile, span: TextSpan): RangeToExtract { + // Walk up starting from the the start position until we find a non-SourceFile node that subsumes the selected span. + // This may fail (e.g. you select two statements in the root of a source file) let start = getParentNodeInSpan(getTokenAtPosition(sourceFile, span.start, /*includeJsDocComment*/ false), sourceFile, span); + // Do the same for the ending position let end = getParentNodeInSpan(findTokenOnLeftOfPosition(sourceFile, textSpanEnd(span)), sourceFile, span); + // We'll modify these flags as we walk the tree to collect data + // about what things need to be done as part of the extraction. let facts = RangeFacts.None; - + if (!start || !end) { // cannot find either start or end node return { errors: [createFileDiagnostic(sourceFile, span.start, span.length, Messages.CannotExtractFunction)] }; } + if (start.parent !== end.parent) { // handle cases like 1 + [2 + 3] + 4 // user selection is marked with []. @@ -197,7 +198,8 @@ namespace ts.refactor.extractMethod { return { targetRange: { range, facts } }; } - function checkNode(n: Node): Diagnostic[] { + // Verifies whether we can actually extract this node or not. + function checkNode(n: Node): Diagnostic[] | undefined { const enum PermittedJumps { None = 0, Break = 1 << 0, @@ -266,6 +268,7 @@ namespace ts.refactor.extractMethod { break; } } + switch (n.kind) { case SyntaxKind.ThisType: case SyntaxKind.ThisKeyword: @@ -321,9 +324,13 @@ namespace ts.refactor.extractMethod { } } - export function collectEnclosingScopes(range: TargetRange) { + /** + * Computes possible places we could extract the function into. For example, + * you may be able to extract into a class method *or* local closure *or* namespace function, + * depending on what's in the extracted body. + */ + export function collectEnclosingScopes(range: TargetRange): Scope[] { let current: Node = isArray(range.range) ? firstOrUndefined(range.range) : range.range; - // 2. collect enclosing scopes if (range.facts & RangeFacts.UsesThis) { // if range uses this as keyword or as type inside the class then it can only be extracted to a method of the containing class const containingClass = getContainingClass(current); @@ -348,6 +355,11 @@ namespace ts.refactor.extractMethod { return scopes; } + /** + * Given a piece of text to extract ('targetRange'), computes a list of possible extractions. + * Each returned ExtractResultForScope corresponds to a possible target scope and is either a set of changes + * or an error explaining why we can't extract into that scope. + */ export function extractRange(targetRange: TargetRange, sourceFile: SourceFile, context: RefactorContext): ReadonlyArray { const scopes = collectEnclosingScopes(targetRange); if (scopes.length === 0) { @@ -424,13 +436,19 @@ namespace ts.refactor.extractMethod { const checker = context.program.getTypeChecker(); const changeTracker = textChanges.ChangeTracker.fromRefactorContext(context); - // TODO: analyze types of usages and introduce type parameters - // TODO: generate unique function name - const functionName = createIdentifier("newFunction"); - // TODO: derive type parameters from parameter types + // Make a unique name for the extracted function + let functionNameText = 'newFunction'; + if (scope.locals && scope.locals.has(functionNameText)) { + let i = 1; + while (scope.locals.has(functionNameText = `newFunction_${i}`)) { + i++; + } + } + + const functionName = createIdentifier(functionNameText); + // Currently doesn't get populated, but we might try to infer from this at some point const typeParameters: TypeParameterDeclaration[] = undefined; - // TODO: use real type? const returnType: TypeNode = undefined; const parameters: ParameterDeclaration[] = []; const callArguments: Identifier[] = []; @@ -550,7 +568,6 @@ namespace ts.refactor.extractMethod { } function generateReturnValueProperty() { - // TODO: generate unique property name return "__return"; } @@ -614,7 +631,6 @@ namespace ts.refactor.extractMethod { suspendLexicalEnvironment: noop }; - function isModuleBlock(n: Node): n is ModuleBlock { return n.kind === SyntaxKind.ModuleBlock; } From 0130ebb8bcdcf18564a487ac44b499dd5349921e Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 5 Jul 2017 14:27:05 -0700 Subject: [PATCH 04/55] Better baselines --- .../reference/extractMethod/extractMethod1.js | 6 ++--- .../extractMethod/extractMethod10.js | 15 ------------ .../extractMethod/extractMethod11.js | 23 +++---------------- .../extractMethod/extractMethod12.js | 2 +- 4 files changed, 7 insertions(+), 39 deletions(-) diff --git a/tests/baselines/reference/extractMethod/extractMethod1.js b/tests/baselines/reference/extractMethod/extractMethod1.js index 699916b0dc229..fea213ff4d3dc 100644 --- a/tests/baselines/reference/extractMethod/extractMethod1.js +++ b/tests/baselines/reference/extractMethod/extractMethod1.js @@ -46,7 +46,7 @@ namespace A { ({ a } = newFunction(a)); } - function newFunction(a: any) { + function newFunction(a: number) { let y = 5; let z = x; a = y; @@ -68,7 +68,7 @@ namespace A { } } - function newFunction(a: any) { + function newFunction(a: number) { let y = 5; let z = x; a = y; @@ -89,7 +89,7 @@ namespace A { } } } -function newFunction(x: any, a: any, foo: any) { +function newFunction(x: number, a: number, foo: () => void) { let y = 5; let z = x; a = y; diff --git a/tests/baselines/reference/extractMethod/extractMethod10.js b/tests/baselines/reference/extractMethod/extractMethod10.js index e416a66e262ac..02923b7ab50c7 100644 --- a/tests/baselines/reference/extractMethod/extractMethod10.js +++ b/tests/baselines/reference/extractMethod/extractMethod10.js @@ -9,21 +9,6 @@ namespace A { } } } -==SCOPE::method a== -namespace A { - export interface I { x: number }; - class C { - a() { - let z = 1; - return newFunction(); - - function newFunction() { - let a1: I = { x: 1 }; - return a1.x + 10; - } - } - } -} ==SCOPE::class C== namespace A { export interface I { x: number }; diff --git a/tests/baselines/reference/extractMethod/extractMethod11.js b/tests/baselines/reference/extractMethod/extractMethod11.js index 4cbd31d445321..ee9785216826a 100644 --- a/tests/baselines/reference/extractMethod/extractMethod11.js +++ b/tests/baselines/reference/extractMethod/extractMethod11.js @@ -11,23 +11,6 @@ namespace A { } } } -==SCOPE::method a== -namespace A { - let y = 1; - class C { - a() { - let z = 1; - return newFunction(); - - function newFunction() { - let a1 = { x: 1 }; - y = 10; - z = 42; - return a1.x + 10; - } - } - } -} ==SCOPE::class C== namespace A { let y = 1; @@ -39,7 +22,7 @@ namespace A { return __return; } - private newFunction(z: any) { + private newFunction(z: number) { let a1 = { x: 1 }; y = 10; z = 42; @@ -59,7 +42,7 @@ namespace A { } } - function newFunction(z: any) { + function newFunction(z: number) { let a1 = { x: 1 }; y = 10; z = 42; @@ -78,7 +61,7 @@ namespace A { } } } -function newFunction(y: any, z: any) { +function newFunction(y: number, z: number) { let a1 = { x: 1 }; y = 10; z = 42; diff --git a/tests/baselines/reference/extractMethod/extractMethod12.js b/tests/baselines/reference/extractMethod/extractMethod12.js index da0788a937fcf..056fdf941d20a 100644 --- a/tests/baselines/reference/extractMethod/extractMethod12.js +++ b/tests/baselines/reference/extractMethod/extractMethod12.js @@ -25,7 +25,7 @@ namespace A { return __return; } - private newFunction(z: any) { + private newFunction(z: number) { let a1 = { x: 1 }; y = 10; z = 42; From bbf5cd757fbab202ebc31b6a7439bc844b75b31d Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 5 Jul 2017 14:27:17 -0700 Subject: [PATCH 05/55] Rename test we we can run it independent of unit tests --- .../{extractMethod1.ts => extract-method1.ts} | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) rename tests/cases/fourslash/{extractMethod1.ts => extract-method1.ts} (55%) diff --git a/tests/cases/fourslash/extractMethod1.ts b/tests/cases/fourslash/extract-method1.ts similarity index 55% rename from tests/cases/fourslash/extractMethod1.ts rename to tests/cases/fourslash/extract-method1.ts index af589aa5e34cb..f8804e7897122 100644 --- a/tests/cases/fourslash/extractMethod1.ts +++ b/tests/cases/fourslash/extract-method1.ts @@ -5,13 +5,15 @@ //// /*start*/var x = m; //// x = x * 3; //// var y = 30; -//// var z = y + x;/*end*/ -//// return z; +//// var z = y + x; +//// console.log(z);/*end*/ +//// var q = 10; +//// return q; //// } //// } goTo.select('start', 'end') -verify.refactorAvailable('Extract method'); +verify.refactorAvailable('Extract Method'); debug.printCurrentFileState(); -edit.applyRefactor('Extract method', 1); +edit.applyRefactor('Extract Method', "scope_0"); debug.printCurrentFileState(); From 9dd6befb96b1ca98f6e4b2ce3f1408ee84f64711 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 5 Jul 2017 14:27:25 -0700 Subject: [PATCH 06/55] Change fourslash refactor API --- tests/cases/fourslash/fourslash.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index 41f2fcc8cc175..35a948614739b 100644 --- a/tests/cases/fourslash/fourslash.ts +++ b/tests/cases/fourslash/fourslash.ts @@ -307,7 +307,7 @@ declare namespace FourSlashInterface { enableFormatting(): void; disableFormatting(): void; - applyRefactor(name: string, index?: number): void; + applyRefactor(refactorName: string, actionName: string): void; } class debug { printCurrentParameterHelp(): void; From f21b7ae0ebe543a9eed7e63e1b5dbff3b2643c88 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 5 Jul 2017 16:04:29 -0700 Subject: [PATCH 07/55] Remove invalid assert --- src/services/formatting/formatting.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/services/formatting/formatting.ts b/src/services/formatting/formatting.ts index c55672229baca..fe85f35a9a448 100644 --- a/src/services/formatting/formatting.ts +++ b/src/services/formatting/formatting.ts @@ -706,7 +706,6 @@ namespace ts.formatting { if (isToken(child) && child.kind !== SyntaxKind.JsxText) { // if child node is a token, it does not impact indentation, proceed it using parent indentation scope rules const tokenInfo = formattingScanner.readTokenInfo(child); - Debug.assert(tokenInfo.token.end === child.end, "Token end is child end"); consumeTokenAndAdvanceScanner(tokenInfo, node, parentDynamicIndentation, child); return inheritedIndentation; } From 674b1bf73350a7a6496495fd688d15c2c49b1b0a Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 5 Jul 2017 16:04:39 -0700 Subject: [PATCH 08/55] Fix undefined computation of length --- src/services/refactors/extractMethod.ts | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index bd3a525fb368d..c5363c3b81fb3 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -3,7 +3,7 @@ /* @internal */ namespace ts.refactor.extractMethod { - export const name = 'extract_method'; + export const name = "extract_method"; const extractMethod: Refactor = { name: "Extract Method", description: Diagnostics.Extract_function.message, @@ -23,13 +23,13 @@ namespace ts.refactor.extractMethod { // No extractions possible return undefined; } - - + + const actions: RefactorActionInfo[] = []; let i = 0; for (const extr of extractions) { actions.push({ - description: formatMessage(extr.scopeDescription, Diagnostics.Extract_function_into_0), + description: formatStringFromArgs(Diagnostics.Extract_function_into_0.message, [extr.scopeDescription]), name: `scope_${i}` }); i++; @@ -44,7 +44,8 @@ namespace ts.refactor.extractMethod { } function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined { - const rangeToExtract = getRangeToExtract(context.file, { start: context.startPosition, length: context.endPosition - context.startPosition }); + const length = context.endPosition === undefined ? 0 : context.endPosition - context.startPosition; + const rangeToExtract = getRangeToExtract(context.file, { start: context.startPosition, length }); const targetRange: TargetRange = rangeToExtract.targetRange; const extractions = extractRange(targetRange, context.file, context); @@ -135,9 +136,10 @@ namespace ts.refactor.extractMethod { // We'll modify these flags as we walk the tree to collect data // about what things need to be done as part of the extraction. let facts = RangeFacts.None; - + if (!start || !end) { // cannot find either start or end node + debugger; return { errors: [createFileDiagnostic(sourceFile, span.start, span.length, Messages.CannotExtractFunction)] }; } @@ -438,7 +440,7 @@ namespace ts.refactor.extractMethod { const changeTracker = textChanges.ChangeTracker.fromRefactorContext(context); // Make a unique name for the extracted function - let functionNameText = 'newFunction'; + let functionNameText = "newFunction"; if (scope.locals && scope.locals.has(functionNameText)) { let i = 1; while (scope.locals.has(functionNameText = `newFunction_${i}`)) { @@ -471,7 +473,7 @@ namespace ts.refactor.extractMethod { const { body, returnValueProperty } = transformFunctionBody(node); let newFunction: MethodDeclaration | FunctionDeclaration; - + if (isClassLike(scope)) { // always create private method const modifiers: Modifier[] = [createToken(SyntaxKind.PrivateKeyword)]; From 93674734d8662313c5977fa143e707bc188cb8fe Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 5 Jul 2017 16:04:56 -0700 Subject: [PATCH 09/55] Improved type baselines for EM .js --- tests/baselines/reference/extractMethod/extractMethod2.js | 2 +- tests/baselines/reference/extractMethod/extractMethod3.js | 6 +++--- tests/baselines/reference/extractMethod/extractMethod4.js | 6 +++--- tests/baselines/reference/extractMethod/extractMethod5.js | 6 +++--- tests/baselines/reference/extractMethod/extractMethod6.js | 6 +++--- tests/baselines/reference/extractMethod/extractMethod7.js | 6 +++--- tests/baselines/reference/extractMethod/extractMethod8.js | 6 +++--- 7 files changed, 19 insertions(+), 19 deletions(-) diff --git a/tests/baselines/reference/extractMethod/extractMethod2.js b/tests/baselines/reference/extractMethod/extractMethod2.js index a89fe52f2fb54..28c27993d7165 100644 --- a/tests/baselines/reference/extractMethod/extractMethod2.js +++ b/tests/baselines/reference/extractMethod/extractMethod2.js @@ -78,7 +78,7 @@ namespace A { } } } -function newFunction(x: any, foo: any) { +function newFunction(x: number, foo: () => void) { let y = 5; let z = x; return foo(); diff --git a/tests/baselines/reference/extractMethod/extractMethod3.js b/tests/baselines/reference/extractMethod/extractMethod3.js index 847e196130ce5..e5903ead18199 100644 --- a/tests/baselines/reference/extractMethod/extractMethod3.js +++ b/tests/baselines/reference/extractMethod/extractMethod3.js @@ -38,7 +38,7 @@ namespace A { return yield* newFunction(z); } - function* newFunction(z: any) { + function* newFunction(z: number) { let y = 5; yield z; return foo(); @@ -56,7 +56,7 @@ namespace A { } } - function* newFunction(z: any) { + function* newFunction(z: number) { let y = 5; yield z; return foo(); @@ -73,7 +73,7 @@ namespace A { } } } -function* newFunction(z: any, foo: any) { +function* newFunction(z: number, foo: () => void) { let y = 5; yield z; return foo(); diff --git a/tests/baselines/reference/extractMethod/extractMethod4.js b/tests/baselines/reference/extractMethod/extractMethod4.js index 25f410eeecbde..08a4d447a0813 100644 --- a/tests/baselines/reference/extractMethod/extractMethod4.js +++ b/tests/baselines/reference/extractMethod/extractMethod4.js @@ -42,7 +42,7 @@ namespace A { return await newFunction(z, z1); } - async function newFunction(z: any, z1: any) { + async function newFunction(z: number, z1: any) { let y = 5; if (z) { await z1; @@ -62,7 +62,7 @@ namespace A { } } - async function newFunction(z: any, z1: any) { + async function newFunction(z: number, z1: any) { let y = 5; if (z) { await z1; @@ -81,7 +81,7 @@ namespace A { } } } -async function newFunction(z: any, z1: any, foo: any) { +async function newFunction(z: number, z1: any, foo: () => void) { let y = 5; if (z) { await z1; diff --git a/tests/baselines/reference/extractMethod/extractMethod5.js b/tests/baselines/reference/extractMethod/extractMethod5.js index 135c4ed5f621c..33c5a6e71d83f 100644 --- a/tests/baselines/reference/extractMethod/extractMethod5.js +++ b/tests/baselines/reference/extractMethod/extractMethod5.js @@ -46,7 +46,7 @@ namespace A { ({ a } = newFunction(a)); } - function newFunction(a: any) { + function newFunction(a: number) { let y = 5; let z = x; a = y; @@ -68,7 +68,7 @@ namespace A { } } - function newFunction(a: any) { + function newFunction(a: number) { let y = 5; let z = x; a = y; @@ -89,7 +89,7 @@ namespace A { } } } -function newFunction(x: any, a: any) { +function newFunction(x: number, a: number) { let y = 5; let z = x; a = y; diff --git a/tests/baselines/reference/extractMethod/extractMethod6.js b/tests/baselines/reference/extractMethod/extractMethod6.js index 94fcc8e6e310d..a1bea905048a3 100644 --- a/tests/baselines/reference/extractMethod/extractMethod6.js +++ b/tests/baselines/reference/extractMethod/extractMethod6.js @@ -48,7 +48,7 @@ namespace A { return __return; } - function newFunction(a: any) { + function newFunction(a: number) { let y = 5; let z = x; a = y; @@ -71,7 +71,7 @@ namespace A { } } - function newFunction(a: any) { + function newFunction(a: number) { let y = 5; let z = x; a = y; @@ -93,7 +93,7 @@ namespace A { } } } -function newFunction(x: any, a: any) { +function newFunction(x: number, a: number) { let y = 5; let z = x; a = y; diff --git a/tests/baselines/reference/extractMethod/extractMethod7.js b/tests/baselines/reference/extractMethod/extractMethod7.js index c7c3cc8d77a7c..fe32016b33a0f 100644 --- a/tests/baselines/reference/extractMethod/extractMethod7.js +++ b/tests/baselines/reference/extractMethod/extractMethod7.js @@ -54,7 +54,7 @@ namespace A { return __return; } - function newFunction(a: any) { + function newFunction(a: number) { let y = 5; let z = x; a = y; @@ -79,7 +79,7 @@ namespace A { } } - function newFunction(a: any) { + function newFunction(a: number) { let y = 5; let z = x; a = y; @@ -103,7 +103,7 @@ namespace A { } } } -function newFunction(x: any, a: any) { +function newFunction(x: number, a: number) { let y = 5; let z = x; a = y; diff --git a/tests/baselines/reference/extractMethod/extractMethod8.js b/tests/baselines/reference/extractMethod/extractMethod8.js index f030b0ea4afac..ddbf13a2f4eb7 100644 --- a/tests/baselines/reference/extractMethod/extractMethod8.js +++ b/tests/baselines/reference/extractMethod/extractMethod8.js @@ -29,7 +29,7 @@ namespace A { return newFunction(a1) + 100; } - function newFunction(a1: any) { 1 + a1 + x; } + function newFunction(a1: number) { 1 + a1 + x; } } } ==SCOPE::namespace A== @@ -42,7 +42,7 @@ namespace A { } } - function newFunction(a1: any) { 1 + a1 + x; } + function newFunction(a1: number) { 1 + a1 + x; } } ==SCOPE::file '/a.ts'== namespace A { @@ -54,4 +54,4 @@ namespace A { } } } -function newFunction(a1: any, x: any) { 1 + a1 + x; } +function newFunction(a1: number, x: number) { 1 + a1 + x; } From 1ea9972951ad0af8adcd2cd8044dfb76dde13907 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 5 Jul 2017 16:05:03 -0700 Subject: [PATCH 10/55] Add validation to fourslash test --- tests/cases/fourslash/extract-method1.ts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/tests/cases/fourslash/extract-method1.ts b/tests/cases/fourslash/extract-method1.ts index f8804e7897122..7f5c2ac2d500a 100644 --- a/tests/cases/fourslash/extract-method1.ts +++ b/tests/cases/fourslash/extract-method1.ts @@ -14,6 +14,20 @@ goTo.select('start', 'end') verify.refactorAvailable('Extract Method'); -debug.printCurrentFileState(); edit.applyRefactor('Extract Method', "scope_0"); -debug.printCurrentFileState(); +verify.currentFileContentIs( +`class Foo { + someMethod(m: number) { + this.newFunction(m); + var q = 10; + return q; + } + + private newFunction(m: number) { + var x = m; + x = x * 3; + var y = 30; + var z = y + x; + console.log(z); + } +}`); From 1787fa8dddc4affeac19d2f44dacf14032eb5078 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 5 Jul 2017 16:05:12 -0700 Subject: [PATCH 11/55] Move text to diagnostic messages --- src/compiler/diagnosticMessages.json | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index 1fd434abf4c13..1d78b2a9f860f 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -3647,6 +3647,16 @@ "code": 95002 }, + "Extract function": { + "category": "Message", + "code": 95003 + }, + + "Extract function into '{0}'": { + "category": "Message", + "code": 95004 + }, + "Octal literal types must use ES2015 syntax. Use the syntax '{0}'.": { "category": "Error", "code": 8017 From 4aef5e7837bc3d225842e9addbfd7bcfa8b6526c Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 5 Jul 2017 16:51:55 -0700 Subject: [PATCH 12/55] Properly widen literal param types + emit correct return statements for exprs --- src/services/refactors/extractMethod.ts | 9 +++++--- tests/cases/fourslash/extract-method2.ts | 27 ++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) create mode 100644 tests/cases/fourslash/extract-method2.ts diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index c5363c3b81fb3..e1591f5ca0a09 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -139,7 +139,6 @@ namespace ts.refactor.extractMethod { if (!start || !end) { // cannot find either start or end node - debugger; return { errors: [createFileDiagnostic(sourceFile, span.start, span.length, Messages.CannotExtractFunction)] }; } @@ -456,13 +455,17 @@ namespace ts.refactor.extractMethod { const callArguments: Identifier[] = []; let writes: UsageEntry[]; usagesInScope.forEach((value, key) => { + let type = checker.getTypeOfSymbolAtLocation(value.symbol, value.node); + // Widen the type so we don't emit nonsense annotations like "function fn(x: 3) {" + type = checker.getBaseTypeOfLiteralType(type); + const paramDecl = createParameter( /*decorators*/ undefined, /*modifiers*/ undefined, /*dotDotDotToken*/ undefined, /*name*/ key, /*questionToken*/ undefined, - createTypeReferenceNode(checker.typeToString(checker.getTypeOfSymbolAtLocation(value.symbol, value.node)), []) + createTypeReferenceNode(checker.typeToString(type, node, TypeFormatFlags.NoTruncation), []) ); parameters.push(paramDecl); if (value.usage === Usage.Write) { @@ -579,7 +582,7 @@ namespace ts.refactor.extractMethod { return { body: n, returnValueProperty: undefined }; } let returnValueProperty: string; - const statements = createNodeArray(isBlock(n) ? n.statements.slice(0) : [isStatement(n) ? n : createStatement(n)]); + const statements = createNodeArray(isBlock(n) ? n.statements.slice(0) : [isStatement(n) ? n : createReturn(n)]); // rewrite body if either there are writes that should be propagated back via return statements or there are substitutions if (writes || substitutions.size) { const rewrittenStatements = visitNodes(statements, visitor); diff --git a/tests/cases/fourslash/extract-method2.ts b/tests/cases/fourslash/extract-method2.ts new file mode 100644 index 0000000000000..50efc4c263aa0 --- /dev/null +++ b/tests/cases/fourslash/extract-method2.ts @@ -0,0 +1,27 @@ +/// + +//// namespace NS { +//// class Q { +//// foo() { +//// console.log('100'); +//// const m = 10, j = "hello", k = {x: "what"}; +//// const q = /*start*/m + j + k/*end*/; +//// } +//// } +//// } +goTo.select('start', 'end') +verify.refactorAvailable('Extract Method'); +edit.applyRefactor('Extract Method', "scope_2"); +debug.printCurrentFileState(); +verify.currentFileContentIs( +`namespace NS { + class Q { + foo() { + console.log('100'); + const m = 10, j = "hello", k = {x: "what"}; + const q = newFunction(m, j, k); + } + } +} +function newFunction(m: number, j: string, k: { x: string; }) { return m + j + k; } +`); From b5c5c40d5248a0b378cca08b7d3c75f9bc34b5bd Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 5 Jul 2017 17:03:42 -0700 Subject: [PATCH 13/55] Don't offer to extract lone identifiers --- src/harness/fourslash.ts | 2 +- src/services/refactors/extractMethod.ts | 20 ++++++++++++++++++-- tests/cases/fourslash/extract-method2.ts | 1 - tests/cases/fourslash/extract-method3.ts | 18 ++++++++++++++++++ 4 files changed, 37 insertions(+), 4 deletions(-) create mode 100644 tests/cases/fourslash/extract-method3.ts diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 87ae1df1825b5..a2ff3085a0245 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2765,7 +2765,7 @@ namespace FourSlash { end: this.selectionEnd === -1 ? this.currentCaretPosition : this.selectionEnd }; - let refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, selection); + let refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, selection) || []; if (name) { refactors = refactors.filter(r => r.name === name); } diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index e1591f5ca0a09..0b545a7bccb16 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -16,9 +16,13 @@ namespace ts.refactor.extractMethod { /** Compute the associated code actions */ function getAvailableActions(context: RefactorContext): ApplicableRefactorInfo[] | undefined { const rangeToExtract = getRangeToExtract(context.file, { start: context.startPosition, length: context.endPosition - context.startPosition }); + const targetRange: TargetRange = rangeToExtract.targetRange; - const extractions = extractRange(targetRange, context.file, context); + if (targetRange === undefined) { + return undefined; + } + const extractions = extractRange(targetRange, context.file, context); if (extractions.length === 0) { // No extractions possible return undefined; @@ -77,6 +81,7 @@ namespace ts.refactor.extractMethod { export const CannotExtractRangeThatContainsWritesToReferencesLocatedOutsideOfTheTargetRangeInGenerators: DiagnosticMessage = m("Cannot extract range containing writes to references located outside of the target range in generators."); export const TypeWillNotBeVisibleInTheNewScope = m("Type will not visible in the new scope."); export const FunctionWillNotBeVisibleInTheNewScope = m("Function will not visible in the new scope."); + export const InsufficientSelection = m("Select more than a single identifier."); } export enum RangeFacts { @@ -189,7 +194,7 @@ namespace ts.refactor.extractMethod { return { targetRange: { range: statements, facts } }; } else { - const errors = checkNode(start); + const errors = checkRootNode(start) || checkNode(start); if (errors) { return { errors }; } @@ -199,6 +204,13 @@ namespace ts.refactor.extractMethod { return { targetRange: { range, facts } }; } + function checkRootNode(n: Node): Diagnostic[] | undefined { + if (isIdentifier(n)) { + return [createDiagnosticForNode(n, Messages.InsufficientSelection)]; + } + return undefined; + } + // Verifies whether we can actually extract this node or not. function checkNode(n: Node): Diagnostic[] | undefined { const enum PermittedJumps { @@ -362,6 +374,10 @@ namespace ts.refactor.extractMethod { * or an error explaining why we can't extract into that scope. */ export function extractRange(targetRange: TargetRange, sourceFile: SourceFile, context: RefactorContext): ReadonlyArray { + if (targetRange === undefined) { + return []; + } + const scopes = collectEnclosingScopes(targetRange); if (scopes.length === 0) { return []; diff --git a/tests/cases/fourslash/extract-method2.ts b/tests/cases/fourslash/extract-method2.ts index 50efc4c263aa0..ff558714b9995 100644 --- a/tests/cases/fourslash/extract-method2.ts +++ b/tests/cases/fourslash/extract-method2.ts @@ -12,7 +12,6 @@ goTo.select('start', 'end') verify.refactorAvailable('Extract Method'); edit.applyRefactor('Extract Method', "scope_2"); -debug.printCurrentFileState(); verify.currentFileContentIs( `namespace NS { class Q { diff --git a/tests/cases/fourslash/extract-method3.ts b/tests/cases/fourslash/extract-method3.ts new file mode 100644 index 0000000000000..af543121eebf2 --- /dev/null +++ b/tests/cases/fourslash/extract-method3.ts @@ -0,0 +1,18 @@ +/// + +//// namespace NS { +//// class Q { +//// foo() { +//// console.log('100'); +//// const m = 10, j = "hello", k = {x: "what"}; +//// const q = /*a*/m/*b*/; +//// } +//// } +//// } + +// Don't offer to to 'extract method' a single identifier + +goTo.marker('a'); +verify.not.refactorAvailable('Extract Method'); +goTo.select('a', 'b'); +verify.not.refactorAvailable('Extract Method'); From b1fa49f827f99aa4347edf56aa3ff5db75e3b318 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 5 Jul 2017 17:35:49 -0700 Subject: [PATCH 14/55] Don't fail if span is zero-length --- src/services/refactors/extractMethod.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 0b545a7bccb16..b386a2a7778e5 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -132,6 +132,7 @@ namespace ts.refactor.extractMethod { * not shown to the user, but can be used by us diagnostically) */ export function getRangeToExtract(sourceFile: SourceFile, span: TextSpan): RangeToExtract { + const length = span.length || 0; // Walk up starting from the the start position until we find a non-SourceFile node that subsumes the selected span. // This may fail (e.g. you select two statements in the root of a source file) let start = getParentNodeInSpan(getTokenAtPosition(sourceFile, span.start, /*includeJsDocComment*/ false), sourceFile, span); @@ -144,7 +145,7 @@ namespace ts.refactor.extractMethod { if (!start || !end) { // cannot find either start or end node - return { errors: [createFileDiagnostic(sourceFile, span.start, span.length, Messages.CannotExtractFunction)] }; + return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractFunction)] }; } if (start.parent !== end.parent) { @@ -170,13 +171,13 @@ namespace ts.refactor.extractMethod { } else { // start and end nodes belong to different subtrees - return { errors: [createFileDiagnostic(sourceFile, span.start, span.length, Messages.CannotExtractFunction)] }; + return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractFunction)] }; } } if (start !== end) { // start and end should be statements and parent should be either block or a source file if (!isBlockLike(start.parent)) { - return { errors: [createFileDiagnostic(sourceFile, span.start, span.length, Messages.CannotExtractFunction)] }; + return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractFunction)] }; } const statements: Statement[] = []; for (const n of (start.parent).statements) { From 825237bb9d008c15a40263852deec120bab28352 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 5 Jul 2017 17:42:12 -0700 Subject: [PATCH 15/55] Don't crash when span is empty --- tests/baselines/reference/extractMethod/extractMethod8.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/baselines/reference/extractMethod/extractMethod8.js b/tests/baselines/reference/extractMethod/extractMethod8.js index ddbf13a2f4eb7..77dee04d97650 100644 --- a/tests/baselines/reference/extractMethod/extractMethod8.js +++ b/tests/baselines/reference/extractMethod/extractMethod8.js @@ -16,7 +16,7 @@ namespace A { let a1 = 1; return newFunction() + 100; - function newFunction() { 1 + a1 + x; } + function newFunction() { return 1 + a1 + x; } } } } @@ -29,7 +29,7 @@ namespace A { return newFunction(a1) + 100; } - function newFunction(a1: number) { 1 + a1 + x; } + function newFunction(a1: number) { return 1 + a1 + x; } } } ==SCOPE::namespace A== @@ -42,7 +42,7 @@ namespace A { } } - function newFunction(a1: number) { 1 + a1 + x; } + function newFunction(a1: number) { return 1 + a1 + x; } } ==SCOPE::file '/a.ts'== namespace A { @@ -54,4 +54,4 @@ namespace A { } } } -function newFunction(a1: number, x: number) { 1 + a1 + x; } +function newFunction(a1: number, x: number) { return 1 + a1 + x; } From aa6f3aa63d0cf0764490adb878b29f48b7589a1f Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Tue, 18 Jul 2017 12:37:58 -0700 Subject: [PATCH 16/55] Restore assert and fix synthetic type node creation --- src/services/formatting/formatting.ts | 1 + src/services/refactors/extractMethod.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/services/formatting/formatting.ts b/src/services/formatting/formatting.ts index fe85f35a9a448..c55672229baca 100644 --- a/src/services/formatting/formatting.ts +++ b/src/services/formatting/formatting.ts @@ -706,6 +706,7 @@ namespace ts.formatting { if (isToken(child) && child.kind !== SyntaxKind.JsxText) { // if child node is a token, it does not impact indentation, proceed it using parent indentation scope rules const tokenInfo = formattingScanner.readTokenInfo(child); + Debug.assert(tokenInfo.token.end === child.end, "Token end is child end"); consumeTokenAndAdvanceScanner(tokenInfo, node, parentDynamicIndentation, child); return inheritedIndentation; } diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index b386a2a7778e5..15dc37c482806 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -482,7 +482,7 @@ namespace ts.refactor.extractMethod { /*dotDotDotToken*/ undefined, /*name*/ key, /*questionToken*/ undefined, - createTypeReferenceNode(checker.typeToString(type, node, TypeFormatFlags.NoTruncation), []) + checker.typeToTypeNode(type, node, NodeBuilderFlags.NoTruncation) ); parameters.push(paramDecl); if (value.usage === Usage.Write) { From b7e4451321b6d8514cac80bbbe08f9b5989ba73c Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Tue, 18 Jul 2017 12:40:41 -0700 Subject: [PATCH 17/55] Re-use noop --- src/services/refactors/extractMethod.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 15dc37c482806..71b8a3f575e23 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -636,7 +636,7 @@ namespace ts.refactor.extractMethod { const nullTransformationContext: TransformationContext = { enableEmitNotification: noop, enableSubstitution: noop, - endLexicalEnvironment: () => undefined, + endLexicalEnvironment: noop as () => undefined, getCompilerOptions: notImplemented, getEmitHost: notImplemented, getEmitResolver: notImplemented, From 419a4c21c388097bcda03c6122884d40efb76fd1 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Tue, 18 Jul 2017 16:24:55 -0700 Subject: [PATCH 18/55] Address portion of CR feedback --- src/compiler/core.ts | 10 +- src/compiler/types.ts | 2 +- src/harness/fourslash.ts | 2 +- src/harness/unittests/extractMethods.ts | 2 +- src/services/refactors/extractMethod.ts | 24 ++-- src/services/textChanges.ts | 150 +++++++++++++++++------- 6 files changed, 131 insertions(+), 59 deletions(-) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index 5c7536a825ed6..6fd62f9c02552 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -755,7 +755,7 @@ namespace ts { * Gets the actual offset into an array for a relative offset. Negative offsets indicate a * position offset from the end of the array. */ - function toOffset(array: any[], offset: number) { + function toOffset(array: ReadonlyArray, offset: number) { return offset < 0 ? array.length + offset : offset; } @@ -807,7 +807,7 @@ namespace ts { * Returns the element at a specific offset in an array if non-empty, `undefined` otherwise. * A negative offset indicates the element should be retrieved from the end of the array. */ - export function elementAt(array: T[] | undefined, offset: number): T | undefined { + export function elementAt(array: ReadonlyArray | undefined, offset: number): T | undefined { if (array) { offset = toOffset(array, offset); if (offset < array.length) { @@ -820,14 +820,14 @@ namespace ts { /** * Returns the first element of an array if non-empty, `undefined` otherwise. */ - export function firstOrUndefined(array: T[]): T | undefined { + export function firstOrUndefined(array: ReadonlyArray): T | undefined { return elementAt(array, 0); } /** * Returns the last element of an array if non-empty, `undefined` otherwise. */ - export function lastOrUndefined(array: T[]): T | undefined { + export function lastOrUndefined(array: ReadonlyArray): T | undefined { return elementAt(array, -1); } @@ -1176,6 +1176,8 @@ namespace ts { /** * Tests whether a value is an array. */ + export function isArray(value: T | ReadonlyArray): value is ReadonlyArray; + export function isArray(value: any): value is any[]; export function isArray(value: any): value is any[] { return Array.isArray ? Array.isArray(value) : value instanceof Array; } diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 022c426d72500..db277b5a1a15f 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -2645,7 +2645,7 @@ namespace ts { * Does not include properties of primitive types. */ /* @internal */ getAllPossiblePropertiesOfType(type: Type): Symbol[]; - /* @internal */ resolveName(name: string, location: Node, meaning: SymbolFlags): Symbol; + /* @internal */ resolveName(name: string, location: Node, meaning: SymbolFlags): Symbol | undefined; /* @internal */ getJsxNamespace(): string; } diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index a2ff3085a0245..7e86cd9e148ee 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -616,7 +616,7 @@ namespace FourSlash { this.verifyGoToXPlain(arg0, endMarkerNames, getDefs); } else if (ts.isArray(arg0)) { - const pairs: [string | string[], string | string[]][] = arg0; + const pairs: ReadonlyArray<[string | string[], string | string[]]> = arg0; for (const [start, end] of pairs) { this.verifyGoToXPlain(start, end, getDefs); } diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts index 1923d1cc7a623..6c0a642f03727 100644 --- a/src/harness/unittests/extractMethods.ts +++ b/src/harness/unittests/extractMethods.ts @@ -575,7 +575,7 @@ namespace A { }; const result = refactor.extractMethod.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); assert.equal(result.errors, undefined, "expect no errors"); - const results = refactor.extractMethod.extractRange(result.targetRange, sourceFile, context); + const results = refactor.extractMethod.extractRange(result.targetRange, context); const data: string[] = []; data.push(`==ORIGINAL==`); data.push(sourceFile.text); diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 71b8a3f575e23..a610e7a7f1a51 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -22,7 +22,7 @@ namespace ts.refactor.extractMethod { return undefined; } - const extractions = extractRange(targetRange, context.file, context); + const extractions = extractRange(targetRange, context); if (extractions.length === 0) { // No extractions possible return undefined; @@ -51,7 +51,7 @@ namespace ts.refactor.extractMethod { const length = context.endPosition === undefined ? 0 : context.endPosition - context.startPosition; const rangeToExtract = getRangeToExtract(context.file, { start: context.startPosition, length }); const targetRange: TargetRange = rangeToExtract.targetRange; - const extractions = extractRange(targetRange, context.file, context); + const extractions = extractRange(targetRange, context); let i = 0; for (const extr of extractions) { @@ -374,7 +374,9 @@ namespace ts.refactor.extractMethod { * Each returned ExtractResultForScope corresponds to a possible target scope and is either a set of changes * or an error explaining why we can't extract into that scope. */ - export function extractRange(targetRange: TargetRange, sourceFile: SourceFile, context: RefactorContext): ReadonlyArray { + export function extractRange(targetRange: TargetRange, context: RefactorContext): ReadonlyArray { + const { file: sourceFile } = context; + if (targetRange === undefined) { return []; } @@ -570,14 +572,16 @@ namespace ts.refactor.extractMethod { newNodes.push(call); } } - changeTracker.replaceNodes( - context.file, - range.range, - newNodes, - { - nodesSeparator: context.newLineCharacter, - suffix: isArray(range.range) ? context.newLineCharacter : undefined // insert newline only when replacing statements + + if (isArray(range.range)) { + changeTracker.replaceNodesWithNodes(context.file, range.range, newNodes, { + nodeSeparator: context.newLineCharacter, + suffix: context.newLineCharacter // insert newline only when replacing statements }); + } + else { + changeTracker.replaceNodeWithNodes(context.file, range.range, newNodes, { nodeSeparator: context.newLineCharacter }); + } return { scope, diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 71bc6d9b4926c..c06d329c1f2f0 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -64,7 +64,7 @@ namespace ts.textChanges { */ export type ConfigurableStartEnd = ConfigurableStart & ConfigurableEnd; - export interface InsertNodeOptions { + interface InsertNodeOptions { /** * Text to be inserted before the new node */ @@ -83,18 +83,41 @@ namespace ts.textChanges { delta?: number; } - export interface ChangeNodesOptions { - nodesSeparator?: string; + enum ChangeKind { + Remove, + ReplaceWithSingleNode, + ReplaceWithMultipleNodes } - export type ChangeNodeOptions = ConfigurableStartEnd & InsertNodeOptions; + type Change = ReplaceWithSingleNode | ReplaceWithMultipleNodes | RemoveNode; - interface Change { + interface BaseChange { readonly sourceFile: SourceFile; readonly range: TextRange; + } + + interface ChangeNodeOptions extends ConfigurableStartEnd, InsertNodeOptions { readonly useIndentationFromFile?: boolean; - readonly node?: Node | Node[]; - readonly options?: ChangeNodeOptions & ChangeNodesOptions; + } + interface ReplaceWithSingleNode extends BaseChange { + readonly kind: ChangeKind.ReplaceWithSingleNode; + readonly node: Node; + readonly options?: ChangeNodeOptions; + } + + interface RemoveNode extends BaseChange { + readonly kind: ChangeKind.Remove; + readonly node?: never; + readonly options?: never; + } + + interface ChangeMultipleNodesOptions extends ChangeNodeOptions { + nodeSeparator: string; + } + interface ReplaceWithMultipleNodes extends BaseChange { + readonly kind: ChangeKind.ReplaceWithMultipleNodes; + readonly nodes: ReadonlyArray; + readonly options?: ChangeMultipleNodesOptions; } export function getSeparatorCharacter(separator: Token) { @@ -157,16 +180,20 @@ namespace ts.textChanges { return s; } + function getNewlineKind(context: { newLineCharacter: string }) { + return context.newLineCharacter === "\n" ? NewLineKind.LineFeed : NewLineKind.CarriageReturnLineFeed; + } + export class ChangeTracker { private changes: Change[] = []; private readonly newLineCharacter: string; public static fromCodeFixContext(context: { newLineCharacter: string, rulesProvider: formatting.RulesProvider }) { - return new ChangeTracker(context.newLineCharacter === "\n" ? NewLineKind.LineFeed : NewLineKind.CarriageReturnLineFeed, context.rulesProvider); + return new ChangeTracker(getNewlineKind(context), context.rulesProvider); } public static fromRefactorContext(context: RefactorContext) { - return new ChangeTracker(context.newLineCharacter === "\n" ? NewLineKind.LineFeed : NewLineKind.CarriageReturnLineFeed, context.rulesProvider); + return new ChangeTracker(getNewlineKind(context), context.rulesProvider); } constructor( @@ -176,22 +203,22 @@ namespace ts.textChanges { this.newLineCharacter = getNewLineCharacter({ newLine }); } - public deleteNode(sourceFile: SourceFile, node: Node, options: ConfigurableStartEnd = {}) { - const startPosition = getAdjustedStartPosition(sourceFile, node, options, Position.FullStart); - const endPosition = getAdjustedEndPosition(sourceFile, node, options); - this.changes.push({ sourceFile, options, range: { pos: startPosition, end: endPosition } }); + public deleteRange(sourceFile: SourceFile, range: TextRange) { + this.changes.push({ kind: ChangeKind.Remove, sourceFile, range }); return this; } - public deleteRange(sourceFile: SourceFile, range: TextRange) { - this.changes.push({ sourceFile, range }); + public deleteNode(sourceFile: SourceFile, node: Node, options: ConfigurableStartEnd = {}) { + const startPosition = getAdjustedStartPosition(sourceFile, node, options, Position.FullStart); + const endPosition = getAdjustedEndPosition(sourceFile, node, options); + this.changes.push({ kind: ChangeKind.Remove, sourceFile, range: { pos: startPosition, end: endPosition } }); return this; } public deleteNodeRange(sourceFile: SourceFile, startNode: Node, endNode: Node, options: ConfigurableStartEnd = {}) { const startPosition = getAdjustedStartPosition(sourceFile, startNode, options, Position.FullStart); const endPosition = getAdjustedEndPosition(sourceFile, endNode, options); - this.changes.push({ sourceFile, options, range: { pos: startPosition, end: endPosition } }); + this.changes.push({ kind: ChangeKind.Remove, sourceFile, range: { pos: startPosition, end: endPosition } }); return this; } @@ -231,40 +258,74 @@ namespace ts.textChanges { } public replaceRange(sourceFile: SourceFile, range: TextRange, newNode: Node, options: InsertNodeOptions = {}) { - this.changes.push({ sourceFile, range, options, node: newNode }); + this.changes.push({ kind: ChangeKind.ReplaceWithSingleNode, sourceFile, range, options, node: newNode }); return this; } public replaceNode(sourceFile: SourceFile, oldNode: Node, newNode: Node, options: ChangeNodeOptions = {}) { const startPosition = getAdjustedStartPosition(sourceFile, oldNode, options, Position.Start); const endPosition = getAdjustedEndPosition(sourceFile, oldNode, options); - this.changes.push({ sourceFile, options, useIndentationFromFile: true, node: newNode, range: { pos: startPosition, end: endPosition } }); - return this; + return this.replaceWithSingle(sourceFile, startPosition, endPosition, newNode, options); } public replaceNodeRange(sourceFile: SourceFile, startNode: Node, endNode: Node, newNode: Node, options: ChangeNodeOptions = {}) { const startPosition = getAdjustedStartPosition(sourceFile, startNode, options, Position.Start); const endPosition = getAdjustedEndPosition(sourceFile, endNode, options); - this.changes.push({ sourceFile, options, useIndentationFromFile: true, node: newNode, range: { pos: startPosition, end: endPosition } }); + return this.replaceWithSingle(sourceFile, startPosition, endPosition, newNode, options); + } + + private replaceWithSingle(sourceFile: SourceFile, startPosition: number, endPosition: number, newNode: Node, options: ChangeNodeOptions): this { + this.changes.push({ + kind: ChangeKind.ReplaceWithSingleNode, + sourceFile, + options: { ...options, useIndentationFromFile: true }, + node: newNode, + range: { pos: startPosition, end: endPosition } + }); return this; } - public replaceNodes(sourceFile: SourceFile, oldNodes: Node | Node[], newNodes: Node | Node[], options: ChangeNodeOptions & ChangeNodesOptions = {}) { - const startPosition = getAdjustedStartPosition(sourceFile, isArray(oldNodes) ? oldNodes[0] : oldNodes, options, Position.Start); - const endPosition = getAdjustedEndPosition(sourceFile, isArray(oldNodes) ? lastOrUndefined(oldNodes) : oldNodes, options); - this.changes.push({ sourceFile, options, useIndentationFromFile: true, node: newNodes, range: { pos: startPosition, end: endPosition } }); + private replaceWithMutiple(sourceFile: SourceFile, startPosition: number, endPosition: number, newNodes: ReadonlyArray, options: ChangeMultipleNodesOptions): this { + this.changes.push({ + kind: ChangeKind.ReplaceWithMultipleNodes, + sourceFile, + options: { ...options, useIndentationFromFile: true }, + nodes: newNodes, + range: { pos: startPosition, end: endPosition } + }); return this; } + public replaceNodeWithNodes(sourceFile: SourceFile, oldNode: Node, newNodes: ReadonlyArray, options: ChangeMultipleNodesOptions) { + const startPosition = getAdjustedStartPosition(sourceFile, oldNode, options, Position.Start); + const endPosition = getAdjustedEndPosition(sourceFile, oldNode, options); + return this.replaceWithMutiple(sourceFile, startPosition, endPosition, newNodes, options); + } + + public replaceNodesWithNodes(sourceFile: SourceFile, oldNodes: ReadonlyArray, newNodes: ReadonlyArray, options: ChangeMultipleNodesOptions) { + const startPosition = getAdjustedStartPosition(sourceFile, oldNodes[0], options, Position.Start); + const endPosition = getAdjustedEndPosition(sourceFile, lastOrUndefined(oldNodes), options); + return this.replaceWithMutiple(sourceFile, startPosition, endPosition, newNodes, options); + } + + public replaceRangeWithNodes(sourceFile: SourceFile, range: TextRange, newNodes: ReadonlyArray, options: ChangeMultipleNodesOptions) { + return this.replaceWithMutiple(sourceFile, range.pos, range.end, newNodes, options); + } + + public replaceNodeRangeWithNodes(sourceFile: SourceFile, startNode: Node, endNode: Node, newNodes: ReadonlyArray, options: ChangeMultipleNodesOptions) { + const startPosition = getAdjustedStartPosition(sourceFile, startNode, options, Position.Start); + const endPosition = getAdjustedEndPosition(sourceFile, endNode, options); + return this.replaceWithMutiple(sourceFile, startPosition, endPosition, newNodes, options); + } + public insertNodeAt(sourceFile: SourceFile, pos: number, newNode: Node, options: InsertNodeOptions = {}) { - this.changes.push({ sourceFile, options, node: newNode, range: { pos: pos, end: pos } }); + this.changes.push({ kind: ChangeKind.ReplaceWithSingleNode, sourceFile, options, node: newNode, range: { pos: pos, end: pos } }); return this; } public insertNodeBefore(sourceFile: SourceFile, before: Node, newNode: Node, options: InsertNodeOptions & ConfigurableStart = {}) { const startPosition = getAdjustedStartPosition(sourceFile, before, options, Position.Start); - this.changes.push({ sourceFile, options, useIndentationFromFile: true, node: newNode, range: { pos: startPosition, end: startPosition } }); - return this; + return this.replaceWithSingle(sourceFile, startPosition, startPosition, newNode, options); } public insertNodeAfter(sourceFile: SourceFile, after: Node, newNode: Node, options: InsertNodeOptions & ConfigurableEnd = {}) { @@ -276,6 +337,7 @@ namespace ts.textChanges { // if not - insert semicolon to preserve the code from changing the meaning due to ASI if (sourceFile.text.charCodeAt(after.end - 1) !== CharacterCodes.semicolon) { this.changes.push({ + kind: ChangeKind.ReplaceWithSingleNode, sourceFile, options: {}, range: { pos: after.end, end: after.end }, @@ -284,8 +346,7 @@ namespace ts.textChanges { } } const endPosition = getAdjustedEndPosition(sourceFile, after, options); - this.changes.push({ sourceFile, options, useIndentationFromFile: true, node: newNode, range: { pos: endPosition, end: endPosition } }); - return this; + return this.replaceWithSingle(sourceFile, endPosition, endPosition, newNode, options); } /** @@ -354,11 +415,12 @@ namespace ts.textChanges { } this.changes.push({ + kind: ChangeKind.ReplaceWithSingleNode, sourceFile, range: { pos: startPos, end: containingList[index + 1].getStart(sourceFile) }, node: newNode, - useIndentationFromFile: true, options: { + useIndentationFromFile: true, prefix, // write separator and leading trivia of the next element as suffix suffix: `${tokenToString(nextToken.kind)}${sourceFile.text.substring(nextToken.end, containingList[index + 1].getStart(sourceFile))}` @@ -398,6 +460,7 @@ namespace ts.textChanges { if (multilineList) { // insert separator immediately following the 'after' node to preserve comments in trailing trivia this.changes.push({ + kind: ChangeKind.ReplaceWithSingleNode, sourceFile, range: { pos: end, end }, node: createToken(separator), @@ -411,6 +474,7 @@ namespace ts.textChanges { insertPos--; } this.changes.push({ + kind: ChangeKind.ReplaceWithSingleNode, sourceFile, range: { pos: insertPos, end: insertPos }, node: newNode, @@ -419,6 +483,7 @@ namespace ts.textChanges { } else { this.changes.push({ + kind: ChangeKind.ReplaceWithSingleNode, sourceFile, range: { pos: end, end }, node: newNode, @@ -461,31 +526,32 @@ namespace ts.textChanges { } private computeNewText(change: Change, sourceFile: SourceFile): string { - if (!change.node) { + if (change.kind === ChangeKind.Remove) { // deletion case return ""; } + const options = change.options || {}; let text: string; const pos = change.range.pos; const posStartsLine = getLineStartPositionForPosition(pos, sourceFile) === pos; - if (isArray(change.node)) { - const parts = change.node.map(n => this.getFormattedTextOfNode(n, sourceFile, pos, change.useIndentationFromFile, options)); - text = parts.join(change.options.nodesSeparator); + if (change.kind === ChangeKind.ReplaceWithMultipleNodes) { + const parts = change.nodes.map(n => this.getFormattedTextOfNode(n, sourceFile, pos, options)); + text = parts.join(change.options.nodeSeparator); } else { - text = this.getFormattedTextOfNode(change.node, sourceFile, pos, change.useIndentationFromFile, options); + text = this.getFormattedTextOfNode(change.node, sourceFile, pos, options); } // strip initial indentation (spaces or tabs) if text will be inserted in the middle of the line // however keep indentation if it is was forced - text = posStartsLine || change.options.indentation !== undefined ? text : text.replace(/^\s+/, ""); + text = (posStartsLine || options.indentation !== undefined) ? text : text.replace(/^\s+/, ""); return (options.prefix || "") + text + (options.suffix || ""); } - private getFormattedTextOfNode(node: Node, sourceFile: SourceFile, pos: number, useIndentationFromFile: boolean, options: ChangeNodeOptions) { - const nonFormattedText = getNonformattedText(node, sourceFile, this.newLine); + private getFormattedTextOfNode(node: Node, sourceFile: SourceFile, pos: number, options: ChangeNodeOptions) { + const nonformattedText = getNonformattedText(node, sourceFile, this.newLine); if (this.validator) { - this.validator(nonFormattedText); + this.validator(nonformattedText); } const formatOptions = this.rulesProvider.getFormatOptions(); @@ -494,7 +560,7 @@ namespace ts.textChanges { const initialIndentation = options.indentation !== undefined ? options.indentation - : useIndentationFromFile + : options.useIndentationFromFile ? formatting.SmartIndenter.getIndentation(pos, sourceFile, formatOptions, posStartsLine || (options.prefix === this.newLineCharacter)) : 0; const delta = @@ -504,7 +570,7 @@ namespace ts.textChanges { ? formatOptions.indentSize : 0; - return applyFormatting(nonFormattedText, sourceFile, initialIndentation, delta, this.rulesProvider); + return applyFormatting(nonformattedText, sourceFile, initialIndentation, delta, this.rulesProvider); } private static normalize(changes: Change[]): Change[] { @@ -681,4 +747,4 @@ namespace ts.textChanges { this.lastNonTriviaPosition = 0; } } -} \ No newline at end of file +} From 42187385c27f1eceea11b2564a3ca1636759cd91 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Tue, 18 Jul 2017 17:55:04 -0700 Subject: [PATCH 19/55] Only compute changes for requested refactors --- src/harness/unittests/extractMethods.ts | 3 +- src/services/refactors/extractMethod.ts | 121 +++++++++++++---------- tests/cases/fourslash/extract-method4.ts | 14 +++ 3 files changed, 84 insertions(+), 54 deletions(-) create mode 100644 tests/cases/fourslash/extract-method4.ts diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts index 6c0a642f03727..4b5d1911b4914 100644 --- a/src/harness/unittests/extractMethods.ts +++ b/src/harness/unittests/extractMethods.ts @@ -580,8 +580,9 @@ namespace A { data.push(`==ORIGINAL==`); data.push(sourceFile.text); for (const r of results) { + const changes = refactor.extractMethod.extractRange(result.targetRange, context, results.indexOf(r))[0].changes; data.push(`==SCOPE::${r.scopeDescription}==`); - data.push(textChanges.applyChanges(sourceFile.text, r.changes[0].textChanges)); + data.push(textChanges.applyChanges(sourceFile.text, changes[0].textChanges)); } return data.join(newLineCharacter); }); diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index a610e7a7f1a51..3831b9f25f16e 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -23,12 +23,11 @@ namespace ts.refactor.extractMethod { } const extractions = extractRange(targetRange, context); - if (extractions.length === 0) { + if (extractions === undefined) { // No extractions possible return undefined; } - const actions: RefactorActionInfo[] = []; let i = 0; for (const extr of extractions) { @@ -51,20 +50,23 @@ namespace ts.refactor.extractMethod { const length = context.endPosition === undefined ? 0 : context.endPosition - context.startPosition; const rangeToExtract = getRangeToExtract(context.file, { start: context.startPosition, length }); const targetRange: TargetRange = rangeToExtract.targetRange; - const extractions = extractRange(targetRange, context); - let i = 0; - for (const extr of extractions) { - const name = `scope_${i}`; - if (name === actionName) { - return ({ - edits: extr.changes - }); - } - i++; + const parsedIndexMatch = /scope_(\d+)/.exec(actionName); + if (!parsedIndexMatch) { + throw new Error("Expected to match the regexp"); + } + const index = +parsedIndexMatch[1]; + if (!isFinite(index)) { + throw new Error("Expected to parse a number from the scope index"); } - // ? - return undefined; + + const extractions = extractRange(targetRange, context, index); + if (extractions === undefined) { + // Scope is no longer valid from when the user issued the refactor + // return undefined; + return undefined; + } + return ({ edits: extractions[0].changes }); } // Move these into diagnostic messages if they become user-facing @@ -103,10 +105,13 @@ namespace ts.refactor.extractMethod { /** * Result of 'getRangeToExtract' operation: contains either a range or a list of errors */ - export interface RangeToExtract { - readonly targetRange?: TargetRange; - readonly errors?: Diagnostic[]; - } + export type RangeToExtract = { + readonly targetRange?: never; + readonly errors: ReadonlyArray; + } | { + readonly targetRange: TargetRange; + readonly errors?: never; + }; /* * Scopes that can store newly extracted method @@ -162,7 +167,7 @@ namespace ts.refactor.extractMethod { // 1 2 // in this case there is no such one node that covers ends of selection and is located inside the selection // to handle this we check if both start and end of the selection belong to some binary operation - // and start node is parented by the parent of the end node (because binary operators are left associative) + // and start node is parented by the parent of the end node // if this is the case - expand the selection to the entire parent of end node (in this case it will be [1 + 2 + 3] + 4) const startParent = skipParentheses(start.parent); const endParent = skipParentheses(end.parent); @@ -232,49 +237,49 @@ namespace ts.refactor.extractMethod { return errors; - function visit(n: Node) { + function visit(node: Node) { if (errors) { // already found an error - can stop now return true; } - if (!n || isFunctionLike(n) || isClassLike(n)) { + if (!node || isFunctionLike(node) || isClassLike(node)) { // do not dive into functions or classes return false; } const savedPermittedJumps = permittedJumps; - if (n.parent) { - switch (n.parent.kind) { + if (node.parent) { + switch (node.parent.kind) { case SyntaxKind.IfStatement: - if ((n.parent).thenStatement === n || (n.parent).elseStatement === n) { + if ((node.parent).thenStatement === node || (node.parent).elseStatement === node) { // forbid all jumps inside thenStatement or elseStatement permittedJumps = PermittedJumps.None; } break; case SyntaxKind.TryStatement: - if ((n.parent).tryBlock === n) { + if ((node.parent).tryBlock === node) { // forbid all jumps inside try blocks permittedJumps = PermittedJumps.None; } - else if ((n.parent).finallyBlock === n) { + else if ((node.parent).finallyBlock === node) { // allow unconditional returns from finally blocks permittedJumps = PermittedJumps.Return; } break; case SyntaxKind.CatchClause: - if ((n.parent).block === n) { + if ((node.parent).block === node) { // forbid all jumps inside the block of catch clause permittedJumps = PermittedJumps.None; } break; case SyntaxKind.CaseClause: - if ((n).expression !== n) { + if ((node).expression !== node) { // allow unlabeled break inside case clauses permittedJumps |= PermittedJumps.Break; } break; default: - if (isIterationStatement(n.parent, /*lookInLabeledStatements*/ false)) { - if ((n.parent).statement === n) { + if (isIterationStatement(node.parent, /*lookInLabeledStatements*/ false)) { + if ((node.parent).statement === node) { // allow unlabeled break/continue inside loops permittedJumps |= PermittedJumps.Break | PermittedJumps.Continue; } @@ -283,33 +288,33 @@ namespace ts.refactor.extractMethod { } } - switch (n.kind) { + switch (node.kind) { case SyntaxKind.ThisType: case SyntaxKind.ThisKeyword: facts |= RangeFacts.UsesThis; break; case SyntaxKind.LabeledStatement: { - const label = (n).label; + const label = (node).label; (seenLabels || (seenLabels = [])).push(label.text); - forEachChild(n, visit); + forEachChild(node, visit); seenLabels.pop(); break; } case SyntaxKind.BreakStatement: case SyntaxKind.ContinueStatement: { - const label = (n).label; + const label = (node).label; if (label) { if (!contains(seenLabels, label.text)) { // attempts to jump to label that is not in range to be extracted - (errors || (errors = [])).push(createDiagnosticForNode(n, Messages.CannotExtractRangeContainingLabeledBreakOrContinueStatementWithTargetOutsideOfTheRange)); + (errors || (errors = [])).push(createDiagnosticForNode(node, Messages.CannotExtractRangeContainingLabeledBreakOrContinueStatementWithTargetOutsideOfTheRange)); } } else { if (!(permittedJumps & (SyntaxKind.BreakStatement ? PermittedJumps.Break : PermittedJumps.Continue))) { // attempt to break or continue in a forbidden context - (errors || (errors = [])).push(createDiagnosticForNode(n, Messages.CannotExtractRangeContainingConditionalBreakOrContinueStatements)); + (errors || (errors = [])).push(createDiagnosticForNode(node, Messages.CannotExtractRangeContainingConditionalBreakOrContinueStatements)); } } break; @@ -325,11 +330,11 @@ namespace ts.refactor.extractMethod { facts |= RangeFacts.HasReturn; } else { - (errors || (errors = [])).push(createDiagnosticForNode(n, Messages.CannotExtractRangeContainingConditionalReturnStatement)); + (errors || (errors = [])).push(createDiagnosticForNode(node, Messages.CannotExtractRangeContainingConditionalReturnStatement)); } break; default: - forEachChild(n, visit); + forEachChild(node, visit); break; } @@ -359,7 +364,7 @@ namespace ts.refactor.extractMethod { // Walk up to the closest parent of a place where we can logically put a sibling: // * Function declaration // * Class declaration or expression - // * Module or source file + // * Module/namespace or source file // Note that we don't use isFunctionLike because we don't want to put the extracted closure *inside* a method if ((current.kind === SyntaxKind.FunctionDeclaration) || isSourceFile(current) || isModuleBlock(current) || isClassLike(current)) { scopes.push(current as FunctionLikeDeclaration); @@ -374,17 +379,18 @@ namespace ts.refactor.extractMethod { * Each returned ExtractResultForScope corresponds to a possible target scope and is either a set of changes * or an error explaining why we can't extract into that scope. */ - export function extractRange(targetRange: TargetRange, context: RefactorContext): ReadonlyArray { + export function extractRange(targetRange: TargetRange, context: RefactorContext, requestedChangesIndex: number = undefined): ReadonlyArray | undefined { const { file: sourceFile } = context; if (targetRange === undefined) { - return []; + return undefined; } const scopes = collectEnclosingScopes(targetRange); if (scopes.length === 0) { - return []; + return undefined; } + const enclosingTextRange = getEnclosingTextRange(targetRange, sourceFile); const { target, usagesPerScope, errorsPerScope } = collectReadsAndWrites( targetRange, @@ -394,17 +400,26 @@ namespace ts.refactor.extractMethod { context.program.getTypeChecker()); context.cancellationToken.throwIfCancellationRequested(); - return scopes.map((scope, i) => { - const errors = errorsPerScope[i]; - if (errors.length) { - return { - scope, - scopeDescription: getDescriptionForScope(scope), - errors - }; - } - return extractFunctionInScope(target, scope, usagesPerScope[i], targetRange, context); - }); + + if (requestedChangesIndex !== undefined) { + if (errorsPerScope[requestedChangesIndex].length) { + return undefined; + } + return [extractFunctionInScope(target, scopes[requestedChangesIndex], usagesPerScope[requestedChangesIndex], targetRange, context)]; + } + else { + return scopes.map((scope, i) => { + const errors = errorsPerScope[i]; + if (errors.length) { + return { + scope, + scopeDescription: getDescriptionForScope(scope), + errors + }; + } + return { scope, scopeDescription: getDescriptionForScope(scope) }; + }); + } } function getDescriptionForScope(s: Scope) { diff --git a/tests/cases/fourslash/extract-method4.ts b/tests/cases/fourslash/extract-method4.ts new file mode 100644 index 0000000000000..ec8f39f354189 --- /dev/null +++ b/tests/cases/fourslash/extract-method4.ts @@ -0,0 +1,14 @@ +/// + +//// let a = 1, b = 2, c = 3, d = 4; +//// namespace NS { +//// class Q { +//// foo() { +//// a = /*1*/b = c/*2*/ = d; +//// } +//// } +//// } + +// Should rewrite to a = newFunc(); function() { return b = c = d; } +goTo.select('1', '2'); +verify.not.refactorAvailable('Extract Method'); From 6d2bc90d19d6fe9e5b7a9759b981d8835b5e2cf5 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 19 Jul 2017 11:10:16 -0700 Subject: [PATCH 20/55] Address more PR comments --- src/compiler/utilities.ts | 29 ++- .../refactors/convertFunctionToEs6Class.ts | 2 +- src/services/refactors/extractMethod.ts | 238 ++++++++---------- src/services/textChanges.ts | 7 +- .../reference/extractMethod/extractMethod4.js | 24 +- 5 files changed, 141 insertions(+), 159 deletions(-) diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 3d7e8c529b894..d830940bdf1d1 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -671,7 +671,7 @@ namespace ts { // At this point, node is either a qualified name or an identifier Debug.assert(node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.QualifiedName || node.kind === SyntaxKind.PropertyAccessExpression, "'node' was expected to be a qualified name, identifier or property access in 'isPartOfTypeNode'."); - // falls through + // falls through case SyntaxKind.QualifiedName: case SyntaxKind.PropertyAccessExpression: case SyntaxKind.ThisKeyword: @@ -956,7 +956,7 @@ namespace ts { if (!includeArrowFunctions) { continue; } - // falls through + // falls through case SyntaxKind.FunctionDeclaration: case SyntaxKind.FunctionExpression: case SyntaxKind.ModuleDeclaration: @@ -1015,7 +1015,7 @@ namespace ts { if (!stopOnFunctions) { continue; } - // falls through + // falls through case SyntaxKind.PropertyDeclaration: case SyntaxKind.PropertySignature: case SyntaxKind.MethodDeclaration: @@ -1200,7 +1200,7 @@ namespace ts { if (node.parent.kind === SyntaxKind.TypeQuery || isJSXTagName(node)) { return true; } - // falls through + // falls through case SyntaxKind.NumericLiteral: case SyntaxKind.StringLiteral: case SyntaxKind.ThisKeyword: @@ -1485,8 +1485,8 @@ namespace ts { parent.parent.kind === SyntaxKind.VariableStatement; const variableStatementNode = isInitializerOfVariableDeclarationInStatement ? parent.parent.parent : - isVariableOfVariableDeclarationStatement ? parent.parent : - undefined; + isVariableOfVariableDeclarationStatement ? parent.parent : + undefined; if (variableStatementNode) { getJSDocsWorker(variableStatementNode); } @@ -1606,7 +1606,7 @@ namespace ts { if (node && (node.flags & NodeFlags.JavaScriptFile)) { if (node.type && node.type.kind === SyntaxKind.JSDocVariadicType || forEach(getJSDocParameterTags(node), - t => t.typeExpression && t.typeExpression.type.kind === SyntaxKind.JSDocVariadicType)) { + t => t.typeExpression && t.typeExpression.type.kind === SyntaxKind.JSDocVariadicType)) { return true; } } @@ -1898,7 +1898,7 @@ namespace ts { if (node.asteriskToken) { flags |= FunctionFlags.Generator; } - // falls through + // falls through case SyntaxKind.ArrowFunction: if (hasModifier(node, ModifierFlags.Async)) { flags |= FunctionFlags.Async; @@ -5079,6 +5079,19 @@ namespace ts { return isUnaryExpressionKind(skipPartiallyEmittedExpressions(node).kind); } + /* @internal */ + export function isUnaryExpressionWithWrite(expr: Node): expr is PrefixUnaryExpression | PostfixUnaryExpression { + switch (expr.kind) { + case SyntaxKind.PostfixUnaryExpression: + return true; + case SyntaxKind.PrefixUnaryExpression: + return (expr).operator === SyntaxKind.PlusPlusToken || + (expr).operator === SyntaxKind.MinusMinusToken; + default: + return false; + } + } + function isExpressionKind(kind: SyntaxKind) { return kind === SyntaxKind.ConditionalExpression || kind === SyntaxKind.YieldExpression diff --git a/src/services/refactors/convertFunctionToEs6Class.ts b/src/services/refactors/convertFunctionToEs6Class.ts index 745ba6d15bfd7..e26301b675e53 100644 --- a/src/services/refactors/convertFunctionToEs6Class.ts +++ b/src/services/refactors/convertFunctionToEs6Class.ts @@ -1,6 +1,6 @@ /* @internal */ -namespace ts.refactor { +namespace ts.refactor.convertFunctionToES6Class { const actionName = "convert"; const convertFunctionToES6Class: Refactor = { diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 3831b9f25f16e..9766d7bdc9e55 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -3,7 +3,6 @@ /* @internal */ namespace ts.refactor.extractMethod { - export const name = "extract_method"; const extractMethod: Refactor = { name: "Extract Method", description: Diagnostics.Extract_function.message, @@ -71,19 +70,19 @@ namespace ts.refactor.extractMethod { // Move these into diagnostic messages if they become user-facing namespace Messages { - function m(message: string): DiagnosticMessage { + function createMessage(message: string): DiagnosticMessage { return { message, code: 0, category: DiagnosticCategory.Message, key: message }; } - export const CannotExtractFunction: DiagnosticMessage = m("Cannot extract function."); - export const StatementOrExpressionExpected: DiagnosticMessage = m("Statement or expression expected."); - export const CannotExtractRangeContainingConditionalBreakOrContinueStatements: DiagnosticMessage = m("Cannot extract range containing conditional break or continue statements."); - export const CannotExtractRangeContainingConditionalReturnStatement: DiagnosticMessage = m("Cannot extract range containing conditional return statement."); - export const CannotExtractRangeContainingLabeledBreakOrContinueStatementWithTargetOutsideOfTheRange: DiagnosticMessage = m("Cannot extract range containing labeled break or continue with target outside of the range."); - export const CannotExtractRangeThatContainsWritesToReferencesLocatedOutsideOfTheTargetRangeInGenerators: DiagnosticMessage = m("Cannot extract range containing writes to references located outside of the target range in generators."); - export const TypeWillNotBeVisibleInTheNewScope = m("Type will not visible in the new scope."); - export const FunctionWillNotBeVisibleInTheNewScope = m("Function will not visible in the new scope."); - export const InsufficientSelection = m("Select more than a single identifier."); + export const CannotExtractFunction: DiagnosticMessage = createMessage("Cannot extract function."); + 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."); + export const CannotExtractRangeContainingLabeledBreakOrContinueStatementWithTargetOutsideOfTheRange: DiagnosticMessage = createMessage("Cannot extract range containing labeled break or continue with target outside of the range."); + 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 enum RangeFacts { @@ -146,7 +145,7 @@ namespace ts.refactor.extractMethod { // We'll modify these flags as we walk the tree to collect data // about what things need to be done as part of the extraction. - let facts = RangeFacts.None; + let rangeFacts = RangeFacts.None; if (!start || !end) { // cannot find either start or end node @@ -176,28 +175,28 @@ namespace ts.refactor.extractMethod { } else { // start and end nodes belong to different subtrees - return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractFunction)] }; + return createErrorResult(sourceFile, span.start, length, Messages.CannotExtractFunction); } } if (start !== end) { // start and end should be statements and parent should be either block or a source file if (!isBlockLike(start.parent)) { - return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractFunction)] }; + return createErrorResult(sourceFile, span.start, length, Messages.CannotExtractFunction); } const statements: Statement[] = []; - for (const n of (start.parent).statements) { - if (n === start || statements.length) { - const errors = checkNode(n); + for (const statement of (start.parent).statements) { + if (statement === start || statements.length) { + const errors = checkNode(statement); if (errors) { return { errors }; } - statements.push(n); + statements.push(statement); } - if (n === end) { + if (statement === end) { break; } } - return { targetRange: { range: statements, facts } }; + return { targetRange: { range: statements, facts: rangeFacts } }; } else { const errors = checkRootNode(start) || checkNode(start); @@ -207,33 +206,37 @@ namespace ts.refactor.extractMethod { const range = isStatement(start) ? [start] : start; - return { targetRange: { range, facts } }; + return { targetRange: { range, facts: rangeFacts } }; } - function checkRootNode(n: Node): Diagnostic[] | undefined { - if (isIdentifier(n)) { - return [createDiagnosticForNode(n, Messages.InsufficientSelection)]; + function createErrorResult(sourceFile: SourceFile, start: number, length: number, message: DiagnosticMessage): RangeToExtract { + return { errors: [createFileDiagnostic(sourceFile, start, length, message)] }; + } + + function checkRootNode(node: Node): Diagnostic[] | undefined { + if (isIdentifier(node)) { + return [createDiagnosticForNode(node, Messages.InsufficientSelection)]; } return undefined; } // Verifies whether we can actually extract this node or not. - function checkNode(n: Node): Diagnostic[] | undefined { + function checkNode(nodeToCheck: Node): Diagnostic[] | undefined { const enum PermittedJumps { None = 0, Break = 1 << 0, Continue = 1 << 1, Return = 1 << 2 } - if (!isStatement(n) && !isExpression(n)) { - return [createDiagnosticForNode(n, Messages.StatementOrExpressionExpected)]; + if (!isStatement(nodeToCheck) && !isExpression(nodeToCheck)) { + return [createDiagnosticForNode(nodeToCheck, Messages.StatementOrExpressionExpected)]; } let errors: Diagnostic[]; let permittedJumps = PermittedJumps.Return; let seenLabels: string[]; - visit(n); + visit(nodeToCheck); return errors; @@ -291,7 +294,7 @@ namespace ts.refactor.extractMethod { switch (node.kind) { case SyntaxKind.ThisType: case SyntaxKind.ThisKeyword: - facts |= RangeFacts.UsesThis; + rangeFacts |= RangeFacts.UsesThis; break; case SyntaxKind.LabeledStatement: { @@ -320,14 +323,14 @@ namespace ts.refactor.extractMethod { break; } case SyntaxKind.AwaitExpression: - facts |= RangeFacts.IsAsyncFunction; + rangeFacts |= RangeFacts.IsAsyncFunction; break; case SyntaxKind.YieldExpression: - facts |= RangeFacts.IsGenerator; + rangeFacts |= RangeFacts.IsGenerator; break; case SyntaxKind.ReturnStatement: if (permittedJumps & PermittedJumps.Return) { - facts |= RangeFacts.HasReturn; + rangeFacts |= RangeFacts.HasReturn; } else { (errors || (errors = [])).push(createDiagnosticForNode(node, Messages.CannotExtractRangeContainingConditionalReturnStatement)); @@ -348,7 +351,7 @@ namespace ts.refactor.extractMethod { * you may be able to extract into a class method *or* local closure *or* namespace function, * depending on what's in the extracted body. */ - export function collectEnclosingScopes(range: TargetRange): Scope[] { + export function collectEnclosingScopes(range: TargetRange): Scope[] | undefined { let current: Node = isArray(range.range) ? firstOrUndefined(range.range) : range.range; if (range.facts & RangeFacts.UsesThis) { // if range uses this as keyword or as type inside the class then it can only be extracted to a method of the containing class @@ -358,7 +361,7 @@ namespace ts.refactor.extractMethod { } } - const scopes: Scope[] = []; + let scopes: Scope[] | undefined = undefined; while (current) { // We want to find the nearest parent where we can place an "equivalent" sibling to the node we're extracting out of. // Walk up to the closest parent of a place where we can logically put a sibling: @@ -367,7 +370,7 @@ namespace ts.refactor.extractMethod { // * Module/namespace or source file // Note that we don't use isFunctionLike because we don't want to put the extracted closure *inside* a method if ((current.kind === SyntaxKind.FunctionDeclaration) || isSourceFile(current) || isModuleBlock(current) || isClassLike(current)) { - scopes.push(current as FunctionLikeDeclaration); + (scopes = scopes || []).push(current as FunctionLikeDeclaration); } current = current.parent; } @@ -387,7 +390,7 @@ namespace ts.refactor.extractMethod { } const scopes = collectEnclosingScopes(targetRange); - if (scopes.length === 0) { + if (scopes === undefined) { return undefined; } @@ -422,39 +425,39 @@ namespace ts.refactor.extractMethod { } } - function getDescriptionForScope(s: Scope) { - if (isFunctionLike(s)) { - switch (s.kind) { + function getDescriptionForScope(scope: Scope) { + if (isFunctionLike(scope)) { + switch (scope.kind) { case SyntaxKind.Constructor: return "constructor"; case SyntaxKind.FunctionExpression: - return s.name - ? `function expression ${s.name.getText()}` + return scope.name + ? `function expression ${scope.name.getText()}` : "anonymous function expression"; case SyntaxKind.FunctionDeclaration: - return `function ${s.name.getText()}`; + return `function ${scope.name.getText()}`; case SyntaxKind.ArrowFunction: return "arrow function"; case SyntaxKind.MethodDeclaration: - return `method ${s.name.getText()}`; + return `method ${scope.name.getText()}`; case SyntaxKind.GetAccessor: - return `get ${s.name.getText()}`; + return `get ${scope.name.getText()}`; case SyntaxKind.SetAccessor: - return `set ${s.name.getText()}`; + return `set ${scope.name.getText()}`; } } - else if (isModuleBlock(s)) { - return `namespace ${s.parent.name.getText()}`; + else if (isModuleBlock(scope)) { + return `namespace ${scope.parent.name.getText()}`; } - else if (isClassLike(s)) { - return s.kind === SyntaxKind.ClassDeclaration - ? `class ${s.name.text}` - : s.name.text - ? `class expression ${s.name.text}` + else if (isClassLike(scope)) { + return scope.kind === SyntaxKind.ClassDeclaration + ? `class ${scope.name.text}` + : scope.name.text + ? `class expression ${scope.name.text}` : "anonymous class expression"; } - else if (isSourceFile(s)) { - return `file '${s.fileName}'`; + else if (isSourceFile(scope)) { + return `file '${scope.fileName}'`; } else { return "unknown"; @@ -470,8 +473,6 @@ namespace ts.refactor.extractMethod { const checker = context.program.getTypeChecker(); - const changeTracker = textChanges.ChangeTracker.fromRefactorContext(context); - // Make a unique name for the extracted function let functionNameText = "newFunction"; if (scope.locals && scope.locals.has(functionNameText)) { @@ -483,13 +484,12 @@ namespace ts.refactor.extractMethod { const functionName = createIdentifier(functionNameText); // Currently doesn't get populated, but we might try to infer from this at some point - const typeParameters: TypeParameterDeclaration[] = undefined; const returnType: TypeNode = undefined; const parameters: ParameterDeclaration[] = []; const callArguments: Identifier[] = []; let writes: UsageEntry[]; - usagesInScope.forEach((value, key) => { - let type = checker.getTypeOfSymbolAtLocation(value.symbol, value.node); + usagesInScope.forEach((usage, name) => { + let type = checker.getTypeOfSymbolAtLocation(usage.symbol, usage.node); // Widen the type so we don't emit nonsense annotations like "function fn(x: 3) {" type = checker.getBaseTypeOfLiteralType(type); @@ -497,15 +497,15 @@ namespace ts.refactor.extractMethod { /*decorators*/ undefined, /*modifiers*/ undefined, /*dotDotDotToken*/ undefined, - /*name*/ key, + /*name*/ name, /*questionToken*/ undefined, checker.typeToTypeNode(type, node, NodeBuilderFlags.NoTruncation) ); parameters.push(paramDecl); - if (value.usage === Usage.Write) { - (writes || (writes = [])).push(value); + if (usage.usage === Usage.Write) { + (writes || (writes = [])).push(usage); } - callArguments.push(createIdentifier(key)); + callArguments.push(createIdentifier(name)); }); const { body, returnValueProperty } = transformFunctionBody(node); @@ -523,7 +523,7 @@ namespace ts.refactor.extractMethod { range.facts & RangeFacts.IsGenerator ? createToken(SyntaxKind.AsteriskToken) : undefined, functionName, /*questionToken*/ undefined, - typeParameters, + /*typeParameters*/ [], parameters, returnType, body @@ -535,12 +535,14 @@ namespace ts.refactor.extractMethod { range.facts & RangeFacts.IsAsyncFunction ? [createToken(SyntaxKind.AsyncKeyword)] : undefined, range.facts & RangeFacts.IsGenerator ? createToken(SyntaxKind.AsteriskToken) : undefined, functionName, - typeParameters, + /*typeParameters*/ [], parameters, returnType, body ); } + + const changeTracker = textChanges.ChangeTracker.fromRefactorContext(context); // insert function at the end of the scope changeTracker.insertNodeBefore(context.file, scope.getLastToken(), newFunction, { prefix: context.newLineCharacter, suffix: context.newLineCharacter }); @@ -612,13 +614,13 @@ namespace ts.refactor.extractMethod { return "__return"; } - function transformFunctionBody(n: Node) { - if (isBlock(n) && !writes && substitutions.size === 0) { + function transformFunctionBody(body: Node) { + if (isBlock(body) && !writes && substitutions.size === 0) { // already block, no writes to propagate back, no substitutions - can use node as is - return { body: n, returnValueProperty: undefined }; + return { body: body, returnValueProperty: undefined }; } let returnValueProperty: string; - const statements = createNodeArray(isBlock(n) ? n.statements.slice(0) : [isStatement(n) ? n : createReturn(n)]); + const statements = createNodeArray(isBlock(body) ? body.statements.slice(0) : [isStatement(body) ? body : createReturn(body)]); // rewrite body if either there are writes that should be propagated back via return statements or there are substitutions if (writes || substitutions.size) { const rewrittenStatements = visitNodes(statements, visitor); @@ -652,26 +654,6 @@ namespace ts.refactor.extractMethod { } } - const nullTransformationContext: TransformationContext = { - enableEmitNotification: noop, - enableSubstitution: noop, - endLexicalEnvironment: noop as () => undefined, - getCompilerOptions: notImplemented, - getEmitHost: notImplemented, - getEmitResolver: notImplemented, - hoistFunctionDeclaration: noop, - hoistVariableDeclaration: noop, - isEmitNotificationEnabled: notImplemented, - isSubstitutionEnabled: notImplemented, - onEmitNode: noop, - onSubstituteNode: notImplemented, - readEmitHelpers: notImplemented, - requestEmitHelper: noop, - resumeLexicalEnvironment: noop, - startLexicalEnvironment: noop, - suspendLexicalEnvironment: noop - }; - function isModuleBlock(n: Node): n is ModuleBlock { return n.kind === SyntaxKind.ModuleBlock; } @@ -731,36 +713,36 @@ namespace ts.refactor.extractMethod { return { target, usagesPerScope, errorsPerScope }; - function collectUsages(n: Node) { - if (isAssignmentExpression(n)) { + function collectUsages(node: Node) { + if (isAssignmentExpression(node)) { const savedValueUsage = valueUsage; // use 'write' as default usage for values valueUsage = Usage.Write; - collectUsages(n.left); + collectUsages(node.left); valueUsage = savedValueUsage; - collectUsages(n.right); + collectUsages(node.right); } - else if (isUnaryExpressionWithWrite(n)) { + else if (isUnaryExpressionWithWrite(node)) { const savedValueUsage = valueUsage; valueUsage = Usage.Write; - collectUsages(n.operand); + collectUsages(node.operand); valueUsage = savedValueUsage; } - else if (isIdentifier(n)) { - if (!n.parent) { + else if (isIdentifier(node)) { + if (!node.parent) { return; } - if (isQualifiedName(n.parent) && n !== n.parent.left) { + if (isQualifiedName(node.parent) && node !== node.parent.left) { return; } - if ((isPropertyAccessExpression(n.parent) || isElementAccessExpression(n.parent)) && n !== n.parent.expression) { + if ((isPropertyAccessExpression(node.parent) || isElementAccessExpression(node.parent)) && node !== node.parent.expression) { return; } - recordUsage(n, valueUsage, /*isTypeNode*/ isPartOfTypeNode(n)); + recordUsage(node, valueUsage, /*isTypeNode*/ isPartOfTypeNode(node)); } else { - forEachChild(n, collectUsages); + forEachChild(node, collectUsages); } } @@ -777,8 +759,8 @@ namespace ts.refactor.extractMethod { } } - function recordUsagebySymbol(n: Identifier, usage: Usage, isTypeName: boolean) { - const symbol = checker.getSymbolAtLocation(n); + function recordUsagebySymbol(identifier: Identifier, usage: Usage, isTypeName: boolean) { + const symbol = checker.getSymbolAtLocation(identifier); if (!symbol) { // cannot find symbol - do nothing return undefined; @@ -800,9 +782,9 @@ namespace ts.refactor.extractMethod { // if we get here this means that we are trying to handle 'write' and 'read' was already processed // walk scopes and update existing records. for (const perScope of usagesPerScope) { - const prevEntry = perScope.usages.get(n.text); + const prevEntry = perScope.usages.get(identifier.text); if (prevEntry) { - perScope.usages.set(n.text, { usage, symbol, node: n }); + perScope.usages.set(identifier.text, { usage, symbol, node: identifier }); } } return symbolId; @@ -820,7 +802,7 @@ namespace ts.refactor.extractMethod { // 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(n, Messages.CannotExtractRangeThatContainsWritesToReferencesLocatedOutsideOfTheTargetRangeInGenerators)); + errors.push(createDiagnosticForNode(identifier, Messages.CannotExtractRangeThatContainsWritesToReferencesLocatedOutsideOfTheTargetRangeInGenerators)); } } for (let i = 0; i < scopes.length; i++) { @@ -835,53 +817,41 @@ namespace ts.refactor.extractMethod { substitutionsPerScope[i].set(symbolId, substitution); } else if (isTypeName) { - errorsPerScope[i].push(createDiagnosticForNode(n, Messages.TypeWillNotBeVisibleInTheNewScope)); + errorsPerScope[i].push(createDiagnosticForNode(identifier, Messages.TypeWillNotBeVisibleInTheNewScope)); } else { - usagesPerScope[i].usages.set(n.text, { usage, symbol, node: n }); + usagesPerScope[i].usages.set(identifier.text, { usage, symbol, node: identifier }); } } } return symbolId; } - function tryReplaceWithQualifiedNameOrPropertyAccess(s: Symbol, scopeDecl: Node, isTypeNode: boolean): PropertyAccessExpression | EntityName { - if (!s) { + function tryReplaceWithQualifiedNameOrPropertyAccess(symbol: Symbol, scopeDecl: Node, isTypeNode: boolean): PropertyAccessExpression | EntityName { + if (!symbol) { return undefined; } - if (s.getDeclarations().some(d => d.parent === scopeDecl)) { - return createIdentifier(s.name); + if (symbol.getDeclarations().some(d => d.parent === scopeDecl)) { + return createIdentifier(symbol.name); } - const prefix = tryReplaceWithQualifiedNameOrPropertyAccess(s.parent, scopeDecl, isTypeNode); + const prefix = tryReplaceWithQualifiedNameOrPropertyAccess(symbol.parent, scopeDecl, isTypeNode); if (prefix === undefined) { return undefined; } - return isTypeNode ? createQualifiedName(prefix, createIdentifier(s.name)) : createPropertyAccess(prefix, s.name); - } - - function isUnaryExpressionWithWrite(n: Node): n is PrefixUnaryExpression | PostfixUnaryExpression { - switch (n.kind) { - case SyntaxKind.PostfixUnaryExpression: - return true; - case SyntaxKind.PrefixUnaryExpression: - return (n).operator === SyntaxKind.PlusPlusToken || - (n).operator === SyntaxKind.MinusMinusToken; - default: - return false; - } + return isTypeNode ? createQualifiedName(prefix, createIdentifier(symbol.name)) : createPropertyAccess(prefix, symbol.name); } } - function getParentNodeInSpan(n: Node, file: SourceFile, span: TextSpan): Node { - while (n) { - if (!n.parent) { + function getParentNodeInSpan(node: Node, file: SourceFile, span: TextSpan): Node { + while (node) { + if (!node.parent) { return undefined; } - if (isSourceFile(n.parent) || !spanContainsNode(span, n.parent, file)) { - return n; + if (isSourceFile(node.parent) || !spanContainsNode(span, node.parent, file)) { + return node; } - n = n.parent; + node = node.parent; } } @@ -890,8 +860,8 @@ namespace ts.refactor.extractMethod { node.getEnd() <= textSpanEnd(span); } - function isBlockLike(n: Node): n is BlockLike { - switch (n.kind) { + function isBlockLike(node: Node): node is BlockLike { + switch (node.kind) { case SyntaxKind.Block: case SyntaxKind.SourceFile: case SyntaxKind.ModuleBlock: diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index c06d329c1f2f0..925da8e79bcad 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -278,7 +278,7 @@ namespace ts.textChanges { this.changes.push({ kind: ChangeKind.ReplaceWithSingleNode, sourceFile, - options: { ...options, useIndentationFromFile: true }, + options, node: newNode, range: { pos: startPosition, end: endPosition } }); @@ -289,7 +289,7 @@ namespace ts.textChanges { this.changes.push({ kind: ChangeKind.ReplaceWithMultipleNodes, sourceFile, - options: { ...options, useIndentationFromFile: true }, + options, nodes: newNodes, range: { pos: startPosition, end: endPosition } }); @@ -420,7 +420,6 @@ namespace ts.textChanges { range: { pos: startPos, end: containingList[index + 1].getStart(sourceFile) }, node: newNode, options: { - useIndentationFromFile: true, prefix, // write separator and leading trivia of the next element as suffix suffix: `${tokenToString(nextToken.kind)}${sourceFile.text.substring(nextToken.end, containingList[index + 1].getStart(sourceFile))}` @@ -560,7 +559,7 @@ namespace ts.textChanges { const initialIndentation = options.indentation !== undefined ? options.indentation - : options.useIndentationFromFile + : (options.useIndentationFromFile !== false) ? formatting.SmartIndenter.getIndentation(pos, sourceFile, formatOptions, posStartsLine || (options.prefix === this.newLineCharacter)) : 0; const delta = diff --git a/tests/baselines/reference/extractMethod/extractMethod4.js b/tests/baselines/reference/extractMethod/extractMethod4.js index 08a4d447a0813..6b9e2eed099d3 100644 --- a/tests/baselines/reference/extractMethod/extractMethod4.js +++ b/tests/baselines/reference/extractMethod/extractMethod4.js @@ -24,9 +24,9 @@ namespace A { async function newFunction() { let y = 5; - if (z) { - await z1; - } + if(z) { + await z1; + } return foo(); } } @@ -44,9 +44,9 @@ namespace A { async function newFunction(z: number, z1: any) { let y = 5; - if (z) { - await z1; - } + if(z) { + await z1; + } return foo(); } } @@ -64,9 +64,9 @@ namespace A { async function newFunction(z: number, z1: any) { let y = 5; - if (z) { - await z1; - } + if(z) { + await z1; + } return foo(); } } @@ -83,8 +83,8 @@ namespace A { } async function newFunction(z: number, z1: any, foo: () => void) { let y = 5; - if (z) { - await z1; - } + if(z) { + await z1; +} return foo(); } From 271ecb172d31e58cb9bc3124f14ce955500914aa Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 19 Jul 2017 12:41:32 -0700 Subject: [PATCH 21/55] Remove WS --- src/services/refactors/extractMethod.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index dde7bd9b7a1fb..e99cd20581b6c 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -117,7 +117,6 @@ namespace ts.refactor.extractMethod { */ export type Scope = FunctionLikeDeclaration | SourceFile | ModuleBlock | ClassLikeDeclaration; - /** * Result of 'extractRange' operation for a specific scope. * Stores either a list of changes that should be applied to extract a range or a list of errors From bdefcc34746bca54e8ebb3055892784498f90957 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 19 Jul 2017 12:54:01 -0700 Subject: [PATCH 22/55] Emit return types in ctxly-typed functions and don't emit annotations in JS --- src/services/refactors/extractMethod.ts | 37 +++++++++++++++--------- tests/cases/fourslash/extract-method5.ts | 18 ++++++++++++ 2 files changed, 42 insertions(+), 13 deletions(-) create mode 100644 tests/cases/fourslash/extract-method5.ts diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index e99cd20581b6c..24674d19125eb 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -108,9 +108,9 @@ namespace ts.refactor.extractMethod { readonly targetRange?: never; readonly errors: ReadonlyArray; } | { - readonly targetRange: TargetRange; - readonly errors?: never; - }; + readonly targetRange: TargetRange; + readonly errors?: never; + }; /* * Scopes that can store newly extracted method @@ -480,17 +480,21 @@ namespace ts.refactor.extractMethod { i++; } } + const isJS = isInJavaScriptFile(node); const functionName = createIdentifier(functionNameText as string); - // Currently doesn't get populated, but we might try to infer from this at some point - const returnType: TypeNode = undefined; + let returnType: TypeNode = undefined; const parameters: ParameterDeclaration[] = []; const callArguments: Identifier[] = []; let writes: UsageEntry[]; usagesInScope.forEach((usage, name) => { - let type = checker.getTypeOfSymbolAtLocation(usage.symbol, usage.node); - // Widen the type so we don't emit nonsense annotations like "function fn(x: 3) {" - type = checker.getBaseTypeOfLiteralType(type); + let typeNode: TypeNode = undefined; + if (!isJS) { + let type = checker.getTypeOfSymbolAtLocation(usage.symbol, usage.node); + // Widen the type so we don't emit nonsense annotations like "function fn(x: 3) {" + type = checker.getBaseTypeOfLiteralType(type); + typeNode = checker.typeToTypeNode(type, node, NodeBuilderFlags.NoTruncation); + } const paramDecl = createParameter( /*decorators*/ undefined, @@ -498,7 +502,7 @@ namespace ts.refactor.extractMethod { /*dotDotDotToken*/ undefined, /*name*/ name, /*questionToken*/ undefined, - checker.typeToTypeNode(type, node, NodeBuilderFlags.NoTruncation) + typeNode ); parameters.push(paramDecl); if (usage.usage === Usage.Write) { @@ -507,12 +511,19 @@ namespace ts.refactor.extractMethod { callArguments.push(createIdentifier(name)); }); + // Provide explicit return types for contexutally-typed functions + // to avoid problems when there are literal types present + if (isExpression(node) && !isInJavaScriptFile(node)) { + const contextualType = checker.getContextualType(node); + returnType = checker.typeToTypeNode(contextualType); + } + const { body, returnValueProperty } = transformFunctionBody(node); let newFunction: MethodDeclaration | FunctionDeclaration; if (isClassLike(scope)) { - // always create private method - const modifiers: Modifier[] = [createToken(SyntaxKind.PrivateKeyword)]; + // always create private method in TypeScript files + const modifiers: Modifier[] = isInJavaScriptFile(node) ? [] : [createToken(SyntaxKind.PrivateKeyword)]; if (range.facts & RangeFacts.IsAsyncFunction) { modifiers.push(createToken(SyntaxKind.AsyncKeyword)); } @@ -522,7 +533,7 @@ namespace ts.refactor.extractMethod { range.facts & RangeFacts.IsGenerator ? createToken(SyntaxKind.AsteriskToken) : undefined, functionName, /*questionToken*/ undefined, - /*typeParameters*/ [], + /*typeParameters*/[], parameters, returnType, body @@ -534,7 +545,7 @@ namespace ts.refactor.extractMethod { range.facts & RangeFacts.IsAsyncFunction ? [createToken(SyntaxKind.AsyncKeyword)] : undefined, range.facts & RangeFacts.IsGenerator ? createToken(SyntaxKind.AsteriskToken) : undefined, functionName, - /*typeParameters*/ [], + /*typeParameters*/[], parameters, returnType, body diff --git a/tests/cases/fourslash/extract-method5.ts b/tests/cases/fourslash/extract-method5.ts new file mode 100644 index 0000000000000..d3d8dc3dcfb3e --- /dev/null +++ b/tests/cases/fourslash/extract-method5.ts @@ -0,0 +1,18 @@ +/// + +// Extraction in the context of a literal contextual +// type needs to produce an explicit return type +// annotation in the extracted function + +//// function f() { +//// var x: 1 | 2 | 3 = /*start*/2/*end*/; +//// } + +goTo.select('start', 'end'); +edit.applyRefactor('Extract Method', 'scope_0'); +verify.currentFileContentIs( +`function f() { + var x: 1 | 2 | 3 = |newFunction(); + + function newFunction(): 1 | 2 | 3 { return 2; } +}`); \ No newline at end of file From 6428b1e11e01f63dfc62e3513464c163a3b91e7b Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 19 Jul 2017 13:46:30 -0700 Subject: [PATCH 23/55] Fix broken test --- tests/cases/fourslash/extract-method5.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/cases/fourslash/extract-method5.ts b/tests/cases/fourslash/extract-method5.ts index d3d8dc3dcfb3e..b60758bd3eefc 100644 --- a/tests/cases/fourslash/extract-method5.ts +++ b/tests/cases/fourslash/extract-method5.ts @@ -1,6 +1,6 @@ /// -// Extraction in the context of a literal contextual +// Extraction in the context of a contextual // type needs to produce an explicit return type // annotation in the extracted function @@ -12,7 +12,7 @@ goTo.select('start', 'end'); edit.applyRefactor('Extract Method', 'scope_0'); verify.currentFileContentIs( `function f() { - var x: 1 | 2 | 3 = |newFunction(); + var x: 1 | 2 | 3 = newFunction(); function newFunction(): 1 | 2 | 3 { return 2; } }`); \ No newline at end of file From 8af8ac2dfbaea2cb831c0a8bd680fe0d0e33cbf8 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 19 Jul 2017 15:00:56 -0700 Subject: [PATCH 24/55] Handle fn params and duplicated scope names properly --- src/compiler/diagnosticMessages.json | 13 --------- src/services/refactors/extractMethod.ts | 36 ++++++++++++++++++++---- src/services/textChanges.ts | 2 +- tests/cases/fourslash/extract-method6.ts | 15 ++++++++++ tests/cases/fourslash/extract-method7.ts | 14 +++++++++ 5 files changed, 61 insertions(+), 19 deletions(-) create mode 100644 tests/cases/fourslash/extract-method6.ts create mode 100644 tests/cases/fourslash/extract-method7.ts diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index 98580f942ed1e..ee8a3dc0f60d9 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -3671,18 +3671,5 @@ "Extract function into '{0}'": { "category": "Message", "code": 95004 - }, - - "Octal literal types must use ES2015 syntax. Use the syntax '{0}'.": { - "category": "Error", - "code": 8017 - }, - "Octal literals are not allowed in enums members initializer. Use the syntax '{0}'.": { - "category": "Error", - "code": 8018 - }, - "Report errors in .js files.": { - "category": "Message", - "code": 8019 } } diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 24674d19125eb..1cf8edc5d2446 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -28,12 +28,19 @@ namespace ts.refactor.extractMethod { } const actions: RefactorActionInfo[] = []; + const usedNames: Map = createMap(); + let i = 0; for (const extr of extractions) { - actions.push({ - description: formatStringFromArgs(Diagnostics.Extract_function_into_0.message, [extr.scopeDescription]), - name: `scope_${i}` - }); + // Don't issue refactorings with duplicated names + const description = formatStringFromArgs(Diagnostics.Extract_function_into_0.message, [extr.scopeDescription]); + if (!usedNames.get(description)) { + usedNames.set(description, true); + actions.push({ + description, + name: `scope_${i}` + }); + } i++; } @@ -245,6 +252,16 @@ namespace ts.refactor.extractMethod { return true; } if (!node || isFunctionLike(node) || isClassLike(node)) { + switch (node.kind) { + case SyntaxKind.FunctionDeclaration: + case SyntaxKind.ClassDeclaration: + if (node.parent.kind === SyntaxKind.SourceFile && (node.parent as ts.SourceFile).externalModuleIndicator === undefined) { + // You cannot extract global declarations + (errors || (errors = [])).push(createDiagnosticForNode(node, Messages.FunctionWillNotBeVisibleInTheNewScope)); + } + break; + } + // do not dive into functions or classes return false; } @@ -371,7 +388,16 @@ namespace ts.refactor.extractMethod { if ((current.kind === SyntaxKind.FunctionDeclaration) || isSourceFile(current) || isModuleBlock(current) || isClassLike(current)) { (scopes = scopes || []).push(current as FunctionLikeDeclaration); } - current = current.parent; + + // 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 = current.parent.parent.parent; + } + else { + current = current.parent; + } + } return scopes; } diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index d387b91ca859d..8e357f01aded4 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -566,7 +566,7 @@ namespace ts.textChanges { options.delta !== undefined ? options.delta : formatting.SmartIndenter.shouldIndentChildNode(node) - ? formatOptions.indentSize + ? (formatOptions.indentSize || 0) : 0; return applyFormatting(nonformattedText, sourceFile, initialIndentation, delta, this.rulesProvider); diff --git a/tests/cases/fourslash/extract-method6.ts b/tests/cases/fourslash/extract-method6.ts new file mode 100644 index 0000000000000..ed5e25c6e88d5 --- /dev/null +++ b/tests/cases/fourslash/extract-method6.ts @@ -0,0 +1,15 @@ +/// + +// Cannot extract globally-declared functions or +// those with non-selected local references + +//// /*f1a*/function f() { +//// /*g1a*/function g() { } +//// g();/*g1b*/ +//// g(); +//// }/*f1b*/ + +goTo.select('f1a', 'f1b'); +verify.not.refactorAvailable('Extract Method'); +goTo.select('g1a', 'g1b'); +// verify.not.refactorAvailable('Extract Method'); diff --git a/tests/cases/fourslash/extract-method7.ts b/tests/cases/fourslash/extract-method7.ts new file mode 100644 index 0000000000000..2b7fed71366af --- /dev/null +++ b/tests/cases/fourslash/extract-method7.ts @@ -0,0 +1,14 @@ +/// + +// 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. + +//// function fn(x = /*a*/3/*b*/) { +//// } + +goTo.select('a', 'b'); +edit.applyRefactor('Extract Method', 'scope_0'); +verify.currentFileContentIs(`function fn(x = newFunction()) { +} +function newFunction() { return 3; } +`); From ede6671a5247180487290399628e402fc9f558b6 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 19 Jul 2017 15:06:49 -0700 Subject: [PATCH 25/55] Re-use isJS result --- src/services/refactors/extractMethod.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 1cf8edc5d2446..4c400b9b1a5ad 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -539,7 +539,7 @@ namespace ts.refactor.extractMethod { // Provide explicit return types for contexutally-typed functions // to avoid problems when there are literal types present - if (isExpression(node) && !isInJavaScriptFile(node)) { + if (isExpression(node) && !isJS) { const contextualType = checker.getContextualType(node); returnType = checker.typeToTypeNode(contextualType); } @@ -549,7 +549,7 @@ namespace ts.refactor.extractMethod { if (isClassLike(scope)) { // always create private method in TypeScript files - const modifiers: Modifier[] = isInJavaScriptFile(node) ? [] : [createToken(SyntaxKind.PrivateKeyword)]; + const modifiers: Modifier[] = isJS ? [] : [createToken(SyntaxKind.PrivateKeyword)]; if (range.facts & RangeFacts.IsAsyncFunction) { modifiers.push(createToken(SyntaxKind.AsyncKeyword)); } From 55b28ad8adeb3d2c7c69db1302872424950dd20d Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 19 Jul 2017 15:20:56 -0700 Subject: [PATCH 26/55] You may not extract-method on exported declarations --- src/services/refactors/extractMethod.ts | 19 +++++++++++++++++-- tests/cases/fourslash/extract-method8.ts | 17 +++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) create mode 100644 tests/cases/fourslash/extract-method8.ts diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 4c400b9b1a5ad..b0119e17a3f42 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -90,6 +90,7 @@ namespace ts.refactor.extractMethod { 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 CannotExtractExportedEntity = createMessage("Cannot extract exported declaration"); } export enum RangeFacts { @@ -106,6 +107,11 @@ namespace ts.refactor.extractMethod { export interface TargetRange { readonly range: Expression | Statement[]; readonly facts: RangeFacts; + /** + * A list of symbols that are declared in the selected range. + * Used to ensure we don't turn something used outside the range free (or worse, resolve to a different entity). + */ + readonly declarations: Symbol[]; } /** @@ -149,6 +155,8 @@ namespace ts.refactor.extractMethod { // Do the same for the ending position let end = getParentNodeInSpan(findTokenOnLeftOfPosition(sourceFile, textSpanEnd(span)), sourceFile, span); + const declarations: Symbol[] = []; + // We'll modify these flags as we walk the tree to collect data // about what things need to be done as part of the extraction. let rangeFacts = RangeFacts.None; @@ -202,7 +210,7 @@ namespace ts.refactor.extractMethod { break; } } - return { targetRange: { range: statements, facts: rangeFacts } }; + return { targetRange: { range: statements, facts: rangeFacts, declarations } }; } else { const errors = checkRootNode(start) || checkNode(start); @@ -212,7 +220,7 @@ namespace ts.refactor.extractMethod { const range = isStatement(start) ? [start] : start; - return { targetRange: { range, facts: rangeFacts } }; + return { targetRange: { range, facts: rangeFacts, declarations } }; } function createErrorResult(sourceFile: SourceFile, start: number, length: number, message: DiagnosticMessage): RangeToExtract { @@ -238,6 +246,13 @@ namespace ts.refactor.extractMethod { return [createDiagnosticForNode(nodeToCheck, Messages.StatementOrExpressionExpected)]; } + if (isDeclaration(nodeToCheck)) { + if (hasModifier(nodeToCheck, ModifierFlags.Export)) { + return [createDiagnosticForNode(nodeToCheck, Messages.CannotExtractExportedEntity)]; + } + declarations.push(nodeToCheck.symbol); + } + let errors: Diagnostic[]; let permittedJumps = PermittedJumps.Return; let seenLabels: Array<__String>; diff --git a/tests/cases/fourslash/extract-method8.ts b/tests/cases/fourslash/extract-method8.ts new file mode 100644 index 0000000000000..28068dd7c783b --- /dev/null +++ b/tests/cases/fourslash/extract-method8.ts @@ -0,0 +1,17 @@ +/// + +// You cannot extract an exported function declaration + +//// namespace ns { +//// /*a*/export function fn() { +//// +//// } +//// fn(); +//// /*b*/ +//// } + +goTo.select('a', 'b'); +verify.not.refactorAvailable("Extract Method"); +edit.deleteAtCaret('export'.length); +goTo.select('a', 'b'); +verify.refactorAvailable("Extract Method"); From 2d9eaa90fff62acaeacd84f07902a95fc0aa4df4 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 19 Jul 2017 17:36:45 -0700 Subject: [PATCH 27/55] Correctly apply overlapped edits in fourslash --- src/harness/fourslash.ts | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 135e40630d16f..4932d52cf7341 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -991,9 +991,9 @@ namespace FourSlash { } for (const reference of expectedReferences) { - const {fileName, start, end} = reference; + const { fileName, start, end } = reference; if (reference.marker && reference.marker.data) { - const {isWriteAccess, isDefinition} = reference.marker.data; + const { isWriteAccess, isDefinition } = reference.marker.data; this.verifyReferencesWorker(actualReferences, fileName, start, end, isWriteAccess, isDefinition); } else { @@ -1204,7 +1204,7 @@ namespace FourSlash { displayParts: ts.SymbolDisplayPart[], documentation: ts.SymbolDisplayPart[], tags: ts.JSDocTagInfo[] - ) { + ) { const actualQuickInfo = this.languageService.getQuickInfoAtPosition(this.activeFile.fileName, this.currentCaretPosition); assert.equal(actualQuickInfo.kind, kind, this.messageAtLastKnownMarker("QuickInfo kind")); @@ -1800,19 +1800,16 @@ namespace FourSlash { // We get back a set of edits, but langSvc.editScript only accepts one at a time. Use this to keep track // of the incremental offset from each edit to the next. We assume these edit ranges don't overlap - edits = edits.sort((a, b) => a.span.start - b.span.start); - for (let i = 0; i < edits.length - 1; i++) { - const firstEditSpan = edits[i].span; - const firstEditEnd = firstEditSpan.start + firstEditSpan.length; - assert.isTrue(firstEditEnd <= edits[i + 1].span.start); - } + // Copy this so we don't ruin someone else's copy + edits = JSON.parse(JSON.stringify(edits)); // Get a snapshot of the content of the file so we can make sure any formatting edits didn't destroy non-whitespace characters const oldContent = this.getFileContent(fileName); let runningOffset = 0; - for (const edit of edits) { - const offsetStart = edit.span.start + runningOffset; + for (let i = 0; i < edits.length; i++) { + const edit = edits[i]; + const offsetStart = edit.span.start; const offsetEnd = offsetStart + edit.span.length; this.editScriptAndUpdateMarkers(fileName, offsetStart, offsetEnd, edit.newText); const editDelta = edit.newText.length - edit.span.length; @@ -1827,8 +1824,13 @@ namespace FourSlash { } } runningOffset += editDelta; - // TODO: Consider doing this at least some of the time for higher fidelity. Currently causes a failure (bug 707150) - // this.languageService.getScriptLexicalStructure(fileName); + + // Update positions of any future edits affected by this change + for (let j = i + 1; j < edits.length; j++) { + if (edits[j].span.start >= edits[i].span.start) { + edits[j].span.start += editDelta; + } + } } if (isFormattingEdit) { @@ -1912,7 +1914,7 @@ namespace FourSlash { this.goToPosition(len); } - public goToRangeStart({fileName, start}: Range) { + public goToRangeStart({ fileName, start }: Range) { this.openFile(fileName); this.goToPosition(start); } @@ -2086,7 +2088,7 @@ namespace FourSlash { return result; } - private rangeText({fileName, start, end}: Range): string { + private rangeText({ fileName, start, end }: Range): string { return this.getFileContent(fileName).slice(start, end); } @@ -2372,7 +2374,7 @@ namespace FourSlash { private applyCodeActions(actions: ts.CodeAction[], index?: number): void { if (index === undefined) { if (!(actions && actions.length === 1)) { - this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found. ${actions ? actions.map(a => `${Harness.IO.newLine()} "${a.description}"`) : "" }`); + this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found. ${actions ? actions.map(a => `${Harness.IO.newLine()} "${a.description}"`) : ""}`); } index = 0; } @@ -2785,7 +2787,7 @@ namespace FourSlash { public applyRefactor(refactorName: string, actionName: string) { const range = { pos: this.currentCaretPosition, end: this.selectionEnd === -1 ? this.currentCaretPosition : this.selectionEnd }; const refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, range); - const refactor = ts.find(refactors, r => r.name === refactorName); + const refactor = ts.find(refactors, r => r.name === refactorName); if (!refactor) { this.raiseError(`The expected refactor: ${refactorName} is not available at the marker location.`); } From 2f2c780d1874ab966b3221207b680d403f340275 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Thu, 20 Jul 2017 15:15:30 -0700 Subject: [PATCH 28/55] Fix some parenting issues & start detecting stranded references --- src/services/refactors/extractMethod.ts | 37 +++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index b0119e17a3f42..6a3c8c3aaea75 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -392,6 +392,9 @@ namespace ts.refactor.extractMethod { } } + // The scope can't be the initial node itself + current = current && current.parent; + let scopes: Scope[] | undefined = undefined; while (current) { // We want to find the nearest parent where we can place an "equivalent" sibling to the node we're extracting out of. @@ -552,6 +555,20 @@ namespace ts.refactor.extractMethod { callArguments.push(createIdentifier(name)); }); + // Verify no usages of declarations originating in the extracted range are used in + // the extracted-to scope (since they will no longer be visible) + const declarations = range.declarations; + if (declarations.length) { + const stranded = getStrandedReferences(scope); + if (stranded) { + return { + scope, + scopeDescription: getDescriptionForScope(scope), + errors: [createDiagnosticForNode(stranded.declarations[0], Messages.FunctionWillNotBeVisibleInTheNewScope, stranded.getUnescapedName())] + }; + } + } + // Provide explicit return types for contexutally-typed functions // to avoid problems when there are literal types present if (isExpression(node) && !isJS) { @@ -657,6 +674,26 @@ namespace ts.refactor.extractMethod { changes: changeTracker.getChanges() }; + function getStrandedReferences(node: Node): Symbol | undefined { + if (node.getStart() < context.startPosition || node.getEnd() > context.endPosition) { + let foundDeclaration: Symbol | undefined = undefined; + ts.forEachChild(node, reference => { + const ref = reference.symbol; + if (ref) { + for (const decl of declarations) { + if (decl === ref) { + foundDeclaration = decl; + return true; + } + } + } + return getStrandedReferences(reference); + }); + return foundDeclaration; + } + return undefined; + } + function getPropertyAssignmentsForWrites(writes: UsageEntry[]) { return writes.map(w => createShorthandPropertyAssignment(w.symbol.getUnescapedName())); } From 63193da1015fe09d9bb60a75fbc6e97bdf3efec9 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Thu, 20 Jul 2017 15:15:58 -0700 Subject: [PATCH 29/55] Add return type annotation and regression test for a prior assert --- src/services/textChanges.ts | 2 +- tests/cases/fourslash/extract-method10.ts | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 tests/cases/fourslash/extract-method10.ts diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 8e357f01aded4..777e97f7c23df 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -547,7 +547,7 @@ namespace ts.textChanges { return (options.prefix || "") + text + (options.suffix || ""); } - private getFormattedTextOfNode(node: Node, sourceFile: SourceFile, pos: number, options: ChangeNodeOptions) { + private getFormattedTextOfNode(node: Node, sourceFile: SourceFile, pos: number, options: ChangeNodeOptions): string { const nonformattedText = getNonformattedText(node, sourceFile, this.newLine); if (this.validator) { this.validator(nonformattedText); diff --git a/tests/cases/fourslash/extract-method10.ts b/tests/cases/fourslash/extract-method10.ts new file mode 100644 index 0000000000000..1a02bfa00f53e --- /dev/null +++ b/tests/cases/fourslash/extract-method10.ts @@ -0,0 +1,6 @@ +/// + +//// (x => x)(/*1*/x => x/*2*/)(1); + +goTo.select('1', '2'); +edit.applyRefactor('Extract Method', 'scope_0'); From 889292a8a65000044fe969176ebf46512d62c8dc Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Mon, 24 Jul 2017 10:48:58 -0700 Subject: [PATCH 30/55] Detect would-be stranded references to extracted declarations --- src/services/refactors/extractMethod.ts | 98 ++++++++++++++----------- 1 file changed, 57 insertions(+), 41 deletions(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 6a3c8c3aaea75..f839bc191d5ac 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -32,6 +32,11 @@ namespace ts.refactor.extractMethod { let i = 0; for (const extr of extractions) { + // Skip these since we don't have a way to report errors yet + if (extr.errors && extr.errors.length) { + continue; + } + // Don't issue refactorings with duplicated names const description = formatStringFromArgs(Diagnostics.Extract_function_into_0.message, [extr.scopeDescription]); if (!usedNames.get(description)) { @@ -44,6 +49,10 @@ namespace ts.refactor.extractMethod { i++; } + if (actions.length === 0) { + return undefined; + } + return [{ name: extractMethod.name, description: extractMethod.description, @@ -108,7 +117,7 @@ namespace ts.refactor.extractMethod { readonly range: Expression | Statement[]; readonly facts: RangeFacts; /** - * A list of symbols that are declared in the selected range. + * A list of symbols that are declared in the selected range which are visible in the containing lexical scope * Used to ensure we don't turn something used outside the range free (or worse, resolve to a different entity). */ readonly declarations: Symbol[]; @@ -377,6 +386,11 @@ namespace ts.refactor.extractMethod { } } + function isValidExtractionTarget(node: Node) { + // 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); + } + /** * Computes possible places we could extract the function into. For example, * you may be able to extract into a class method *or* local closure *or* namespace function, @@ -392,8 +406,7 @@ namespace ts.refactor.extractMethod { } } - // The scope can't be the initial node itself - current = current && current.parent; + const start = current; let scopes: Scope[] | undefined = undefined; while (current) { @@ -402,8 +415,7 @@ namespace ts.refactor.extractMethod { // * Function declaration // * Class declaration or expression // * Module/namespace or source file - // Note that we don't use isFunctionLike because we don't want to put the extracted closure *inside* a method - if ((current.kind === SyntaxKind.FunctionDeclaration) || isSourceFile(current) || isModuleBlock(current) || isClassLike(current)) { + if (current !== start && isValidExtractionTarget(current)) { (scopes = scopes || []).push(current as FunctionLikeDeclaration); } @@ -555,20 +567,6 @@ namespace ts.refactor.extractMethod { callArguments.push(createIdentifier(name)); }); - // Verify no usages of declarations originating in the extracted range are used in - // the extracted-to scope (since they will no longer be visible) - const declarations = range.declarations; - if (declarations.length) { - const stranded = getStrandedReferences(scope); - if (stranded) { - return { - scope, - scopeDescription: getDescriptionForScope(scope), - errors: [createDiagnosticForNode(stranded.declarations[0], Messages.FunctionWillNotBeVisibleInTheNewScope, stranded.getUnescapedName())] - }; - } - } - // Provide explicit return types for contexutally-typed functions // to avoid problems when there are literal types present if (isExpression(node) && !isJS) { @@ -674,26 +672,6 @@ namespace ts.refactor.extractMethod { changes: changeTracker.getChanges() }; - function getStrandedReferences(node: Node): Symbol | undefined { - if (node.getStart() < context.startPosition || node.getEnd() > context.endPosition) { - let foundDeclaration: Symbol | undefined = undefined; - ts.forEachChild(node, reference => { - const ref = reference.symbol; - if (ref) { - for (const decl of declarations) { - if (decl === ref) { - foundDeclaration = decl; - return true; - } - } - } - return getStrandedReferences(reference); - }); - return foundDeclaration; - } - return undefined; - } - function getPropertyAssignmentsForWrites(writes: UsageEntry[]) { return writes.map(w => createShorthandPropertyAssignment(w.symbol.getUnescapedName())); } @@ -750,6 +728,15 @@ namespace ts.refactor.extractMethod { return isArray(v); } + /** + * Produces a range that spans the entirety of nodes, given a selection + * that might start/end in the middle of nodes. + * + * For example, when the user makes a selection like this + * v---v + * var someThing = foo + bar; + * this returns ^-------^ + */ function getEnclosingTextRange(targetRange: TargetRange, sourceFile: SourceFile): TextRange { return isReadonlyArray(targetRange.range) ? { pos: targetRange.range[0].getStart(sourceFile), end: targetRange.range[targetRange.range.length - 1].getEnd() } @@ -784,6 +771,7 @@ namespace ts.refactor.extractMethod { const usagesPerScope: ScopeUsages[] = []; const substitutionsPerScope: Map[] = []; const errorsPerScope: Diagnostic[][] = []; + const visibleDeclarationsInExtractedRange: Symbol[] = []; // initialize results for (const _ of scopes) { @@ -792,16 +780,25 @@ namespace ts.refactor.extractMethod { errorsPerScope.push([]); } const seenUsages = createMap(); - let valueUsage = Usage.Read; - const target = isReadonlyArray(targetRange.range) ? createBlock(targetRange.range) : targetRange.range; + const containingLexicalScopeOfExtraction = isBlockScope(scopes[0], scopes[0].parent) ? scopes[0] : getEnclosingBlockScopeContainer(scopes[0]); forEachChild(target, collectUsages); + // If there are any declarations in the extracted block that are used in the same enclosing + // lexical scope, we can't move the extraction "up" as those declarations will become unreachable + if (visibleDeclarationsInExtractedRange.length) { + forEachChild(containingLexicalScopeOfExtraction, checkForUsedDeclarations); + } + return { target, usagesPerScope, errorsPerScope }; function collectUsages(node: Node) { + if (isDeclaration(node) && node.symbol) { + visibleDeclarationsInExtractedRange.push(node.symbol); + } + if (isAssignmentExpression(node)) { const savedValueUsage = valueUsage; // use 'write' as default usage for values @@ -915,6 +912,25 @@ namespace ts.refactor.extractMethod { return symbolId; } + function checkForUsedDeclarations(node: Node) { + // If this node is entirely within the original extraction range, we don't need to do anything. + if (node === targetRange.range || (isArray(targetRange.range) && targetRange.range.indexOf(node as Statement) >= 0)) { + return; + } + + // 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)); + } + return true; + } + else { + forEachChild(node, checkForUsedDeclarations); + } + } + function tryReplaceWithQualifiedNameOrPropertyAccess(symbol: Symbol, scopeDecl: Node, isTypeNode: boolean): PropertyAccessExpression | EntityName { if (!symbol) { return undefined; From e229d42e16fee0e09e8e4ac75b4c0a71fff9bcfe Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Mon, 24 Jul 2017 10:49:10 -0700 Subject: [PATCH 31/55] Tests --- tests/cases/fourslash/extract-method6.ts | 3 ++- tests/cases/fourslash/extract-method7.ts | 3 +-- tests/cases/fourslash/extract-method9.ts | 11 +++++++++++ 3 files changed, 14 insertions(+), 3 deletions(-) create mode 100644 tests/cases/fourslash/extract-method9.ts diff --git a/tests/cases/fourslash/extract-method6.ts b/tests/cases/fourslash/extract-method6.ts index ed5e25c6e88d5..f188ab319e9b8 100644 --- a/tests/cases/fourslash/extract-method6.ts +++ b/tests/cases/fourslash/extract-method6.ts @@ -12,4 +12,5 @@ goTo.select('f1a', 'f1b'); verify.not.refactorAvailable('Extract Method'); goTo.select('g1a', 'g1b'); -// verify.not.refactorAvailable('Extract Method'); +verify.not.refactorAvailable('Extract Method'); + diff --git a/tests/cases/fourslash/extract-method7.ts b/tests/cases/fourslash/extract-method7.ts index 2b7fed71366af..c4c7350e46f44 100644 --- a/tests/cases/fourslash/extract-method7.ts +++ b/tests/cases/fourslash/extract-method7.ts @@ -10,5 +10,4 @@ goTo.select('a', 'b'); edit.applyRefactor('Extract Method', 'scope_0'); verify.currentFileContentIs(`function fn(x = newFunction()) { } -function newFunction() { return 3; } -`); +function newFunction() { return 3; }`, true); diff --git a/tests/cases/fourslash/extract-method9.ts b/tests/cases/fourslash/extract-method9.ts new file mode 100644 index 0000000000000..f70ef20a87b56 --- /dev/null +++ b/tests/cases/fourslash/extract-method9.ts @@ -0,0 +1,11 @@ +/// + +//// function f() { +//// /*a*/function q() { } +//// q();/*b*/ +//// q(); +//// } + +goTo.select('a', 'b'); +verify.not.refactorAvailable("Extract Method"); + From 7b2dd8a55d89d6c72df8bd403d4f3d1eac2b4b78 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Mon, 24 Jul 2017 10:53:21 -0700 Subject: [PATCH 32/55] Fix line ending --- tests/cases/fourslash/extract-method7.ts | 27 ++++++++++++------------ 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/tests/cases/fourslash/extract-method7.ts b/tests/cases/fourslash/extract-method7.ts index c4c7350e46f44..3b1c71f72c4ad 100644 --- a/tests/cases/fourslash/extract-method7.ts +++ b/tests/cases/fourslash/extract-method7.ts @@ -1,13 +1,14 @@ -/// - -// 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. - -//// function fn(x = /*a*/3/*b*/) { -//// } - -goTo.select('a', 'b'); -edit.applyRefactor('Extract Method', 'scope_0'); -verify.currentFileContentIs(`function fn(x = newFunction()) { -} -function newFunction() { return 3; }`, true); +/// + +// 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. + +//// function fn(x = /*a*/3/*b*/) { +//// } + +goTo.select('a', 'b'); +edit.applyRefactor('Extract Method', 'scope_0'); +verify.currentFileContentIs(`function fn(x = newFunction()) { +} +function newFunction() { return 3; } +`, true); From bf6c0a9026d71bcfea0ca93316fec562419c2710 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Tue, 25 Jul 2017 13:51:38 -0700 Subject: [PATCH 33/55] Function blocks aren't statements! --- src/compiler/utilities.ts | 2 +- tests/cases/fourslash/completionListIsGlobalCompletion.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 2d856521ea34e..61247637aeaf6 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -5335,7 +5335,7 @@ namespace ts { const kind = node.kind; return isStatementKindButNotDeclarationKind(kind) || isDeclarationStatementKind(kind) - || kind === SyntaxKind.Block; + || (kind === SyntaxKind.Block && !isFunctionBlock(node)); } // Module references diff --git a/tests/cases/fourslash/completionListIsGlobalCompletion.ts b/tests/cases/fourslash/completionListIsGlobalCompletion.ts index 121ab940d6549..49196bdabd4f6 100644 --- a/tests/cases/fourslash/completionListIsGlobalCompletion.ts +++ b/tests/cases/fourslash/completionListIsGlobalCompletion.ts @@ -47,7 +47,7 @@ verify.completionListIsGlobal(true); goTo.marker("6"); verify.completionListIsGlobal(false); goTo.marker("7"); -verify.completionListIsGlobal(true); +verify.completionListIsGlobal(false); goTo.marker("8"); verify.completionListIsGlobal(false); goTo.marker("9"); From 3424312d78d31e5eb13109598884d8981974a234 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Tue, 25 Jul 2017 13:51:49 -0700 Subject: [PATCH 34/55] Disallow certain classes of refactors --- src/services/refactors/extractMethod.ts | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index f839bc191d5ac..cd0665bbefff9 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -275,6 +275,29 @@ namespace ts.refactor.extractMethod { // already found an error - can stop now return true; } + + // Some things can't be extracted in certain situations + switch (node.kind) { + case SyntaxKind.ImportDeclaration: + (errors || (errors = [])).push(createDiagnosticForNode(node, Messages.CannotExtractFunction)); + return true; + case SyntaxKind.SuperKeyword: + // For a super *constructor call*, we have to be extracting the entire class, + // but a super *method call* simply implies a 'this' reference + if (node.parent.kind === SyntaxKind.CallExpression) { + // 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)); + return true; + } + } + else { + rangeFacts |= RangeFacts.UsesThis; + } + break; + } + if (!node || isFunctionLike(node) || isClassLike(node)) { switch (node.kind) { case SyntaxKind.FunctionDeclaration: From ff18df3300f40e38deb508c03dbefe0103b681a2 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Tue, 25 Jul 2017 13:51:58 -0700 Subject: [PATCH 35/55] Add test --- tests/cases/fourslash/extract-method11.ts | 26 +++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 tests/cases/fourslash/extract-method11.ts diff --git a/tests/cases/fourslash/extract-method11.ts b/tests/cases/fourslash/extract-method11.ts new file mode 100644 index 0000000000000..2aa3200ae700f --- /dev/null +++ b/tests/cases/fourslash/extract-method11.ts @@ -0,0 +1,26 @@ +/// + +// Nonexhaustive list of things it should be illegal to be extract-method on + +// * Import declarations +// * Super calls +// * Function body blocks + +//// /*1a*/import * as x from 'y';/*1b*/ +//// namespace N { +//// /*oka*/class C extends B { +//// constructor() { +//// /*2a*/super();/*2b*/ +//// } +//// }/*okb*/ +//// } +//// function f() /*3a*/{ return 0 }/*3b*/ + +for (const m of ['1', '2', '3']) { + goTo.select(m + 'a', m + 'b'); + verify.not.refactorAvailable('Extract Method'); +} + +// Verify we can still extract the entire class +goTo.select('oka', 'okb'); +verify.refactorAvailable('Extract Method'); From 7f29fcddc2c8ac87f40aac2b3d36c7cb8793cf5c Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Tue, 25 Jul 2017 16:30:42 -0700 Subject: [PATCH 36/55] Handle static methods and static method name conflicts --- src/services/refactors/extractMethod.ts | 59 ++++++++++++++++++++--- tests/cases/fourslash/extract-method13.ts | 26 ++++++++++ 2 files changed, 77 insertions(+), 8 deletions(-) create mode 100644 tests/cases/fourslash/extract-method13.ts diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index cd0665bbefff9..8fac9604504df 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -107,7 +107,11 @@ namespace ts.refactor.extractMethod { HasReturn = 1 << 0, IsGenerator = 1 << 1, IsAsyncFunction = 1 << 2, - UsesThis = 1 << 3 + UsesThis = 1 << 3, + /** + * The range is in a function which needs the 'static' modifier in a class + */ + InStaticRegion = 1 << 4 } /** @@ -262,6 +266,28 @@ namespace ts.refactor.extractMethod { declarations.push(nodeToCheck.symbol); } + // If we're in a class, see if we're in a static region (static property initializer; class constructor parameter default) or not + const stoppingPoint: Node = getContainingClass(nodeToCheck); + if (stoppingPoint) { + let current: Node = nodeToCheck; + while (current !== stoppingPoint) { + if (current.kind === SyntaxKind.PropertyDeclaration) { + if (hasModifier(current, ModifierFlags.Static)) { + rangeFacts |= RangeFacts.InStaticRegion; + } + break; + } + else if (current.kind === SyntaxKind.Parameter) { + const ctorOrMethod = getContainingFunction(current); + if (ctorOrMethod.kind === SyntaxKind.Constructor) { + rangeFacts |= RangeFacts.InStaticRegion; + } + break; + } + current = current.parent; + } + } + let errors: Diagnostic[]; let permittedJumps = PermittedJumps.Return; let seenLabels: Array<__String>; @@ -542,6 +568,18 @@ namespace ts.refactor.extractMethod { } } + function getUniqueName(isNameOkay: (name: __String) => boolean) { + let functionNameText = "newFunction" as __String; + if (isNameOkay(functionNameText)) { + return functionNameText; + } + let i = 1; + while (!isNameOkay(functionNameText = `newFunction_${i}` as __String)) { + i++; + } + return functionNameText; + } + export function extractFunctionInScope( node: Node, scope: Scope, @@ -552,12 +590,14 @@ namespace ts.refactor.extractMethod { const checker = context.program.getTypeChecker(); // Make a unique name for the extracted function - let functionNameText = "newFunction" as __String; - if (scope.locals && scope.locals.has(functionNameText)) { - let i = 1; - while (scope.locals.has(functionNameText = `newFunction_${i}` as __String)) { - i++; - } + let functionNameText: __String; + if (isClassLike(scope)) { + const type = range.facts & RangeFacts.InStaticRegion ? checker.getTypeOfSymbolAtLocation(scope.symbol, scope) : checker.getDeclaredTypeOfSymbol(scope.symbol); + const props = checker.getPropertiesOfType(type); + functionNameText = getUniqueName(n => props.every(p => p.name !== n)); + } + else { + functionNameText = getUniqueName(n => !(scope.locals && scope.locals.has(n))); } const isJS = isInJavaScriptFile(node); @@ -606,6 +646,9 @@ namespace ts.refactor.extractMethod { if (range.facts & RangeFacts.IsAsyncFunction) { modifiers.push(createToken(SyntaxKind.AsyncKeyword)); } + if (range.facts & RangeFacts.InStaticRegion) { + modifiers.push(createToken(SyntaxKind.StaticKeyword)); + } newFunction = createMethod( /*decorators*/ undefined, modifiers, @@ -638,7 +681,7 @@ namespace ts.refactor.extractMethod { const newNodes: Node[] = []; // replace range with function call let call: Expression = createCall( - isClassLike(scope) ? createPropertyAccess(createThis(), functionName) : functionName, + isClassLike(scope) ? createPropertyAccess(range.facts & RangeFacts.InStaticRegion ? createIdentifier(scope.name.getText()) : createThis(), functionName) : functionName, /*typeArguments*/ undefined, callArguments); if (range.facts & RangeFacts.IsGenerator) { diff --git a/tests/cases/fourslash/extract-method13.ts b/tests/cases/fourslash/extract-method13.ts new file mode 100644 index 0000000000000..fea65c65aad0d --- /dev/null +++ b/tests/cases/fourslash/extract-method13.ts @@ -0,0 +1,26 @@ +/// + +// Extracting from a static context should make static methods. +// Also checks that we correctly find non-conflicting names in static contexts. + +//// class C { +//// static j = /*c*/100/*d*/; +//// constructor(q: string = /*a*/"hello"/*b*/) { +//// } +//// } + +goTo.select('a', 'b'); +edit.applyRefactor('Extract Method', 'scope_0'); + +goTo.select('c', 'd'); +edit.applyRefactor('Extract Method', 'scope_0'); + +verify.currentFileContentIs(`class C { + static j = C.newFunction_1(); + constructor(q: string = C.newFunction()) { + } + + private static newFunction(): string { return "hello"; } + + private static newFunction_1() { return 100; } +}`); \ No newline at end of file From dda54963b416192185befb04dcdd853fd3dea3ee Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Thu, 27 Jul 2017 17:44:50 -0700 Subject: [PATCH 37/55] No extracting other non-statement blocks --- src/compiler/utilities.ts | 5 ++++- tests/cases/fourslash/extract-method11.ts | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index af4d011878da5..d3f48823b0794 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -5336,7 +5336,10 @@ namespace ts { const kind = node.kind; return isStatementKindButNotDeclarationKind(kind) || isDeclarationStatementKind(kind) - || (kind === SyntaxKind.Block && !isFunctionBlock(node)); + || (kind === SyntaxKind.Block && + node.parent.kind !== SyntaxKind.TryStatement && + node.parent.kind !== SyntaxKind.CatchClause && + !isFunctionBlock(node)); } // Module references diff --git a/tests/cases/fourslash/extract-method11.ts b/tests/cases/fourslash/extract-method11.ts index 2aa3200ae700f..705f2373104de 100644 --- a/tests/cases/fourslash/extract-method11.ts +++ b/tests/cases/fourslash/extract-method11.ts @@ -5,6 +5,7 @@ // * Import declarations // * Super calls // * Function body blocks +// * try/catch blocks //// /*1a*/import * as x from 'y';/*1b*/ //// namespace N { @@ -15,8 +16,9 @@ //// }/*okb*/ //// } //// function f() /*3a*/{ return 0 }/*3b*/ +//// try /*4a*/{ console.log }/*4b*/ catch (e) /*5a*/{ console.log; }/*5b*/ -for (const m of ['1', '2', '3']) { +for (const m of ['1', '2', '3', '4', '5']) { goTo.select(m + 'a', m + 'b'); verify.not.refactorAvailable('Extract Method'); } From e88ff21e6c819f461cc4a6a89fc794e48d81ce32 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Thu, 27 Jul 2017 18:30:34 -0700 Subject: [PATCH 38/55] More disallowed extraction position --- src/services/refactors/extractMethod.ts | 32 ++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index f1af98b8e4545..dd2900a54ea6c 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -255,7 +255,7 @@ namespace ts.refactor.extractMethod { Continue = 1 << 1, Return = 1 << 2 } - if (!isStatement(nodeToCheck) && !isExpression(nodeToCheck)) { + if (!isStatement(nodeToCheck) || !isExpression(nodeToCheck) || !isLegalExpressionExtraction(nodeToCheck)) { return [createDiagnosticForNode(nodeToCheck, Messages.StatementOrExpressionExpected)]; } @@ -1030,6 +1030,36 @@ namespace ts.refactor.extractMethod { node.getEnd() <= textSpanEnd(span); } + /** + * Computes whether or not a node represents an expression in a position where it could + * be extracted. + * The isExpression() in utilities.ts returns some false positives we need to handle, + * such as `import x from 'y'` -- the 'y' is a StringLiteral but is *not* an expression + * in the sense of something that you could extract on + */ + function isLegalExpressionExtraction(node: Node): boolean { + switch (node.parent.kind) { + case SyntaxKind.EnumMember: + return false; + } + + switch (node.kind) { + case SyntaxKind.StringLiteral: + return node.parent.kind !== SyntaxKind.ImportDeclaration && + node.parent.kind !== SyntaxKind.ImportSpecifier; + + case SyntaxKind.SpreadElement: + case SyntaxKind.ObjectBindingPattern: + case SyntaxKind.BindingElement: + return false; + + case SyntaxKind.Identifier: + return node.parent.kind !== SyntaxKind.BindingElement && + node.parent.kind !== SyntaxKind.ImportSpecifier; + } + return true; + } + function isBlockLike(node: Node): node is BlockLike { switch (node.kind) { case SyntaxKind.Block: From bd8000524bae6eba71bbd945bfcbf7f924c52dcb Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Thu, 27 Jul 2017 18:40:10 -0700 Subject: [PATCH 39/55] Block more nonworking extraction locations --- src/compiler/utilities.ts | 15 +++++++++++---- src/services/refactors/extractMethod.ts | 2 +- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index d3f48823b0794..ca9a39a000b5a 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -5336,10 +5336,17 @@ namespace ts { const kind = node.kind; return isStatementKindButNotDeclarationKind(kind) || isDeclarationStatementKind(kind) - || (kind === SyntaxKind.Block && - node.parent.kind !== SyntaxKind.TryStatement && - node.parent.kind !== SyntaxKind.CatchClause && - !isFunctionBlock(node)); + || isBlockStatement(node); + } + + function isBlockStatement(node: Node): node is Block { + if (node.kind !== SyntaxKind.Block) return false; + if (node.parent !== undefined) { + if (node.parent.kind === SyntaxKind.TryStatement || node.parent.kind === SyntaxKind.CatchClause) { + return false; + } + } + return !isFunctionBlock(node); } // Module references diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index dd2900a54ea6c..d76a1fec19169 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -255,7 +255,7 @@ namespace ts.refactor.extractMethod { Continue = 1 << 1, Return = 1 << 2 } - if (!isStatement(nodeToCheck) || !isExpression(nodeToCheck) || !isLegalExpressionExtraction(nodeToCheck)) { + if (!isStatement(nodeToCheck) && !(isExpression(nodeToCheck) && isLegalExpressionExtraction(nodeToCheck))) { return [createDiagnosticForNode(nodeToCheck, Messages.StatementOrExpressionExpected)]; } From ff716d06d084a05ea7983cb31efe988e433b1b64 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Fri, 28 Jul 2017 11:19:23 -0700 Subject: [PATCH 40/55] Properly enumerate class blocks --- src/compiler/transformers/es2015.ts | 2 +- src/compiler/utilities.ts | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/compiler/transformers/es2015.ts b/src/compiler/transformers/es2015.ts index f97de018d4ad0..da827d7b63e9d 100644 --- a/src/compiler/transformers/es2015.ts +++ b/src/compiler/transformers/es2015.ts @@ -394,7 +394,7 @@ namespace ts { function shouldVisitNode(node: Node): boolean { return (node.transformFlags & TransformFlags.ContainsES2015) !== 0 || convertedLoopState !== undefined - || (hierarchyFacts & HierarchyFacts.ConstructorWithCapturedSuper && isStatement(node)) + || (hierarchyFacts & HierarchyFacts.ConstructorWithCapturedSuper && isStatementOrBlock(node)) || (isIterationStatement(node, /*lookInLabeledStatements*/ false) && shouldConvertIterationStatementBody(node)) || isTypeScriptClassWrapper(node); } diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index ca9a39a000b5a..b8c7f127fbc5a 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -5339,6 +5339,14 @@ namespace ts { || isBlockStatement(node); } + /* @internal */ + export function isStatementOrBlock(node: Node): node is Statement { + const kind = node.kind; + return isStatementKindButNotDeclarationKind(kind) + || isDeclarationStatementKind(kind) + || node.kind === SyntaxKind.Block; + } + function isBlockStatement(node: Node): node is Block { if (node.kind !== SyntaxKind.Block) return false; if (node.parent !== undefined) { From 1291cfda85786bb3411ab857929724d46f287a62 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Fri, 28 Jul 2017 14:41:48 -0700 Subject: [PATCH 41/55] Add tests task --- .vscode/tasks.json | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.vscode/tasks.json b/.vscode/tasks.json index f3c59a6171768..31928f73ce217 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -18,6 +18,13 @@ "problemMatcher": [ "$tsc" ] + }, + { + "taskName": "tests", + "showOutput": "silent", + "problemMatcher": [ + "$tsc" + ] } ] } \ No newline at end of file From a26790f9902f9feed0692e0b7c00dbe7e897fb2a Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Fri, 28 Jul 2017 14:42:07 -0700 Subject: [PATCH 42/55] Don't re-use nodes --- src/services/refactors/extractMethod.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index d76a1fec19169..3922bee94c9f7 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -602,6 +602,8 @@ namespace ts.refactor.extractMethod { const isJS = isInJavaScriptFile(node); const functionName = createIdentifier(functionNameText as string); + const functionReference = createIdentifier(functionNameText as string); + let returnType: TypeNode = undefined; const parameters: ParameterDeclaration[] = []; const callArguments: Identifier[] = []; @@ -681,7 +683,7 @@ namespace ts.refactor.extractMethod { const newNodes: Node[] = []; // replace range with function call let call: Expression = createCall( - isClassLike(scope) ? createPropertyAccess(range.facts & RangeFacts.InStaticRegion ? createIdentifier(scope.name.getText()) : createThis(), functionName) : functionName, + isClassLike(scope) ? createPropertyAccess(range.facts & RangeFacts.InStaticRegion ? createIdentifier(scope.name.getText()) : createThis(), functionReference) : functionReference, /*typeArguments*/ undefined, callArguments); if (range.facts & RangeFacts.IsGenerator) { From 56e1d6d43b222a7c5ef9c4e8721a5ef3472752ea Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Fri, 28 Jul 2017 15:18:19 -0700 Subject: [PATCH 43/55] Correctly detect JavaScriptness --- src/services/refactors/extractMethod.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 3922bee94c9f7..a301b77e20b29 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -599,7 +599,7 @@ namespace ts.refactor.extractMethod { else { functionNameText = getUniqueName(n => !(scope.locals && scope.locals.has(n))); } - const isJS = isInJavaScriptFile(node); + const isJS = isInJavaScriptFile(scope); const functionName = createIdentifier(functionNameText as string); const functionReference = createIdentifier(functionNameText as string); From 7f3a0a88cc22f4416b757ca2286ff881d19d95d5 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Mon, 31 Jul 2017 11:23:02 -0700 Subject: [PATCH 44/55] Create multi-line bodies --- src/services/refactors/extractMethod.ts | 4 ++-- .../reference/extractMethod/extractMethod8.js | 16 ++++++++++++---- tests/cases/fourslash/extract-method13.ts | 8 ++++++-- tests/cases/fourslash/extract-method2.ts | 4 +++- tests/cases/fourslash/extract-method5.ts | 4 +++- tests/cases/fourslash/extract-method7.ts | 6 ++++-- 6 files changed, 30 insertions(+), 12 deletions(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index a301b77e20b29..72f3e81bc4aed 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -763,10 +763,10 @@ namespace ts.refactor.extractMethod { // it is ok to know that range has at least one return since it we only allow unconditional returns rewrittenStatements.push(createReturn(createObjectLiteral(getPropertyAssignmentsForWrites(writes)))); } - return { body: createBlock(rewrittenStatements), returnValueProperty }; + return { body: createBlock(rewrittenStatements, /*multiLine*/ true), returnValueProperty }; } else { - return { body: createBlock(statements), returnValueProperty: undefined }; + return { body: createBlock(statements, /*multiLine*/ true), returnValueProperty: undefined }; } function visitor(node: Node): VisitResult { diff --git a/tests/baselines/reference/extractMethod/extractMethod8.js b/tests/baselines/reference/extractMethod/extractMethod8.js index 77dee04d97650..fe9cf2a92f0fd 100644 --- a/tests/baselines/reference/extractMethod/extractMethod8.js +++ b/tests/baselines/reference/extractMethod/extractMethod8.js @@ -16,7 +16,9 @@ namespace A { let a1 = 1; return newFunction() + 100; - function newFunction() { return 1 + a1 + x; } + function newFunction() { + return 1 + a1 + x; + } } } } @@ -29,7 +31,9 @@ namespace A { return newFunction(a1) + 100; } - function newFunction(a1: number) { return 1 + a1 + x; } + function newFunction(a1: number) { + return 1 + a1 + x; + } } } ==SCOPE::namespace A== @@ -42,7 +46,9 @@ namespace A { } } - function newFunction(a1: number) { return 1 + a1 + x; } + function newFunction(a1: number) { + return 1 + a1 + x; + } } ==SCOPE::file '/a.ts'== namespace A { @@ -54,4 +60,6 @@ namespace A { } } } -function newFunction(a1: number, x: number) { return 1 + a1 + x; } +function newFunction(a1: number, x: number) { + return 1 + a1 + x; +} diff --git a/tests/cases/fourslash/extract-method13.ts b/tests/cases/fourslash/extract-method13.ts index fea65c65aad0d..b9b7c0096aabc 100644 --- a/tests/cases/fourslash/extract-method13.ts +++ b/tests/cases/fourslash/extract-method13.ts @@ -20,7 +20,11 @@ verify.currentFileContentIs(`class C { constructor(q: string = C.newFunction()) { } - private static newFunction(): string { return "hello"; } + private static newFunction(): string { + return "hello"; + } - private static newFunction_1() { return 100; } + private static newFunction_1() { + return 100; + } }`); \ No newline at end of file diff --git a/tests/cases/fourslash/extract-method2.ts b/tests/cases/fourslash/extract-method2.ts index ff558714b9995..0a4f346307b5f 100644 --- a/tests/cases/fourslash/extract-method2.ts +++ b/tests/cases/fourslash/extract-method2.ts @@ -22,5 +22,7 @@ verify.currentFileContentIs( } } } -function newFunction(m: number, j: string, k: { x: string; }) { return m + j + k; } +function newFunction(m: number, j: string, k: { x: string; }) { + return m + j + k; +} `); diff --git a/tests/cases/fourslash/extract-method5.ts b/tests/cases/fourslash/extract-method5.ts index b60758bd3eefc..ac09f92cc0520 100644 --- a/tests/cases/fourslash/extract-method5.ts +++ b/tests/cases/fourslash/extract-method5.ts @@ -14,5 +14,7 @@ verify.currentFileContentIs( `function f() { var x: 1 | 2 | 3 = newFunction(); - function newFunction(): 1 | 2 | 3 { return 2; } + function newFunction(): 1 | 2 | 3 { + return 2; + } }`); \ No newline at end of file diff --git a/tests/cases/fourslash/extract-method7.ts b/tests/cases/fourslash/extract-method7.ts index 3b1c71f72c4ad..4c95c6a551d57 100644 --- a/tests/cases/fourslash/extract-method7.ts +++ b/tests/cases/fourslash/extract-method7.ts @@ -10,5 +10,7 @@ goTo.select('a', 'b'); edit.applyRefactor('Extract Method', 'scope_0'); verify.currentFileContentIs(`function fn(x = newFunction()) { } -function newFunction() { return 3; } -`, true); +function newFunction() { + return 3; +} +`); From 003cd0a489c87d8940ee4db0d7fd0adf0a4f0939 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Thu, 3 Aug 2017 15:25:44 -0700 Subject: [PATCH 45/55] Add support for validating specific refactor actions' availability --- src/harness/fourslash.ts | 9 +++++---- tests/cases/fourslash/fourslash.ts | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index c68dbb2178eff..737d2fed146ec 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2749,7 +2749,7 @@ namespace FourSlash { } } - public verifyRefactorAvailable(negative: boolean, name?: string) { + public verifyRefactorAvailable(negative: boolean, name?: string, subName?: string) { const selection = { pos: this.currentCaretPosition, end: this.selectionEnd === -1 ? this.currentCaretPosition : this.selectionEnd @@ -2757,9 +2757,10 @@ namespace FourSlash { let refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, selection) || []; if (name) { - refactors = refactors.filter(r => r.name === name); + refactors = refactors.filter(r => r.name === name && (subName === undefined || r.actions.some(a => a.name === subName))); } const isAvailable = refactors.length > 0; + if (negative && isAvailable) { this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected no refactor but found some.`); } @@ -3668,8 +3669,8 @@ namespace FourSlashInterface { this.state.verifyApplicableRefactorAvailableForRange(this.negative); } - public refactorAvailable(name?: string) { - this.state.verifyRefactorAvailable(this.negative, name); + public refactorAvailable(name?: string, subName?: string) { + this.state.verifyRefactorAvailable(this.negative, name, subName); } } diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index 9f3c919b00f2b..5175b52df6c9f 100644 --- a/tests/cases/fourslash/fourslash.ts +++ b/tests/cases/fourslash/fourslash.ts @@ -158,7 +158,7 @@ declare namespace FourSlashInterface { codeFixDiagnosticsAvailableAtMarkers(markerNames: string[], diagnosticCode?: number): void; applicableRefactorAvailableForRange(): void; - refactorAvailable(name?: string); + refactorAvailable(name?: string, subName?: string); } class verify extends verifyNegatable { assertHasRanges(ranges: Range[]): void; From a76664c9b259239a35539795a723a3ce3bafaafe Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Thu, 3 Aug 2017 15:25:54 -0700 Subject: [PATCH 46/55] More testcases --- tests/cases/fourslash/extract-method14.ts | 24 +++++++++++++++++++++++ tests/cases/fourslash/extract-method15.ts | 22 +++++++++++++++++++++ tests/cases/fourslash/extract-method17.ts | 10 ++++++++++ tests/cases/fourslash/extract-method18.ts | 21 ++++++++++++++++++++ 4 files changed, 77 insertions(+) create mode 100644 tests/cases/fourslash/extract-method14.ts create mode 100644 tests/cases/fourslash/extract-method15.ts create mode 100644 tests/cases/fourslash/extract-method17.ts create mode 100644 tests/cases/fourslash/extract-method18.ts diff --git a/tests/cases/fourslash/extract-method14.ts b/tests/cases/fourslash/extract-method14.ts new file mode 100644 index 0000000000000..370c43de60b21 --- /dev/null +++ b/tests/cases/fourslash/extract-method14.ts @@ -0,0 +1,24 @@ +/// + +// Don't emit type annotations in JavaScript files +// Also tests that single-variable return extractions don't get superfluous destructuring + +// @allowNonTsExtensions: true +// @Filename: foo.js +//// function foo() { +//// var i = 10; +//// /*a*/return i++;/*b*/ +//// } + +goTo.select('a', 'b'); +edit.applyRefactor('Extract Method', 'scope_1'); +verify.currentFileContentIs(`function foo() { + var i = 10; + var __return: any; + ({ i, __return } = newFunction(i)); + return __return; +} +function newFunction(i) { + return { i, __return: i++ }; +} +`); \ No newline at end of file diff --git a/tests/cases/fourslash/extract-method15.ts b/tests/cases/fourslash/extract-method15.ts new file mode 100644 index 0000000000000..ef62bd3fd3f74 --- /dev/null +++ b/tests/cases/fourslash/extract-method15.ts @@ -0,0 +1,22 @@ +/// + +// Extracting an increment expression (not statement) should do the right thing, +// including not generating extra destructuring unless needed + +//// function foo() { +//// var i = 10; +//// /*a*/i++/*b*/; +//// } + +goTo.select('a', 'b'); +edit.applyRefactor('Extract Method', 'scope_1'); + +verify.currentFileContentIs(`function foo() { + var i = 10; + i = newFunction(i); +} +function newFunction(i: number) { + i++; + return i; +} +`); diff --git a/tests/cases/fourslash/extract-method17.ts b/tests/cases/fourslash/extract-method17.ts new file mode 100644 index 0000000000000..ce54896604dd9 --- /dev/null +++ b/tests/cases/fourslash/extract-method17.ts @@ -0,0 +1,10 @@ +/// + +//// function foo () { +//// var x = 3; +//// var y = /*start*/x++ + 5/*end*/; +//// } + +goTo.select('start', 'end') +verify.refactorAvailable('Extract Method', 'scope_0'); +verify.not.refactorAvailable('Extract Method', 'scope_1'); diff --git a/tests/cases/fourslash/extract-method18.ts b/tests/cases/fourslash/extract-method18.ts new file mode 100644 index 0000000000000..9d87979a1ed1c --- /dev/null +++ b/tests/cases/fourslash/extract-method18.ts @@ -0,0 +1,21 @@ +/// + +// Don't try to propagate property accessed variables back, +// or emit spurious returns when the value is clearly ignored + +//// function fn() { +//// const x = { m: 1 }; +//// /*a*/x.m = 3/*b*/; +//// } + +goTo.select('a', 'b') +verify.refactorAvailable('Extract Method'); +edit.applyRefactor('Extract Method', "scope_1"); +verify.currentFileContentIs(`function fn() { + const x = { m: 1 }; + newFunction(x); +} +function newFunction(x: { m: number; }) { + x.m = 3; +} +`); From 42e06528d2926eae60b3b81e8a1a94b37e702f14 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Thu, 3 Aug 2017 15:26:06 -0700 Subject: [PATCH 47/55] Accept baselines --- .../reference/extractMethod/extractMethod1.js | 12 ++++++------ .../reference/extractMethod/extractMethod5.js | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/baselines/reference/extractMethod/extractMethod1.js b/tests/baselines/reference/extractMethod/extractMethod1.js index fea213ff4d3dc..60c7e301616e8 100644 --- a/tests/baselines/reference/extractMethod/extractMethod1.js +++ b/tests/baselines/reference/extractMethod/extractMethod1.js @@ -43,7 +43,7 @@ namespace A { function a() { let a = 1; - ({ a } = newFunction(a)); + a = newFunction(a); } function newFunction(a: number) { @@ -51,7 +51,7 @@ namespace A { let z = x; a = y; foo(); - return { a }; + return a; } } } @@ -64,7 +64,7 @@ namespace A { function a() { let a = 1; - ({ a } = newFunction(a)); + a = newFunction(a); } } @@ -73,7 +73,7 @@ namespace A { let z = x; a = y; foo(); - return { a }; + return a; } } ==SCOPE::file '/a.ts'== @@ -85,7 +85,7 @@ namespace A { function a() { let a = 1; - ({ a } = newFunction(x, a, foo)); + a = newFunction(x, a, foo); } } } @@ -94,5 +94,5 @@ function newFunction(x: number, a: number, foo: () => void) { let z = x; a = y; foo(); - return { a }; + return a; } diff --git a/tests/baselines/reference/extractMethod/extractMethod5.js b/tests/baselines/reference/extractMethod/extractMethod5.js index 33c5a6e71d83f..cf63cb2a93214 100644 --- a/tests/baselines/reference/extractMethod/extractMethod5.js +++ b/tests/baselines/reference/extractMethod/extractMethod5.js @@ -43,7 +43,7 @@ namespace A { function a() { let a = 1; - ({ a } = newFunction(a)); + a = newFunction(a); } function newFunction(a: number) { @@ -51,7 +51,7 @@ namespace A { let z = x; a = y; foo(); - return { a }; + return a; } } } @@ -64,7 +64,7 @@ namespace A { function a() { let a = 1; - ({ a } = newFunction(a)); + a = newFunction(a); } } @@ -73,7 +73,7 @@ namespace A { let z = x; a = y; foo(); - return { a }; + return a; } } ==SCOPE::file '/a.ts'== @@ -85,7 +85,7 @@ namespace A { function a() { let a = 1; - ({ a } = newFunction(x, a)); + a = newFunction(x, a); } } } @@ -94,5 +94,5 @@ function newFunction(x: number, a: number) { let z = x; a = y; A.foo(); - return { a }; + return a; } From 46a84c31f2e73e0965f6e7ca865afee042ed709c Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Thu, 3 Aug 2017 15:26:23 -0700 Subject: [PATCH 48/55] Innumerable bugfixes --- src/services/refactors/extractMethod.ts | 78 +++++++++++++++++++++---- 1 file changed, 67 insertions(+), 11 deletions(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 72f3e81bc4aed..4b922f9c9aaa1 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -100,6 +100,7 @@ namespace ts.refactor.extractMethod { export const FunctionWillNotBeVisibleInTheNewScope = createMessage("Function will not visible in the new scope."); export const InsufficientSelection = 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 enum RangeFacts { @@ -226,13 +227,22 @@ namespace ts.refactor.extractMethod { return { targetRange: { range: statements, facts: rangeFacts, declarations } }; } else { + // We have a single expression (start) const errors = checkRootNode(start) || checkNode(start); if (errors) { return { errors }; } + + // If our selection is the expression in an ExrpessionStatement, expand + // the selection to include the enclosing Statement (this stops us + // from trying to care about the return value of the extracted function + // and eliminates double semicolon insertion in certain scenarios) const range = isStatement(start) ? [start] - : start; + : start.parent && start.parent.kind === SyntaxKind.ExpressionStatement + ? [start.parent as Statement] + : start; + return { targetRange: { range, facts: rangeFacts, declarations } }; } @@ -581,7 +591,7 @@ namespace ts.refactor.extractMethod { } export function extractFunctionInScope( - node: Node, + node: Statement | Expression | Block, scope: Scope, { usages: usagesInScope, substitutions }: ScopeUsages, range: TargetRange, @@ -597,7 +607,8 @@ namespace ts.refactor.extractMethod { functionNameText = getUniqueName(n => props.every(p => p.name !== n)); } else { - functionNameText = getUniqueName(n => !(scope.locals && scope.locals.has(n))); + const file = scope.getSourceFile(); + functionNameText = getUniqueName(n => !file.identifiers.has(n as string)); } const isJS = isInJavaScriptFile(scope); @@ -706,10 +717,24 @@ namespace ts.refactor.extractMethod { if (returnValueProperty) { assignments.push(createShorthandPropertyAssignment(returnValueProperty)); } + // propagate writes back - newNodes.push(createStatement(createBinary(createObjectLiteral(assignments), SyntaxKind.EqualsToken, call))); - if (returnValueProperty) { - newNodes.push(createReturn(createIdentifier(returnValueProperty))); + if (assignments.length === 1) { + if (returnValueProperty) { + newNodes.push(createReturn(createIdentifier(returnValueProperty))); + } + else { + newNodes.push(createStatement(createBinary(assignments[0].name, SyntaxKind.EqualsToken, call))); + } + } + else { + // emit e.g. + // { a, b, __return } = newFunction(a, b); + // return __return; + newNodes.push(createStatement(createBinary(createObjectLiteral(assignments), SyntaxKind.EqualsToken, call))); + if (returnValueProperty) { + newNodes.push(createReturn(createIdentifier(returnValueProperty))); + } } } else { @@ -751,17 +776,23 @@ namespace ts.refactor.extractMethod { function transformFunctionBody(body: Node) { if (isBlock(body) && !writes && substitutions.size === 0) { // already block, no writes to propagate back, no substitutions - can use node as is - return { body, returnValueProperty: undefined }; + return { body: createBlock(body.statements, /*multLine*/ true), returnValueProperty: undefined }; } let returnValueProperty: string; const statements = createNodeArray(isBlock(body) ? body.statements.slice(0) : [isStatement(body) ? body : createReturn(body)]); // rewrite body if either there are writes that should be propagated back via return statements or there are substitutions if (writes || substitutions.size) { const rewrittenStatements = visitNodes(statements, visitor).slice(); - if (writes && !(range.facts & RangeFacts.HasReturn)) { + if (writes && !(range.facts & RangeFacts.HasReturn) && isStatement(body)) { // add return at the end to propagate writes back in case if control flow falls out of the function body // it is ok to know that range has at least one return since it we only allow unconditional returns - rewrittenStatements.push(createReturn(createObjectLiteral(getPropertyAssignmentsForWrites(writes)))); + const assignments = getPropertyAssignmentsForWrites(writes); + if (assignments.length === 1) { + rewrittenStatements.push(createReturn(assignments[0].name)); + } + else { + rewrittenStatements.push(createReturn(createObjectLiteral(assignments))); + } } return { body: createBlock(rewrittenStatements, /*multiLine*/ true), returnValueProperty }; } @@ -778,7 +809,12 @@ namespace ts.refactor.extractMethod { } assignments.push(createPropertyAssignment(returnValueProperty, visitNode((node).expression, visitor))); } - return createReturn(createObjectLiteral(assignments)); + if (assignments.length === 1) { + return createReturn(assignments[0].name as Expression); + } + else { + return createReturn(createObjectLiteral(assignments)); + } } else { const substitution = substitutions.get(getNodeId(node).toString()); @@ -852,7 +888,20 @@ namespace ts.refactor.extractMethod { const target = isReadonlyArray(targetRange.range) ? createBlock(targetRange.range) : targetRange.range; const containingLexicalScopeOfExtraction = isBlockScope(scopes[0], scopes[0].parent) ? scopes[0] : getEnclosingBlockScopeContainer(scopes[0]); - forEachChild(target, collectUsages); + collectUsages(target); + + for (let i = 0; i < scopes.length; i++) { + let hasWrite = false; + usagesPerScope[i].usages.forEach(value => { + if (value.usage === Usage.Write) { + hasWrite = true; + return false; + } + }); + if (hasWrite && !isArray(targetRange.range) && isExpression(targetRange.range)) { + errorsPerScope[i].push(createDiagnosticForNode(targetRange.range, Messages.CannotCombineWritesAndReturns)); + } + } // If there are any declarations in the extracted block that are used in the same enclosing // lexical scope, we can't move the extraction "up" as those declarations will become unreachable @@ -882,6 +931,13 @@ namespace ts.refactor.extractMethod { collectUsages(node.operand); valueUsage = savedValueUsage; } + else if (isPropertyAccessExpression(node) || isElementAccessExpression(node)) { + const savedValueUsage = valueUsage; + // use 'write' as default usage for values + valueUsage = Usage.Read; + forEachChild(node, collectUsages); + valueUsage = savedValueUsage; + } else if (isIdentifier(node)) { if (!node.parent) { return; From 37ae95792c83ae6e7447decaca2904208f67a509 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Thu, 3 Aug 2017 15:57:54 -0700 Subject: [PATCH 49/55] Add additional test --- tests/cases/fourslash/extract-method19.ts | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 tests/cases/fourslash/extract-method19.ts diff --git a/tests/cases/fourslash/extract-method19.ts b/tests/cases/fourslash/extract-method19.ts new file mode 100644 index 0000000000000..54f79311cc72b --- /dev/null +++ b/tests/cases/fourslash/extract-method19.ts @@ -0,0 +1,22 @@ +/// + +// New function names should be totally new to the file + +//// function fn() { +//// /*a*/console.log("hi");/*b*/ +//// } +//// +//// function newFunction() { } + +goTo.select('a', 'b') +verify.refactorAvailable('Extract Method'); +edit.applyRefactor('Extract Method', "scope_0"); +verify.currentFileContentIs(`function fn() { + newFunction_1(); + + function newFunction_1() { + console.log("hi"); + } +} + +function newFunction() { }`); From 2adb2b3f84ba6974b0a44f3e7b57554399ada050 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Fri, 4 Aug 2017 11:21:49 -0700 Subject: [PATCH 50/55] Fix some class scenarios (static methods, readonly inits in ctor) --- src/services/refactors/extractMethod.ts | 21 ++++++++++++++++--- tests/cases/fourslash/extract-method20.ts | 14 +++++++++++++ tests/cases/fourslash/extract-method21.ts | 25 +++++++++++++++++++++++ 3 files changed, 57 insertions(+), 3 deletions(-) create mode 100644 tests/cases/fourslash/extract-method20.ts create mode 100644 tests/cases/fourslash/extract-method21.ts diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 4b922f9c9aaa1..7580137c32dd9 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -101,6 +101,7 @@ namespace ts.refactor.extractMethod { export const InsufficientSelection = 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 enum RangeFacts { @@ -276,7 +277,7 @@ namespace ts.refactor.extractMethod { declarations.push(nodeToCheck.symbol); } - // If we're in a class, see if we're in a static region (static property initializer; class constructor parameter default) or not + // If we're in a class, see if we're in a static region (static property initializer, static method, class constructor parameter default) or not const stoppingPoint: Node = getContainingClass(nodeToCheck); if (stoppingPoint) { let current: Node = nodeToCheck; @@ -294,6 +295,11 @@ namespace ts.refactor.extractMethod { } break; } + else if (current.kind === SyntaxKind.MethodDeclaration) { + if (hasModifier(current, ModifierFlags.Static)) { + rangeFacts |= RangeFacts.InStaticRegion; + } + } current = current.parent; } } @@ -447,7 +453,7 @@ namespace ts.refactor.extractMethod { function isValidExtractionTarget(node: Node) { // 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); + return (node.kind === SyntaxKind.FunctionDeclaration) || (node.kind === SyntaxKind.Constructor) || isSourceFile(node) || isModuleBlock(node) || isClassLike(node); } /** @@ -892,15 +898,24 @@ namespace ts.refactor.extractMethod { for (let i = 0; i < scopes.length; i++) { let hasWrite = false; + let readonlyClassPropertyWrite: Declaration | undefined = undefined; usagesPerScope[i].usages.forEach(value => { if (value.usage === Usage.Write) { hasWrite = true; - return false; + if (value.symbol.flags & SymbolFlags.ClassMember && + value.symbol.valueDeclaration && + hasModifier(value.symbol.valueDeclaration, ModifierFlags.Readonly)) { + readonlyClassPropertyWrite = value.symbol.valueDeclaration; + } } }); + if (hasWrite && !isArray(targetRange.range) && isExpression(targetRange.range)) { errorsPerScope[i].push(createDiagnosticForNode(targetRange.range, Messages.CannotCombineWritesAndReturns)); } + else if (readonlyClassPropertyWrite && i > 0) { + errorsPerScope[i].push(createDiagnosticForNode(readonlyClassPropertyWrite, Messages.CannotCombineWritesAndReturns)); + } } // If there are any declarations in the extracted block that are used in the same enclosing diff --git a/tests/cases/fourslash/extract-method20.ts b/tests/cases/fourslash/extract-method20.ts new file mode 100644 index 0000000000000..04990001b5f0e --- /dev/null +++ b/tests/cases/fourslash/extract-method20.ts @@ -0,0 +1,14 @@ +/// + +// Shouldn't be able to extract a readonly property initializer outside the constructor + +//// class Foo { +//// readonly prop; +//// constructor() { +//// /*a*/this.prop = 10;/*b*/ +//// } +//// } + +goTo.select('a', 'b') +verify.refactorAvailable('Extract Method', 'scope_0'); +verify.not.refactorAvailable('Extract Method', 'scope_1'); diff --git a/tests/cases/fourslash/extract-method21.ts b/tests/cases/fourslash/extract-method21.ts new file mode 100644 index 0000000000000..0168daf5fcb02 --- /dev/null +++ b/tests/cases/fourslash/extract-method21.ts @@ -0,0 +1,25 @@ +/// + +// Extracting from a static method should create a static method + +//// class Foo { +//// static method() { +//// /*start*/return 1;/*end*/ +//// } +//// } + +goTo.select('start', 'end') + +verify.refactorAvailable('Extract Method'); + +edit.applyRefactor('Extract Method', "scope_0"); + +verify.currentFileContentIs(`class Foo { + static method() { + return Foo.newFunction(); + } + + private static newFunction() { + return 1; + } +}`); \ No newline at end of file From ba37830723dd3ed80c9d9e8090769a083500fa43 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Fri, 4 Aug 2017 11:46:56 -0700 Subject: [PATCH 51/55] Prevent extraction of exported variables --- src/services/refactors/extractMethod.ts | 17 +++++++++-------- tests/cases/fourslash/extract-method22.ts | 10 ++++++++++ 2 files changed, 19 insertions(+), 8 deletions(-) create mode 100644 tests/cases/fourslash/extract-method22.ts diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 7580137c32dd9..dc331ebf4d164 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -269,14 +269,6 @@ namespace ts.refactor.extractMethod { if (!isStatement(nodeToCheck) && !(isExpression(nodeToCheck) && isLegalExpressionExtraction(nodeToCheck))) { return [createDiagnosticForNode(nodeToCheck, Messages.StatementOrExpressionExpected)]; } - - if (isDeclaration(nodeToCheck)) { - if (hasModifier(nodeToCheck, ModifierFlags.Export)) { - return [createDiagnosticForNode(nodeToCheck, Messages.CannotExtractExportedEntity)]; - } - declarations.push(nodeToCheck.symbol); - } - // If we're in a class, see if we're in a static region (static property initializer, static method, class constructor parameter default) or not const stoppingPoint: Node = getContainingClass(nodeToCheck); if (stoppingPoint) { @@ -318,6 +310,15 @@ namespace ts.refactor.extractMethod { return true; } + if (isDeclaration(node)) { + const declaringNode = (node.kind === SyntaxKind.VariableDeclaration) ? node.parent.parent : node; + if (hasModifier(declaringNode, ModifierFlags.Export)) { + (errors || (errors = []).push(createDiagnosticForNode(node, Messages.CannotExtractExportedEntity))); + return true; + } + declarations.push(node.symbol); + } + // Some things can't be extracted in certain situations switch (node.kind) { case SyntaxKind.ImportDeclaration: diff --git a/tests/cases/fourslash/extract-method22.ts b/tests/cases/fourslash/extract-method22.ts new file mode 100644 index 0000000000000..7486520e700f7 --- /dev/null +++ b/tests/cases/fourslash/extract-method22.ts @@ -0,0 +1,10 @@ +/// + +// You may not extract variable declarations with the export modifier + +//// namespace NS { +//// /*start*/export var x = 10;/*end*/ +//// } + +goTo.select('start', 'end') +verify.not.refactorAvailable('Extract Method'); From 4dafb40584012f8462c8169d7dd5b2c391b84157 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Fri, 4 Aug 2017 11:55:24 -0700 Subject: [PATCH 52/55] No extraction of ambient declarations --- src/services/refactors/extractMethod.ts | 6 ++++++ tests/cases/fourslash/extract-method23.ts | 8 ++++++++ 2 files changed, 14 insertions(+) create mode 100644 tests/cases/fourslash/extract-method23.ts diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index dc331ebf4d164..8bbdadeafbcc3 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -102,6 +102,7 @@ namespace ts.refactor.extractMethod { 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 enum RangeFacts { @@ -269,6 +270,11 @@ namespace ts.refactor.extractMethod { if (!isStatement(nodeToCheck) && !(isExpression(nodeToCheck) && isLegalExpressionExtraction(nodeToCheck))) { return [createDiagnosticForNode(nodeToCheck, Messages.StatementOrExpressionExpected)]; } + + if (isInAmbientContext(nodeToCheck)) { + return [createDiagnosticForNode(nodeToCheck, Messages.CannotExtractAmbientBlock)]; + } + // If we're in a class, see if we're in a static region (static property initializer, static method, class constructor parameter default) or not const stoppingPoint: Node = getContainingClass(nodeToCheck); if (stoppingPoint) { diff --git a/tests/cases/fourslash/extract-method23.ts b/tests/cases/fourslash/extract-method23.ts new file mode 100644 index 0000000000000..7da8f175f1353 --- /dev/null +++ b/tests/cases/fourslash/extract-method23.ts @@ -0,0 +1,8 @@ +/// + +//// declare namespace Foo { +//// const x = /*start*/3/*end*/; +//// } + +goTo.select('start', 'end') +verify.not.refactorAvailable('Extract Method'); From c1c7e83bc4be83b1b4f004f9a2142408746a4658 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Fri, 4 Aug 2017 12:40:01 -0700 Subject: [PATCH 53/55] Put __return first --- src/services/refactors/extractMethod.ts | 4 ++-- .../reference/extractMethod/extractMethod11.js | 12 ++++++------ .../reference/extractMethod/extractMethod12.js | 4 ++-- .../reference/extractMethod/extractMethod6.js | 12 ++++++------ .../reference/extractMethod/extractMethod7.js | 12 ++++++------ tests/cases/fourslash/extract-method14.ts | 4 ++-- 6 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 8bbdadeafbcc3..86faaf9a022d6 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -728,7 +728,7 @@ namespace ts.refactor.extractMethod { const assignments = getPropertyAssignmentsForWrites(writes); if (returnValueProperty) { - assignments.push(createShorthandPropertyAssignment(returnValueProperty)); + assignments.unshift(createShorthandPropertyAssignment(returnValueProperty)); } // propagate writes back @@ -820,7 +820,7 @@ namespace ts.refactor.extractMethod { if (!returnValueProperty) { returnValueProperty = generateReturnValueProperty(); } - assignments.push(createPropertyAssignment(returnValueProperty, visitNode((node).expression, visitor))); + assignments.unshift(createPropertyAssignment(returnValueProperty, visitNode((node).expression, visitor))); } if (assignments.length === 1) { return createReturn(assignments[0].name as Expression); diff --git a/tests/baselines/reference/extractMethod/extractMethod11.js b/tests/baselines/reference/extractMethod/extractMethod11.js index ee9785216826a..77565e7b0a778 100644 --- a/tests/baselines/reference/extractMethod/extractMethod11.js +++ b/tests/baselines/reference/extractMethod/extractMethod11.js @@ -18,7 +18,7 @@ namespace A { a() { let z = 1; var __return: any; - ({ z, __return } = this.newFunction(z)); + ({ __return, z } = this.newFunction(z)); return __return; } @@ -26,7 +26,7 @@ namespace A { let a1 = { x: 1 }; y = 10; z = 42; - return { z, __return: a1.x + 10 }; + return { __return: a1.x + 10, z }; } } } @@ -37,7 +37,7 @@ namespace A { a() { let z = 1; var __return: any; - ({ z, __return } = newFunction(z)); + ({ __return, z } = newFunction(z)); return __return; } } @@ -46,7 +46,7 @@ namespace A { let a1 = { x: 1 }; y = 10; z = 42; - return { z, __return: a1.x + 10 }; + return { __return: a1.x + 10, z }; } } ==SCOPE::file '/a.ts'== @@ -56,7 +56,7 @@ namespace A { a() { let z = 1; var __return: any; - ({ y, z, __return } = newFunction(y, z)); + ({ __return, y, z } = newFunction(y, z)); return __return; } } @@ -65,5 +65,5 @@ function newFunction(y: number, z: number) { let a1 = { x: 1 }; y = 10; z = 42; - return { y, z, __return: a1.x + 10 }; + return { __return: a1.x + 10, y, z }; } diff --git a/tests/baselines/reference/extractMethod/extractMethod12.js b/tests/baselines/reference/extractMethod/extractMethod12.js index 056fdf941d20a..8ff6e130dad2a 100644 --- a/tests/baselines/reference/extractMethod/extractMethod12.js +++ b/tests/baselines/reference/extractMethod/extractMethod12.js @@ -21,7 +21,7 @@ namespace A { a() { let z = 1; var __return: any; - ({ z, __return } = this.newFunction(z)); + ({ __return, z } = this.newFunction(z)); return __return; } @@ -30,7 +30,7 @@ namespace A { y = 10; z = 42; this.b(); - return { z, __return: a1.x + 10 }; + return { __return: a1.x + 10, z }; } } } \ No newline at end of file diff --git a/tests/baselines/reference/extractMethod/extractMethod6.js b/tests/baselines/reference/extractMethod/extractMethod6.js index a1bea905048a3..99f52e9febbe0 100644 --- a/tests/baselines/reference/extractMethod/extractMethod6.js +++ b/tests/baselines/reference/extractMethod/extractMethod6.js @@ -44,7 +44,7 @@ namespace A { let a = 1; var __return: any; - ({ a, __return } = newFunction(a)); + ({ __return, a } = newFunction(a)); return __return; } @@ -52,7 +52,7 @@ namespace A { let y = 5; let z = x; a = y; - return { a, __return: foo() }; + return { __return: foo(), a }; } } } @@ -66,7 +66,7 @@ namespace A { let a = 1; var __return: any; - ({ a, __return } = newFunction(a)); + ({ __return, a } = newFunction(a)); return __return; } } @@ -75,7 +75,7 @@ namespace A { let y = 5; let z = x; a = y; - return { a, __return: foo() }; + return { __return: foo(), a }; } } ==SCOPE::file '/a.ts'== @@ -88,7 +88,7 @@ namespace A { let a = 1; var __return: any; - ({ a, __return } = newFunction(x, a)); + ({ __return, a } = newFunction(x, a)); return __return; } } @@ -97,5 +97,5 @@ function newFunction(x: number, a: number) { let y = 5; let z = x; a = y; - return { a, __return: A.foo() }; + return { __return: A.foo(), a }; } diff --git a/tests/baselines/reference/extractMethod/extractMethod7.js b/tests/baselines/reference/extractMethod/extractMethod7.js index fe32016b33a0f..09d5edaa2c05d 100644 --- a/tests/baselines/reference/extractMethod/extractMethod7.js +++ b/tests/baselines/reference/extractMethod/extractMethod7.js @@ -50,7 +50,7 @@ namespace A { let a = 1; var __return: any; - ({ a, __return } = newFunction(a)); + ({ __return, a } = newFunction(a)); return __return; } @@ -58,7 +58,7 @@ namespace A { let y = 5; let z = x; a = y; - return { a, __return: C.foo() }; + return { __return: C.foo(), a }; } } } @@ -74,7 +74,7 @@ namespace A { let a = 1; var __return: any; - ({ a, __return } = newFunction(a)); + ({ __return, a } = newFunction(a)); return __return; } } @@ -83,7 +83,7 @@ namespace A { let y = 5; let z = x; a = y; - return { a, __return: C.foo() }; + return { __return: C.foo(), a }; } } ==SCOPE::file '/a.ts'== @@ -98,7 +98,7 @@ namespace A { let a = 1; var __return: any; - ({ a, __return } = newFunction(x, a)); + ({ __return, a } = newFunction(x, a)); return __return; } } @@ -107,5 +107,5 @@ function newFunction(x: number, a: number) { let y = 5; let z = x; a = y; - return { a, __return: A.C.foo() }; + return { __return: A.C.foo(), a }; } diff --git a/tests/cases/fourslash/extract-method14.ts b/tests/cases/fourslash/extract-method14.ts index 370c43de60b21..c8bab1b3a56d7 100644 --- a/tests/cases/fourslash/extract-method14.ts +++ b/tests/cases/fourslash/extract-method14.ts @@ -15,10 +15,10 @@ edit.applyRefactor('Extract Method', 'scope_1'); verify.currentFileContentIs(`function foo() { var i = 10; var __return: any; - ({ i, __return } = newFunction(i)); + ({ __return, i } = newFunction(i)); return __return; } function newFunction(i) { - return { i, __return: i++ }; + return { __return: i++, i }; } `); \ No newline at end of file From a4c41b8f2b7e8eca2e0380f5f41e4cafcb2a0398 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Fri, 4 Aug 2017 13:47:57 -0700 Subject: [PATCH 54/55] Don't wrongly exclude accessor in indexed access exprs --- src/services/refactors/extractMethod.ts | 2 +- tests/cases/fourslash/extract-method24.ts | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 tests/cases/fourslash/extract-method24.ts diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 86faaf9a022d6..e74f66701eeba 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -967,7 +967,7 @@ namespace ts.refactor.extractMethod { if (isQualifiedName(node.parent) && node !== node.parent.left) { return; } - if ((isPropertyAccessExpression(node.parent) || isElementAccessExpression(node.parent)) && node !== node.parent.expression) { + if (isPropertyAccessExpression(node.parent) && node !== node.parent.expression) { return; } recordUsage(node, valueUsage, /*isTypeNode*/ isPartOfTypeNode(node)); diff --git a/tests/cases/fourslash/extract-method24.ts b/tests/cases/fourslash/extract-method24.ts new file mode 100644 index 0000000000000..9eebc00316bd6 --- /dev/null +++ b/tests/cases/fourslash/extract-method24.ts @@ -0,0 +1,19 @@ +/// + +//// function M() { +//// let a = [1,2,3]; +//// let x = 0; +//// console.log(/*a*/a[x]/*b*/); +//// } + +goTo.select('a', 'b') +edit.applyRefactor('Extract Method', 'scope_1'); +verify.currentFileContentIs(`function M() { + let a = [1,2,3]; + let x = 0; + console.log(newFunction(a, x)); +} +function newFunction(a: number[], x: number): any { + return a[x]; +} +`); \ No newline at end of file From b5a88c1f174dce854cb1780b36ce16b067407d75 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Fri, 4 Aug 2017 15:29:13 -0700 Subject: [PATCH 55/55] Share selection logic; fix typos --- src/harness/fourslash.ts | 14 +++++++++----- src/services/refactors/extractMethod.ts | 2 +- src/services/textChanges.ts | 18 +++++++----------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 737d2fed146ec..7f8ded3e2599b 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2749,11 +2749,15 @@ namespace FourSlash { } } - public verifyRefactorAvailable(negative: boolean, name?: string, subName?: string) { - const selection = { + private getSelection() { + return ({ pos: this.currentCaretPosition, end: this.selectionEnd === -1 ? this.currentCaretPosition : this.selectionEnd - }; + }); + } + + public verifyRefactorAvailable(negative: boolean, name?: string, subName?: string) { + const selection = this.getSelection(); let refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, selection) || []; if (name) { @@ -2764,7 +2768,7 @@ namespace FourSlash { if (negative && isAvailable) { this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected no refactor but found some.`); } - if (!negative && !isAvailable) { + else if (!negative && !isAvailable) { this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected a refactor but found none.`); } } @@ -2786,7 +2790,7 @@ namespace FourSlash { } public applyRefactor(refactorName: string, actionName: string) { - const range = { pos: this.currentCaretPosition, end: this.selectionEnd === -1 ? this.currentCaretPosition : this.selectionEnd }; + const range = this.getSelection(); const refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, range); const refactor = ts.find(refactors, r => r.name === refactorName); if (!refactor) { diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index e74f66701eeba..65e29138e9597 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -700,7 +700,7 @@ namespace ts.refactor.extractMethod { ); } - const changeTracker = textChanges.ChangeTracker.fromRefactorContext(context); + const changeTracker = textChanges.ChangeTracker.fromCodeFixContext(context); // insert function at the end of the scope changeTracker.insertNodeBefore(context.file, scope.getLastToken(), newFunction, { prefix: context.newLineCharacter, suffix: context.newLineCharacter }); diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 777e97f7c23df..5480c8f34a5fe 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -188,11 +188,7 @@ namespace ts.textChanges { private changes: Change[] = []; private readonly newLineCharacter: string; - public static fromCodeFixContext(context: { newLineCharacter: string, rulesProvider: formatting.RulesProvider }) { - return new ChangeTracker(getNewlineKind(context), context.rulesProvider); - } - - public static fromRefactorContext(context: RefactorContext) { + public static fromCodeFixContext(context: { newLineCharacter: string, rulesProvider?: formatting.RulesProvider }) { return new ChangeTracker(getNewlineKind(context), context.rulesProvider); } @@ -285,7 +281,7 @@ namespace ts.textChanges { return this; } - private replaceWithMutiple(sourceFile: SourceFile, startPosition: number, endPosition: number, newNodes: ReadonlyArray, options: ChangeMultipleNodesOptions): this { + private replaceWithMultiple(sourceFile: SourceFile, startPosition: number, endPosition: number, newNodes: ReadonlyArray, options: ChangeMultipleNodesOptions): this { this.changes.push({ kind: ChangeKind.ReplaceWithMultipleNodes, sourceFile, @@ -299,23 +295,23 @@ namespace ts.textChanges { public replaceNodeWithNodes(sourceFile: SourceFile, oldNode: Node, newNodes: ReadonlyArray, options: ChangeMultipleNodesOptions) { const startPosition = getAdjustedStartPosition(sourceFile, oldNode, options, Position.Start); const endPosition = getAdjustedEndPosition(sourceFile, oldNode, options); - return this.replaceWithMutiple(sourceFile, startPosition, endPosition, newNodes, options); + return this.replaceWithMultiple(sourceFile, startPosition, endPosition, newNodes, options); } public replaceNodesWithNodes(sourceFile: SourceFile, oldNodes: ReadonlyArray, newNodes: ReadonlyArray, options: ChangeMultipleNodesOptions) { const startPosition = getAdjustedStartPosition(sourceFile, oldNodes[0], options, Position.Start); const endPosition = getAdjustedEndPosition(sourceFile, lastOrUndefined(oldNodes), options); - return this.replaceWithMutiple(sourceFile, startPosition, endPosition, newNodes, options); + return this.replaceWithMultiple(sourceFile, startPosition, endPosition, newNodes, options); } public replaceRangeWithNodes(sourceFile: SourceFile, range: TextRange, newNodes: ReadonlyArray, options: ChangeMultipleNodesOptions) { - return this.replaceWithMutiple(sourceFile, range.pos, range.end, newNodes, options); + return this.replaceWithMultiple(sourceFile, range.pos, range.end, newNodes, options); } public replaceNodeRangeWithNodes(sourceFile: SourceFile, startNode: Node, endNode: Node, newNodes: ReadonlyArray, options: ChangeMultipleNodesOptions) { const startPosition = getAdjustedStartPosition(sourceFile, startNode, options, Position.Start); const endPosition = getAdjustedEndPosition(sourceFile, endNode, options); - return this.replaceWithMutiple(sourceFile, startPosition, endPosition, newNodes, options); + return this.replaceWithMultiple(sourceFile, startPosition, endPosition, newNodes, options); } public insertNodeAt(sourceFile: SourceFile, pos: number, newNode: Node, options: InsertNodeOptions = {}) { @@ -539,10 +535,10 @@ namespace ts.textChanges { text = parts.join(change.options.nodeSeparator); } else { + Debug.assert(change.kind === ChangeKind.ReplaceWithSingleNode, "change.kind === ReplaceWithSingleNode"); text = this.getFormattedTextOfNode(change.node, sourceFile, pos, options); } // strip initial indentation (spaces or tabs) if text will be inserted in the middle of the line - // however keep indentation if it is was forced text = (posStartsLine || options.indentation !== undefined) ? text : text.replace(/^\s+/, ""); return (options.prefix || "") + text + (options.suffix || ""); }