clojure-lsp / clojure-lsp-intellij

Intellij Plugin for Clojure & ClojureScript development via Language Server (LSP) made in Clojure
https://clojure-lsp.io
MIT License
76 stars 5 forks source link

What about using LSP4IJ? #53

Open angelozerr opened 2 weeks ago

angelozerr commented 2 weeks ago

I noticed that you have implemented your own lsp support, but I would like to notify you that it exists now a free LSP support for IntelliJ https://github.com/redhat-developer/lsp4ij

You can read an article about LSP4IJ at https://idetools.dev/blog/lsp4ij-announcement/

Hope you could be interested with LSP4IJ and don't hesitate to answer me if you have any questions.

Thanks!

ericdallo commented 2 days ago

Hey @angelozerr, thanks for bringing that, I was not aware of that and it's awesome to see free open source alternatives like that one! I'll take a closer look, but some things I noticed:

When I created this plugin, the idea is to provide LSP support but in a way intellij users don't need to setup nothing, like just works out of the box, with minor customization as possible, is it possible to have a plugin that uses lsp4ij under the hood and have all those things configured already out of the box? Most Clojure Intellij users came from Cursive which is a great Clojure paid plugin, and works really well out of the box, so I'd change this plugin base only if it could have the same things and work out of the box as possible

angelozerr commented 2 days ago

When I hold Ctrl, and hover over a symbol, I see the whole buffer text underlined, seems a little bit weird

I think it is the same issue than https://github.com/redhat-developer/lsp4ij/issues/98 It is because if you have use just TextMate or TEXT language, the PsiFile is not tokenized and the range is the full file content. It is badly not possible to customize that with gotoDeclarationHandler extension point -( But if you have a plugin which tokenized the PsiFile with PsiElement you will not have this problem.

Do you support semantic tokens?

I have pushed last hours this support, see doc at https://github.com/redhat-developer/lsp4ij/blob/main/docs/UserGuide.md#semantic-tokens-support but it is experimental and it seems that there is a CPU issue https://github.com/redhat-developer/lsp4ij/issues/394 but if you want to play with this feature you can install nightly build https://github.com/redhat-developer/lsp4ij?tab=readme-ov-file#testing-nightly-builds

I have played with clojure lsp and it seems it colorizes some tokens.

When I click on a code lens I see this, which I'm not entirely sure what means:

It means that language server report Codelens with command code-lens-references, you can see those CodeLens by using LSP console https://github.com/redhat-developer/lsp4ij/blob/main/docs/UserGuide.md#lsp-console

If language server doesn't provide this command, it tries to search if it exists a command on client side, if this command doesn't exist, you have this popup error and you have a link which suggest to implement an IJ plugin which register the expected command.

I think you have implemente this LSP command in your plugin, right?

clojure-lsp-intellij has a good feedback during initialization which I don't see easily, I need to click on LanguageServers and even so have logs enabled, check the $/progress notifications

$/progress has been implemented with IntelliJ Backgroind Task, see https://github.com/redhat-developer/lsp4ij/blob/main/docs/LSPSupport.md#progress-support

What is your suggestion to improve that?

When I created this plugin, the idea is to provide LSP support but in a way intellij users don't need to setup nothing, like just works out of the box, with minor customization as possible, is it possible to have a plugin that uses lsp4ij under the hood and have all those things configured already out of the box?

Yes! It is the goal of LSP4IJ that we use https://github.com/redhat-developer/intellij-quarkus and we have the luck that some projects are based on LSP4IJ https://github.com/redhat-developer/lsp4ij?tab=readme-ov-file#who-is-using-lsp4ij

See the section of our article https://idetools.dev/blog/lsp4ij-announcement/#how-to-integrate-your-language-server-in-an-intellij-plugin

It seems that you have played with https://github.com/redhat-developer/lsp4ij/blob/main/docs/user-defined-ls/clojure-lsp.md which is working just with few settings and you don't need to write an IJ plugin, but it is better to develop an IJ plugin:

In other words, my suggestion is to update your IJ plugin, by removing all LSP features and based on LSP4IJ and register the Clojure LSP with the LSP4IJ server extension point https://github.com/redhat-developer/lsp4ij/blob/main/docs/DeveloperGuide.md

The goal of your plugin is to:

ericdallo commented 1 day ago

@angelozerr thanks for the explanation, it completly makes sense! I wish we had that plugin when I started developing this one hehe, but I agree it makes sense to leave the LSP logic to LSP4IJ indeed, especially because of semantic tokens which I know is hard to support on client side, it's just that it's a huge transition we'd need to make carefully, especially because there are lots of people already using clojure-lsp-intellij plugin.

But if you have a plugin which tokenized the PsiFile with PsiElement you will not have this problem.

clojure-lsp-intellij already has a language parser for Clojure, why that still happens?

