-
Notifications
You must be signed in to change notification settings - Fork 212
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
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,41 +18,40 @@ func (h *Handler) definition(params *lsp.TextDocumentPositionParams) ([]lsp.Loca | |
| ast := h.parseIfNeeded(doc) | ||
| f := doc.AspFile() | ||
|
|
||
| var locs []lsp.Location | ||
| locs := []lsp.Location{} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this change, and is it to prevent JSON serialising our slice as |
||
| pos := aspPos(params.Position) | ||
| asp.WalkAST(ast, func(expr *asp.Expression) bool { | ||
| if !asp.WithinRange(pos, f.Pos(expr.Pos), f.Pos(expr.EndPos)) { | ||
| exprStart := f.Pos(expr.Pos) | ||
| exprEnd := f.Pos(expr.EndPos) | ||
| if !asp.WithinRange(pos, exprStart, exprEnd) { | ||
|
Comment on lines
+24
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this change? |
||
| return false | ||
| } | ||
|
|
||
| if expr.Val.Ident != nil { | ||
| if loc := h.findGlobal(expr.Val.Ident.Name); loc.URI != "" { | ||
| locs = append(locs, loc) | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| if expr.Val.String != "" { | ||
| label := astutils.TrimStrLit(expr.Val.String) | ||
| if loc := h.findLabel(doc.PkgName, label); loc.URI != "" { | ||
| locs = append(locs, loc) | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| }) | ||
| // It might also be a statement. | ||
| // It might also be a statement (e.g. a function call like go_library(...)) | ||
| asp.WalkAST(ast, func(stmt *asp.Statement) bool { | ||
| if stmt.Ident != nil { | ||
| endPos := f.Pos(stmt.Pos) | ||
| stmtStart := f.Pos(stmt.Pos) | ||
| endPos := stmtStart | ||
| // TODO(jpoole): The AST should probably just have this information | ||
| endPos.Column += len(stmt.Ident.Name) | ||
|
|
||
| if !asp.WithinRange(pos, f.Pos(stmt.Pos), endPos) { | ||
| return false | ||
| if !asp.WithinRange(pos, stmtStart, endPos) { | ||
| return true // continue to other statements | ||
| } | ||
|
|
||
| if loc := h.findGlobal(stmt.Ident.Name); loc.URI != "" { | ||
| locs = append(locs, loc) | ||
| } | ||
|
|
@@ -78,6 +77,9 @@ func (h *Handler) findLabel(currentPath, label string) lsp.Location { | |
| } | ||
|
|
||
| pkg := h.state.Graph.PackageByLabel(l) | ||
| if pkg == nil { | ||
| return lsp.Location{} | ||
| } | ||
| uri := lsp.DocumentURI("file://" + filepath.Join(h.root, pkg.Filename)) | ||
| loc := lsp.Location{URI: uri} | ||
| doc, err := h.maybeOpenDoc(uri) | ||
|
|
@@ -137,9 +139,17 @@ func findName(args []asp.CallArgument) string { | |
|
|
||
| // findGlobal returns the location of a global of the given name. | ||
| func (h *Handler) findGlobal(name string) lsp.Location { | ||
| if f, present := h.builtins[name]; present { | ||
| h.mutex.Lock() | ||
| f, present := h.builtins[name] | ||
| h.mutex.Unlock() | ||
| if present { | ||
| filename := f.Pos.Filename | ||
| // Make path absolute if it's relative | ||
| if !filepath.IsAbs(filename) { | ||
| filename = filepath.Join(h.root, filename) | ||
| } | ||
| return lsp.Location{ | ||
| URI: lsp.DocumentURI("file://" + f.Pos.Filename), | ||
| URI: lsp.DocumentURI("file://" + filename), | ||
| Range: rng(f.Pos, f.EndPos), | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import ( | |
| "github.com/thought-machine/please/rules" | ||
| "github.com/thought-machine/please/src/core" | ||
| "github.com/thought-machine/please/src/fs" | ||
| "github.com/thought-machine/please/src/parse" | ||
| "github.com/thought-machine/please/src/parse/asp" | ||
| "github.com/thought-machine/please/src/plz" | ||
| ) | ||
|
|
@@ -173,6 +174,12 @@ func (h *Handler) handle(method string, params *json.RawMessage) (res interface{ | |
| return nil, &jsonrpc2.Error{Code: jsonrpc2.CodeInvalidParams} | ||
| } | ||
| return h.definition(positionParams) | ||
| case "textDocument/references": | ||
| referenceParams := &lsp.ReferenceParams{} | ||
| if err := json.Unmarshal(*params, referenceParams); err != nil { | ||
| return nil, &jsonrpc2.Error{Code: jsonrpc2.CodeInvalidParams} | ||
| } | ||
| return h.references(referenceParams) | ||
| default: | ||
| return nil, &jsonrpc2.Error{Code: jsonrpc2.CodeMethodNotFound} | ||
| } | ||
|
|
@@ -195,16 +202,38 @@ func (h *Handler) initialize(params *lsp.InitializeParams) (*lsp.InitializeResul | |
| } | ||
| h.state = core.NewBuildState(config) | ||
| h.state.NeedBuild = false | ||
| // We need an unwrapped parser instance as well for raw access. | ||
| h.parser = asp.NewParser(h.state) | ||
| // Initialize the parser on state first, so that plz.RunHost uses the same parser. | ||
| // This ensures plugin subincludes are stored in the same AST cache we use. | ||
| parse.InitParser(h.state) | ||
| h.parser = parse.GetAspParser(h.state) | ||
| if h.parser == nil { | ||
| return nil, fmt.Errorf("failed to get asp parser from state") | ||
| } | ||
| // Parse everything in the repo up front. | ||
| // This is a lot easier than trying to do clever partial parses later on, although | ||
| // eventually we may want that if we start dealing with truly large repos. | ||
| go func() { | ||
| // Start a goroutine to periodically load parser functions as they become available. | ||
| // This allows go-to-definition to work progressively while the full parse runs. | ||
| done := make(chan struct{}) | ||
| go func() { | ||
| ticker := time.NewTicker(2 * time.Second) | ||
| defer ticker.Stop() | ||
| for { | ||
| select { | ||
| case <-done: | ||
| return | ||
| case <-ticker.C: | ||
| h.loadParserFunctions() | ||
| } | ||
| } | ||
| }() | ||
| plz.RunHost(core.WholeGraph, h.state) | ||
| close(done) | ||
| log.Debug("initial parse complete") | ||
| h.buildPackageTree() | ||
| log.Debug("built completion package tree") | ||
| h.loadParserFunctions() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any concern about this running concurrently with an invocation of |
||
| }() | ||
| // Record all the builtin functions now | ||
| if err := h.loadBuiltins(); err != nil { | ||
|
|
@@ -221,6 +250,7 @@ func (h *Handler) initialize(params *lsp.InitializeParams) (*lsp.InitializeResul | |
| DocumentFormattingProvider: true, | ||
| DocumentSymbolProvider: true, | ||
| DefinitionProvider: true, | ||
| ReferencesProvider: true, | ||
| CompletionProvider: &lsp.CompletionOptions{ | ||
| TriggerCharacters: []string{"/", ":"}, | ||
| }, | ||
|
|
@@ -268,6 +298,37 @@ func (h *Handler) loadBuiltins() error { | |
| return nil | ||
| } | ||
|
|
||
| // loadParserFunctions loads function definitions from the parser's ASTs. | ||
| // This includes plugin-defined functions like go_library, python_library, etc. | ||
| func (h *Handler) loadParserFunctions() { | ||
| funcsByFile := h.parser.AllFunctionsByFile() | ||
| if funcsByFile == nil { | ||
| return | ||
| } | ||
| h.mutex.Lock() | ||
| defer h.mutex.Unlock() | ||
| for filename, stmts := range funcsByFile { | ||
| // Read the file to create a File object for position conversion | ||
| data, err := os.ReadFile(filename) | ||
| if err != nil { | ||
| log.Warning("failed to read file %s: %v", filename, err) | ||
| continue | ||
| } | ||
| file := asp.NewFile(filename, data) | ||
| for _, stmt := range stmts { | ||
| name := stmt.FuncDef.Name | ||
| // Only add if not already present (don't override core builtins) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| if _, present := h.builtins[name]; !present { | ||
| h.builtins[name] = builtin{ | ||
| Stmt: stmt, | ||
| Pos: file.Pos(stmt.Pos), | ||
| EndPos: file.Pos(stmt.EndPos), | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // fromURI converts a DocumentURI to a path. | ||
| func fromURI(uri lsp.DocumentURI) string { | ||
| if !strings.HasPrefix(string(uri), "file://") { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value is unused