Fix #2297: guard PrepareRenameHandler against null rename capability#2299
Fix #2297: guard PrepareRenameHandler against null rename capability#2299manderse21 wants to merge 1 commit into
Conversation
The rename provider added in v4.6.0 (PowerShell#2292) reads capability.PrepareSupport in GetRegistrationOptions on both PrepareRenameHandler and RenameHandler. The LSP framework passes a null RenameCapability when the client's initialize omits textDocument.rename, so the dereference throws a NullReferenceException during capability registration. The exception leaves the initialize request unanswered and the handshake hangs for any client that legitimately omits the optional rename capability. Guard both handlers with a property pattern (capability is { PrepareSupport: true }) so an absent capability is treated as no prepare support rather than dereferenced. Add regression tests asserting GetRegistrationOptions tolerates a null capability.
|
@manderse21 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a regression guard for omitted textDocument.rename capabilities during LSP initialize so GetRegistrationOptions no longer throws (and hangs the handshake) when RenameCapability is null.
Changes:
- Harden
GetRegistrationOptionsin both rename handlers to toleratenullRenameCapability. - Add regression tests covering the omitted rename capability scenario for both handlers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/PowerShellEditorServices.Test/Refactoring/RenameHandlerTests.cs | Adds regression test ensuring registration options don’t throw when rename capability is omitted. |
| test/PowerShellEditorServices.Test/Refactoring/PrepareRenameHandlerTests.cs | Adds regression test ensuring prepare-rename registration options don’t throw when rename capability is omitted. |
| src/PowerShellEditorServices/Services/TextDocument/Handlers/RenameHandler.cs | Guards against null RenameCapability when computing registration options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // request; treat that as "no prepare support" instead of dereferencing it. Dereferencing | ||
| // the null capability threw a NullReferenceException that hung the initialize handshake. | ||
| // Fixes PowerShell/PowerShellEditorServices#2297. | ||
| public RenameRegistrationOptions GetRegistrationOptions(RenameCapability capability, ClientCapabilities clientCapabilities) => capability is { PrepareSupport: true } ? new() { PrepareProvider = true } : new(); |
| public RenameRegistrationOptions GetRegistrationOptions(RenameCapability capability, ClientCapabilities clientCapabilities) => capability.PrepareSupport ? new() { PrepareProvider = true } : new(); | ||
| // A null capability means the client omitted textDocument.rename; guard it rather than | ||
| // dereferencing (the NRE hung initialize). Fixes PowerShell/PowerShellEditorServices#2297. | ||
| public RenameRegistrationOptions GetRegistrationOptions(RenameCapability capability, ClientCapabilities clientCapabilities) => capability is { PrepareSupport: true } ? new() { PrepareProvider = true } : new(); |
| [Fact] | ||
| public void GetRegistrationOptionsToleratesOmittedRenameCapability() | ||
| { | ||
| // Regression for PowerShell/PowerShellEditorServices#2297: when the client's | ||
| // initialize omits textDocument.rename, the framework passes a null | ||
| // RenameCapability. GetRegistrationOptions must not dereference it -- the | ||
| // NullReferenceException hung the initialize handshake. A null capability means | ||
| // the client has no prepare support. | ||
| RenameRegistrationOptions options = testHandler.GetRegistrationOptions(null!, new()); |
Drafted PR -- PSES PrepareRename/Rename null RenameCapability guard (#2297)
Status: PR-READY, NOT SUBMITTED. The fix branch is pushed to Mike's fork:
manderse21/PowerShellEditorServices@fix/2297-prepare-rename-null-capabilityPowerShell/PowerShellEditorServices@main(which isv4.6.0, commitd2112c21)Fix #2297: guard null RenameCapability in rename handlersOpening the PR (fork branch -> upstream
main) is Mike's explicit action. Nothinghas been submitted, commented, or posted upstream.
PR title
PR body (draft)
Summary
PrepareRenameHandlerandRenameHandlerboth readcapability.PrepareSupportinGetRegistrationOptions. The language-server framework passes a nullRenameCapabilitywhen the client'sinitializeomitstextDocument.rename, so thedereference throws a
NullReferenceExceptionduring capability registration. Theexception leaves the
initializerequest unanswered, so the handshake hangs forany client that legitimately omits the optional
renamecapability (#2297).The rename provider is new in
v4.6.0(#2292), so this affectsv4.6.0.Fix
Guard both handlers with a property pattern, so an absent capability is treated as
"no prepare support" instead of dereferenced:
Tests
Adds a regression test to each handler's test class asserting
GetRegistrationOptions(null, ...)does not throw and reports no prepare provider.Validation (performed locally before submission)
dotnet build -c Releaseis clean (the Release configuration's documentation/analyzergate passes).
(
Category=PrepareRename|Category=RenameHandlerFunction, net8.0 Release).exact NRE --
RenameHandler.GetRegistrationOptions(RenameHandler.cs:37) andPrepareRenameHandler.GetRegistrationOptions(RenameHandler.cs:23) -- and they passagain with the guard restored, confirming the tests guard the regression and the fix
is the cause.
initializethat omitsrename, sent over-Stdio, never returnsan
initializeresponse against a stockv4.6.0bundle (hang reproduced); the sameprobe against a bundle in which only
Microsoft.PowerShell.EditorServices.dllisrebuilt from this branch returns the
initializeresponse and completes the handshake.PowerShellEditorServices.Testunit project runs 315/320 green on net8.0Release. The 5 failures are pre-existing environment dependencies unrelated to this
change --
CanLoadPSReadLine,CanLoadPSScriptAnalyzerAsync, and three PSSA-dependenttests (parse-error / script-marker / built-in command help) -- which need PSReadLine and
PSScriptAnalyzer provisioned (this local sandbox does not install them; the project's CI
does). No rename test fails.
Closes #2297.