So, just summarizing some things I have in mind so we can be aligned:

ericdallo commented 1 day ago

BTW, really cool project the LSP4IJ, it remembers me when we extracted clojure-lsp logic to https://github.com/clojure-lsp/lsp4clj 😄

angelozerr commented 1 day ago

clojure-lsp-intellij already has a language parser for Clojure, why that still happens?

How have you tested LSP4IJ,with your plugin? If you did that you should have duplicate completion (one from your pluin, and one from LSP4IJ), right?

I still need to test each existing feature and check it's working or the most important ones are working

Exactly, when I discovered your project, I have spend some time to check the feature that you have implemented before annoying you and I think a feature that you will loose is the code action available from the refactor menu. I have not implemented this feature. Perhaps LSP4IJ will need to provide some customization, I don't know, I'm waiting for feedback from adaptor like you if you decide to migrate to LSP4IJ.

we would need to launch a 3.0.0 (next major version) of clojure-lsp-intellij, which would delegate all LSP connection to LSP4IJ

Indeed it will be more careful if you decide to adopt LSP4IJ.

I wanna a way to download clojure-lsp automatically the same way it does ATM, is there any way to have this logic working and after downloaded manually trigger the LSP start like we do today?

Yes, LSP4IJ provide an API to enable/disable and start/stop language server. See https://github.com/redhat-developer/lsp4ij/blob/main/docs/DeveloperGuide.md#language-server-manager

about the code-lens-reference, got it, it should be easy to support it.

I think.Please note that it exists the default command https://github.com/redhat-developer/lsp4ij/blob/main/docs/DeveloperGuide.md#editoractionshowreferences perhaps you could declare you action by using the class of this action?

we would need to fix this underlined issue

Also, there is a behavior I'd like you to consider supporting, which is when Ctrl+click on a definition, we show the references, it's something I brought from Cursive which Intellij users use a lot instead the need to manually triggering the find-references shortcut, WDYT?

Today Ctrl+Click is associate to the textDocument/definition, you woule like to associate to textDocument/references? I would like to avoid doing that because references can be expensive, but if you need it we could provide a customization or perhaps you could implement on your side the GoToDeclarationHandler with references. The main thing to do is to create issues on LSP4IJ to discuss about that

ericdallo commented 1 day ago

How have you tested LSP4IJ,with your plugin? If you did that you should have duplicate completion (one from your pluin, and one from LSP4IJ), right?

I disabled my plugin, enabled yours and restarted intellij

a feature that you will loose is the code action available from the refactor menu.

TBH, That's not a core feature, if code actions are provided via the context menu (lightbulb), that's its good enough. It'd be nice though to support that in the future

provide an API to enable/disable and start/stop language server.

awesome, so I think that will keep working as expected

perhaps you could declare you action by using the class of this action?

Ah, yeah, that should be enough!

Today Ctrl+Click is associate to the textDocument/definition, you woule like to associate to textDocument/references?

Yeah, first time I heard about that I disliked too, but I understood later, the point is that if you are already on the definition, you don't want to go to definition, but the references of it, so users can use the same shortcut to go and come back. IMO would be nice to have this in the core without the need to support any customization, but if you disagree, a customization would be really useful.

I'll find some time during the next week to deep dive and check if there are other bugs, thanks again for raising this.

angelozerr commented 1 day ago

I disabled my plugin, enabled yours and restarted intellij

Ok so it means that you have not a PsiFile for clojure, right?

TBH, That's not a core feature, if code actions are provided via the context menu (lightbulb), that's its good enough.

It is and with fast performance (it should)

It'd be nice though to support that in the future

Please create a new ossue for that with your detailled idea (ex : do you want some customization of the icon,etc)

IMO would be nice to have this in the core without the need to support any customization, but if you disagree, a customization would be really useful.

Let's create a new issue to discuss about that and see the feedback from the community.

I'll find some time during the next week to deep dive and check if there are other bugs, thanks again for raising this.

Great!

ericdallo commented 1 day ago

Ok so it means that you have not a PsiFile for clojure, right?

Ah makes sense 😅, so if we had clojure-lsp-intellij using lsp4ij and defining a Parser like it does today, that would work, right? BTW, with lsp4ij, would be required to change anything related to Langauge declaration, like parser, lexer etc?

Please create a new ossue for that with your detailled idea (ex : do you want some customization of the icon,etc)

Will do!

angelozerr commented 1 day ago

Ok so it means that you have not a PsiFile for clojure, right?

Ah makes sense 😅, so if we had clojure-lsp-intellij using lsp4ij and defining a Parser like it does today, that would worinspecter?

Yes!

BTW, with lsp4ij, would be required to change anything related to Langauge declaration, like parser, lexer etc?

No LSP4IJ works with or without a PsiFile

Please create a new ossue for that with your detailled idea (ex : do you want some customization of the icon,etc)

Will do!