Skip to content

Support multiple completions with the same name but different source module #19455

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.

Sign up for GitHub

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
5 commits merged into from
Oct 26, 2017
Merged

Support multiple completions with the same name but different source module #19455

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f0d44ce
Support multiple completions with the same name but different source …
Oct 23, 2017
a2bed45
Use optional parameters for source
Oct 25, 2017
7d2568a
Simplify use of `uniques`
Oct 25, 2017
c09d464
Update test
Oct 25, 2017
264d2b7
Fix `undefined` error
Oct 26, 2017
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
53 changes: 30 additions & 23 deletions src/harness/fourslash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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.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.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,9 @@ 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) {
const id = source === undefined ? entryName : { name: entryName, source };
return this.languageService.getCompletionEntryDetails(this.activeFile.fileName, this.currentCaretPosition, id, this.formatCodeSettings);
}

private getReferencesAtCaret() {
Expand Down Expand Up @@ -1640,7 +1641,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 +2297,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 +2985,35 @@ Actual: ${stringify(fullActual)}`);

private assertItemInCompletionList(
items: ts.CompletionEntry[],
name: string,
entryId: ts.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 +3024,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 +3735,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.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 +4498,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
Open in desktop
Original file line number Diff line number Diff line change
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[] | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Is undefined needed since this is already optional?

}

export interface CompletionsResponse extends Response {
Expand Down
6 changes: 3 additions & 3 deletions src/server/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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,7 +1206,7 @@ namespace ts.server {
const position = this.getPosition(args, scriptInfo);
const formattingOptions = project.projectService.getFormatCodeOptions(file);

return mapDefined(args.entryNames, entryName => {
return mapDefined<string | protocol.CompletionEntryIdentifier, protocol.CompletionEntryDetails>(args.entryNames, entryName => {
const details = project.getLanguageService().getCompletionEntryDetails(file, position, entryName, formattingOptions);
if (details) {
const mappedCodeActions = map(details.codeActions, action => this.mapCodeAction(action, scriptInfo));
Expand Down
19 changes: 12 additions & 7 deletions src/services/codefixes/importFixes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Loading