LSP: Don't respond to non-GDScript files#103186
LSP: Don't respond to non-GDScript files#103186Na-r wants to merge 1 commit intogodotengine:masterfrom
Conversation
|
|
||
| bool GDScriptTextDocument::is_valid_gd_file(const lsp::TextDocumentItem &p_doc) { | ||
| // LanguageId isn't always set, but check it for good practice | ||
| return p_doc.languageId == "gdscript" || p_doc.uri.ends_with(".gd"); |
There was a problem hiding this comment.
this could need a change after #97657 is merged, unles p_doc.languageId == "gdscript"is good enough
There was a problem hiding this comment.
This would definitely need a change. Some of the notifications (including didSave) don't return anything besides the URI and text content. Should a check against .gdt be put in now to get ahead of it? There's also a similar update that would be needed at here.
There was a problem hiding this comment.
well, for now both prs are open, i just wanted to reference this PR in the other one 👍
There was a problem hiding this comment.
I don't think checking the file extension on our side is a good idea. Editors like vscode allow changing the language mode for documents and with this PR, this will not work anymore (it receives initial diagnostics, but they never get updated). This might become a problem with custom tooling (e.g. preprocessor script which uses custom extensions). Also it might become a roadblock for implementing language embeddings (not sure if VSCode has such a capability, but for Idea IDE's a plugin could embed GDScript blocks into scene files for builtin scripts).
Interpretation of the content type should be up to the editor IMO, so we should only be using the language id. As pointed out this is not sent with every request, but that is intended, since LSP is not stateless. When we receive a text document identifier we should already have received a text document item for the uri which contains this information.
(Well that's the case for most notifications, sadly didSave seems under specified in this regard. I opened an issue against the specification to clear this up.)
There was a problem hiding this comment.
Based on the answer to the issue over at the LSP repo, I think we can work under the assumption that didSave requires taking ownership. So the correct approach would be to save the content type the editor provides with didOpen and then use it for other notifications.
|
The above mentioned persisting of language ids was implemented in #105236 along with some basic safeguards in common endpoints. Closing as superseded, but thanks for your contribution nevertheless! |
4 participants
Background
When an LSP client saves, Godot's LSP server receives a
textDocument/didSavenotification. Depending on how the LSP client is implemented, it may send this notification for any file, not necessarily only GDScript files. Godot's LSP server wouldn't do any checking on the file type received from this notification, so it would send back syntax errors for whatever file was saved.According to the LSP spec, unless the server has previously sent
TextDocumentSaveRegistrationOptions, the LSP client should send thetextDocument/didSavenotification for all files saved. See: the LSP spec and zed-industries/zed/pull/19183. (Can someone more familiar with LSP spec confirm this?)Godot's LSP mostly uses static registrations during initialization for its LSP server capabilities, but
TextDocumentSaveRegistrationOptionsis only available as a dynamic registration. This makes implementing the fix that way a bit uglier than just checking file types when receiving notifications.Fix
We will now check for a file being a valid GDScript file, via the language ID or file extension, before processing it any further. While this fix was specifically for the
textDocument/didSavenotification, I added checks for other related notifications as well.