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

Add LSP plugin #1331

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add LSP plugin #1331

wants to merge 9 commits into from

Conversation

techee
Copy link
Member

@techee techee commented Apr 22, 2024

This is a work-in-progress PR to add the LSP plugin. There are still some things missing like the documentation but in general the plugin should work. There are 3 modes of operation:

  1. Without Geany LSP support
  2. With basic LSP support using Plugin extensions (aka LSP API) geany#3849
  3. With full LSP support using Complete plugin extensions patches geany#3850

The plugin should detect with which of these modes Geany was compiled and adjust itself automatically.

For the time being I still plan continue the main development of the plugin under https://github.com/techee/geany-lsp and sync the changes here from time to time.

@techee
Copy link
Member Author

techee commented May 10, 2024

@frlan @b4n @eht16 The major things are "finished" from my perspective. If I missed something that needs to be done regarding geany-plugins integration, please let me know.

The word "finished" above means the plugin works but soft-depends on some things which are not in Geany yet, it's:

The missing signals cause some warnings when started from the command-line.

Also, having some official support for LSP in Geany like geany/geany#3849 would improve the user-friendliness and usefulness of the plugin.

I also plan to perform some more testing of the plugin and fix possible bugs but this should be independent of the string freeze if we want to make a Geany release soon.

@techee techee changed the title [WIP] Add LSP plugin Add LSP plugin May 10, 2024
@techee
Copy link
Member Author

techee commented May 26, 2024

@b4n Ping :-)

I think I'm pretty much done with the plugin and I'd like some feedback. I tried to address most of the complaints you had in geany/geany#3571 (comment):

The config file now contains these settings with these default values:

# Defines whether the plugin should be enabled automatically for new or existing
# projects (that have not yet been configured to use LSP). This configuration
# is only valid in the [all] section
enable_by_default=false
# Defines whether the server should be used when no project is open. Servers
# may not work correctly without a project because most of them need to know
# the path to the project directory which corresponds to the path defined under
# Project->Properties->Base path
use_without_project=false
# Defines whether the server should be used for files whose path is not within
# the project directory
use_outside_project_dir=false

This means that unless you explicitly enable LSP for the project, it isn't used. Similarly, files outside the project directory or plain non-project Geany use TM by default. No LSP unless you enable it.

On the other hand, I disagree there should be a fallback to TM when e.g. clangd isn't installed but LSP is enabled for the project - in this case it should be pretty clear something is wrong and falling back to TM would just hide the real problem.

I don't think clangd will ever work without a project - it really just compiles it behind the scenes - so testing it on individual files won't work and better to use TM for those.

I added

