From 36607e1bde77ba57bb09023ded321f18516abaf5 Mon Sep 17 00:00:00 2001 From: Andy Date: Wed, 6 Sep 2017 14:39:53 -0700 Subject: [PATCH] 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 --- src/harness/fourslash.ts | 14 ++++-- src/services/completions.ts | 46 +++++++++++-------- ...nForQuotedPropertyInPropertyAssignment1.ts | 11 +---- ...nForQuotedPropertyInPropertyAssignment2.ts | 11 +---- ...nForQuotedPropertyInPropertyAssignment3.ts | 15 ++---- .../completionListInvalidMemberNames.ts | 15 ++---- .../completionListInvalidMemberNames2.ts | 9 ++-- ...entifiers-should-not-show-in-completion.ts | 10 +--- tests/cases/fourslash/fourslash.ts | 2 +- 9 files changed, 57 insertions(+), 76 deletions(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 3010fc53533..c224fd210ea 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -762,7 +762,7 @@ namespace FourSlash { } } - public verifyCompletionsAt(markerName: string, expected: string[]) { + public verifyCompletionsAt(markerName: string, expected: string[], options?: FourSlashInterface.CompletionsAtOptions) { this.goToMarker(markerName); const actualCompletions = this.getCompletionListAtCaret(); @@ -770,6 +770,10 @@ namespace FourSlash { 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; + } } diff --git a/src/services/completions.ts b/src/services/completions.ts index fde07aa78f6..300ade2da48 100644 --- a/src/services/completions.ts +++ b/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,7 +56,7 @@ 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 { @@ -64,7 +64,7 @@ namespace ts.Completions { 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, location: Node, performCharacterChecks: boolean, typeChecker: TypeChecker, target: ScriptTarget, log: Log): Map { + function getCompletionEntriesFromSymbols(symbols: Symbol[], entries: Push, location: Node, performCharacterChecks: boolean, typeChecker: TypeChecker, target: ScriptTarget, log: Log, allowStringLiteral: boolean): Map { const start = timestamp(); const uniqueNames = createMap(); 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((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; @@ -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; diff --git a/tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment1.ts b/tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment1.ts index 15f5901113b..ab218cea93d 100644 --- a/tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment1.ts +++ b/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 }); diff --git a/tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment2.ts b/tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment2.ts index 1d20b57e2a1..66ba4ada241 100644 --- a/tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment2.ts +++ b/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 }); diff --git a/tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment3.ts b/tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment3.ts index 764011d90c1..1ab9aa4a8cf 100644 --- a/tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment3.ts +++ b/tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment3.ts @@ -4,7 +4,7 @@ //// jspm: string; //// 'jspm:browser': string; //// } = { -//// /*0*/: "", +//// /*0*/: "", //// } //// let configFiles2: { @@ -12,15 +12,8 @@ //// '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 }); diff --git a/tests/cases/fourslash/completionListInvalidMemberNames.ts b/tests/cases/fourslash/completionListInvalidMemberNames.ts index e0a65bfca4d..8e62ba2fb67 100644 --- a/tests/cases/fourslash/completionListInvalidMemberNames.ts +++ b/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"]); diff --git a/tests/cases/fourslash/completionListInvalidMemberNames2.ts b/tests/cases/fourslash/completionListInvalidMemberNames2.ts index 6b25cf1f5d9..753f9bbcb30 100644 --- a/tests/cases/fourslash/completionListInvalidMemberNames2.ts +++ b/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); \ No newline at end of file +verify.completionsAt("a", ["X", "Y"]); +verify.completionsAt("b", ["X", "Y", "☆"]); diff --git a/tests/cases/fourslash/completion_enum-members-with-invalid-identifiers-should-not-show-in-completion.ts b/tests/cases/fourslash/completion_enum-members-with-invalid-identifiers-should-not-show-in-completion.ts index d01856a54fb..6d5b3167198 100644 --- a/tests/cases/fourslash/completion_enum-members-with-invalid-identifiers-should-not-show-in-completion.ts +++ b/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'); \ No newline at end of file +verify.completionsAt("", ["a", "b"]); diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index 652ed4812c0..5150fa16ae9 100644 --- a/tests/cases/fourslash/fourslash.ts +++ 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;