Skip to content

Commit

Permalink
Support multiple completions with the same name but different source …
Browse files Browse the repository at this point in the history 
…module (#19455) (#19496)

* Support multiple completions with the same name but different source module

* Use optional parameters for source

* Simplify use of `uniques`

* Update test

* Fix `undefined` error
  • Loading branch information
Andy committed Oct 26, 2017
1 parent c35e90e commit a7e172b
Show file tree
Hide file tree
Showing 23 changed files with 301 additions and 117 deletions.
52 changes: 29 additions & 23 deletions src/harness/fourslash.ts
View file
Expand Up @@ -783,13 +783,13 @@ namespace FourSlash {
});
}

public verifyCompletionListContains(symbol: string, text?: string, documentation?: string, kind?: string, spanIndex?: number, hasAction?: boolean) {
public verifyCompletionListContains(entryId: ts.Completions.CompletionEntryIdentifier, text?: string, documentation?: string, kind?: string, spanIndex?: number, hasAction?: boolean) {
const completions = this.getCompletionListAtCaret();
if (completions) {
this.assertItemInCompletionList(completions.entries, symbol, text, documentation, kind, spanIndex, hasAction);
this.assertItemInCompletionList(completions.entries, entryId, text, documentation, kind, spanIndex, hasAction);
}
else {
this.raiseError(`No completions at position '${this.currentCaretPosition}' when looking for '${symbol}'.`);
this.raiseError(`No completions at position '${this.currentCaretPosition}' when looking for '${JSON.stringify(entryId)}'.`);
}
}

Expand All @@ -804,7 +804,7 @@ namespace FourSlash {
* @param expectedKind the kind of symbol (see ScriptElementKind)
* @param spanIndex the index of the range that the completion item's replacement text span should match
*/
public verifyCompletionListDoesNotContain(symbol: string, expectedText?: string, expectedDocumentation?: string, expectedKind?: string, spanIndex?: number) {
public verifyCompletionListDoesNotContain(entryId: ts.Completions.CompletionEntryIdentifier, expectedText?: string, expectedDocumentation?: string, expectedKind?: string, spanIndex?: number) {
const that = this;
let replacementSpan: ts.TextSpan;
if (spanIndex !== undefined) {
Expand Down Expand Up @@ -833,14 +833,14 @@ namespace FourSlash {

const completions = this.getCompletionListAtCaret();
if (completions) {
let filterCompletions = completions.entries.filter(e => e.name === symbol);
let filterCompletions = completions.entries.filter(e => e.name === entryId.name && e.source === entryId.source);
filterCompletions = expectedKind ? filterCompletions.filter(e => e.kind === expectedKind) : filterCompletions;
filterCompletions = filterCompletions.filter(filterByTextOrDocumentation);
if (filterCompletions.length !== 0) {
// After filtered using all present criterion, if there are still symbol left in the list
// then these symbols must meet the criterion for Not supposed to be in the list. So we
// raise an error
let error = "Completion list did contain \'" + symbol + "\'.";
let error = `Completion list did contain '${JSON.stringify(entryId)}\'.`;
const details = this.getCompletionEntryDetails(filterCompletions[0].name);
if (expectedText) {
error += "Expected text: " + expectedText + " to equal: " + ts.displayPartsToString(details.displayParts) + ".";
Expand Down Expand Up @@ -1130,8 +1130,8 @@ Actual: ${stringify(fullActual)}`);
return this.languageService.getCompletionsAtPosition(this.activeFile.fileName, this.currentCaretPosition);
}

private getCompletionEntryDetails(entryName: string) {
return this.languageService.getCompletionEntryDetails(this.activeFile.fileName, this.currentCaretPosition, entryName, this.formatCodeSettings);
private getCompletionEntryDetails(entryName: string, source?: string) {
return this.languageService.getCompletionEntryDetails(this.activeFile.fileName, this.currentCaretPosition, entryName, this.formatCodeSettings, source);
}

private getReferencesAtCaret() {
Expand Down Expand Up @@ -1640,7 +1640,7 @@ Actual: ${stringify(fullActual)}`);
const longestNameLength = max(entries, m => m.name.length);
const longestKindLength = max(entries, m => m.kind.length);
entries.sort((m, n) => m.sortText > n.sortText ? 1 : m.sortText < n.sortText ? -1 : m.name > n.name ? 1 : m.name < n.name ? -1 : 0);
const membersString = entries.map(m => `${pad(m.name, longestNameLength)} ${pad(m.kind, longestKindLength)} ${m.kindModifiers}`).join("\n");
const membersString = entries.map(m => `${pad(m.name, longestNameLength)} ${pad(m.kind, longestKindLength)} ${m.kindModifiers} ${m.source === undefined ? "" : m.source}`).join("\n");
Harness.IO.log(membersString);
}

Expand Down Expand Up @@ -2296,13 +2296,13 @@ Actual: ${stringify(fullActual)}`);
public applyCodeActionFromCompletion(markerName: string, options: FourSlashInterface.VerifyCompletionActionOptions) {
this.goToMarker(markerName);

const actualCompletion = this.getCompletionListAtCaret().entries.find(e => e.name === options.name);
const actualCompletion = this.getCompletionListAtCaret().entries.find(e => e.name === options.name && e.source === options.source);

if (!actualCompletion.hasAction) {
this.raiseError(`Completion for ${options.name} does not have an associated action.`);
}

const details = this.getCompletionEntryDetails(options.name);
const details = this.getCompletionEntryDetails(options.name, actualCompletion.source);
if (details.codeActions.length !== 1) {
this.raiseError(`Expected one code action, got ${details.codeActions.length}`);
}
Expand Down Expand Up @@ -2984,33 +2984,35 @@ Actual: ${stringify(fullActual)}`);

private assertItemInCompletionList(
items: ts.CompletionEntry[],
name: string,
entryId: ts.Completions.CompletionEntryIdentifier,
text: string | undefined,
documentation: string | undefined,
kind: string | undefined,
spanIndex: number | undefined,
hasAction: boolean | undefined,
) {
for (const item of items) {
if (item.name === name) {
if (documentation !== undefined || text !== undefined) {
const details = this.getCompletionEntryDetails(item.name);
if (item.name === entryId.name && item.source === entryId.source) {
if (documentation !== undefined || text !== undefined || entryId.source !== undefined) {
const details = this.getCompletionEntryDetails(item.name, item.source);

if (documentation !== undefined) {
assert.equal(ts.displayPartsToString(details.documentation), documentation, this.assertionMessageAtLastKnownMarker("completion item documentation for " + name));
assert.equal(ts.displayPartsToString(details.documentation), documentation, this.assertionMessageAtLastKnownMarker("completion item documentation for " + entryId));
}
if (text !== undefined) {
assert.equal(ts.displayPartsToString(details.displayParts), text, this.assertionMessageAtLastKnownMarker("completion item detail text for " + name));
assert.equal(ts.displayPartsToString(details.displayParts), text, this.assertionMessageAtLastKnownMarker("completion item detail text for " + entryId));
}

assert.deepEqual(details.source, entryId.source === undefined ? undefined : [ts.textPart(entryId.source)]);
}

if (kind !== undefined) {
assert.equal(item.kind, kind, this.assertionMessageAtLastKnownMarker("completion item kind for " + name));
assert.equal(item.kind, kind, this.assertionMessageAtLastKnownMarker("completion item kind for " + entryId));
}

if (spanIndex !== undefined) {
const span = this.getTextSpanForRangeAtIndex(spanIndex);
assert.isTrue(TestState.textSpansEqual(span, item.replacementSpan), this.assertionMessageAtLastKnownMarker(stringify(span) + " does not equal " + stringify(item.replacementSpan) + " replacement span for " + name));
assert.isTrue(TestState.textSpansEqual(span, item.replacementSpan), this.assertionMessageAtLastKnownMarker(stringify(span) + " does not equal " + stringify(item.replacementSpan) + " replacement span for " + entryId));
}

assert.equal(item.hasAction, hasAction);
Expand All @@ -3021,7 +3023,7 @@ Actual: ${stringify(fullActual)}`);

const itemsString = items.map(item => stringify({ name: item.name, kind: item.kind })).join(",\n");

this.raiseError(`Expected "${stringify({ name, text, documentation, kind })}" to be in list [${itemsString}]`);
this.raiseError(`Expected "${stringify({ entryId, text, documentation, kind })}" to be in list [${itemsString}]`);
}

private findFile(indexOrName: any) {
Expand Down Expand Up @@ -3732,12 +3734,15 @@ namespace FourSlashInterface {

// Verifies the completion list contains the specified symbol. The
// completion list is brought up if necessary
public completionListContains(symbol: string, text?: string, documentation?: string, kind?: string, spanIndex?: number, hasAction?: boolean) {
public completionListContains(entryId: string | ts.Completions.CompletionEntryIdentifier, text?: string, documentation?: string, kind?: string, spanIndex?: number, hasAction?: boolean) {
if (typeof entryId === "string") {
entryId = { name: entryId, source: undefined };
}
if (this.negative) {
this.state.verifyCompletionListDoesNotContain(symbol, text, documentation, kind, spanIndex);
this.state.verifyCompletionListDoesNotContain(entryId, text, documentation, kind, spanIndex);
}
else {
this.state.verifyCompletionListContains(symbol, text, documentation, kind, spanIndex, hasAction);
this.state.verifyCompletionListContains(entryId, text, documentation, kind, spanIndex, hasAction);
}
}

Expand Down Expand Up @@ -4492,6 +4497,7 @@ namespace FourSlashInterface {

export interface VerifyCompletionActionOptions extends NewContentOptions {
name: string;
source?: string;
description: string;
}
}
16 changes: 15 additions & 1 deletion src/server/protocol.ts
View file
Expand Up @@ -1625,7 +1625,12 @@ namespace ts.server.protocol {
/**
* Names of one or more entries for which to obtain details.
*/
entryNames: string[];
entryNames: (string | CompletionEntryIdentifier)[];
}

export interface CompletionEntryIdentifier {
name: string;
source: string;
}

/**
Expand Down Expand Up @@ -1685,6 +1690,10 @@ namespace ts.server.protocol {
* made to avoid errors. The CompletionEntryDetails will have these actions.
*/
hasAction?: true;
/**
* Identifier (not necessarily human-readable) identifying where this completion came from.
*/
source?: string;
}

/**
Expand Down Expand Up @@ -1722,6 +1731,11 @@ namespace ts.server.protocol {
* The associated code actions for this entry
*/
codeActions?: CodeAction[];

/**
* Human-readable description of the `source` from the CompletionEntry.
*/
source?: SymbolDisplayPart[];
}

export interface CompletionsResponse extends Response {
Expand Down
9 changes: 5 additions & 4 deletions src/server/session.ts
View file
Expand Up @@ -1188,10 +1188,10 @@ namespace ts.server {
if (simplifiedResult) {
return mapDefined<CompletionEntry, protocol.CompletionEntry>(completions && completions.entries, entry => {
if (completions.isMemberCompletion || (entry.name.toLowerCase().indexOf(prefix.toLowerCase()) === 0)) {
const { name, kind, kindModifiers, sortText, replacementSpan, hasAction } = entry;
const { name, kind, kindModifiers, sortText, replacementSpan, hasAction, source } = entry;
const convertedSpan = replacementSpan ? this.decorateSpan(replacementSpan, scriptInfo) : undefined;
// Use `hasAction || undefined` to avoid serializing `false`.
return { name, kind, kindModifiers, sortText, replacementSpan: convertedSpan, hasAction: hasAction || undefined };
return { name, kind, kindModifiers, sortText, replacementSpan: convertedSpan, hasAction: hasAction || undefined, source };
}
}).sort((a, b) => compareStrings(a.name, b.name));
}
Expand All @@ -1206,8 +1206,9 @@ namespace ts.server {
const position = this.getPosition(args, scriptInfo);
const formattingOptions = project.projectService.getFormatCodeOptions(file);

return mapDefined(args.entryNames, entryName => {
const details = project.getLanguageService().getCompletionEntryDetails(file, position, entryName, formattingOptions);
return mapDefined<string | protocol.CompletionEntryIdentifier, protocol.CompletionEntryDetails>(args.entryNames, entryName => {
const { name, source } = typeof entryName === "string" ? { name: entryName, source: undefined } : entryName;
const details = project.getLanguageService().getCompletionEntryDetails(file, position, name, formattingOptions, source);
if (details) {
const mappedCodeActions = map(details.codeActions, action => this.mapCodeAction(action, scriptInfo));
return { ...details, codeActions: mappedCodeActions };
Expand Down
19 changes: 12 additions & 7 deletions src/services/codefixes/importFixes.ts
View file
Expand Up @@ -249,7 +249,11 @@ namespace ts.codefix {
const lastImportDeclaration = findLast(sourceFile.statements, isAnyImportSyntax);

const moduleSpecifierWithoutQuotes = stripQuotes(moduleSpecifier);
const importDecl = createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, createImportClauseOfKind(kind, symbolName), createStringLiteralWithQuoteStyle(sourceFile, moduleSpecifierWithoutQuotes));
const importDecl = createImportDeclaration(
/*decorators*/ undefined,
/*modifiers*/ undefined,
createImportClauseOfKind(kind, symbolName),
createStringLiteralWithQuoteStyle(sourceFile, moduleSpecifierWithoutQuotes));
const changes = ChangeTracker.with(context, changeTracker => {
if (lastImportDeclaration) {
changeTracker.insertNodeAfter(sourceFile, lastImportDeclaration, importDecl, { suffix: newLineCharacter });
Expand Down Expand Up @@ -279,13 +283,14 @@ namespace ts.codefix {
}

function createImportClauseOfKind(kind: ImportKind, symbolName: string) {
const id = createIdentifier(symbolName);
switch (kind) {
case ImportKind.Default:
return createImportClause(createIdentifier(symbolName), /*namedBindings*/ undefined);
return createImportClause(id, /*namedBindings*/ undefined);
case ImportKind.Namespace:
return createImportClause(/*name*/ undefined, createNamespaceImport(createIdentifier(symbolName)));
return createImportClause(/*name*/ undefined, createNamespaceImport(id));
case ImportKind.Named:
return createImportClause(/*name*/ undefined, createNamedImports([createImportSpecifier(/*propertyName*/ undefined, createIdentifier(symbolName))]));
return createImportClause(/*name*/ undefined, createNamedImports([createImportSpecifier(/*propertyName*/ undefined, id)]));
default:
Debug.assertNever(kind);
}
Expand Down Expand Up @@ -529,7 +534,7 @@ namespace ts.codefix {
declarations: ReadonlyArray<AnyImportSyntax>): ImportCodeAction {
const fromExistingImport = firstDefined(declarations, declaration => {
if (declaration.kind === SyntaxKind.ImportDeclaration && declaration.importClause) {
const changes = tryUpdateExistingImport(ctx, ctx.kind, declaration.importClause);
const changes = tryUpdateExistingImport(ctx, declaration.importClause);
if (changes) {
const moduleSpecifierWithoutQuotes = stripQuotes(declaration.moduleSpecifier.getText());
return createCodeAction(
Expand Down Expand Up @@ -559,8 +564,8 @@ namespace ts.codefix {
return expression && isStringLiteral(expression) ? expression.text : undefined;
}

function tryUpdateExistingImport(context: SymbolContext, kind: ImportKind, importClause: ImportClause): FileTextChanges[] | undefined {
const { symbolName, sourceFile } = context;
function tryUpdateExistingImport(context: SymbolContext & { kind: ImportKind }, importClause: ImportClause): FileTextChanges[] | undefined {
const { symbolName, sourceFile, kind } = context;
const { name, namedBindings } = importClause;
switch (kind) {
case ImportKind.Default:
Expand Down

0 comments on commit a7e172b

Please sign in to comment.