Skip to content

WIP - Get more detailed completions by default #167

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

Closed
wants to merge 2 commits into from
Closed

WIP - Get more detailed completions by default #167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
498202b
Allow the user to enable more detailed completions
DonnieWest Nov 12, 2020
f0e08a5
Filter warnings before processing
DonnieWest Nov 17, 2020
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
10 changes: 9 additions & 1 deletion server/src/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ const program = new Command('typescript-language-server')
.version(require('../package.json').version)
.option('--stdio', 'use stdio')
.option('--node-ipc', 'use node-ipc')
.option('--detailed-completions', 'use detailed completions')
.option('--detailed-completions-limit <limit>', 'limit the number of completions provided with details. Defaults to `500`')
.option('--log-level <logLevel>', 'A number indicating the log level (4 = log, 3 = info, 2 = warn, 1 = error). Defaults to `2`.')
.option('--socket <port>', 'use socket. example: --socket=5000')
.option('--tsserver-log-file <tsserverLogFile>', 'Specify a tsserver log file. example: --tsserver-log-file ts-logs.txt')
Expand All @@ -32,6 +34,10 @@ if (program.tsserverLogFile && !program.tsserverLogVerbosity) {
program.tsserverLogVerbosity = 'normal'
}

if (!program.detailedCompletionsLimit) {
program.detailedCompletionsLimit = 500
Copy link
Member

Choose a reason for hiding this comment

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

This is IMO risky as completions are not re-requested when you are narrowing your completion results. So if we exclude something initially, it will be unavailable even if the user narrows down his query.

Due to that, all those optimizations should be optional IMO. Better to be correct than shaving some milliseconds.

Copy link

Choose a reason for hiding this comment

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

It's not just a matter of shaving milliseconds, the LSP server in neovim has been having issues handling large payloads, leading to freezes and OOM issues; see this issue.

But I agree that arbitrarily limiting results is risky, people need to trust that they are seeing all available completion items.

Maybe a good middle ground would be to visually indicate to the user that the list has been truncated? ie. adding a <RESULTS TRUNCATED> item to the completion list; I know this sucks but I'm not sure there is a better way.

Copy link
Member

@rchl rchl Nov 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

For servers that are likely to return a lot of completions it's OK to limit but then also use the isIncomplete flag so that completions are re-requested on query change.

So that could be the solution here too - add a flag that enables the use of isIncomplete option and limiting the results. But that would likely require more effort to implement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good points and thanks for the feedback!

To be clear, this limit doesn't change the total number of candidates completed: it only makes those candidates less detailed. I'm finding it's needed (and I probably need to make the default smaller) because of a compounding issue with as-you-type completions. Sometimes a completion that is more broad (ie user types co) will return after a more specific query (ie user continues typing console.).

I need to do a little more research into how this is handled on other servers. I know our cancellation story is pretty rough right now, but I don't know that it would handle this specific situation.

Copy link
Member

Choose a reason for hiding this comment

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

I can't see how we can come up with a good solution for this issue. Even if we would use the isIncomplete flag and limit number of completions, that could result in client missing some completion that he wanted to use. It would likely come on typing another characters but for some (maybe all) editors the completion popup might already be dismissed since there were no matching completions.

Also, with the isIncomplete flag we would trade the initial heavy completions with a need to request completions on every typed character which would make things worse for editors that currently don't struggle with large number of completions.

So IMO this PR should be closed.

}

let logLevel = lsp.MessageType.Warning
if (program.logLevel) {
logLevel = parseInt(program.logLevel, 10);
Expand All @@ -45,5 +51,7 @@ createLspConnection({
tsserverPath: program.tsserverPath as string,
tsserverLogFile: program.tsserverLogFile as string,
tsserverLogVerbosity: program.tsserverLogVerbosity as string,
showMessageLevel: logLevel as lsp.MessageType
showMessageLevel: logLevel as lsp.MessageType,
detailedCompletionsLimit: program.detailedCompletionsLimit,
detailedCompletions: program.detailedCompletions,
}).listen();
21 changes: 21 additions & 0 deletions server/src/completion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,27 @@ export function asCommitCharacters(kind: ScriptElementKind): string[] | undefine
return commitCharacters.length === 0 ? undefined : commitCharacters;
}

