Fix VS Code prepareRename raw error popup at non-renameable positions - #52
Merged
Merged
Conversation
textDocument/prepareRename threw InvalidOperationException whenever StepRenameHandler.HandlePrepareRenameAsync returned null (cursor not on a renameable binding). vscode-languageclient surfaces that as a visible error popup with the raw exception text, instead of quietly doing nothing as the LSP spec intends for a null prepareRename result. Register the handler with a nullable LspRange? result and let the null flow straight through, matching the pattern already used for textDocument/rename and reqnroll/renameTargets on the same handler. Fixes #47 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The prior fix (b614aa1) registered the handler with a nullable LspRange? result and let null flow straight through, but OmniSharp's DelegatingRequestHandler<T, TResponse>.Handle always calls JToken.FromObject((object)response, ...) with no null-check, so a null TResponse throws ArgumentNullException regardless of declared nullability -- reproducing the same raw-error-popup symptom the original PR was fixing, just from a different exception. textDocument/rename and reqnroll/renameTargets avoid this by coalescing null into an empty WorkspaceEdit/RenameTargetsResponse before it reaches OmniSharp -- but prepareRename's Range has no "empty" value that means the same as null (any Range is a valid, concrete rename target). This repo hit the identical JToken.FromObject(null) crash once before for SemanticTokensHandler and worked around it the same way there. Register the route with TResponse = JToken instead, and return JValue.CreateNull() -- a non-null JToken instance whose value *is* JSON null -- so OmniSharp never sees an actual null reference while the wire response still round-trips as null. This mirrors the identical workaround in OmniSharp's own reference consumer, omnisharp-roslyn's LanguageServerHost.cs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Summary
textDocument/prepareRename's handler inLanguageServerOptionsExtensions.InitializeCustomProtocolRoutingthrew anInvalidOperationExceptionwheneverStepRenameHandler.HandlePrepareRenameAsyncreturnednull(i.e. "rename not available at this cursor position"). The comment above the code assumedvscode-languageclientwould treat a thrown JSON-RPC error fromprepareRenameas a quiet no-op — it doesn't. It surfaces the raw exception text as a visible error popup, which is confusing for what should be silent, native-like rename behavior.prepareRenameto returnnullto mean "rename not supported here," and clients are expected to handle that quietly.HandlePrepareRenameAsyncalready returnsTask<LspRange?>, so the fix is to register theOnRequesthandler with a nullableLspRange?result and letnullpass straight through, instead of translating it into a thrown exception.textDocument/rename(HandleRenameAsync) falls back to an emptyWorkspaceEdit, andreqnroll/renameTargets(HandleRenameTargetsAsync) falls back to an emptyRenameTargetsResponse— neither of those throws.Test plan
PrepareRename_from_feature_returns_null_when_no_binding_matches_the_steptoStepRenameHandlerTests, assertingHandlePrepareRenameAsyncreturnsnull(not a thrown exception) when no binding matches the cursor position.dotnet build src/LSP/Reqnroll.IdeSupport.LSP.Server/Reqnroll.IdeSupport.LSP.Server.csproj -nologo— Build succeeded, 0 Warning(s), 0 Error(s).dotnet test tests/LSP/Reqnroll.IdeSupport.LSP.Server.Tests -nologo—Passed! - Failed: 0, Passed: 570, Skipped: 0, Total: 570, Duration: 8 sFixes #47
🤖 Generated with Claude Code