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

Support disabling function symbol comletion when there is snippet equivalent #638

Closed
kkharji opened this issue Nov 11, 2021 · 26 comments
Closed
Labels
enhancement New feature or request
Projects

Comments

@kkharji
Copy link

kkharji commented Nov 11, 2021

Is your feature request related to a problem? Please describe.
I'm always frustrated when with seeing duplication of symbols, and hate that fact that snippet is less prioritized in my editor.

For me instead of letf|<cr>, I have to letf|<down><cr>

Describe the solution you'd like

  • An option to filter out symbols that has snippet with the same name.
  • Doc for builtin symbols instead of snippet description.

Additional context
Add any other context or screenshots about the feature request here.
image
image
image

@kkharji kkharji added the enhancement New feature or request label Nov 11, 2021
@ericdallo
Copy link
Member

@tami5 I'd like to understand this one better, for me we show the elements before the snippets, at least for me on emacs:
Is this the same behavior your are seeing?
image

@kkharji
Copy link
Author

kkharji commented Nov 11, 2021

Is this the same behavior your are seeing?

Yes exactly, 9/10 what the user want is the snippet. it's rare that I'd want to complete and confirm a symbol by it's own. What I'm suggesting is an option to filter out function/macro symbol when there's a symbol with the same name.

@floke
this plugin went extra setup and made ALL lua function transformed to snippets which made writing lua code fun and intuitive. When the user confirm the function, the function argument becomes named $placeholders, e.g. function a(name, age) end; a|<cr> ===> a($name, $age)

Of course this would be tricky to implement because we'd need to take in consideration some cases:

  • Is in threading macro, if so remove the first/last argument and complete the rest,
  • What if the function has multiple arguments, easy the fewer arguments get prioritized.
  • Is inside (), ignore appending ()
  • Is inside (|arg1 arg2 arg3) then just symbol?

Symbol completion is cool, but completion for code structure is awesome. "tab" over to fill the blank 🤣 and that's it.

This would solve another issue I have when writing functions that are new to me, basically, I'd repeatedly press <c-s> which is bind to show the function signature, e.g. if a function has 4 params, I'd press <c-s> at least 2 times to get it right.

srry I keep jumping out scope 😆

@ericdallo
Copy link
Member

So what you want is snippets sorted first than symbols, not sure it makes sense remove the symbols since it's a general completion.
Also, In emacs we have custom shortcut to get only completion snippets, maybe you could have something similar for vim?
At last resource we could add a flag to prioritize snippets over other symbols, but not sure is a good alternative.

About the snippets enhancement, we could have some improvements on snippets maybe to add custom capabilities I think

@kkharji
Copy link
Author

kkharji commented Nov 12, 2021

So what you want is snippets sorted first than symbols, not sure it makes sense remove the symbols since it's a general completion.

Well I feel it's totally redundant to have them both, either symbol or snippet. In fact, it can be argued that it shouldn't be an option but perhaps a default behavior. For example, In rust-analyzer server, all function/method are snippets by default:

Screen Shot 2021-11-12 at 4 17 27 AM
Screen Shot 2021-11-12 at 4 19 40 AM

idk, I can't speak for all users, but an option should be a great start.

Also, In emacs we have custom shortcut to get only completion snippets, maybe you could have something similar for vim?

Hmm it's possible, but feels more like a hack, and definitely unnecessary one .

At last resource we could add a flag to prioritize snippets over other symbols, but not sure is a good alternative.

I've tried playing with prioritization of lsp kinds in cmp completion plugin, I gave up, couldn't get it right

About the snippets enhancement, we could have some improvements on snippets maybe to add custom capabilities I think

Oh god that will be amazing!!!, in rust-analyzer, it doesn't show that something is even a snippet, here's a preview
rust-lsp2

rust-lsp3

@ericdallo
Copy link
Member

Alright, I see your point, maybe for some key symbols like defn, if, let etc it doesn't make much sense to only complete to the symbol, but complete to the snippet, right?
I think we can start with a flag that doesn't return the core symbols but only the snippets indeed, if that works well, in the future we could turn on that flag by default, WDYT?

