Skip to content

Commit e64f04b

Browse files
gabrittogabritto
authored
Fix modifier order for class member completions (microsoft#48066)
* fix modifier order & tests * remove empty replacement span from tests
1 parent e4fe50c commit e64f04b

14 files changed

+95
-201
lines changed

‎src/services/completions.ts

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -768,7 +768,7 @@ namespace ts.Completions {
768768
completionKind === CompletionKind.MemberLike &&
769769
isClassLikeMemberCompletion(symbol, location)) {
770770
let importAdder;
771-
({ insertText, isSnippet, importAdder } = getEntryForMemberCompletion(host, program, options, preferences, name, symbol, location, contextToken, formatContext));
771+
({ insertText, isSnippet, importAdder, replacementSpan } = getEntryForMemberCompletion(host, program, options, preferences, name, symbol, location, contextToken, formatContext));
772772
if (importAdder?.hasFixes()) {
773773
hasAction = true;
774774
source = CompletionSource.ClassMemberSnippet;
@@ -894,13 +894,14 @@ namespace ts.Completions {
894894
location: Node,
895895
contextToken: Node | undefined,
896896
formatContext: formatting.FormatContext | undefined,
897-
): { insertText: string, isSnippet?: true, importAdder?: codefix.ImportAdder } {
897+
): { insertText: string, isSnippet?: true, importAdder?: codefix.ImportAdder, replacementSpan?: TextSpan } {
898898
const classLikeDeclaration = findAncestor(location, isClassLike);
899899
if (!classLikeDeclaration) {
900900
return { insertText: name };
901901
}
902902

903903
let isSnippet: true | undefined;
904+
let replacementSpan: TextSpan | undefined;
904905
let insertText: string = name;
905906

906907
const checker = program.getTypeChecker();
@@ -932,10 +933,8 @@ namespace ts.Completions {
932933
let modifiers = ModifierFlags.None;
933934
// Whether the suggested member should be abstract.
934935
// e.g. in `abstract class C { abstract | }`, we should offer abstract method signatures at position `|`.
935-
// Note: We are relying on checking if the context token is `abstract`,
936-
// since other visibility modifiers (e.g. `protected`) should come *before* `abstract`.
937-
// However, that is not true for the e.g. `override` modifier, so this check has its limitations.
938-
const isAbstract = contextToken && isModifierLike(contextToken) === SyntaxKind.AbstractKeyword;
936+
const { modifiers: presentModifiers, span: modifiersSpan } = getPresentModifiers(contextToken);
937+
const isAbstract = !!(presentModifiers & ModifierFlags.Abstract);
939938
const completionNodes: Node[] = [];
940939
codefix.addNewNodeForMemberSymbol(
941940
symbol,
@@ -961,26 +960,22 @@ namespace ts.Completions {
961960
requiredModifiers |= ModifierFlags.Override;
962961
}
963962

964-
let presentModifiers = ModifierFlags.None;
965963
if (!completionNodes.length) {
966-
// Omit already present modifiers from the first completion node/signature.
967-
if (contextToken) {
968-
presentModifiers = getPresentModifiers(contextToken);
969-
}
970964
// Keep track of added missing required modifiers and modifiers already present.
971965
// This is needed when we have overloaded signatures,
972966
// so this callback will be called for multiple nodes/signatures,
973967
// and we need to make sure the modifiers are uniform for all nodes/signatures.
974968
modifiers = node.modifierFlagsCache | requiredModifiers | presentModifiers;
975969
}
976-
node = factory.updateModifiers(node, modifiers & (~presentModifiers));
970+
node = factory.updateModifiers(node, modifiers);
977971
completionNodes.push(node);
978972
},
979973
body,
980974
codefix.PreserveOptionalFlags.Property,
981975
isAbstract);
982976

983977
if (completionNodes.length) {
978+
replacementSpan = modifiersSpan;
984979
// If we have access to formatting settings, we print the nodes using the emitter,
985980
// and then format the printed text.
986981
if (formatContext) {
@@ -1015,11 +1010,15 @@ namespace ts.Completions {
10151010
}
10161011
}
10171012

1018-
return { insertText, isSnippet, importAdder };
1013+
return { insertText, isSnippet, importAdder, replacementSpan };
10191014
}
10201015

1021-
function getPresentModifiers(contextToken: Node): ModifierFlags {
1016+
function getPresentModifiers(contextToken: Node | undefined): { modifiers: ModifierFlags, span?: TextSpan } {
1017+
if (!contextToken) {
1018+
return { modifiers: ModifierFlags.None };
1019+
}
10221020
let modifiers = ModifierFlags.None;
1021+
let span;
10231022
let contextMod;
10241023
/*
10251024
Cases supported:
@@ -1042,11 +1041,13 @@ namespace ts.Completions {
10421041
*/
10431042
if (contextMod = isModifierLike(contextToken)) {
10441043
modifiers |= modifierToFlag(contextMod);
1044+
span = createTextSpanFromNode(contextToken);
10451045
}
10461046
if (isPropertyDeclaration(contextToken.parent)) {
10471047
modifiers |= modifiersToFlags(contextToken.parent.modifiers);
1048+
span = createTextSpanFromNode(contextToken.parent);
10481049
}
1049-
return modifiers;
1050+
return { modifiers, span };
10501051
}
10511052

10521053
function isModifierLike(node: Node): ModifierSyntaxKind | undefined {

‎tests/cases/fourslash/completionsOverridingMethod.ts

Lines changed: 3 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@
9393
////
9494
////class HSub extends HBase {
9595
//// /*h1*/
96-
//// static /*h2*/
96+
//// [|static|] /*h2*/
9797
////}
9898

9999
// @Filename: i.ts
@@ -124,11 +124,6 @@ verify.completions({
124124
{
125125
name: "foo",
126126
sortText: completion.SortText.LocationPriority,
127-
replacementSpan: {
128-
fileName: "",
129-
pos: 0,
130-
end: 0,
131-
},
132127
insertText: "foo(param1: string, param2: boolean): Promise<void> {\n}",
133128
}
134129
],
@@ -146,11 +141,6 @@ verify.completions({
146141
{
147142
name: "foo",
148143
sortText: completion.SortText.LocationPriority,
149-
replacementSpan: {
150-
fileName: "",
151-
pos: 0,
152-
end: 0,
153-
},
154144
insertText: "foo(a: string, b: string): string {\n}",
155145
}
156146
],
@@ -168,11 +158,6 @@ verify.completions({
168158
{
169159
name: "foo",
170160
sortText: completion.SortText.LocationPriority,
171-
replacementSpan: {
172-
fileName: "",
173-
pos: 0,
174-
end: 0,
175-
},
176161
insertText: "foo(a: string): string {\n}",
177162
}
178163
],
@@ -190,11 +175,6 @@ verify.completions({
190175
{
191176
name: "foo",
192177
sortText: completion.SortText.LocationPriority,
193-
replacementSpan: {
194-
fileName: "",
195-
pos: 0,
196-
end: 0,
197-
},
198178
insertText: "foo(a: string): string {\n}",
199179
}
200180
],
@@ -212,11 +192,6 @@ verify.completions({
212192
{
213193
name: "foo",
214194
sortText: completion.SortText.LocationPriority,
215-
replacementSpan: {
216-
fileName: "",
217-
pos: 0,
218-
end: 0,
219-
},
220195
insertText: "foo(a: string): string {\n}",
221196
}
222197
],
@@ -234,11 +209,6 @@ verify.completions({
234209
{
235210
name: "foo",
236211
sortText: completion.SortText.LocationPriority,
237-
replacementSpan: {
238-
fileName: "",
239-
pos: 0,
240-
end: 0,
241-
},
242212
insertText: "foo(a: string): string {\n}",
243213
}
244214
],
@@ -256,11 +226,6 @@ verify.completions({
256226
{
257227
name: "foo",
258228
sortText: completion.SortText.LocationPriority,
259-
replacementSpan: {
260-
fileName: "",
261-
pos: 0,
262-
end: 0,
263-
},
264229
insertText:
265230
`foo(a: string): string;
266231
foo(a: undefined, b: number): string;
@@ -292,12 +257,8 @@ verify.completions({
292257
{
293258
name: "met",
294259
sortText: completion.SortText.LocationPriority,
295-
replacementSpan: {
296-
fileName: "",
297-
pos: 0,
298-
end: 0,
299-
},
300-
insertText: "met(n: number): number {\n}",
260+
replacementSpan: test.ranges()[0],
261+
insertText: "static met(n: number): number {\n}",
301262
}
302263
],
303264
});
@@ -314,21 +275,11 @@ verify.completions({
314275
{
315276
name: "met",
316277
sortText: completion.SortText.LocationPriority,
317-
replacementSpan: {
318-
fileName: "",
319-
pos: 0,
320-
end: 0,
321-
},
322278
insertText: "met<T>(t: T): T {\n}",
323279
},
324280
{
325281
name: "metcons",
326282
sortText: completion.SortText.LocationPriority,
327-
replacementSpan: {
328-
fileName: "",
329-
pos: 0,
330-
end: 0,
331-
},
332283
insertText: "metcons<T extends string | number>(t: T): T {\n}",
333284
}
334285
],

‎tests/cases/fourslash/completionsOverridingMethod1.ts

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,6 @@ verify.completions({
2525
{
2626
name: "foo",
2727
sortText: completion.SortText.LocationPriority,
28-
replacementSpan: {
29-
fileName: "",
30-
pos: 0,
31-
end: 0,
32-
},
3328
insertText: "override foo(a: string): void {\n}",
3429
}
3530
],

‎tests/cases/fourslash/completionsOverridingMethod10.ts

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -26,33 +26,18 @@ verify.completions({
2626
{
2727
name: "a",
2828
sortText: completion.SortText.LocationPriority,
29-
replacementSpan: {
30-
fileName: "",
31-
pos: 0,
32-
end: 0,
33-
},
3429
insertText: "a: string;",
3530
},
3631
{
3732
name: "b",
3833
sortText: completion.SortText.LocationPriority,
39-
replacementSpan: {
40-
fileName: "",
41-
pos: 0,
42-
end: 0,
43-
},
4434
insertText:
4535
`b(a: string): void {
4636
}`,
4737
},
4838
{
4939
name: "c",
5040
sortText: completion.SortText.LocationPriority,
51-
replacementSpan: {
52-
fileName: "",
53-
pos: 0,
54-
end: 0,
55-
},
5641
insertText:
5742
`c(a: string): string;
5843
c(a: number): number;

‎tests/cases/fourslash/completionsOverridingMethod11.ts

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -32,33 +32,18 @@ verify.completions({
3232
{
3333
name: "a",
3434
sortText: completion.SortText.LocationPriority,
35-
replacementSpan: {
36-
fileName: "",
37-
pos: 0,
38-
end: 0,
39-
},
4035
insertText: "a: string",
4136
},
4237
{
4338
name: "b",
4439
sortText: completion.SortText.LocationPriority,
45-
replacementSpan: {
46-
fileName: "",
47-
pos: 0,
48-
end: 0,
49-
},
5040
insertText:
5141
`b(a: string): void {
5242
}`,
5343
},
5444
{
5545
name: "c",
5646
sortText: completion.SortText.LocationPriority,
57-
replacementSpan: {
58-
fileName: "",
59-
pos: 0,
60-
end: 0,
61-
},
6247
insertText:
6348
`c(a: string): string
6449
c(a: number): number

‎tests/cases/fourslash/completionsOverridingMethod12.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @Filename: a.ts
4+
// @newline: LF
5+
// Case: modifier order
6+
////abstract class A {
7+
//// public get P(): string {
8+
//// return "";
9+
//// }
10+
////}
11+
////
12+
////abstract class B extends A {
13+
//// [|abstract|] /*a*/
14+
////}
15+
////
16+
////abstract class B1 extends A {
17+
//// [|abstract override|] /*b*/
18+
////}
19+
20+
verify.completions({
21+
marker: "a",
22+
isNewIdentifierLocation: true,
23+
preferences: {
24+
includeCompletionsWithInsertText: true,
25+
includeCompletionsWithSnippetText: false,
26+
includeCompletionsWithClassMemberSnippets: true,
27+
},
28+
includes: [
29+
{
30+
name: "P",
31+
sortText: completion.SortText.LocationPriority,
32+
replacementSpan: test.ranges()[0],
33+
insertText: "public abstract get P(): string;",
34+
},
35+
],
36+
});
37+
38+
verify.completions({
39+
marker: "b",
40+
isNewIdentifierLocation: true,
41+
preferences: {
42+
includeCompletionsWithInsertText: true,
43+
includeCompletionsWithSnippetText: false,
44+
includeCompletionsWithClassMemberSnippets: true,
45+
},
46+
includes: [
47+
{
48+
name: "P",
49+
sortText: completion.SortText.LocationPriority,
50+
replacementSpan: test.ranges()[1],
51+
insertText: "public abstract override get P(): string;",
52+
},
53+
],
54+
});

‎tests/cases/fourslash/completionsOverridingMethod2.ts

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,6 @@ verify.completions({
2323
{
2424
name: "$usd",
2525
sortText: completion.SortText.LocationPriority,
26-
replacementSpan: {
27-
fileName: "",
28-
pos: 0,
29-
end: 0,
30-
},
3126
isSnippet: true,
3227
insertText: "\"\\$usd\"(a: number): number {\n $0\n}",
3328
}

‎tests/cases/fourslash/completionsOverridingMethod3.ts

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,6 @@ verify.completions({
2424
{
2525
name: "boo",
2626
sortText: completion.SortText.LocationPriority,
27-
replacementSpan: {
28-
fileName: "",
29-
pos: 0,
30-
end: 0,
31-
},
3227
insertText: "boo(): string;",
3328
}
3429
],

0 commit comments

Comments
 (0)