From dd6ec68762ce52ec05ac6e5eef6c727943edc683 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 9 Jul 2026 22:16:06 -0400 Subject: [PATCH] fix(lsp): convert rename/prepareRename positions per position encoding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit request_rename and request_prepare_rename sent raw byte columns instead of routing through outbound_position — the same bug class as the semantic-range and code-action fixes that just merged (#105). On a UTF-16 server, a rename at a position past non-ASCII text resolves the wrong character (or an invalid one) and renames the wrong symbol. Both single-Position builders now convert. The posecho fake validates request positions on its rename/prepareRename arms in UTF-16 units, and the new test drives both requests at byte offset 3 of "éx" (UTF-16 character 2) — both stores filling proves both builders converted. (Fix authored locally by Levi during the round-5 review; recovered from the working tree after the #105 merge and landed verbatim, plus a cargo fmt pass.) Co-Authored-By: Claude Fable 5 --- src/bin/pmacs_fake_lsp.rs | 72 +++++++++++++++++++++++++++++++++++++++ src/lsp.rs | 4 +-- tests/m4_acceptance.rs | 36 ++++++++++++++++++++ 3 files changed, 110 insertions(+), 2 deletions(-) diff --git a/src/bin/pmacs_fake_lsp.rs b/src/bin/pmacs_fake_lsp.rs index 9be1cd0..d5b916b 100644 --- a/src/bin/pmacs_fake_lsp.rs +++ b/src/bin/pmacs_fake_lsp.rs @@ -593,6 +593,20 @@ fn main() { write_frame(&mut stdout, &resp); } ("textDocument/prepareRename", Some(idv)) => { + // `posecho` negotiates UTF-16. Validate the request + // position before replying so the position-codec test + // below catches byte-column regressions in this builder. + if mode == "posecho" + && let Some(message) = utf16_position_error(¶ms, &open_docs) + { + let resp = serde_json::json!({ + "jsonrpc": "2.0", + "id": idv, + "error": { "code": -32602, "message": message } + }); + write_frame(&mut stdout, &resp); + continue; + } // T M4.5: `preprefuse` → null (not renameable here); // otherwise the `{ range, placeholder }` shape over // the line-0 cols 3..6 span ("foo"). @@ -615,6 +629,19 @@ fn main() { write_frame(&mut stdout, &resp); } ("textDocument/rename", Some(idv)) => { + // Same UTF-16 validation as prepareRename: rename and + // prepareRename both carry a single Position. + if mode == "posecho" + && let Some(message) = utf16_position_error(¶ms, &open_docs) + { + let resp = serde_json::json!({ + "jsonrpc": "2.0", + "id": idv, + "error": { "code": -32602, "message": message } + }); + write_frame(&mut stdout, &resp); + continue; + } // T M4.5 L2: reply with a `WorkspaceEdit`. The edit // replaces the 3-char span at line 0, cols 3..6 with // the requested `newName` (so the test can assert the @@ -1126,6 +1153,51 @@ fn utf16_range_error( } } +/// `posecho` validation for single-position requests. Unlike the +/// whole-document range fixture, this checks the requested line itself +/// so an overlarge byte column cannot hide behind a later line. +fn utf16_position_error( + params: &serde_json::Value, + open_docs: &HashMap, +) -> Option { + let Some(uri) = params + .get("textDocument") + .and_then(|t| t.get("uri")) + .and_then(serde_json::Value::as_str) + else { + return Some("position request carried no textDocument.uri".into()); + }; + let Some(text) = open_docs.get(uri) else { + return Some(format!("no didOpen text recorded for {uri}")); + }; + let Some(position) = params.get("position") else { + return Some("position request carried no position".into()); + }; + let Some(line) = position.get("line").and_then(serde_json::Value::as_u64) else { + return Some("position request carried no numeric line".into()); + }; + let Some(col) = position + .get("character") + .and_then(serde_json::Value::as_u64) + else { + return Some("position request carried no numeric character".into()); + }; + let Ok(line_index) = usize::try_from(line) else { + return Some(format!("invalid utf-16 position line {line}")); + }; + let Some(line_text) = text.split('\n').nth(line_index) else { + return Some(format!("invalid utf-16 position line {line}")); + }; + let max_col = line_text.chars().map(char::len_utf16).sum::() as u64; + if col > max_col { + Some(format!( + "invalid utf-16 position {line}:{col}; line ends at {line}:{max_col}" + )) + } else { + None + } +} + fn inlay_range_error( params: &serde_json::Value, open_docs: &HashMap, diff --git a/src/lsp.rs b/src/lsp.rs index 9849b3e..afabd9d 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -1951,7 +1951,7 @@ impl LspManager { let uri = uri.into(); let params = json!({ "textDocument": { "uri": uri.clone() }, - "position": { "line": line, "character": col }, + "position": self.outbound_position(sid, &uri, line, col), "newName": new_name.into(), }); let req_id = self.send_request(sid, "textDocument/rename", params)?; @@ -1976,7 +1976,7 @@ impl LspManager { let uri = uri.into(); let params = json!({ "textDocument": { "uri": uri.clone() }, - "position": { "line": line, "character": col }, + "position": self.outbound_position(sid, &uri, line, col), }); let req_id = self.send_request(sid, "textDocument/prepareRename", params)?; let job_id = self.register_awaiter(sid, req_id, "textDocument/prepareRename", &uri); diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index bb8b77f..70d674a 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -6470,6 +6470,42 @@ fn m4_5_position_encoding_utf16_round_trips_non_ascii() { ); } +/// T M4.5 Option B — rename and prepareRename are single-Position +/// requests too. At byte offset 3 (the end of `éx`), a UTF-16 server +/// must receive character 2, not byte column 3. The `posecho` fake +/// rejects an out-of-bounds UTF-16 position, so both response stores +/// appearing proves both request builders used `outbound_position`. +#[test] +fn m4_5_utf16_rename_and_prepare_rename_convert_positions() { + use pmacs::editor::EditorState; + + let mut state = EditorState::new(); + spawn_lsp_and_init(&mut state, Some("posecho")); + let uri = "file:///tmp/m4_5_rename_utf16.rs"; + state + .lua_host + .lua() + .load(format!( + "pmacs.lsp.did_open(_G._lsp, '{uri}', 1, 'éx')\n\ + pmacs.lsp.request_prepare_rename(_G._lsp, '{uri}', 0, 3)\n\ + pmacs.lsp.request_rename(_G._lsp, '{uri}', 0, 3, 'renamed')" + )) + .exec() + .expect("dispatch UTF-16 rename requests"); + + let both_landed = format!( + "(function() \ + local pr = pmacs.prepare_rename.result(_G._lsp, '{uri}') \ + local ops = pmacs.rename.ops(_G._lsp, '{uri}') \ + return pr ~= nil and ops ~= nil and #ops > 0 \ + end)()" + ); + assert!( + pump_lua_flag(&mut state, &both_landed, 5), + "rename and prepareRename must send UTF-16, not byte, columns" + ); +} + /// T M4.5: pmacs answers the server→client `workspace/configuration` /// pull from the per-server `settings` (the capability gopls / /// pyright / clangd rely on). The `wsconfig` fake issues the request