From e28df13e4b35df2c011dd329bda15cbd29f845d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20K=C3=A5gedal=20Reimer?= Date: Wed, 13 May 2026 20:44:17 +0200 Subject: [PATCH 01/10] feat(rename): support renaming service and RPC symbols MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename was previously gated on message_name/enum_name parents in can_rename, so invoking rename on a service or RPC method silently did nothing. Add a focused is_renameable predicate that also accepts service_name and rpc_name, and handle the empty-ancestor case in rename_tree by passing the unqualified identifier to the workspace pass — services and RPCs have no cross-file references, so that pass becomes a harmless no-op for them. --- src/nodekind.rs | 6 ++ src/parser/input/test_rename_service.proto | 14 ++++ src/parser/rename.rs | 72 ++++++++++++++++++- ...e__test__can_rename_service_and_rpc-2.snap | 10 +++ ...e__test__can_rename_service_and_rpc-3.snap | 5 ++ ...ame__test__can_rename_service_and_rpc.snap | 10 +++ ...ename__test__rename_service_and_rpc-2.snap | 12 ++++ ..._rename__test__rename_service_and_rpc.snap | 12 ++++ 8 files changed, 140 insertions(+), 1 deletion(-) create mode 100644 src/parser/input/test_rename_service.proto create mode 100644 src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc-2.snap create mode 100644 src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc-3.snap create mode 100644 src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc.snap create mode 100644 src/parser/snapshots/protols__parser__rename__test__rename_service_and_rpc-2.snap create mode 100644 src/parser/snapshots/protols__parser__rename__test__rename_service_and_rpc.snap diff --git a/src/nodekind.rs b/src/nodekind.rs index fc042e4..8909825 100644 --- a/src/nodekind.rs +++ b/src/nodekind.rs @@ -67,6 +67,12 @@ impl NodeKind { n.kind() == Self::EnumName.as_str() || n.kind() == Self::MessageName.as_str() } + pub fn is_renameable(n: &Node) -> bool { + Self::is_userdefined(n) + || n.kind() == Self::ServiceName.as_str() + || n.kind() == Self::RpcName.as_str() + } + pub fn is_actionable(n: &Node) -> bool { n.kind() == Self::MessageName.as_str() || n.kind() == Self::EnumName.as_str() diff --git a/src/parser/input/test_rename_service.proto b/src/parser/input/test_rename_service.proto new file mode 100644 index 0000000..c7a50c0 --- /dev/null +++ b/src/parser/input/test_rename_service.proto @@ -0,0 +1,14 @@ +syntax = "proto3"; + +package com.parser; + +message Empty {} + +message Book { + string title = 1; +} + +service Library { + rpc GetBook(Empty) returns (Book); + rpc ListBooks(Empty) returns (Book); +} diff --git a/src/parser/rename.rs b/src/parser/rename.rs index fee43bd..a096119 100644 --- a/src/parser/rename.rs +++ b/src/parser/rename.rs @@ -11,7 +11,7 @@ impl ParsedTree { .filter(NodeKind::is_identifier) .and_then(|n| { if let Some(parent) = n.parent() - && NodeKind::is_userdefined(&parent) + && NodeKind::is_renameable(&parent) { Some(Range { start: ts_to_lsp_position(&n.start_position()), @@ -84,6 +84,18 @@ impl ParsedTree { let nodes = self.get_ancestor_nodes_at_position(pos); + // Renameable symbols that don't sit inside a message (services, RPCs, top-level + // enums) have no qualifying ancestors. Hand the unqualified name to the workspace + // pass so it can update any references; services and RPCs have none. + if nodes.is_empty() { + let otext = self + .get_node_at_position(pos)? + .utf8_text(content.as_ref()) + .ok()? + .to_owned(); + return Some((v, otext, new_name.to_owned())); + } + let mut i = 1; let mut otext = nodes.first()?.utf8_text(content.as_ref()).ok()?.to_owned(); let mut ntext = new_name.to_owned(); @@ -262,4 +274,62 @@ mod test { assert_yaml_snapshot!(tree.can_rename(&pos_inner_type)); assert_yaml_snapshot!(tree.can_rename(&pos_outer_type)); } + + #[test] + fn test_can_rename_service_and_rpc() { + let uri: Url = "file://foo/bar/test.proto".parse().unwrap(); + let contents = include_str!("input/test_rename_service.proto"); + let parsed = ProtoParser::new().parse(uri, contents); + assert!(parsed.is_some()); + let tree = parsed.unwrap(); + + let pos_service = Position { + line: 10, + character: 10, + }; + let pos_rpc = Position { + line: 11, + character: 9, + }; + let pos_rpc_request_type = Position { + line: 11, + character: 17, + }; + + assert_yaml_snapshot!(tree.can_rename(&pos_service)); + assert_yaml_snapshot!(tree.can_rename(&pos_rpc)); + // Type references inside an RPC declaration are not (yet) renameable + // from the reference site itself — that's a separate change. + assert_yaml_snapshot!(tree.can_rename(&pos_rpc_request_type)); + } + + #[test] + fn test_rename_service_and_rpc() { + let uri: Url = "file://foo/bar.proto".parse().unwrap(); + let contents = include_str!("input/test_rename_service.proto"); + let parsed = ProtoParser::new().parse(uri, contents); + assert!(parsed.is_some()); + let tree = parsed.unwrap(); + + let pos_service = Position { + line: 10, + character: 10, + }; + let pos_rpc = Position { + line: 11, + character: 9, + }; + + let rename_fn = |nt: &str, pos: &Position| match tree.rename_tree(pos, nt, contents) { + Some(k) => { + let mut v = tree.rename_field(&k.1, &k.2, contents); + v.extend(k.0); + v + } + _ => vec![], + }; + + assert_yaml_snapshot!(rename_fn("Catalog", &pos_service)); + assert_yaml_snapshot!(rename_fn("FetchBook", &pos_rpc)); + } } diff --git a/src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc-2.snap b/src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc-2.snap new file mode 100644 index 0000000..e4e36a2 --- /dev/null +++ b/src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc-2.snap @@ -0,0 +1,10 @@ +--- +source: src/parser/rename.rs +expression: tree.can_rename(&pos_rpc) +--- +start: + line: 11 + character: 8 +end: + line: 11 + character: 15 diff --git a/src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc-3.snap b/src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc-3.snap new file mode 100644 index 0000000..ede6d8d --- /dev/null +++ b/src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc-3.snap @@ -0,0 +1,5 @@ +--- +source: src/parser/rename.rs +expression: tree.can_rename(&pos_rpc_request_type) +--- +~ diff --git a/src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc.snap b/src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc.snap new file mode 100644 index 0000000..74b2212 --- /dev/null +++ b/src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc.snap @@ -0,0 +1,10 @@ +--- +source: src/parser/rename.rs +expression: tree.can_rename(&pos_service) +--- +start: + line: 10 + character: 8 +end: + line: 10 + character: 15 diff --git a/src/parser/snapshots/protols__parser__rename__test__rename_service_and_rpc-2.snap b/src/parser/snapshots/protols__parser__rename__test__rename_service_and_rpc-2.snap new file mode 100644 index 0000000..6359cbc --- /dev/null +++ b/src/parser/snapshots/protols__parser__rename__test__rename_service_and_rpc-2.snap @@ -0,0 +1,12 @@ +--- +source: src/parser/rename.rs +expression: "rename_fn(\"FetchBook\", &pos_rpc)" +--- +- range: + start: + line: 11 + character: 8 + end: + line: 11 + character: 15 + newText: FetchBook diff --git a/src/parser/snapshots/protols__parser__rename__test__rename_service_and_rpc.snap b/src/parser/snapshots/protols__parser__rename__test__rename_service_and_rpc.snap new file mode 100644 index 0000000..b69ea55 --- /dev/null +++ b/src/parser/snapshots/protols__parser__rename__test__rename_service_and_rpc.snap @@ -0,0 +1,12 @@ +--- +source: src/parser/rename.rs +expression: "rename_fn(\"Catalog\", &pos_service)" +--- +- range: + start: + line: 10 + character: 8 + end: + line: 10 + character: 15 + newText: Catalog From 6a750a9602aed745a8a177ff8b4cf60a66fa4fbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20K=C3=A5gedal=20Reimer?= Date: Wed, 13 May 2026 20:52:38 +0200 Subject: [PATCH 02/10] feat(rename): support renaming from a type-reference site MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously rename was only available at the declaration of a message or enum. Invoking Rename Symbol on a type reference (e.g. the `MyRequest` in `rpc DoThing(MyRequest)`, or the `Author` in `Book.Author field = 1;`) was rejected by can_rename, forcing users to first navigate to the declaration. The LSP rename handler now detects when the cursor is on an identifier inside a `message_or_enum_type` node and pivots: it builds the partial qualified path up to and including the segment under cursor (`Book.Author` vs `Book`, depending on which segment the cursor is on), resolves it to the declaration via the existing definition machinery, and runs the regular rename from there. The workspace pass then propagates the rename to all references — including the site the user was standing on. Existing can_rename snapshots that previously returned None at reference positions are updated to reflect the new behavior. --- src/lsp.rs | 46 ++++++-- src/nodekind.rs | 1 + src/parser/rename.rs | 101 +++++++++++++++++- ...s__parser__rename__test__can_rename-3.snap | 7 +- ...s__parser__rename__test__can_rename-4.snap | 7 +- ...e__test__can_rename_service_and_rpc-3.snap | 7 +- 6 files changed, 157 insertions(+), 12 deletions(-) diff --git a/src/lsp.rs b/src/lsp.rs index 73450a3..f042fe9 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -20,6 +20,7 @@ use async_lsp::{Error, LanguageClient, ResponseError}; use futures::future::BoxFuture; use serde_json::Value; +use crate::context::jumpable::Jumpable; use crate::formatter::ProtoFormatter; use crate::server::ProtoLanguageServer; use crate::{docs, log}; @@ -253,17 +254,46 @@ impl ProtoLanguageServer { }; let content = self.state.get_content(&uri); - let current_package = tree.get_package_name(content.as_bytes()).unwrap_or("."); - let Some((edit, otext, ntext)) = tree.rename_tree(&pos, &new_name, content.as_bytes()) + // If the cursor is on a type reference (inside a message_or_enum_type + // node), pivot to the declaration and rename from there. The existing + // workspace pass then handles all references — including the one the + // user is standing on. + let (rename_uri, rename_pos) = match tree.rename_pivot_identifier(&pos, content.as_bytes()) + { + Some(decl_path) => { + let ipath = self.configs.get_include_paths(&uri).unwrap_or_default(); + let locations = + self.state + .definition(&ipath, current_package, Jumpable::Identifier(decl_path)); + let Some(decl) = locations.into_iter().next() else { + error!(uri=%uri, "failed to resolve declaration for reference-site rename"); + return Box::pin(async move { Ok(None) }); + }; + (decl.uri, decl.range.start) + } + None => (uri.clone(), pos), + }; + + let Some(rename_tree) = self.state.get_tree(&rename_uri) else { + error!(uri=%rename_uri, "failed to get tree at declaration"); + return Box::pin(async move { Ok(None) }); + }; + let rename_content = self.state.get_content(&rename_uri); + let rename_package = rename_tree + .get_package_name(rename_content.as_bytes()) + .unwrap_or("."); + + let Some((edit, otext, ntext)) = + rename_tree.rename_tree(&rename_pos, &new_name, rename_content.as_bytes()) else { - error!(uri=%uri, "failed to rename in a tree"); + error!(uri=%rename_uri, "failed to rename in a tree"); return Box::pin(async move { Ok(None) }); }; - let Some(workspace) = self.configs.get_workspace_for_uri(&uri) else { - error!(uri=%uri, "failed to get workspace"); + let Some(workspace) = self.configs.get_workspace_for_uri(&rename_uri) else { + error!(uri=%rename_uri, "failed to get workspace"); return Box::pin(async move { Ok(None) }); }; @@ -272,14 +302,16 @@ impl ProtoLanguageServer { let mut h = HashMap::new(); h.extend(self.state.rename_fields( - current_package, + rename_package, &otext, &ntext, workspace.to_file_path().unwrap(), progress_sender, )); - h.entry(tree.uri).or_insert(edit.clone()).extend(edit); + h.entry(rename_tree.uri.clone()) + .or_insert(edit.clone()) + .extend(edit); let response = Some(WorkspaceEdit { changes: Some(h), diff --git a/src/nodekind.rs b/src/nodekind.rs index 8909825..212c5ae 100644 --- a/src/nodekind.rs +++ b/src/nodekind.rs @@ -71,6 +71,7 @@ impl NodeKind { Self::is_userdefined(n) || n.kind() == Self::ServiceName.as_str() || n.kind() == Self::RpcName.as_str() + || n.kind() == Self::FieldName.as_str() } pub fn is_actionable(n: &Node) -> bool { diff --git a/src/parser/rename.rs b/src/parser/rename.rs index a096119..4e97f39 100644 --- a/src/parser/rename.rs +++ b/src/parser/rename.rs @@ -23,6 +23,42 @@ impl ParsedTree { }) } + /// When the cursor is on a type-reference identifier (inside a + /// `message_or_enum_type` node), return the partial qualified path up to + /// and including the segment under cursor. This is the identifier whose + /// declaration the rename should pivot to. + /// + /// For `Outer.Inner` with cursor on `Inner`, returns `Some("Outer.Inner")`. + /// For `Outer.Inner` with cursor on `Outer`, returns `Some("Outer")`. + /// For a non-reference position, returns `None`. + pub fn rename_pivot_identifier( + &self, + pos: &Position, + content: impl AsRef<[u8]>, + ) -> Option { + let n = self.get_node_at_position(pos)?; + if !NodeKind::is_identifier(&n) { + return None; + } + let parent = n.parent()?; + if !NodeKind::is_field_name(&parent) { + return None; + } + + let cursor_end = n.end_byte(); + let bytes = content.as_ref(); + let mut path = String::new(); + let mut cursor = parent.walk(); + for child in parent.children(&mut cursor) { + let text = child.utf8_text(bytes).ok()?; + path.push_str(text); + if child.end_byte() >= cursor_end { + break; + } + } + Some(path) + } + fn nodes_within<'a>( &self, n: Node<'a>, @@ -298,8 +334,8 @@ mod test { assert_yaml_snapshot!(tree.can_rename(&pos_service)); assert_yaml_snapshot!(tree.can_rename(&pos_rpc)); - // Type references inside an RPC declaration are not (yet) renameable - // from the reference site itself — that's a separate change. + // Type references inside an RPC declaration are renameable from the + // reference site; the LSP layer pivots to the declaration. assert_yaml_snapshot!(tree.can_rename(&pos_rpc_request_type)); } @@ -332,4 +368,65 @@ mod test { assert_yaml_snapshot!(rename_fn("Catalog", &pos_service)); assert_yaml_snapshot!(rename_fn("FetchBook", &pos_rpc)); } + + #[test] + fn test_rename_pivot_identifier() { + let uri: Url = "file://foo/bar/test.proto".parse().unwrap(); + let contents = include_str!("input/test_rename_service.proto"); + let parsed = ProtoParser::new().parse(uri, contents).unwrap(); + + // `Empty` reference at line 11 (the rpc Get(Empty) ...): char 16 = 'E' + let pos_unqualified_ref = Position { + line: 11, + character: 17, + }; + // `Book` return type at line 11 char 32..36 + let pos_other_ref = Position { + line: 11, + character: 33, + }; + // Service declaration site — not a reference, so no pivot needed + let pos_decl = Position { + line: 10, + character: 10, + }; + + assert_eq!( + parsed.rename_pivot_identifier(&pos_unqualified_ref, contents), + Some("Empty".to_owned()) + ); + assert_eq!( + parsed.rename_pivot_identifier(&pos_other_ref, contents), + Some("Book".to_owned()) + ); + assert_eq!(parsed.rename_pivot_identifier(&pos_decl, contents), None); + } + + #[test] + fn test_rename_pivot_identifier_qualified() { + // `Book.Author a = 1;` at line 19 of test_can_rename.proto + let uri: Url = "file://foo/bar/test.proto".parse().unwrap(); + let contents = include_str!("input/test_can_rename.proto"); + let parsed = ProtoParser::new().parse(uri, contents).unwrap(); + + // Cursor on `Book` (the outer segment): chars 4..8 + let pos_outer = Position { + line: 19, + character: 5, + }; + // Cursor on `Author` (the inner segment): chars 9..15 + let pos_inner = Position { + line: 19, + character: 11, + }; + + assert_eq!( + parsed.rename_pivot_identifier(&pos_outer, contents), + Some("Book".to_owned()) + ); + assert_eq!( + parsed.rename_pivot_identifier(&pos_inner, contents), + Some("Book.Author".to_owned()) + ); + } } diff --git a/src/parser/snapshots/protols__parser__rename__test__can_rename-3.snap b/src/parser/snapshots/protols__parser__rename__test__can_rename-3.snap index 9292d37..25a8302 100644 --- a/src/parser/snapshots/protols__parser__rename__test__can_rename-3.snap +++ b/src/parser/snapshots/protols__parser__rename__test__can_rename-3.snap @@ -2,4 +2,9 @@ source: src/parser/rename.rs expression: tree.can_rename(&pos_inner_type) --- -~ +start: + line: 19 + character: 9 +end: + line: 19 + character: 15 diff --git a/src/parser/snapshots/protols__parser__rename__test__can_rename-4.snap b/src/parser/snapshots/protols__parser__rename__test__can_rename-4.snap index 9545453..491df1e 100644 --- a/src/parser/snapshots/protols__parser__rename__test__can_rename-4.snap +++ b/src/parser/snapshots/protols__parser__rename__test__can_rename-4.snap @@ -2,4 +2,9 @@ source: src/parser/rename.rs expression: tree.can_rename(&pos_outer_type) --- -~ +start: + line: 19 + character: 4 +end: + line: 19 + character: 8 diff --git a/src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc-3.snap b/src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc-3.snap index ede6d8d..27f212b 100644 --- a/src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc-3.snap +++ b/src/parser/snapshots/protols__parser__rename__test__can_rename_service_and_rpc-3.snap @@ -2,4 +2,9 @@ source: src/parser/rename.rs expression: tree.can_rename(&pos_rpc_request_type) --- -~ +start: + line: 11 + character: 16 +end: + line: 11 + character: 21 From e59683b905ac69d770e17d4cf8d5b9da60cf1472 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20K=C3=A5gedal=20Reimer?= Date: Wed, 13 May 2026 20:53:57 +0200 Subject: [PATCH 03/10] docs: mention service/RPC and reference-site rename in README --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 0daf0a4..7bc031b 100644 --- a/README.md +++ b/README.md @@ -186,7 +186,7 @@ Hover over any symbol or imports to get detailed documentation and comments asso ### Rename Symbols -Rename symbols like messages or enums, and Propagate the changes throughout the codebase. Currently, field renaming within symbols is not supported. +Rename symbols like messages, enums, services and RPC methods, and propagate the changes throughout the codebase. Rename also works when invoked on a type reference (e.g. the request or response type of an `rpc`) — the LSP pivots to the declaration and applies the rename from there. Currently, field renaming within symbols is not supported. ### Find References From 31554619229a80b8d5bfdbfac4f57cab575458eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20K=C3=A5gedal=20Reimer?= Date: Wed, 13 May 2026 21:45:23 +0200 Subject: [PATCH 04/10] feat(rename): support renaming field names, oneofs, and enum values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend is_renameable to accept identifiers whose parent is a field-like declaration: regular fields, map fields, oneof fields, the oneof itself, and enum values. These are single-site renames — they aren't referenced as types from other .proto files — so they follow the empty-ancestor path in rename_tree. Refine the empty-ancestor handling to only propagate the identifier to the workspace pass for top-level enums (which can be referenced as types from other files). Services, RPCs, fields, oneofs, and enum values now produce a clean single-site edit with no risk of inadvertently renaming an unrelated type that happens to share the name. --- README.md | 2 +- src/nodekind.rs | 14 +++ src/parser/input/test_rename_field.proto | 22 ++++ src/parser/rename.rs | 109 ++++++++++++++++-- ...st__can_rename_field_and_enum_value-2.snap | 10 ++ ...st__can_rename_field_and_enum_value-3.snap | 5 + ...test__can_rename_field_and_enum_value.snap | 10 ++ ...__test__rename_field_and_enum_value-2.snap | 12 ++ ...__test__rename_field_and_enum_value-3.snap | 12 ++ ...__test__rename_field_and_enum_value-4.snap | 12 ++ ...__test__rename_field_and_enum_value-5.snap | 12 ++ ...__test__rename_field_and_enum_value-6.snap | 12 ++ ...me__test__rename_field_and_enum_value.snap | 12 ++ 13 files changed, 234 insertions(+), 10 deletions(-) create mode 100644 src/parser/input/test_rename_field.proto create mode 100644 src/parser/snapshots/protols__parser__rename__test__can_rename_field_and_enum_value-2.snap create mode 100644 src/parser/snapshots/protols__parser__rename__test__can_rename_field_and_enum_value-3.snap create mode 100644 src/parser/snapshots/protols__parser__rename__test__can_rename_field_and_enum_value.snap create mode 100644 src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-2.snap create mode 100644 src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-3.snap create mode 100644 src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-4.snap create mode 100644 src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-5.snap create mode 100644 src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-6.snap create mode 100644 src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value.snap diff --git a/README.md b/README.md index 7bc031b..0b2b766 100644 --- a/README.md +++ b/README.md @@ -186,7 +186,7 @@ Hover over any symbol or imports to get detailed documentation and comments asso ### Rename Symbols -Rename symbols like messages, enums, services and RPC methods, and propagate the changes throughout the codebase. Rename also works when invoked on a type reference (e.g. the request or response type of an `rpc`) — the LSP pivots to the declaration and applies the rename from there. Currently, field renaming within symbols is not supported. +Rename symbols like messages, enums, services and RPC methods, and propagate the changes throughout the codebase. Rename also works when invoked on a type reference (e.g. the request or response type of an `rpc`) — the LSP pivots to the declaration and applies the rename from there. Field names, oneof names, and enum values can also be renamed at their declaration site (single-site rename, since they aren't referenced as types from other `.proto` files). ### Find References diff --git a/src/nodekind.rs b/src/nodekind.rs index 212c5ae..6a5ccd9 100644 --- a/src/nodekind.rs +++ b/src/nodekind.rs @@ -72,6 +72,20 @@ impl NodeKind { || n.kind() == Self::ServiceName.as_str() || n.kind() == Self::RpcName.as_str() || n.kind() == Self::FieldName.as_str() + || Self::is_field_decl_parent(n) + } + + /// Kinds whose direct identifier child is the *name* of a field-like + /// declaration: regular fields, map fields, oneof fields, the oneof itself, + /// and enum values. For `string title = 1;`, the identifier `title` has + /// parent `field` — that's what we match here. The type identifier (e.g. + /// `Author` in `Author author = 2;`) is nested deeper under + /// `message_or_enum_type`, so it isn't caught by this predicate. + pub fn is_field_decl_parent(n: &Node) -> bool { + matches!( + n.kind(), + "field" | "map_field" | "oneof_field" | "oneof" | "enum_field" + ) } pub fn is_actionable(n: &Node) -> bool { diff --git a/src/parser/input/test_rename_field.proto b/src/parser/input/test_rename_field.proto new file mode 100644 index 0000000..8b124ec --- /dev/null +++ b/src/parser/input/test_rename_field.proto @@ -0,0 +1,22 @@ +syntax = "proto3"; + +package com.parser; + +enum Color { + RED = 0; + GREEN = 1; +} + +message Author { + string name = 1; +} + +message Book { + string title = 1; + Author author = 2; + map counts = 3; + oneof body { + string text = 4; + bytes blob = 5; + } +} diff --git a/src/parser/rename.rs b/src/parser/rename.rs index 4e97f39..22c4aa7 100644 --- a/src/parser/rename.rs +++ b/src/parser/rename.rs @@ -120,16 +120,24 @@ impl ParsedTree { let nodes = self.get_ancestor_nodes_at_position(pos); - // Renameable symbols that don't sit inside a message (services, RPCs, top-level - // enums) have no qualifying ancestors. Hand the unqualified name to the workspace - // pass so it can update any references; services and RPCs have none. + // Renameable symbols with no message ancestor: top-level enums, services, RPCs, + // and the various field-like declarations (regular fields, map fields, oneof, + // oneof fields, enum values). Only top-level enums are referenced as types + // from other files; the rest are single-site, so we hand the workspace pass + // a name it won't find — making it a harmless no-op without risking that a + // field named the same as some lowercase type triggers an unwanted rename. if nodes.is_empty() { - let otext = self - .get_node_at_position(pos)? - .utf8_text(content.as_ref()) - .ok()? - .to_owned(); - return Some((v, otext, new_name.to_owned())); + let n = self.get_node_at_position(pos)?; + let identifier = n.utf8_text(content.as_ref()).ok()?.to_owned(); + let is_type_symbol = n + .parent() + .is_some_and(|p| p.kind() == NodeKind::EnumName.as_str()); + let (otext, ntext) = if is_type_symbol { + (identifier, new_name.to_owned()) + } else { + (new_name.to_owned(), new_name.to_owned()) + }; + return Some((v, otext, ntext)); } let mut i = 1; @@ -402,6 +410,89 @@ mod test { assert_eq!(parsed.rename_pivot_identifier(&pos_decl, contents), None); } + #[test] + fn test_rename_field_and_enum_value() { + let uri: Url = "file://foo/bar.proto".parse().unwrap(); + let contents = include_str!("input/test_rename_field.proto"); + let parsed = ProtoParser::new().parse(uri, contents).unwrap(); + let tree = parsed; + + let rename_fn = |nt: &str, pos: &Position| match tree.rename_tree(pos, nt, contents) { + Some(k) => { + let mut v = tree.rename_field(&k.1, &k.2, contents); + v.extend(k.0); + v + } + _ => vec![], + }; + + // Enum value: RED at line 5 chars 4..7 + let pos_enum_value = Position { + line: 5, + character: 5, + }; + // Plain field: title at line 14 chars 11..16 + let pos_plain_field = Position { + line: 14, + character: 12, + }; + // User-type field: author at line 15 chars 11..17 + let pos_user_type_field = Position { + line: 15, + character: 12, + }; + // Map field: counts at line 16 chars 23..29 + let pos_map_field = Position { + line: 16, + character: 24, + }; + // Oneof name: body at line 17 chars 10..14 + let pos_oneof_name = Position { + line: 17, + character: 11, + }; + // Oneof field: text at line 18 chars 15..19 + let pos_oneof_field = Position { + line: 18, + character: 16, + }; + + assert_yaml_snapshot!(rename_fn("CRIMSON", &pos_enum_value)); + assert_yaml_snapshot!(rename_fn("name", &pos_plain_field)); + assert_yaml_snapshot!(rename_fn("writer", &pos_user_type_field)); + assert_yaml_snapshot!(rename_fn("tallies", &pos_map_field)); + assert_yaml_snapshot!(rename_fn("content", &pos_oneof_name)); + assert_yaml_snapshot!(rename_fn("words", &pos_oneof_field)); + } + + #[test] + fn test_can_rename_field_and_enum_value() { + let uri: Url = "file://foo/bar.proto".parse().unwrap(); + let contents = include_str!("input/test_rename_field.proto"); + let parsed = ProtoParser::new().parse(uri, contents).unwrap(); + + // Cursor on a type identifier inside a field (`Author` at line 15 + // chars 4..10) is a reference site — already supported. + let pos_type_ref = Position { + line: 15, + character: 6, + }; + // Cursor on the field name should now be renameable. + let pos_plain_field = Position { + line: 14, + character: 12, + }; + // Cursor on the int_lit `1` is not a renameable identifier. + let pos_field_number = Position { + line: 14, + character: 19, + }; + + assert_yaml_snapshot!(parsed.can_rename(&pos_type_ref)); + assert_yaml_snapshot!(parsed.can_rename(&pos_plain_field)); + assert_yaml_snapshot!(parsed.can_rename(&pos_field_number)); + } + #[test] fn test_rename_pivot_identifier_qualified() { // `Book.Author a = 1;` at line 19 of test_can_rename.proto diff --git a/src/parser/snapshots/protols__parser__rename__test__can_rename_field_and_enum_value-2.snap b/src/parser/snapshots/protols__parser__rename__test__can_rename_field_and_enum_value-2.snap new file mode 100644 index 0000000..d52c7fc --- /dev/null +++ b/src/parser/snapshots/protols__parser__rename__test__can_rename_field_and_enum_value-2.snap @@ -0,0 +1,10 @@ +--- +source: src/parser/rename.rs +expression: parsed.can_rename(&pos_plain_field) +--- +start: + line: 14 + character: 11 +end: + line: 14 + character: 16 diff --git a/src/parser/snapshots/protols__parser__rename__test__can_rename_field_and_enum_value-3.snap b/src/parser/snapshots/protols__parser__rename__test__can_rename_field_and_enum_value-3.snap new file mode 100644 index 0000000..4943cbd --- /dev/null +++ b/src/parser/snapshots/protols__parser__rename__test__can_rename_field_and_enum_value-3.snap @@ -0,0 +1,5 @@ +--- +source: src/parser/rename.rs +expression: parsed.can_rename(&pos_field_number) +--- +~ diff --git a/src/parser/snapshots/protols__parser__rename__test__can_rename_field_and_enum_value.snap b/src/parser/snapshots/protols__parser__rename__test__can_rename_field_and_enum_value.snap new file mode 100644 index 0000000..f5227ad --- /dev/null +++ b/src/parser/snapshots/protols__parser__rename__test__can_rename_field_and_enum_value.snap @@ -0,0 +1,10 @@ +--- +source: src/parser/rename.rs +expression: parsed.can_rename(&pos_type_ref) +--- +start: + line: 15 + character: 4 +end: + line: 15 + character: 10 diff --git a/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-2.snap b/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-2.snap new file mode 100644 index 0000000..19d2434 --- /dev/null +++ b/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-2.snap @@ -0,0 +1,12 @@ +--- +source: src/parser/rename.rs +expression: "rename_fn(\"name\", &pos_plain_field)" +--- +- range: + start: + line: 14 + character: 11 + end: + line: 14 + character: 16 + newText: name diff --git a/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-3.snap b/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-3.snap new file mode 100644 index 0000000..0154bca --- /dev/null +++ b/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-3.snap @@ -0,0 +1,12 @@ +--- +source: src/parser/rename.rs +expression: "rename_fn(\"writer\", &pos_user_type_field)" +--- +- range: + start: + line: 15 + character: 11 + end: + line: 15 + character: 17 + newText: writer diff --git a/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-4.snap b/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-4.snap new file mode 100644 index 0000000..2b01e20 --- /dev/null +++ b/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-4.snap @@ -0,0 +1,12 @@ +--- +source: src/parser/rename.rs +expression: "rename_fn(\"tallies\", &pos_map_field)" +--- +- range: + start: + line: 16 + character: 23 + end: + line: 16 + character: 29 + newText: tallies diff --git a/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-5.snap b/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-5.snap new file mode 100644 index 0000000..47afe03 --- /dev/null +++ b/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-5.snap @@ -0,0 +1,12 @@ +--- +source: src/parser/rename.rs +expression: "rename_fn(\"content\", &pos_oneof_name)" +--- +- range: + start: + line: 17 + character: 10 + end: + line: 17 + character: 14 + newText: content diff --git a/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-6.snap b/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-6.snap new file mode 100644 index 0000000..e36760e --- /dev/null +++ b/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value-6.snap @@ -0,0 +1,12 @@ +--- +source: src/parser/rename.rs +expression: "rename_fn(\"words\", &pos_oneof_field)" +--- +- range: + start: + line: 18 + character: 15 + end: + line: 18 + character: 19 + newText: words diff --git a/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value.snap b/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value.snap new file mode 100644 index 0000000..faaf177 --- /dev/null +++ b/src/parser/snapshots/protols__parser__rename__test__rename_field_and_enum_value.snap @@ -0,0 +1,12 @@ +--- +source: src/parser/rename.rs +expression: "rename_fn(\"CRIMSON\", &pos_enum_value)" +--- +- range: + start: + line: 5 + character: 4 + end: + line: 5 + character: 7 + newText: CRIMSON From 3b9f699a9f90e0051def9c2ab490d328946ce26f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20K=C3=A5gedal=20Reimer?= Date: Wed, 13 May 2026 22:01:29 +0200 Subject: [PATCH 05/10] feat(rename): chain rpc, request, and response renames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an rpc follows the convention rpc (Request) returns (Response); (enforced by buf lint's RPC_REQUEST_STANDARD_NAME / RPC_RESPONSE_STANDARD_NAME), renaming any one of the trio now triggers a chained rename of the other two. The chain only kicks in when: - the matching request/response message is named exactly per the convention (trailing segment of its qualified text), - it is used by exactly one rpc in the workspace, and - the user's new name preserves the convention (i.e. when renaming a request, the new name still ends with `Request`). If any check fails, only the symbol the user invoked rename on is renamed — the primary rename is never blocked by chain-detection failures. New parser-level helpers `rpc_at_position`, `message_name_at_position`, and `all_rpc_signatures` expose the information the orchestrator needs; new workspace-level helpers `find_rpc_decl` and `count_rpc_uses_of_type` handle the cross-file lookups. The orchestration lives in `lsp.rs::rename`, which now factors out `run_single_rename` so multiple rename ops can share the existing per-file + workspace machinery and merge their edits. --- README.md | 2 + src/lsp.rs | 259 +++++++++++++++++++++++++---- src/parser/rename.rs | 125 ++++++++++++++ src/workspace/input/messages.proto | 12 ++ src/workspace/input/service.proto | 12 ++ src/workspace/rename.rs | 102 +++++++++++- 6 files changed, 479 insertions(+), 33 deletions(-) create mode 100644 src/workspace/input/messages.proto create mode 100644 src/workspace/input/service.proto diff --git a/README.md b/README.md index 0b2b766..4cb1d5f 100644 --- a/README.md +++ b/README.md @@ -188,6 +188,8 @@ Hover over any symbol or imports to get detailed documentation and comments asso Rename symbols like messages, enums, services and RPC methods, and propagate the changes throughout the codebase. Rename also works when invoked on a type reference (e.g. the request or response type of an `rpc`) — the LSP pivots to the declaration and applies the rename from there. Field names, oneof names, and enum values can also be renamed at their declaration site (single-site rename, since they aren't referenced as types from other `.proto` files). +When an `rpc` follows the standard `rpc (Request) returns (Response)` convention (enforced by `buf lint`'s `RPC_REQUEST_STANDARD_NAME` and `RPC_RESPONSE_STANDARD_NAME` rules), renaming any one of the three triggers a chained rename of the other two — but only when (a) the matching message name follows the convention exactly, (b) the request/response is used by exactly one rpc in the workspace, and (c) the user's new name preserves the convention. If any check fails, only the symbol the user invoked rename on is renamed. + ### Find References Find all references to user-defined types like messages or enums. Nested fields are fully supported, making it easier to track symbol usage across your project. diff --git a/src/lsp.rs b/src/lsp.rs index f042fe9..af64a50 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -260,8 +260,7 @@ impl ProtoLanguageServer { // node), pivot to the declaration and rename from there. The existing // workspace pass then handles all references — including the one the // user is standing on. - let (rename_uri, rename_pos) = match tree.rename_pivot_identifier(&pos, content.as_bytes()) - { + let (decl_uri, decl_pos) = match tree.rename_pivot_identifier(&pos, content.as_bytes()) { Some(decl_path) => { let ipath = self.configs.get_include_paths(&uri).unwrap_or_default(); let locations = @@ -276,51 +275,249 @@ impl ProtoLanguageServer { None => (uri.clone(), pos), }; - let Some(rename_tree) = self.state.get_tree(&rename_uri) else { - error!(uri=%rename_uri, "failed to get tree at declaration"); - return Box::pin(async move { Ok(None) }); + // Always apply the user's primary rename. If the declaration is part + // of an `rpc (Request) returns (Response)` trio and + // the user's new name preserves the convention, also rename the + // sibling(s). + let primary = RenameOp { + uri: decl_uri.clone(), + pos: decl_pos, + new_name: new_name.clone(), }; - let rename_content = self.state.get_content(&rename_uri); - let rename_package = rename_tree - .get_package_name(rename_content.as_bytes()) - .unwrap_or("."); + let mut ops = vec![primary]; + ops.extend(self.compute_rpc_chain_siblings(&decl_uri, decl_pos, &new_name)); - let Some((edit, otext, ntext)) = - rename_tree.rename_tree(&rename_pos, &new_name, rename_content.as_bytes()) - else { - error!(uri=%rename_uri, "failed to rename in a tree"); - return Box::pin(async move { Ok(None) }); - }; + let work_done_token = params.work_done_progress_params.work_done_token; + let mut progress_sender = work_done_token.map(|token| self.with_report_progress(token)); + + let mut all_edits: HashMap> = HashMap::new(); + for op in ops { + // Only send progress for the first op; subsequent ops would + // double-report. + let sender = progress_sender.take(); + let Some(op_edits) = self.run_single_rename(&op, sender) else { + continue; + }; + for (u, edits) in op_edits { + all_edits.entry(u).or_default().extend(edits); + } + } - let Some(workspace) = self.configs.get_workspace_for_uri(&rename_uri) else { - error!(uri=%rename_uri, "failed to get workspace"); - return Box::pin(async move { Ok(None) }); + let response = if all_edits.is_empty() { + None + } else { + Some(WorkspaceEdit { + changes: Some(all_edits), + ..Default::default() + }) }; - let work_done_token = params.work_done_progress_params.work_done_token; - let progress_sender = work_done_token.map(|token| self.with_report_progress(token)); + Box::pin(async move { Ok(response) }) + } + + fn run_single_rename( + &mut self, + op: &RenameOp, + progress_sender: Option>, + ) -> Option>> { + let tree = self.state.get_tree(&op.uri)?; + let content = self.state.get_content(&op.uri); + let package = tree.get_package_name(content.as_bytes()).unwrap_or("."); + + let (edit, otext, ntext) = tree.rename_tree(&op.pos, &op.new_name, content.as_bytes())?; + let workspace = self.configs.get_workspace_for_uri(&op.uri)?; - let mut h = HashMap::new(); + let mut h: HashMap> = HashMap::new(); h.extend(self.state.rename_fields( - rename_package, + package, &otext, &ntext, - workspace.to_file_path().unwrap(), + workspace.to_file_path().ok()?, progress_sender, )); + h.entry(tree.uri.clone()).or_default().extend(edit); + Some(h) + } - h.entry(rename_tree.uri.clone()) - .or_insert(edit.clone()) - .extend(edit); + /// Compute the additional rpc/request/response sibling renames triggered + /// by the user's primary rename at `(decl_uri, decl_pos) → new_name`. + /// Returns an empty vec when no chain applies. The user's primary rename + /// is *not* included in the returned ops — the caller always applies it + /// separately, so a primary on a convention-named message still happens + /// even if uniqueness checks block the siblings. + fn compute_rpc_chain_siblings( + &mut self, + decl_uri: &Url, + decl_pos: async_lsp::lsp_types::Position, + new_name: &str, + ) -> Vec { + if new_name.is_empty() { + return vec![]; + } + let Some(tree) = self.state.get_tree(decl_uri) else { + return vec![]; + }; + let content = self.state.get_content(decl_uri); + let bytes = content.as_bytes(); - let response = Some(WorkspaceEdit { - changes: Some(h), - ..Default::default() - }); + // Case A: user invoked rename on an rpc_name. The primary is the rpc + // rename; siblings are the convention-matching request/response. + if let Some((old_rpc_name, request_text, response_text)) = + tree.rpc_at_position(&decl_pos, bytes) + { + return self.sibling_message_ops( + decl_uri, + &old_rpc_name, + new_name, + &[ + ("Request", request_text.as_str()), + ("Response", response_text.as_str()), + ], + ); + } - Box::pin(async move { Ok(response) }) + // Case B: user invoked rename on a message_name matching the + // convention. The primary is the message rename; we additionally + // rename the rpc and the opposite sibling (if it follows convention + // and the new name preserves the convention). + let Some(msg_name) = tree.message_name_at_position(&decl_pos, bytes) else { + return vec![]; + }; + let (rpc_base, primary_suffix, new_rpc_base) = if let (Some(rpc), Some(new_rpc)) = ( + msg_name.strip_suffix("Request"), + new_name.strip_suffix("Request"), + ) { + (rpc.to_owned(), "Request", new_rpc.to_owned()) + } else if let (Some(rpc), Some(new_rpc)) = ( + msg_name.strip_suffix("Response"), + new_name.strip_suffix("Response"), + ) { + (rpc.to_owned(), "Response", new_rpc.to_owned()) + } else { + return vec![]; + }; + if rpc_base.is_empty() || new_rpc_base.is_empty() { + return vec![]; + } + + // Locate the rpc and confirm it matches both the rpc base name and + // uses this message in the expected slot. + let Some(rpc_loc) = self.state.find_rpc_decl(&rpc_base) else { + return vec![]; + }; + let Some(rpc_tree) = self.state.get_tree(&rpc_loc.uri) else { + return vec![]; + }; + let rpc_content = self.state.get_content(&rpc_loc.uri); + let Some((_, rpc_req, rpc_resp)) = + rpc_tree.rpc_at_position(&rpc_loc.range.start, rpc_content.as_bytes()) + else { + return vec![]; + }; + let expected_primary = format!("{rpc_base}{primary_suffix}"); + let primary_slot_matches = if primary_suffix == "Request" { + trailing_segment(&rpc_req) == expected_primary + } else { + trailing_segment(&rpc_resp) == expected_primary + }; + if !primary_slot_matches { + return vec![]; + } + + // Uniqueness: only chain the rpc + opposite sibling if the user's + // primary message is referenced by exactly this one rpc. Otherwise a + // chained rename would silently break another rpc that shares the + // type. + if self.state.count_rpc_uses_of_type(&expected_primary) != 1 { + return vec![]; + } + + let mut ops = vec![RenameOp { + uri: rpc_loc.uri.clone(), + pos: rpc_loc.range.start, + new_name: new_rpc_base.clone(), + }]; + + let opposite_suffix = if primary_suffix == "Request" { + "Response" + } else { + "Request" + }; + let opposite_text = if opposite_suffix == "Request" { + rpc_req.as_str() + } else { + rpc_resp.as_str() + }; + ops.extend(self.sibling_message_ops( + &rpc_loc.uri, + &rpc_base, + &new_rpc_base, + &[(opposite_suffix, opposite_text)], + )); + ops } + /// Resolve each `(suffix, type_text)` slot to a message declaration and + /// build the corresponding rename op, gated on convention + uniqueness. + fn sibling_message_ops( + &mut self, + anchor_uri: &Url, + old_rpc_name: &str, + new_rpc_name: &str, + slots: &[(&str, &str)], + ) -> Vec { + let Some(anchor_tree) = self.state.get_tree(anchor_uri) else { + return vec![]; + }; + let anchor_content = self.state.get_content(anchor_uri); + let anchor_package = anchor_tree + .get_package_name(anchor_content.as_bytes()) + .unwrap_or(".") + .to_owned(); + let ipath = self + .configs + .get_include_paths(anchor_uri) + .unwrap_or_default(); + + let mut ops = vec![]; + for (suffix, type_text) in slots { + let expected_name = format!("{old_rpc_name}{suffix}"); + if trailing_segment(type_text) != expected_name { + continue; + } + if self.state.count_rpc_uses_of_type(&expected_name) != 1 { + continue; + } + let locations = self.state.definition( + &ipath, + &anchor_package, + Jumpable::Identifier((*type_text).to_owned()), + ); + let Some(decl) = locations.into_iter().next() else { + continue; + }; + ops.push(RenameOp { + uri: decl.uri, + pos: decl.range.start, + new_name: format!("{new_rpc_name}{suffix}"), + }); + } + ops + } +} + +#[derive(Clone, Debug)] +struct RenameOp { + uri: Url, + pos: async_lsp::lsp_types::Position, + new_name: String, +} + +fn trailing_segment(qualified: &str) -> &str { + qualified.rsplit('.').next().unwrap_or(qualified) +} + +impl ProtoLanguageServer { pub(super) fn references( &mut self, param: ReferenceParams, diff --git a/src/parser/rename.rs b/src/parser/rename.rs index 22c4aa7..40915c3 100644 --- a/src/parser/rename.rs +++ b/src/parser/rename.rs @@ -59,6 +59,73 @@ impl ParsedTree { Some(path) } + /// If the given position is on the rpc name of an rpc declaration, returns + /// the rpc's name along with its declared request and response type texts. + /// Used to drive the rpc/request/response chained rename. + pub fn rpc_at_position( + &self, + pos: &Position, + content: impl AsRef<[u8]>, + ) -> Option<(String, String, String)> { + let n = self.get_node_at_position(pos)?; + if !NodeKind::is_identifier(&n) { + return None; + } + let parent = n.parent()?; + if parent.kind() != NodeKind::RpcName.as_str() { + return None; + } + let rpc = parent.parent()?; + let bytes = content.as_ref(); + let mut cursor = rpc.walk(); + let mut types = rpc + .children(&mut cursor) + .filter(|c| c.kind() == NodeKind::FieldName.as_str()); + let request = types.next()?.utf8_text(bytes).ok()?.to_owned(); + let response = types.next()?.utf8_text(bytes).ok()?.to_owned(); + let rpc_name = n.utf8_text(bytes).ok()?.to_owned(); + Some((rpc_name, request, response)) + } + + /// If the given position is on a message name, returns that name. + pub fn message_name_at_position( + &self, + pos: &Position, + content: impl AsRef<[u8]>, + ) -> Option { + let n = self.get_node_at_position(pos)?; + if !NodeKind::is_identifier(&n) { + return None; + } + let parent = n.parent()?; + if parent.kind() != NodeKind::MessageName.as_str() { + return None; + } + Some(n.utf8_text(content.as_ref()).ok()?.to_owned()) + } + + /// Returns the (request, response) type texts for every `rpc` node in this + /// tree. Used to verify that a request/response type is uniquely used by a + /// single rpc before chain-renaming it. + pub fn all_rpc_signatures(&self, content: impl AsRef<[u8]>) -> Vec<(String, String)> { + let bytes = content.as_ref(); + let mut out = vec![]; + for rpc in self.find_all_nodes(|n: &Node| n.kind() == "rpc") { + let mut cursor = rpc.walk(); + let mut types = rpc + .children(&mut cursor) + .filter(|c| c.kind() == NodeKind::FieldName.as_str()); + let Some(req) = types.next().and_then(|c| c.utf8_text(bytes).ok()) else { + continue; + }; + let Some(resp) = types.next().and_then(|c| c.utf8_text(bytes).ok()) else { + continue; + }; + out.push((req.to_owned(), resp.to_owned())); + } + out + } + fn nodes_within<'a>( &self, n: Node<'a>, @@ -410,6 +477,64 @@ mod test { assert_eq!(parsed.rename_pivot_identifier(&pos_decl, contents), None); } + #[test] + fn test_rpc_at_position_and_signatures() { + let uri: Url = "file://foo/bar.proto".parse().unwrap(); + let contents = include_str!("input/test_rename_service.proto"); + let parsed = ProtoParser::new().parse(uri, contents).unwrap(); + + // `GetBook` rpc at line 11 chars 8..15 + let pos_rpc = Position { + line: 11, + character: 10, + }; + assert_eq!( + parsed.rpc_at_position(&pos_rpc, contents), + Some(("GetBook".to_owned(), "Empty".to_owned(), "Book".to_owned(),)), + ); + + // Cursor on a non-rpc identifier should return None. + let pos_service = Position { + line: 10, + character: 10, + }; + assert_eq!(parsed.rpc_at_position(&pos_service, contents), None); + + // all_rpc_signatures should pick up both rpcs in the file. + let sigs = parsed.all_rpc_signatures(contents); + assert_eq!( + sigs, + vec![ + ("Empty".to_owned(), "Book".to_owned()), + ("Empty".to_owned(), "Book".to_owned()), + ] + ); + } + + #[test] + fn test_message_name_at_position() { + let uri: Url = "file://foo/bar.proto".parse().unwrap(); + let contents = include_str!("input/test_rename_service.proto"); + let parsed = ProtoParser::new().parse(uri, contents).unwrap(); + + // `Book` declaration at line 6 chars 8..12 + let pos = Position { + line: 6, + character: 9, + }; + assert_eq!( + parsed.message_name_at_position(&pos, contents), + Some("Book".to_owned()) + ); + + // RPC name shouldn't match. + let pos_rpc = Position { + line: 11, + character: 10, + }; + assert_eq!(parsed.message_name_at_position(&pos_rpc, contents), None); + } + #[test] fn test_rename_field_and_enum_value() { let uri: Url = "file://foo/bar.proto".parse().unwrap(); diff --git a/src/workspace/input/messages.proto b/src/workspace/input/messages.proto new file mode 100644 index 0000000..1950205 --- /dev/null +++ b/src/workspace/input/messages.proto @@ -0,0 +1,12 @@ +syntax = "proto3"; + +package com.workspace; + +message GetBookRequest {} +message GetBookResponse { + string title = 1; +} +message FrobRequest {} +message Bar {} +message SharedReq {} +message SharedResp {} diff --git a/src/workspace/input/service.proto b/src/workspace/input/service.proto new file mode 100644 index 0000000..2a62b98 --- /dev/null +++ b/src/workspace/input/service.proto @@ -0,0 +1,12 @@ +syntax = "proto3"; + +package com.workspace; + +import "messages.proto"; + +service Library { + rpc GetBook(GetBookRequest) returns (GetBookResponse); + rpc Frob(FrobRequest) returns (Bar); + rpc ListA(SharedReq) returns (SharedResp); + rpc ListB(SharedReq) returns (SharedResp); +} diff --git a/src/workspace/rename.rs b/src/workspace/rename.rs index 88a1d89..27914f0 100644 --- a/src/workspace/rename.rs +++ b/src/workspace/rename.rs @@ -1,8 +1,9 @@ -use crate::utils::split_identifier_package; +use crate::nodekind::NodeKind; +use crate::utils::{split_identifier_package, ts_to_lsp_position}; use std::collections::HashMap; use std::path::PathBuf; -use async_lsp::lsp_types::{Location, TextEdit, Url}; +use async_lsp::lsp_types::{Location, Range, TextEdit, Url}; use crate::state::ProtoLanguageState; use async_lsp::lsp_types::ProgressParamsValue; @@ -128,6 +129,58 @@ impl ProtoLanguageState { }); if r.is_empty() { None } else { Some(r) } } + + /// Find the declaration of an rpc by simple name across the workspace. + /// Returns the first match. Used by the rpc/request/response chain rename + /// to anchor the chain when the user invokes rename on a matching message. + pub fn find_rpc_decl(&self, rpc_name: &str) -> Option { + for tree in self.get_trees() { + let content = self.get_content(&tree.uri); + for node in tree.find_all_nodes(NodeKind::is_identifier) { + let Some(parent) = node.parent() else { + continue; + }; + if parent.kind() != NodeKind::RpcName.as_str() { + continue; + } + let Ok(text) = node.utf8_text(content.as_bytes()) else { + continue; + }; + if text == rpc_name { + return Some(Location { + uri: tree.uri.clone(), + range: Range { + start: ts_to_lsp_position(&node.start_position()), + end: ts_to_lsp_position(&node.end_position()), + }, + }); + } + } + } + None + } + + /// Count the number of rpcs in the workspace whose request or response + /// type's trailing identifier segment matches `type_simple_name`. Used for + /// the uniqueness check before chain-renaming a request/response message. + pub fn count_rpc_uses_of_type(&self, type_simple_name: &str) -> usize { + let mut count = 0; + for tree in self.get_trees() { + let content = self.get_content(&tree.uri); + for (req, resp) in tree.all_rpc_signatures(content.as_bytes()) { + if trailing_segment(&req) == type_simple_name + || trailing_segment(&resp) == type_simple_name + { + count += 1; + } + } + } + count + } +} + +fn trailing_segment(qualified: &str) -> &str { + qualified.rsplit('.').next().unwrap_or(qualified) } #[cfg(test)] @@ -207,4 +260,49 @@ mod test { None )); } + + #[test] + fn test_find_rpc_decl_and_count_uses() { + let ipath = vec![PathBuf::from("src/workspace/input")]; + let svc_uri = "file://input/service.proto".parse().unwrap(); + let msg_uri = "file://input/messages.proto".parse().unwrap(); + let svc = include_str!("input/service.proto"); + let msg = include_str!("input/messages.proto"); + + let mut state: ProtoLanguageState = ProtoLanguageState::new(); + state.upsert_file( + &svc_uri, + svc.to_owned(), + &ipath, + 2, + &Config::default(), + false, + ); + state.upsert_file( + &msg_uri, + msg.to_owned(), + &ipath, + 2, + &Config::default(), + false, + ); + + // Lookup hits the rpc_name node in service.proto. + let loc = state.find_rpc_decl("GetBook").expect("rpc not found"); + assert!(loc.uri.as_str().ends_with("service.proto")); + assert_eq!(loc.range.start.line, 7); + + assert!(state.find_rpc_decl("DoesNotExist").is_none()); + + // Convention-following types are uniquely used. + assert_eq!(state.count_rpc_uses_of_type("GetBookRequest"), 1); + assert_eq!(state.count_rpc_uses_of_type("GetBookResponse"), 1); + // Shared types are seen twice. + assert_eq!(state.count_rpc_uses_of_type("SharedReq"), 2); + assert_eq!(state.count_rpc_uses_of_type("SharedResp"), 2); + // Non-convention type still uniquely used as a response. + assert_eq!(state.count_rpc_uses_of_type("Bar"), 1); + // Unrelated names see no uses. + assert_eq!(state.count_rpc_uses_of_type("Unrelated"), 0); + } } From c4fbf5781ff0db464e81b74cce9f6e9b38dabd4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20K=C3=A5gedal=20Reimer?= Date: Wed, 13 May 2026 22:02:15 +0200 Subject: [PATCH 06/10] docs: cite Google API design guide rather than buf lint for the rpc convention --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 4cb1d5f..8945922 100644 --- a/README.md +++ b/README.md @@ -188,7 +188,7 @@ Hover over any symbol or imports to get detailed documentation and comments asso Rename symbols like messages, enums, services and RPC methods, and propagate the changes throughout the codebase. Rename also works when invoked on a type reference (e.g. the request or response type of an `rpc`) — the LSP pivots to the declaration and applies the rename from there. Field names, oneof names, and enum values can also be renamed at their declaration site (single-site rename, since they aren't referenced as types from other `.proto` files). -When an `rpc` follows the standard `rpc (Request) returns (Response)` convention (enforced by `buf lint`'s `RPC_REQUEST_STANDARD_NAME` and `RPC_RESPONSE_STANDARD_NAME` rules), renaming any one of the three triggers a chained rename of the other two — but only when (a) the matching message name follows the convention exactly, (b) the request/response is used by exactly one rpc in the workspace, and (c) the user's new name preserves the convention. If any check fails, only the symbol the user invoked rename on is renamed. +When an `rpc` follows the `rpc (Request) returns (Response)` convention from the [Google API design guide](https://google.aip.dev/) (AIPs 131–136), renaming any one of the three triggers a chained rename of the other two — but only when (a) the matching message name follows the convention exactly, (b) the request/response is used by exactly one rpc in the workspace, and (c) the user's new name preserves the convention. If any check fails, only the symbol the user invoked rename on is renamed. ### Find References From 4d49372990933c57e95f3f6cc5e4ae1002b092ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20K=C3=A5gedal=20Reimer?= Date: Thu, 14 May 2026 08:39:19 +0200 Subject: [PATCH 07/10] refactor(rename): polish review feedback - Move trailing_segment into utils.rs with rsplit_once-style impl to dedupe the helper from lsp.rs and workspace/rename.rs. - Add NodeKind::is_rpc_name and use it in find_rpc_decl to walk rpc_name nodes directly instead of every identifier in every file. - Document rename_pivot_identifier's dependency on the tree-sitter-proto grammar emitting dot separators as anonymous children. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lsp.rs | 5 +---- src/nodekind.rs | 4 ++++ src/parser/rename.rs | 10 ++++++++++ src/utils.rs | 17 ++++++++++++++++- src/workspace/rename.rs | 14 ++------------ 5 files changed, 33 insertions(+), 17 deletions(-) diff --git a/src/lsp.rs b/src/lsp.rs index af64a50..8744e5c 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -23,6 +23,7 @@ use serde_json::Value; use crate::context::jumpable::Jumpable; use crate::formatter::ProtoFormatter; use crate::server::ProtoLanguageServer; +use crate::utils::trailing_segment; use crate::{docs, log}; impl ProtoLanguageServer { @@ -513,10 +514,6 @@ struct RenameOp { new_name: String, } -fn trailing_segment(qualified: &str) -> &str { - qualified.rsplit('.').next().unwrap_or(qualified) -} - impl ProtoLanguageServer { pub(super) fn references( &mut self, diff --git a/src/nodekind.rs b/src/nodekind.rs index 6a5ccd9..4575e6a 100644 --- a/src/nodekind.rs +++ b/src/nodekind.rs @@ -63,6 +63,10 @@ impl NodeKind { n.kind() == Self::FieldName.as_str() } + pub fn is_rpc_name(n: &Node) -> bool { + n.kind() == Self::RpcName.as_str() + } + pub fn is_userdefined(n: &Node) -> bool { n.kind() == Self::EnumName.as_str() || n.kind() == Self::MessageName.as_str() } diff --git a/src/parser/rename.rs b/src/parser/rename.rs index 40915c3..67454c2 100644 --- a/src/parser/rename.rs +++ b/src/parser/rename.rs @@ -31,6 +31,16 @@ impl ParsedTree { /// For `Outer.Inner` with cursor on `Inner`, returns `Some("Outer.Inner")`. /// For `Outer.Inner` with cursor on `Outer`, returns `Some("Outer")`. /// For a non-reference position, returns `None`. + /// + /// The reconstruction walks every child of `message_or_enum_type` (named + /// *and* anonymous) and concatenates their `utf8_text`. This depends on + /// tree-sitter-proto emitting the `.` separators as anonymous children + /// whose `utf8_text` is literally `"."`, and on the grammar leaving no + /// whitespace between identifier tokens. Both invariants are exercised by + /// `test_rename_pivot_identifier_qualified`, which asserts the exact + /// `"Book.Author"` reconstruction — a grammar change that violated either + /// invariant would fail that test rather than silently produce e.g. + /// `"BookAuthor"`. pub fn rename_pivot_identifier( &self, pos: &Position, diff --git a/src/utils.rs b/src/utils.rs index d7bf3d8..2ab843a 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -36,6 +36,12 @@ pub fn is_inner_identifier(s: &str) -> bool { s.split('.').all(is_title_case) } +/// Returns the segment after the last `.` in a dotted identifier, or the whole +/// string if it contains no dot. +pub fn trailing_segment(qualified: &str) -> &str { + qualified.rsplit_once('.').map_or(qualified, |(_, t)| t) +} + pub fn split_identifier_package(s: &str) -> (&str, &str) { let s = s.trim_start_matches("."); if is_inner_identifier(s) || !s.contains('.') { @@ -59,7 +65,16 @@ pub fn split_identifier_package(s: &str) -> (&str, &str) { #[cfg(test)] mod test { - use crate::utils::{is_inner_identifier, split_identifier_package}; + use crate::utils::{is_inner_identifier, split_identifier_package, trailing_segment}; + + #[test] + fn test_trailing_segment() { + assert_eq!(trailing_segment("Foo"), "Foo"); + assert_eq!(trailing_segment("foo.Bar"), "Bar"); + assert_eq!(trailing_segment("foo.bar.Baz"), "Baz"); + assert_eq!(trailing_segment(".foo.Bar"), "Bar"); + assert_eq!(trailing_segment(""), ""); + } #[test] fn test_is_inner_identifier() { diff --git a/src/workspace/rename.rs b/src/workspace/rename.rs index 27914f0..0f4e8bf 100644 --- a/src/workspace/rename.rs +++ b/src/workspace/rename.rs @@ -1,5 +1,5 @@ use crate::nodekind::NodeKind; -use crate::utils::{split_identifier_package, ts_to_lsp_position}; +use crate::utils::{split_identifier_package, trailing_segment, ts_to_lsp_position}; use std::collections::HashMap; use std::path::PathBuf; @@ -136,13 +136,7 @@ impl ProtoLanguageState { pub fn find_rpc_decl(&self, rpc_name: &str) -> Option { for tree in self.get_trees() { let content = self.get_content(&tree.uri); - for node in tree.find_all_nodes(NodeKind::is_identifier) { - let Some(parent) = node.parent() else { - continue; - }; - if parent.kind() != NodeKind::RpcName.as_str() { - continue; - } + for node in tree.find_all_nodes(NodeKind::is_rpc_name) { let Ok(text) = node.utf8_text(content.as_bytes()) else { continue; }; @@ -179,10 +173,6 @@ impl ProtoLanguageState { } } -fn trailing_segment(qualified: &str) -> &str { - qualified.rsplit('.').next().unwrap_or(qualified) -} - #[cfg(test)] mod test { use std::path::PathBuf; From 456db172317e45840edb6dace75db0b356aaf0a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20K=C3=A5gedal=20Reimer?= Date: Thu, 14 May 2026 08:43:55 +0200 Subject: [PATCH 08/10] refactor(rename): move chain orchestration onto ProtoLanguageState Move the rpc/request/response chain detection out of the LSP handler and onto ProtoLanguageState as `compute_rename_ops` + `apply_rename_ops`, with RenameOp as a public struct and per-case helpers (`chain_from_rpc_cursor`, `chain_from_message_cursor`, `sibling_message_ops`, `strip_convention_suffix`). The LSP layer now just pivots from reference to declaration and delegates the rest. Restores logging on workspace lookup and primary-rename failures. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lsp.rs | 258 ++++--------------------------------- src/workspace/rename.rs | 279 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 301 insertions(+), 236 deletions(-) diff --git a/src/lsp.rs b/src/lsp.rs index 8744e5c..05e3106 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -1,5 +1,5 @@ use std::ops::ControlFlow; -use std::{collections::HashMap, fs::read_to_string, path::PathBuf}; +use std::{fs::read_to_string, path::PathBuf}; use tracing::{error, info, warn}; use async_lsp::lsp_types::{ @@ -23,7 +23,6 @@ use serde_json::Value; use crate::context::jumpable::Jumpable; use crate::formatter::ProtoFormatter; use crate::server::ProtoLanguageServer; -use crate::utils::trailing_segment; use crate::{docs, log}; impl ProtoLanguageServer { @@ -246,7 +245,6 @@ impl ProtoLanguageServer { ) -> BoxFuture<'static, Result, ResponseError>> { let uri = params.text_document_position.text_document.uri; let pos = params.text_document_position.position; - let new_name = params.new_name; let Some(tree) = self.state.get_tree(&uri) else { @@ -256,14 +254,14 @@ impl ProtoLanguageServer { let content = self.state.get_content(&uri); let current_package = tree.get_package_name(content.as_bytes()).unwrap_or("."); + let ipath = self.configs.get_include_paths(&uri).unwrap_or_default(); // If the cursor is on a type reference (inside a message_or_enum_type - // node), pivot to the declaration and rename from there. The existing - // workspace pass then handles all references — including the one the - // user is standing on. + // node), pivot to the declaration and rename from there. The workspace + // pass then handles all references — including the one the user is + // standing on. let (decl_uri, decl_pos) = match tree.rename_pivot_identifier(&pos, content.as_bytes()) { Some(decl_path) => { - let ipath = self.configs.get_include_paths(&uri).unwrap_or_default(); let locations = self.state .definition(&ipath, current_package, Jumpable::Identifier(decl_path)); @@ -276,33 +274,30 @@ impl ProtoLanguageServer { None => (uri.clone(), pos), }; - // Always apply the user's primary rename. If the declaration is part - // of an `rpc (Request) returns (Response)` trio and - // the user's new name preserves the convention, also rename the - // sibling(s). - let primary = RenameOp { - uri: decl_uri.clone(), - pos: decl_pos, - new_name: new_name.clone(), + let Some(workspace) = self.configs.get_workspace_for_uri(&decl_uri) else { + error!(uri=%decl_uri, "failed to get workspace"); + return Box::pin(async move { Ok(None) }); + }; + let Ok(workspace_path) = workspace.to_file_path() else { + error!(uri=%workspace, "workspace url is not a file path"); + return Box::pin(async move { Ok(None) }); }; - let mut ops = vec![primary]; - ops.extend(self.compute_rpc_chain_siblings(&decl_uri, decl_pos, &new_name)); - let work_done_token = params.work_done_progress_params.work_done_token; - let mut progress_sender = work_done_token.map(|token| self.with_report_progress(token)); - - let mut all_edits: HashMap> = HashMap::new(); - for op in ops { - // Only send progress for the first op; subsequent ops would - // double-report. - let sender = progress_sender.take(); - let Some(op_edits) = self.run_single_rename(&op, sender) else { - continue; - }; - for (u, edits) in op_edits { - all_edits.entry(u).or_default().extend(edits); - } - } + let progress_sender = params + .work_done_progress_params + .work_done_token + .map(|token| self.with_report_progress(token)); + + let ops = self + .state + .compute_rename_ops(&decl_uri, decl_pos, &new_name, &ipath); + let Some(all_edits) = self + .state + .apply_rename_ops(&ops, workspace_path, progress_sender) + else { + error!(uri=%decl_uri, "failed to apply primary rename"); + return Box::pin(async move { Ok(None) }); + }; let response = if all_edits.is_empty() { None @@ -316,205 +311,6 @@ impl ProtoLanguageServer { Box::pin(async move { Ok(response) }) } - fn run_single_rename( - &mut self, - op: &RenameOp, - progress_sender: Option>, - ) -> Option>> { - let tree = self.state.get_tree(&op.uri)?; - let content = self.state.get_content(&op.uri); - let package = tree.get_package_name(content.as_bytes()).unwrap_or("."); - - let (edit, otext, ntext) = tree.rename_tree(&op.pos, &op.new_name, content.as_bytes())?; - let workspace = self.configs.get_workspace_for_uri(&op.uri)?; - - let mut h: HashMap> = HashMap::new(); - h.extend(self.state.rename_fields( - package, - &otext, - &ntext, - workspace.to_file_path().ok()?, - progress_sender, - )); - h.entry(tree.uri.clone()).or_default().extend(edit); - Some(h) - } - - /// Compute the additional rpc/request/response sibling renames triggered - /// by the user's primary rename at `(decl_uri, decl_pos) → new_name`. - /// Returns an empty vec when no chain applies. The user's primary rename - /// is *not* included in the returned ops — the caller always applies it - /// separately, so a primary on a convention-named message still happens - /// even if uniqueness checks block the siblings. - fn compute_rpc_chain_siblings( - &mut self, - decl_uri: &Url, - decl_pos: async_lsp::lsp_types::Position, - new_name: &str, - ) -> Vec { - if new_name.is_empty() { - return vec![]; - } - let Some(tree) = self.state.get_tree(decl_uri) else { - return vec![]; - }; - let content = self.state.get_content(decl_uri); - let bytes = content.as_bytes(); - - // Case A: user invoked rename on an rpc_name. The primary is the rpc - // rename; siblings are the convention-matching request/response. - if let Some((old_rpc_name, request_text, response_text)) = - tree.rpc_at_position(&decl_pos, bytes) - { - return self.sibling_message_ops( - decl_uri, - &old_rpc_name, - new_name, - &[ - ("Request", request_text.as_str()), - ("Response", response_text.as_str()), - ], - ); - } - - // Case B: user invoked rename on a message_name matching the - // convention. The primary is the message rename; we additionally - // rename the rpc and the opposite sibling (if it follows convention - // and the new name preserves the convention). - let Some(msg_name) = tree.message_name_at_position(&decl_pos, bytes) else { - return vec![]; - }; - let (rpc_base, primary_suffix, new_rpc_base) = if let (Some(rpc), Some(new_rpc)) = ( - msg_name.strip_suffix("Request"), - new_name.strip_suffix("Request"), - ) { - (rpc.to_owned(), "Request", new_rpc.to_owned()) - } else if let (Some(rpc), Some(new_rpc)) = ( - msg_name.strip_suffix("Response"), - new_name.strip_suffix("Response"), - ) { - (rpc.to_owned(), "Response", new_rpc.to_owned()) - } else { - return vec![]; - }; - if rpc_base.is_empty() || new_rpc_base.is_empty() { - return vec![]; - } - - // Locate the rpc and confirm it matches both the rpc base name and - // uses this message in the expected slot. - let Some(rpc_loc) = self.state.find_rpc_decl(&rpc_base) else { - return vec![]; - }; - let Some(rpc_tree) = self.state.get_tree(&rpc_loc.uri) else { - return vec![]; - }; - let rpc_content = self.state.get_content(&rpc_loc.uri); - let Some((_, rpc_req, rpc_resp)) = - rpc_tree.rpc_at_position(&rpc_loc.range.start, rpc_content.as_bytes()) - else { - return vec![]; - }; - let expected_primary = format!("{rpc_base}{primary_suffix}"); - let primary_slot_matches = if primary_suffix == "Request" { - trailing_segment(&rpc_req) == expected_primary - } else { - trailing_segment(&rpc_resp) == expected_primary - }; - if !primary_slot_matches { - return vec![]; - } - - // Uniqueness: only chain the rpc + opposite sibling if the user's - // primary message is referenced by exactly this one rpc. Otherwise a - // chained rename would silently break another rpc that shares the - // type. - if self.state.count_rpc_uses_of_type(&expected_primary) != 1 { - return vec![]; - } - - let mut ops = vec![RenameOp { - uri: rpc_loc.uri.clone(), - pos: rpc_loc.range.start, - new_name: new_rpc_base.clone(), - }]; - - let opposite_suffix = if primary_suffix == "Request" { - "Response" - } else { - "Request" - }; - let opposite_text = if opposite_suffix == "Request" { - rpc_req.as_str() - } else { - rpc_resp.as_str() - }; - ops.extend(self.sibling_message_ops( - &rpc_loc.uri, - &rpc_base, - &new_rpc_base, - &[(opposite_suffix, opposite_text)], - )); - ops - } - - /// Resolve each `(suffix, type_text)` slot to a message declaration and - /// build the corresponding rename op, gated on convention + uniqueness. - fn sibling_message_ops( - &mut self, - anchor_uri: &Url, - old_rpc_name: &str, - new_rpc_name: &str, - slots: &[(&str, &str)], - ) -> Vec { - let Some(anchor_tree) = self.state.get_tree(anchor_uri) else { - return vec![]; - }; - let anchor_content = self.state.get_content(anchor_uri); - let anchor_package = anchor_tree - .get_package_name(anchor_content.as_bytes()) - .unwrap_or(".") - .to_owned(); - let ipath = self - .configs - .get_include_paths(anchor_uri) - .unwrap_or_default(); - - let mut ops = vec![]; - for (suffix, type_text) in slots { - let expected_name = format!("{old_rpc_name}{suffix}"); - if trailing_segment(type_text) != expected_name { - continue; - } - if self.state.count_rpc_uses_of_type(&expected_name) != 1 { - continue; - } - let locations = self.state.definition( - &ipath, - &anchor_package, - Jumpable::Identifier((*type_text).to_owned()), - ); - let Some(decl) = locations.into_iter().next() else { - continue; - }; - ops.push(RenameOp { - uri: decl.uri, - pos: decl.range.start, - new_name: format!("{new_rpc_name}{suffix}"), - }); - } - ops - } -} - -#[derive(Clone, Debug)] -struct RenameOp { - uri: Url, - pos: async_lsp::lsp_types::Position, - new_name: String, -} - -impl ProtoLanguageServer { pub(super) fn references( &mut self, param: ReferenceParams, diff --git a/src/workspace/rename.rs b/src/workspace/rename.rs index 0f4e8bf..c2cc026 100644 --- a/src/workspace/rename.rs +++ b/src/workspace/rename.rs @@ -1,13 +1,24 @@ -use crate::nodekind::NodeKind; -use crate::utils::{split_identifier_package, trailing_segment, ts_to_lsp_position}; use std::collections::HashMap; use std::path::PathBuf; +use std::sync::mpsc::Sender; -use async_lsp::lsp_types::{Location, Range, TextEdit, Url}; +use async_lsp::lsp_types::{Location, Position, ProgressParamsValue, Range, TextEdit, Url}; +use crate::context::jumpable::Jumpable; +use crate::nodekind::NodeKind; use crate::state::ProtoLanguageState; -use async_lsp::lsp_types::ProgressParamsValue; -use std::sync::mpsc::Sender; +use crate::utils::{split_identifier_package, trailing_segment, ts_to_lsp_position}; + +/// A single rename operation to apply against the workspace: rename whatever +/// symbol is declared at `(uri, pos)` to `new_name`. Multiple ops are merged +/// into one `WorkspaceEdit` when a single user invocation triggers chained +/// renames (e.g. rpc + request + response). +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct RenameOp { + pub uri: Url, + pub pos: Position, + pub new_name: String, +} impl ProtoLanguageState { pub fn rename_fields( @@ -171,6 +182,264 @@ impl ProtoLanguageState { } count } + + /// Compute every rename operation that should run for a user's rename + /// invocation. The first op is the user's *primary* rename (always + /// present); any additional ops are rpc/request/response chain siblings. + /// + /// `decl_uri`/`decl_pos` must already point at the *declaration* of the + /// symbol being renamed — the caller is responsible for pivoting from a + /// reference site to the declaration before calling this. + pub fn compute_rename_ops( + &self, + decl_uri: &Url, + decl_pos: Position, + new_name: &str, + ipath: &[PathBuf], + ) -> Vec { + let mut ops = vec![RenameOp { + uri: decl_uri.clone(), + pos: decl_pos, + new_name: new_name.to_owned(), + }]; + ops.extend(self.compute_chain_siblings(decl_uri, decl_pos, new_name, ipath)); + ops + } + + /// Apply a sequence of rename ops, merging their per-file edits into a + /// single map. Returns `None` if the *primary* (first) op fails — in that + /// case the user's invocation should produce no edit at all. Sibling + /// failures are silently skipped so the primary always lands. + pub fn apply_rename_ops( + &mut self, + ops: &[RenameOp], + workspace: PathBuf, + progress_sender: Option>, + ) -> Option>> { + let mut all: HashMap> = HashMap::new(); + let mut progress = progress_sender; + for (i, op) in ops.iter().enumerate() { + // Only the first op gets the progress sender; subsequent ops would + // double-report. + let sender = progress.take(); + match self.run_single_rename(op, workspace.clone(), sender) { + Some(edits) => { + for (u, e) in edits { + all.entry(u).or_default().extend(e); + } + } + None if i == 0 => return None, + None => continue, + } + } + Some(all) + } + + fn run_single_rename( + &mut self, + op: &RenameOp, + workspace: PathBuf, + progress_sender: Option>, + ) -> Option>> { + let tree = self.get_tree(&op.uri)?; + let content = self.get_content(&op.uri); + let package = tree.get_package_name(content.as_bytes()).unwrap_or("."); + + let (edit, otext, ntext) = tree.rename_tree(&op.pos, &op.new_name, content.as_bytes())?; + + let mut h: HashMap> = HashMap::new(); + h.extend(self.rename_fields(package, &otext, &ntext, workspace, progress_sender)); + h.entry(tree.uri.clone()).or_default().extend(edit); + Some(h) + } + + fn compute_chain_siblings( + &self, + decl_uri: &Url, + decl_pos: Position, + new_name: &str, + ipath: &[PathBuf], + ) -> Vec { + if new_name.is_empty() { + return vec![]; + } + let Some(tree) = self.get_tree(decl_uri) else { + return vec![]; + }; + let content = self.get_content(decl_uri); + let bytes = content.as_bytes(); + + if tree.rpc_at_position(&decl_pos, bytes).is_some() { + return self.chain_from_rpc_cursor(decl_uri, decl_pos, new_name, ipath); + } + if tree.message_name_at_position(&decl_pos, bytes).is_some() { + return self.chain_from_message_cursor(decl_uri, decl_pos, new_name, ipath); + } + vec![] + } + + /// Case A: user invoked rename on an `rpc_name`. The primary is the rpc + /// rename; siblings are the convention-matching request/response messages. + fn chain_from_rpc_cursor( + &self, + decl_uri: &Url, + decl_pos: Position, + new_name: &str, + ipath: &[PathBuf], + ) -> Vec { + let tree = self.get_tree(decl_uri).expect("checked by caller"); + let content = self.get_content(decl_uri); + let (old_rpc_name, request_text, response_text) = tree + .rpc_at_position(&decl_pos, content.as_bytes()) + .expect("checked by caller"); + self.sibling_message_ops( + decl_uri, + &old_rpc_name, + new_name, + ipath, + &[ + ("Request", request_text.as_str()), + ("Response", response_text.as_str()), + ], + ) + } + + /// Case B: user invoked rename on a `message_name` matching the + /// `Request` / `Response` convention. Primary is the message + /// rename; we additionally rename the rpc and the opposite sibling. + fn chain_from_message_cursor( + &self, + decl_uri: &Url, + decl_pos: Position, + new_name: &str, + ipath: &[PathBuf], + ) -> Vec { + let tree = self.get_tree(decl_uri).expect("checked by caller"); + let content = self.get_content(decl_uri); + let msg_name = tree + .message_name_at_position(&decl_pos, content.as_bytes()) + .expect("checked by caller"); + + let Some((rpc_base, primary_suffix, new_rpc_base)) = + strip_convention_suffix(&msg_name, new_name) + else { + return vec![]; + }; + if rpc_base.is_empty() || new_rpc_base.is_empty() { + return vec![]; + } + + // Locate the rpc and confirm it uses this message in the expected slot. + let Some(rpc_loc) = self.find_rpc_decl(&rpc_base) else { + return vec![]; + }; + let Some(rpc_tree) = self.get_tree(&rpc_loc.uri) else { + return vec![]; + }; + let rpc_content = self.get_content(&rpc_loc.uri); + let Some((_, rpc_req, rpc_resp)) = + rpc_tree.rpc_at_position(&rpc_loc.range.start, rpc_content.as_bytes()) + else { + return vec![]; + }; + let expected_primary = format!("{rpc_base}{primary_suffix}"); + let primary_slot_matches = if primary_suffix == "Request" { + trailing_segment(&rpc_req) == expected_primary + } else { + trailing_segment(&rpc_resp) == expected_primary + }; + if !primary_slot_matches { + return vec![]; + } + + // Uniqueness: only chain if the user's primary message is referenced + // by exactly one rpc. Otherwise a chained rename would silently break + // another rpc that shares the type. + if self.count_rpc_uses_of_type(&expected_primary) != 1 { + return vec![]; + } + + let mut ops = vec![RenameOp { + uri: rpc_loc.uri.clone(), + pos: rpc_loc.range.start, + new_name: new_rpc_base.clone(), + }]; + let (opposite_suffix, opposite_text) = if primary_suffix == "Request" { + ("Response", rpc_resp.as_str()) + } else { + ("Request", rpc_req.as_str()) + }; + ops.extend(self.sibling_message_ops( + &rpc_loc.uri, + &rpc_base, + &new_rpc_base, + ipath, + &[(opposite_suffix, opposite_text)], + )); + ops + } + + /// Resolve each `(suffix, type_text)` slot to a message declaration and + /// build the corresponding rename op, gated on convention + uniqueness. + fn sibling_message_ops( + &self, + anchor_uri: &Url, + old_rpc_name: &str, + new_rpc_name: &str, + ipath: &[PathBuf], + slots: &[(&str, &str)], + ) -> Vec { + let Some(anchor_tree) = self.get_tree(anchor_uri) else { + return vec![]; + }; + let anchor_content = self.get_content(anchor_uri); + let anchor_package = anchor_tree + .get_package_name(anchor_content.as_bytes()) + .unwrap_or(".") + .to_owned(); + + let mut ops = vec![]; + for (suffix, type_text) in slots { + let expected_name = format!("{old_rpc_name}{suffix}"); + if trailing_segment(type_text) != expected_name { + continue; + } + if self.count_rpc_uses_of_type(&expected_name) != 1 { + continue; + } + let locations = self.definition( + ipath, + &anchor_package, + Jumpable::Identifier((*type_text).to_owned()), + ); + let Some(decl) = locations.into_iter().next() else { + continue; + }; + ops.push(RenameOp { + uri: decl.uri, + pos: decl.range.start, + new_name: format!("{new_rpc_name}{suffix}"), + }); + } + ops + } +} + +/// If `msg_name` ends with `Request` or `Response` and `new_name` ends with +/// the same suffix, return `(rpc_base, suffix, new_rpc_base)`. Otherwise +/// `None`. Used to detect when a message rename can plausibly drive a chain. +fn strip_convention_suffix( + msg_name: &str, + new_name: &str, +) -> Option<(String, &'static str, String)> { + for suffix in ["Request", "Response"] { + if let (Some(base), Some(new_base)) = + (msg_name.strip_suffix(suffix), new_name.strip_suffix(suffix)) + { + return Some((base.to_owned(), suffix, new_base.to_owned())); + } + } + None } #[cfg(test)] From 5be6127bee321074616fe219191b5c55e7e24e32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20K=C3=A5gedal=20Reimer?= Date: Thu, 14 May 2026 08:47:23 +0200 Subject: [PATCH 09/10] fix(rename): guard rpc chain against cross-package name collisions When the user renames a `Request` / `Response` message, the chain previously looked up the matching rpc by simple name and took the first hit. If another package happened to declare an rpc with the same simple name, iteration order alone determined whether the chain fired or silently no-op'd. Replace `find_rpc_decl` (Option) with `find_rpc_decls` (Vec), enumerate all candidates, and keep only the unique one whose request/response slot resolves (through the workspace's name-resolution rules) back to the user's primary message. Comparison uses a span-inclusive `position_in_range` so a mid-identifier cursor still matches the definition's full-range Location. Adds collision_foo.proto / collision_bar.proto fixtures and a regression test demonstrating the chain stays inside foo. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/workspace/input/collision_bar.proto | 10 ++ src/workspace/input/collision_foo.proto | 10 ++ src/workspace/rename.rs | 138 +++++++++++++++++++----- 3 files changed, 129 insertions(+), 29 deletions(-) create mode 100644 src/workspace/input/collision_bar.proto create mode 100644 src/workspace/input/collision_foo.proto diff --git a/src/workspace/input/collision_bar.proto b/src/workspace/input/collision_bar.proto new file mode 100644 index 0000000..6904a34 --- /dev/null +++ b/src/workspace/input/collision_bar.proto @@ -0,0 +1,10 @@ +syntax = "proto3"; + +package com.bar; + +message ListReq {} +message ListResp {} + +service Other { + rpc GetBook(ListReq) returns (ListResp); +} diff --git a/src/workspace/input/collision_foo.proto b/src/workspace/input/collision_foo.proto new file mode 100644 index 0000000..9b4bc27 --- /dev/null +++ b/src/workspace/input/collision_foo.proto @@ -0,0 +1,10 @@ +syntax = "proto3"; + +package com.foo; + +message GetBookRequest {} +message GetBookResponse {} + +service Library { + rpc GetBook(GetBookRequest) returns (GetBookResponse); +} diff --git a/src/workspace/rename.rs b/src/workspace/rename.rs index c2cc026..080f1e0 100644 --- a/src/workspace/rename.rs +++ b/src/workspace/rename.rs @@ -141,10 +141,13 @@ impl ProtoLanguageState { if r.is_empty() { None } else { Some(r) } } - /// Find the declaration of an rpc by simple name across the workspace. - /// Returns the first match. Used by the rpc/request/response chain rename - /// to anchor the chain when the user invokes rename on a matching message. - pub fn find_rpc_decl(&self, rpc_name: &str) -> Option { + /// Find every rpc declaration in the workspace whose simple name matches + /// `rpc_name`. Used by the rpc/request/response chain rename to enumerate + /// candidate rpcs when the user invokes rename on a convention-named + /// message; the caller then narrows the list by checking which candidate + /// actually references the user's primary message. + pub fn find_rpc_decls(&self, rpc_name: &str) -> Vec { + let mut out = vec![]; for tree in self.get_trees() { let content = self.get_content(&tree.uri); for node in tree.find_all_nodes(NodeKind::is_rpc_name) { @@ -152,7 +155,7 @@ impl ProtoLanguageState { continue; }; if text == rpc_name { - return Some(Location { + out.push(Location { uri: tree.uri.clone(), range: Range { start: ts_to_lsp_position(&node.start_position()), @@ -162,7 +165,7 @@ impl ProtoLanguageState { } } } - None + out } /// Count the number of rpcs in the workspace whose request or response @@ -307,6 +310,12 @@ impl ProtoLanguageState { /// Case B: user invoked rename on a `message_name` matching the /// `Request` / `Response` convention. Primary is the message /// rename; we additionally rename the rpc and the opposite sibling. + /// + /// To guard against unrelated rpcs in other packages that happen to share + /// the same simple name, we enumerate *all* candidate rpcs and pick the + /// unique one whose request/response slot resolves (via the workspace's + /// name-resolution rules) to the user's primary message. If zero or more + /// than one rpc matches, the chain is silently dropped. fn chain_from_message_cursor( &self, decl_uri: &Url, @@ -329,32 +338,44 @@ impl ProtoLanguageState { return vec![]; } - // Locate the rpc and confirm it uses this message in the expected slot. - let Some(rpc_loc) = self.find_rpc_decl(&rpc_base) else { - return vec![]; - }; - let Some(rpc_tree) = self.get_tree(&rpc_loc.uri) else { - return vec![]; - }; - let rpc_content = self.get_content(&rpc_loc.uri); - let Some((_, rpc_req, rpc_resp)) = - rpc_tree.rpc_at_position(&rpc_loc.range.start, rpc_content.as_bytes()) - else { - return vec![]; - }; - let expected_primary = format!("{rpc_base}{primary_suffix}"); - let primary_slot_matches = if primary_suffix == "Request" { - trailing_segment(&rpc_req) == expected_primary - } else { - trailing_segment(&rpc_resp) == expected_primary - }; - if !primary_slot_matches { + // Enumerate rpcs by simple name, then keep only those whose primary + // slot resolves to the user's actual declaration. + let mut matching: Vec<(Location, String, String)> = vec![]; + for rpc_loc in self.find_rpc_decls(&rpc_base) { + let Some(rpc_tree) = self.get_tree(&rpc_loc.uri) else { + continue; + }; + let rpc_content = self.get_content(&rpc_loc.uri); + let Some((_, rpc_req, rpc_resp)) = + rpc_tree.rpc_at_position(&rpc_loc.range.start, rpc_content.as_bytes()) + else { + continue; + }; + let slot_text = if primary_suffix == "Request" { + &rpc_req + } else { + &rpc_resp + }; + let rpc_pkg = rpc_tree + .get_package_name(rpc_content.as_bytes()) + .unwrap_or("."); + let resolves_to_primary = self + .definition(ipath, rpc_pkg, Jumpable::Identifier(slot_text.clone())) + .iter() + .any(|l| l.uri == *decl_uri && position_in_range(decl_pos, l.range)); + if resolves_to_primary { + matching.push((rpc_loc, rpc_req, rpc_resp)); + } + } + if matching.len() != 1 { return vec![]; } + let (rpc_loc, rpc_req, rpc_resp) = matching.into_iter().next().unwrap(); // Uniqueness: only chain if the user's primary message is referenced // by exactly one rpc. Otherwise a chained rename would silently break // another rpc that shares the type. + let expected_primary = format!("{rpc_base}{primary_suffix}"); if self.count_rpc_uses_of_type(&expected_primary) != 1 { return vec![]; } @@ -425,6 +446,14 @@ impl ProtoLanguageState { } } +/// True iff `pos` falls within `range` (inclusive of both endpoints). Used to +/// match a possibly mid-identifier cursor against a definition's full-span +/// range when verifying that two locations refer to the same symbol. +fn position_in_range(pos: Position, range: Range) -> bool { + (pos.line, pos.character) >= (range.start.line, range.start.character) + && (pos.line, pos.character) <= (range.end.line, range.end.character) +} + /// If `msg_name` ends with `Request` or `Response` and `new_name` ends with /// the same suffix, return `(rpc_base, suffix, new_rpc_base)`. Otherwise /// `None`. Used to detect when a message rename can plausibly drive a chain. @@ -521,7 +550,7 @@ mod test { } #[test] - fn test_find_rpc_decl_and_count_uses() { + fn test_find_rpc_decls_and_count_uses() { let ipath = vec![PathBuf::from("src/workspace/input")]; let svc_uri = "file://input/service.proto".parse().unwrap(); let msg_uri = "file://input/messages.proto".parse().unwrap(); @@ -547,11 +576,13 @@ mod test { ); // Lookup hits the rpc_name node in service.proto. - let loc = state.find_rpc_decl("GetBook").expect("rpc not found"); + let mut locs = state.find_rpc_decls("GetBook"); + assert_eq!(locs.len(), 1, "expected exactly one GetBook rpc"); + let loc = locs.pop().unwrap(); assert!(loc.uri.as_str().ends_with("service.proto")); assert_eq!(loc.range.start.line, 7); - assert!(state.find_rpc_decl("DoesNotExist").is_none()); + assert!(state.find_rpc_decls("DoesNotExist").is_empty()); // Convention-following types are uniquely used. assert_eq!(state.count_rpc_uses_of_type("GetBookRequest"), 1); @@ -564,4 +595,53 @@ mod test { // Unrelated names see no uses. assert_eq!(state.count_rpc_uses_of_type("Unrelated"), 0); } + + #[test] + fn test_compute_rename_ops_cross_package_collision_blocks_chain() { + // Two services in different packages each declare `rpc GetBook`. Only + // pkg foo follows the convention; pkg bar uses unrelated message + // names. Without the per-candidate full-qualified resolution check, + // find_rpc_decls' iteration order could let bar's GetBook poison the + // chain. With the fix, foo's GetBook is uniquely identified by + // resolving its request slot back to the user's primary message. + let ipath = vec![PathBuf::from("src/workspace/input")]; + let foo_uri = "file://input/collision_foo.proto".parse().unwrap(); + let bar_uri = "file://input/collision_bar.proto".parse().unwrap(); + let foo = include_str!("input/collision_foo.proto"); + let bar = include_str!("input/collision_bar.proto"); + + let mut state: ProtoLanguageState = ProtoLanguageState::new(); + state.upsert_file( + &foo_uri, + foo.to_owned(), + &ipath, + 2, + &Config::default(), + false, + ); + state.upsert_file( + &bar_uri, + bar.to_owned(), + &ipath, + 2, + &Config::default(), + false, + ); + + // Cursor on foo.GetBookRequest message_name (line 4, col 8..22). + let pos = async_lsp::lsp_types::Position { + line: 4, + character: 12, + }; + let ops = state.compute_rename_ops(&foo_uri, pos, "FetchBookRequest", &ipath); + + // Primary + foo's rpc + foo's response. bar's GetBook is NOT touched. + assert_eq!(ops.len(), 3, "expected exactly the foo trio, got {ops:?}"); + for op in &ops { + assert!( + op.uri.as_str().contains("collision_foo"), + "chain leaked into bar: {op:?}" + ); + } + } } From a6b6f408385ac5a7ae13fb98f8ea7dbf071137a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20K=C3=A5gedal=20Reimer?= Date: Thu, 14 May 2026 08:53:20 +0200 Subject: [PATCH 10/10] test(rename): add chain orchestration tests for ProtoLanguageState End-to-end coverage of the rpc/request/response chain logic without needing a ClientSocket: - chain from rpc cursor (primary + 2 sibling ops) - chain from request cursor (primary + rpc + response) - shared request blocks chain via uniqueness check - new name not preserving convention blocks chain - reference-site cursor: documents that the LSP layer must pivot first - apply_rename_ops merges edits across primary + siblings Pulls out `make_state` and `op` helpers to keep the new tests terse. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/workspace/rename.rs | 295 ++++++++++++++++-- ...pply_rename_ops_chain_from_rpc_cursor.snap | 46 +++ 2 files changed, 321 insertions(+), 20 deletions(-) create mode 100644 src/workspace/snapshots/protols__workspace__rename__test__apply_rename_ops_chain_from_rpc_cursor.snap diff --git a/src/workspace/rename.rs b/src/workspace/rename.rs index 080f1e0..e68e76c 100644 --- a/src/workspace/rename.rs +++ b/src/workspace/rename.rs @@ -475,10 +475,36 @@ fn strip_convention_suffix( mod test { use std::path::PathBuf; + use async_lsp::lsp_types::Position; use insta::assert_yaml_snapshot; use crate::config::Config; use crate::state::ProtoLanguageState; + use crate::workspace::rename::RenameOp; + + fn make_state(files: &[(&str, &str)], ipath: &[PathBuf]) -> ProtoLanguageState { + let mut state = ProtoLanguageState::new(); + for (uri, content) in files { + let parsed_uri = uri.parse().unwrap(); + state.upsert_file( + &parsed_uri, + (*content).to_owned(), + ipath, + 2, + &Config::default(), + false, + ); + } + state + } + + fn op(uri: &str, line: u32, character: u32, new_name: &str) -> RenameOp { + RenameOp { + uri: uri.parse().unwrap(), + pos: Position { line, character }, + new_name: new_name.to_owned(), + } + } #[test] fn test_rename() { @@ -606,30 +632,22 @@ mod test { // resolving its request slot back to the user's primary message. let ipath = vec![PathBuf::from("src/workspace/input")]; let foo_uri = "file://input/collision_foo.proto".parse().unwrap(); - let bar_uri = "file://input/collision_bar.proto".parse().unwrap(); - let foo = include_str!("input/collision_foo.proto"); - let bar = include_str!("input/collision_bar.proto"); - - let mut state: ProtoLanguageState = ProtoLanguageState::new(); - state.upsert_file( - &foo_uri, - foo.to_owned(), - &ipath, - 2, - &Config::default(), - false, - ); - state.upsert_file( - &bar_uri, - bar.to_owned(), + let state = make_state( + &[ + ( + "file://input/collision_foo.proto", + include_str!("input/collision_foo.proto"), + ), + ( + "file://input/collision_bar.proto", + include_str!("input/collision_bar.proto"), + ), + ], &ipath, - 2, - &Config::default(), - false, ); // Cursor on foo.GetBookRequest message_name (line 4, col 8..22). - let pos = async_lsp::lsp_types::Position { + let pos = Position { line: 4, character: 12, }; @@ -644,4 +662,241 @@ mod test { ); } } + + #[test] + fn test_compute_rename_ops_chain_from_rpc_cursor() { + let ipath = vec![PathBuf::from("src/workspace/input")]; + let svc_uri = "file://input/service.proto".parse().unwrap(); + let state = make_state( + &[ + ( + "file://input/service.proto", + include_str!("input/service.proto"), + ), + ( + "file://input/messages.proto", + include_str!("input/messages.proto"), + ), + ], + &ipath, + ); + + // Cursor on the `GetBook` rpc name (line 7, col 8..15). + let pos = Position { + line: 7, + character: 10, + }; + let ops = state.compute_rename_ops(&svc_uri, pos, "FetchBook", &ipath); + + // Primary + Request + Response. + assert_eq!( + ops.len(), + 3, + "expected primary rpc + two sibling ops, got {ops:?}" + ); + assert_eq!(ops[0], op("file://input/service.proto", 7, 10, "FetchBook")); + assert_eq!( + ops[1], + op("file://input/messages.proto", 4, 8, "FetchBookRequest") + ); + assert_eq!( + ops[2], + op("file://input/messages.proto", 5, 8, "FetchBookResponse") + ); + } + + #[test] + fn test_compute_rename_ops_chain_from_request_cursor() { + let ipath = vec![PathBuf::from("src/workspace/input")]; + let msg_uri = "file://input/messages.proto".parse().unwrap(); + let state = make_state( + &[ + ( + "file://input/service.proto", + include_str!("input/service.proto"), + ), + ( + "file://input/messages.proto", + include_str!("input/messages.proto"), + ), + ], + &ipath, + ); + + // Cursor on `GetBookRequest` message name (line 4, col 8..22). + let pos = Position { + line: 4, + character: 12, + }; + let ops = state.compute_rename_ops(&msg_uri, pos, "FetchBookRequest", &ipath); + + // Primary (request) + rpc + response. + assert_eq!( + ops.len(), + 3, + "expected primary + rpc + response sibling ops, got {ops:?}" + ); + assert_eq!( + ops[0], + op("file://input/messages.proto", 4, 12, "FetchBookRequest") + ); + assert_eq!(ops[1], op("file://input/service.proto", 7, 8, "FetchBook")); + assert_eq!( + ops[2], + op("file://input/messages.proto", 5, 8, "FetchBookResponse") + ); + } + + #[test] + fn test_compute_rename_ops_shared_request_blocks_chain() { + let ipath = vec![PathBuf::from("src/workspace/input")]; + let msg_uri = "file://input/messages.proto".parse().unwrap(); + let state = make_state( + &[ + ( + "file://input/service.proto", + include_str!("input/service.proto"), + ), + ( + "file://input/messages.proto", + include_str!("input/messages.proto"), + ), + ], + &ipath, + ); + + // Cursor on `SharedReq` message name (line 10). Both ListA and ListB + // reference SharedReq — chain must not fire. + let pos = Position { + line: 10, + character: 12, + }; + let ops = state.compute_rename_ops(&msg_uri, pos, "RenamedReq", &ipath); + assert_eq!( + ops.len(), + 1, + "expected primary-only (no chain), got {ops:?}" + ); + assert_eq!( + ops[0], + op("file://input/messages.proto", 10, 12, "RenamedReq") + ); + } + + #[test] + fn test_compute_rename_ops_new_name_breaks_convention() { + let ipath = vec![PathBuf::from("src/workspace/input")]; + let msg_uri = "file://input/messages.proto".parse().unwrap(); + let state = make_state( + &[ + ( + "file://input/service.proto", + include_str!("input/service.proto"), + ), + ( + "file://input/messages.proto", + include_str!("input/messages.proto"), + ), + ], + &ipath, + ); + + // Cursor on GetBookRequest, but new name doesn't preserve the + // `Request` convention — primary-only. + let pos = Position { + line: 4, + character: 12, + }; + let ops = state.compute_rename_ops(&msg_uri, pos, "Whatever", &ipath); + assert_eq!( + ops.len(), + 1, + "expected primary-only (no chain), got {ops:?}" + ); + assert_eq!(ops[0], op("file://input/messages.proto", 4, 12, "Whatever")); + } + + #[test] + fn test_compute_rename_ops_reference_site_pivot_unsupported() { + // compute_rename_ops requires the caller to have pivoted to the + // declaration. As a sanity check, calling it with a cursor on a + // reference site (not a declaration) yields a primary-only op that + // would no-op the workspace pass. The LSP layer is responsible for + // resolving the reference to its declaration *before* calling this. + let ipath = vec![PathBuf::from("src/workspace/input")]; + let svc_uri = "file://input/service.proto".parse().unwrap(); + let state = make_state( + &[ + ( + "file://input/service.proto", + include_str!("input/service.proto"), + ), + ( + "file://input/messages.proto", + include_str!("input/messages.proto"), + ), + ], + &ipath, + ); + + // Cursor on the `GetBookRequest` reference inside the rpc signature + // (line 7, col 16..30). + let pos = Position { + line: 7, + character: 20, + }; + let ops = state.compute_rename_ops(&svc_uri, pos, "RenamedRequest", &ipath); + // Single op (the primary at the reference site) — no chain. This + // documents the contract: the LSP layer must pivot first. + assert_eq!(ops.len(), 1, "{ops:?}"); + } + + #[test] + fn test_apply_rename_ops_chain_from_rpc_cursor() { + // End-to-end snapshot of the full `rpc (Request) returns + // (Response)` convention chain. Renaming `GetBook` → + // `FetchBook` should fan out into: + // - the rpc_name span in service.proto, + // - the request type-reference inside that rpc's signature, + // - the response type-reference inside that rpc's signature, + // - the `GetBookRequest` message decl in messages.proto, + // - the `GetBookResponse` message decl in messages.proto. + // The snapshot pins both the URIs and the exact edit ranges so any + // regression in chain detection, edit merging, or workspace-pass + // resolution will show up here. + let ipath = vec![PathBuf::from("src/workspace/input")]; + let svc_uri = "file://input/service.proto".parse().unwrap(); + let mut state = make_state( + &[ + ( + "file://input/service.proto", + include_str!("input/service.proto"), + ), + ( + "file://input/messages.proto", + include_str!("input/messages.proto"), + ), + ], + &ipath, + ); + + let pos = Position { + line: 7, + character: 10, + }; + let ops = state.compute_rename_ops(&svc_uri, pos, "FetchBook", &ipath); + let edits = state + .apply_rename_ops(&ops, PathBuf::from("src/workspace/input"), None) + .expect("primary rename should not fail"); + + // Sort within each file so the snapshot is order-independent across + // unrelated implementation changes (e.g. op evaluation order). + let mut normalized: std::collections::BTreeMap> = + std::collections::BTreeMap::new(); + for (url, mut v) in edits { + v.sort_by_key(|e| (e.range.start.line, e.range.start.character)); + normalized.insert(url.to_string(), v); + } + assert_yaml_snapshot!(normalized); + } } diff --git a/src/workspace/snapshots/protols__workspace__rename__test__apply_rename_ops_chain_from_rpc_cursor.snap b/src/workspace/snapshots/protols__workspace__rename__test__apply_rename_ops_chain_from_rpc_cursor.snap new file mode 100644 index 0000000..9e38963 --- /dev/null +++ b/src/workspace/snapshots/protols__workspace__rename__test__apply_rename_ops_chain_from_rpc_cursor.snap @@ -0,0 +1,46 @@ +--- +source: src/workspace/rename.rs +expression: normalized +--- +"file://input/messages.proto": + - range: + start: + line: 4 + character: 8 + end: + line: 4 + character: 22 + newText: FetchBookRequest + - range: + start: + line: 5 + character: 8 + end: + line: 5 + character: 23 + newText: FetchBookResponse +"file://input/service.proto": + - range: + start: + line: 7 + character: 8 + end: + line: 7 + character: 15 + newText: FetchBook + - range: + start: + line: 7 + character: 16 + end: + line: 7 + character: 30 + newText: FetchBookRequest + - range: + start: + line: 7 + character: 41 + end: + line: 7 + character: 56 + newText: FetchBookResponse