# Semicolon-separated glob patterns specifying files for which diagnostic
# messages are not shown. Useful when the server has a problem with some files
diagnostics_disable_for=*/scintilla/*/*.h

config option so diagnostic messages can be disabled for some files like the Scintilla headers (apart from diagnostic messages I believe other things work fine also for these headers).

This PR allows using the plugin without any Geany support but IMO this sucks. One has to disable TM parsing in filetype configuration and then it's all or nothing, one cannot use e.g. one feature using TM and one using LSP or one project (or just single file) using TM (without having to change the configuration). I'm still hoping we find some API that is acceptable for everyone, see geany/geany#3849 for my current proposal.

So when testing, I still recommend using the combined https://github.com/techee/geany-lsp repository.

Also, when trying the plugin, please delete ~/.config/geany/plugins/lsp/lsp.conf first - I made some changes to the config file and the previous version will be incompatible.

@elextr
Copy link
Member

elextr commented May 26, 2024

I don't think clangd will ever work without a project - it really just compiles it behind the scenes - so testing it on individual files won't work and better to use TM for those.

Well, although in theory it is possible to not have compile_commands.json but really clangd is a pain without, so not only will it not work on isolated files, it won't work on any build system that doesn't make a compile_commands.json like Meson, Cmake, or other modern ones do automatically. For others use of bear is supposedly possible.

So the restrictions of the default settings seems fine.

Just to confirm geany-lsp is Geany with #3849 and this plugin, all previous versions removed? If so I think I'll just get a new clone [end lazy git user] :-)

As I said elsewhere I am running behind in getting to be able to try these, but real "soon" now I hope.

Signed not-b4n :-)

@techee
Copy link
Member Author

techee commented May 26, 2024

Just to confirm geany-lsp is Geany with #3849 and this plugin, all previous versions removed? If so I think I'll just get a new clone [end lazy git user] :-)

It's:

  1. Geany
  2. This PR
  3. #3850, i.e. including the symbol tree patches. But you can easily simulate Plugin extensions (aka LSP API) geany#3849 by setting document_symbols_enable=false
  4. Fix emission of document-activate signal and various related GUI glitches geany#3870
  5. The patch from https://sourceforge.net/p/scintilla/bugs/2403/ which is merged upstream and we can either get it by updating Scintilla or backporting to Geany (just a few lines of code)

@elextr
Copy link
Member

elextr commented May 27, 2024

Does it include #3865?

@techee
Copy link
Member Author

techee commented May 27, 2024

Does it include #3865?

No, it doesn't, but you don't need to disable the ctags parser for geany-lsp as it contains the proper LSP interface.

@techee
Copy link
Member Author

techee commented Jun 7, 2024

@b4n I added this feature to the plugin that other LSP clients implement as well (not tested much yet):

https://github.com/techee/geany-lsp/blob/c56de66cecf4dff2c1b1a98ea37b22bd2af73f03/plugins/lsp/data/lsp.conf#L56-L68

This should essentially allow automatic opening/closing of multiple projects even if no Geany project is open and at least partially fix the problem you had when opening random files.

@techee techee force-pushed the lsp branch 3 times, most recently from 5fba42b to 831c3ac Compare September 11, 2024 08:58
@techee
Copy link
Member Author

techee commented Sep 11, 2024

Alright, I've pushed the latest version here with squashed commits. Apart from bugfixes (if some bugs appear), I don't plan anything more for this release.

Please let me know what needs to be done to get the plugin merged to geany-plugins.

@elextr
Copy link
Member

elextr commented Sep 12, 2024

Please let me know what needs to be done to get the plugin merged to geany-plugins.

Good question, plugins maintainer was @frlan but he hasn't been around much so I'm not sure if he doesn't have time any more. So somebody else maybe, @b4n, @eht16 ?

@eht16
Copy link
Member

eht16 commented Sep 21, 2024

I will not do a code review but I intend to use the plugin with my work project which has a huge Python code base and I'm very curious how it will work and maybe I'm going to switch to the LSP approach completely but no promises yet :).

Though this might take a few weeks to complete as I need to get in touch with LSPs and find my way through the config.
But I already like a lot that the config file is well documented!

@techee
Copy link
Member Author

techee commented Sep 21, 2024

I will not do a code review but I intend to use the plugin with my work project which has a huge Python code base and I'm very curious how it will work and maybe I'm going to switch to the LSP approach completely but no promises yet :).

I'm really interested in your feedback - some things may just be limitations of particular servers but some things can possibly be improved in the behavior of the plugin or new config options could possibly be added to control the plugin's behavior if some servers behave differently.

I mostly tested the plugin with the clangd server and not so much with Python and other pre-configured servers which I tested only lightly. I found pylsp a little too slow so I'd recommend pyright or jedi-language-server. If you only want to use a linting language server, ruff might be a good option as it's super-fast. There has been some discussion about various Python language servers in

techee/geany-lsp#51

Also, it's always possible to disable a particular feature and fall back to Geany's default TM implementation.

Though this might take a few weeks to complete as I need to get in touch with LSPs and find my way through the config.

It's actually really simple and you don't have to study anything too much. Just install the server you want to use, check its documentation regarding how to start it and whether it needs some special command-line options and enter this to the cmd config file option. Then just follow

https://github.com/techee/geany-lsp?tab=readme-ov-file#quick-start

Some servers require the project's root directory, some don't. There are some options at the beginning of the config file you can use to configure the plugin so it runs without Geany projects if you don't want to use those.

Also, I recommend getting the plugin from https://github.com/techee/geany-lsp and not this PR as I still keep working on some stuff and sync the changes to this PR infrequently.

@techee
Copy link
Member Author

techee commented Sep 24, 2024

Alright, I got some more experience with python language servers while developing

https://github.com/techee/lsp-proxy

  1. I don't like pylsp much - seems slow and kind of buggy
  2. pyright-langserver seems to be very good but requires type annotations, otherwise it tends to report false positive errors - but I'm sure this is possible to configure by a config file where it should be possible to specify a less strict behavior.
  3. jedi-language-server also seems to be good - it doesn't do such an advanced type analysis as pyright but on the other hand it doesn't suffer from false positives because of this
  4. ruff is super fast, usable just for linting and can be a good supplemental language server to one of the three servers above - using multiple servers can be achieved by the lsp-proxy project above

The difference between 2 and 3 can be seen in code like

foo = ""
bar = foo + 1

where pyright reports an error about adding a string and a number while jedi-language-server doesn't report anything.

@Johnmcenroyy
Copy link

Johnmcenroyy commented Sep 25, 2024

@techee Also wanted to add some info about python's lsp

                   autocompletion  linter/formatter   type checker    refactoring         language
pylsp                 V(jedi)         V(ruff)          V(mypy)        V(rope,advanced)    python
jedi-language-server  V(jedi)         -                -              V                   python
ruff server           -               V(ruff)          -              -                   rust
pyright               V               -                V              V                   python/typescript
pylyzer               V               -                V              V                   rust


Type checkers are                 lsp server               
mypy    (python community)           -    seems most feature rich
pytype  (google)                     -    can work with code without annotations
pyright (microsoft)                  V    up to 3x to 5x faster than mypy
pyre    (facebook and instagram)     V
pylyzer (rust)                       V    up to 100x faster than pytype and pyright

Pylyzer is very new and in development.

I like very much jedi-language-server with ruff server (NOT ruff-lsp - this will be deprecated and written in python)
More info about ruff server: https://astral.sh/blog/ruff-v0.4.5

@elextr elextr mentioned this pull request Oct 3, 2024
9 tasks
@elextr
Copy link
Member

elextr commented Oct 25, 2024

Have tried this with some C++ with clangd and very limited Python pyright/pylsp. I havn't tried any other languages.

Nothing has crashed or atrocious behaviour. The servers themselves have various annoying behaviours, but thats not part of this plugin AFAIK.

One problem is it needs to try to switch to using json-glib and jsonrpc-glib as dependency packages not included source code. I will note some distros (I suspect Debian for eg) will require that or they might not add the plugin due to the included source.

Does the build script drop the source code if the dependency packages are available? That might make Debian/others happy but still have code for people using older systems?

@techee
Copy link
Member Author

techee commented Oct 27, 2024

One problem is it needs to try to switch to using json-glib and jsonrpc-glib as dependency packages not included source code.

The reason is that there's this performance problem

https://gitlab.gnome.org/GNOME/json-glib/-/merge_requests/60

which got only fixed in the latest 1.10 release which was released 1 month ago and probably isn't part of many distributions yet. I was hoping to use the bundled version until the latest dependencies get to more distros.

Since the performance problem only affects semantic tokens as far as I know (but now thinking about it, it could also be document symbols for the symbol tree), one possibility would be to ifdef-out semantic token support for older json-glib versions.

I will note some distros (I suspect Debian for eg) will require that or they might not add the plugin due to the included source.

Is there some policy preventing this? Couldn't I just claim these are the sources of the plugin? Scintilla and ctags are also bundled with Geany (and, well, it's true that Scintilla is supposed to be used this way and there's no official library version of ctags).

Does the build script drop the source code if the dependency packages are available?

There's nothing like that (yet). But would it be OK for Debian to fall back to the bundled version in this case?

@elextr
Copy link
Member

elextr commented Oct 27, 2024

got only fixed in the latest 1.10 release which was released 1 month ago and probably isn't part of many distributions yet

Yeah, it would be a while before distros support that version.

Is there some policy preventing this?

I don't know Debian policies, too complex, don't care.

My concern was from another project that I was reading, that was blocked because it included source for a library that was available on Debian. I am afraid I don't remember if the source had changes, if they were accepted upstream or not, and what the final solution was, I stopped following it since it all seemed too hard.

Neither Scintilla nor ctags libraries appear to be in the Debian repository (at least not in Linux Mint Debian), so included source is fine.

But would it be OK for Debian to fall back to the bundled version in this case?

Debian expert advice somebody please?

But I would think it would be acceptable, you still need the source for Win and Mac, and so if the build only builds the source instead of depending on the library on Win, Mac, and Linux distros with library less that 1.10 or whatever version. This information should be clearly documented so that Debian and other packagers understand it is temporary until they have a suitable library version.

I presume jsonrpc-glib needs to use the matching version of json-glib and that is why its source is included too.

@techee
Copy link
Member Author

techee commented Oct 28, 2024

I presume jsonrpc-glib needs to use the matching version of json-glib and that is why its source is included too.

I don't think this is the case but if I used the system jsonrpc-glib, it would use the system (possibly old) json-glib and not the one statically linked to the plugin.

@techee
Copy link
Member Author

techee commented Dec 11, 2024

As Lex suggested, I implemented the check whether a jsonrpc-glib and json-glib of the required version are installed and if so, use these instead of the builtin ones. It is also possible to enforce this behavior using --enable-system-jsonrpc configure option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants