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

Known issues #26

Open
1 of 6 tasks
yioneko opened this issue Jan 29, 2023 · 8 comments
Open
1 of 6 tasks

Known issues #26

yioneko opened this issue Jan 29, 2023 · 8 comments

Comments

@yioneko
Copy link
Owner

yioneko commented Jan 29, 2023

  • Mandatory publish mode for diagnostics. Either textDocument/diagnostic or workspace/diagnostic are not implemented yet.
  • Locale setting only works for diagnostics and human messages from tsserver, but other messages from this server itself are all in English. Fixable: Use language packs from https://github.com/microsoft/vscode-loc.
  • Features include completion, code action involve a hacky implementation for corresponding resolve method because some carrying data is not serializable for communication with client. It is likely completionItem/resolve and codeAction/resolve will fail in some cases.
  • CodeLens command is not implemented. It requires clients to implement command editor.action.showReferences, which only exists in vscode. Unfixable on server side currently.
  • JSX tag autoclose is not implemented.
  • Move to file refactoring. Needs custom LSP extension or protocol support.
@yioneko yioneko pinned this issue Jan 29, 2023
@rchl
Copy link

rchl commented Jan 29, 2023

  • It requires clients to implement command editor.action.showReferences, which only exists in vscode. Unfixable on server side currently.

Should be fine to just document and require clients to implement it. I'm thinking that we'll probably just support that command in ST as we already support editor.action.triggerSuggest and editor.action.triggerParameterHints.

  • Mandatory publish mode for diagnostics.

You mean pull diagnostics model?
What do you mean by "mandatory"?

@yioneko
Copy link
Owner Author

yioneko commented Jan 29, 2023

Should be fine to just document and require clients to implement it.

Agreed. I've also seen that many clients support client side commands.

You mean pull diagnostics model?
What do you mean by "mandatory"?

Yes, currently only textDocument/publishDiagnostics is supported and there is no way to let client control the timing for fetching diagnostics. "mandatory" means that client has no choice to disable the automatic diagnostics publish and take over the control.

@llllvvuu
Copy link

llllvvuu commented Oct 9, 2023

It is likely completionItem/resolve and codeAction/resolve will fail in some cases.

Example: "Move to a new file" code action. Maybe that can just be hidden until figured out how to resolve

@yioneko
Copy link
Owner Author

yioneko commented Oct 10, 2023

Example: "Move to a new file" code action

I doubt that the fail case you encountered is not the one I intended to describe, I think I need to expand on it here. The problem involved there is that some code actions carry complex additional data (like action context, last request position, etc. Also see this for a real example) which is required for their resolve in the extension, in VSCode it relies on the object referential equality to directly access that information while we need to serialize all of them to the data field through LSP so that the server can know those additional information on the client's request for resolve, which is quite complex to implement at the moment.

To circumvent that, the same working mechanism as VSCode that depends on the object referential equality is incorporated here so that we do not need to care about serialization. Every result of completion and code action is cached on server side, and the data field is now only:

{
  providerId: 0,
  index: 1, // the index of this item in the last response
  cacheId: 2, // the cache id for the response contains this item
}

On the resolve, the item entity is actually retrieved from the cache regardless of the actual passed item (code action). But to prevent from the limitless expansion of cache, a hard limit of cache size is set, which means that if an item is considered staled and removed on the server, the client won't get an expected response for its resolve.

So generally, I don't think that the issue involved here is special to any kind of code action; and your mentioned case is possibly caused by other factors. "Move to new file" action is in the scope of support, a concrete example of its non-functioning is appreciated and worth deeper investigation.

@rchl
Copy link

rchl commented Oct 11, 2023

Your comment inspired me to do this optimization in typescript-language-server - typescript-language-server/typescript-language-server#768 though mostly due to potential performance gain rather than issue due to equality checks since I haven't noticed this being a problem. I don't see where this equality issue could come up since typescript-language-server doesn't do equality checking (at least for completions) and when it comes to tsserver, the data is serialized anyway due to having to do the CompletionDetails TS request. And VSCode has to do that too.

