Skip to content

Commit

Permalink
fix #7810: support new CompletionItem.range shape
Browse files Browse the repository at this point in the history 
Also don't send:
- duplicate ranges on provide completions
- entire completion back and forth on resolve completion

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
  • Loading branch information
@akosyakov
akosyakov committed May 15, 2020
1 parent 79f3212 commit 96e3c72
Show file tree
Hide file tree
Showing 7 changed files with 77 additions and 62 deletions.
6 changes: 5 additions & 1 deletion packages/plugin-ext/src/common/plugin-api-rpc-model.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ export interface Completion {
preselect?: boolean;
insertText: string;
insertTextRules?: CompletionItemInsertTextRule;
range: Range | {
range?: Range | {
insert: Range;
replace: Range;
};
Expand Down Expand Up @@ -201,6 +201,10 @@ export interface CompletionDto extends Completion {

export interface CompletionResultDto extends IdObject {
id: number;
defaultRange: {
insert: Range,
replace: Range
}
completions: CompletionDto[];
incomplete?: boolean;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/plugin-ext/src/common/plugin-api-rpc.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1166,7 +1166,7 @@ export interface PluginInfo {
export interface LanguagesExt {
$provideCompletionItems(handle: number, resource: UriComponents, position: Position,
context: CompletionContext, token: CancellationToken): Promise<CompletionResultDto | undefined>;
$resolveCompletionItem(handle: number, resource: UriComponents, position: Position, completion: Completion, token: CancellationToken): Promise<Completion | undefined>;
$resolveCompletionItem(handle: number, parentId: number, id: number, token: CancellationToken): Promise<Completion | undefined>;
$releaseCompletionItems(handle: number, id: number): void;
$provideImplementation(handle: number, resource: UriComponents, position: Position, token: CancellationToken): Promise<Definition | undefined>;
$provideTypeDefinition(handle: number, resource: UriComponents, position: Position, token: CancellationToken): Promise<Definition | undefined>;
Expand Down
15 changes: 12 additions & 3 deletions packages/plugin-ext/src/main/browser/languages-main.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import {
import { injectable, inject } from 'inversify';
import {
SerializedDocumentFilter, MarkerData, Range, RelatedInformation,
MarkerSeverity, DocumentLink, WorkspaceSymbolParams, CodeAction
MarkerSeverity, DocumentLink, WorkspaceSymbolParams, CodeAction, CompletionDto
} from '../../common/plugin-api-rpc-model';
import { RPCProtocol } from '../../common/rpc-protocol';
import { fromLanguageSelector } from '../../plugin/type-converters';
Expand All @@ -57,6 +57,7 @@ import { toDefinition, toUriComponents, fromDefinition, fromPosition, toCaller }
import { Position, DocumentUri } from 'vscode-languageserver-types';
import { ObjectIdentifier } from '../../common/object-identifier';
import { WorkspaceSymbolProvider } from '@theia/languages/lib/browser/language-client-services';
import { mixin } from '../../common/types';

@injectable()
export class LanguagesMainImpl implements LanguagesMain, Disposable {
Expand Down Expand Up @@ -142,7 +143,9 @@ export class LanguagesMainImpl implements LanguagesMain, Disposable {
return undefined;
}
return {
suggestions: result.completions,
suggestions: result.completions.map(c => Object.assign(c, {
range: c.range || result.defaultRange
})),
incomplete: result.incomplete,
dispose: () => this.proxy.$releaseCompletionItems(handle, result.id)
};
Expand All @@ -151,7 +154,13 @@ export class LanguagesMainImpl implements LanguagesMain, Disposable {

protected resolveCompletionItem(handle: number, model: monaco.editor.ITextModel, position: monaco.Position,
item: monaco.languages.CompletionItem, token: monaco.CancellationToken): monaco.languages.ProviderResult<monaco.languages.CompletionItem> {
return this.proxy.$resolveCompletionItem(handle, model.uri, position, item, token);
const { parentId, id } = item as CompletionDto;
return this.proxy.$resolveCompletionItem(handle, parentId, id, token).then(resolved => {
if (resolved) {
mixin(item, resolved, true);
}
return item;
});
}

$registerDefinitionProvider(handle: number, pluginInfo: PluginInfo, selector: SerializedDocumentFilter[]): void {
Expand Down
4 changes: 2 additions & 2 deletions packages/plugin-ext/src/plugin/languages.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,8 +235,8 @@ export class LanguagesExtImpl implements LanguagesExt {
return this.withAdapter(handle, CompletionAdapter, adapter => adapter.provideCompletionItems(URI.revive(resource), position, context, token), undefined);
}

$resolveCompletionItem(handle: number, resource: UriComponents, position: Position, completion: Completion, token: theia.CancellationToken): Promise<Completion | undefined> {
return this.withAdapter(handle, CompletionAdapter, adapter => adapter.resolveCompletionItem(URI.revive(resource), position, completion, token), undefined);
$resolveCompletionItem(handle: number, parentId: number, id: number, token: theia.CancellationToken): Promise<Completion | undefined> {
return this.withAdapter(handle, CompletionAdapter, adapter => adapter.resolveCompletionItem(parentId, id, token), undefined);
}

$releaseCompletionItems(handle: number, id: number): void {
Expand Down
71 changes: 35 additions & 36 deletions packages/plugin-ext/src/plugin/languages/completion.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import * as theia from '@theia/plugin';
import { CompletionList, Range, SnippetString } from '../types-impl';
import { DocumentsExtImpl } from '../documents';
import * as Converter from '../type-converters';
import { mixin } from '../../common/types';
import { Position } from '../../common/plugin-api-rpc';
import { CompletionContext, CompletionResultDto, Completion, CompletionDto, CompletionItemInsertTextRule } from '../../common/plugin-api-rpc-model';
import { CommandRegistryImpl } from '../command-registry';
Expand All @@ -45,6 +44,13 @@ export class CompletionAdapter {
const doc = document.document;

const pos = Converter.toPosition(position);

// The default insert/replace ranges. It's important to compute them
// before asynchronously asking the provider for its results. See
// https://github.com/microsoft/vscode/issues/83400#issuecomment-546851421
const replacing = doc.getWordRangeAtPosition(pos) || new Range(pos, pos);
const inserting = replacing.with({ end: pos });

return Promise.resolve(this.delegate.provideCompletionItems(doc, pos, token, context)).then(value => {
const id = this.cacheId++;

Expand All @@ -54,6 +60,10 @@ export class CompletionAdapter {
const result: CompletionResultDto = {
id,
completions: [],
defaultRange: {
insert: Converter.fromRange(inserting),
replace: Converter.fromRange(replacing)
}
};

let list: CompletionList;
Expand All @@ -66,11 +76,8 @@ export class CompletionAdapter {
result.incomplete = list.isIncomplete;
}

const wordRangeBeforePos = (doc.getWordRangeAtPosition(pos) as Range || new Range(pos, pos))
.with({ end: pos });

for (let i = 0; i < list.items.length; i++) {
const suggestion = this.convertCompletionItem(list.items[i], pos, wordRangeBeforePos, i, id);
const suggestion = this.convertCompletionItem(list.items[i], i, id, inserting, replacing);
if (suggestion) {
result.completions.push(suggestion);
}
Expand All @@ -81,33 +88,19 @@ export class CompletionAdapter {
});
}

resolveCompletionItem(resource: URI, position: Position, completion: Completion, token: theia.CancellationToken): Promise<Completion> {
async resolveCompletionItem(parentId: number, id: number, token: theia.CancellationToken): Promise<Completion | undefined> {
if (typeof this.delegate.resolveCompletionItem !== 'function') {
return Promise.resolve(completion);
return undefined;
}

const { parentId, id } = (<CompletionDto>completion);
const item = this.cache.has(parentId) && this.cache.get(parentId)![id];
const item = this.cache.get(parentId)?.[id];
if (!item) {
return Promise.resolve(completion);
return undefined;
}

return Promise.resolve(this.delegate.resolveCompletionItem(item, token)).then(resolvedItem => {

if (!resolvedItem) {
return completion;
}

const doc = this.documents.getDocumentData(resource)!.document;
const pos = Converter.toPosition(position);
const wordRangeBeforePos = (doc.getWordRangeAtPosition(pos) as Range || new Range(pos, pos)).with({ end: pos });
const newCompletion = this.convertCompletionItem(resolvedItem, pos, wordRangeBeforePos, id, parentId);
if (newCompletion) {
mixin(completion, newCompletion, true);
}

return completion;
});
const resolvedItem = await this.delegate.resolveCompletionItem(item, token);
if (!resolvedItem) {
return undefined;
}
return this.convertCompletionItem(resolvedItem, id, parentId);
}

async releaseCompletionItems(id: number): Promise<void> {
Expand All @@ -119,7 +112,8 @@ export class CompletionAdapter {
}
}

private convertCompletionItem(item: theia.CompletionItem, position: theia.Position, defaultRange: theia.Range, id: number, parentId: number): CompletionDto | undefined {
private convertCompletionItem(item: theia.CompletionItem, id: number, parentId: number,
defaultInserting?: theia.Range, defaultReplacing?: theia.Range): CompletionDto | undefined {
if (typeof item.label !== 'string' || item.label.length === 0) {
console.warn('Invalid Completion Item -> must have at least a label');
return undefined;
Expand All @@ -130,12 +124,6 @@ export class CompletionAdapter {
throw Error('DisposableCollection is missing...');
}

const range = item.textEdit ? item.textEdit.range : item.range || defaultRange;
if (range && (!range.isSingleLine || range.start.line !== position.line)) {
console.warn('Invalid Completion Item -> must be single line and on the same line');
return undefined;
}

let insertText = item.label;
let insertTextRules = item.keepWhitespace ? CompletionItemInsertTextRule.KeepWhitespace : 0;
if (item.textEdit) {
Expand All @@ -147,6 +135,17 @@ export class CompletionAdapter {
insertTextRules |= CompletionItemInsertTextRule.InsertAsSnippet;
}

let range: Completion['range'] | undefined;
const itemRange = item.textEdit?.range || item.range;
if (Range.isRange(itemRange)) {
range = Converter.fromRange(itemRange);
} else if (itemRange && (!defaultInserting?.isEqual(itemRange.inserting) || !defaultReplacing?.isEqual(itemRange.replacing))) {
range = {
insert: Converter.fromRange(itemRange.inserting),
replace: Converter.fromRange(itemRange.replacing)
};
}

return {
id,
parentId,
Expand All @@ -159,7 +158,7 @@ export class CompletionAdapter {
preselect: item.preselect,
insertText,
insertTextRules,
range: Converter.fromRange(range),
range,
additionalTextEdits: item.additionalTextEdits && item.additionalTextEdits.map(Converter.fromTextEdit),
command: this.commands.converter.toSafeCommand(item.command, toDispose),
commitCharacters: item.commitCharacters
Expand Down
21 changes: 11 additions & 10 deletions packages/plugin-ext/src/plugin/types-impl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,8 @@ export class Range {
return new Range(start, end);
}

static isRange(thing: {}): thing is theia.Range {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
static isRange(thing: any): thing is theia.Range {
if (thing instanceof Range) {
return true;
}
Expand Down Expand Up @@ -2013,17 +2014,17 @@ export enum FoldingRangeKind {

export class SelectionRange {

range: Range;
parent?: SelectionRange;
range: Range;
parent?: SelectionRange;

constructor(range: Range, parent?: SelectionRange) {
this.range = range;
this.parent = parent;
constructor(range: Range, parent?: SelectionRange) {
this.range = range;
this.parent = parent;

if (parent && !parent.range.contains(this.range)) {
throw new Error('Invalid argument: parent must contain this range');
}
}
if (parent && !parent.range.contains(this.range)) {
throw new Error('Invalid argument: parent must contain this range');
}
}
}

/**
Expand Down
20 changes: 11 additions & 9 deletions packages/plugin/src/theia.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5982,15 +5982,17 @@ declare module '@theia/plugin' {
insertText?: string | SnippetString;

/**
* A range of text that should be replaced by this completion item.
*
* Defaults to a range from the start of the [current word](#TextDocument.getWordRangeAtPosition) to the
* current position.
*
* *Note:* The range must be a [single line](#Range.isSingleLine) and it must
* [contain](#Range.contains) the position at which completion has been [requested](#CompletionItemProvider.provideCompletionItems).
*/
range?: Range;
* A range or a insert and replace range selecting the text that should be replaced by this completion item.
*
* When omitted, the range of the [current word](#TextDocument.getWordRangeAtPosition) is used as replace-range
* and as insert-range the start of the [current word](#TextDocument.getWordRangeAtPosition) to the
* current position is used.
*
* *Note 1:* A range must be a [single line](#Range.isSingleLine) and it must
* [contain](#Range.contains) the position at which completion has been [requested](#CompletionItemProvider.provideCompletionItems).
* *Note 2:* A insert range must be a prefix of a replace range, that means it must be contained and starting at the same position.
*/
range?: Range | { inserting: Range; replacing: Range; };

/**
* An optional set of characters that when pressed while this completion is active will accept it first and
Expand Down

0 comments on commit 96e3c72

Please sign in to comment.