-
-
Notifications
You must be signed in to change notification settings - Fork 22k
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
Add Language Server Protocol for GDScript #29780
Conversation
core/script_language.h
Outdated
@@ -200,6 +200,34 @@ class ScriptInstance { | |||
virtual ~ScriptInstance(); | |||
}; | |||
|
|||
struct ScriptCodeCompletionOption { | |||
enum Kind { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for you other PR, "Kind" is not the correct word here but "Type".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really good at English.
I just followed the LSP CompletionItem
https://code.visualstudio.com/api/references/vscode-api#CompletionItem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just did a simple review based on the code style stuff. I have tested this branch and functionally works well.
lsp::Position pos; | ||
int line = get_error_line() - 1; | ||
const String &line_text = get_lines()[line]; | ||
pos.line = line; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error position resolving code is so similar with the warning position resolving one below, maybe we can make a static function to do the work to convert gdscript position to lsp position? And the document symbol position resolving is similar as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good.
lsp::Position pos; | ||
int line = warning.line - 1; | ||
const String &line_text = get_lines()[line]; | ||
pos.line = line; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above
symbol.detail = m.data_type.to_string(); | ||
symbol.deprecated = false; | ||
const int line = m.line - 1; | ||
symbol.range.start.line = line; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above
symbol.kind = lsp::SymbolKind::Event; | ||
symbol.deprecated = false; | ||
const int line = signal.line - 1; | ||
symbol.range.start.line = line; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above
symbol.kind = lsp::SymbolKind::Constant; | ||
symbol.deprecated = false; | ||
const int line = E->get().expression->line - 1; | ||
symbol.range.start.line = line; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above
#include "modules/websocket/websocket_peer.h" | ||
#include "modules/websocket/websocket_server.h" | ||
|
||
class GDScriptLanguageProtocol : public JSONRPC { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Protocol
is not a good name for this class from the perspective of English, I would name it like GDScriptLanguageServerImplementation
(definitely too long.. I am really bad at giving variable name🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the best name would be GDScriptLanguageServer
, the Implementation
isn't necessary, alternatively GDScriptLSP
} | ||
|
||
void GDScriptLanguageProtocol::on_client_disconnected(int p_id, bool p_was_clean_close) { | ||
clients.erase(p_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we close the WebSocketPeer
before removing it from client list?
} | ||
} | ||
|
||
String GDScriptLanguageProtocol::format_output(const String &p_text) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about rename it as packup_output
?
@@ -0,0 +1,1293 @@ | |||
/*************************************************************************/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this specification header file comes from https://github.com/Microsoft/language-server-protocol/blob/gh-pages/specification.md, if so, should we make it a thirdparty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we may need to rewrite non-static member initializer to constructor as Godot not support c++11 yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want the lsp.hpp
contains so much constructors I'd like remove the default value assignments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lsp.hpp
was written in godot code by myself. Some of the ducumentations are coped fromhttps://github.com/Microsoft/language-server-protocol/blob/gh-pages/specification.md
. So I don't want to add more thirdparty files and messup the build scripts.
BIND_ENUM_CONSTANT(InternalError) | ||
} | ||
|
||
Dictionary JSONRPC::make_response_error(int p_code, const String &p_message, const Variant &p_id) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make function can be rewrite to a class like JSONRPC_Error_Response
, and are the dict keys are made as class member and has a member function to convert it a dict or directly serialize it. Anyway just some subjective opinions
Should be rebased now that #29626 is merged. |
@akien-mga Removed the first commit and rebased to master branch. |
I'm testing this PR and it's overall working pretty well, but I'm having a weird crash using auto-complete when writing a "_init" constructor. To reproduce, just start writing "_init" on a newly created script and use auto-complete; it will write an extra "(" at the end (not sure if it's relevant for the crash), than state a "Expected '(' after identifier (syntax: 'func ([arguments]):'" error on VSCode, and saving the file will crash the engine. Godot will return this error on console: https://gist.github.com/henriquelalves/594746c6df44540567552eb7b8294c09. Also, I'm not sure if it was supposed to be working, but contextual node auto-completion (e.g. "get_node([..]" is only working for singletones, and F2 (renaming a symbol) doesn't work (apparently it's a Language Server method that has yet to be implemented, but I wasn't sure). I'm going to test this PR a lot and I would love to help as much as I can with feedback, but is here the most appropriate channel? The branch this PR comes from doesn't have the "Issues" tab enabled, and I'm unsure if those are errors already being taken in consideration. |
Implement hover Implement completion documentation resolve Implement hover documentation Implement go to definition
The smart resolvaion can guess most symbols but it might be slow so disabled by default users can turn on it in the editor setting
Improved uri and workspace path translatation on windows platform. The smart resolvation is much faster than builtin's in the server side. The smart resolve mode is still disabled as default as the clients might be slow with a planty of completion items.
Only level one inner classes would be resolved currently but it sould cover most real world use case Improve documation parseing for const values Improve documation format for native symbols
Drop test initialize message sent to client Remove unused code property for the parser class
Expose GDScriptLanguageProtocol singleton and classes for editor plugins (Not visiable in class tree) Fix minor bug in symbol resolve
This action will show help for target symbol in godot editor and bring the godot editor window to foreground Improved markdown documentation for symbols.
@henriquelalves Thanks for your testing.
The LSP server doesn't resolve node pathes as I think I not really neccessary because we code in external text editor with LSP client and it is really often we open diffrent script files in the external editor which is not attached to the opening scene in the godot editor so the node pathes from would be wrong.
GDScript is a dynamic typed language so it is hard to track references for symbols. So I'm not going to implement this feature as it may cause bugs randomly. Any more feedbacks are welcomed. You can pin me here or in IRC. |
I still believe that this would be a useful feature for really big scenes, so instead of going back and forth from the editor to Godot, you could just focus on the editor itself - but I understand it's not necessary to make the feature go live.
I just recompiled with the last commit and it's still happening tho! But after I autocomplete any virtual method, it now finishes with a "()" (e.g. "func _input(event: InputEvent) -> void:()") and the editor crashes in the same way as before. I'll keep testing as much as I can, as I'm already having a lot of fun coding in VSCode again! I just missed my plugins so much. |
Probably we can make this feature dispatchable at runtime, for example, let the plugin parsing some .tscn file in the workspace, if it can build a tree(subtree) of the script file, we can make use of it, otherwise we just don't care about node path. It could be very useful. @henriquelalves thanks for the testing! It is glad to see people interested in this work😄 @Geequlim maybe we should push it a little bit to get more reviews on this PR. I will also tell @ankitpriyarup to merge his work into this branch (he has a doc support, partly working renaming and some fixes to autocompletion). |
|
||
String ExtendGDScriptParser::get_text_for_completion(const lsp::Position &p_cursor) const { | ||
|
||
String longthing; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The StringBuilder
class might be more suitable for this kind of string concatenation.
|
||
void GDScriptWorkspace::list_script_files(const String &p_root_dir, List<String> &r_files) { | ||
Error err; | ||
DirAccessRef dir = DirAccess::open(p_root_dir, &err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Traversing the file system might be really slow on large project. I think it would be better to get the EditorFileSystem
cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is running in a seperated thread so I think it would be ok for performance.
I'm not sure the EditorFileSystem cache is the latest version of the script code.
Took a glance at the code and it looks mostly okay, just pointed out a couple of things that I commented. Since this is mostly new code I believe it's okay to be merged, as it shouldn't affect any current feature. We can fix potential issues with LSP itself later on. |
Thanks for tackling this important task! When it works, it is fantastic. It sometimes seems to crash though and has some minor annoyances. As the bugs in the vscode client repository are disabled, I post them here as well. Symptoms/ProblemsCyclic references disable completionsSometimes godot does not detect the cyclic reference or the message is only shown later. It is obvious when the error message is there, but this is not always the case. No completion will be shown afterward. In I guess any file that is affected? The message is only shown in one file for me in a larger project, but the completion seems to be broken everywhere. In the attached sample, the completion works in unaffected files. Sometimes godot hangs while closingI am unable to reproduce it reliably. It seems to happen when the lsp crashed. I cannot reproduce it in the minimal sample. Syntax highlighting is for CamelCase variables and functions is strangeStarting godot from vscode fails on LinuxIn godot-tools.ts:78 I had to replace windows by linux:
This issue seems similar: Starting the project closes the editorWhen I start the editor via the lsp dialog stating that godot is not running and I start the game from vscode afterward, the editor is closed. My configurationI compiled godot from this commit:
Extension version: |
#include "lsp.hpp" | ||
#include "modules/jsonrpc/jsonrpc.h" | ||
#include "modules/websocket/websocket_peer.h" | ||
#include "modules/websocket/websocket_server.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this will fail building if either the jsonrpc or websocket modules are disabled. We don't have a good system to define module inter-dependencies, but this should be addressed eventually.
Especially since building Godot without websocket but with gdscript might be a valid use case, so it should be made possible to build gdscript without the LSP in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved by #31741.
I'll merge this PR as is so that @ankitpriyarup can submit their additional (re)work. We should then review if @ankitpriyarup's work solves the comments which have been raised in this PR or not, and if not, we should ensure that all of them are handled further on (and marked as resolved when they are). |
Thanks a lot! |
@Schroedi Thanks for your testing. The bug about shell should be fixed in the coming days. Thanks for your test. You can report related issuses here godotengine/godot-vscode-plugin |
It basicly works but not finished yet.
Add a plugin for godot editor to provide a LSP Server for GDScript language.
More features would be added in the future by @ankitpriyarup who's the selected student of this slot of the GSoC this year.
This PR contains a commit of #29626 for the code completion usage.
I submit the PR to master. Once it was meged @ankitpriyarup could add other features by submit other PRs based on this to master branch.
UPDATE: 2019-06-30
The LSP implementation is works correctly both the server and client side.
Current supported features:
Planning features:
Testers are wellcomed. Here is the client for Visual Studio Code extension
godot-tools-0.9.0.zip