Merge pull request #18165 from amcasey/GH18144

Simplify and correct PermittedJumps computation
microsoft · Sep 8, 2017 · deefb01 · deefb01
2 parents d4e3e19 + baefdd2
commit deefb01
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 40 deletions.
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;|]
+}`);
+        // Return in finally block
+        testExtractMethod("extractMethod22",
+            `function test() {
+    try {
+    }
+    finally {
+        [#|return 1;|]
+    }
 }`);
     });
 

diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts
@@ -352,45 +352,31 @@ namespace ts.refactor.extractMethod {
                     return false;
                 }
                 const savedPermittedJumps = permittedJumps;
-                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) {
@@ -417,7 +403,7 @@ namespace ts.refactor.extractMethod {
                                 }
                             }
                             else {
-                                if (!(permittedJumps & (SyntaxKind.BreakStatement ? PermittedJumps.Break : PermittedJumps.Continue))) {
+                                if (!(permittedJumps & (node.kind === SyntaxKind.BreakStatement ? PermittedJumps.Break : PermittedJumps.Continue))) {
                                     // attempt to break or continue in a forbidden context
                                     (errors || (errors = [])).push(createDiagnosticForNode(node, Messages.CannotExtractRangeContainingConditionalBreakOrContinueStatements));
                                 }

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;
+}