Expand extractSymbol ranges (#42770)

* allow partial selections of node ranges

* separate tests for better failure investigation

* gate span expansion behind invoked command

* add invoked test

* comment wording

* for test

Author: Jesse Trinity

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];

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",

tests/cases/fourslash/extractSymbolForTriggerReason.ts renamed to tests/cases/fourslash/extractSymbolForTriggerReason1.ts

@@ -0,0 +1,8 @@
+/// <reference path='fourslash.ts' />
+
+////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");