Extract method

src/harness/fourslash.ts

@@ -2749,17 +2749,18 @@ 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
             };
 
             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);
         }
     }
 

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]
-                : <Expression>start;
+                : start.parent && start.parent.kind === SyntaxKind.ExpressionStatement
+                    ? [start.parent as Statement]
+                    : <Expression>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(<Expression>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((<ReturnStatement>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(<Statement[]>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;

tests/baselines/reference/extractMethod/extractMethod1.js

@@ -43,15 +43,15 @@ namespace A {
         function a() {
             let a = 1;
 
-            ({ a } = newFunction(a));
+            a = newFunction(a);
         }
 
         function newFunction(a: number) {
             let y = 5;
             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;
 }

tests/baselines/reference/extractMethod/extractMethod5.js

@@ -43,15 +43,15 @@ namespace A {
         function a() {
             let a = 1;
 
-            ({ a } = newFunction(a));
+            a = newFunction(a);
         }
 
         function newFunction(a: number) {
             let y = 5;
             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;
 }

tests/cases/fourslash/extract-method14.ts

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

tests/cases/fourslash/extract-method15.ts

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

tests/cases/fourslash/extract-method17.ts

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

tests/cases/fourslash/extract-method18.ts

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

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;