feat(lsp): hover, documentSymbol, willSaveWaitUntil, token-wide diagnostic ranges

2026-10-06 14:56:32 +02:00
parent e6c4e8b2c3
commit 488801b8b1
4 changed files with 277 additions and 53 deletions
@@ -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`.
@@ -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

@@ -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
+}
@@ -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)
+	}
+}