Skip to content
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

auto complete should replace if the response returns a text edit with range value #85741

Closed
testforstephen opened this issue Nov 28, 2019 · 17 comments
Assignees
Labels
*dev-question VS Code Extension Development Question info-needed Issue requires more information from poster suggest IntelliSense, Auto Complete
Milestone

Comments

@testforstephen
Copy link

VS Code version:

Version: 1.40.2 (user setup)
Commit: f359dd69833dd8800b54d458f6d37ab7c78df520
Date: 2019-11-25T14:54:45.096Z
Electron: 6.1.5
Chrome: 76.0.3809.146
Node.js: 12.4.0
V8: 7.6.303.31-electron.0
OS: Windows_NT x64 10.0.18363

When i manually type import java. in Java file, it didn't replace the words. See the gif.
autocomplete_bug

Below is the completion response returned by the Java LS. Actually it returns a textEdit to replace the target range with the newText, but looks like VS Code didn't apply the TextEdit well.

[Trace - 12:24:21 PM] Received response 'completionItem/resolve - (54)' in 4ms.
Result: {
    "label": "java.awt.color",
    "kind": 9,
    "detail": "(package) java.awt.color",
    "sortText": "999999215",
    "insertTextFormat": 2,
    "textEdit": {
        "range": {
            "start": {
                "line": 21,
                "character": 7
            },
            "end": {
                "line": 21,
                "character": 12
            }
        },
        "newText": "java.awt.color.*;"
    }
}
@jrieken
Copy link
Member

jrieken commented Dec 2, 2019

Pretty sure this isn't us. Having java. it seems that completion got recomputed and that things like java.awt.im are suggested and not awt.im. When having that prefix, show suggest details and press cmd+? for a details about the text operation. It will show what the extension intents to replace

@testforstephen
Copy link
Author

Actually the same Java extension works at 1.39.0. This issue just appears in the latest VS Code version 1.40.2. Looks like a regression in VS Code.

@jrieken
Copy link
Member

jrieken commented Dec 2, 2019

Ping on #85741 (comment)

@testforstephen
Copy link
Author

testforstephen commented Dec 2, 2019

I enabled the cmd + ? in the new screenshot.

If i type import java first, and at the end, click ctrl+space to trigger the completion. The returned completion is similar to replace the prefix java, but it works as normal.
complete_ctrl_space

But if i type an extra ., the completion will not replace.
complete_dot

The different part of these two responses is only the range. One is 7 to 11, another is 7 to 12.

@jrieken
Copy link
Member

jrieken commented Dec 2, 2019

But if i type an extra ., the completion will not replace.

Pretty sure that Java uses . as trigger character and that it doesn't honour the current line prefix. E.g. the hover in the second gif shows "(no prefix)" which means the completion inserts at the position (no replace). VS Code doesn't "invent" those ranges, it simply mirrors what extension give to it.

@testforstephen
Copy link
Author

To be honest, i didn't fully get what you mean. Is there any bug to explain why changed the behavior in recent 1.40?

@jrieken
Copy link
Member

jrieken commented Dec 2, 2019

Is there any bug to explain why changed the behavior in recent 1.40?

I don't recall any change that would affect this (and be unnoticed for the long time 1.40 is out). What I am trying to say is that I am certain that this is an issue with the Java extension.

@testforstephen
Copy link
Author

I'm curious that this Java feature is implemented a long long time ago, and it always works. For example, in previous 1.39, it also shows (no prefix) in the completion details, but VS Code will follow the range value to replace the prefix java.. So is it a potential bug in the previous VS Code version, right?

Besides, is there any documentation to explain the expected behavior for the dot case? In case someone else gets the same error, and wants a reference. Because from the lsp documentation about completion, it just says the range in the text edit should contains the position at which the completion has been requested. It didn't say the range cannot contain the prefix before dot (e.g. java.).

/**
  * An edit which is applied to a document when selecting this completion. When an edit is provided the value of
  * `insertText` is ignored.
  *
  * *Note:* The range of the edit must be a single line range and it must contain the position at which completion  has been requested.
*/
textEdit?: TextEdit;

@jrieken jrieken added the suggest IntelliSense, Auto Complete label Dec 2, 2019
@jrieken
Copy link
Member

jrieken commented Dec 2, 2019

I can actually reproduce with insiders and Java dev containers but I don't understand what's going on.

With a completion item like below (vscode.dts-land) I get what's expected, e.g after java.| I see how the prefix is selected to be overwritten. @dbaeumer given the protocol message above what is the vscode.dts object that I would receive?

