Fix modifier order for class member completions (#48066)

* fix modifier order & tests * remove empty replacement span from tests
microsoft · Mar 1, 2022 · e64f04b · e64f04b
1 parent e4fe50c
commit e64f04b
Show file tree

Hide file tree

Showing 14 changed files with 95 additions and 201 deletions.
diff --git a/src/services/completions.ts b/src/services/completions.ts
@@ -768,7 +768,7 @@ namespace ts.Completions {
             completionKind === CompletionKind.MemberLike &&
             isClassLikeMemberCompletion(symbol, location)) {
             let importAdder;
-            ({ insertText, isSnippet, importAdder } = getEntryForMemberCompletion(host, program, options, preferences, name, symbol, location, contextToken, formatContext));
+            ({ insertText, isSnippet, importAdder, replacementSpan } = getEntryForMemberCompletion(host, program, options, preferences, name, symbol, location, contextToken, formatContext));
             if (importAdder?.hasFixes()) {
                 hasAction = true;
                 source = CompletionSource.ClassMemberSnippet;
@@ -894,13 +894,14 @@ namespace ts.Completions {
         location: Node,
         contextToken: Node | undefined,
         formatContext: formatting.FormatContext | undefined,
-    ): { insertText: string, isSnippet?: true, importAdder?: codefix.ImportAdder } {
+    ): { insertText: string, isSnippet?: true, importAdder?: codefix.ImportAdder, replacementSpan?: TextSpan } {
         const classLikeDeclaration = findAncestor(location, isClassLike);
         if (!classLikeDeclaration) {
             return { insertText: name };
         }
 
         let isSnippet: true | undefined;
+        let replacementSpan: TextSpan | undefined;
         let insertText: string = name;
 
         const checker = program.getTypeChecker();
@@ -932,10 +933,8 @@ namespace ts.Completions {
         let modifiers = ModifierFlags.None;
         // Whether the suggested member should be abstract.
         // e.g. in `abstract class C { abstract | }`, we should offer abstract method signatures at position `|`.
-        // Note: We are relying on checking if the context token is `abstract`,
-        // since other visibility modifiers (e.g. `protected`) should come *before* `abstract`.
-        // However, that is not true for the e.g. `override` modifier, so this check has its limitations.
-        const isAbstract = contextToken && isModifierLike(contextToken) === SyntaxKind.AbstractKeyword;
+        const { modifiers: presentModifiers, span: modifiersSpan } = getPresentModifiers(contextToken);
+        const isAbstract = !!(presentModifiers & ModifierFlags.Abstract);
         const completionNodes: Node[] = [];
         codefix.addNewNodeForMemberSymbol(
             symbol,
@@ -961,26 +960,22 @@ namespace ts.Completions {
                     requiredModifiers |= ModifierFlags.Override;
                 }
 
-                let presentModifiers = ModifierFlags.None;
                 if (!completionNodes.length) {
-                    // Omit already present modifiers from the first completion node/signature.
-                    if (contextToken) {
-                        presentModifiers = getPresentModifiers(contextToken);
-                    }
                     // Keep track of added missing required modifiers and modifiers already present.
                     // This is needed when we have overloaded signatures,
                     // so this callback will be called for multiple nodes/signatures,
                     // and we need to make sure the modifiers are uniform for all nodes/signatures.
                     modifiers = node.modifierFlagsCache | requiredModifiers | presentModifiers;
                 }
-                node = factory.updateModifiers(node, modifiers & (~presentModifiers));
+                node = factory.updateModifiers(node, modifiers);
                 completionNodes.push(node);
             },
             body,
             codefix.PreserveOptionalFlags.Property,
             isAbstract);
 
         if (completionNodes.length) {
+            replacementSpan = modifiersSpan;
              // If we have access to formatting settings, we print the nodes using the emitter,
              // and then format the printed text.
             if (formatContext) {
@@ -1015,11 +1010,15 @@ namespace ts.Completions {
             }
         }
 
-        return { insertText, isSnippet, importAdder };
+        return { insertText, isSnippet, importAdder, replacementSpan };
     }
 
-    function getPresentModifiers(contextToken: Node): ModifierFlags {
+    function getPresentModifiers(contextToken: Node | undefined): { modifiers: ModifierFlags, span?: TextSpan } {
+        if (!contextToken) {
+            return { modifiers: ModifierFlags.None };
+        }
         let modifiers = ModifierFlags.None;
+        let span;
         let contextMod;
         /*
         Cases supported:
@@ -1042,11 +1041,13 @@ namespace ts.Completions {
         */
         if (contextMod = isModifierLike(contextToken)) {
             modifiers |= modifierToFlag(contextMod);
+            span = createTextSpanFromNode(contextToken);
         }
         if (isPropertyDeclaration(contextToken.parent)) {
             modifiers |= modifiersToFlags(contextToken.parent.modifiers);
+            span = createTextSpanFromNode(contextToken.parent);
         }
-        return modifiers;
+        return { modifiers, span };
     }
 
     function isModifierLike(node: Node): ModifierSyntaxKind | undefined {

diff --git a/tests/cases/fourslash/completionsOverridingMethod.ts b/tests/cases/fourslash/completionsOverridingMethod.ts
@@ -93,7 +93,7 @@
 ////
 ////class HSub extends HBase {
 ////    /*h1*/
-////    static /*h2*/
+////    [|static|] /*h2*/
 ////}
 
 // @Filename: i.ts
@@ -124,11 +124,6 @@ verify.completions({
         {
             name: "foo",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText: "foo(param1: string, param2: boolean): Promise<void> {\n}",
         }
     ],
@@ -146,11 +141,6 @@ verify.completions({
         {
             name: "foo",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText: "foo(a: string, b: string): string {\n}",
         }
     ],
@@ -168,11 +158,6 @@ verify.completions({
         {
             name: "foo",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText: "foo(a: string): string {\n}",
         }
     ],
@@ -190,11 +175,6 @@ verify.completions({
         {
             name: "foo",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText: "foo(a: string): string {\n}",
         }
     ],
@@ -212,11 +192,6 @@ verify.completions({
         {
             name: "foo",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText: "foo(a: string): string {\n}",
         }
     ],
@@ -234,11 +209,6 @@ verify.completions({
         {
             name: "foo",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText: "foo(a: string): string {\n}",
         }
     ],
@@ -256,11 +226,6 @@ verify.completions({
         {
             name: "foo",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText:
 `foo(a: string): string;
 foo(a: undefined, b: number): string;
@@ -292,12 +257,8 @@ verify.completions({
         {
             name: "met",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
-            insertText: "met(n: number): number {\n}",
+            replacementSpan: test.ranges()[0],
+            insertText: "static met(n: number): number {\n}",
         }
     ],
 });
@@ -314,21 +275,11 @@ verify.completions({
         {
             name: "met",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText: "met<T>(t: T): T {\n}",
         },
         {
             name: "metcons",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText: "metcons<T extends string | number>(t: T): T {\n}",
         }
     ],

diff --git a/tests/cases/fourslash/completionsOverridingMethod1.ts b/tests/cases/fourslash/completionsOverridingMethod1.ts
@@ -25,11 +25,6 @@ verify.completions({
         {
             name: "foo",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText: "override foo(a: string): void {\n}",
         }
     ],

diff --git a/tests/cases/fourslash/completionsOverridingMethod10.ts b/tests/cases/fourslash/completionsOverridingMethod10.ts
@@ -26,33 +26,18 @@ verify.completions({
         {
             name: "a",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText: "a: string;",
         },
         {
             name: "b",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText:
 `b(a: string): void {
 }`,
         },
         {
             name: "c",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText:
 `c(a: string): string;
 c(a: number): number;

diff --git a/tests/cases/fourslash/completionsOverridingMethod11.ts b/tests/cases/fourslash/completionsOverridingMethod11.ts
@@ -32,33 +32,18 @@ verify.completions({
         {
             name: "a",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText: "a: string",
         },
         {
             name: "b",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText:
 `b(a: string): void {
 }`,
         },
         {
             name: "c",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText:
 `c(a: string): string
 c(a: number): number

diff --git a/tests/cases/fourslash/completionsOverridingMethod12.ts b/tests/cases/fourslash/completionsOverridingMethod12.ts
@@ -0,0 +1,54 @@
+/// <reference path="fourslash.ts" />
+
+// @Filename: a.ts
+// @newline: LF
+// Case: modifier order
+////abstract class A {
+////    public get P(): string {
+////        return "";
+////    }
+////}
+////
+////abstract class B extends A {
+////    [|abstract|] /*a*/
+////}
+////
+////abstract class B1 extends A {
+////    [|abstract override|] /*b*/
+////}
+
+verify.completions({
+    marker: "a",
+    isNewIdentifierLocation: true,
+    preferences: {
+        includeCompletionsWithInsertText: true,
+        includeCompletionsWithSnippetText: false,
+        includeCompletionsWithClassMemberSnippets: true,
+    },
+    includes: [
+        {
+            name: "P",
+            sortText: completion.SortText.LocationPriority,
+            replacementSpan: test.ranges()[0],
+            insertText: "public abstract get P(): string;",
+        },
+    ],
+});
+
+verify.completions({
+    marker: "b",
+    isNewIdentifierLocation: true,
+    preferences: {
+        includeCompletionsWithInsertText: true,
+        includeCompletionsWithSnippetText: false,
+        includeCompletionsWithClassMemberSnippets: true,
+    },
+    includes: [
+        {
+            name: "P",
+            sortText: completion.SortText.LocationPriority,
+            replacementSpan: test.ranges()[1],
+            insertText: "public abstract override get P(): string;",
+        },
+    ],
+});
diff --git a/tests/cases/fourslash/completionsOverridingMethod2.ts b/tests/cases/fourslash/completionsOverridingMethod2.ts
@@ -23,11 +23,6 @@ verify.completions({
         {
             name: "$usd",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             isSnippet: true,
             insertText: "\"\\$usd\"(a: number): number {\n    $0\n}",
         }

diff --git a/tests/cases/fourslash/completionsOverridingMethod3.ts b/tests/cases/fourslash/completionsOverridingMethod3.ts
@@ -24,11 +24,6 @@ verify.completions({
         {
             name: "boo",
             sortText: completion.SortText.LocationPriority,
-            replacementSpan: {
-                fileName: "",
-                pos: 0,
-                end: 0,
-            },
             insertText: "boo(): string;",
         }
     ],