Allow quoted names in completions (#18162)

* Allow quoted names in completions

* Don't allow string literal completions if not in an object literal; and use string literals for number keys

* Add TODO

Author: Andy

src/harness/fourslash.ts

@@ -762,14 +762,18 @@ namespace FourSlash {
             }
         }
 
-        public verifyCompletionsAt(markerName: string, expected: string[]) {
+        public verifyCompletionsAt(markerName: string, expected: string[], options?: FourSlashInterface.CompletionsAtOptions) {
             this.goToMarker(markerName);
 
             const actualCompletions = this.getCompletionListAtCaret();
             if (!actualCompletions) {
                 this.raiseError(`No completions at position '${this.currentCaretPosition}'.`);
             }
 
+            if (options && options.isNewIdentifierLocation !== undefined && actualCompletions.isNewIdentifierLocation !== options.isNewIdentifierLocation) {
+                this.raiseError(`Expected 'isNewIdentifierLocation' to be ${options.isNewIdentifierLocation}, got ${actualCompletions.isNewIdentifierLocation}`);
+            }
+
             const actual = actualCompletions.entries;
 
             if (actual.length !== expected.length) {
@@ -3705,8 +3709,8 @@ namespace FourSlashInterface {
             super(state);
         }
 
-        public completionsAt(markerName: string, completions: string[]) {
-            this.state.verifyCompletionsAt(markerName, completions);
+        public completionsAt(markerName: string, completions: string[], options?: CompletionsAtOptions) {
+            this.state.verifyCompletionsAt(markerName, completions, options);
         }
 
         public quickInfoIs(expectedText: string, expectedDocumentation?: string) {
@@ -4314,4 +4318,8 @@ namespace FourSlashInterface {
         actionName: string;
         actionDescription: string;
     }
+
+    export interface CompletionsAtOptions {
+        isNewIdentifierLocation?: boolean;
+    }
 }

src/services/completions.ts

@@ -24,7 +24,7 @@ namespace ts.Completions {
             return undefined;
         }
 
-        const { symbols, isGlobalCompletion, isMemberCompletion, isNewIdentifierLocation, location, request, keywordFilters } = completionData;
+        const { symbols, isGlobalCompletion, isMemberCompletion, allowStringLiteral, isNewIdentifierLocation, location, request, keywordFilters } = completionData;
 
         if (sourceFile.languageVariant === LanguageVariant.JSX &&
             location && location.parent && location.parent.kind === SyntaxKind.JsxClosingElement) {
@@ -56,15 +56,15 @@ namespace ts.Completions {
         const entries: CompletionEntry[] = [];
 
         if (isSourceFileJavaScript(sourceFile)) {
-            const uniqueNames = getCompletionEntriesFromSymbols(symbols, entries, location, /*performCharacterChecks*/ true, typeChecker, compilerOptions.target, log);
+            const uniqueNames = getCompletionEntriesFromSymbols(symbols, entries, location, /*performCharacterChecks*/ true, typeChecker, compilerOptions.target, log, allowStringLiteral);
             getJavaScriptCompletionEntries(sourceFile, location.pos, uniqueNames, compilerOptions.target, entries);
         }
         else {
             if ((!symbols || symbols.length === 0) && keywordFilters === KeywordCompletionFilters.None) {
                 return undefined;
             }
 
-            getCompletionEntriesFromSymbols(symbols, entries, location, /*performCharacterChecks*/ true, typeChecker, compilerOptions.target, log);
+            getCompletionEntriesFromSymbols(symbols, entries, location, /*performCharacterChecks*/ true, typeChecker, compilerOptions.target, log, allowStringLiteral);
         }
 
         // TODO add filter for keyword based on type/value/namespace and also location
@@ -97,7 +97,7 @@ namespace ts.Completions {
             }
 
             uniqueNames.set(realName, true);
-            const displayName = getCompletionEntryDisplayName(realName, target, /*performCharacterChecks*/ true);
+            const displayName = getCompletionEntryDisplayName(realName, target, /*performCharacterChecks*/ true, /*allowStringLiteral*/ false);
             if (displayName) {
                 entries.push({
                     name: displayName,
@@ -109,11 +109,11 @@ namespace ts.Completions {
         });
     }
 
-    function createCompletionEntry(symbol: Symbol, location: Node, performCharacterChecks: boolean, typeChecker: TypeChecker, target: ScriptTarget): CompletionEntry {
+    function createCompletionEntry(symbol: Symbol, location: Node, performCharacterChecks: boolean, typeChecker: TypeChecker, target: ScriptTarget, allowStringLiteral: boolean): CompletionEntry {
         // Try to get a valid display name for this symbol, if we could not find one, then ignore it.
         // We would like to only show things that can be added after a dot, so for instance numeric properties can
         // not be accessed with a dot (a.1 <- invalid)
-        const displayName = getCompletionEntryDisplayNameForSymbol(symbol, target, performCharacterChecks);
+        const displayName = getCompletionEntryDisplayNameForSymbol(symbol, target, performCharacterChecks, allowStringLiteral);
         if (!displayName) {
             return undefined;
         }
@@ -134,12 +134,12 @@ namespace ts.Completions {
         };
     }
 
-    function getCompletionEntriesFromSymbols(symbols: Symbol[], entries: Push<CompletionEntry>, location: Node, performCharacterChecks: boolean, typeChecker: TypeChecker, target: ScriptTarget, log: Log): Map<true> {
+    function getCompletionEntriesFromSymbols(symbols: Symbol[], entries: Push<CompletionEntry>, location: Node, performCharacterChecks: boolean, typeChecker: TypeChecker, target: ScriptTarget, log: Log, allowStringLiteral: boolean): Map<true> {
         const start = timestamp();
         const uniqueNames = createMap<true>();
         if (symbols) {
             for (const symbol of symbols) {
-                const entry = createCompletionEntry(symbol, location, performCharacterChecks, typeChecker, target);
+                const entry = createCompletionEntry(symbol, location, performCharacterChecks, typeChecker, target, allowStringLiteral);
                 if (entry) {
                     const id = entry.name;
                     if (!uniqueNames.has(id)) {
@@ -224,7 +224,7 @@ namespace ts.Completions {
         const type = typeChecker.getContextualType((<ObjectLiteralExpression>element.parent));
         const entries: CompletionEntry[] = [];
         if (type) {
-            getCompletionEntriesFromSymbols(type.getApparentProperties(), entries, element, /*performCharacterChecks*/ false, typeChecker, target, log);
+            getCompletionEntriesFromSymbols(type.getApparentProperties(), entries, element, /*performCharacterChecks*/ false, typeChecker, target, log, /*allowStringLiteral*/ true);
             if (entries.length) {
                 return { isGlobalCompletion: false, isMemberCompletion: true, isNewIdentifierLocation: true, entries };
             }
@@ -253,7 +253,7 @@ namespace ts.Completions {
         const type = typeChecker.getTypeAtLocation(node.expression);
         const entries: CompletionEntry[] = [];
         if (type) {
-            getCompletionEntriesFromSymbols(type.getApparentProperties(), entries, node, /*performCharacterChecks*/ false, typeChecker, target, log);
+            getCompletionEntriesFromSymbols(type.getApparentProperties(), entries, node, /*performCharacterChecks*/ false, typeChecker, target, log, /*allowStringLiteral*/ true);
             if (entries.length) {
                 return { isGlobalCompletion: false, isMemberCompletion: true, isNewIdentifierLocation: true, entries };
             }
@@ -302,13 +302,13 @@ namespace ts.Completions {
         // Compute all the completion symbols again.
         const completionData = getCompletionData(typeChecker, log, sourceFile, position);
         if (completionData) {
-            const { symbols, location } = completionData;
+            const { symbols, location, allowStringLiteral } = completionData;
 
             // Find the symbol with the matching entry name.
             // We don't need to perform character checks here because we're only comparing the
             // name against 'entryName' (which is known to be good), not building a new
             // completion entry.
-            const symbol = forEach(symbols, s => getCompletionEntryDisplayNameForSymbol(s, compilerOptions.target, /*performCharacterChecks*/ false) === entryName ? s : undefined);
+            const symbol = forEach(symbols, s => getCompletionEntryDisplayNameForSymbol(s, compilerOptions.target, /*performCharacterChecks*/ false, allowStringLiteral) === entryName ? s : undefined);
 
             if (symbol) {
                 const { displayParts, documentation, symbolKind, tags } = SymbolDisplay.getSymbolDisplayPartsDocumentationAndSymbolKind(typeChecker, symbol, sourceFile, location, location, SemanticMeaning.All);
@@ -345,17 +345,22 @@ namespace ts.Completions {
     export function getCompletionEntrySymbol(typeChecker: TypeChecker, log: (message: string) => void, compilerOptions: CompilerOptions, sourceFile: SourceFile, position: number, entryName: string): Symbol | undefined {
         // Compute all the completion symbols again.
         const completionData = getCompletionData(typeChecker, log, sourceFile, position);
+        if (!completionData) {
+            return undefined;
+        }
+        const { symbols, allowStringLiteral } = completionData;
         // Find the symbol with the matching entry name.
         // We don't need to perform character checks here because we're only comparing the
         // name against 'entryName' (which is known to be good), not building a new
         // completion entry.
-        return completionData && forEach(completionData.symbols, s => getCompletionEntryDisplayNameForSymbol(s, compilerOptions.target, /*performCharacterChecks*/ false) === entryName ? s : undefined);
+        return forEach(symbols, s => getCompletionEntryDisplayNameForSymbol(s, compilerOptions.target, /*performCharacterChecks*/ false, allowStringLiteral) === entryName ? s : undefined);
     }
 
     interface CompletionData {
         symbols: Symbol[];
         isGlobalCompletion: boolean;
         isMemberCompletion: boolean;
+        allowStringLiteral: boolean;
         isNewIdentifierLocation: boolean;
         location: Node;
         isRightOfDot: boolean;
@@ -436,7 +441,7 @@ namespace ts.Completions {
             }
 
             if (request) {
-                return { symbols: undefined, isGlobalCompletion: false, isMemberCompletion: false, isNewIdentifierLocation: false, location: undefined, isRightOfDot: false, request, keywordFilters: KeywordCompletionFilters.None };
+                return { symbols: undefined, isGlobalCompletion: false, isMemberCompletion: false, allowStringLiteral: false, isNewIdentifierLocation: false, location: undefined, isRightOfDot: false, request, keywordFilters: KeywordCompletionFilters.None };
             }
 
             if (!insideJsDocTagTypeExpression) {
@@ -534,6 +539,7 @@ namespace ts.Completions {
         const semanticStart = timestamp();
         let isGlobalCompletion = false;
         let isMemberCompletion: boolean;
+        let allowStringLiteral = false;
         let isNewIdentifierLocation: boolean;
         let keywordFilters = KeywordCompletionFilters.None;
         let symbols: Symbol[] = [];
@@ -573,7 +579,7 @@ namespace ts.Completions {
 
         log("getCompletionData: Semantic work: " + (timestamp() - semanticStart));
 
-        return { symbols, isGlobalCompletion, isMemberCompletion, isNewIdentifierLocation, location, isRightOfDot: (isRightOfDot || isRightOfOpenTag), request, keywordFilters };
+        return { symbols, isGlobalCompletion, isMemberCompletion, allowStringLiteral, isNewIdentifierLocation, location, isRightOfDot: (isRightOfDot || isRightOfOpenTag), request, keywordFilters };
 
         type JSDocTagWithTypeExpression = JSDocAugmentsTag | JSDocParameterTag | JSDocPropertyTag | JSDocReturnTag | JSDocTypeTag | JSDocTypedefTag;
 
@@ -961,6 +967,7 @@ namespace ts.Completions {
         function tryGetObjectLikeCompletionSymbols(objectLikeContainer: ObjectLiteralExpression | ObjectBindingPattern): boolean {
             // We're looking up possible property names from contextual/inferred/declared type.
             isMemberCompletion = true;
+            allowStringLiteral = true;
 
             let typeMembers: Symbol[];
             let existingMembers: ReadonlyArray<Declaration>;
@@ -1609,7 +1616,7 @@ namespace ts.Completions {
      *
      * @return undefined if the name is of external module
      */
-    function getCompletionEntryDisplayNameForSymbol(symbol: Symbol, target: ScriptTarget, performCharacterChecks: boolean): string | undefined {
+    function getCompletionEntryDisplayNameForSymbol(symbol: Symbol, target: ScriptTarget, performCharacterChecks: boolean, allowStringLiteral: boolean): string | undefined {
         const name = symbol.name;
         if (!name) return undefined;
 
@@ -1623,20 +1630,21 @@ namespace ts.Completions {
             }
         }
 
-        return getCompletionEntryDisplayName(name, target, performCharacterChecks);
+        return getCompletionEntryDisplayName(name, target, performCharacterChecks, allowStringLiteral);
     }
 
     /**
      * Get a displayName from a given for completion list, performing any necessary quotes stripping
      * and checking whether the name is valid identifier name.
      */
-    function getCompletionEntryDisplayName(name: string, target: ScriptTarget, performCharacterChecks: boolean): string {
+    function getCompletionEntryDisplayName(name: string, target: ScriptTarget, performCharacterChecks: boolean, allowStringLiteral: boolean): string {
         // If the user entered name for the symbol was quoted, removing the quotes is not enough, as the name could be an
         // invalid identifier name. We need to check if whatever was inside the quotes is actually a valid identifier name.
         // e.g "b a" is valid quoted name but when we strip off the quotes, it is invalid.
         // We, thus, need to check if whatever was inside the quotes is actually a valid identifier name.
         if (performCharacterChecks && !isIdentifierText(name, target)) {
-            return undefined;
+            // TODO: GH#18169
+            return allowStringLiteral ? JSON.stringify(name) : undefined;
         }
 
         return name;

tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment1.ts

@@ -13,12 +13,5 @@
 ////    '/*1*/': ''
 //// }
 
-goTo.marker('0');
-verify.completionListContains("jspm");
-verify.completionListAllowsNewIdentifier();
-verify.completionListCount(1);
-
-goTo.marker('1');
-verify.completionListContains("jspm:dev");
-verify.completionListAllowsNewIdentifier();
-verify.completionListCount(4);
+verify.completionsAt("0", ["jspm", '"jspm:browser"', '"jspm:dev"', '"jspm:node"'], { isNewIdentifierLocation: true });
+verify.completionsAt("1", ["jspm", "jspm:browser", "jspm:dev", "jspm:node"], { isNewIdentifierLocation: true });

tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment2.ts

@@ -19,12 +19,5 @@
 ////    }
 //// }
 
-goTo.marker('0');
-verify.completionListContains("jspm");
-verify.completionListAllowsNewIdentifier();
-verify.completionListCount(1);
-
-goTo.marker('1');
-verify.completionListContains("jspm:dev");
-verify.completionListAllowsNewIdentifier();
-verify.completionListCount(4);
+verify.completionsAt("0", ["jspm", '"jspm:browser"', '"jspm:dev"', '"jspm:node"'], { isNewIdentifierLocation: true });
+verify.completionsAt("1", ["jspm", "jspm:browser", "jspm:dev", "jspm:node"], { isNewIdentifierLocation: true });

tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment3.ts

@@ -4,23 +4,16 @@
 ////      jspm: string;
 ////      'jspm:browser': string;
 ////  } = {
-////          /*0*/: "", 
+////          /*0*/: "",
 ////  }
 
 ////  let configFiles2: {
 ////      jspm: string;
 ////      'jspm:browser': string;
 ////  } = {
 ////         jspm: "",
-////         '/*1*/': "" 
+////         '/*1*/': ""
 ////  }
 
-goTo.marker('0');
-verify.completionListContains("jspm");
-verify.completionListAllowsNewIdentifier();
-verify.completionListCount(1);
-
-goTo.marker('1');
-verify.completionListContains("jspm:browser");
-verify.completionListAllowsNewIdentifier();
-verify.completionListCount(2);
+verify.completionsAt("0", ["jspm", '"jspm:browser"'], { isNewIdentifierLocation: true });
+verify.completionsAt("1", ["jspm", "jspm:browser"], { isNewIdentifierLocation: true });

tests/cases/fourslash/completionListInvalidMemberNames.ts

@@ -11,15 +11,8 @@
 ////    "\u0031\u0062": "invalid unicode identifer name (1b)"
 ////};
 ////
-////x./**/
+////x./*a*/;
+////x["/*b*/"];
 
-goTo.marker();
-
-verify.completionListContains("bar");
-verify.completionListContains("break");
-verify.completionListContains("any");
-verify.completionListContains("$");
-verify.completionListContains("b");
-
-// Nothing else should show up
-verify.completionListCount(5);
+verify.completionsAt("a", ["bar", "break", "any", "$", "b"]);
+verify.completionsAt("b", ["foo ", "bar", "break", "any", "#", "$", "b", "\u0031\u0062"]);

tests/cases/fourslash/completionListInvalidMemberNames2.ts

@@ -3,9 +3,8 @@
 ////enum Foo {
 ////    X, Y, '☆'
 ////}
-////var x = Foo./**/
+////Foo./*a*/;
+////Foo["/*b*/"];
 
-goTo.marker();
-verify.completionListContains("X");
-verify.completionListContains("Y");
-verify.completionListCount(2);
+verify.completionsAt("a", ["X", "Y"]);
+verify.completionsAt("b", ["X", "Y", "☆"]);

tests/cases/fourslash/completion_enum-members-with-invalid-identifiers-should-not-show-in-completion.ts

@@ -7,13 +7,7 @@
 ////     a,
 ////     b
 //// }
-//// 
+////
 //// e./**/
 
-goTo.marker();
-verify.not.completionListContains('1');
-verify.not.completionListContains('"1"');
-verify.not.completionListContains('2');
-verify.not.completionListContains('3');
-verify.completionListContains('a');
-verify.completionListContains('b');
+verify.completionsAt("", ["a", "b"]);

tests/cases/fourslash/fourslash.ts

@@ -164,7 +164,7 @@ declare namespace FourSlashInterface {
     class verify extends verifyNegatable {
         assertHasRanges(ranges: Range[]): void;
         caretAtMarker(markerName?: string): void;
-        completionsAt(markerName: string, completions: string[]): void;
+        completionsAt(markerName: string, completions: string[], options?: { isNewIdentifierLocation?: boolean }): void;
         indentationIs(numberOfSpaces: number): void;
         indentationAtPositionIs(fileName: string, position: number, numberOfSpaces: number, indentStyle?: ts.IndentStyle, baseIndentSize?: number): void;
         textAtCaretIs(text: string): void;