Support snippet-less completion
matklad opened this issue · 21 comments
Some clients don't support completions snippets. It can be argued that clients generally should be improved to support snippets, but
- I can imagine embedding rust-analyzer in the contexts where full snippet support does not make sense (for example, completion in the
evalfield of a debugger) - I can imagine some users might prefer simple identifier completions
So, we should support this use-case. Specifically, we should only send snippet completions if supports them.
A simplest fix here (and a good initial implemtation) is change code around here to just strip $i snippet markers from text edit, if the client doesn't support snippets. A similar "fixup" approach is used for folding ranges.
However in general it seems like the core completion engine should just provide different completions depending on whether snippets are enable or not. This should be done via feature flags (example), and requires some design to make sure that we don't twice as much code, but still provide best possible completions with and without snippets.
Thanks for opening this issue. As mentioned in #1705, not providing plain text completion is a protocol violation, so "clients should be improved" is not a valid statement.
Some clients don't support completions snippets. It can be argued that clients generally should be improved to support snippets
As a maintainer of such a client and someone with a strong opinion that goes against snippets, here's my .02c:
I'm a vim user. Snippets in vim are expanded on something called CompleteDone event, which is triggered whenever something closes the completion menu. That can be ctrl-y, which works fine with snippets and simply closes the menu. However this completely breaks my workflow. I'm used to just continuing to type after selecting a completion. If I hit enter instead, the new line is entered before the snippet is expanded, which messes up a lot of things and to work around this is incredibly tricky (speaking from the client maintainer's point of view). So I have two options if I want to use snippets:
- Break my workflow in favour of a less efficient one.
- Put much more work than I think it's worth, for a feature that I would still find bad.
- That's because with plain text completion, I know ahead of time what text will be inserted. This is again, very important for my workflow.
So the instead, ycmd maintainers, unanimously, settled for a third option - not implementing snippets.
However in general it seems like the core completion engine should just provide different completions depending on whether snippets are enable or not.
So, I haven't looked at the rust-analyzer code, but a client like ycmd would expect foo insertion text for a foo(/* any amount of args */).
Now I didn't mean to sound harsh, in case I did. It's just that a lot of servers assume snippets are supported, even though it can be argued that they provide a pretty horrible user experience.
On a side note, my workflow is basically the workflow of all ycmd users, so drastic changes like snippets could very much be more harm than good, considering that ycmd is much older than LSP.
EDIT:
A simplest fix here (and a good initial implemtation) is change code around here to just strip $i snippet markers from text edit, if the client doesn't support snippets.
Would that make insertion text just foo()? That's something ycmd can work with, but it's less than idea. The Apple's Swift server, when used with a client that doesn't support snippets, just strips ${}, but leaves the test in between { and }, i.e. it returns foo(int bar). This forced me to override the completion algorithm and strip everything after the first (. Like I said, workable, but not ideal.
Would that make insertion text just foo()?
Yes. As I've said, that would be a good initial implementation, which would primarily focus on plumbing the required flags.
For the final implementation, a corresponding flag should be added to the internal CompletionConfig, and than this setting need to be honored everywhere where we use snippets.
After that, we either should assert here that we don't see a compleiton with a snippet, or we should just not add such completions.
After that, we either should assert here that we don't see a compleiton with a snippet, or we should just not add such completions.
I've actually figured a much neater way to do this: introduce an unforgable SnippetCap token, which proves that this capability is available, and add it as an argument to snippet producing functions.
I've implemented this idea in #4116
No the only thing left is the actual plumbing to fetch the cap from the client....
Plumbing implemented in #4117
Here's example of completion with a snippet:
And here's the same completion without the snippet:
In your example I would still argue that the completion is probably completing too much. It’s unexpected that selecting ‘size’ competes a whole line of text including a function declaration.
I kind of get it. It’s boilerplate, so let’s write it for the user.
My concern is that sort of thing is divisive and opinionated. Perhaps that’s intentional (if so, so be it), but certainly my users are unlikely to expect that and would likely be model-breaking for completion systems and users accustomed to more natural word expansion.
The original point put across by Boris is that completion systems doing too much breach’s workflow because when stuff happens that you don’t expect you end up having to rewind. Let’s say you develop in N languages and N-1 behave one way, eg like clangd or jdt.ls or gopls) and the Nth one behaves like this. The challenge for the user in that case is to switch gears. We would like to avoid having the user switch gears and offer consistent powerful experience across languages.
Anyway just my 2p. Tl:dr there are a large quantity of users that don’t expect the completion system to complete more than a word at a time. It’s probably worth considering that group.
As usual, the user is (almost) always right, and, if some users complain about this feature, we'll introduce a feature flag, like, for example, we did for automatic () insertion. However, the reasoning should be "I've tried this feature and I hate this", rather "this is wrong because completion isn't supposed to work this way".
With my "unsolicited advice giver" hat on ( :-) ), I think for ide-like tools it's generally good to:
- provide opinionated defaults,
- but give the user power to choose.
I did add support for features which I, personally, don't use and find counter-productive, like argument snippets (yes, I hate stupid snippets myself), which, I believe, only exists because parameter hint UI is universally horrible (IntelliJ's version is kind of ok, actually, but the API used to implement it is very crufty).
From re-reading original comment here, I get the impression that the primary reason for not supporting snippets is not that "users expect a single word", but that vim expects a single word, and that it's a pain to work around this limitation. So, to me, the path that maximizes user happiness (without taking work needed into account), is:
- fix Vim's core to make snippet-based completion ergonomic for the user. I think this might entail some interesting UX design questions around combining completion and modal editing. I think the best example to learn from would be IDEA Vim. It is maintained by a person who is both a JetBrainser and a Vimmer, so it probably solves the problem taking both perspective into account.
- add an option to ycmd to take advantage of snippets, to give the user the choice
- keep this option off by default, to nudge users towards more productive (according to opinionated views of ycmd maintainers, which are totally valid) workflows.
I get the impression that the primary reason for not supporting snippets is not that "users expect a single word", but that vim expects a single word, and that it's a pain to work around this limitation
No, vim plugins can adequately support snippets. I have even implemented it for YCM. But, as you say "provide opinionated defaults" - we tried it, we evaluated it, and we decided we don't like it as it doesn't fit well with our philosophy and the mental model our users have of what we provide.
Add to that the complexity of doing it well, the fact there's only 2 of us and neither of us really want it, and we decided against it.
Similarly to you, if we have 1000s of users asking for it, we might rethink, but so far not so many. The "option" we offer is to use my fork. Mileage variance notwithstanding.
Our point was really that we made this decision and we declared that decision to the Language Server using capabilities. We expect that decision to be honoured. We could just as well have merged my snippets branch and offered the user the ability to globally disable it by, again, declaring in the capabilities that we don't support them (this prevents the user having to configure this for each and every project/language combination in unique, marvellous and usually undocumented ways).
Anyway, as you say, that's water under the bridge. The capability is now honoured (thanks!).
I've tested rust-analyzer today with YCM and updated instructions for using it: https://github.com/ycm-core/lsp-examples#rust-rust-analyzer. (For the record i looked in the log and it'f full of errors for other parts of the capabilities that are not being honoured, but i know you have a GitHub issue for the general case, and everyone has a backlog/priorities).
But getting back to it, my suggestion about the function-declaration was largely orthogonal to that, and comes back to the original philosophy of largely getting out of the way and assuming users know what to type (it's mentioned here from 2013 http://val.markovic.io/articles/youcompleteme-a-fast-as-you-type-fuzzy-search-code-completion-engine-for-vim), but more that the consistency of experience across languages is a truly powerful thing. YCM has been providing consistent experience across languages for years, and i think our users appreciate that, so we're generally in favour of minimising surprising behaviours. Like i said it was really just a take-it-or-leave-it thought. In this case, opinionated default = complete full declaration, feature option=don't.
On a side note, i'd like to test the "full function declaration" completion because it may hit edge cases in the textEdit expansion - would you be willing to let me know a simple test case for it ? I don't rust, so i'd be grateful if there's a place i can trigger it (e.g. in rust-analyzer source code).
YCM has been providing consistent experience across languages for years, and i think our users appreciate that, so we're generally in favour of minimising surprising behaviours.
Obviously we care more about providing a great experience for Rust than about providing a consistent experience across languages. But I want to push back a bit on the "surprising" part -- the completion is labeled fn size_hint(..). It seems to me pretty obvious that it's not going to insert just one word (though I actually think it should even be labeled fn size_hint(..) {} to make that even clearer). Maybe this is also about how exactly the UI for the completions works.
On a side note, i'd like to test the "full function declaration" completion because it may hit edge cases in the textEdit expansion - would you be willing to let me know a simple test case for it ?
trait Foo {
fn bar(&self, x: u64) -> f32;
}
struct S;
impl Foo for S {
// (type bar here)
}I think that should be enough to trigger it (if you put it into main.rs in a new Cargo project), though I haven't tested it.
actually that does work quite well
did notice something odd though (you can briefly see it in the demo). when selecting a macro, while the label contains the !, the insertion_text (or textEdit) does not .
You can see here that the 'newText' does not contain the !
"filterText":"debug_assert!","insertTextFormat":1,"kind":2,"label":"debug_assert!","textEdit":{"newText":"debug_assert","range":{"end":{"character":8,"line":7},"start":{"character":4,"line":7}}}},
Seems wrong to me? Expected?
Kind of expected: ! is the part of !($0) snippet, that's why it is left out of the edit (so that only identifier is completed). It is present in the label because that's a natural way to indicate that this is macro. However, that last bit seems to work by accident -- the label for snippeted completion should be debug_assert!(...) and it isn't
but the label and what's inserted don't match....i mean the label looks like the right thing to insert to me (and that's what rls would insert here):
Note: rls is also giving a "not all traits implemented" warning with a fixit (code action) to insert it. can/does rust-analyzer do the same ?
Sorry for the comparison to rls. I realise apples/pairs, but it's in relation to the suggestion that rust-analyzer be the "official" LS for rust, which i'm fine with, just YCM uses rls by default (as the "supported" LS) and should that decision go ahead, we'd want to switch to rust-analyzer too.
but the label and what's inserted don't match....i
In general, label and insert text don't need to match. For example, when importing a macro, like in
use std::debug_assert;the ! is not needed in the insert text (because it is for syntactically disambiguate macro invocations), but i'd still expect to see it in a label, because ! is perceived as part of macro names (for example, docs refer to macros with !).
In this case, I didn't add ! without snippets as that's close to "complete only identifier" model and requires one less case to handle, but, as you find that adding ! is not surprising, I'll add it: #4121
can/does rust-analyzer do the same ?
We do, but only the file is saved. For compiler provided fixits, we just run compiler once in a while, like the old-school flymake/flycheck/ale(?). In the end, we hope to provide the same fixits from in-memory model, but we are not there yet.
(We do provide the "implement missing trait items" fixit though even without saving, we just don't show a diagnostic!)
We do, but only the file is saved
I see, is it based on didSave ? If so that would explain it (we don't support the didSave notification)....
Client Capability:
property name (optional): textDocument.synchronization.didSave
property type: boolean
The capability indicates that the client supports textDocument/didSave notifications.
(We do provide the "implement missing trait items" fixit though even without saving, we just don't show a diagnostic!)
Oh right. so a code-action in the broad sense. I'll try it.
Well, i just get an error when running that. Apologies, that's off-topic for this conversation, so posting in case you're interested. Feel free to hide this comment (and the previous 2)
[ERROR rust_analyzer::main_loop] unknown request: Request { id: RequestId(U64(3)), method: "workspace/executeCommand", params: Object({"arguments": Array([Object({"cursorPosition": Object({"position": Object({"character": Number(4), "line": Number(6)}), "textDocument": Object({"uri": String("file:///Users/ben/.vim/bundle/lsp-examples/rust/test/Test/src/main.rs")})}), "label": String("Implement missing members"), "workspaceEdit": Object({"documentChanges": Array([Object({"edits": Array([Object({"newText": String("{\n fn bar(x: u64) -> i32 { todo!() }\n}"), "range": Object({"end": Object({"character": Number(1), "line": Number(6)}), "start": Object({"character": Number(15), "line": Number(5)})})})]), "textDocument": Object({"uri": String("file:///Users/ben/.vim/bundle/lsp-examples/rust/test/Test/src/main.rs"), "version": Null})})])})})]), "command": String("rust-analyzer.applySourceChange")}) }
our log:
2020-04-24 12:50:36,171 - DEBUG - TX: Sending message: b'Content-Length: 946\r\n\r\n{"id":2,"jsonrpc":"2.0","method":"textDocument/codeAction","params":{"context":{"diagnostics":[{"code":"E0046","message":"not all trait items implemented, missing: `bar`\\nmissing `bar` in implementation","range":{"end":{"character":14,"line":5},"start":{"character":0,"line":5}},"relatedInformation":[{"location":{"range":{"end":{"character":24,"line":1},"start":{"character":2,"line":1}},"uri":"file:///Users/ben/.vim/bundle/lsp-examples/rust/test/Test/src/main.rs"},"message":"`bar` from trait"},{"location":{"range":{"end":{"character":0,"line":6},"start":{"character":0,"line":6}},"uri":"file:///Users/ben/.vim/bundle/lsp-examples/rust/test/Test/src/main.rs"},"message":"implement the missing item: `fn bar(_: u64) -> i32 { todo!() }`"}],"severity":1,"source":"rustc"}]},"range":{"end":{"character":14,"line":5},"start":{"character":0,"line":5}},"textDocument":{"uri":"file:///Users/ben/.vim/bundle/lsp-examples/rust/test/Test/src/main.rs"}}}'
2020-04-24 12:50:40,425 - DEBUG - RX: Received message: b'{"jsonrpc":"2.0","id":2,"result":[{"command":{"arguments":[{"cursorPosition":{"position":{"character":4,"line":6},"textDocument":{"uri":"file:///Users/ben/.vim/bundle/lsp-examples/rust/test/Test/src/main.rs"}},"label":"Implement missing members","workspaceEdit":{"documentChanges":[{"edits":[{"newText":"{\\n fn bar(x: u64) -> i32 { todo!() }\\n}","range":{"end":{"character":1,"line":6},"start":{"character":15,"line":5}}}],"textDocument":{"uri":"file:///Users/ben/.vim/bundle/lsp-examples/rust/test/Test/src/main.rs","version":null}}]}}],"command":"rust-analyzer.applySourceChange","title":"Implement missing members"},"title":"Implement missing members"}]}'
2020-04-24 12:50:40,425 - DEBUG - Refreshing file /Users/ben/.vim/bundle/lsp-examples/rust/test/Test/src/main.rs: State is Open/action None
2020-04-24 12:50:40,425 - DEBUG - TX: Sending message: b'Content-Length: 610\r\n\r\n{"id":3,"jsonrpc":"2.0","method":"workspace/executeCommand","params":{"arguments":[{"cursorPosition":{"position":{"character":4,"line":6},"textDocument":{"uri":"file:///Users/ben/.vim/bundle/lsp-examples/rust/test/Test/src/main.rs"}},"label":"Implement missing members","workspaceEdit":{"documentChanges":[{"edits":[{"newText":"{\\n fn bar(x: u64) -> i32 { todo!() }\\n}","range":{"end":{"character":1,"line":6},"start":{"character":15,"line":5}}}],"textDocument":{"uri":"file:///Users/ben/.vim/bundle/lsp-examples/rust/test/Test/src/main.rs","version":null}}]}}],"command":"rust-analyzer.applySourceChange"}}'
2020-04-24 12:50:40,426 - DEBUG - RX: Received message: b'{"jsonrpc":"2.0","id":3,"error":{"code":-32601,"message":"unknown request"}}'
Ah yeah, most (if not all) of our code actions require support from the client (i.e. that it implements the rust-analyzer.applySourceChange command). I wonder whether we actually need that in most cases.
most (if not all) of our code actions require support from the client
:(
@flodiebold I apologize if this is off topic, but can you say more about that (or point me to more info about that)? I've just set up rust-analyzer with vim using vim-lsp, and I'm trying to test with the "fill match arms" code action. It fails with this error:
{'response': {'id': 39, 'jsonrpc': '2.0', 'error': {'code': -32601, 'message': 'unknown request'}}, 'request': {'id': 39, 'jsonrpc': '2.0', 'method': 'workspace/executeCommand', 'params': {'arguments': [{'label': 'Fill match arms', 'cursorPosition': ...
In the full error output I see the expected text of the match arms coming back, so this does seem to be a client issue as you alluded to, but I can't find more information about how this is supposed to work.
RA responds to textDocument/codeAction requests with a CodeAction containing a Command named rust-analyzer.applySourceChange. After this point is where the error comes from.
Your client is trying to handle that Command according to the specification:
- Make an
ExecuteCommand. - Catch all
ApplyEditserver-to-client requests and apply them.
That requires the server to implement ExecuteCommand for each of the named Commands that the server produces.
Instead, rust-analyzer expects clients to know about rust-analyzer.applySourceChange and implement a special case. To make it work in my rust-analyzer branch, I've done this:
if code_action_command[ 'command' ][ 'command' ] == 'rust-analyzer.applySourceChange':
code_action_command[ 'edit' ] = code_action_command.pop( 'command' )[ 'arguments' ][ 0 ][ 'workspaceEdit' ]
return self.CodeActionLiteralToFixIt( request_data, code_action_command )Except for the name, this is the exact same thing I had to do for java.apply.workspaceEdit. However there's one more thing. rust-analyzer.applySourceChange also contains the "place cursor at X:Y position" extension to WorkspaceEdit object, which I have completely ignored.