From 74d9b19a27cf8513f0307710091d77388cd9bcb0 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Thu, 11 Feb 2021 15:04:52 -0800 Subject: [PATCH 1/6] allow partial selections of node ranges --- src/services/refactors/extractSymbol.ts | 43 +++++++++++++------ .../unittests/services/extract/ranges.ts | 21 +++++++-- 2 files changed, 47 insertions(+), 17 deletions(-) diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 6478ad24d5ba5..420203bd34117 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,8 +264,8 @@ 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 { @@ -276,13 +275,18 @@ namespace ts.refactor.extractSymbol { } const cursorRequest = length === 0 && considerEmptySpans; + const startToken = getTokenAtPosition(sourceFile, span.start); + const endToken = findTokenOnLeftOfPosition(sourceFile, textSpanEnd(span)); + /* Users don't necessarily know the exact range they need to select to get this refactor so we adjust the + span to fully cover the start and end tokens. This improves discoverability and ease of use. */ + const adjustedSpan = startToken && endToken ? 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 +299,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 +340,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 +609,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..9bf028119c133 100644 --- a/src/testRunner/unittests/services/extract/ranges.ts +++ b/src/testRunner/unittests/services/extract/ranges.ts @@ -50,14 +50,23 @@ namespace ts { var y = 2;|]|] `); testExtractRange(` - [#| - var x = 1; - var y = 2|]; + [$|[#|var x = 1; + var y = 2|];|] `); testExtractRange(` - [#|var x = 1|]; + [#|var x = [$|1|]|]; var y = 2; `); + testExtractRange(` + var x = [$|10[#|00|]|]; + `); + testExtractRange(` + [$|va[#|r foo = 1; + var y = 200|]0;|] + `); + testExtractRange(` + var x = [$|fo[#|o.bar.baz()|]|]; + `); testExtractRange(` if ([#|[#extracted|a && b && c && d|]|]) { } @@ -66,6 +75,10 @@ namespace ts { if [#|(a && b && c && d|]) { } `); + testExtractRange(` + if ([$|a[#|a && b && c && d|]d|]) { + } + `); testExtractRange(` if (a && b && c && d) { [#| [$|var x = 1; From 9774b3af96d6b8dc21c9904f8201f9bc528391e6 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Thu, 11 Feb 2021 15:19:40 -0800 Subject: [PATCH 2/6] separate tests for better failure investigation --- .../unittests/services/extract/ranges.ts | 106 +++++++++--------- 1 file changed, 54 insertions(+), 52 deletions(-) diff --git a/src/testRunner/unittests/services/extract/ranges.ts b/src/testRunner/unittests/services/extract/ranges.ts index 9bf028119c133..071337463c783 100644 --- a/src/testRunner/unittests/services/extract/ranges.ts +++ b/src/testRunner/unittests/services/extract/ranges.ts @@ -14,98 +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(` + testExtractRange("extractRange2", ` [$|[#|var x = 1; var y = 2|];|] `); - testExtractRange(` + testExtractRange("extractRange3", ` [#|var x = [$|1|]|]; var y = 2; `); - testExtractRange(` + testExtractRange("extractRange4", ` var x = [$|10[#|00|]|]; `); - testExtractRange(` + testExtractRange("extractRange5", ` [$|va[#|r foo = 1; var y = 200|]0;|] `); - testExtractRange(` + testExtractRange("extractRange6", ` var x = [$|fo[#|o.bar.baz()|]|]; `); - testExtractRange(` + 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(` + 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: { @@ -114,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: { [#| @@ -137,7 +139,7 @@ namespace ts { break l3; |] } `); - testExtractRange(` + testExtractRange("extractRange18", ` function f() { while (true) { [#| @@ -147,7 +149,7 @@ namespace ts { } } `); - testExtractRange(` + testExtractRange("extractRange19", ` function f() { while (true) { [#| @@ -158,13 +160,13 @@ namespace ts { } } `); - testExtractRange(` + testExtractRange("extractRange20", ` function f() { return [#| [$|1 + 2|] |]+ 3; } } `); - testExtractRange(` + testExtractRange("extractRange21", ` function f(x: number) { [#|[$|try { x++; @@ -176,17 +178,17 @@ 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|];|]`); }); testExtractRangeFailed("extractRangeFailed1", From 2ec179b25a685b5d07b7e8e0f46400cfcf2fca2c Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Fri, 12 Feb 2021 13:48:07 -0800 Subject: [PATCH 3/6] gate span expansion behind invoked command --- src/services/refactors/extractSymbol.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 420203bd34117..bc0f07eec2c42 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -268,18 +268,19 @@ namespace ts.refactor.extractSymbol { * 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)); - /* Users don't necessarily know the exact range they need to select to get this refactor so we adjust the - span to fully cover the start and end tokens. This improves discoverability and ease of use. */ - const adjustedSpan = startToken && endToken ? getAdjustedSpanFromNodes(startToken, endToken, sourceFile) : span; + /* If the refactor command is invoked through a keyboard action it's safe to assume that the user is actively looking for + refactor actions at the span location. As they may not know the exact range that will trigger a refactor, 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) From 20db66f2445520a2924dfcd485e5518a5b67ed93 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Fri, 12 Feb 2021 13:54:32 -0800 Subject: [PATCH 4/6] add invoked test --- src/services/refactors/extractSymbol.ts | 2 +- ...TriggerReason.ts => extractSymbolForTriggerReason1.ts} | 0 tests/cases/fourslash/extractSymbolForTriggerReason2.ts | 8 ++++++++ 3 files changed, 9 insertions(+), 1 deletion(-) rename tests/cases/fourslash/{extractSymbolForTriggerReason.ts => extractSymbolForTriggerReason1.ts} (100%) create mode 100644 tests/cases/fourslash/extractSymbolForTriggerReason2.ts diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index bc0f07eec2c42..58de6ed25578d 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -278,7 +278,7 @@ namespace ts.refactor.extractSymbol { const startToken = getTokenAtPosition(sourceFile, span.start); const endToken = findTokenOnLeftOfPosition(sourceFile, textSpanEnd(span)); /* If the refactor command is invoked through a keyboard action it's safe to assume that the user is actively looking for - refactor actions at the span location. As they may not know the exact range that will trigger a refactor, we expand the + refactor actions at the span location. As they may not know the exact range that will trigger a refactor, 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; 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"); From 610da7ee2419dbeb35dc3bb1f5020667c0473b4b Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 16 Feb 2021 12:08:41 -0800 Subject: [PATCH 5/6] comment wording --- src/services/refactors/extractSymbol.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 58de6ed25578d..62ba653961f06 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -277,8 +277,8 @@ namespace ts.refactor.extractSymbol { const startToken = getTokenAtPosition(sourceFile, span.start); const endToken = findTokenOnLeftOfPosition(sourceFile, textSpanEnd(span)); - /* If the refactor command is invoked through a keyboard action it's safe to assume that the user is actively looking for - refactor actions at the span location. As they may not know the exact range that will trigger a refactor, we expand the + /* 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; From 8a630483235eee0ff55e74c1dabf1b59a5bb820e Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Wed, 17 Feb 2021 12:20:46 -0800 Subject: [PATCH 6/6] for test --- src/testRunner/unittests/services/extract/ranges.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/testRunner/unittests/services/extract/ranges.ts b/src/testRunner/unittests/services/extract/ranges.ts index 071337463c783..fd7efa79cc588 100644 --- a/src/testRunner/unittests/services/extract/ranges.ts +++ b/src/testRunner/unittests/services/extract/ranges.ts @@ -189,6 +189,10 @@ namespace ts { // Return statements 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",