[LSP] Add textDocument/references & prepareRename, Enhancements for resolving symbols
#79988
Conversation
is obsolete Instead `textDocument/resolveSymbols` now returns `DocumentSymbol`. Practical difference in VSCode: in breadcrumb navigation bar at top of source file: Instead of a flat list of all elements its now a hierarchical tree.
Note: just stub -- doesn't return anything yet
Note: `includeDeclaration` isn't handled yet (decl is always included) Add `find_usages_in_file` & `find_all_usages`
Enhancement: Don't look through other files when renaming local symbol
Example:
* `a.gd`:
```gdscript
extends Node
var member = 42
```
* `b.gd`:
```gdscript
extends Node
const A = preload("res://a.gd")
func test(a: A) -> void:
var v = a.member
```
-> `Find all references` (or `rename`) on `member` inside:
* `a.gd` -> only find `member` inside `a.gd`
* `b.gd` -> doesn't find any references
Issue was: variable was always treated as local
Example:
```gdscript
func do_stuff(arg: int) -> int:
arg += 2
return arg
```
-> `resolve_symbol` on `arg` (in `arg += 2`) returns `do_stuff` (the function), NOT `arg` (the parameter)
Reason: `resolve_symbol` searches for the first symbol in the definition line -- which is the function, NOT the parameter.
Effects of this fix:
* Tooltip for `arg` usage now correctly displays parameter info instead of function info
* `Go to definition` now locates parameter instead of function
* `Find all references` now finds references of parameter instead of references of function
* `Rename` now renames parameter instead of function
* **Regression**: resolves to first symbol declaration with that name. If there's another variable/parameter with same name defined before in script it resolves to that symbol. This issue was already present before with things other than parameters, but this commit "introduces" the issue to parameter.
* Example:
```gdscript
func a(arg: int):
pass
func b(arg: int):
arg += 1
```
-> `arg` (in `arg += 1`) resolve to `arg` in `a`
* Reason: prev: parameter was resolved to its function. Now its correctly resolved and as result: handled like variable.
Note: I now require the definition symbol to have the same name as usage name. (In example above: symbol must be named `arg`).
A better solution would be to adjust `GDScriptLanguage::lookup_code` to return not just the line number (`location`), but also the column (or even better: complete range -- or maybe even node?). But that's out of scope of this PR. And additionally: would require deeper changes (`lookup_code` & `LookupResult` isn't just local to GDScript, but for all `ScriptLanguage`s).
prevents renaming of invalid rename locations, like keywords or builtins Effect in VSCode: rename box doesn't open when rename not valid.
prev: return `defaultBehavior: true`
Reason for change:
`defaultBehavior` doesn't work with current `godot-vscode-plugin`:
Uses old `vscode-languageclient` -- and updating requires significant more changes than just updating the package.
-> current `godot-vscode-plugin` (Version `1.3.1`) works with `range`, but not `defaultBehavior`
Fix/Enhancement: `ExtendGDScriptParser::get_identifier_under_position` returns incorrect or unclear offset
Though I don't think that had any real consequences -- might also just me misunderstanding how exactly it should be used.
To better document and clarify: `get_identifier_under_position` now returns a `lsp::Range` which is:
* zero based
* start: inclusive
* end: exclusive
Example:
```gdscript
[...]
var member := 2
```
Hover on `b` in `member`: (pos: `{ line: 3, character: 7}`)
* prev: offset (`Vector2i`): `(-4, 2)` -> range in `line: 3`: from `char: 3` to `char: 9`
* `char: 3` exclusive? or 1-based & inclusive?
* `char: 9`: inclusive? or 1-based & exclusive?
* now: LSP range `{ "start": { "line": 3, "character": 4 }, "end": { "line": 3, "character": 10 } }`
* `char: 4`: inclusive
* `char 10`: exclusive
… uses cursor position (instead of character position)
Consequence: Commands now recognize identifier when cursor at end of identifier
Example: (`|`: cursor, `^`: position)
```gdscript
var member| := my_func|(my_var|)
```
* prev: position was used to determine identifier
-> no valid identifier under position, but instead space or parens
-> rename didn't work
* now: cursor is BETWEEN chars
-> position is adjusted when at end of identifier
-> rename does work
Note:
Commands via mouse on character AFTER identifier (`^` in example above) now apply to identifier. That's just how LSP work: no difference between cursor (between chars) & mouse interaction (on char). `Position` in LSP is specified as "between two character" -- which is what this commit does.
Other extensions show same behaviour. For example: `rust-analyzer` allows commands on characters after identifier too.
Though not all extensions: `C/C++` extension allows rename & find references on char after identifier, but doesn't show hover.
Same check as in `textDocument/prepareRename`
run with `--test --test-suite="[Modules][GDScript][LSP]"` Note: Currently doesn't pass Note: doesn't cover much. For further testing. Currently focus on `resolve_symbol`
Ranges are now directly marked inside gdscript. Every Range can be annotated with a name and a ref Rewrite LSP tests from prev commit to use inline markers Note: still failing: range of symbol spans more than just identifier
There are two ranges: * `range`: covers the complete node * `selectionRange`: covers just the identifier Note: Fixes some tests, but not all: `resolve_symbol` for `test`s is still failing. I think because GDScript AST doesn't keep tabs as characters but converts them to columns?
Godot expands `\t` with `text_editor/behavior/indent/size`, while in LSP `\t` is just a single character. -> Requires conversion between LSP and Godot Note: currently only parsing to `DocumentSymbols`s converts Godot to LSP. That should cover most cases -- especially `resolve_symbol`. But there might be still some unhandled cases -> not yet checked. Note: requires more tests -- currently only simple test with single tab
Addendum to prev commit message: prev commit seems to cover all cases which require conversion between LSP and Godot. Currently there seem to be only two main bridges between GDScriptParser & ExtendGDScriptParser: * parsing in ExtendGDScriptParser -> transforms GDScript AST (Godot Positions) into `DocumentSymbol` (LSP positions). That's then used inside `resolve_symbol` (-> LSP positions) * `GDScriptLanguage::get_text_for_completion` && `lookup_code` -> take source code with cursor marked in string instead of extra passed position -> no explicit conversion necessary
in `textDocument/resolveSymbol`. VSCode shows result as breadcrumbs & navigation at top of source. With local `DocumentSymbol`: includes parameters & variables local to `func`s -> quite crowded & distracting clutter -> only emit public `DocumentSymbol`s (classes, variables, signals, functions, ...) (same behaviour as in other LSP implementations)
Handled here: * in getter/setter * in lambdas inside functions * parameters in signals * in different scopes * inside match * when script contains multiple variables with same name Though not all cases are fixed. For example: * lambdas in fields * match cases * shadowed variables Though: shadowing is discouraged in gdscript
Merge lambdas with parent variable
|
In the future please do not make multiple commits in such rapid succession, currently it won't fire CI because you're a first time contributor but in the future it will |
|
Ok, reading files fails. ( And additional: why does it work locally? difference I found: Godot Project Settings root path isn't with trailing |
|
@Booksbaum If the status of this PR is Work In Progress / Early Preview, then you can mark it as draft for now, to make it more obvious to contributors. And mark it as ready for review later, when you decide that the moment has come. Thanks! |
|
Error is: Which explains why it worked locally: I either always filtered to test-suite But godot/core/config/project_settings.cpp Lines 486 to 619 in 031aa99 OS::get_singleton()->get_resource_dir().is_empty() checks which use old resource_path.
Simple fix seems to be manually reset Edit: just changing
Wasn't planned as such :D ..... :( But marked as WIP for now -> enough for today |
Note: Running `[Modules][GDScript]` and `[Modules][GDScript][LSP]` together (`[Modules][GDScript]*`) fails because it cannot load resources of the later test. Issue: `ProjectSettings::resource_path` doesn't get reset inside `ProjectSettings::setup` (`_setup`) -- but instead if `resource_path` is already set it's reused -> LSP tests use `resource_path` of GDScript tests -> loading LSP resources fails
Issue was: `ProjectSettings` cannot be reset, but uses old state (especially `resource_path`) when `setup` gets called. -> Cannot change workspace (from `modules/gdscript/tests/scripts/` to `module/gdscript/tests/lsp/`) -> Loading resources (script files) in LSP tests fails (because tries to read from old gdscript folder) LSP scripts are now inside GDScript tests project, and GDScript tests & LSP tests now share a common scripts directory. LSP scripts are inside `lsp` sub-directory.
Required for LSP server
|
Tests should now run. I moved LSP test scripts into same workspace as other GDScript test scripts -> both test suites use same |
|
Yay...Unit tests succeeded But: in |
|
Yes unrelated to this PR |
|
I'd like to take this on and fix the unsuccessful check if possible.
Was this ever fixed? Is this the only blocker to getting this approved? |
|
This has been fixed and this needs a rebase to include the fix |
Rebase finished! Had to fix some references to class variables that were moved to other places. The workflow checks passed on my fork, and I created a PR: #80973 |
|
superseeded by #80973 |
Started as just adding support for
textDocument/references(godotengine/godot-proposals#3687) -- but led to a bunch of fixes & enhancements regarding what symbol is under the cursor/at a certain location (-> better results for: where are usages of a certain symbol). Which improves results for other LSP methods too.Note: with the exception of one small addition to
./modules/gdscript/gdscript_editor.cpp, all changes are isolated to just LSP (inside./modules/gdscript/language_server/)textDocument/references("Find All References" in VSCode):textDocument/rename: prevent renaming of native or external symbolstextDocument/prepareRenamerename/references: Don't search through other script files when symbol is local to current filerenameing a parameter at usage renames function instead of parametertextDocument/documentSymbolnow returnsDocumentSymbols instead ofSymbolInformations(
SymbolInformationis marked as deprecated in LSP specs)(hierarchy is auto-created by VSCode)
textDocument/documentSymbol: return only public symbols, not local ones (like parameters or variables inside afunc)do_stuffwere listed (value,desc)-> some unrelated fallback
-> no tooltip
(another example: see images in next fix)
(mentioned in LSP rename not working correctly #65563 (comment))
(Cursor is between
rand:)That's because LSP uses cursor position that are BETWEEN characters, while block-cursor (and mouse) is in practice on a certain character/column.
\t), while GDScript Parser expands these based ontext_editor/behavior/indent/size. Previously that wasn't handled, while now there's a conversion between these two if necessary.DocumentSymbol-> ranges VSCode uses for selection and cursor movement)-> shows tooltip for
intvaluein otherfunc, but hover is onstringvalue-> Only partial fix for refactor/rename feature not working godot-vscode-plugin#361 / LSP rename not working correctly #65563
resolve_symbol(what is under the cursor, where is it defined).The notable fixes (or the ones I remember, or are easy to showcase) are already mentioned above. This here just as extra bullet point to provide some general Notes:
GDScriptWorkspace::resolve_symbol, NOTGDScriptLanguage::lookup_code(-> for Godot editor too). LSP already had couple of special handlings for thingslookup_codefailed or didn't return all needed data (columns) and I expanded on these.Long-term solution and proper fixes would probably be to handle all things in
lookup_code(and remove special cases from LSP) -- but also more difficult and might need changes in related classes (like GDScriptParser) too.But this also means: I just added or expanded special handling for "simple" cases and just for LSP.
For example:
valuein comments gets resolved to usage of propertysymbol-> results in: tooltip for hover, Find References returns occurrence in comment too, rename renames
valuein comment too.lookup_codereturns nothing, it tries to find member with same name as word under cursor. Often right, but here it's wrongvaluegets resolve tovaluedefined in same line instead of membervalueGDScriptLanguage::lookup_code: doesn't consider rhs as part of outer suiteAlphaGDScriptLanguage::lookup_code(-> fails to resolve, didn't look closer)[Modules][GDScript][LSP])resolve_symbol& converting Positions between LSP & Godot)./modules/gdscript/tests/lsp_project/) ... which led to A LOT of tests ... which take some time: LSP tests are slower than gdscript tests:#!). These are some of the not handled cases mentioned above.