diff options
| -rw-r--r-- | docs/lsp.md | 43 | ||||
| -rw-r--r-- | src/lsp/lsp.zig | 16 | ||||
| -rw-r--r-- | src/lsp/lsp_zls.zig | 26 | ||||
| -rw-r--r-- | src/output_pane.zig | 7 | ||||
| -rw-r--r-- | src/pardes.zig | 117 | ||||
| -rw-r--r-- | test/snapshots/lsp-rename.golden | 75 | ||||
| -rw-r--r-- | test/snapshots/lsp-rename.snap | 27 |
7 files changed, 269 insertions, 42 deletions
diff --git a/docs/lsp.md b/docs/lsp.md index 258e409e..097386e6 100644 --- a/docs/lsp.md +++ b/docs/lsp.md @@ -23,9 +23,9 @@ core shell worker | | snapshot path + content | | |--------------------------->| | | | lsp.query(...) - | | Event .lsp_resp{id,rows}| + | | Event .lsp_resp{id,rows} | |<--------------------------|<---------------------------| - | lspResponse -> jump, or open a results buffer | + | lspResponse -> atomic edit, jump, or results buffer | ``` The shell already ran this exact pattern for pty readers, so the async part is @@ -40,8 +40,10 @@ Three rules make it safe: query is in flight; a borrowed slice would be a use-after-free the length of one keystroke. - **One query in flight, identified by a monotonic id.** A second press bumps - the id, which makes the older answer stale — `lspResponse` drops any id it is - not waiting for. This is also what makes a closed pane safe. + the id, which makes the older answer stale. The pending request also records + the pane serial, so a closed-and-reused slot cannot accept its response. +- **Mutating answers are revision-checked.** Rename records the file revision + sent to the worker and applies nothing if the user edited before it answered. - **No rows is a legal answer.** A backend that cannot answer appends nothing, which is indistinguishable from a language server still starting up, and the core does nothing. There is no error path to render. @@ -67,10 +69,19 @@ already produces and `n`/`N` already step — so: - **several rows** → an output buffer, which `n`/`N` walk which means helix's multi-result picker required **no picker code at all**. The -`+Search` buffer *is* the picker. Kinds whose answer is prose rather than -locations (`hover`, `code_action`, `format`, `rename`) open `+Hover`/`+Lsp` -instead and do not arm the stepper — `n` over a documentation blurb would step -to nowhere. +`+Search` buffer *is* the picker. Non-location answers (`hover`, `code_action`, +`format`) open `+Hover`/`+Lsp` instead and do not arm the stepper — `n` over a +documentation blurb would step to nowhere. + +Rename is deliberately the one exception to rows as presentation. The backend +emits `@edit START END` records through `lsp.edit()`, using half-open byte +offsets into the exact `Req.source` snapshot it resolved. The core validates +that every range is ordered, non-overlapping and in bounds, checks that the +pane serial and file revision still match, then substitutes the requested name +across all ranges with one allocation and one undo transaction. A malformed, +stale, or empty response changes nothing. The current ZLS backend resolves and +renames references in the **current file only**; it does not claim a workspace +rename. **A path UNDER `Req.root` is written relative to it; everything else keeps its full absolute path** (`lsp.rel`). `Req.root` is the directory of the file the @@ -186,7 +197,7 @@ Verified against `helix-term/src/keymap/default.rs`, not from memory. | `gi` | implementation | | | `gr` | references | | | `SPC l k` | hover | opens `+Hover` | -| `SPC l r` | rename | tag input, like Find/Grep | +| `SPC l r` | rename | tag input; applies current-file references in one undo step | | `SPC l a` | code action | | | `SPC l h` | select references | | | `SPC l s` / `SPC l S` | document / workspace symbols | `S` takes a query | @@ -266,9 +277,11 @@ read is in `req` (`path`, `source` (NUL-terminated), `offset`, `arg`, `root`). `Io.Writer.Allocating`), so a backend never allocates the result, never frees it, and cannot get the allocator wrong. `arena` is freed wholesale on return; `gpa` is for a backend's own scratch. Use `lsp.row()` to emit a location, -`lsp.rel()` to spell its path against `req.root` and `lsp.lineCol()` to convert -an offset, so every backend's rows are byte-identical in shape. `rel` allocates -nothing — it returns a slice of what you hand it. +`lsp.rel()` to spell its path against `req.root`, `lsp.lineCol()` to convert an +offset, and `lsp.edit()` for each half-open range of a rename response. Location +rows are byte-identical across backends; rename ranges are consumed by the core +and never rendered. `rel` allocates nothing — it returns a slice of what you +hand it. ## How the implementations are judged @@ -281,6 +294,7 @@ probes, every backend. and a kind that answers without claiming is `unclaimed-works`. Correctness is a substring the rows must contain, so returning a confident wrong location scores worse than returning nothing. + - **Latency.** `cold` (first query, index construction included) and `warm` (median of 20). They differ by orders of magnitude for an indexing backend and both matter: cold is what the first keypress costs, warm is what every @@ -292,4 +306,9 @@ probes, every backend. but is charged in build time and dependency surface rather than in lines we maintain. +The user-visible rename contract is also pinned through the actual TTY, +leader prompt, worker and ZLS backend by `test/snapshots/lsp-rename.snap`: both +resolved occurrences change, a shadowed local does not, and undo/redo treats +the response as one transaction. + Run `zig build lspbench -- --json` for machine-readable output. diff --git a/src/lsp/lsp.zig b/src/lsp/lsp.zig index 90d4fc6b..29e82968 100644 --- a/src/lsp/lsp.zig +++ b/src/lsp/lsp.zig @@ -6,14 +6,14 @@ //! execution model — the same shape the pty readers already use, because a //! language query is just another thing that answers later. //! -//! Every backend renders into ONE format: `+Search` rows. A location is +//! Location answers render as `+Search` rows. A location is //! `path:LINE:COL text` — or `path:LINE:COL-ENDCOL text` where the protocol //! answered with a real range, which a look then SELECTS — and that is what //! look.zig already resolves and what n/N already steps, so a multi-result //! answer IS helix's picker and a single result IS a jump, with no picker UI -//! written for it. Free text (hover, a rename's diff) rides the same buffer as -//! plain lines. A path under `Req.root` is written relative to it and any -//! other keeps its full absolute self — see `rel`. +//! written for it. Free text (hover, formatting) rides the same buffer. Rename +//! is the one mutating answer: it emits byte ranges through `edit`, and the core +//! applies them atomically only while the source revision is still current. //! //! `query` is the ONLY thing an implementation supplies. Swapping backends is //! swapping this one function, which is also how the three competing @@ -162,6 +162,14 @@ pub fn spanRow( }) catch {}; } +/// Emit one half-open byte range for a mutating response. Rename is the only +/// current user: every other answer remains human-readable rows. Byte offsets +/// avoid converting the displayed 1-based locations back into source offsets +/// in the core, and the prefix makes malformed or mixed responses fail closed. +pub fn edit(out: *std.Io.Writer, start: usize, end: usize) void { + out.print("@edit {d} {d}\n", .{ start, end }) catch {}; +} + /// Byte offset -> (line, column), both 0-based. Every backend needs it to turn /// an AST token into a row, so it lives here rather than three times over. pub fn lineCol(source: []const u8, offset: usize) struct { line: usize, col: usize } { diff --git a/src/lsp/lsp_zls.zig b/src/lsp/lsp_zls.zig index 76c5594c..bf220c47 100644 --- a/src/lsp/lsp_zls.zig +++ b/src/lsp/lsp_zls.zig @@ -1123,9 +1123,9 @@ fn containsIgnoreCase(hay: []const u8, needle: []const u8) bool { /// milliseconds, on every keypress, and the store's own workspace iteration /// has the same restriction (it can only see handles that were loaded). /// -/// `new_name` non-null makes it a rename PREVIEW: the same rows, annotated -/// with the replacement. The seam returns rows, not edits, so `SPC r` shows -/// what would change and changes nothing — an honest half of rename. +/// `new_name` non-null makes it a rename EDIT: the same resolved tokens become +/// half-open byte ranges. The core owns the replacement text and applies every +/// range in one undo transaction after checking the source revision. fn references( arena: std.mem.Allocator, analyser: *Analyser, @@ -1149,10 +1149,10 @@ fn references( const want = offsets.identifierTokenToNameSlice(decl_tree, name_tok); if (want.len == 0) return; - const lines: Lines = try .build(arena, tree.source); - // every row names THIS file (the walk is this file's tokens), so the path - // is spelled once rather than per hit - const path = lsp.rel(base, handle.uri.toFsPath(arena) catch return); + // Rename consumes exact byte ranges. Reference rows need the source line + // and displayed path; avoid building either for the mutating response. + const lines: ?Lines = if (new_name == null) try .build(arena, tree.source) else null; + const path = if (new_name == null) lsp.rel(base, handle.uri.toFsPath(arena) catch return) else ""; var n: usize = 0; for (0..tree.tokens.len) |i| { if (n >= max_rows) return; @@ -1163,12 +1163,12 @@ fn references( const d = (declAt(arena, analyser, handle, at) catch continue) orelse continue; if (!d.eql(target)) continue; n += 1; - const r = offsets.tokenToRange(tree, tok, enc); - const text = if (new_name) |nn| - try std.fmt.allocPrint(arena, "{s} -> {s} {s}", .{ want, nn, std.mem.trim(u8, lines.line(r.start.line), " \t") }) - else - lines.line(r.start.line); - lsp.spanRow(out, path, r.start.line, r.start.character, r.end.line, r.end.character, text); + if (new_name != null) { + lsp.edit(out, at, at + want.len); + } else { + const r = offsets.tokenToRange(tree, tok, enc); + lsp.spanRow(out, path, r.start.line, r.start.character, r.end.line, r.end.character, lines.?.line(r.start.line)); + } } } diff --git a/src/output_pane.zig b/src/output_pane.zig index 7b2d12d9..8c93709c 100644 --- a/src/output_pane.zig +++ b/src/output_pane.zig @@ -81,7 +81,7 @@ pub const Traits = struct { name: []const u8, /// n/N walk the rows: each is a `path:LINE:COL text` location the ordinary /// look path resolves, so the buffer IS helix's picker. Prose (a hover - /// blurb, a rename diff) has nowhere to step to. + /// blurb, a formatting diff) has nowhere to step to. steps: bool = false, /// ...and what a step DOES with the row it lands on. Off, the row is a /// LOCATION and its leading word is LOOKED. On, the row is a COMMAND LINE @@ -135,7 +135,10 @@ pub fn traits(o: Origin) Traits { .query => |k| switch (k) { .hover => .{ .name = config.hover_buffer }, // prose: an action list, a diff, a report about the backend - .code_action, .format, .rename, .status, .explain => .{ .name = config.lsp_buffer }, + .code_action, .format, .status, .explain => .{ .name = config.lsp_buffer }, + // Rename responses are edits consumed before an output can open; + // the exhaustive table still records the otherwise-unused trait. + .rename => .{ .name = config.lsp_buffer }, .definition, .declaration, .type_definition, .implementation, .references => .{ .name = config.search_buffer, .steps = true, diff --git a/src/pardes.zig b/src/pardes.zig index 4b9e8b1d..4ce4d101 100644 --- a/src/pardes.zig +++ b/src/pardes.zig @@ -4321,16 +4321,24 @@ pub const Pardes = struct { /// this one" — the id bump makes the older answer stale and lspResponse /// drops it. A queue would only buy the right to render an answer nobody /// is waiting for any more. - /// `arg` rides along only so the buffer the answer opens can record what - /// was asked (a rename's new name, a symbol query) — the query itself has - /// it in the effect already. - /// `row`/`col` are where the cursor was when the question was asked. Only - /// `completion` reads them, and only to undo itself: Tab diverted instead - /// of indenting, so an empty answer has to put the indent back — but only - /// if the cursor has not moved since, or four spaces appear under someone - /// who kept typing. + /// `arg` holds the replacement name for rename and the query for workspace + /// symbols. `serial` rejects a response after its pane slot was reused; + /// `revision` makes a mutating rename conditional on the source snapshot + /// the worker actually analysed. `row`/`col` are where the cursor was when + /// the question was asked. Only `completion` reads them, and only to undo + /// itself: Tab diverted instead of indenting, so an empty answer has to put + /// the indent back — but only if the cursor has not moved since. lsp_seq: u32 = 0, - lsp_wait: ?struct { id: u32, kind: lsp.Kind, pane: usize, arg: Effect.Buf(128), row: i32 = 0, col: i32 = 0 } = null, + lsp_wait: ?struct { + id: u32, + kind: lsp.Kind, + pane: usize, + serial: u32, + revision: u32, + arg: Effect.Buf(128), + row: i32 = 0, + col: i32 = 0, + } = null, /// One current shell-filter request. A newer submit frees and supersedes /// it; old worker answers then fail the id check. The request itself owns @@ -7642,6 +7650,7 @@ pub const Pardes = struct { if (f.output != null) return; } if (arg.len > 128) return; // the effect's arg is a Buf(128) + if (kind == .rename and (!std.zig.isValidId(arg) or std.zig.isUnderscore(arg))) return; const off = if (pane.file) |f| modal.hxOff(f.content, .{ .row = @intCast(@max(0, pane.cur_row)), .col = @intCast(@max(0, pane.cur_col)), @@ -7651,6 +7660,8 @@ pub const Pardes = struct { .id = p.lsp_seq, .kind = kind, .pane = id, + .serial = pane.serial, + .revision = if (pane.file) |f| f.revision else 0, .arg = .from(arg), .row = pane.cur_row, .col = pane.cur_col, @@ -7664,8 +7675,90 @@ pub const Pardes = struct { } }); } - /// A worker answered. Rows are `+Search` format with ABSOLUTE paths, so - /// both dispositions below are the ordinary look path: + const LspEdit = struct { start: usize, end: usize }; + + fn parseLspEdits(p: *Pardes, bytes: []const u8) ?[]LspEdit { + if (bytes.len == 0 or bytes[bytes.len - 1] != '\n') return null; + const edits = p.scratch.allocator().alloc(LspEdit, std.mem.count(u8, bytes, "\n")) catch return null; + var lines = std.mem.splitScalar(u8, bytes, '\n'); + var n: usize = 0; + while (lines.next()) |line| { + if (line.len == 0) { + if (lines.peek() == null) break; + return null; + } + var fields = std.mem.tokenizeScalar(u8, line, ' '); + if (!std.mem.eql(u8, fields.next() orelse return null, "@edit")) return null; + const start = std.fmt.parseInt(usize, fields.next() orelse return null, 10) catch return null; + const end = std.fmt.parseInt(usize, fields.next() orelse return null, 10) catch return null; + if (fields.next() != null) return null; + edits[n] = .{ .start = start, .end = end }; + n += 1; + } + return if (n == 0) null else edits[0..n]; + } + + fn mapLspEditOffset(edits: []const LspEdit, replacement_len: usize, old: usize) usize { + var old_at: usize = 0; + var new_at: usize = 0; + for (edits) |e| { + if (old < e.start) return new_at + (old - old_at); + new_at += e.start - old_at; + if (old < e.end) return new_at + @min(old - e.start, replacement_len - 1); + new_at += replacement_len; + if (old == e.end) return new_at; + old_at = e.end; + } + return new_at + (old - old_at); + } + + fn applyLspRename(p: *Pardes, pane: *Pane, revision: u32, new_name: []const u8, bytes: []const u8) void { + const f = if (pane.file) |*file| file else return; + if (f.revision != revision) return; + const edits = p.parseLspEdits(bytes) orelse return; + + var removed: usize = 0; + var previous_end: usize = 0; + for (edits) |e| { + if (e.start < previous_end or e.start >= e.end or e.end > f.content.len) return; + removed = std.math.add(usize, removed, e.end - e.start) catch return; + previous_end = e.end; + } + if (std.mem.eql(u8, f.content[edits[0].start..edits[0].end], new_name)) return; + const added = std.math.mul(usize, edits.len, new_name.len) catch return; + const final_len = std.math.add(usize, f.content.len - removed, added) catch return; + const replacement = p.gpa.alloc(u8, final_len) catch return; + + const old_cursor = modal.hxOff(f.content, .{ + .row = @intCast(@max(0, pane.cur_row)), + .col = @intCast(@max(0, pane.cur_col)), + }); + const mapped_cursor = mapLspEditOffset(edits, new_name.len, old_cursor); + var read_at: usize = 0; + var write_at: usize = 0; + for (edits) |e| { + @memcpy(replacement[write_at .. write_at + (e.start - read_at)], f.content[read_at..e.start]); + write_at += e.start - read_at; + @memcpy(replacement[write_at .. write_at + new_name.len], new_name); + write_at += new_name.len; + read_at = e.end; + } + @memcpy(replacement[write_at..], f.content[read_at..]); + + p.pushUndo(pane); + file_pane.setContent(p, f, replacement); + const cursor = modal.hxPos(f.content, mapped_cursor); + pane.cur_row = @intCast(cursor.row); + pane.cur_col = @intCast(cursor.col); + pane.vsel.active = false; + pane.msel.active = false; + pane.select = false; + pane.sticky_col = -1; + pane.ensureCursorVisible(); + } + + /// A worker answered. Rename's edit records are consumed first and never + /// rendered. Every other response is the ordinary look/output path: /// one row, a goto -> jump straight there (helix jumps on a single /// location and shows a picker on several) /// anything else -> an output buffer, which n/N already steps. That @@ -7675,6 +7768,8 @@ pub const Pardes = struct { if (w.id != id) return; // superseded by a newer press, or the pane died p.lsp_wait = null; const pane = p.panes[w.pane] orelse return; + if (pane.serial != w.serial) return; + if (w.kind == .rename) return p.applyLspRename(pane, w.revision, w.arg.slice(), rows); if (rows.len == 0) { // No rows is a legal answer everywhere except here. Tab DIVERTED // instead of indenting, so an empty answer would eat the keystroke diff --git a/test/snapshots/lsp-rename.golden b/test/snapshots/lsp-rename.golden new file mode 100644 index 00000000..c15092b7 --- /dev/null +++ b/test/snapshots/lsp-rename.golden @@ -0,0 +1,75 @@ +== snap renamed grid=110x24 cursor=17,9 +|New Newcol Find Grep Help Tutor Dump NextColor Debug Kill +| /tmp/pardes-snap/lsp-rename/cwd/rename.zig Save New Del +| 1 const std = @import("std"); +| 2 +| 3 fn renamed_helper(x: u32) u32 { +| 4 return x + 1; +| 5 } +| 6 +| 7 pub fn main() void { +| 8 _ = renamed_helper(41); +| 9 } +| 10 +| 11 fn shadowed() void { +| 12 const helper: u32 = 7; +| 13 _ = helper; +| 14 } +| 15 +| +| +| +| +| +| +| +== snap undone grid=110x24 cursor=17,9 +|New Newcol Find Grep Help Tutor Dump NextColor Debug Kill +| /tmp/pardes-snap/lsp-rename/cwd/rename.zig Save New Del +| 1 const std = @import("std"); +| 2 +| 3 fn helper(x: u32) u32 { +| 4 return x + 1; +| 5 } +| 6 +| 7 pub fn main() void { +| 8 _ = helper(41); +| 9 } +| 10 +| 11 fn shadowed() void { +| 12 const helper: u32 = 7; +| 13 _ = helper; +| 14 } +| 15 +| +| +| +| +| +| +| +== snap redone grid=110x24 cursor=17,9 +|New Newcol Find Grep Help Tutor Dump NextColor Debug Kill +| /tmp/pardes-snap/lsp-rename/cwd/rename.zig Save New Del +| 1 const std = @import("std"); +| 2 +| 3 fn renamed_helper(x: u32) u32 { +| 4 return x + 1; +| 5 } +| 6 +| 7 pub fn main() void { +| 8 _ = renamed_helper(41); +| 9 } +| 10 +| 11 fn shadowed() void { +| 12 const helper: u32 = 7; +| 13 _ = helper; +| 14 } +| 15 +| +| +| +| +| +| +| diff --git a/test/snapshots/lsp-rename.snap b/test/snapshots/lsp-rename.snap new file mode 100644 index 00000000..c29b001e --- /dev/null +++ b/test/snapshots/lsp-rename.snap @@ -0,0 +1,27 @@ +# Rename end to end: the leader prompt, async ZLS resolution, atomic file edit, +# shadow filtering, and one-step undo/redo. The source has a same-spelled local +# which must not be changed when the top-level helper is renamed. +file rename.zig const std = @import("std");\n\nfn helper(x: u32) u32 {\n return x + 1;\n}\n\npub fn main() void {\n _ = helper(41);\n}\n\nfn shadowed() void {\n const helper: u32 = 7;\n _ = helper;\n}\n +start 24 110 rename.zig +wait 8000 helper(41) +stable 700 20000 +# Put the cursor inside the top-level call, then use the helix rename binding. +press left 18 10 +release left 18 10 +key space +key l +key r +stable 400 5000 +text renamed_helper +key enter +wait 10000 renamed_helper(41) +stable 700 15000 +snap renamed +# Both resolved top-level occurrences changed; the shadowed local stayed helper. +# The whole asynchronous rename is one history transaction. +key u +stable 400 5000 +snap undone +key U +stable 400 5000 +snap redone |