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

What about using LSP4IJ? #53

Open
angelozerr opened this issue Jun 18, 2024 · 13 comments
Open

What about using LSP4IJ? #53

angelozerr opened this issue Jun 18, 2024 · 13 comments
Labels
enhancement New feature or request

Comments

@angelozerr
Copy link

angelozerr commented Jun 18, 2024

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!

@angelozerr angelozerr added the enhancement New feature or request label Jun 18, 2024
@ericdallo
Copy link
Member

ericdallo commented Jul 1, 2024

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 hold Ctrl, and hover over a symbol, I see the whole buffer text underlined, seems a little bit weird
  • Do you support semantic tokens?
  • When I click on a code lens I see this, which I'm not entirely sure what means:
    image
  • 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:
    image

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
Copy link
Author

angelozerr commented Jul 1, 2024

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 redhat-developer/lsp4ij#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 redhat-developer/lsp4ij#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:

  • to embed the clojure-lsp in the plugin
  • provide required LSP command like the code-lens-references command.

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:

  • embed default clojure lsp
  • provide an action to download last version of clojure lsp
  • implement required LSP command like code-lens-references command

@ericdallo
Copy link
Member

@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:

  • I still need to test each existing feature and check it's working or the most important ones are working
  • we would need to launch a 3.0.0 (next major version) of clojure-lsp-intellij, which would delegate all LSP connection to 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?
  • about the code-lens-reference, got it, it should be easy to support it.
  • 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?

@ericdallo
Copy link
Member

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

@angelozerr
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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!

@angelozerr
Copy link
Author

@ericdallo just for your information, we will release 0.6.0 next week and we provide a cool new feature with document symbol.

Here a screenshot with clojure:

image

@ericdallo
Copy link
Member

Awesome!
I still need to check the feature parity with current features so we can consider switch to LSP4IJ

@angelozerr
Copy link
Author

Awesome!

Glad it pleases you!

For some shortcut, I think it will not the same thing than you, since I have not found a solution to override the Action menu for Find implementation for instances https://github.com/redhat-developer/lsp4ij/blob/main/docs/LSPSupport.md#implementation

I had to provide new shortcut. I don'tunderstand how you have manage this usecase without breaking this feature for standard IntelliJ plugin (ex : Java plugin which doesn't use LSP).

@angelozerr
Copy link
Author

angelozerr commented Nov 25, 2024

@ericdallo I have started the 0.8.0 release of LSP4IJ (it will be available in few days by waiting for approvment from JetBrains) which provides type + call hierarchy. Here a sample with clojure:

image

See https://github.com/redhat-developer/lsp4ij/blob/main/docs/LSPSupport.md#call-hierarchy to know how to integrate call hierarchy with clojure since you are using a custom clojure language

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
None yet
Development

No branches or pull requests

2 participants