Simplify and correct PermittedJumps computation

amcasey · amcasey · commit e3808b65d429 · 2017-09-07T16:21:20.000-07:00
1. It was looking at the parent which wasn't guaranteed to be in the extracted range. 2. It was checking direct, rather than indirect containment - apparently to avoid applying the rules to certain expressions (which can't contain jumps anyway, unless they're in anonymous functions, in which case they're fine). Fixes microsoft#18144
diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts
@@ -378,6 +378,32 @@ namespace A {
             "Cannot extract range containing conditional return statement."
         ]);
 
+        testExtractRangeFailed("extractRangeFailed7",
+        `
+function test(x: number) {
+    while (x) {
+        x--;
+        [#|break;|]
+    }
+}
+        `,
+        [
+            "Cannot extract range containing conditional break or continue statements."
+        ]);
+
+        testExtractRangeFailed("extractRangeFailed8",
+        `
+function test(x: number) {
+    switch (x) {
+        case 1:
+            [#|break;|]
+    }
+}
+        `,
+        [
+            "Cannot extract range containing conditional break or continue statements."
+        ]);
+
         testExtractMethod("extractMethod1",
             `namespace A {
     let x = 1;
@@ -620,6 +646,15 @@ namespace A {
     let x = 10;
     [#|x++;
     return;|]
+}`);
+        // Write + void return
+        testExtractMethod("extractMethod22",
+            `function test() {
+    try {
+    }
+    finally {
+        [#|return 1;|]
+    }
 }`);
     });
 
diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts
@@ -350,45 +350,31 @@ namespace ts.refactor.extractMethod {
                     // do not dive into functions or classes
                     return false;
                 }
-                if (node.parent) {
-                    switch (node.parent.kind) {
-                        case SyntaxKind.IfStatement:
-                            if ((<IfStatement>node.parent).thenStatement === node || (<IfStatement>node.parent).elseStatement === node) {
-                                // forbid all jumps inside thenStatement or elseStatement
-                                permittedJumps = PermittedJumps.None;
-                            }
-                            break;
-                        case SyntaxKind.TryStatement:
-                            if ((<TryStatement>node.parent).tryBlock === node) {
-                                // forbid all jumps inside try blocks
-                                permittedJumps = PermittedJumps.None;
-                            }
-                            else if ((<TryStatement>node.parent).finallyBlock === node) {
-                                // allow unconditional returns from finally blocks
-                                permittedJumps = PermittedJumps.Return;
-                            }
-                            break;
-                        case SyntaxKind.CatchClause:
-                            if ((<CatchClause>node.parent).block === node) {
-                                // forbid all jumps inside the block of catch clause
-                                permittedJumps = PermittedJumps.None;
-                            }
-                            break;
-                        case SyntaxKind.CaseClause:
-                            if ((<CaseClause>node).expression !== node) {
-                                // allow unlabeled break inside case clauses
-                                permittedJumps |= PermittedJumps.Break;
-                            }
-                            break;
-                        default:
-                            if (isIterationStatement(node.parent, /*lookInLabeledStatements*/ false)) {
-                                if ((<IterationStatement>node.parent).statement === node) {
-                                    // allow unlabeled break/continue inside loops
-                                    permittedJumps |= PermittedJumps.Break | PermittedJumps.Continue;
-                                }
-                            }
-                            break;
-                    }
+
+                switch (node.kind) {
+                    case SyntaxKind.IfStatement:
+                        permittedJumps = PermittedJumps.None;
+                        break;
+                    case SyntaxKind.TryStatement:
+                        // forbid all jumps inside try blocks
+                        permittedJumps = PermittedJumps.None;
+                        break;
+                    case SyntaxKind.Block:
+                        if (node.parent && node.parent.kind === SyntaxKind.TryStatement && (<TryStatement>node).finallyBlock === node) {
+                            // allow unconditional returns from finally blocks
+                            permittedJumps = PermittedJumps.Return;
+                        }
+                        break;
+                    case SyntaxKind.CaseClause:
+                        // allow unlabeled break inside case clauses
+                        permittedJumps |= PermittedJumps.Break;
+                        break;
+                    default:
+                        if (isIterationStatement(node, /*lookInLabeledStatements*/ false)) {
+                            // allow unlabeled break/continue inside loops
+                            permittedJumps |= PermittedJumps.Break | PermittedJumps.Continue;
+                        }
+                        break;
                 }
 
                 switch (node.kind) {
diff --git a/tests/baselines/reference/extractMethod/extractMethod22.ts b/tests/baselines/reference/extractMethod/extractMethod22.ts
@@ -0,0 +1,31 @@
+// ==ORIGINAL==
+function test() {
+    try {
+    }
+    finally {
+        return 1;
+    }
+}
+// ==SCOPE::function 'test'==
+function test() {
+    try {
+    }
+    finally {
+        return newFunction();
+    }
+
+    function newFunction() {
+        return 1;
+    }
+}
+// ==SCOPE::global scope==
+function test() {
+    try {
+    }
+    finally {
+        return newFunction();
+    }
+}
+function newFunction() {
+    return 1;
+}
