Skip to content

Simplify and correct PermittedJumps computation #18165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Sep 8, 2017
Merged

Simplify and correct PermittedJumps computation #18165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0e50da6
Handle the combination of a write and a void return
amcasey Aug 30, 2017
9d11fbb
Correct permitted jumps check
amcasey Aug 30, 2017
a81fa7a
Make permittedJumps a parameter to eliminate save-restore pattern
amcasey Aug 30, 2017
e3808b6
Simplify and correct PermittedJumps computation
amcasey Aug 30, 2017
73bc0c9
Correct copied comment
amcasey Aug 30, 2017
baefdd2
Revert "Make permittedJumps a parameter to eliminate save-restore pat…
amcasey Sep 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions src/harness/unittests/extractMethods.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;|]
}
}`);
});

Expand Down
70 changes: 30 additions & 40 deletions src/services/refactors/extractMethod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function now switches on node.kind four different times, is there any way it could be simplified or broken up?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand your concern. The switches don't appear to overlap (I didn't check exhaustively) and different kinds are interesting at different times. I imagine that parts of the method could be split out into helpers but I don't see much advantage to doing so.

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) {
Expand All @@ -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));
}
Expand Down Expand Up @@ -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 {
Expand Down
26 changes: 26 additions & 0 deletions tests/baselines/reference/extractMethod/extractMethod21.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}
31 changes: 31 additions & 0 deletions tests/baselines/reference/extractMethod/extractMethod22.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}