Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Language Server: Improve hovered symbol resolution, fix renaming bugs, implement reference lookup #80973

Merged
merged 1 commit into from
Sep 12, 2023
Merged

Language Server: Improve hovered symbol resolution, fix renaming bugs, implement reference lookup #80973

merged 1 commit into from
Sep 12, 2023

Conversation

ryanabx
Copy link
Contributor

@ryanabx ryanabx commented Aug 24, 2023 •
edited by YuriSizov
Loading

Continuation of #79988 to match current master. Changes can be found on that PR. Link to original comment: #79988 (comment)

Production edit: Fixes godotengine/godot-vscode-plugin#464, Fixes godotengine/godot-vscode-plugin#448, Closes godotengine/godot-proposals#3687.

@ryanabx ryanabx requested a review from a team as a code owner August 24, 2023 22:05
@ryanabx ryanabx marked this pull request as draft August 25, 2023 00:26
@ryanabx ryanabx changed the title Rebase of #79988: [LSP] Add textDocument/references & prepareRename, Enhancements for resolving symbols [LSP] Add textDocument/references & prepareRename, Enhancements for resolving symbols (continuation of #79988) Aug 25, 2023
@ryanabx ryanabx marked this pull request as ready for review August 25, 2023 02:24
@ryanabx ryanabx mentioned this pull request Aug 25, 2023
Closed
@YuriSizov YuriSizov changed the title [LSP] Add textDocument/references & prepareRename, Enhancements for resolving symbols (continuation of #79988) [LSP] Improve hovered symbol resolution, renaming, and reference lookup Aug 25, 2023
@YuriSizov YuriSizov added this to the 4.2 milestone Aug 25, 2023
@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 25, 2023 •
edited
Loading

Please squash all the commits into one and amend the commit message to reflect, in general, what the PR does. I've renamed the PR itself, and you can use that same text as your final commit message. See this documentation, if you need help with squashing.

@YuriSizov YuriSizov requested a review from adamscott August 25, 2023 11:18
@ryanabx
Copy link
Contributor Author

ryanabx commented Aug 25, 2023

Please squash all the commits into one and amend the commit message to reflect, in general, what the PR does. I've renamed the PR itself, and you can use that same text as your final commit message. See this documentation, if you need help with squashing.

Should all be squashed, let me know what else, if anything, needs to be done!

@YuriSizov
Copy link
Contributor

Thanks! I think the remaining description should be removed, or at the very least significantly simplified. This amount of detail shouldn't be added to the git history.

Given that this isn't your work, originally, it would probably be simpler to just remove everything after the first line, but if you can succinctly describe the changes, that can still be put into the commit's description. Up to you!

@ryanabx
Copy link
Contributor Author

ryanabx commented Aug 25, 2023

Thanks! I think the remaining description should be removed, or at the very least significantly simplified. This amount of detail shouldn't be added to the git history.

Given that this isn't your work, originally, it would probably be simpler to just remove everything after the first line, but if you can succinctly describe the changes, that can still be put into the commit's description. Up to you!

Just to be clear, the first line being "Add == for lsp Position and Range"?

@YuriSizov
Copy link
Contributor

No, the first line would be the commit's title, "[LSP] Improve hovered symbol resolution, renaming, and reference lookup"

@ryanabx
Copy link
Contributor Author

ryanabx commented Aug 25, 2023

No, the first line would be the commit's title, "[LSP] Improve hovered symbol resolution, renaming, and reference lookup"

Ah, ok. I removed the description!

@ryanabx ryanabx mentioned this pull request Aug 25, 2023
Merged
@ryanabx
Copy link
Contributor Author

ryanabx commented Aug 28, 2023

@adamscott This PR is ready for review whenever you have time 😁

adamscott
adamscott requested changes Aug 28, 2023
View reviewed changes
Copy link
Member

@adamscott adamscott left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine in general! I have a few comments on the PR itself (even if it's not directly your code @ryanabx).

I tested the PR and it fixes godotengine/godot-vscode-plugin#464.

But unfortunately, it doesn't seem to fix godotengine/godot-vscode-plugin#448. It works the first rename, but stops afterwards (ie. you can't rename after a rename).

Didn't test yet godotengine/godot-proposals#3687 though.

modules/gdscript/language_server/gdscript_text_document.cpp Outdated Show resolved Hide resolved
modules/gdscript/tests/gdscript_test_runner.cpp Outdated Show resolved Hide resolved
modules/gdscript/language_server/gdscript_workspace.cpp Outdated Show resolved Hide resolved
modules/gdscript/language_server/gdscript_extend_parser.h Outdated Show resolved Hide resolved
modules/gdscript/language_server/gdscript_text_document.cpp Outdated Show resolved Hide resolved
modules/gdscript/tests/test_lsp.h Outdated Show resolved Hide resolved
modules/gdscript/tests/test_lsp.h Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member

In the future please don't push individual commits while resolving feedback but push once you're done with all, if you weren't first time contributor it'd have heavily taxing the CI

@ryanabx
Copy link
Contributor Author

ryanabx commented Aug 28, 2023

In the future please don't push individual commits while resolving feedback but push once you're done with all, if you weren't first time contributor it'd have heavily taxing the CI

Sorry about that! It's my first time working with a repo with CI and I forget that Godot, being such a large project, uses a lot of resources to compile in it's CI. I'll be more careful next time

@ryanabx ryanabx requested a review from adamscott August 28, 2023 18:22
@ryanabx
Copy link
Contributor Author

ryanabx commented Aug 28, 2023 •
edited
Loading

But unfortunately, it doesn't seem to fix godotengine/godot-vscode-plugin#448. It works the first rename, but stops afterwards (ie. you can't rename after a rename).

