Skip to content

Commit aa10243

Browse files
author
Andy
authored
Fix insertNodeAtClassStart for empty class with comment (#23342)
1 parent 76e7d91 commit aa10243

16 files changed

+152
-46
lines changed

‎src/compiler/utilities.ts

+4-2
Original file line numberDiff line numberDiff line change
@@ -4025,12 +4025,14 @@ namespace ts {
40254025
}
40264026

40274027
/** Add a value to a set, and return true if it wasn't already present. */
4028-
export function addToSeen(seen: Map<true>, key: string | number): boolean {
4028+
export function addToSeen(seen: Map<true>, key: string | number): boolean;
4029+
export function addToSeen<T>(seen: Map<T>, key: string | number, value: T): boolean;
4030+
export function addToSeen<T>(seen: Map<T>, key: string | number, value: T = true as any): boolean {
40294031
key = String(key);
40304032
if (seen.has(key)) {
40314033
return false;
40324034
}
4033-
seen.set(key, true);
4035+
seen.set(key, value);
40344036
return true;
40354037
}
40364038

‎src/services/textChanges.ts

+27-24
Original file line numberDiff line numberDiff line change
@@ -212,8 +212,7 @@ namespace ts.textChanges {
212212
export class ChangeTracker {
213213
private readonly changes: Change[] = [];
214214
private readonly deletedNodesInLists: true[] = []; // Stores ids of nodes in lists that we already deleted. Used to avoid deleting `, ` twice in `a, b`.
215-
// Map from class id to nodes to insert at the start
216-
private readonly nodesInsertedAtClassStarts = createMap<{ sourceFile: SourceFile, cls: ClassLikeDeclaration, members: ClassElement[] }>();
215+
private readonly classesWithNodesInsertedAtStart = createMap<ClassDeclaration>(); // Set<ClassDeclaration> implemented as Map<node id, ClassDeclaration>
217216

218217
public static fromContext(context: TextChangesContext): ChangeTracker {
219218
return new ChangeTracker(getNewLineOrDefaultFromHost(context.host, context.formatContext.options), context.formatContext);
@@ -343,8 +342,7 @@ namespace ts.textChanges {
343342
}
344343

345344
public insertNodeBefore(sourceFile: SourceFile, before: Node, newNode: Node, blankLineBetween = false) {
346-
const pos = getAdjustedStartPosition(sourceFile, before, {}, Position.Start);
347-
return this.replaceRange(sourceFile, { pos, end: pos }, newNode, this.getOptionsForInsertNodeBefore(before, blankLineBetween));
345+
this.insertNodeAt(sourceFile, getAdjustedStartPosition(sourceFile, before, {}, Position.Start), newNode, this.getOptionsForInsertNodeBefore(before, blankLineBetween));
348346
}
349347

350348
public insertModifierBefore(sourceFile: SourceFile, modifier: SyntaxKind, before: Node): void {
@@ -443,21 +441,20 @@ namespace ts.textChanges {
443441
}
444442

445443
public insertNodeAtClassStart(sourceFile: SourceFile, cls: ClassLikeDeclaration, newElement: ClassElement): void {
446-
const firstMember = firstOrUndefined(cls.members);
447-
if (!firstMember) {
448-
const id = getNodeId(cls).toString();
449-
const newMembers = this.nodesInsertedAtClassStarts.get(id);
450-
if (newMembers) {
451-
Debug.assert(newMembers.sourceFile === sourceFile && newMembers.cls === cls);
452-
newMembers.members.push(newElement);
444+
const clsStart = cls.getStart(sourceFile);
445+
let prefix = "";
446+
let suffix = this.newLineCharacter;
447+
if (addToSeen(this.classesWithNodesInsertedAtStart, getNodeId(cls), cls)) {
448+
prefix = this.newLineCharacter;
449+
// For `class C {\n}`, don't add the trailing "\n"
450+
if (cls.members.length === 0 && !(positionsAreOnSameLine as any)(...getClassBraceEnds(cls, sourceFile), sourceFile)) { // TODO: GH#4130 remove 'as any'
451+
suffix = "";
453452
}
454-
else {
455-
this.nodesInsertedAtClassStarts.set(id, { sourceFile, cls, members: [newElement] });
456-
}
457-
}
458-
else {
459-
this.insertNodeBefore(sourceFile, firstMember, newElement);
460453
}
454+
455+
const indentation = formatting.SmartIndenter.findFirstNonWhitespaceColumn(getLineStartPositionForPosition(clsStart, sourceFile), clsStart, sourceFile, this.formatContext.options)
456+
+ this.formatContext.options.indentSize;
457+
this.insertNodeAt(sourceFile, cls.members.pos, newElement, { indentation, prefix, suffix });
461458
}
462459

463460
public insertNodeAfter(sourceFile: SourceFile, after: Node, newNode: Node): this {
@@ -638,12 +635,14 @@ namespace ts.textChanges {
638635
return this;
639636
}
640637

641-
private finishInsertNodeAtClassStart(): void {
642-
this.nodesInsertedAtClassStarts.forEach(({ sourceFile, cls, members }) => {
643-
const newCls = cls.kind === SyntaxKind.ClassDeclaration
644-
? updateClassDeclaration(cls, cls.decorators, cls.modifiers, cls.name, cls.typeParameters, cls.heritageClauses, members)
645-
: updateClassExpression(cls, cls.modifiers, cls.name, cls.typeParameters, cls.heritageClauses, members);
646-
this.replaceNode(sourceFile, cls, newCls);
638+
private finishClassesWithNodesInsertedAtStart(): void {
639+
this.classesWithNodesInsertedAtStart.forEach(cls => {
640+
const sourceFile = cls.getSourceFile();
641+
const [openBraceEnd, closeBraceEnd] = getClassBraceEnds(cls, sourceFile);
642+
// For `class C { }` remove the whitespace inside the braces.
643+
if (positionsAreOnSameLine(openBraceEnd, closeBraceEnd, sourceFile) && openBraceEnd !== closeBraceEnd - 1) {
644+
this.deleteRange(sourceFile, createTextRange(openBraceEnd, closeBraceEnd - 1));
645+
}
647646
});
648647
}
649648

@@ -654,11 +653,15 @@ namespace ts.textChanges {
654653
* so we can only call this once and can't get the non-formatted text separately.
655654
*/
656655
public getChanges(validate?: ValidateNonFormattedText): FileTextChanges[] {
657-
this.finishInsertNodeAtClassStart();
656+
this.finishClassesWithNodesInsertedAtStart();
658657
return changesToText.getTextChangesFromChanges(this.changes, this.newLineCharacter, this.formatContext, validate);
659658
}
660659
}
661660

661+
function getClassBraceEnds(cls: ClassLikeDeclaration, sourceFile: SourceFile): [number, number] {
662+
return [findChildOfKind(cls, SyntaxKind.OpenBraceToken, sourceFile).end, findChildOfKind(cls, SyntaxKind.CloseBraceToken, sourceFile).end];
663+
}
664+
662665
export type ValidateNonFormattedText = (node: Node, text: string) => void;
663666

664667
namespace changesToText {

‎tests/cases/fourslash/codeFixAddMissingMember.ts

+1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ verify.codeFix({
1111
index: 0,
1212
newFileContent: `class C {
1313
foo: number;
14+
1415
method() {
1516
this.foo = 10;
1617
}

‎tests/cases/fourslash/codeFixAddMissingMember2.ts

+1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ verify.codeFix({
1111
index: 1,
1212
newFileContent: `class C {
1313
[x: string]: number;
14+
1415
method() {
1516
this.foo = 10;
1617
}

‎tests/cases/fourslash/codeFixAddMissingMember3.ts

+1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ verify.codeFix({
1111
index: 0,
1212
newFileContent: `class C {
1313
static foo: number;
14+
1415
static method() {
1516
this.foo = 10;
1617
}

‎tests/cases/fourslash/codeFixAddMissingMember_all.ts

+1
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ verify.codeFixAll({
1717
y(): any {
1818
throw new Error("Method not implemented.");
1919
}
20+
2021
method() {
2122
this.x = 0;
2223
this.y();

‎tests/cases/fourslash/codeFixAddMissingMember_all_js.ts

+1
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ verify.codeFixAll({
2121
y() {
2222
throw new Error("Method not implemented.");
2323
}
24+
2425
constructor() {
2526
this.x = undefined;
2627
}

‎tests/cases/fourslash/codeFixClassExtendAbstractMethod_comment.ts

+24
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
////abstract class A {
4+
//// abstract m() : void;
5+
////}
6+
////
7+
////class B extends A {
8+
//// // comment
9+
////}
10+
11+
verify.codeFix({
12+
description: "Implement inherited abstract class",
13+
newFileContent:
14+
`abstract class A {
15+
abstract m() : void;
16+
}
17+
18+
class B extends A {
19+
m(): void {
20+
throw new Error("Method not implemented.");
21+
}
22+
// comment
23+
}`,
24+
});

‎tests/cases/fourslash/codeFixClassImplementClassMultipleSignatures2.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
//// method(a: string): Function;
77
//// method(a: string | number, b?: string | number): boolean | Function { return a + b as any; }
88
////}
9-
////class C implements A {[| |]}
9+
////class C implements A { }
1010

1111
verify.codeFix({
1212
description: "Implement interface 'A'",

‎tests/cases/fourslash/codeFixClassImplementInterfaceTypeParamInstantiateNumber.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/// <reference path='fourslash.ts' />
22

33
////interface I<T> { x: T; }
4-
////class C implements I<number> {[| |]}
4+
////class C implements I<number> { }
55

66
verify.codeFix({
77
description: "Implement interface 'I<number>'",

‎tests/cases/fourslash/codeFixUndeclaredInStaticMethod.ts

+48-10
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/// <reference path='fourslash.ts' />
22

3-
//// class A {[|
4-
//// |]static foo0() {
3+
//// class A {
4+
//// static foo0() {
55
//// this.m1(1,2,3);
66
//// A.m2(1,2);
77
//// this.prop1 = 10;
@@ -12,51 +12,89 @@
1212
verify.codeFix({
1313
description: "Declare static method 'm1'",
1414
index: 0,
15-
newRangeContent: `
15+
newFileContent:
16+
`class A {
1617
static m1(arg0: any, arg1: any, arg2: any): any {
1718
throw new Error("Method not implemented.");
1819
}
19-
`,
20+
21+
static foo0() {
22+
this.m1(1,2,3);
23+
A.m2(1,2);
24+
this.prop1 = 10;
25+
A.prop2 = "asdf";
26+
}
27+
}`,
2028
});
2129

2230
verify.codeFix({
2331
description: "Declare static method 'm2'",
2432
index: 0,
25-
newRangeContent: `
33+
newFileContent:
34+
`class A {
2635
static m2(arg0: any, arg1: any): any {
2736
throw new Error("Method not implemented.");
2837
}
38+
2939
static m1(arg0: any, arg1: any, arg2: any): any {
3040
throw new Error("Method not implemented.");
3141
}
32-
`,
42+
43+
static foo0() {
44+
this.m1(1,2,3);
45+
A.m2(1,2);
46+
this.prop1 = 10;
47+
A.prop2 = "asdf";
48+
}
49+
}`,
3350
});
3451

3552
verify.codeFix({
3653
description: "Declare static property 'prop1'",
3754
index: 0,
38-
newRangeContent: `
55+
newFileContent:
56+
`class A {
3957
static prop1: number;
58+
4059
static m2(arg0: any, arg1: any): any {
4160
throw new Error("Method not implemented.");
4261
}
62+
4363
static m1(arg0: any, arg1: any, arg2: any): any {
4464
throw new Error("Method not implemented.");
4565
}
46-
`,
66+
67+
static foo0() {
68+
this.m1(1,2,3);
69+
A.m2(1,2);
70+
this.prop1 = 10;
71+
A.prop2 = "asdf";
72+
}
73+
}`,
4774
});
4875

4976
verify.codeFix({
5077
description: "Declare static property 'prop2'",
5178
index: 1, // fix at index 0 is to change the spelling to 'prop1'
52-
newRangeContent: `
79+
newFileContent:
80+
`class A {
5381
static prop2: string;
82+
5483
static prop1: number;
84+
5585
static m2(arg0: any, arg1: any): any {
5686
throw new Error("Method not implemented.");
5787
}
88+
5889
static m1(arg0: any, arg1: any, arg2: any): any {
5990
throw new Error("Method not implemented.");
6091
}
61-
`,
92+
93+
static foo0() {
94+
this.m1(1,2,3);
95+
A.m2(1,2);
96+
this.prop1 = 10;
97+
A.prop2 = "asdf";
98+
}
99+
}`,
62100
});

‎tests/cases/fourslash/codeFixUndeclaredMethod.ts

+38-8
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/// <reference path='fourslash.ts' />
22

3-
//// class A {[|
4-
//// |]constructor() {
3+
//// class A {
4+
//// constructor() {
55
//// this.foo1(1,2,3);
66
//// // 7 type args
77
//// this.foo2<1,2,3,4,5,6,7>();
@@ -13,38 +13,68 @@
1313
verify.codeFix({
1414
description: "Declare method 'foo1'",
1515
index: 0,
16-
newRangeContent: `
16+
newFileContent:
17+
`class A {
1718
foo1(arg0: any, arg1: any, arg2: any): any {
1819
throw new Error("Method not implemented.");
1920
}
20-
`,
21+
22+
constructor() {
23+
this.foo1(1,2,3);
24+
// 7 type args
25+
this.foo2<1,2,3,4,5,6,7>();
26+
// 8 type args
27+
this.foo3<1,2,3,4,5,6,7,8>();
28+
}
29+
}`,
2130
});
2231

2332
verify.codeFix({
2433
description: "Declare method 'foo2'",
2534
index: 0,
26-
newRangeContent: `
35+
newFileContent:
36+
`class A {
2737
foo2<T, U, V, W, X, Y, Z>(): any {
2838
throw new Error("Method not implemented.");
2939
}
40+
3041
foo1(arg0: any, arg1: any, arg2: any): any {
3142
throw new Error("Method not implemented.");
3243
}
33-
`
44+
45+
constructor() {
46+
this.foo1(1,2,3);
47+
// 7 type args
48+
this.foo2<1,2,3,4,5,6,7>();
49+
// 8 type args
50+
this.foo3<1,2,3,4,5,6,7,8>();
51+
}
52+
}`
3453
});
3554

3655
verify.codeFix({
3756
description: "Declare method 'foo3'",
3857
index: 0,
39-
newRangeContent:`
58+
newFileContent:
59+
`class A {
4060
foo3<T0, T1, T2, T3, T4, T5, T6, T7>(): any {
4161
throw new Error("Method not implemented.");
4262
}
63+
4364
foo2<T, U, V, W, X, Y, Z>(): any {
4465
throw new Error("Method not implemented.");
4566
}
67+
4668
foo1(arg0: any, arg1: any, arg2: any): any {
4769
throw new Error("Method not implemented.");
4870
}
49-
`
71+
72+
constructor() {
73+
this.foo1(1,2,3);
74+
// 7 type args
75+
this.foo2<1,2,3,4,5,6,7>();
76+
// 8 type args
77+
this.foo3<1,2,3,4,5,6,7,8>();
78+
}
79+
}`
5080
});

‎tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess16.ts

+1
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ edit.applyRefactor({
1717
public set a(value: string) {
1818
this._a = value;
1919
}
20+
2021
constructor() { }
2122
}`,
2223
});

0 commit comments

Comments
 (0)