Oh god that will be amazing!!!, in rust-analyzer, it doesn't show that something is even a snippet, here's a preview

actually, it shows with a ~ at the end I think, just like we show with a $ at the end, right?
Can you confirm if the ~ is on the label of the completion item or is just something added by the vim LSP client when the kind of the completion is snippet?

@kkharji
Copy link
Author

kkharji commented Nov 12, 2021

Alright, I see your point, maybe for some key symbols like defn, if, let etc it doesn't make much sense to only complete to the symbol, but complete to the snippet, right?

Yes. These ones no one can argue it should return only symbol. because 9/10 they just duplicate the exact structure by hand.

I think we can start with a flag that doesn't return the core symbols but only the snippets indeed, if that works well, in the future we could turn on that flag by default, WDYT?

Yes, this the best way onward, maybe even suffixed with experimental. But we shouldn't scope it to core symbols only, and definitely it won't make sense to take the current approach of defining snippet, instead it should auto generate using something like (doc symbol) output, and when the user want to to have defn be something like (defn $action "$what-i-do" [$1] $2)), then defining it under current way should stop generating or using the generated one.

actually, it shows with a ~ at the end I think, just like we show with a $ at the end, right? Can you confirm if the ~ is on the label of the completion item or is just something added by the vim LSP client when the kind of the completion is snippet?

Yes, not sure if it's only in neovim, but anything that is expandable is followed by ~. With clojure-lsp I have both $ and ~ unlike other lsps. I'd suggest dropping $, but I don't know how important it is for current users on emacs.

But what I meant by it doesn't show that something is a snippet, is that the last part doesn't say [snippet] (kind) but the actual type of the item, [method] or [function], thus I think they are not sending a snippet, but possibly, some type of code action that get executed by the completion plugin 🤔

EDIT: I'm leaning towards code action because I see snippet kind being sent over, see
rust-lsp4

@ericdallo
Copy link
Member

ericdallo commented Nov 12, 2021

and when the user want to to have defn be something like (defn $action "$what-i-do" [$1] $2)), then defining it under current way should stop generating or using the generated one.

I don't understand why the current defn$ snippet is not enough, we do that:

defn$

Here you can find all snippets code: https://github.com/clojure-lsp/clojure-lsp/blob/master/src/clojure_lsp/feature/completion_snippet.clj#L10-L70

Yes, not sure if it's only in neovim, but anything that is expandable is followed by ~. With clojure-lsp I have both $ and ~ unlike other lsps. I'd suggest dropping $, but I don't know how important it is for current users on emacs.

It was something I added to differ from snippets, but I think it makes sense to drop it, we already have the kind to define that is a snippet.

EDIT: I'm leaning towards code action because I see snippet kind being sent over, see

They could be command which is what code actions call under the hood, but since the kind there is snippet, probably is just a snippet indeed.

@ericdallo ericdallo added this to Low priority in clojure-lsp via automation Nov 12, 2021
@ericdallo ericdallo moved this from Low priority to High priority (Probably next release) in clojure-lsp Nov 12, 2021
@kkharji
Copy link
Author

kkharji commented Nov 12, 2021

I don't understand why the current defn$ snippet is not enough, we do that:

I'd suspect some user would want to overwrite it to add the documentation part or other stuff.

I was trying to understand how rust-analyzer does it without defining a list of snippets, some relevant parts worth looking into,

https://github.com/rust-analyzer/rust-analyzer/blob/a8247685cfa09084bd620c0877ea1eb3d605d8a2/crates/ide_completion/src/lib.rs
https://github.com/rust-analyzer/rust-analyzer/blob/a8247685cfa09084bd620c0877ea1eb3d605d8a2/crates/ide_completion/src/patterns.rs
https://github.com/rust-analyzer/rust-analyzer/blob/a8247685cfa09084bd620c0877ea1eb3d605d8a2/crates/ide_completion/src/render.rs

They could be command which is what code actions call under the hood, but since the kind there is snippet, probably is just a snippet indeed.

