From 488801b8b1069d5377a62d9edebfc514770f3bc2 Mon Sep 17 00:00:00 2001 From: Hein Date: Tue, 6 Oct 2026 14:56:32 +0200 Subject: [PATCH] feat(lsp): hover, documentSymbol, willSaveWaitUntil, token-wide diagnostic ranges --- docs/lsp-status.md | 71 +++++++---------- docs/todo.md | 2 +- pkg/lsp/server.go | 170 +++++++++++++++++++++++++++++++++++++++-- pkg/lsp/server_test.go | 87 +++++++++++++++++++++ 4 files changed, 277 insertions(+), 53 deletions(-) diff --git a/docs/lsp-status.md b/docs/lsp-status.md index 7cf9d57..edc13d1 100644 --- a/docs/lsp-status.md +++ b/docs/lsp-status.md @@ -16,10 +16,12 @@ Verified by hand against the built binary (`pgtidy lsp`, JSON-RPC 2.0 over stdio | Capability | Value | Where | |---|---|---| -| `textDocumentSync` | `1` (full sync) | `serverCaps` | +| `textDocumentSync` | `{openClose: true, change: 1 (full), willSaveWaitUntil: true}` | `serverCaps` | | `documentFormattingProvider` | `true` | `handle("initialize")` | | `documentRangeFormattingProvider` | `true` | `handle("initialize")` | | `codeActionProvider` | `true` | `handle("initialize")` | +| `hoverProvider` | `true` | `handle("initialize")` | +| `documentSymbolProvider` | `true` | `handle("initialize")` | ### Supported methods @@ -32,7 +34,10 @@ Verified by hand against the built binary (`pgtidy lsp`, JSON-RPC 2.0 over stdio | `textDocument/formatting` | req/resp | Full-doc format via `pkg/format`; returns one `fullReplace` `TextEdit`. | | `textDocument/rangeFormatting` | req/resp | Formats doc, returns minimal edit over the selected line range. | | `textDocument/codeAction` | req/resp | Returns quick-fix `WorkspaceEdit`s for fixable diagnostics overlapping the range. | -| `textDocument/publishDiagnostics` | notif | Sent on every open/change; diagnostic code = `RuleID`, source = `pgtidy`. | +| `textDocument/willSaveWaitUntil` | req/resp | Format-on-save: returns the same safety-gated edit as `formatting`. | +| `textDocument/hover` | req/resp | Markdown with the rule ID + message of the diagnostic under the cursor; `null` elsewhere. | +| `textDocument/documentSymbol` | req/resp | `CREATE FUNCTION`/`PROCEDURE` statements (name, kind Function, `function`/`procedure` detail, full range + name selection range). | +| `textDocument/publishDiagnostics` | notif | Sent on every open/change; diagnostic code = `RuleID`, source = `pgtidy`; the range covers the offending token, not one character. | | `$/cancelRequest` | req | Ignored (per LSP, no response). | | unknown | req/resp | `-32601 method not found` (when the request has an `id`). | @@ -47,33 +52,18 @@ Verified by hand against the built binary (`pgtidy lsp`, JSON-RPC 2.0 over stdio ## What the server does NOT provide (gaps) -These are the most useful, well-scoped gaps to fill next. None are blockers for the -current shipping state. +Done since the first inventory: real diagnostic highlight range, `hover`, `documentSymbol`, +`willSaveWaitUntil`. Still open: -1. **No `hover`.** `textDocument/hover` is unimplemented and returns `-32601`. - A natural first add: return the `RuleID` + a short explanation for diagnostics - on the hovered range, or a keyword/type doc for `hover` on SQL identifiers. -2. **No `documentSymbol` / `documentLink`.** No outline/symbol tree. For a formatter - that already parses `CREATE FUNCTION`/`PROCEDURE` headers into a CST, a symbol - provider listing functions/procedures would be low-cost and high-value in large - schema files. -3. **`hover`-style diagnostics shape.** Diagnostics currently use a `Range` whose - `end.character` is `start.character + 1` (a 1-char caret), not the actual - offending span. A real highlight range would improve editor UX. -4. **`textDocument/willSave` / `willSaveWaitUntil` / `didSave`.** No save hooks — - "format-on-save" must currently be driven by the client binding - `textDocument/formatting` to the editor's save event. A `willSaveWaitUntil` - handler would let the server own format-on-save. -5. **No `completion`.** `textDocument/completion` is not implemented. Not urgent for - a formatter/linter, but relevant if PL/pgSQL autocompletion (keywords, types) is - ever in scope. -6. **No diagnostics debounce/coalescing beyond full-sync.** Every `didChange` - re-runs the full lint engine. Fine for now; a debounce + incremental re-check - becomes relevant on large files. -7. **`initializationOptions` / workspace config.** `initialize` params are parsed - nowhere — no way to pass style overrides or a config path over the protocol. -8. **No `textDocument/prepareRename`, `rename`, `references`, `foldingRange`.** - Low priority; would be natural extensions once symbol info exists. +1. **No `completion`.** `textDocument/completion` is not implemented. Not urgent for a + formatter/linter. +2. **No diagnostics debounce/coalescing beyond full-sync.** Every `didChange` re-runs the full + lint engine. Fine for now; matters on large files. +3. **`initializationOptions` / workspace config.** `initialize` params are parsed nowhere — no + way to pass style overrides or a config path over the protocol. +4. **No `prepareRename`, `rename`, `references`, `foldingRange`, `documentLink`.** Low priority; + `documentSymbol` now provides the symbol info they would build on. +5. **Hover is diagnostics-only.** No keyword/type glossary for hover on plain identifiers. ## Conventions to keep consistent @@ -89,22 +79,13 @@ current shipping state. ## Concrete next steps (recommended, smallest-first) -Ranked by effort/value for the smallest useful delta: +1. **`initializationOptions`**: accept a config path / style overrides in `initialize`. +2. **Debounce `didChange` diagnostics.** +3. **Hover glossary** for keywords/types. -1. **Add a real diagnostic highlight range** (swap the 1-char caret for the actual - offending span) — ~1 file, no new method, immediate UX win. Reuses existing - `RuleID`/severity data. -2. **Add `textDocument/hover`** returning the rule explanation for the hovered - diagnostic, or a keyword/type glossary. Reuses `pkg/lint` rule metadata. -3. **Add `documentSymbol`** listing `CREATE FUNCTION`/`PROCEDURE` signatures. - Reuses the existing CST header parse in `pkg/format`. -4. **Add `willSaveWaitUntil`** to own format-on-save instead of relying on client - binding. - -> Do NOT: expand the LSP surface into a broad design (workspace features, - incremental parsing, custom `textDocument/*` extensions) as part of this issue. - Keep any change scoped to the above and evidence-backed by a `pkg/lsp` test - (see `server_test.go` for the framed-request/response harness). +> Do NOT: expand the LSP surface into a broad design (workspace features, incremental + parsing, custom `textDocument/*` extensions). Keep any change scoped and evidence-backed by + a `pkg/lsp` test (see `server_test.go` for the framed-request/response harness). ## Verification @@ -112,5 +93,5 @@ Ranked by effort/value for the smallest useful delta: - `go test ./...` passes (LSP unit tests in `pkg/lsp/server_test.go` exercise `initialize`, formatting, range formatting, `didClose` diagnostics clearing). - Runtime e2e smoke test (framed JSON-RPC over stdio) confirmed `initialize` - capabilities, `COR001` diagnostics, and a formatting edit; `hover` correctly - returns `-32601`. + capabilities, `COR001` diagnostics, and a formatting edit. `hover`, `documentSymbol` and + `willSaveWaitUntil` are covered by tests in `pkg/lsp/server_test.go`. diff --git a/docs/todo.md b/docs/todo.md index de22a4d..2f5928f 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -168,7 +168,7 @@ Legend: ✅ done · 🚧 in progress · ⬜ not started - Note: SQL keywords inside PL/pgSQL function bodies remain lowercase (matching the corpus golden files); casing is applied only to top-level DML. - ✅ Doc-IR core (`pkg/format/doc.go`: Text/Line/SoftLine/Group/Indent/IfBreak + `Render`) and `line_width` (default 120, 0 = unlimited). First consumer: `where_wrap: when_long` (`dmlWhereWhenLong`). `select_wrap` (default `always`) and `join_wrap` (default `never`) also take `always|when_long|never`; `when_long` keeps the SELECT list / `JOIN … ON` on one line when it fits `line_width`, otherwise uses the existing one-column-per-line / ON-per-line layout. Items containing a subquery or wrapped CASE always use the broken layout. _Still TODO: move the remaining DML builders (subqueries, CTEs, VALUES) onto Doc._ -- _Still TODO: LSP range formatting._ +- ✅ LSP range formatting (already implemented; this note was stale). LSP also has hover (diagnostic rule + message), documentSymbol (functions/procedures), willSaveWaitUntil (format-on-save) and token-wide diagnostic ranges — see `docs/lsp-status.md`. ## ✅ Config expansion — DataGrip settings parity diff --git a/pkg/lsp/server.go b/pkg/lsp/server.go index a6adc15..41dc310 100644 --- a/pkg/lsp/server.go +++ b/pkg/lsp/server.go @@ -2,7 +2,9 @@ // // The server communicates over stdio using JSON-RPC 2.0 with Content-Length // framing. It provides: -// - textDocument/formatting — full-document formatting via pkg/format +// - textDocument/formatting, rangeFormatting, willSaveWaitUntil — formatting via pkg/format +// - textDocument/hover — rule ID + message of the diagnostic under the cursor +// - textDocument/documentSymbol — CREATE FUNCTION / PROCEDURE outline // - textDocument/publishDiagnostics — lint findings via pkg/lint, sent on // every didOpen/didChange notification package lsp @@ -17,8 +19,10 @@ import ( "strings" "git.warky.dev/wdevs/pgtidy/pkg/config" + "git.warky.dev/wdevs/pgtidy/pkg/cst" "git.warky.dev/wdevs/pgtidy/pkg/diagnostics" "git.warky.dev/wdevs/pgtidy/pkg/format" + "git.warky.dev/wdevs/pgtidy/pkg/lexer" "git.warky.dev/wdevs/pgtidy/pkg/lint" "git.warky.dev/wdevs/pgtidy/pkg/parser" ) @@ -30,6 +34,7 @@ func Serve(ctx context.Context, r io.Reader, w io.Writer, startDir string) error cfg, _ := config.Load(startDir) srv := &server{ docs: make(map[string]string), + diags: make(map[string][]lspDiagnostic), fixes: make(map[string][]diagnostics.Diagnostic), cfg: cfg, w: w, @@ -39,6 +44,7 @@ func Serve(ctx context.Context, r io.Reader, w io.Writer, startDir string) error type server struct { docs map[string]string // URI → current text + diags map[string][]lspDiagnostic // URI → last published diagnostics fixes map[string][]diagnostics.Diagnostic // URI → diagnostics that have fixes cfg config.Style w io.Writer @@ -76,10 +82,12 @@ func (s *server) handle(raw []byte) bool { case "initialize": s.reply(req.ID, initResult{ Capabilities: serverCaps{ - TextDocumentSync: 1, // full sync + TextDocumentSync: syncOptions{OpenClose: true, Change: 1, WillSaveWaitUntil: true}, // full sync DocumentFormattingProvider: true, DocumentRangeFormattingProvider: true, CodeActionProvider: true, + HoverProvider: true, + DocumentSymbolProvider: true, }, }) case "initialized": // no-op notification @@ -111,6 +119,18 @@ func (s *server) handle(raw []byte) bool { URI: p.TextDocument.URI, Diagnostics: []lspDiagnostic{}, }) + case "textDocument/willSaveWaitUntil": + var p formattingParams // only textDocument is used + _ = json.Unmarshal(req.Params, &p) + s.reply(req.ID, s.fullFormatEdits(p.TextDocument.URI)) + case "textDocument/hover": + var p positionParams + _ = json.Unmarshal(req.Params, &p) + s.reply(req.ID, s.hover(p.TextDocument.URI, p.Position)) + case "textDocument/documentSymbol": + var p formattingParams + _ = json.Unmarshal(req.Params, &p) + s.reply(req.ID, s.documentSymbols(p.TextDocument.URI)) case "textDocument/formatting": var p formattingParams _ = json.Unmarshal(req.Params, &p) @@ -167,7 +187,7 @@ func (s *server) pushDiagnostics(uri, text string) { col-- } lspD := lspDiagnostic{ - Range: lspRange{Start: position{line, col}, End: position{line, col + 1}}, + Range: lspRange{Start: position{line, col}, End: position{line, col + diagSpan(text, line, col)}}, Severity: severityCode(d.Severity), Code: d.RuleID, Source: "pgtidy", @@ -179,6 +199,7 @@ func (s *server) pushDiagnostics(uri, text string) { } } s.fixes[uri] = fixable + s.diags[uri] = out s.notify("textDocument/publishDiagnostics", publishDiagnosticsParams{ URI: uri, Diagnostics: out, @@ -432,10 +453,18 @@ type initResult struct { } type serverCaps struct { - TextDocumentSync int `json:"textDocumentSync"` - DocumentFormattingProvider bool `json:"documentFormattingProvider"` - DocumentRangeFormattingProvider bool `json:"documentRangeFormattingProvider"` - CodeActionProvider bool `json:"codeActionProvider"` + TextDocumentSync syncOptions `json:"textDocumentSync"` + DocumentFormattingProvider bool `json:"documentFormattingProvider"` + DocumentRangeFormattingProvider bool `json:"documentRangeFormattingProvider"` + CodeActionProvider bool `json:"codeActionProvider"` + HoverProvider bool `json:"hoverProvider"` + DocumentSymbolProvider bool `json:"documentSymbolProvider"` +} + +type syncOptions struct { + OpenClose bool `json:"openClose"` + Change int `json:"change"` // 1 = full + WillSaveWaitUntil bool `json:"willSaveWaitUntil"` } type textDocItem struct { @@ -509,3 +538,130 @@ type codeAction struct { Kind string `json:"kind,omitempty"` Edit *workspaceEdit `json:"edit,omitempty"` } + +type positionParams struct { + TextDocument textDocID `json:"textDocument"` + Position position `json:"position"` +} + +type hoverResult struct { + Contents markupContent `json:"contents"` + Range lspRange `json:"range"` +} + +type markupContent struct { + Kind string `json:"kind"` + Value string `json:"value"` +} + +type documentSymbol struct { + Name string `json:"name"` + Detail string `json:"detail,omitempty"` + Kind int `json:"kind"` + Range lspRange `json:"range"` + SelectionRange lspRange `json:"selectionRange"` +} + +const symbolKindFunction = 12 + +// fullFormatEdits returns the whole-document formatting edit for uri, or an +// empty list when the document is unknown, already formatted, or the result +// fails the safety gate. +func (s *server) fullFormatEdits(uri string) []textEdit { + text, ok := s.docs[uri] + if !ok { + return []textEdit{} + } + formatted := format.File(parser.Parse(text), s.cfg) + if formatted == text || format.VerifySafe(text, formatted, s.cfg) != nil { + return []textEdit{} + } + return []textEdit{fullReplace(text, formatted)} +} + +// diagSpan returns the width in characters of the token starting at the given +// (0-based) line/col, so a diagnostic highlights the offending token rather +// than a single caret. Falls back to 1. +func diagSpan(text string, line, col uint32) uint32 { + off := positionToOffset(text, position{line, col}) + for _, t := range lexer.Lex(text) { + if t.Off == off && !t.IsTrivia() && t.Kind != lexer.EOF && !strings.Contains(t.Text, "\n") && len(t.Text) > 0 { + return uint32(len(t.Text)) + } + if t.Off > off { + break + } + } + return 1 +} + +// positionToOffset converts an LSP position to a byte offset in text. +func positionToOffset(text string, p position) int { + off := 0 + for line := uint32(0); line < p.Line; line++ { + i := strings.IndexByte(text[off:], '\n') + if i < 0 { + return len(text) + } + off += i + 1 + } + off += int(p.Character) + if off > len(text) { + off = len(text) + } + return off +} + +// hover describes the diagnostic under the cursor, if any. +func (s *server) hover(uri string, p position) *hoverResult { + for _, d := range s.diags[uri] { + if !rangesOverlap(d.Range.Start, d.Range.End, p, p) || p == d.Range.End { + continue + } + return &hoverResult{ + Contents: markupContent{Kind: "markdown", Value: "**" + d.Code + "**\n\n" + d.Message}, + Range: d.Range, + } + } + return nil +} + +// documentSymbols lists the CREATE FUNCTION / PROCEDURE statements in the document. +func (s *server) documentSymbols(uri string) []documentSymbol { + text, ok := s.docs[uri] + if !ok { + return []documentSymbol{} + } + out := []documentSymbol{} + for _, item := range parser.Parse(text).Items { + cf, ok := item.(*cst.CreateFunction) + if !ok || len(cf.Name) == 0 { + continue + } + all := cst.Tokens(cf) + last := all[len(all)-1].Tok + nameFirst, nameLast := cf.Name[0].Tok, cf.Name[len(cf.Name)-1].Tok + var name strings.Builder + for _, t := range cf.Name { + name.WriteString(t.Tok.Text) + } + detail := "function" + if cf.IsProcedure() { + detail = "procedure" + } + out = append(out, documentSymbol{ + Name: name.String(), + Detail: detail, + Kind: symbolKindFunction, + Range: lspRange{ + Start: offsetToPosition(text, all[0].Tok.Off), + End: offsetToPosition(text, last.Off+len(last.Text)), + }, + SelectionRange: lspRange{ + Start: offsetToPosition(text, nameFirst.Off), + End: offsetToPosition(text, nameLast.Off+len(nameLast.Text)), + }, + }) + } + return out +} diff --git a/pkg/lsp/server_test.go b/pkg/lsp/server_test.go index c322247..1a0bb21 100644 --- a/pkg/lsp/server_test.go +++ b/pkg/lsp/server_test.go @@ -268,3 +268,90 @@ func TestDidClose_ClearsDiagnostics(t *testing.T) { t.Errorf("expected empty diagnostics after didClose, got %d", len(lastDiags)) } } + +// request opens text at uri, sends one request, and returns its response +// (skipping the initialize response and any notifications). +func request(t *testing.T, text, method string, params map[string]interface{}) map[string]interface{} { + t.Helper() + uri := "file:///t.sql" + params["textDocument"] = map[string]interface{}{"uri": uri} + var input []byte + input = append(input, frame(1, "initialize", map[string]interface{}{})...) + input = append(input, notifFrame("initialized", nil)...) + input = append(input, notifFrame("textDocument/didOpen", map[string]interface{}{ + "textDocument": map[string]interface{}{"uri": uri, "languageId": "sql", "version": 1, "text": text}, + })...) + input = append(input, frame(2, method, params)...) + input = append(input, frame(3, "shutdown", nil)...) + input = append(input, notifFrame("exit", nil)...) + out := runServer(t, input) + for i := 0; i < 10; i++ { + resp := readResp(t, out) + if id, ok := resp["id"]; ok && id.(float64) == 2 { + return resp + } + } + t.Fatalf("no response to %s", method) + return nil +} + +func TestDocumentSymbol(t *testing.T) { + src := "create function public.foo(a int) returns void language plpgsql as $$ begin end $$;\n\ncreate procedure bar() language plpgsql as $$ begin end $$;\n" + resp := request(t, src, "textDocument/documentSymbol", map[string]interface{}{}) + syms := resp["result"].([]interface{}) + if len(syms) != 2 { + t.Fatalf("want 2 symbols, got %v", syms) + } + first := syms[0].(map[string]interface{}) + if first["name"] != "public.foo" || first["detail"] != "function" { + t.Errorf("first symbol: %v", first) + } + second := syms[1].(map[string]interface{}) + if second["name"] != "bar" || second["detail"] != "procedure" { + t.Errorf("second symbol: %v", second) + } + line := second["range"].(map[string]interface{})["start"].(map[string]interface{})["line"].(float64) + if line != 2 { + t.Errorf("bar should start on line 2, got %v", line) + } +} + +func TestHoverShowsDiagnostic(t *testing.T) { + resp := request(t, "select * from t;", "textDocument/hover", map[string]interface{}{ + "position": map[string]interface{}{"line": 0, "character": 7}, + }) + res, ok := resp["result"].(map[string]interface{}) + if !ok { + t.Fatalf("expected hover result, got %v", resp["result"]) + } + val := res["contents"].(map[string]interface{})["value"].(string) + if !strings.Contains(val, "COR001") { + t.Errorf("hover should name the rule, got %q", val) + } + resp = request(t, "select * from t;", "textDocument/hover", map[string]interface{}{ + "position": map[string]interface{}{"line": 0, "character": 14}, + }) + if resp["result"] != nil { + t.Errorf("no diagnostic at col 14, want null, got %v", resp["result"]) + } +} + +func TestWillSaveWaitUntilFormats(t *testing.T) { + resp := request(t, "select a from t", "textDocument/willSaveWaitUntil", map[string]interface{}{"reason": 1}) + edits := resp["result"].([]interface{}) + if len(edits) != 1 || !strings.Contains(edits[0].(map[string]interface{})["newText"].(string), "SELECT a") { + t.Errorf("expected a formatting edit, got %v", edits) + } +} + +func TestDiagSpanCoversToken(t *testing.T) { + if got := diagSpan("select * from t;", 0, 7); got != 1 { + t.Errorf("'*' span = %d, want 1", got) + } + if got := diagSpan("select foo from t;", 0, 7); got != 3 { + t.Errorf("'foo' span = %d, want 3", got) + } + if got := diagSpan("select foo from t;", 0, 40); got != 1 { + t.Errorf("out-of-range span = %d, want 1", got) + } +}