Support textDocument/prepareRename & textDocument/documentHighlight, improve accuracy of reference finding #732
Conversation
…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`).
|
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 |
|
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. |
|
Fair enough Could you also write some test cases for the bugs you fixed and the 2 new features you added? |
|
I just added some test cases that capture the things I've added and fixed. |
|
So sorry for the delay, will try to take a look this weekend |
| @@ -12,6 +12,8 @@ static bool isGlobalBinding(const WorkspaceFolder* workspaceFolder, const lsp::R | |||
| auto position = textDocument->convertPosition(params.position); | |||
|
|
|||
| auto sourceModule = workspaceFolder->frontend.getSourceModule(moduleName); | |||
| if (!sourceModule) | |||
There was a problem hiding this comment.
When would we hit such a case?
| if (!sourceModule) | ||
| throw JsonRpcException(lsp::ErrorCode::RequestFailed, "Unable to read source code"); | ||
|
|
||
| auto exprOrLocal = findExprOrLocalAtPositionClosed(*sourceModule, position); |
There was a problem hiding this comment.
I see a lot of this code is still duplicated from References.cpp.
AIUI, DocumentHighlight should basically use the exact same logic (and hence return the exact same data) as textDocument/references, except filtered down to the current file and maybe with R/W data.
Can we extract this out into one whole thing that is used by both? (similar to how rename calls out to find all references). This "universal" setup would return all the details, then references would filter out the R/W data, whilst documenthighlight will filter out just those in the current file. If that makes sense
There was a problem hiding this comment.
If it's easier, we can also do this in a followup
There was a problem hiding this comment.
I think document highlight should process only the current file for performance reasons, if you mean that it could process like the references request but only use the results for the current file. If I remember correctly, there's nothing preventing us from having a more unified setup, so I can implement that.
|
|
||
| explicit FindSymbolReferences(Luau::Symbol symbol) | ||
| explicit FindSymbolReferences(Luau::Symbol symbol, bool withKinds) |
There was a problem hiding this comment.
To make this simpler, let's just always add kinds. The consumer can then choose to discard the info if necessary.
Hence result should just store a std::pair<Location, Kind>
| @@ -624,7 +712,8 @@ struct FindSymbolReferences : public Luau::AstVisitor | |||
| { | |||
| if (visitLocal(var)) | |||
| { | |||
| result.push_back(var->location); | |||
| addResult(var->location, lsp::DocumentHighlightKind::Write); | |||
There was a problem hiding this comment.
this is the "i" in for i,v in ... do right? In that case, should it be a read?
There was a problem hiding this comment.
Yep, it is the i,v. I think all loop variables should be write, because it's basically declaring a variable (similar to local i, v = fun(...))? Sumneko lua has them be write too.
| if (ttv->definitionModuleName.empty()) | ||
| // When the definition module name is starts with '@', e.g. "@luau" or "@roblox", `LUAU_ASSERT(!buildQueueItems.empty());` in Frontend.cpp fails, | ||
| // after we call checkStrict below | ||
| if (!ttv || ttv->definitionModuleName.empty() || ttv->definitionModuleName[0] == '@') |
There was a problem hiding this comment.
Ideally we map this to a definitions file in future.
Can you add a test and a TODO for this?
| @@ -637,19 +726,205 @@ struct FindSymbolReferences : public Luau::AstVisitor | |||
|
|
|||
| bool visit(Luau::AstTypeReference* typeReference) override | |||
| { | |||
| // TODO: this is not *completely* correct in the case of shadowing, as it is just a name comparison | |||
There was a problem hiding this comment.
Please keep the TODO, but instead mention that this can be simplified in future
| auto requireInfo = findClosestAncestorModuleImport(*sourceModule, reference->prefix.value(), reference->prefixLocation->begin); | ||
| if (!requireInfo) | ||
| return std::nullopt; |
There was a problem hiding this comment.
Do we need this? We don't use the result
|
FWIW, still waiting on the comments being resolved here before merging this. Let me know if there are any issues with it, I can also take a look |
I've had to put this aside for a long time, so I haven't been able to resolve the comments yet. Hopefully I can get to it soon. I've been using luau-lsp with these changes personally and haven't experienced any bugs so far, so AFAIK there's no issues of that sort |
|
Cool no worries, just wanted to double check you weren't expecting anything since I noticed you're still merging main into this PR |
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
foointbl.foo = barwouldn't be recognized as a reference to thefooproperty inside oftbl. 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.