I agree, somehow on enter, they are deleting text range and moving the cursor, e.g. path.| ==> let mut | = path. This is something I like to see with e.g. #funname ==> #(funame arg1| arg2 arg3) or ->varname| ==> (-> var |)/->func ==> (-> (func arg1| arg2)\n $0). basically, on top of generating func args as snippet, additional hacks to save keystrokes

@ericdallo
Copy link
Member

I agree, somehow on enter, they are deleting text range and moving the cursor, e.g. path.| ==> let mut | = path. This is something I like to see with e.g. #funname ==> #(funame arg1| arg2 arg3) or ->varname| ==> (-> var |)/->func ==> (-> (func arg1| arg2)\n $0). basically, on top of generating func args as snippet, additional hacks to save keystrokes

Move cursor also works with snippets, and it's way easier to implement then with code actions, maybe we just need to enhance the snippets with documentation about the symbol and things like that dynamically

ericdallo added a commit that referenced this issue Nov 14, 2021
@ericdallo
Copy link
Member

@tami5 I made a commit that should fix this, could you test it on master? we know replace the symbols with the equivalent snippets

@kkharji
Copy link
Author

kkharji commented Nov 14, 2021

Okay, managed to test it just now,
snippet-completion-test

Remarks:

  • println -> (println) is a simple hack I have in my editor to make any function kind wrapped with paran when confirmed.
  • The part after [] get distributed due to parinfer-rust integration "(defn ${1:foo} [$2]\n $0)", though with "(if ${1:test-expr}\n ${2:then-expr}\n ${3:else-expr})" worked okay, maybe avoid new lines in snippets? @eraserhd
  • Seems that completion got slower, or maybe because I'm running on the jvm, not sure. but there is definitely a performance hit

Missing:

  • Should return as kind as function?
  • Should have simple doc instead of detail field for defaults?

@ericdallo Should I rename this is issue to be more general "Improve Function Completion"?

@ericdallo
Copy link
Member

ericdallo commented Nov 14, 2021

I don't think we should remove new lines in snippets, they totally makes sense for a lot of snippets and works very well for most cases, probably something to fix on parinfer-rust?

The JVM certainly affects the performance vs the graalvm one, maybe you should compare with and without the latest commit?

Should return as kind as function?

I thought about that, but that doesn't makes sense since it's indeed a snippet kind, it could be misleading to people complete defn saying that is a function but it'd introduce a snippet without any feedback.

Should have simple doc instead of detail field for defaults?

Maybe we could replace the snippet detail with the symbol detail which I think usually is the docs
EDIT: just made a commit doing that

@kkharji
Copy link
Author

kkharji commented Nov 14, 2021

I don't think we should remove new lines in snippets, they totally makes sense for a lot of snippets and works very well for most cases, probably something to fix on parinfer-rust?

I agree, but a found a proper workaround: Anything with new line should be followed by named placeholder, $0 breaks it, maybe body WDYT? Also, this won't matter when any function inherits the args from the doc, right, I think all of them is named

The JVM certainly affects the performance vs the graalvm one, maybe you should compare with and without the latest commit?

Yep, its the JVM

Should return as kind as function?

I thought about that, but that doesn't makes sense since it's indeed a snippet kind, it could be misleading to people complete defn saying that is a function but it'd introduce a snippet without any feedback.

hmmm, I don't know about that, it will still show ~ at the end to indicate that it's a snippet, like with rust-analizer. Also, when all function types becomes snippets, i.e. auto expand based on the discussion we had before, then the server will mostly return snippet type, which by it self is missing leading.

Should have simple doc instead of detail field for defaults?
Maybe we could replace the snippet detail with the symbol detail which I think usually is the docs EDIT: just made a commit doing that

AWESOME, testing it out now

EDIT: Missing all the default stuff
Screen Shot 2021-11-14 at 6 02 49 PM
Screen Shot 2021-11-14 at 6 02 39 PM

@ericdallo
Copy link
Member

hmmm, I don't know about that, it will still show ~ at the end to indicate that it's a snippet, like with rust-analizer

Almost sure it will show the ~ only when the kind is a snippet, also other editors don't have this ~ thing, so it's a neovim thing.
I'll double check the LSP spec

