From 0e50da62c47368e7141301d0cf4e9cc105497ef2 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 30 Aug 2017 13:11:21 -0700 Subject: [PATCH 1/6] Handle the combination of a write and a void return When the return type is void, there's no `returnValueProperty`, but that doesn't mean we don't need a `return` at the call site. Fixes #18140. --- src/harness/unittests/extractMethods.ts | 7 +++++ src/services/refactors/extractMethod.ts | 4 +++ .../extractMethod/extractMethod21.ts | 26 +++++++++++++++++++ 3 files changed, 37 insertions(+) create mode 100644 tests/baselines/reference/extractMethod/extractMethod21.ts diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts index c8b4b35ff1c3e..6fe5789572545 100644 --- a/src/harness/unittests/extractMethods.ts +++ b/src/harness/unittests/extractMethods.ts @@ -613,6 +613,13 @@ namespace A { [#|let a1 = { x: 1 }; return a1.x + 10;|] } +}`); + // Write + void return + testExtractMethod("extractMethod21", + `function foo() { + let x = 10; + [#|x++; + return;|] }`); }); diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 25a995dc23139..c497b12675990 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -748,6 +748,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; +} From 9d11fbb9b9c4e7fbbc4a54d1b95d41e28e13ea42 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 30 Aug 2017 13:25:35 -0700 Subject: [PATCH 2/6] Correct permitted jumps check --- src/services/refactors/extractMethod.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index c497b12675990..e4fd0e85dc2e5 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -417,7 +417,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)); } From a81fa7a801c7eff1865bcc2cd451bfc618ce832b Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 30 Aug 2017 13:55:18 -0700 Subject: [PATCH 3/6] Make permittedJumps a parameter to eliminate save-restore pattern --- src/services/refactors/extractMethod.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index e4fd0e85dc2e5..b73613350a8bf 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -293,14 +293,13 @@ namespace ts.refactor.extractMethod { } let errors: Diagnostic[]; - let permittedJumps = PermittedJumps.Return; let seenLabels: Array<__String>; - visit(nodeToCheck); + visit(nodeToCheck, PermittedJumps.Return); return errors; - function visit(node: Node) { + function visit(node: Node, permittedJumps: PermittedJumps) { if (errors) { // already found an error - can stop now return true; @@ -351,7 +350,6 @@ namespace ts.refactor.extractMethod { // do not dive into functions or classes return false; } - const savedPermittedJumps = permittedJumps; if (node.parent) { switch (node.parent.kind) { case SyntaxKind.IfStatement: @@ -402,7 +400,7 @@ namespace ts.refactor.extractMethod { { const label = (node).label; (seenLabels || (seenLabels = [])).push(label.escapedText); - forEachChild(node, visit); + forEachChild(node, child => visit(child, permittedJumps)); seenLabels.pop(); break; } @@ -439,11 +437,10 @@ namespace ts.refactor.extractMethod { } break; default: - forEachChild(node, visit); + forEachChild(node, child => visit(child, permittedJumps)); break; } - permittedJumps = savedPermittedJumps; } } } From e3808b65d4291c982cba374d336d65b1be87fae2 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 30 Aug 2017 14:23:11 -0700 Subject: [PATCH 4/6] Simplify and correct PermittedJumps computation 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 #18144 --- src/harness/unittests/extractMethods.ts | 35 ++++++++++ src/services/refactors/extractMethod.ts | 64 ++++++++----------- .../extractMethod/extractMethod22.ts | 31 +++++++++ 3 files changed, 91 insertions(+), 39 deletions(-) create mode 100644 tests/baselines/reference/extractMethod/extractMethod22.ts diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts index 6fe5789572545..02ddbf4cae37f 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; @@ -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 index b73613350a8bf..c0031f26cc8f4 100644 --- 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 ((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) { 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; +} From 73bc0c9796ce2e294e8b0fa1a1dec46da1268187 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 30 Aug 2017 14:36:20 -0700 Subject: [PATCH 5/6] Correct copied comment --- src/harness/unittests/extractMethods.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts index 02ddbf4cae37f..75404d12bf01e 100644 --- a/src/harness/unittests/extractMethods.ts +++ b/src/harness/unittests/extractMethods.ts @@ -647,7 +647,7 @@ function test(x: number) { [#|x++; return;|] }`); - // Write + void return + // Return in finally block testExtractMethod("extractMethod22", `function test() { try { From baefdd2ccb21542b7aeab8b6f825e45c516566da Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 7 Sep 2017 15:36:32 -0700 Subject: [PATCH 6/6] Revert "Make permittedJumps a parameter to eliminate save-restore pattern" This reverts commit 57906fe90e8efd2fb285fcb67f018c0438ba06dd. --- src/services/refactors/extractMethod.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index c0031f26cc8f4..83908def44455 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -293,13 +293,14 @@ namespace ts.refactor.extractMethod { } let errors: Diagnostic[]; + let permittedJumps = PermittedJumps.Return; let seenLabels: Array<__String>; - visit(nodeToCheck, PermittedJumps.Return); + visit(nodeToCheck); return errors; - function visit(node: Node, permittedJumps: PermittedJumps) { + function visit(node: Node) { if (errors) { // already found an error - can stop now return true; @@ -350,6 +351,7 @@ namespace ts.refactor.extractMethod { // do not dive into functions or classes return false; } + const savedPermittedJumps = permittedJumps; switch (node.kind) { case SyntaxKind.IfStatement: @@ -386,7 +388,7 @@ namespace ts.refactor.extractMethod { { const label = (node).label; (seenLabels || (seenLabels = [])).push(label.escapedText); - forEachChild(node, child => visit(child, permittedJumps)); + forEachChild(node, visit); seenLabels.pop(); break; } @@ -423,10 +425,11 @@ namespace ts.refactor.extractMethod { } break; default: - forEachChild(node, child => visit(child, permittedJumps)); + forEachChild(node, visit); break; } + permittedJumps = savedPermittedJumps; } } }