@Booksbaum while you're here and available what do you think of this issue? I'm unfortunately not familiar enough with the material to know why this occurs

@Booksbaum
Copy link
Contributor

it might be internal Godot structure (or just LSP DocumentSymbol tree?) get out of sync with VSCode text.

Does it work when you:

  • rename
  • make some other changes to explicitly trigger a reparse (for example: add some space somewhere)
  • try rename again

Though if rename then works -- I don't know exactly how to fix it. VSCode should notify LSP about change and a reparse should happen.
(Might get more extreme when rename involves multiple documents :/)

@ryanabx
Copy link
Contributor Author

ryanabx commented Aug 28, 2023

(Might get more extreme when rename involves multiple documents :/)

How so?

@Booksbaum
Copy link
Contributor

Booksbaum commented Aug 28, 2023 •
edited
Loading

Because we're now having multiple documents that changed -> lots of things to check.

Additional: A changed doc might not be open yet (VSCode only "opens" a doc when you open a tab with it) -> we now have to deal with a new_ish document.

In short: One doc: things can go wrong in one document. Multiple docs: things can go wrong in multiple documents :D

And depending how parallel that happens: Request (or even reparse/check) might read outdated state -- or worse: state that changes during its execution -> uses two different states in same request...



But the issue you described sound like there wasn't a reparse yet (or is still in the process) -> reads old state -> cannot rename. (Though then shouldn't it rename whatever was there in old state? hm...)

Edit: Just tested it (with an old version): It's saving. After Ctrl+S renaming of that symbol works again.

Edit: the internal DocumentSymbol tree seems to be correct: Name gets changed in breadcrumb. So doesn't seem to be a parsing issue

@ryanabx
Copy link
Contributor Author

ryanabx commented Aug 29, 2023

Thanks @Booksbaum for the help! Looks like all checks passed as well, @adamscott can you check to see if your renaming issue is fixed?

@ryanabx
Copy link
Contributor Author

ryanabx commented Sep 4, 2023 •
edited
Loading

I went ahead and tested the changes for myself.
godotengine/godot-vscode-plugin#464 is fixed.
godotengine/godot-vscode-plugin#448 is fixed. (I couldn't reproduce Adam's issue anymore)
godotengine/godot-proposals#3687 would be closed.

Since I went ahead and tested it, I also rebased changes to match master again.

EDIT: I know it says changes were requested, but I have resolved all comments on the requested changes. Is this something that has to be manually updated by the reviewer?

AThousandShips
AThousandShips reviewed Sep 5, 2023
View reviewed changes
modules/gdscript/language_server/gdscript_extend_parser.cpp Outdated Show resolved Hide resolved
modules/gdscript/language_server/gdscript_extend_parser.cpp Show resolved Hide resolved
modules/gdscript/language_server/gdscript_extend_parser.cpp Show resolved Hide resolved
modules/gdscript/language_server/gdscript_extend_parser.cpp Show resolved Hide resolved
modules/gdscript/language_server/gdscript_extend_parser.h Show resolved Hide resolved
modules/gdscript/tests/test_lsp.h Show resolved Hide resolved
modules/gdscript/tests/test_lsp.h Show resolved Hide resolved
modules/gdscript/tests/test_lsp.h Show resolved Hide resolved
modules/gdscript/tests/test_lsp.h Show resolved Hide resolved
modules/gdscript/tests/test_lsp.h Show resolved Hide resolved
@YuriSizov
Copy link
Contributor

Is this something that has to be manually updated by the reviewer?

This is the status of the latest review by that particular maintainer. It will be updated once a new review is submitted :)

@ryanabx ryanabx changed the title [LSP] Improve hovered symbol resolution, renaming, and reference lookup Language Server: Improve hovered symbol resolution, fix renaming bugs, implement reference lookup Sep 7, 2023
adamscott
adamscott approved these changes Sep 11, 2023
View reviewed changes
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed during the GDScript Team meeting. Looks good! A lot of code all at once, but the LSP needs it.

This PR need to be tested anyway, so let's merge this sooner than later.

@Booksbaum @ryanabx
Language Server: Improve hovered symbol resolution, fix renaming bugs…
0202a36 
…, implement reference lookup

Co-Authored-By: Ryan Brue <56272643+ryanabx@users.noreply.github.com>
Co-Authored-By: BooksBaum <15612932+booksbaum@users.noreply.github.com>
@akien-mga akien-mga merged commit d507708 into godotengine:master Sep 12, 2023
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@ryanabx ryanabx deleted the pr/79988 branch September 12, 2023 17:02
@atirut-w atirut-w mentioned this pull request Sep 14, 2023
Closed
@akien-mga akien-mga mentioned this pull request May 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips left review comments

@adamscott adamscott adamscott approved these changes

@Booksbaum Booksbaum Awaiting requested review from Booksbaum

Assignees
No one assigned
Labels
bug enhancement topic:editor topic:gdscript
Projects
None yet
Milestone
4.2
6 participants
@ryanabx @YuriSizov @AThousandShips @Booksbaum @akien-mga @adamscott