From e28d386e20951103d8f00ddb2f945263b8b8986d Mon Sep 17 00:00:00 2001 From: Michael Greene Date: Thu, 4 Jun 2026 17:16:09 -0500 Subject: [PATCH 1/3] Enhance RenameHandler to handle null capabilities gracefully and add tests for registration options --- .../TextDocument/Handlers/RenameHandler.cs | 6 +- .../Refactoring/RenameHandlerTests.cs | 69 ++++++++++++++++--- 2 files changed, 62 insertions(+), 13 deletions(-) diff --git a/src/PowerShellEditorServices/Services/TextDocument/Handlers/RenameHandler.cs b/src/PowerShellEditorServices/Services/TextDocument/Handlers/RenameHandler.cs index 83e2b5ea1..17943dbca 100644 --- a/src/PowerShellEditorServices/Services/TextDocument/Handlers/RenameHandler.cs +++ b/src/PowerShellEditorServices/Services/TextDocument/Handlers/RenameHandler.cs @@ -20,7 +20,7 @@ internal class PrepareRenameHandler RenameService renameService ) : IPrepareRenameHandler { - public RenameRegistrationOptions GetRegistrationOptions(RenameCapability capability, ClientCapabilities clientCapabilities) => capability.PrepareSupport ? new() { PrepareProvider = true } : new(); + public RenameRegistrationOptions GetRegistrationOptions(RenameCapability capability, ClientCapabilities clientCapabilities) => capability?.PrepareSupport == true ? new() { PrepareProvider = true } : new(); public async Task Handle(PrepareRenameParams request, CancellationToken cancellationToken) => await renameService.PrepareRenameSymbol(request, cancellationToken).ConfigureAwait(false); @@ -34,7 +34,9 @@ RenameService renameService ) : IRenameHandler { // RenameOptions may only be specified if the client states that it supports prepareSupport in its initial initialize request. - public RenameRegistrationOptions GetRegistrationOptions(RenameCapability capability, ClientCapabilities clientCapabilities) => capability.PrepareSupport ? new() { PrepareProvider = true } : new(); + // Passes a null capability when the client omits textDocument.rename from its advertised capabilities (e.g. a completion-only client). + // Use the null-conditional operator so we don't throw NullReferenceException during initialize. + public RenameRegistrationOptions GetRegistrationOptions(RenameCapability capability, ClientCapabilities clientCapabilities) => capability?.PrepareSupport == true ? new() { PrepareProvider = true } : new(); public async Task Handle(RenameParams request, CancellationToken cancellationToken) => await renameService.RenameSymbol(request, cancellationToken).ConfigureAwait(false); diff --git a/test/PowerShellEditorServices.Test/Refactoring/RenameHandlerTests.cs b/test/PowerShellEditorServices.Test/Refactoring/RenameHandlerTests.cs index 3f0be5be4..0ea1155b5 100644 --- a/test/PowerShellEditorServices.Test/Refactoring/RenameHandlerTests.cs +++ b/test/PowerShellEditorServices.Test/Refactoring/RenameHandlerTests.cs @@ -7,6 +7,7 @@ using Microsoft.PowerShell.EditorServices.Services.TextDocument; using Microsoft.PowerShell.EditorServices.Test.Shared; using OmniSharp.Extensions.LanguageServer.Protocol; +using OmniSharp.Extensions.LanguageServer.Protocol.Client.Capabilities; using OmniSharp.Extensions.LanguageServer.Protocol.Models; using static PowerShellEditorServices.Test.Refactoring.RefactorUtilities; using System.Linq; @@ -24,6 +25,7 @@ public class RenameHandlerTests private readonly WorkspaceService workspace = new(NullLoggerFactory.Instance); private readonly RenameHandler testHandler; + private readonly PrepareRenameHandler testPrepareHandler; public RenameHandlerTests() { workspace.WorkspaceFolders.Add(new WorkspaceFolder @@ -31,18 +33,17 @@ public RenameHandlerTests() Uri = DocumentUri.FromFileSystemPath(TestUtilities.GetSharedPath("Refactoring")) }); - testHandler = new + RenameService renameService = new ( - new RenameService - ( - workspace, - new FakeLspSendMessageRequestFacade("I Accept"), - new EmptyConfiguration() - ) - { - DisclaimerAcceptedForSession = true //Disables UI prompts - } - ); + workspace, + new FakeLspSendMessageRequestFacade("I Accept"), + new EmptyConfiguration() + ) + { + DisclaimerAcceptedForSession = true //Disables UI prompts + }; + testHandler = new(renameService); + testPrepareHandler = new(renameService); } // Decided to keep this DAMP instead of DRY due to memberdata boundaries, duplicates with PrepareRenameHandler @@ -125,4 +126,50 @@ public async Task RenamedVariable(RenameTestTarget s) Assert.Equal(expected, actual); } + + [Fact] + public void GetRegistrationOptionsDoesNotThrowWhenCapabilityIsNull() + { + // Acts: framework hands us null when client omits the capability. + RenameRegistrationOptions opts = testHandler.GetRegistrationOptions( + capability: null, + clientCapabilities: new ClientCapabilities()); + + Assert.NotNull(opts); + // Without PrepareSupport advertised, PrepareProvider should be false. + Assert.False(opts.PrepareProvider); + } + + [Fact] + public void GetRegistrationOptionsHonorsPrepareSupportWhenCapabilityProvided() + { + RenameRegistrationOptions opts = testHandler.GetRegistrationOptions( + capability: new RenameCapability { PrepareSupport = true }, + clientCapabilities: new ClientCapabilities()); + + Assert.NotNull(opts); + Assert.True(opts.PrepareProvider); + } + + [Fact] + public void PrepareGetRegistrationOptionsDoesNotThrowWhenCapabilityIsNull() + { + RenameRegistrationOptions opts = testPrepareHandler.GetRegistrationOptions( + capability: null, + clientCapabilities: new ClientCapabilities()); + + Assert.NotNull(opts); + Assert.False(opts.PrepareProvider); + } + + [Fact] + public void PrepareGetRegistrationOptionsHonorsPrepareSupportWhenCapabilityProvided() + { + RenameRegistrationOptions opts = testPrepareHandler.GetRegistrationOptions( + capability: new RenameCapability { PrepareSupport = true }, + clientCapabilities: new ClientCapabilities()); + + Assert.NotNull(opts); + Assert.True(opts.PrepareProvider); + } } From 972a8286b2623b0f283bf2612207c65a579da04e Mon Sep 17 00:00:00 2001 From: Michael Greene Date: Thu, 4 Jun 2026 17:31:58 -0500 Subject: [PATCH 2/3] Refactor RenameHandler to support nullable capabilities and enhance tests for registration options --- .../TextDocument/Handlers/RenameHandler.cs | 4 +- .../Refactoring/RenameHandlerTests.cs | 59 ++++++++----------- 2 files changed, 27 insertions(+), 36 deletions(-) diff --git a/src/PowerShellEditorServices/Services/TextDocument/Handlers/RenameHandler.cs b/src/PowerShellEditorServices/Services/TextDocument/Handlers/RenameHandler.cs index 17943dbca..9cfebd5e1 100644 --- a/src/PowerShellEditorServices/Services/TextDocument/Handlers/RenameHandler.cs +++ b/src/PowerShellEditorServices/Services/TextDocument/Handlers/RenameHandler.cs @@ -20,7 +20,7 @@ internal class PrepareRenameHandler RenameService renameService ) : IPrepareRenameHandler { - public RenameRegistrationOptions GetRegistrationOptions(RenameCapability capability, ClientCapabilities clientCapabilities) => capability?.PrepareSupport == true ? new() { PrepareProvider = true } : new(); + public RenameRegistrationOptions GetRegistrationOptions(RenameCapability? capability, ClientCapabilities clientCapabilities) => capability?.PrepareSupport == true ? new() { PrepareProvider = true } : new(); public async Task Handle(PrepareRenameParams request, CancellationToken cancellationToken) => await renameService.PrepareRenameSymbol(request, cancellationToken).ConfigureAwait(false); @@ -36,7 +36,7 @@ RenameService renameService // RenameOptions may only be specified if the client states that it supports prepareSupport in its initial initialize request. // Passes a null capability when the client omits textDocument.rename from its advertised capabilities (e.g. a completion-only client). // Use the null-conditional operator so we don't throw NullReferenceException during initialize. - public RenameRegistrationOptions GetRegistrationOptions(RenameCapability capability, ClientCapabilities clientCapabilities) => capability?.PrepareSupport == true ? new() { PrepareProvider = true } : new(); + public RenameRegistrationOptions GetRegistrationOptions(RenameCapability? capability, ClientCapabilities clientCapabilities) => capability?.PrepareSupport == true ? new() { PrepareProvider = true } : new(); public async Task Handle(RenameParams request, CancellationToken cancellationToken) => await renameService.RenameSymbol(request, cancellationToken).ConfigureAwait(false); diff --git a/test/PowerShellEditorServices.Test/Refactoring/RenameHandlerTests.cs b/test/PowerShellEditorServices.Test/Refactoring/RenameHandlerTests.cs index 0ea1155b5..8dfca4271 100644 --- a/test/PowerShellEditorServices.Test/Refactoring/RenameHandlerTests.cs +++ b/test/PowerShellEditorServices.Test/Refactoring/RenameHandlerTests.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +using System; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.PowerShell.EditorServices.Handlers; using Microsoft.PowerShell.EditorServices.Services; @@ -127,49 +128,39 @@ public async Task RenamedVariable(RenameTestTarget s) Assert.Equal(expected, actual); } - [Fact] - public void GetRegistrationOptionsDoesNotThrowWhenCapabilityIsNull() + public enum RegistrationHandlerKind { - // Acts: framework hands us null when client omits the capability. - RenameRegistrationOptions opts = testHandler.GetRegistrationOptions( - capability: null, - clientCapabilities: new ClientCapabilities()); - - Assert.NotNull(opts); - // Without PrepareSupport advertised, PrepareProvider should be false. - Assert.False(opts.PrepareProvider); + Rename, + PrepareRename } - [Fact] - public void GetRegistrationOptionsHonorsPrepareSupportWhenCapabilityProvided() + // A null prepareSupport represents the client omitting the capability entirely (framework hands us null). + public static TheoryData RegistrationOptionsTestCases() => new() { - RenameRegistrationOptions opts = testHandler.GetRegistrationOptions( - capability: new RenameCapability { PrepareSupport = true }, - clientCapabilities: new ClientCapabilities()); - - Assert.NotNull(opts); - Assert.True(opts.PrepareProvider); - } + { RegistrationHandlerKind.Rename, null, false }, + { RegistrationHandlerKind.Rename, true, true }, + { RegistrationHandlerKind.PrepareRename, null, false }, + { RegistrationHandlerKind.PrepareRename, true, true } + }; - [Fact] - public void PrepareGetRegistrationOptionsDoesNotThrowWhenCapabilityIsNull() + [Theory] + [MemberData(nameof(RegistrationOptionsTestCases))] + public void GetRegistrationOptionsReflectsPrepareSupport(RegistrationHandlerKind handlerKind, bool? prepareSupport, bool expectedPrepareProvider) { - RenameRegistrationOptions opts = testPrepareHandler.GetRegistrationOptions( - capability: null, - clientCapabilities: new ClientCapabilities()); + RenameCapability capability = prepareSupport is bool ps + ? new RenameCapability { PrepareSupport = ps } + : null; - Assert.NotNull(opts); - Assert.False(opts.PrepareProvider); - } + Func getRegistrationOptions = handlerKind switch + { + RegistrationHandlerKind.Rename => testHandler.GetRegistrationOptions, + RegistrationHandlerKind.PrepareRename => testPrepareHandler.GetRegistrationOptions, + _ => throw new ArgumentOutOfRangeException(nameof(handlerKind)) + }; - [Fact] - public void PrepareGetRegistrationOptionsHonorsPrepareSupportWhenCapabilityProvided() - { - RenameRegistrationOptions opts = testPrepareHandler.GetRegistrationOptions( - capability: new RenameCapability { PrepareSupport = true }, - clientCapabilities: new ClientCapabilities()); + RenameRegistrationOptions opts = getRegistrationOptions(capability, new ClientCapabilities()); Assert.NotNull(opts); - Assert.True(opts.PrepareProvider); + Assert.Equal(expectedPrepareProvider, opts.PrepareProvider); } } From 4173fe8498c25b4b969fb58313075934dab749aa Mon Sep 17 00:00:00 2001 From: Michael Greene Date: Thu, 4 Jun 2026 18:46:49 -0500 Subject: [PATCH 3/3] Address review: keep interface-compatible signature; cover explicit false capability - Revert GetRegistrationOptions parameter to the non-nullable RenameCapability declared by IPrepareRenameHandler/IRenameHandler, and rely on the null-conditional check (capability?.PrepareSupport == true) for the runtime case where the framework passes a null capability. This matches the OmniSharp interface signature exactly, avoiding any nullable-annotation mismatch (e.g. CS8765) while remaining null-safe during initialize. - Add explicit { false, false } theory cases for both handler kinds so the three distinct PrepareSupport inputs (omitted/null, true, false) are all covered and a regression that treats false as enabled would be caught. Verified: src and test projects build with 0 warnings under #nullable enable; all 6 GetRegistrationOptionsReflectsPrepareSupport cases pass on net462 and net8.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Services/TextDocument/Handlers/RenameHandler.cs | 8 ++++---- .../Refactoring/RenameHandlerTests.cs | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/PowerShellEditorServices/Services/TextDocument/Handlers/RenameHandler.cs b/src/PowerShellEditorServices/Services/TextDocument/Handlers/RenameHandler.cs index 9cfebd5e1..5d5ed7894 100644 --- a/src/PowerShellEditorServices/Services/TextDocument/Handlers/RenameHandler.cs +++ b/src/PowerShellEditorServices/Services/TextDocument/Handlers/RenameHandler.cs @@ -20,7 +20,7 @@ internal class PrepareRenameHandler RenameService renameService ) : IPrepareRenameHandler { - public RenameRegistrationOptions GetRegistrationOptions(RenameCapability? capability, ClientCapabilities clientCapabilities) => capability?.PrepareSupport == true ? new() { PrepareProvider = true } : new(); + public RenameRegistrationOptions GetRegistrationOptions(RenameCapability capability, ClientCapabilities clientCapabilities) => capability?.PrepareSupport == true ? new() { PrepareProvider = true } : new(); public async Task Handle(PrepareRenameParams request, CancellationToken cancellationToken) => await renameService.PrepareRenameSymbol(request, cancellationToken).ConfigureAwait(false); @@ -34,9 +34,9 @@ RenameService renameService ) : IRenameHandler { // RenameOptions may only be specified if the client states that it supports prepareSupport in its initial initialize request. - // Passes a null capability when the client omits textDocument.rename from its advertised capabilities (e.g. a completion-only client). - // Use the null-conditional operator so we don't throw NullReferenceException during initialize. - public RenameRegistrationOptions GetRegistrationOptions(RenameCapability? capability, ClientCapabilities clientCapabilities) => capability?.PrepareSupport == true ? new() { PrepareProvider = true } : new(); + // The framework passes a null capability when the client omits textDocument.rename from its advertised capabilities (e.g. a completion-only client). + // The parameter keeps the interface's non-nullable signature; the null-conditional operator avoids a NullReferenceException during initialize. + public RenameRegistrationOptions GetRegistrationOptions(RenameCapability capability, ClientCapabilities clientCapabilities) => capability?.PrepareSupport == true ? new() { PrepareProvider = true } : new(); public async Task Handle(RenameParams request, CancellationToken cancellationToken) => await renameService.RenameSymbol(request, cancellationToken).ConfigureAwait(false); diff --git a/test/PowerShellEditorServices.Test/Refactoring/RenameHandlerTests.cs b/test/PowerShellEditorServices.Test/Refactoring/RenameHandlerTests.cs index 8dfca4271..30cc75144 100644 --- a/test/PowerShellEditorServices.Test/Refactoring/RenameHandlerTests.cs +++ b/test/PowerShellEditorServices.Test/Refactoring/RenameHandlerTests.cs @@ -134,12 +134,15 @@ public enum RegistrationHandlerKind PrepareRename } - // A null prepareSupport represents the client omitting the capability entirely (framework hands us null). + // prepareSupport has three distinct inputs: null = client omitted the capability entirely (framework hands us null), + // true = client supports prepareRename, false = client explicitly does not. Only true should enable PrepareProvider. public static TheoryData RegistrationOptionsTestCases() => new() { { RegistrationHandlerKind.Rename, null, false }, + { RegistrationHandlerKind.Rename, false, false }, { RegistrationHandlerKind.Rename, true, true }, { RegistrationHandlerKind.PrepareRename, null, false }, + { RegistrationHandlerKind.PrepareRename, false, false }, { RegistrationHandlerKind.PrepareRename, true, true } };