Skip to content

Commit fd89808

Browse files
amcaseyamcasey
authored
Merge pull request microsoft#19147 from amcasey/ReturnDeclExpr
Demote some extraction ranges to produce better results
2 parents 228a885 + e4d5d1c commit fd89808

16 files changed

+116
-29
lines changed

‎src/harness/unittests/extractFunctions.ts

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -362,27 +362,32 @@ function parsePrimaryExpression(): any {
362362
}`);
363363

364364
testExtractFunction("extractFunction_VariableDeclaration_Var", `
365-
[#|var x = 1;|]
365+
[#|var x = 1;
366+
"hello"|]
366367
x;
367368
`);
368369

369370
testExtractFunction("extractFunction_VariableDeclaration_Let_Type", `
370-
[#|let x: number = 1;|]
371+
[#|let x: number = 1;
372+
"hello";|]
371373
x;
372374
`);
373375

374376
testExtractFunction("extractFunction_VariableDeclaration_Let_NoType", `
375-
[#|let x = 1;|]
377+
[#|let x = 1;
378+
"hello";|]
376379
x;
377380
`);
378381

379382
testExtractFunction("extractFunction_VariableDeclaration_Const_Type", `
380-
[#|const x: number = 1;|]
383+
[#|const x: number = 1;
384+
"hello";|]
381385
x;
382386
`);
383387

384388
testExtractFunction("extractFunction_VariableDeclaration_Const_NoType", `
385-
[#|const x = 1;|]
389+
[#|const x = 1;
390+
"hello";|]
386391
x;
387392
`);
388393

@@ -404,7 +409,8 @@ x; y; z;
404409
`);
405410

406411
testExtractFunction("extractFunction_VariableDeclaration_ConsumedTwice", `
407-
[#|const x: number = 1;|]
412+
[#|const x: number = 1;
413+
"hello";|]
408414
x; x;
409415
`);
410416

‎src/harness/unittests/extractRanges.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,19 @@ namespace ts {
162162
}|]|]
163163
}
164164
`);
165+
166+
// Variable statements
167+
testExtractRange(`[#|let x = [$|1|];|]`);
168+
testExtractRange(`[#|let x = [$|1|], y;|]`);
169+
testExtractRange(`[#|[$|let x = 1, y = 1;|]|]`);
170+
171+
// Variable declarations
172+
testExtractRange(`let [#|x = [$|1|]|];`);
173+
testExtractRange(`let [#|x = [$|1|]|], y = 2;`);
174+
testExtractRange(`let x = 1, [#|y = [$|2|]|];`);
175+
176+
// Return statements
177+
testExtractRange(`[#|return [$|1|];|]`);
165178
});
166179

167180
testExtractRangeFailed("extractRangeFailed1",
@@ -340,6 +353,18 @@ switch (x) {
340353
refactor.extractSymbol.Messages.CannotExtractRangeContainingConditionalBreakOrContinueStatements.message
341354
]);
342355

356+
testExtractRangeFailed("extractRangeFailed12",
357+
`let [#|x|];`,
358+
[
359+
refactor.extractSymbol.Messages.StatementOrExpressionExpected.message
360+
]);
361+
362+
testExtractRangeFailed("extractRangeFailed13",
363+
`[#|return;|]`,
364+
[
365+
refactor.extractSymbol.Messages.CannotExtractRange.message
366+
]);
367+
343368
testExtractRangeFailed("extract-method-not-for-token-expression-statement", `[#|a|]`, [refactor.extractSymbol.Messages.CannotExtractIdentifier.message]);
344369
});
345370
}

‎src/services/refactors/extractSymbol.ts

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -244,12 +244,52 @@ namespace ts.refactor.extractSymbol {
244244
return { targetRange: { range: statements, facts: rangeFacts, declarations } };
245245
}
246246

247+
if (isReturnStatement(start) && !start.expression) {
248+
// Makes no sense to extract an expression-less return statement.
249+
return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractRange)] };
250+
}
251+
247252
// We have a single node (start)
248-
const errors = checkRootNode(start) || checkNode(start);
253+
const node = refineNode(start);
254+
255+
const errors = checkRootNode(node) || checkNode(node);
249256
if (errors) {
250257
return { errors };
251258
}
252-
return { targetRange: { range: getStatementOrExpressionRange(start), facts: rangeFacts, declarations } };
259+
return { targetRange: { range: getStatementOrExpressionRange(node), facts: rangeFacts, declarations } };
260+
261+
/**
262+
* Attempt to refine the extraction node (generally, by shrinking it) to produce better results.
263+
* @param node The unrefined extraction node.
264+
*/
265+
function refineNode(node: Node) {
266+
if (isReturnStatement(node)) {
267+
if (node.expression) {
268+
return node.expression;
269+
}
270+
}
271+
else if (isVariableStatement(node)) {
272+
let numInitializers = 0;
273+
let lastInitializer: Expression | undefined = undefined;
274+
for (const declaration of node.declarationList.declarations) {
275+
if (declaration.initializer) {
276+
numInitializers++;
277+
lastInitializer = declaration.initializer;
278+
}
279+
}
280+
if (numInitializers === 1) {
281+
return lastInitializer;
282+
}
283+
// No special handling if there are multiple initializers.
284+
}
285+
else if (isVariableDeclaration(node)) {
286+
if (node.initializer) {
287+
return node.initializer;
288+
}
289+
}
290+
291+
return node;
292+
}
253293

