language_server: fix "go to definition" to handle build_defs in plugins, add "show usages" that handle both build_defs, plugins and reverse dependencies#3485
Conversation
Previously, go-to-definition only worked for core builtin functions. Plugin-defined rules like go_library, go_repo, etc. would return no results because they were parsed by a different parser instance than the one used by the language server. Changes: - Use parse.InitParser() to initialize the parser on BuildState, then get the same parser via parse.GetAspParser() for the language server - Add periodic loading of function definitions (every 2 seconds) so go-to-definition works progressively while the full parse runs - Add Range() method to cmap types to iterate over parsed ASTs - Add AllFunctionsByFile() to asp.Parser to retrieve function definitions - Fix file URIs to use absolute paths
Implements textDocument/references for the BUILD file language server. Supports two modes: 1. Function references: When cursor is on a function definition (e.g., `def go_repo(...)`), finds all BUILD files that call that function. 2. Build label references: When cursor is on a build label, uses query.FindRevdeps to find all targets that depend on it, then locates the exact string literal positions in their BUILD files.
- Add unit tests for textDocument/references functionality - Fix panic when package name is invalid (check graph before FindRevdeps) - Support finding references when cursor is on function call, not just definition - Add require dependency to BUILD for stricter test assertions
toastwaffle
left a comment
toastwaffle
left a comment
There was a problem hiding this comment.
Please could you split the implementation of finding references out into a separate PR? There's quite a lot here for a single PR.
| // Range calls f for each key-value pair in this shard. | ||
| // Returns false if iteration was stopped early. | ||
| func (s *shard[K, V]) Range(f func(key K, val V) bool) bool { | ||
| s.l.RLock() | ||
| defer s.l.RUnlock() | ||
| for k, v := range s.m { | ||
| if v.Wait == nil { // Only include completed values | ||
| if !f(k, v.Val) { | ||
| return false | ||
| } | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
| // Range calls f for each key-value pair in this shard. | |
| // Returns false if iteration was stopped early. | |
| func (s *shard[K, V]) Range(f func(key K, val V) bool) bool { | |
| s.l.RLock() | |
| defer s.l.RUnlock() | |
| for k, v := range s.m { | |
| if v.Wait == nil { // Only include completed values | |
| if !f(k, v.Val) { | |
| return false | |
| } | |
| } | |
| } | |
| return true | |
| } | |
| // Range calls f for each key-value pair in this shard. | |
| func (s *shard[K, V]) Range(f func(key K, val V) bool) { | |
| s.l.RLock() | |
| defer s.l.RUnlock() | |
| for k, v := range s.m { | |
| if v.Wait == nil { // Only include completed values | |
| if !f(k, v.Val) { | |
| return | |
| } | |
| } | |
| } | |
| } |
The return value is unused
| f := doc.AspFile() | ||
|
|
||
| var locs []lsp.Location | ||
| locs := []lsp.Location{} |
There was a problem hiding this comment.
Why this change, and is it to prevent JSON serialising our slice as null instead of []?
| exprStart := f.Pos(expr.Pos) | ||
| exprEnd := f.Pos(expr.EndPos) | ||
| if !asp.WithinRange(pos, exprStart, exprEnd) { |
| log.Debug("initial parse complete") | ||
| h.buildPackageTree() | ||
| log.Debug("built completion package tree") | ||
| h.loadParserFunctions() |
There was a problem hiding this comment.
Is there any concern about this running concurrently with an invocation of h.loadParserFunctions() inside the goroutine? Would it make more sense to have:
case <-done:
// Run one last time once everything has been parsed
h.loadParserFunctions()
return
case <-ticker.C:
h.loadParserFunctions()
| file := asp.NewFile(filename, data) | ||
| for _, stmt := range stmts { | ||
| name := stmt.FuncDef.Name | ||
| // Only add if not already present (don't override core builtins) |
There was a problem hiding this comment.
I'm not sure this is the right rule to follow (but I am very happy for you to convince me that it is). We have a few cases in our codebase where we override definitions to alter the behaviour (often adding extra labels to things). I'd expect that if we do have multiple definitions for a single identifier, the LSP would return all the definitions, and not just the ones we saw first? that would imply that h.builtins should really be a map[string][]builtin (I'd also argue that builtin is the wrong terminology, but that's a whole other problem).
jwong-beep
left a comment
jwong-beep
left a comment
There was a problem hiding this comment.
A couple of questions:
-
How does the asp parser handle syntactically invalid functions? Does the LSP server panic because of a parse error?
-
I see some parseIfNeeded and the handler's builtins map is prepopulated on LSP server init. If you add a new build def file or BUILD file, how do function defs and refs get populated in the cache? Do they get populated at all?
| refFile := refDoc.AspFile() | ||
|
|
||
| // Find all statement calls to the function (e.g., go_library(...)) | ||
| asp.WalkAST(refAst, func(stmt *asp.Statement) bool { |
There was a problem hiding this comment.
This walk doesn't look like it's actually looking for function calls. It's just looking for identifiers that have the same name as the funcName. Should there be lookaheads for "(" and ")"?
4 participants
fix "go to definition" to handle build_defs in plugins
add "show usages" that handle both build_defs, plugins and reverse dependencies
This makes the languageserver work like a charm, enabling good code navigation and all the magical AI tools that rely on LSP.