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

Consider using textEdit for completion #40

Open
ul opened this issue May 22, 2018 · 19 comments
Open

Consider using textEdit for completion #40

ul opened this issue May 22, 2018 · 19 comments
Assignees
Labels
enhancement New feature or request external dependency Something need to be fixed upstream help wanted Extra attention is needed high priority

Comments

@ul
Copy link
Collaborator

ul commented May 22, 2018

23-Wed 01:00:04 Franciman not sure it will be significant, but it may be useful. Do you remember the completion problem I had, and you said that you are keeping track of the offset. I found out that in the response to the textDocument/complete request, there can also be a textEdit which contains the range of text to apply the text transformation to (in layman's words write the completion :P)
23-Wed 01:01:08 Franciman so instead of keeping track of the offset you can use the start position of the range of the text edit. This has also the advantage of being able to support, in future, the use of the textEdit, when you select a completion, which may be useful
23-Wed 01:02:04 Franciman since insertText field is deprecated in favor of the textEdit, when the text to insert is different from the label

@ul ul added the enhancement New feature or request label Aug 2, 2018
@vnagarnaik
Copy link

Could you prioritize this feature please? Along with adding support for "additionalTextEdits"?

I am using a C++ language server that provides completions and suggests the appropriate #include header file through "additionalTextEdits". This would make working in kakoune much more productive.

I don't know much rust to be helpful with an actual PR.

@ul
Copy link
Collaborator Author

ul commented Aug 27, 2018

Yes, I think it would be useful to have this done. It should be much easier to implement now because we have textEdit counterparts in lsp.kak. The only problem which remains is that we should cancel Kakoune's autocompletion when item is selected and do our own text edit instead. I'm not sure how to do it in elegant way, going to ask around at IRC channel.

@vnagarnaik
Copy link

Cool, thanks. I can help with testing it if you have pre-release candidates after the change.

@ul
Copy link
Collaborator Author

ul commented Aug 30, 2018

Unfortunately I still don't know how to solve bottleneck I flagged in my previous comment. I raised an issue in Kakoune repo to get some advice.

@ul ul added external dependency Something need to be fixed upstream help wanted Extra attention is needed labels Aug 30, 2018
@ul
Copy link
Collaborator Author

ul commented Jan 9, 2019

This issue seems to be solvable now when we've got completions-commands branch in Kakoune, which I think eventually will be merged.

It's still non-trivial, though.

  1. Applying and clearing textEdit and additionalTextEdits live while the user is cycling through completions menu is fragile as it involves many operations and can interrupt completions engine. I think the complexity of the solution doesn't worth it, and we will stick with default completions behaviour for cycling through items and will apply edits only when completion is confirmed.

  2. As language server is not aware of multiselection, kak-lsp should take care of projecting textEdit to all selections, either by offsetting textEdit for the main selection or by making multiple requests to language server (which has an interesting side-effect that we can take the intersection of all responses to have only candidates which are relevant for all selections). We also need to assume something about additionalTextEdits, for example, that they are always used only for auto-inserting imports and we should apply them only once even in multiselection case.

@Avi-D-coder
Copy link

tsserver sends adtional text edits adding namespace prefixes.

If the additional edits given by a completion at the intersection of the multiselection differ in content or range they should both be applied unless they overlap.

@ul
Copy link
Collaborator Author

ul commented Mar 21, 2019

Does anyone want to give this issue a go? I consistently find myself failing to allocate enough time to have a proper look into it. This is a non-trivial feature which should be quite rewarding once implemented. Of course, I'm keen to help as much as I can.

@ul
Copy link
Collaborator Author

ul commented May 12, 2019

Waiting on mawww/kakoune#2898

@topisani topisani self-assigned this Dec 24, 2019
krobelus added a commit to krobelus/kakoune-lsp that referenced this issue May 4, 2020
This is a targeted fix for the cases where Kakoune insertion would do
exactly the same as the text edit.
If the language server sends simple textEdit objects that replace
precisely the token left of the cursor, then we can just pass them to
Kakoune as normal completions. This is strictly better than using the
completion's label, because Kakoune does exactly what the text edit
is supposed to do by default.

See below for an example that is not fixed by this commit. I believe
it makes sense to have this partial support until kakoune-lsp#40 is properly
implemented, because it seems safe and fixes what is arguably the most
common completion scenario: completing the suffix of a token.

Reproduce with this example C file using ccls. Cursor is |.

	void foobar(){}
	void main(){
		fo|	// Fixed:	Completion inserts foobar()
		fo|ba	// Not fixed:	Completion inserts foobar() -> void
	}

I thought about also changing the behavior for the "Not fixed" case,
where we could also just pass on the completion.
The result would be that Kakoune replaces "fo" with "foobar()",
ignoring the existing suffix, so we'd end up with "foobar()ba".
This is better than the labels here, but I'm not sure if it's
a good idea to knowingly leak this error.

Improves on kakoune-lsp#275
@tadeokondrak
Copy link
Contributor

I was trying to implement this, but it seems that the argument to the InsertCompletionHide hook is empty if you use <ret> to accept the completion.

@ul
Copy link
Collaborator Author

ul commented May 20, 2020

it seems that the argument to the InsertCompletionHide hook is empty if you use to accept the completion.

@mawww do you have any ideas why is this the case?

@mawww
Copy link
Contributor

mawww commented Jun 5, 2020

When you use <ret> to validate the selection, you also add a newline just after it, then once the menu gets hidden, kakoune detects that the cursor is not preceeded by the completion text anymore and filters that out.

I think the existing logic in Kakoune side is too brittle, we should probably keep track of the inserted ranges instead of trying to infer them back from cursor positions and buffer text.

@mawww
Copy link
Contributor

mawww commented Jun 27, 2020

fc3e5ea419aa79c7adf38a9252586d867b3eb19b should have improved things, keen to get feedback.

@krobelus
Copy link
Member

krobelus commented Jul 25, 2020

I have a rough prototype for this in krobelus@textedit-completion

Here is how to try it:

cat > test.c << EOF
void foobar(int x) {}
void main() {
        foba
}
EOF

cat > min-kak-lsp.toml << EOF
[language.c_cpp]
filetypes = ["c", "cpp"]
command = "ccls"
roots = []
EOF

(I'm using ccls, I did not test with other language servers.)

cargo build
: > log
HOME=$PWD kak test.c +3:11 -e '
	source rc/lsp.kak
	set global lsp_cmd "target/debug/kak-lsp -s %val{session} -c min-kak-lsp.toml -vvv --log log"
	lsp-enable
'
pkill -f target/debug/kak-lsp

What already works:

  • i<c-n><esc> -> line 3 will be foobar(
  • i<c-n>x -> line 3 will be foobar(x

What doesn't work:

  • i<c-n><ret> -> leftover indentation from <ret> (see 2.)
  • multiple selections
  • multi-line completions
  • non-ASCII source files (I am experiencing some glitches, see 3.)

There are two problems I'm still facing

  1. The range passed to InsertCompletionHide includes the last character typed by
    the user when accepting the completion.
    I use a quite ugly workaround where I just drop the last character of the range.
    @mawww, maybe we should change InsertCompletionHide to
    also pass the raw completion text (which I use to identify the textEdit to apply).

  2. If the cursor is in the middle of the token to complete
    as in fo%()ba, then after accepting the completion by typing any character, we want to:

    a) replace the completion label with the original text edit
    b) replace the stray ba

I couldn't figure out how to do that reliably with <ret> which autoindents.

  1. When selecting a completion in insert mode we first receive a new
    textDocument/completion and only after that receive the
    textDocument/didChange event that tells kak-lsp that we inserted the
    completion earlier. Because of this wrong ordering, the textDocument/completion
    event contains a completion that is out-of-bounds in the buffer that kak-lsp knows
    about. This causes a panic in the handler for textDocument/completion.

I added a horrible workaround in src/position.rs` that assumes ASCII characters.
This is probably why I'm getting bad results when completing identifiers
with non-ASCII chars.

I'm not yet sure how to fix that. Probably the order of textDocument/completion and
textDocument/didChange should be swapped.

krobelus added a commit to krobelus/kakoune-lsp that referenced this issue Nov 1, 2021
LSP completions are served in two variants: plain strings (without
information where to insert), and LSP text edits that give a
destination range [spec]. It looks like there is no capability to
prevent servers from sending text edits.  The upside is that they
are more powerful; see below for some more detailed motivation.

Implement support for text edit completions.  Drop
workarounds/heuristics we used to partial implemented textEdit support.

To do so, we needed two changes to Kakoune's completion engine:
1. We need to make sure that we have control over the completion
   insertion, because Kakoune would always insert in the same place.
2. For the \xi -> ξ completion we need to make sure that Kakoune
   doesn't filter away a completion labeled ξ (even if the user
   has typed \xi).

This is enabled by a proposed extension to Kakoune's "completions"
options.  Instead of the traditional completions of the form
<text>|<select cmd>|<menu text>, we omit the <text> field.  These
special completions implement the changes 1. (no automatic insertion)
and 2. (no filtering) described above.

Here is a small example - let's say we type ABC in the first line of
a buffer and have this completion set up:

	declare-option completions my_completions
	set-option global completers option=my_completions
	set-option global my_completions \
		"1.1@%val{timestamp}" \
		eval -draft %{ select 1.1,1.3; exec cABCDE, info -style menu "More info on ABCDE" }|ABCDE (Function)

We abuse the on-select handler (which is normally only used to draw a menu),
to insert our completion candidate in the right position.

---

Extended motivation:

* We've had issues where our default lsp_completion_fragment_start
  would fail for tokens starting with sigils, like Ruby @attributes
  and Rust 'lifetimes (kakoune-lsp#378, kakoune-lsp#510).  For this we added workaround
  c413929 (Use column from textEdit as completion start instead of
  heuristic, 2021-09-12) that already used data from textEdit fields.

* Some language servers like Julia's send completions that transform
  text like \xi into ξ (kakoune-lsp#444). This didn't work because of Kakoune's
  completion filtering.

* Some language servers (ccls) always send textEdit completions with
  labels that can't be used as fallback.  This motivated a workaround
  to handle some text edits correctly 6f5c0cd (Use textEdit completions
  that replace token left of cursor, 2020-05-03).

[spec]: <https://microsoft.github.io/language-server-protocol/specifications/specification-3-17/#textDocument_completion>

Closes kakoune-lsp#40, kakoune-lsp#444
krobelus added a commit to krobelus/kakoune that referenced this issue Nov 1, 2021
By giving more control to the user, this allows to resolve a
long-standing issue in kak-lsp to support completions that use LSP
text edits, see kakoune-lsp/kakoune-lsp#40

Option type "completions" takes a list of triples of the form
<text>|<select cmd>|<menu text>.

The <text> field is used
1. as text to insert when this completions is selected
2. for filtering (based on the fragments that user types after the
   completion widget has spawned).

To implement LSP's textEdit completions, kak-lsp needs to
1. customize *where* the text is inserted - the language server is
   probably more correct than our heuristic
   (lsp_completion_fragment_start).
2. disable Kakoune's filtering, because the query text might
   not match what will be inserted (for example Julia's LSP server
   offers completions like \xi -> ξ).

This patch extends the "completions" option by allowing to drop the
<text> field. This places the user in control of how to insert
completions (via <select cmd>), and disables filtering for these
completions.

Possible concerns
* We expect the users's <select cmd> to always modify the buffer when
  selecting a text-less. To get back to the canonical state before
  completion selection, we run "execute-keys -draft u" whenever a
  text-less completion is deselected. This will break if the user
  does not modify the buffer. Can we fix this somehow?

* Also, the new m_did_select feels hacky, but let's settle on the
  design first.

* I didn't test how normal + textless completions interact if both
  are available. I think with the current operator<, normal ones
  dominate text-less completions. Not sure how important this case
  is; in practise all completions are either one or the other.
@krobelus
Copy link
Member

krobelus commented Nov 1, 2021

I've implemented a different approach now, see #551 if you wanna try it.
Some tricky issues are remaining, e.g. if/how we should support multi-selections.

Does anyone know a server that sends additionalTextEdits, then I can test that. Thanks.

krobelus added a commit to krobelus/kakoune-lsp that referenced this issue Nov 1, 2021
LSP completions are served in two variants: plain strings (without
information where to insert), and LSP text edits that give a
destination range [spec]. It looks like there is no capability to
prevent servers from sending text edits.  The upside is that they
are more powerful; see below for some more detailed motivation.

Implement support for text edit completions.  Drop
workarounds/heuristics we used to partial implemented textEdit support.

To do so, we needed two changes to Kakoune's completion engine:
1. We need to make sure that we have control over the completion
   insertion, because Kakoune would always insert in the same place.
2. For the \xi -> ξ completion we need to make sure that Kakoune
   doesn't filter away a completion labeled ξ (even if the user
   has typed \xi).

This is enabled by a proposed extension to Kakoune's "completions"
options.  Instead of the traditional completions of the form
<text>|<select cmd>|<menu text>, we omit the <text> field.  These
special completions implement the changes 1. (no automatic insertion)
and 2. (no filtering) described above.

Here is a small example - let's say we type ABC in the first line of
a buffer and have this completion set up:

	declare-option completions my_completions
	set-option global completers option=my_completions
	set-option global my_completions \
		"1.1@%val{timestamp}" \
		eval -draft %{ select 1.1,1.3; exec cABCDE, info -style menu "More info on ABCDE" }|ABCDE (Function)

We abuse the on-select handler (which is normally only used to draw a menu),
to insert our completion candidate in the right position.

---

Extended motivation:

* We've had issues where our default lsp_completion_fragment_start
  would fail for tokens starting with sigils, like Ruby @attributes
  and Rust 'lifetimes (kakoune-lsp#378, kakoune-lsp#510).  For this we added workaround
  c413929 (Use column from textEdit as completion start instead of
  heuristic, 2021-09-12) that already used data from textEdit fields.

* Some language servers like Julia's send completions that transform
  text like \xi into ξ (kakoune-lsp#444). This didn't work because of Kakoune's
  completion filtering.

* Some language servers (ccls) always send textEdit completions with
  labels that can't be used as fallback.  This motivated a workaround
  to handle some text edits correctly 6f5c0cd (Use textEdit completions
  that replace token left of cursor, 2020-05-03).

[spec]: <https://microsoft.github.io/language-server-protocol/specifications/specification-3-17/#textDocument_completion>

Closes kakoune-lsp#40, kakoune-lsp#444
@daboross
Copy link
Contributor

I believe rust-analyzer uses additionalTextEdits when completing an autocomplete suggestion which requires adding an additional import.

If I have the following program:

fn main() {
    println!("Hello, world!");
    let x = Hash<cursor here>
}

Then autocomplete in VS Code with rust-analyzer plugin and select the second option, "HashMap (use std::collections::HashMap)`, it results in the following program:

use std::collections::HashMap;

fn main() {
    println!("Hello, world!");
    let x = HashMap<cursor here>
}

Note, I'm pretty sure rust-analyzer also specifically disables this functionality if the client does not report additionalTextEdit support, so that might be something to watch out for.

I don't have time right now to test this myself, but I hope this info helps!

@krobelus
Copy link
Member

let x = Hash

yeah, rust-analyzer does not compute the additionalTextEdits eagerly, so we need to call completionItem/resolve.
But first we need to expose that capability (like VSCode), otherwise rust-analyzer won't even send the HashMap completion

diff --git a/src/general.rs b/src/general.rs
index c35ee43..0c7a362 100644
--- a/src/general.rs
+++ b/src/general.rs
@@ -103,7 +103,13 @@ pub fn initialize(root_path: &str, meta: EditorMeta, ctx: &mut Context) {
                         preselect_support: Some(false),
                         tag_support: None,
                         insert_replace_support: None,
-                        resolve_support: None,
+                        resolve_support: Some(CompletionItemCapabilityResolveSupport {
+                            properties: vec![
+                                "documentation".to_string(),
+                                "detail".to_string(),
+                                "additionalTextEdits".to_string(),
+                            ],
+                        }),
                         insert_text_mode_support: None,
                         label_details_support: None,
                     }),

Then, whenever the user confirms (not just selects!) a completion, we'd need to call completionItem/resolve and apply the resulting text edits.

Here's how Emacs' lsp-mode does it. Apparently nvim doesn't support this.

The problem is that in Kakoune (like in Vim) there is no difference between selecting and confirming a completion, so this might get tricky. The server does undo the import statements, so we'd need to clean up, or leave them around.
LSP and Kakoune have different ideas of how completion works, and I don't think it's possible to reconcile them without changing one of them.

As a workaround, we can use :lsp-code-actions Import after inserting the completion. I'm not sure however if we should lie about resolve_support, maybe we can just ask rust-analyzer to compute additionalTextEdits eagerly if the client does not support resolve. They can just send two batches of completions, unresolved and resolved.


BTW the original issue here should be solved for many practical cases by
c413929 (Use column from textEdit as completion start instead of heuristic, 2021-09-12)

@krobelus
Copy link
Member

Ok nevermind I believe we can use InsertCompletionHide to figure out when a completion was "accepted", so additionalTextEdits should be doable.

@krobelus
Copy link
Member

Gotta go now but krobelus@completion-additionalTextEdits is a quick-and-dirty fix for the rust-analyzer example.
In some scenarios it will also add the import of the selected item if you hit backspace, I'm not sure how to avoid that.
This happens when you type HashMap, select the completion, then type backspace. The backspace makes kak-lsp ask for completions again, so it triggers another InsertCompletionHide and InsertCompletionShow

krobelus added a commit that referenced this issue Dec 14, 2021
One of the uses of completionItem/resolve is to compute edits to
be applied when the completion is accepted. Support this.

For example, this makes kak-lsp insert
import statements from rust-analyzer, see
#40 (comment)

This adds some noisy indentation changes to the detection of completion
offsets / simple text edits but "git diff -w" takes care of that.
@krobelus
Copy link
Member

2637b26 takes care of adding the import statements, thanks for the test case

topisani pushed a commit that referenced this issue Dec 15, 2021
One of the uses of completionItem/resolve is to compute edits to
be applied when the completion is accepted. Support this.

For example, this makes kak-lsp insert
import statements from rust-analyzer, see
#40 (comment)

This adds some noisy indentation changes to the detection of completion
offsets / simple text edits but "git diff -w" takes care of that.
@topisani topisani mentioned this issue May 15, 2022
krobelus added a commit to krobelus/kakoune that referenced this issue May 29, 2022
… menu

Insert mode completions are accepted by typing any key.  For example,
if there is a completion "somefunction()", then typing

	some<c-n>;

will insert

	somefunction();

and then the InsertCompletionHide hook will fire.  The hook parameter
is a range that contains the entire thing: the actual completion plus
the trailing semicolon that closed the completion menu.

The [original motivation] for the hook parameter was to support
removing text inserted by completion, so we can apply text edits
or expand snippets instead. One problem is that we don't want to
remove the semicolon. Another problem came up in a discussion
about [snippets]: let's say we have a snippet "add" that expands to

	add(?, ?)

where ? are placeholders. After snippet expansion the cursor replaces
the first placeholder. If I type "ad<c-n>1" I expect to get "add(1, ?)".
If the InsertCompletionHide hook only runs after processing the "1"
keystroke, this is not possible without evil hacks.

Fix these problems by running InsertCompletionHide when a completion is
accepted _before_ inserting anything else into the buffer.  This should
make it much easier to fully implement [LSP text edits]. I doubt
that anyone besides kak-lsp is using the hook parameter today so this
should be a low-risk fix.

[original motivation]: mawww#2898
[snippets]: kakoune-lsp/kakoune-lsp#616 (comment)
[LSP text edits] kakoune-lsp/kakoune-lsp#40
krobelus added a commit to krobelus/kakoune that referenced this issue May 29, 2022
… menu

Insert mode completions are accepted by typing any key.  For example,
if there is a completion "somefunction()", then typing

	some<c-n>;

will insert

	somefunction();

and then the InsertCompletionHide hook will fire.  The hook parameter
is a range that contains the entire thing: the actual completion plus
the trailing semicolon that closed the completion menu.

The [original motivation] for the hook parameter was to support
removing text inserted by completion, so we can apply text edits
or expand snippets instead. One problem is that we don't want to
remove the semicolon. Another problem came up in a discussion
about [snippets]: let's say we have a snippet "add" that expands to

	add(?, ?)

where ? are placeholders. After snippet expansion the cursor replaces
the first placeholder. If I type "ad<c-n>1" I expect to get "add(1, ?)".
If the InsertCompletionHide hook only runs after processing the "1"
keystroke, this is not possible without evil hacks.

Fix these problems by running InsertCompletionHide when a completion is
accepted _before_ inserting anything else into the buffer.  This should
make it much easier to fully implement [LSP text edits]. I doubt
that anyone besides kak-lsp is using the hook parameter today so this
should be a low-risk fix.

[original motivation]: mawww#2898
[snippets]: kakoune-lsp/kakoune-lsp#616 (comment)
[LSP text edits]: kakoune-lsp/kakoune-lsp#40
krobelus added a commit to krobelus/kakoune that referenced this issue May 29, 2022
… menu

Insert mode completions are accepted by typing any key.  For example,
if there is a completion "somefunction()", then typing

	some<c-n>;

will insert

	somefunction();

and then the InsertCompletionHide hook will fire.  The hook parameter
is a range that contains the entire thing: the actual completion plus
the trailing semicolon that closed the completion menu.

The [original motivation] for the hook parameter was to support
removing text inserted by completion, so we can apply text edits
or expand snippets instead. One problem is that we don't want to
remove the semicolon. Another problem came up in a discussion
about [snippets]: let's say we have a snippet "add" that expands to

	add(?, ?)

where ? are placeholders. After snippet expansion the cursor replaces
the first placeholder. If I type "ad<c-n>1" I expect to get "add(1, ?)".
If the InsertCompletionHide hook only runs after processing the "1"
keystroke, this is not possible without evil hacks.

Fix these problems by running InsertCompletionHide when a completion is
accepted _before_ inserting anything else into the buffer.  This should
make it much easier to fully implement [LSP text edits]. I doubt
that anyone besides kak-lsp is using the hook parameter today so this
should be a low-risk fix.

[original motivation]: mawww#2898
[snippets]: kakoune-lsp/kakoune-lsp#616 (comment)
[LSP text edits]: kakoune-lsp/kakoune-lsp#40
TeddyDD pushed a commit to TeddyDD/kakoune that referenced this issue Aug 19, 2022
… menu

Insert mode completions are accepted by typing any key.  For example,
if there is a completion "somefunction()", then typing

	some<c-n>;

will insert

	somefunction();

and then the InsertCompletionHide hook will fire.  The hook parameter
is a range that contains the entire thing: the actual completion plus
the trailing semicolon that closed the completion menu.

The [original motivation] for the hook parameter was to support
removing text inserted by completion, so we can apply text edits
or expand snippets instead. One problem is that we don't want to
remove the semicolon. Another problem came up in a discussion
about [snippets]: let's say we have a snippet "add" that expands to

	add(?, ?)

where ? are placeholders. After snippet expansion the cursor replaces
the first placeholder. If I type "ad<c-n>1" I expect to get "add(1, ?)".
If the InsertCompletionHide hook only runs after processing the "1"
keystroke, this is not possible without evil hacks.

Fix these problems by running InsertCompletionHide when a completion is
accepted _before_ inserting anything else into the buffer.  This should
make it much easier to fully implement [LSP text edits]. I doubt
that anyone besides kak-lsp is using the hook parameter today so this
should be a low-risk fix.

[original motivation]: mawww#2898
[snippets]: kakoune-lsp/kakoune-lsp#616 (comment)
[LSP text edits]: kakoune-lsp/kakoune-lsp#40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request external dependency Something need to be fixed upstream help wanted Extra attention is needed high priority
Projects
None yet
Development

No branches or pull requests

8 participants