diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts index c8b4b35ff1c3e..75404d12bf01e 100644 --- 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; @@ -613,6 +639,22 @@ namespace A { [#|let a1 = { x: 1 }; return a1.x + 10;|] } +}`); + // Write + void return + testExtractMethod("extractMethod21", + `function foo() { + 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 index 25a995dc23139..83908def44455 100644 --- 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 ((node.parent).thenStatement === node || (node.parent).elseStatement === node) { - // forbid all jumps inside thenStatement or elseStatement - permittedJumps = PermittedJumps.None; - } - break; - case SyntaxKind.TryStatement: - if ((node.parent).tryBlock === node) { - // forbid all jumps inside try blocks - permittedJumps = PermittedJumps.None; - } - else if ((node.parent).finallyBlock === node) { - // allow unconditional returns from finally blocks - permittedJumps = PermittedJumps.Return; - } - break; - case SyntaxKind.CatchClause: - if ((node.parent).block === node) { - // forbid all jumps inside the block of catch clause - permittedJumps = PermittedJumps.None; - } - break; - case SyntaxKind.CaseClause: - if ((node).expression !== node) { - // allow unlabeled break inside case clauses - permittedJumps |= PermittedJumps.Break; - } - break; - default: - if (isIterationStatement(node.parent, /*lookInLabeledStatements*/ false)) { - if ((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 && (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)); } @@ -748,6 +734,10 @@ namespace ts.refactor.extractMethod { } else { newNodes.push(createStatement(createBinary(assignments[0].name, SyntaxKind.EqualsToken, call))); + + if (range.facts & RangeFacts.HasReturn) { + newNodes.push(createReturn()); + } } } else { diff --git a/tests/baselines/reference/extractMethod/extractMethod21.ts b/tests/baselines/reference/extractMethod/extractMethod21.ts new file mode 100644 index 0000000000000..4b73a6ed3c70d --- /dev/null +++ b/tests/baselines/reference/extractMethod/extractMethod21.ts @@ -0,0 +1,26 @@ +// ==ORIGINAL== +function foo() { + let x = 10; + x++; + return; +} +// ==SCOPE::function 'foo'== +function foo() { + let x = 10; + return newFunction(); + + function newFunction() { + x++; + return; + } +} +// ==SCOPE::global scope== +function foo() { + let x = 10; + x = newFunction(x); + return; +} +function newFunction(x: number) { + x++; + return x; +} diff --git a/tests/baselines/reference/extractMethod/extractMethod22.ts b/tests/baselines/reference/extractMethod/extractMethod22.ts new file mode 100644 index 0000000000000..7603c01681ff2 --- /dev/null +++ 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; +}