Skip to content

Allow quoted names in completions #18162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
3 commits merged into from
Sep 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -4314,4 +4318,8 @@ namespace FourSlashInterface {
actionName: string;
actionDescription: string;
}

export interface CompletionsAtOptions {
isNewIdentifierLocation?: boolean;
}
}
46 changes: 27 additions & 19 deletions src/services/completions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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;
}
Expand All @@ -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)) {
Expand Down Expand Up @@ -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 };
}
Expand Down Expand Up @@ -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 };
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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[] = [];
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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>;
Expand Down Expand Up @@ -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;

Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
15 changes: 4 additions & 11 deletions tests/cases/fourslash/completionListInvalidMemberNames.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"]);
9 changes: 4 additions & 5 deletions tests/cases/fourslash/completionListInvalidMemberNames2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", "☆"]);
Original file line number Diff line number Diff line change
Expand Up @@ -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"]);
2 changes: 1 addition & 1 deletion tests/cases/fourslash/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down