V0.0.4 lsp completions in buffer

[SHA888]

No description provided.

[SHA888]

Good diff visibility this time. Here's the full review:

---

PR #3 Review — v0.0.4-lsp-completions-in-buffer

2 commits, +427 / −23, 8 files. Implements textDocument/completion round-trip, ranking, buffer apply, document sync, and cursor-position tests.

---

🔴 Blockers

apply_completion ignores the textEdit range. This is a semantic correctness bug. The CompletionTextEdit::Edit branch extracts new_text but discards the range entirely and just inserts at cursor:

CompletionTextEdit::Edit(e) => e.new_text.as_str(),
// ...
let pos = self.cursor_char_index();
self.insert(pos, text);

LSP textEdit completions are meant to replace the range specified in the edit — that range covers the partial token already typed. rust-analyzer always sends textEdit not insertText for proper completions, so in practice this path will fire constantly and produce doubled text (e.g. fn fo + fn foo instead of replacing fn fo with fn foo). The InsertAndReplace variant has the same problem. The fix is to apply the edit's range as a delete before inserting, or add a Buffer::apply_text_edit(range, text) method.

Dead code shadow in update_cursor. The first line_len binding is immediately shadowed:

let line_len = self.rope.line(line).len_chars().saturating_sub(1); // dead — never read
// …
let mut line_len = self.rope.line(line).len_chars(); // shadows it

The first binding is never used. Clippy with -D warnings will catch this and fail CI.

---

⚠️ Issues worth addressing

rank_completions ignores sort_text. The LSP spec has a dedicated sort_text field on CompletionItem precisely for server-controlled ordering. rust-analyzer always populates it. The current implementation ranks by label length, which will override the server's intent and produce worse results. At minimum: sort by sort_text first, fall back to label length only when sort_text is absent.

rank_completions dedup is non-deterministic. HashMap::or_insert keeps whichever duplicate happens to be inserted first, but HashMap iteration order is random. When two items have the same label but different detail or sort_text, you'll silently drop one arbitrarily on each run. Use IndexMap or collect into a BTreeMap<String, CompletionItem> to make this deterministic.

rank_completions belongs in completion.rs, not on LspClient. It's a pure function with no self access. Putting it as an associated function on the client struct is the wrong layer. The fact that a completion.rs module was added in this PR suggests this was the intended home — it should move there.

Python mock subprocess for the completion test. Spawning python3 -c script works locally but python3 availability isn't guaranteed in the CI environment. The existing mock pattern from PR #2 (Rust-native mock via a script + piped stdio) is more portable. Either use the same pattern or gate the test with an environment check.

let _ = client.process.kill() — accessing .process directly in tests couples the test to the internal field name. Also .kill() on a process that's already exited returns an error which is silently dropped. Fine for now, but worth wrapping in a helper.

---

✅ What's solid

Buffer::apply_completion priority logic (textEdit > insertText > label) is correct per spec. The test matrix for apply_completion is thorough — buffer start, middle, end, each priority level covered. The move_cursor_to_end / cursor_char_index helpers are clean. The update_cursor fix (correctly clamping to max_col = line_len - 1 rather than line_len) is a genuine bug fix.

Document sync coverage in document_sync.rs is the right scope for this PR.

---

Verdict: request changes. The textEdit range bug is a correctness blocker — the completion path that rust-analyzer actually uses will silently corrupt buffer content. The sort_text omission and dead code shadow are secondary but should be fixed in the same pass before merge.