Missing all the default stuff

Could you elaborate?

I agree, but a found a proper workaround: Anything with new line should be followed by named placeholder, $0 breaks it, maybe body WDYT? Also, this won't matter when any function inherits the args from the doc, right, I think all of them is named

I'll double check the spec, is not fresh on my mind how the snippets works, but just to make it clear, we are not changing the snippet based on the symbol yet, so they are all static, in the future we could replace the snippet args with the function args, but I'd leave to another issue as is not that easy

@kkharji
Copy link
Author

kkharji commented Nov 14, 2021

Missing all the default stuff

Could you elaborate?

e.g. description, examples from clojure docs

I agree, but a found a proper workaround: Anything with new line should be followed by named placeholder, $0 breaks it, maybe body WDYT? Also, this won't matter when any function inherits the args from the doc, right, I think all of them is named

I'll double check the spec, is not fresh on my mind how the snippets works, but just to make it clear, we are not changing the snippet based on the symbol yet, so they are all static, in the future we could replace the snippet args with the function args, but I'd leave to another issue as is not that easy

KK

@ericdallo
Copy link
Member

ericdallo commented Nov 14, 2021

I'm not on the PC right now, but I need to check the spec docs, I'm not sure that the detail is the correct place to put those things

@ericdallo
Copy link
Member

I just checked that there is the field documentation: https://microsoft.github.io/language-server-protocol/specifications/specification-3-17/#completionItem and we already fill it, I'll fix it to make snippets show them as well

@ericdallo
Copy link
Member

@tami5 I just found that I didn't have the completion popup enabled on my emacs 😅 I enabled and I'm fixing the issue:
image

@kkharji
Copy link
Author

kkharji commented Nov 14, 2021

@tami5 I just found that I didn't have the completion popup enabled on my emacs 😅 I enabled and I'm fixing the issue:

image

😍 Awesome we're getting closer

@ericdallo
Copy link
Member

@tami5 I think everything is good now, could you please test it master again?

@kkharji
Copy link
Author

kkharji commented Nov 14, 2021

Works flawlessly, I'd add before closing this issue is to modify the snippets that has new lines and $0 with $body or something descriptive.

@ericdallo
Copy link
Member

ericdallo commented Nov 14, 2021

Works flawlessly, I'd add before closing this issue is to modify the snippets that has new lines and $0 with $body or something descriptive.

This break things, $0 is part of the snippet LSP spec where it tells to client that tabstop number, if we change that we loose the tabstop: https://microsoft.github.io/language-server-protocol/specification#snippet_syntax
EDIT: I found we can achieve that with ${0:body}, it should keep the tabstop + fix your issue, will do

@kkharji
Copy link
Author

kkharji commented Nov 14, 2021

EDIT: I found we can achieve that with ${0:body}, it should keep the tabstop + fix your issue

Yah also, we can keep $0 as last place outside ()$0. I'm testing overwrite now. EDIT: seems broken: config

    {:use-metadata-for-privacy? true
     :clean {:ns-inner-blocks-indentation :same-line}
     :text-document-sync-kind :full
     :auto-add-ns-to-new-files? true
     :hover {:hide-file-location? true}
     :notify-references-on-file-change true
     :additional-snippets [{:name "wrap-let"
                            :detail "Wrap in let sexpr"
                            :snippet "(let [$1]\n $0$current-form)"}
                           {:name "defn"
                            :snippet "(defn%s $\{1:name} [$2]\n  $\{3:body})$0"}]

     }

@ericdallo
Copy link
Member

I made one more commit improving the snippets and adding descriptive names to $0

@kkharji
Copy link
Author

kkharji commented Nov 14, 2021

Okay much better now. I think this resolves this issue, I will move parts that ware out of the scope to a new issue later. Thanks @ericdallo for your amazing work 🙏

@ericdallo
Copy link
Member

Good, glad to fix it, thanks for the help on improving it

clojure-lsp automation moved this from High priority (Probably next release) to Next release Nov 14, 2021
@ericdallo ericdallo moved this from Next release to Done in clojure-lsp Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

No branches or pull requests

2 participants