diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 6478ad24d5ba5..62ba653961f06 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -70,7 +70,7 @@ namespace ts.refactor.extractSymbol { let i = 0; for (const { functionExtraction, constantExtraction } of extractions) { const description = functionExtraction.description; - if(refactorKindBeginsWith(extractFunctionAction.kind, requestedRefactor)){ + if (refactorKindBeginsWith(extractFunctionAction.kind, requestedRefactor)) { if (functionExtraction.errors.length === 0) { // Don't issue refactorings with duplicated names. // Scopes come back in "innermost first" order, so extractions will @@ -94,8 +94,7 @@ namespace ts.refactor.extractSymbol { } } - // Skip these since we don't have a way to report errors yet - if(refactorKindBeginsWith(extractConstantAction.kind, requestedRefactor)) { + if (refactorKindBeginsWith(extractConstantAction.kind, requestedRefactor)) { if (constantExtraction.errors.length === 0) { // Don't issue refactorings with duplicated names. // Scopes come back in "innermost first" order, so extractions will @@ -265,24 +264,30 @@ namespace ts.refactor.extractSymbol { /** * 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) + * process may fail, in which case a set of errors is returned instead. These errors are shown to + * users if they have the provideRefactorNotApplicableReason option set. */ // exported only for tests - export function getRangeToExtract(sourceFile: SourceFile, span: TextSpan, considerEmptySpans = true): RangeToExtract { + export function getRangeToExtract(sourceFile: SourceFile, span: TextSpan, invoked = true): RangeToExtract { const { length } = span; - if (length === 0 && !considerEmptySpans) { + if (length === 0 && !invoked) { return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.cannotExtractEmpty)] }; } - const cursorRequest = length === 0 && considerEmptySpans; + const cursorRequest = length === 0 && invoked; + + const startToken = getTokenAtPosition(sourceFile, span.start); + const endToken = findTokenOnLeftOfPosition(sourceFile, textSpanEnd(span)); + /* If the refactoring command is invoked through a keyboard action it's safe to assume that the user is actively looking for + refactoring actions at the span location. As they may not know the exact range that will trigger a refactoring, we expand the + searched span to cover a real node range making it more likely that something useful will show up. */ + const adjustedSpan = startToken && endToken && invoked ? getAdjustedSpanFromNodes(startToken, endToken, sourceFile) : span; // 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) - const startToken = getTokenAtPosition(sourceFile, span.start); - const start = cursorRequest ? getExtractableParent(startToken): getParentNodeInSpan(startToken, sourceFile, span); + const start = cursorRequest ? getExtractableParent(startToken): getParentNodeInSpan(startToken, sourceFile, adjustedSpan); + // Do the same for the ending position - const endToken = findTokenOnLeftOfPosition(sourceFile, textSpanEnd(span)); - const end = cursorRequest ? start : getParentNodeInSpan(endToken, sourceFile, span); + const end = cursorRequest ? start : getParentNodeInSpan(endToken, sourceFile, adjustedSpan); const declarations: Symbol[] = []; @@ -295,6 +300,10 @@ namespace ts.refactor.extractSymbol { return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.cannotExtractRange)] }; } + if (isJSDoc(start)) { + return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.cannotExtractJSDoc)] }; + } + if (start.parent !== end.parent) { // start and end nodes belong to different subtrees return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.cannotExtractRange)] }; @@ -332,10 +341,6 @@ namespace ts.refactor.extractSymbol { return { targetRange: { range: statements, facts: rangeFacts, declarations } }; } - if (isJSDoc(start)) { - return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.cannotExtractJSDoc)] }; - } - if (isReturnStatement(start) && !start.expression) { // Makes no sense to extract an expression-less return statement. return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.cannotExtractRange)] }; @@ -605,6 +610,19 @@ namespace ts.refactor.extractSymbol { } } + /** + * Includes the final semicolon so that the span covers statements in cases where it would otherwise + * only cover the declaration list. + */ + function getAdjustedSpanFromNodes(startNode: Node, endNode: Node, sourceFile: SourceFile): TextSpan { + const start = startNode.getStart(sourceFile); + let end = endNode.getEnd(); + if (sourceFile.text.charCodeAt(end) === CharacterCodes.semicolon) { + end++; + } + return { start, length: end - start }; + } + function getStatementOrExpressionRange(node: Node): Statement[] | Expression | undefined { if (isStatement(node)) { return [node]; diff --git a/src/testRunner/unittests/services/extract/ranges.ts b/src/testRunner/unittests/services/extract/ranges.ts index 8fe5aeb3c131f..fd7efa79cc588 100644 --- a/src/testRunner/unittests/services/extract/ranges.ts +++ b/src/testRunner/unittests/services/extract/ranges.ts @@ -14,85 +14,100 @@ namespace ts { }); } - function testExtractRange(s: string): void { - const t = extractTest(s); - const f = createSourceFile("a.ts", t.source, ScriptTarget.Latest, /*setParentNodes*/ true); - const selectionRange = t.ranges.get("selection"); - if (!selectionRange) { - throw new Error(`Test ${s} does not specify selection range`); - } - const result = refactor.extractSymbol.getRangeToExtract(f, createTextSpanFromRange(selectionRange)); - const expectedRange = t.ranges.get("extracted"); - if (expectedRange) { - let pos: number, end: number; - const targetRange = result.targetRange!; - if (isArray(targetRange.range)) { - pos = targetRange.range[0].getStart(f); - end = last(targetRange.range).getEnd(); + function testExtractRange(caption: string, s: string) { + return it(caption, () => { + const t = extractTest(s); + const f = createSourceFile("a.ts", t.source, ScriptTarget.Latest, /*setParentNodes*/ true); + const selectionRange = t.ranges.get("selection"); + if (!selectionRange) { + throw new Error(`Test ${s} does not specify selection range`); + } + const result = refactor.extractSymbol.getRangeToExtract(f, createTextSpanFromRange(selectionRange)); + const expectedRange = t.ranges.get("extracted"); + if (expectedRange) { + let pos: number, end: number; + const targetRange = result.targetRange!; + if (isArray(targetRange.range)) { + pos = targetRange.range[0].getStart(f); + end = last(targetRange.range).getEnd(); + } + else { + pos = targetRange.range.getStart(f); + end = targetRange.range.getEnd(); + } + assert.equal(pos, expectedRange.pos, "incorrect pos of range"); + assert.equal(end, expectedRange.end, "incorrect end of range"); } else { - pos = targetRange.range.getStart(f); - end = targetRange.range.getEnd(); + assert.isTrue(!result.targetRange, `expected range to extract to be undefined`); } - assert.equal(pos, expectedRange.pos, "incorrect pos of range"); - assert.equal(end, expectedRange.end, "incorrect end of range"); - } - else { - assert.isTrue(!result.targetRange, `expected range to extract to be undefined`); - } + }); } describe("unittests:: services:: extract:: extractRanges", () => { - it("get extract range from selection", () => { - testExtractRange(` + describe("get extract range from selection", () => { + testExtractRange("extractRange1", ` [#| [$|var x = 1; var y = 2;|]|] `); - testExtractRange(` - [#| - var x = 1; - var y = 2|]; + testExtractRange("extractRange2", ` + [$|[#|var x = 1; + var y = 2|];|] `); - testExtractRange(` - [#|var x = 1|]; + testExtractRange("extractRange3", ` + [#|var x = [$|1|]|]; var y = 2; `); - testExtractRange(` + testExtractRange("extractRange4", ` + var x = [$|10[#|00|]|]; + `); + testExtractRange("extractRange5", ` + [$|va[#|r foo = 1; + var y = 200|]0;|] + `); + testExtractRange("extractRange6", ` + var x = [$|fo[#|o.bar.baz()|]|]; + `); + testExtractRange("extractRange7", ` if ([#|[#extracted|a && b && c && d|]|]) { } `); - testExtractRange(` + testExtractRange("extractRange8", ` if [#|(a && b && c && d|]) { } `); - testExtractRange(` + testExtractRange("extractRange9", ` + if ([$|a[#|a && b && c && d|]d|]) { + } + `); + testExtractRange("extractRange10", ` if (a && b && c && d) { [#| [$|var x = 1; console.log(x);|] |] } `); - testExtractRange(` + testExtractRange("extractRange11", ` [#| if (a) { return 100; } |] `); - testExtractRange(` + testExtractRange("extractRange12", ` function foo() { [#| [$|if (a) { } return 100|] |] } `); - testExtractRange(` + testExtractRange("extractRange13", ` [#| [$|l1: if (x) { break l1; }|]|] `); - testExtractRange(` + testExtractRange("extractRange14", ` [#| [$|l2: { @@ -101,21 +116,21 @@ namespace ts { break l2; }|]|] `); - testExtractRange(` + testExtractRange("extractRange15", ` while (true) { [#| if(x) { } break; |] } `); - testExtractRange(` + testExtractRange("extractRange16", ` while (true) { [#| if(x) { } continue; |] } `); - testExtractRange(` + testExtractRange("extractRange17", ` l3: { [#| @@ -124,7 +139,7 @@ namespace ts { break l3; |] } `); - testExtractRange(` + testExtractRange("extractRange18", ` function f() { while (true) { [#| @@ -134,7 +149,7 @@ namespace ts { } } `); - testExtractRange(` + testExtractRange("extractRange19", ` function f() { while (true) { [#| @@ -145,13 +160,13 @@ namespace ts { } } `); - testExtractRange(` + testExtractRange("extractRange20", ` function f() { return [#| [$|1 + 2|] |]+ 3; } } `); - testExtractRange(` + testExtractRange("extractRange21", ` function f(x: number) { [#|[$|try { x++; @@ -163,17 +178,21 @@ namespace ts { `); // Variable statements - testExtractRange(`[#|let x = [$|1|];|]`); - testExtractRange(`[#|let x = [$|1|], y;|]`); - testExtractRange(`[#|[$|let x = 1, y = 1;|]|]`); + testExtractRange("extractRange22", `[#|let x = [$|1|];|]`); + testExtractRange("extractRange23", `[#|let x = [$|1|], y;|]`); + testExtractRange("extractRange24", `[#|[$|let x = 1, y = 1;|]|]`); // Variable declarations - testExtractRange(`let [#|x = [$|1|]|];`); - testExtractRange(`let [#|x = [$|1|]|], y = 2;`); - testExtractRange(`let x = 1, [#|y = [$|2|]|];`); + testExtractRange("extractRange25", `let [#|x = [$|1|]|];`); + testExtractRange("extractRange26", `let [#|x = [$|1|]|], y = 2;`); + testExtractRange("extractRange27", `let x = 1, [#|y = [$|2|]|];`); // Return statements - testExtractRange(`[#|return [$|1|];|]`); + testExtractRange("extractRange28", `[#|return [$|1|];|]`); + + // For statements + testExtractRange("extractRange29", `for ([#|var i = 1|]; i < 2; i++) {}`); + testExtractRange("extractRange30", `for (var i = [#|[$|1|]|]; i < 2; i++) {}`); }); testExtractRangeFailed("extractRangeFailed1", diff --git a/tests/cases/fourslash/extractSymbolForTriggerReason.ts b/tests/cases/fourslash/extractSymbolForTriggerReason1.ts similarity index 100% rename from tests/cases/fourslash/extractSymbolForTriggerReason.ts rename to tests/cases/fourslash/extractSymbolForTriggerReason1.ts diff --git a/tests/cases/fourslash/extractSymbolForTriggerReason2.ts b/tests/cases/fourslash/extractSymbolForTriggerReason2.ts new file mode 100644 index 0000000000000..f0f42a3cc3fab --- /dev/null +++ b/tests/cases/fourslash/extractSymbolForTriggerReason2.ts @@ -0,0 +1,8 @@ +/// + +////const foo = ba/*a*/r + b/*b*/az; + +// Expand selection to fit nodes if refactors are explicitly requested +goTo.select("a", "b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Extract Symbol"); +verify.refactorAvailableForTriggerReason("invoked", "Extract Symbol", "constant_scope_0");