Skip to content

Commit e3808b6

Browse files
committed
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
1 parent a81fa7a commit e3808b6

File tree

3 files changed

+91
-39
lines changed

3 files changed

+91
-39
lines changed

src/harness/unittests/extractMethods.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,32 @@ namespace A {
378378
"Cannot extract range containing conditional return statement."
379379
]);
380380

381+
testExtractRangeFailed("extractRangeFailed7",
382+
`
383+
function test(x: number) {
384+
while (x) {
385+
x--;
386+
[#|break;|]
387+
}
388+
}
389+
`,
390+
[
391+
"Cannot extract range containing conditional break or continue statements."
392+
]);
393+
394+
testExtractRangeFailed("extractRangeFailed8",
395+
`
396+
function test(x: number) {
397+
switch (x) {
398+
case 1:
399+
[#|break;|]
400+
}
401+
}
402+
`,
403+
[
404+
"Cannot extract range containing conditional break or continue statements."
405+
]);
406+
381407
testExtractMethod("extractMethod1",
382408
`namespace A {
383409
let x = 1;
@@ -620,6 +646,15 @@ namespace A {
620646
let x = 10;
621647
[#|x++;
622648
return;|]
649+
}`);
650+
// Write + void return
651+
testExtractMethod("extractMethod22",
652+
`function test() {
653+
try {
654+
}
655+
finally {
656+
[#|return 1;|]
657+
}
623658
}`);
624659
});
625660

src/services/refactors/extractMethod.ts

Lines changed: 25 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -350,45 +350,31 @@ namespace ts.refactor.extractMethod {
350350
// do not dive into functions or classes
351351
return false;
352352
}
353-
if (node.parent) {
354-
switch (node.parent.kind) {
355-
case SyntaxKind.IfStatement:
356-
if ((<IfStatement>node.parent).thenStatement === node || (<IfStatement>node.parent).elseStatement === node) {
357-
// forbid all jumps inside thenStatement or elseStatement
358-
permittedJumps = PermittedJumps.None;
359-
}
360-
break;
361-
case SyntaxKind.TryStatement:
362-
if ((<TryStatement>node.parent).tryBlock === node) {
363-
// forbid all jumps inside try blocks
364-
permittedJumps = PermittedJumps.None;
365-
}
366-
else if ((<TryStatement>node.parent).finallyBlock === node) {
367-
// allow unconditional returns from finally blocks
368-
permittedJumps = PermittedJumps.Return;
369-
}
370-
break;
371-
case SyntaxKind.CatchClause:
372-
if ((<CatchClause>node.parent).block === node) {
373-
// forbid all jumps inside the block of catch clause
374-
permittedJumps = PermittedJumps.None;
375-
}
376-
break;
377-
case SyntaxKind.CaseClause:
378-
if ((<CaseClause>node).expression !== node) {
379-
// allow unlabeled break inside case clauses
380-
permittedJumps |= PermittedJumps.Break;
381-
}
382-
break;
383-
default:
384-
if (isIterationStatement(node.parent, /*lookInLabeledStatements*/ false)) {
385-
if ((<IterationStatement>node.parent).statement === node) {
386-
// allow unlabeled break/continue inside loops
387-
permittedJumps |= PermittedJumps.Break | PermittedJumps.Continue;
388-
}
389-
}
390-
break;
391-
}
353+
354+
switch (node.kind) {
355+
case SyntaxKind.IfStatement:
356+
permittedJumps = PermittedJumps.None;
357+
break;
358+
case SyntaxKind.TryStatement:
359+
// forbid all jumps inside try blocks
360+
permittedJumps = PermittedJumps.None;
361+
break;
362+
case SyntaxKind.Block:
363+
if (node.parent && node.parent.kind === SyntaxKind.TryStatement && (<TryStatement>node).finallyBlock === node) {
364+
// allow unconditional returns from finally blocks
365+
permittedJumps = PermittedJumps.Return;
366+
}
367+
break;
368+
case SyntaxKind.CaseClause:
369+
// allow unlabeled break inside case clauses
370+
permittedJumps |= PermittedJumps.Break;
371+
break;
372+
default:
373+
if (isIterationStatement(node, /*lookInLabeledStatements*/ false)) {
374+
// allow unlabeled break/continue inside loops
375+
permittedJumps |= PermittedJumps.Break | PermittedJumps.Continue;
376+
}
377+
break;
392378
}
393379

394380
switch (node.kind) {
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
// ==ORIGINAL==
2+
function test() {
3+
try {
4+
}
5+
finally {
6+
return 1;
7+
}
8+
}
9+
// ==SCOPE::function 'test'==
10+
function test() {
11+
try {
12+
}
13+
finally {
14+
return newFunction();
15+
}
16+
17+
function newFunction() {
18+
return 1;
19+
}
20+
}
21+
// ==SCOPE::global scope==
22+
function test() {
23+
try {
24+
}
25+
finally {
26+
return newFunction();
27+
}
28+
}
29+
function newFunction() {
30+
return 1;
31+
}

0 commit comments

Comments
 (0)