export function asDetailedCompletionItems(items: tsp.CompletionEntryDetails[], file: string, line: number, offset: number): TSCompletionItem[] {
return items.map(entry => {
const item: TSCompletionItem = {
label: entry.name,
kind: asCompletionItemKind(entry.kind),
commitCharacters: asCommitCharacters(entry.kind),
detail: asDetail(entry),
documentation: asDocumentation(entry),
data: {
file,
line,
offset,
entryNames: [
entry.source ? { name: entry.name, source: entry.source.join(' ') } : entry.name
]
}
}
return { ...item, ...asCodeActions(entry, file) }
})
}

export function asResolvedCompletionItem(item: TSCompletionItem, details: tsp.CompletionEntryDetails): TSCompletionItem {
item.detail = asDetail(details);
item.documentation = asDocumentation(details);
Expand Down
8 changes: 6 additions & 2 deletions server/src/lsp-connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ export interface IServerOptions {
tsserverPath: string;
tsserverLogFile?: string;
tsserverLogVerbosity?: string;
showMessageLevel: lsp.MessageType
showMessageLevel: lsp.MessageType;
detailedCompletionsLimit: number;
detailedCompletions: boolean;
}

export function createLspConnection(options: IServerOptions): lsp.IConnection {
Expand All @@ -28,7 +30,9 @@ export function createLspConnection(options: IServerOptions): lsp.IConnection {
lspClient,
tsserverPath: options.tsserverPath,
tsserverLogFile: options.tsserverLogFile,
tsserverLogVerbosity: options.tsserverLogVerbosity
tsserverLogVerbosity: options.tsserverLogVerbosity,
detailedCompletions: options.detailedCompletions,
detailedCompletionsLimit: options.detailedCompletionsLimit,
});

connection.onInitialize(server.initialize.bind(server));
Expand Down
40 changes: 34 additions & 6 deletions server/src/lsp-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
} from './protocol-translation';
import { getTsserverExecutable } from './utils';
import { LspDocuments, LspDocument } from './document';
import { asCompletionItem, TSCompletionItem, asResolvedCompletionItem } from './completion';
import { asCompletionItem, TSCompletionItem, asResolvedCompletionItem, asDetailedCompletionItems } from './completion';
import { asSignatureHelp } from './hover';
import { Commands } from './commands';
import { provideQuickFix } from './quickfix';
Expand All @@ -45,6 +45,8 @@ export interface IServerOptions {
tsserverLogFile?: string;
tsserverLogVerbosity?: string;
lspClient: LspClient;
detailedCompletions: boolean;
detailedCompletionsLimit: number;
}

export class LspServer {
Expand Down Expand Up @@ -444,10 +446,20 @@ export class LspServer {
includeExternalModuleExports: true,
includeInsertTextCompletions: true
}));
const body = result.body || [];
return body
.filter(entry => entry.kind !== 'warning')
.map(entry => asCompletionItem(entry, file, params.position, document));

const body = (result.body || []).filter(entry => entry.kind !== 'warning');

if (!this.options.detailedCompletions || body.length > this.options.detailedCompletionsLimit) {
return body
.map(entry => asCompletionItem(entry, file, params.position, document));
}

return this.completionDetails(
body,
file,
params.position.line + 1,
params.position.character + 1,
)
} catch (error) {
if (error.message === "No content available.") {
this.logger.info('No content was available for completion request');
Expand All @@ -458,7 +470,23 @@ export class LspServer {
}
}

async completionResolve(item: TSCompletionItem): Promise<lsp.CompletionItem> {
async completionDetails(
items: tsp.CompletionEntry[],
file: string,
line: number,
offset: number
): Promise<TSCompletionItem[]> {
const { body } = await this.interuptDiagnostics(() => this.tspClient.request(CommandTypes.CompletionDetails, {
file,
line,
offset,
entryNames: items.map(entry => entry.name)
}))

return asDetailedCompletionItems(body || [], file, line, offset)
}

async completionResolve(item: TSCompletionItem): Promise<TSCompletionItem> {
this.logger.log('completion/resolve', item);
const { body } = await this.interuptDiagnostics(() => this.tspClient.request(CommandTypes.CompletionDetails, item.data));
const details = body && body.length && body[0];
Expand Down
2 changes: 2 additions & 0 deletions server/src/test-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ export async function createServer(options: {
applyWorkspaceEdit: () => Promise.reject(new Error('unsupported')),
rename: () => Promise.reject(new Error('unsupported'))
},
detailedCompletions: false,
detailedCompletionsLimit: 500,
});

await server.initialize({
Expand Down