@unphased
Copy link

unphased commented Jan 13, 2024

I think this project may be the one that is the closest to achieving the "Move to file" code action. For example, I think it is farther along in support compared to https://github.com/pmizio/typescript-tools.nvim which maybe a while ago was superior to tsserver via nvim-lspconfig, but, from my testing today, typescript-tools offers nothing new (in particular not providing "move to file"). I looked into the source code of vscode and found that "move to file" is provided by the exact vscode ts extensions which you're patching here, so it is not a part of tsserver as I originally thought. Then I learned through this issue that sadly "move to file" is disabled.

I would like to understand more about what we would need to implement to achieve "Move to file". It is my holy grail right now. I am currently building a completely independent software development paradigm and I've been able to get very far on a simple treesitter foundation and coding it all up in typescript. Hence I am neck deep in typescript right now and looking at advanced typescript refactoring automations to help me progress, and I am loathe to fire up VSCode just in order to use "move to file"... but I will this if I have to. And I will reimplement "move to file" on my own in my metaprogramming tool i'm building, also, if I have to (and go ahead and try to do the same on all the other supported languages as well).

Another thing that i'm confused about right now is I don't get why things like codelens (isn't this just about finding references?) and diagnostics are "not implemented" or "not complete" and yet I've been building a very complex code base using bone standard tsserver obtained with mason and nvim-lspconfig and it's been completely reliable and comprehensive in fetching diagnostics and references... This makes me worry that if by some miracle we get "move to file" working, once I switch over, i'll lose references and diagnostics...

@yioneko
Copy link
Owner Author

yioneko commented Jan 14, 2024

@unphased In the implementation in VSCode, the calls to VSCode apis vscode.window.createQuickPick and vscode.window.showOpenDialog have no corresponding standard LSP methods, so it is infeasible to directly expose the move to file code action through this server without involving out-of-spec protocol support. Considering most of the LSP clients only have support for standard specification, even if we add the support on the server side, it is also required that the client should add necessary logic for its handling.

For diagnostics feature: what is not implemented is the pull model, while the publish model should work normally. The missing support for the former one doesn't matter much currently.

For code lens feature: it is the command field of CodeLens not implemented on the server side because the command editor.action.showReferences should be executed on the client side. This feature has no relationship with the references feature and is disabled by default.

@unphased
Copy link

unphased commented Jan 14, 2024

Thanks for summarizing. That is very helpful. I agree that without client LSP support, there is no clear path forward. However, I am certain that for something like "move to file" the juice will be worth the squeeze even by creating a nonstandard pathway (e.g. in nvim-vtsls) through which to implement the support. After all we just need a way to communicate a target path for this code action and that can be done with a telescope or fzf or any other file picker. Do you think that's a reasonable description or am I grossly oversimplifying the scope of the problem?

I can see that probably the way vscode implements this one is with some significant quantity of back and forth logic (client requests code action, server sends query for file, client responds with presentation of file picking gui, sends the user choice asynchronously, to which server responds with a set of refactor edits) but the happy path here could be implemented by proactively asking for a target file the moment we pick this particular code action.

Standard conforming solutions are almost always chicken egg problems. It's easy if you already have a viable hen or egg, but otherwise somebody has to step up and be a mad scientist to bootstrap the thing. A Frankenstein implementation that gets the job done is often the most practical way to motivate standards to catch up to the needs of users. The fact that vscode has already implemented move to file for typescript makes it the path of least resistance for implementing this in neovim for typescript. I see this as the best way to motivate the LSP standard toward expanding the protocol, which will in turn open the door to this scale of automation across LSPs for other languages. This seems appropriate, given tsserver was what gave birth to LSP in the first place.

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

No branches or pull requests

4 participants