LSP: Don't respond to non-GDScript files #103186
Open
+20
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MarianoGnu
reviewed
Feb 23, 2025
| @@ -133,6 +147,11 @@ lsp::TextDocumentItem GDScriptTextDocument::load_document_item(const Variant &p_ | |||
| return doc; | |||
| } | |||
|
|
|||
| 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 👍
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.