Expand rename to cover services, RPCs, fields, reference sites, and rpc/request/response chains#131
Open
skagedal wants to merge 13 commits into
Open
Expand rename to cover services, RPCs, fields, reference sites, and rpc/request/response chains#131skagedal wants to merge 13 commits into
skagedal wants to merge 13 commits into
Conversation
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.
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.
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.
When an rpc follows the convention
rpc <Name>(<Name>Request) returns (<Name>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.
- 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
When the user renames a `<Rpc>Request` / `<Rpc>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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Owner
Agreed this seems useful but I think it should be behind some configuration. |
Author
Cool, I'll have a look at that! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I would like the "rename" action to just work everywhere for the symbol under the cursor. This is wired into my muscle-memory from using other editors and I tend to do it even when it's a bit silly, like in cases where the symbol just would never be referenced from anywhere else (such as a service name in Protobuf). I like to just not have to think about it.
I filed an issue before (#129) about wanting to be able to rename a message from a referenced site. This PR includes that, but also some other rename improvements. If you would rather want me to split it into separate PRs, let me know.
I find the rpc/Request/Response chain rename especially useful. I realize all protobuf users may not follow this convention, but it should (hopefully) not get in the way for people who don't. Let me know if you would rather like this to be configurable.
Additions
Service and RPC symbol rename.
can_renamewas gated onmessage_name/enum_nameparents only, so invoking rename on a service or RPC method was a silent no-op. A newis_renameablepredicate acceptsservice_nameandrpc_name, andrename_tree's empty-ancestor path returns a single in-file edit (services/RPCs have no cross-file references to propagate).Rename from a type-reference site. Previously you had to navigate to the declaration first. Now invoking rename on a
MyRequestreference insiderpc DoThing(MyRequest)— or on the inner segment of a qualified type likeOuter.Inner— works: the LSP resolves the partial qualified path under the cursor to the declaration via the existing definition machinery and runs the regular rename from there. The user's "rename the segment under cursor" intent is honored: cursor onOuterrenamesOuter, cursor onInnerrenamesInner.Field name, oneof, and enum-value rename. Identifiers whose parent is
field,map_field,oneof_field,oneof, orenum_fieldare now renameable. Each is single-site — these don't appear as type references in other.protofiles — so the workspace pass is intentionally a no-op for them, avoiding accidental renames if a lowercase type happens to share a name.Chained
rpc/Request/Responserename. When anrpcfollows therpc <Name>(<Name>Request) returns (<Name>Response)convention from the Google API design guide (AIPs 131–136), renaming any one of the three triggers a chained rename of the other two. The chain only kicks in when all of these hold: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.
Implementation
This implementation has been assisted by Claude Code using Opus 4.7. I have read and generally understood the code and sign off on it as a human being. I have previous experience with Rust, but would not consider myself an expert. I have no previous experience with writing LSP:s.