> I think understand the intention of the LSP interfaces. That's why I'm pretty > clear that they cannot be used itself for implementing goto-definition and > tooltip on the symbols sidebar. These functionalities in Geany have to be > implemented using other LSP interfaces or the TM fallback.
But this is not how this discussion started, just re-read your response here: https://github.com/geany/geany/pull/3571#issuecomment-1793149958. If it's clear now, great. > But this is essentially what I want that we do generally. I looked at your > code in more detail and you're actually doing what I'm asking for all the > time: you're generating TMTags from the LSP data. Now can we not go one step > further and not only attach these tags to the sidebar menu items but to the > GeanyDocument itself? I understand your point but there are many related problems: 1. What do you want to take as the source of truth - tag manager, or LSP server? E.g., if one tag is present using tag manager and not when using LSP server, what is correct? Did the tag manager misparse the file and inserted an incorrect tag or did the LSP server misparse it and forgot about the tag? Or replace all tags from a file with the LSP ones (losing TMTag info which, as I understand, is not what you want). 2. You cannot be sure that the LSP server parses correctly files that are not part of the project. For instance, clangd depends on `compile_commands.json` you get from meson - other files might compile somehow but you get errors. Now tag manager has a file-based API like `tm_workspace_add_source_file()` but you cannot do something like that with LSP. So the result will be a mix of tags, some coming from TM and some from LSP in the tag manager and I'm not sure if this is what we want. 3. Now if you wanted to replace all symbols in TM and not just those in the currently edited file, you'd probably have to call https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_symbolResolve and place symbols from there to the TM. But when some of the workspace files change (e.g. after doing `git pull`), you won't get any notification from the LSP server and you get outdated tags in the database. (I think this is an important point that you still don't completely get - you still want to have LSP tags cached locally but this is not how LSP works. Tags are placed in the server and you perform the queries only when you need some functionality so you always get up-to-date results.) 4. When you have a look what I did for the symbol tree (and note, I'm not proud of it and I definitely want some feedback here), I placed `detail` from LSP to `arglist` of TMTag and LSP `kind` to `type` in TMTag and process them differently in the symbol tree than TM tags. We already discussed the `detail` problem which isn't semantically equal to `arglist` in TM and would require further processing depending on the language server used. Now there's also the `kind` problem: we can't map it to arbitrary TMTagType because we have hard-coded mappings here https://github.com/geany/geany/blob/b02ee9a95a48690836f08ba10ccc9db89ee89c10/src/tagmanager/tm_parser.c#L120 which tell us e.g. what icon to use for the given TMTagType or to which group they belong in the sidebar. This is language-specific and if we wanted to map LSP kind to TMTagType, we'd have to do it in a language-specific way which would introduce another set of tables and this is something I'd like to avoid. So instead, what I did was that I placed `kind` to `type` and introduces a fixed mapping `kind->icon` that is used for all languages with special code for this in the symbol tree. And also, disabled "group by type" when LSP is used. Solving the above points would be really hard and actually impossible for some things. So I'm still not convinced that updating TMTags in TM is the way to go. > And please don't re-iterate the "documentSymbol" support is not guaranteed. > To me this is basic functionality for any language server. If that's not > provided the server is worthless (we couldn't have the symbols sidebar at > all!). Please show me one server that does not provide it at all. Just have a look here: https://langserver.org lots of language servers work as linters or code formatters only. That said, these don't matter to us symbol-wise. I was only mentioning that before because you seemed to insist that goto symbol and friends should be implemented only using symbols, but we are past that I hope :-), > I want to get a feeling if the documentSymbol interface itself is lacking or > if existing servers aren't good enough. I would call it more "incompatible interface" to what we do with TMTags, see point (4) above. We for instance have `var_type` and `arglist`, LSP has `detail` which is so vaguely described that one cannot be sure what we'd get from it. It's enough to show it as a tooltip in the symbol tree but we cannot be sure we derive `var_type` and `arglist` from it and this would need server-specific parsing of the field. Similar problem with the `kind` discussed above. This again comes to the way LSP works and that its features are intended to be used - the interface is meant for you to show the symbols e.g. in a symbol tree or some popup window, filter it and allow you to jump to the specific place, but they are not meant to be used for e.g. some client-side autocompletion. > Speaking about superior ctags parsing: I understand your implementation is > susceptible for this scenario for the sidebar use cases: If the language > server does not provide detail in documentSymbol, then we won't have a > tooltip. Correct. But you can always disable LSP for this particular feature if you run into some shitty LSP server where our ctags parsers work better. > Similarly, if the server does not provide range/location then we won't have > goto-definition. Notice that there's no `?` at these members so these are mandatory for the LSP interface. > Now if we would have that information in the TM fallback because the parser > recorded this information in doc->tm_file->tags_array, then these two use > cases are effectively regressed. I don't have a feeling how realistic this is > because I don't have a good picture about the quality of server > implementations, but generelly speaking this is a situation that I want to > avoid if possible as it would make our LSP support look poor compared to the > status quo, even if it excels in other use cases. Basically it's up to the user to evaluate the individual language servers. If they suck, they'd be probably happier with our TM implementation and could disable e.g. the `documentSymbols` feature for that particular language server and will get pure TM as the fallback. I'm really worried that if you somehow merge the information from TM with the information from the LSP, the result will be even worse, i.e. `bad_ctags_parser + bad_lsp` is worse than `bad_ctags_parser` or `bad_lsp` alone. > On the other hand, if we follow-up on your other idea and use LSP even to > interface with the ctags parsers then documentSymbol ought to be sufficient > for all existing features (except maybe where we can specialized LSP > interfaces like goto-implementation), right? I was thinking about it and not to lose functionality, I think we'd have to introduce some Geany-specific members like `_geany_var_type`. It sucks a bit and I haven't checked the LSP specification in detail if it explicitly prohibits something like that but I think even with extra members we should be LSP-compliant. Now again, I'm not really suggesting we should do that and I think nobody has time to do it in the short term anyway. > Actually testing requires me to setup an LSP server (or perhaps multiple > ones) which I don't yet know how to do so I'm afraid it takes more than a few > minutes of my time. See https://github.com/techee/geany-lsp and README.md at its bottom. If you are using a debian-based OS, it also tells you how to install the required packages. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3571#issuecomment-1794981280 You are receiving this because you are subscribed to this thread. Message ID: <geany/geany/pull/3571/[email protected]>
