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.

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

Support textDocument/prepareRename & textDocument/documentHighlight, improve accuracy of reference finding #732

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

4teapo
Copy link
Contributor

@4teapo 4teapo commented Aug 9, 2024

Zed requires that language servers support textDocument/prepareRename for symbol renaming to work. Zed also requires textDocument/documentHighlighting for highlighting to work, and this request is also used to mark everything that is about to get renamed when you enter symbol renaming mode. This PR adds support for both these capabilities.

Before, with the current type solver, finding property references didn't recognize assigning to properties, meaning foo in tbl.foo = bar wouldn't be recognized as a reference to the foo property inside of tbl. This PR changes the logic behind finding property references to instead go through the AST, which also makes it possible to properly determine if symbols should be marked as Read or Write for the documentHighlight request.

Previously, finding references of imported modules in types also didn't work with scopes or shadowing. This PR alters the algorithm so that it stops processing a statement block when the symbol is shadowed by a module import, and when the scope in which the symbol is declared ends, which also is an optimization.

The old findAllReferences function would also allow finding references of properties of globals, which makes an assertment fail in debug builds. With these changes, it returns early instead.

Resolves 4teapo/zed-luau#2.

4teapo and others added 5 commits August 9, 2024 01:37
…ight, improve reference results

Zed requires servers to support textDocument/prepareRename in order for symbol renaming to work. Zed also doesn't do any highlighting without textDocument/documentHighlight, and this request is also used to create a visual effect, highlighting everything that will get renamed when you start the symbol renaming action. This commit adds support for both these capabilities.

It also resolves a previous issue regarding variable shadowing and module imports when searching for symbols, and makes assigning fields in tables work with references (before, `bar` wouldn't be recognized as a reference in `foo.bar = baz`, at least with the old solver, provided the type of `foo` contains `bar`).
@JohnnyMorganz
Copy link
Owner

Apologies, but if it's not too difficult could you split out these different features + bug fixes into separate PRs? It makes it a bit easier on me to review, but it's alright if not.

Could you also add changelog entries for each of them too?

I hope to review within the next week

@4teapo
Copy link
Contributor Author

4teapo commented Aug 9, 2024

I don't think there's a good way to separate these into different PRs, because the two main bug fixes are both related to AST traversal structures that depend on lsp::DocumentHighlightKind. I could put prepareRename in a different one, but it's a very small part of this.

@JohnnyMorganz
Copy link
Owner

Fair enough

Could you also write some test cases for the bugs you fixed and the 2 new features you added?

@4teapo
Copy link
Contributor Author

4teapo commented Aug 12, 2024

I just added some test cases that capture the things I've added and fixed.

@JohnnyMorganz
Copy link
Owner

So sorry for the delay, will try to take a look this weekend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't rename symbols
2 participants