254294
function checkRootNode(node: Node): Diagnostic[] | undefined {
255295
if (isIdentifier(isExpressionStatement(node) ? node.expression : node)) {

‎tests/baselines/reference/extractFunction/extractFunction29.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ interface UnaryExpression {
2626
function parseUnaryExpression(operator: string): UnaryExpression {
2727
return /*RENAME*/newFunction();
2828

29-
function newFunction() {
29+
function newFunction(): UnaryExpression {
3030
return {
3131
kind: "Unary",
3232
operator,
@@ -49,7 +49,7 @@ function parseUnaryExpression(operator: string): UnaryExpression {
4949
return /*RENAME*/newFunction(operator);
5050
}
5151

52-
function newFunction(operator: string) {
52+
function newFunction(operator: string): UnaryExpression {
5353
return {
5454
kind: "Unary",
5555
operator,

‎tests/baselines/reference/extractFunction/extractFunction32.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,11 @@ namespace N {
1717
export const value = 1;
1818

1919
() => {
20-
/*RENAME*/newFunction();
20+
var c = /*RENAME*/newFunction()
2121
}
2222

2323
function newFunction() {
24-
var c = class {
24+
return class {
2525
M() {
2626
return value;
2727
}
@@ -34,12 +34,12 @@ namespace N {
3434
export const value = 1;
3535

3636
() => {
37-
/*RENAME*/newFunction();
37+
var c = /*RENAME*/newFunction()
3838
}
3939
}
4040

4141
function newFunction() {
42-
var c = class {
42+
return class {
4343
M() {
4444
return N.value;
4545
}

‎tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_NoType.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// ==ORIGINAL==
22

3-
/*[#|*/const x = 1;/*|]*/
3+
/*[#|*/const x = 1;
4+
"hello";/*|]*/
45
x;
56

67
// ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
1011

1112
function newFunction() {
1213
const x = 1;
14+
"hello";
1315
return x;
1416
}

‎tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_NoType.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// ==ORIGINAL==
22

3-
/*[#|*/const x = 1;/*|]*/
3+
/*[#|*/const x = 1;
4+
"hello";/*|]*/
45
x;
56

67
// ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
1011

1112
function newFunction() {
1213
const x = 1;
14+
"hello";
1315
return x;
1416
}

‎tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_Type.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// ==ORIGINAL==
22

3-
/*[#|*/const x: number = 1;/*|]*/
3+
/*[#|*/const x: number = 1;
4+
"hello";/*|]*/
45
x;
56

67
// ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
1011

1112
function newFunction() {
1213
const x: number = 1;
14+
"hello";
1315
return x;
1416
}

‎tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_ConsumedTwice.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// ==ORIGINAL==
22

3-
/*[#|*/const x: number = 1;/*|]*/
3+
/*[#|*/const x: number = 1;
4+
"hello";/*|]*/
45
x; x;
56

67
// ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x; x;
1011

1112
function newFunction() {
1213
const x: number = 1;
14+
"hello";
1315
return x;
1416
}

‎tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_NoType.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// ==ORIGINAL==
22

3-
/*[#|*/let x = 1;/*|]*/
3+
/*[#|*/let x = 1;
4+
"hello";/*|]*/
45
x;
56

67
// ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
1011

1112
function newFunction() {
1213
let x = 1;
14+
"hello";
1315
return x;
1416
}

‎tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_NoType.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// ==ORIGINAL==
22

3-
/*[#|*/let x = 1;/*|]*/
3+
/*[#|*/let x = 1;
4+
"hello";/*|]*/
45
x;
56

67
// ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
1011

1112
function newFunction() {
1213
let x = 1;
14+
"hello";
1315
return x;
1416
}

‎tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_Type.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// ==ORIGINAL==
22

3-
/*[#|*/let x: number = 1;/*|]*/
3+
/*[#|*/let x: number = 1;
4+
"hello";/*|]*/
45
x;
56

67
// ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
1011

1112
function newFunction() {
1213
let x: number = 1;
14+
"hello";
1315
return x;
1416
}

‎tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Var.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// ==ORIGINAL==
22

3-
/*[#|*/var x = 1;/*|]*/
3+
/*[#|*/var x = 1;
4+
"hello"/*|]*/
45
x;
56

67
// ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
1011

1112
function newFunction() {
1213
var x = 1;
14+
"hello";
1315
return x;
1416
}

‎tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Var.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// ==ORIGINAL==
22

3-
/*[#|*/var x = 1;/*|]*/
3+
/*[#|*/var x = 1;
4+
"hello"/*|]*/
45
x;
56

67
// ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
1011

1112
function newFunction() {
1213
var x = 1;
14+
"hello";
1315
return x;
1416
}

‎tests/cases/fourslash/extract-method14.ts

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
// @Filename: foo.js
88
//// function foo() {
99
//// var i = 10;
10-
//// /*a*/return i++;/*b*/
10+
//// /*a*/return i + 1;/*b*/
1111
//// }
1212

1313
goTo.select('a', 'b');
@@ -18,13 +18,11 @@ edit.applyRefactor({
1818
newContent:
1919
`function foo() {
2020
var i = 10;
21-
let __return;
22-
({ __return, i } = /*RENAME*/newFunction(i));
23-
return __return;
21+
return /*RENAME*/newFunction(i);
2422
}
2523
2624
function newFunction(i) {
27-
return { __return: i++, i };
25+
return i + 1;
2826
}
2927
`
3028
});

‎tests/cases/fourslash/extract-method22.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
// You may not extract variable declarations with the export modifier
44

55
//// namespace NS {
6-
//// /*start*/export var x = 10;/*end*/
6+
//// /*start*/export var x = 10, y = 15;/*end*/
77
//// }
88

99
goTo.select('start', 'end')

0 commit comments

Comments
 (0)