Skip to content

Commit 40e4591

Browse files
amcaseyamcasey
authored
Merge pull request microsoft#18343 from amcasey/InsertionPosition
Improve insertion position of extracted methods
2 parents 403f585 + 62899d1 commit 40e4591

10 files changed

Lines changed: 323 additions & 11 deletions

File tree

‎src/compiler/utilities.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -500,13 +500,11 @@ namespace ts {
500500
case SyntaxKind.ArrowFunction:
501501
return true;
502502
default:
503-
staticAssertNever(node);
503+
assertTypeIsNever(node);
504504
return false;
505505
}
506506
}
507507

508-
function staticAssertNever(_: never): void {}
509-
510508
// Gets the nearest enclosing block scope container that has the provided node
511509
// as a descendant, that is not the provided node.
512510
export function getEnclosingBlockScopeContainer(node: Node): Node {

‎src/harness/unittests/extractMethods.ts‎

Lines changed: 56 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ namespace ts {
224224
testExtractRange(`
225225
function f() {
226226
while (true) {
227-
[#|
227+
[#|
228228
if (x) {
229229
return;
230230
} |]
@@ -234,7 +234,7 @@ namespace ts {
234234
testExtractRange(`
235235
function f() {
236236
while (true) {
237-
[#|
237+
[#|
238238
[$|if (x) {
239239
}
240240
return;|]
@@ -655,6 +655,60 @@ function test(x: number) {
655655
finally {
656656
[#|return 1;|]
657657
}
658+
}`);
659+
// Extraction position - namespace
660+
testExtractMethod("extractMethod23",
661+
`namespace NS {
662+
function M1() { }
663+
function M2() {
664+
[#|return 1;|]
665+
}
666+
function M3() { }
667+
}`);
668+
// Extraction position - function
669+
testExtractMethod("extractMethod24",
670+
`function Outer() {
671+
function M1() { }
672+
function M2() {
673+
[#|return 1;|]
674+
}
675+
function M3() { }
676+
}`);
677+
// Extraction position - file
678+
testExtractMethod("extractMethod25",
679+
`function M1() { }
680+
function M2() {
681+
[#|return 1;|]
682+
}
683+
function M3() { }`);
684+
// Extraction position - class without ctor
685+
testExtractMethod("extractMethod26",
686+
`class C {
687+
M1() { }
688+
M2() {
689+
[#|return 1;|]
690+
}
691+
M3() { }
692+
}`);
693+
// Extraction position - class with ctor in middle
694+
testExtractMethod("extractMethod27",
695+
`class C {
696+
M1() { }
697+
M2() {
698+
[#|return 1;|]
699+
}
700+
constructor() { }
701+
M3() { }
702+
}`);
703+
// Extraction position - class with ctor at end
704+
testExtractMethod("extractMethod28",
705+
`class C {
706+
M1() { }
707+
M2() {
708+
[#|return 1;|]
709+
}
710+
M3() { }
711+
constructor() { }
658712
}`);
659713
});
660714

‎src/services/refactors/extractMethod.ts‎

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -698,8 +698,14 @@ namespace ts.refactor.extractMethod {
698698
}
699699

700700
const changeTracker = textChanges.ChangeTracker.fromContext(context);
701-
// insert function at the end of the scope
702-
changeTracker.insertNodeBefore(context.file, scope.getLastToken(), newFunction, { prefix: context.newLineCharacter, suffix: context.newLineCharacter });
701+
const minInsertionPos = (isReadonlyArray(range.range) ? lastOrUndefined(range.range) : range.range).end;
702+
const nodeToInsertBefore = getNodeToInsertBefore(minInsertionPos, scope);
703+
if (nodeToInsertBefore) {
704+
changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newFunction, { suffix: context.newLineCharacter + context.newLineCharacter });
705+
}
706+
else {
707+
changeTracker.insertNodeBefore(context.file, scope.getLastToken(), newFunction, { prefix: context.newLineCharacter, suffix: context.newLineCharacter });
708+
}
703709

704710
const newNodes: Node[] = [];
705711
// replace range with function call
@@ -832,6 +838,39 @@ namespace ts.refactor.extractMethod {
832838
return "__return";
833839
}
834840

841+
function getStatementsOrClassElements(scope: Scope): ReadonlyArray<Statement> | ReadonlyArray<ClassElement> {
842+
if (isFunctionLike(scope)) {
843+
const body = scope.body;
844+
if (isBlock(body)) {
845+
return body.statements;
846+
}
847+
}
848+
else if (isModuleBlock(scope) || isSourceFile(scope)) {
849+
return scope.statements;
850+
}
851+
else if (isClassLike(scope)) {
852+
return scope.members;
853+
}
854+
else {
855+
assertTypeIsNever(scope);
856+
}
857+
858+
return emptyArray;
859+
}
860+
861+
/**
862+
* If `scope` contains a function after `minPos`, then return the first such function.
863+
* Otherwise, return `undefined`.
864+
*/
865+
function getNodeToInsertBefore(minPos: number, scope: Scope): Node | undefined {
866+
const children = getStatementsOrClassElements(scope);
867+
for (const child of children) {
868+
if (child.pos >= minPos && isFunctionLike(child) && !isConstructorDeclaration(child)) {
869+
return child;
870+
}
871+
}
872+
}
873+
835874
function transformFunctionBody(body: Node) {
836875
if (isBlock(body) && !writes && substitutions.size === 0) {
837876
// already block, no writes to propagate back, no substitutions - can use node as is

‎tests/baselines/reference/extractMethod/extractMethod23.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// ==ORIGINAL==
2+
namespace NS {
3+
function M1() { }
4+
function M2() {
5+
return 1;
6+
}
7+
function M3() { }
8+
}
9+
// ==SCOPE::function 'M2'==
10+
namespace NS {
11+
function M1() { }
12+
function M2() {
13+
return newFunction();
14+
15+
function newFunction() {
16+
return 1;
17+
}
18+
}
19+
function M3() { }
20+
}
21+
// ==SCOPE::namespace 'NS'==
22+
namespace NS {
23+
function M1() { }
24+
function M2() {
25+
return newFunction();
26+
}
27+
function newFunction() {
28+
return 1;
29+
}
30+
31+
function M3() { }
32+
}
33+
// ==SCOPE::global scope==
34+
namespace NS {
35+
function M1() { }
36+
function M2() {
37+
return newFunction();
38+
}
39+
function M3() { }
40+
}
41+
function newFunction() {
42+
return 1;
43+
}

‎tests/baselines/reference/extractMethod/extractMethod24.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// ==ORIGINAL==
2+
function Outer() {
3+
function M1() { }
4+
function M2() {
5+
return 1;
6+
}
7+
function M3() { }
8+
}
9+
// ==SCOPE::function 'M2'==
10+
function Outer() {
11+
function M1() { }
12+
function M2() {
13+
return newFunction();
14+
15+
function newFunction() {
16+
return 1;
17+
}
18+
}
19+
function M3() { }
20+
}
21+
// ==SCOPE::function 'Outer'==
22+
function Outer() {
23+
function M1() { }
24+
function M2() {
25+
return newFunction();
26+
}
27+
function newFunction() {
28+
return 1;
29+
}
30+
31+
function M3() { }
32+
}
33+
// ==SCOPE::global scope==
34+
function Outer() {
35+
function M1() { }
36+
function M2() {
37+
return newFunction();
38+
}
39+
function M3() { }
40+
}
41+
function newFunction() {
42+
return 1;
43+
}

‎tests/baselines/reference/extractMethod/extractMethod25.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
// ==ORIGINAL==
2+
function M1() { }
3+
function M2() {
4+
return 1;
5+
}
6+
function M3() { }
7+
// ==SCOPE::function 'M2'==
8+
function M1() { }
9+
function M2() {
10+
return newFunction();
11+
12+
function newFunction() {
13+
return 1;
14+
}
15+
}
16+
function M3() { }
17+
// ==SCOPE::global scope==
18+
function M1() { }
19+
function M2() {
20+
return newFunction();
21+
}
22+
function newFunction() {
23+
return 1;
24+
}
25+
26+
function M3() { }

‎tests/baselines/reference/extractMethod/extractMethod26.ts‎

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

‎tests/baselines/reference/extractMethod/extractMethod27.ts‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// ==ORIGINAL==
2+
class C {
3+
M1() { }
4+
M2() {
5+
return 1;
6+
}
7+
constructor() { }
8+
M3() { }
9+
}
10+
// ==SCOPE::class 'C'==
11+
class C {
12+
M1() { }
13+
M2() {
14+
return this.newFunction();
15+
}
16+
constructor() { }
17+
private newFunction() {
18+
return 1;
19+
}
20+
21+
M3() { }
22+
}
23+
// ==SCOPE::global scope==
24+
class C {
25+
M1() { }
26+
M2() {
27+
return newFunction();
28+
}
29+
constructor() { }
30+
M3() { }
31+
}
32+
function newFunction() {
33+
return 1;
34+
}

‎tests/baselines/reference/extractMethod/extractMethod28.ts‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// ==ORIGINAL==
2+
class C {
3+
M1() { }
4+
M2() {
5+
return 1;
6+
}
7+
M3() { }
8+
constructor() { }
9+
}
10+
// ==SCOPE::class 'C'==
11+
class C {
12+
M1() { }
13+
M2() {
14+
return this.newFunction();
15+
}
16+
private newFunction() {
17+
return 1;
18+
}
19+
20+
M3() { }
21+
constructor() { }
22+
}
23+
// ==SCOPE::global scope==
24+
class C {
25+
M1() { }
26+
M2() {
27+
return newFunction();
28+
}
29+
M3() { }
30+
constructor() { }
31+
}
32+
function newFunction() {
33+
return 1;
34+
}

0 commit comments

Comments
 (0)