const item2 = new vscode.CompletionItem('java.awt');
item2.detail = 'java.awt';
item2.textEdit = vscode.TextEdit.replace(new vscode.Range(pos.line, 7, pos.line, 12), 'java.awt')		

Screenshot 2019-12-02 at 15 08 39

@jrieken jrieken added this to the November 2019 milestone Dec 2, 2019
@jrieken
Copy link
Member

jrieken commented Dec 2, 2019

I cannot reproduce using the lsp-sample and this LSP suggestion

connection.onCompletion(
	(_textDocumentPosition: TextDocumentPositionParams): CompletionItem[] => {
		// The pass parameter contains the position of the text document in
		// which code complete got requested. For the example we ignore this
		// info and always provide the same completion items.
		return [
			{
				label: 'TypeScript',
				kind: CompletionItemKind.Text,
				data: 1
			},
			{
				label: 'JavaScript',
				kind: CompletionItemKind.Text,
				data: 2
			},
			{
				label: 'java.awt',
				detail: 'hehehe',
				textEdit: { newText: 'java.awt', range: { start: { line: 1, character: 7 }, end: { line: 1, character: 12 } } },
			}
		];
	}
);

@testforstephen Can you reproduce with the lsp-sample? What message do I need to send to reproduce this?

@testforstephen
Copy link
Author

// This handler provides the initial list of the completion items.
connection.onCompletion(
	(_textDocumentPosition: TextDocumentPositionParams): CompletionItem[] => {
		// The pass parameter contains the position of the text document in
		// which code complete got requested. For the example we ignore this
		// info and always provide the same completion items.
		return [
			{
				label: 'TypeScript',
				kind: CompletionItemKind.Text,
				data: 1
			},
			{
				label: 'JavaScript',
				kind: CompletionItemKind.Text,
				data: 2
			},
			{
				label: 'java.awt',
				kind: CompletionItemKind.Module,
				data: 3
			}
		];
	}
);

// This handler resolves additional information for the item selected in
// the completion list.
connection.onCompletionResolve(
	(item: CompletionItem): CompletionItem => {
		if (item.data === 1) {
			item.detail = 'TypeScript details';
			item.documentation = 'TypeScript documentation';
		} else if (item.data === 2) {
			item.detail = 'JavaScript details';
			item.documentation = 'JavaScript documentation';
		}
		else if (item.data === 3) {
			item.detail = 'Java details';
			item.documentation = 'Java documentation';
			item.textEdit = { newText: 'java.awt', range: { start: { line: 1, character: 7 }, end: { line: 1, character: 12 } } };
		}
		return item;
	}
);

Stop at import java.|, and then click ctrl + space to trigger completion, i can reproduce the problem.

@jrieken
Copy link
Member

jrieken commented Dec 3, 2019

Oh, it happens when resolving... But it seem that you only set the textEdit/range when resolving. That's not really supported because it will change the ranking and sorting and it will yield in unexpected results. Resolve should only be used for details/doc and for nothing that influences the insert behaviour. Has your extension always been doing that?

@testforstephen
Copy link
Author

Yes, the Java extension doesn't calculate textEdit property during completion response, and it will lazy set it during resolving.

@jrieken
Copy link
Member

jrieken commented Dec 3, 2019

doesn't calculate textEdit property during completion response, and it will lazy set it during resolving.

That's not good and you are lucky that this has been working in the past. I believe it was a coincidence because in the past we didn't cache ranges of completion items - something that we have changed for #10266.

For sorting and filtering the range (derived from the edit) is needed because it defines what leading text to compare with. Also there is no guarantee that resolve is called at all, and when inserting/accepting an item we do not await resolving (this is some background why we cannot do that).

There is doc about that but I will make sure this is more clear and that we print warnings when resolve is doing too much.

For the Java-extension the only way out is to return items that are ready to be inserted when onCompletion is called and to only add detail and documentation during resolve.

@testforstephen
Copy link
Author

got that, thanks for the discussion.

@jrieken
Copy link
Member

jrieken commented Dec 4, 2019

Closing this as question. I have also created #86122 follow-up on our side to be more explicit (telemetry, logging) about this.

@jrieken jrieken added the *dev-question VS Code Extension Development Question label Dec 4, 2019
@vscodebot
Copy link

vscodebot bot commented Dec 4, 2019

We have a great developer community over on slack where extension authors help each other. This is a great place for you to ask questions and find support.

Happy Coding!

@vscodebot vscodebot bot closed this as completed Dec 4, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
*dev-question VS Code Extension Development Question info-needed Issue requires more information from poster suggest IntelliSense, Auto Complete
Projects
None yet
Development

No branches or pull requests

4 participants
@jrieken @dbaeumer @testforstephen and others