LSP: Rework management of client owned files#105236
LSP: Rework management of client owned files#105236Repiteo merged 1 commit intogodotengine:masterfrom
Conversation
HolonProduction
force-pushed
the
lsp-sync
branch
3 times, most recently
from
bbe01d9 to
96cca19
Compare
HolonProduction
force-pushed
the
lsp-sync
branch
6 times, most recently
from
4c10b2b to
a2d3012
Compare
HolonProduction
force-pushed
the
lsp-sync
branch
2 times, most recently
from
5186a9f to
fd71dda
Compare
HolonProduction
force-pushed
the
lsp-sync
branch
from
fd71dda to
d67e00b
Compare
|
Rebased after #111684 This PR effectively reverts the changes to |
|
I tested locally with Zed and it seems to work as expected, I am no longer getting errors when editing non-gdscript files. |
HolonProduction
force-pushed
the
lsp-sync
branch
from
d67e00b to
4cc88e5
Compare
|
Any chance this will make it into 4.6? |
|
@van800 Would you be able to test this against Rider to confirm that it doesn't introduce regressions? |
|
Sure, I will try within today-tomorrow. |
|
At the moment Rider doesn't send anything about non-gd files. It would be interesting to know what the benefits of tracking the metadata of non-gd files are. Do any document links like gd-tscn work in Zed? |
Zed does no client side filtering and sends lifecycle messages for all files to the server. So we need to filter for GDScript on the server side. Keeping track of the metadata is necessary for that. Because the language id associated with a file only gets send when it is first opened. If we would just discard all non gdscript metadata and later receive some change notification for e.g. a scene file. It would not be clear whether the file is no GDScript or the client is broken (the VSCode extension used to send change requests without opening documents properly). |
|
If it just satisfies Zed, why shouldn't it be fixed there? That said, I confirm, I haven't found any regressions introduced by this pull request. I have made a ticket for myself to update languageId in Rider to "gdscript" to be aligned with other lsp clients. In this PR the case is handled nicely. |
Zed plugins can't do that and the conversation in zed-industries/zed#37813 does not sound as if Zed would filter for relevant files anytime soon. |
akien-mga
left a comment
akien-mga
left a comment
There was a problem hiding this comment.
Rationale makes sense to me. I haven't reviewed the code in depth but I trust Holon on that as the LSP maintainer.
|
Thanks! |
7 participants
Related to godotengine/godot-vscode-plugin#842 (there is at least one other issue that makes the resolve not work).
Fixes GDQuest/zed-gdscript#6
Supersedes partially #103186 (this PR includes saveguards for all methods except
willSaveWaitUntilanddidSavewhich might still do weird stuff when editing non gdscript files).Our current approach for managing client owned content is as follows:
The catch: scripts with errors are cached differently, so if the client sends a change with parse errors it does not get cached correctly and subsequent requests (such as resolve) might reload the file from disk. Then the content that the server uses might not match with position information from the client which will result in early returns from the resolve endpoint.
This PR splits this solution up, by tracking client owned files separately from the parser cache (this is also relevant for #103186).
It also moves those caches to be per client, to prevent issues when multiple clients are connected.
Known issues:
textDocument/didOpenafter reconnecting godot-vscode-plugin#844TODO:
textDocument/didOpenafter reconnecting godot-vscode-plugin#844 should be resolved, since the VSCode extension would likely break if the server enforces the protocol