From e921f3e7278d55e91a79fbbb874159c9b0125e42 Mon Sep 17 00:00:00 2001 From: Howard van Rooijen Date: Fri, 23 Jan 2026 16:50:51 +0000 Subject: [PATCH 1/3] fix: Issue 306 - handle exceptions in ProgressReporter.Begin and add verification tests --- .../Lsp/ProgressReporter.fs | 20 ++++-- .../CSharpLanguageServer.Tests.fsproj | 1 + .../ProgressReporterTests.fs | 68 +++++++++++++++++++ 3 files changed, 82 insertions(+), 7 deletions(-) create mode 100644 tests/CSharpLanguageServer.Tests/ProgressReporterTests.fs diff --git a/src/CSharpLanguageServer/Lsp/ProgressReporter.fs b/src/CSharpLanguageServer/Lsp/ProgressReporter.fs index d475c15d..55b77004 100644 --- a/src/CSharpLanguageServer/Lsp/ProgressReporter.fs +++ b/src/CSharpLanguageServer/Lsp/ProgressReporter.fs @@ -13,13 +13,19 @@ type ProgressReporter(client: ILspClient) = member val Token = ProgressToken.C2(Guid.NewGuid().ToString()) member this.Begin(title, ?cancellable, ?message, ?percentage) = async { - let! progressCreateResult = client.WindowWorkDoneProgressCreate { Token = this.Token } - - match progressCreateResult with - | Error _ -> canReport <- false - | Ok() -> - canReport <- true - + let! createSucceeded = async { + try + match! client.WindowWorkDoneProgressCreate { Token = this.Token } with + | Ok() -> return true + | Error _ -> return false + with _ -> + // Client does not support window/workDoneProgress/create + return false + } + + canReport <- createSucceeded + + if canReport then let param = WorkDoneProgressBegin.Create( title = title, diff --git a/tests/CSharpLanguageServer.Tests/CSharpLanguageServer.Tests.fsproj b/tests/CSharpLanguageServer.Tests/CSharpLanguageServer.Tests.fsproj index b949e463..58a345fc 100644 --- a/tests/CSharpLanguageServer.Tests/CSharpLanguageServer.Tests.fsproj +++ b/tests/CSharpLanguageServer.Tests/CSharpLanguageServer.Tests.fsproj @@ -28,6 +28,7 @@ + diff --git a/tests/CSharpLanguageServer.Tests/ProgressReporterTests.fs b/tests/CSharpLanguageServer.Tests/ProgressReporterTests.fs new file mode 100644 index 00000000..f49d5c80 --- /dev/null +++ b/tests/CSharpLanguageServer.Tests/ProgressReporterTests.fs @@ -0,0 +1,68 @@ +module CSharpLanguageServer.Tests.ProgressReporterTests + +open System +open NUnit.Framework +open Ionide.LanguageServerProtocol +open Ionide.LanguageServerProtocol.Types +open Ionide.LanguageServerProtocol.JsonRpc +open CSharpLanguageServer.Lsp + +/// A test stub that throws an exception when WindowWorkDoneProgressCreate is called, +/// simulating a client that doesn't support this method. +type ThrowingLspClientStub() = + interface System.IDisposable with + member _.Dispose() = () + + interface ILspClient with + member _.WindowWorkDoneProgressCreate(_: WorkDoneProgressCreateParams) = + async { return raise (Exception("Method not supported")) } + + member _.Progress(_: ProgressParams) = async { return () } + + // Other required interface members return default values + member _.WindowShowMessage(_) = async { return () } + member _.WindowLogMessage(_) = async { return () } + member _.TelemetryEvent(_) = async { return () } + member _.TextDocumentPublishDiagnostics(_) = async { return () } + member _.LogTrace(_) = async { return () } + member _.CancelRequest(_) = async { return () } + member _.WorkspaceWorkspaceFolders() = async { return LspResult.Ok None } + member _.WorkspaceConfiguration(_) = async { return LspResult.Ok [||] } + member _.WorkspaceSemanticTokensRefresh() = async { return LspResult.Ok() } + member _.WindowShowDocument(_) = async { return LspResult.Ok { Success = false } } + member _.WorkspaceInlineValueRefresh() = async { return LspResult.Ok() } + member _.WorkspaceInlayHintRefresh() = async { return LspResult.Ok() } + member _.WorkspaceDiagnosticRefresh() = async { return LspResult.Ok() } + member _.ClientRegisterCapability(_) = async { return LspResult.Ok() } + member _.ClientUnregisterCapability(_) = async { return LspResult.Ok() } + member _.WindowShowMessageRequest(_) = async { return LspResult.Ok None } + member _.WorkspaceCodeLensRefresh() = async { return LspResult.Ok() } + member _.WorkspaceApplyEdit(_) = async { + return LspResult.Ok { Applied = false; FailureReason = None; FailedChange = None } + } + +[] +let ``ProgressReporter Begin does not throw when client throws exception`` () = + let client = new ThrowingLspClientStub() + let reporter = new ProgressReporter(client) + + // Should not throw - exception should be caught internally + Assert.DoesNotThrowAsync(fun () -> + reporter.Begin("Test Title") |> Async.StartAsTask :> System.Threading.Tasks.Task + ) + +[] +let ``ProgressReporter Report and End are no-ops after failed Begin`` () = + let client = new ThrowingLspClientStub() + let reporter = new ProgressReporter(client) + + // Begin with a throwing client + reporter.Begin("Test Title") |> Async.RunSynchronously + + // Report and End should not throw (they should be no-ops since canReport is false) + Assert.DoesNotThrowAsync(fun () -> + reporter.Report(message = "Progress") |> Async.StartAsTask :> System.Threading.Tasks.Task + ) + Assert.DoesNotThrowAsync(fun () -> + reporter.End(message = "Done") |> Async.StartAsTask :> System.Threading.Tasks.Task + ) From 74828992487753adb89f743b9abc56f684c411c4 Mon Sep 17 00:00:00 2001 From: Howard van Rooijen Date: Fri, 23 Jan 2026 17:26:43 +0000 Subject: [PATCH 2/3] Summary of Changes 1. Removed unsafe .Value call (line 455) - The original code accessed wf.Value before the match to check if wf was Some. This caused crashes when the workspace folder wasn't found. 2. Moved wfPathToUri into the Some wf, Some doc branch (line 510) - The calculation now only happens when we're certain wf is Some. 3. Added .cshtml extension check (line 460) - The Some wf, None branch now only processes files as Razor/cshtml if they actually have a .cshtml extension, instead of treating ALL unfound documents as cshtml files. 4. Safe handling of wf.Solution (lines 461-466) - Instead of the unsafe wf.Solution.Value, the code now pattern matches on wf.Solution to safely handle the case where the solution hasn't loaded yet. 5. Added retry logic for unfound documents (lines 464-465, 504-505) - When a document isn't found (due to timing or solution not being fully loaded), the code now posts PushDiagnosticsDocumentBacklogUpdate and PushDiagnosticsProcessPendingDocuments to rebuild the backlog and retry later. 6. Added continuation for unfound .cshtml documents (line 495) - When a .cshtml document isn't found as a Razor document, processing continues with the next document. All 67 tests pass, including the two previously failing tests: - testPushDiagnosticsWork - testDiagnoseCommandWorks --- src/CSharpLanguageServer/State/ServerState.fs | 75 ++++++++++++------- 1 file changed, 46 insertions(+), 29 deletions(-) diff --git a/src/CSharpLanguageServer/State/ServerState.fs b/src/CSharpLanguageServer/State/ServerState.fs index b5835cb2..1bd35566 100644 --- a/src/CSharpLanguageServer/State/ServerState.fs +++ b/src/CSharpLanguageServer/State/ServerState.fs @@ -452,46 +452,63 @@ let processServerEvent (logger: ILogger) state postSelf msg : Async PushDiagnosticsDocumentBacklog = newBacklog } let wf, docForUri = docUri |> workspaceDocument state.Workspace AnyDocument - let wfPathToUri = workspaceFolderPathToUri wf.Value match wf, docForUri with | Some wf, None -> - let cshtmlPath = workspaceFolderUriToPath wf docUri |> _.Value - - match! solutionGetRazorDocumentForPath wf.Solution.Value cshtmlPath with - | Some(_, compilation, cshtmlTree) -> - let semanticModelMaybe = compilation.GetSemanticModel cshtmlTree |> Option.ofObj - - match semanticModelMaybe with + // Only try to process as a .cshtml file if it actually is one + match workspaceFolderUriToPath wf docUri with + | Some cshtmlPath when cshtmlPath.EndsWith(".cshtml", StringComparison.OrdinalIgnoreCase) -> + match wf.Solution with | None -> - Error(Exception "could not GetSemanticModelAsync") - |> PushDiagnosticsDocumentDiagnosticsResolution - |> postSelf - - | Some semanticModel -> - let diagnostics = - semanticModel.GetDiagnostics() - |> Seq.map (Diagnostic.fromRoslynDiagnostic (workspaceFolderPathToUri wf)) - |> Seq.filter (fun (_, uri) -> uri = docUri) - |> Seq.map fst - |> Array.ofSeq - - Ok(docUri, None, diagnostics) - |> PushDiagnosticsDocumentDiagnosticsResolution - |> postSelf - - | None -> - // could not find document for this enqueued uri + // Solution not loaded yet, rebuild backlog and try again later + postSelf PushDiagnosticsDocumentBacklogUpdate + postSelf PushDiagnosticsProcessPendingDocuments + | Some solution -> + match! solutionGetRazorDocumentForPath solution cshtmlPath with + | Some(_, compilation, cshtmlTree) -> + let semanticModelMaybe = compilation.GetSemanticModel cshtmlTree |> Option.ofObj + + match semanticModelMaybe with + | None -> + Error(Exception "could not GetSemanticModelAsync") + |> PushDiagnosticsDocumentDiagnosticsResolution + |> postSelf + + | Some semanticModel -> + let diagnostics = + semanticModel.GetDiagnostics() + |> Seq.map (Diagnostic.fromRoslynDiagnostic (workspaceFolderPathToUri wf)) + |> Seq.filter (fun (_, uri) -> uri = docUri) + |> Seq.map fst + |> Array.ofSeq + + Ok(docUri, None, diagnostics) + |> PushDiagnosticsDocumentDiagnosticsResolution + |> postSelf + + | None -> + logger.LogDebug( + "PushDiagnosticsProcessPendingDocuments: could not find razor document for \"{cshtmlPath}\"", + cshtmlPath + ) + // Continue with next document + postSelf PushDiagnosticsProcessPendingDocuments + + | _ -> + // Not a .cshtml file or couldn't convert URI + // This can happen if solution hasn't loaded yet - rebuild backlog for retry logger.LogDebug( - "PushDiagnosticsProcessPendingDocuments: could not find document w/ uri \"{docUri}\"", + "PushDiagnosticsProcessPendingDocuments: could not find document w/ uri \"{docUri}\", will retry", string docUri ) - - () + postSelf PushDiagnosticsDocumentBacklogUpdate + postSelf PushDiagnosticsProcessPendingDocuments return newState | Some wf, Some doc -> + let wfPathToUri = workspaceFolderPathToUri wf + let resolveDocumentDiagnostics () : Task = task { let! semanticModelMaybe = doc.GetSemanticModelAsync() From e0fd4071a8529b903f7686e6e27435658a63f84f Mon Sep 17 00:00:00 2001 From: Howard van Rooijen Date: Fri, 23 Jan 2026 22:03:14 +0000 Subject: [PATCH 3/3] fix: Respect client window.workDoneProgress capability for progress reporting Checks the client's window.workDoneProgress capability before attempting to create progress tokens. If the capability is not advertised or is false, progress reporting is silently skipped. Changes: - ProgressReporter.fs: Add ClientCapabilities parameter to constructor; check Window.WorkDoneProgress capability before calling WindowWorkDoneProgressCreate; remove try-catch exception handling - Workspace.fs: Pass ClientCapabilities to ProgressReporter - ServerState.fs: Pass state.ClientCapabilities to workspace loading - Diagnostics.fs: Pass emptyClientCapabilities to ProgressReporter - Tooling.fs: Add Window.WorkDoneProgress = true to test client capabilities - ProgressReporterTests.fs: Replace exception-based tests with capability-based tests that verify WindowWorkDoneProgressCreate is only called when supported --- src/CSharpLanguageServer/Diagnostics.fs | 2 +- .../Lsp/ProgressReporter.fs | 53 +++++++------- src/CSharpLanguageServer/Lsp/Workspace.fs | 4 +- src/CSharpLanguageServer/State/ServerState.fs | 2 +- .../ProgressReporterTests.fs | 71 ++++++++++++++----- tests/CSharpLanguageServer.Tests/Tooling.fs | 5 ++ 6 files changed, 89 insertions(+), 48 deletions(-) diff --git a/src/CSharpLanguageServer/Diagnostics.fs b/src/CSharpLanguageServer/Diagnostics.fs index c0b7495a..e552d76c 100644 --- a/src/CSharpLanguageServer/Diagnostics.fs +++ b/src/CSharpLanguageServer/Diagnostics.fs @@ -65,7 +65,7 @@ module Diagnostics = let lspClient = new LspClientStub() let cwd = string (Directory.GetCurrentDirectory()) - let progressReporter = ProgressReporter lspClient + let progressReporter = ProgressReporter(lspClient, emptyClientCapabilities) let! _sln = solutionLoadSolutionWithPathOrOnDir lspClient progressReporter None cwd logger.LogDebug("diagnose: done") diff --git a/src/CSharpLanguageServer/Lsp/ProgressReporter.fs b/src/CSharpLanguageServer/Lsp/ProgressReporter.fs index 55b77004..061def95 100644 --- a/src/CSharpLanguageServer/Lsp/ProgressReporter.fs +++ b/src/CSharpLanguageServer/Lsp/ProgressReporter.fs @@ -5,39 +5,40 @@ open Ionide.LanguageServerProtocol open Ionide.LanguageServerProtocol.Server open Ionide.LanguageServerProtocol.Types -type ProgressReporter(client: ILspClient) = +type ProgressReporter(client: ILspClient, clientCapabilities: ClientCapabilities) = let mutable canReport = false - let mutable endSent = false + let workDoneProgressSupported = + clientCapabilities.Window + |> Option.bind _.WorkDoneProgress + |> Option.defaultValue false + member val Token = ProgressToken.C2(Guid.NewGuid().ToString()) member this.Begin(title, ?cancellable, ?message, ?percentage) = async { - let! createSucceeded = async { - try - match! client.WindowWorkDoneProgressCreate { Token = this.Token } with - | Ok() -> return true - | Error _ -> return false - with _ -> - // Client does not support window/workDoneProgress/create - return false - } - - canReport <- createSucceeded - - if canReport then - let param = - WorkDoneProgressBegin.Create( - title = title, - ?cancellable = cancellable, - ?message = message, - ?percentage = percentage - ) + if not workDoneProgressSupported then + canReport <- false + else + let! progressCreateResult = client.WindowWorkDoneProgressCreate { Token = this.Token } - do! - client.Progress - { Token = this.Token - Value = serialize param } + match progressCreateResult with + | Error _ -> canReport <- false + | Ok() -> + canReport <- true + + let param = + WorkDoneProgressBegin.Create( + title = title, + ?cancellable = cancellable, + ?message = message, + ?percentage = percentage + ) + + do! + client.Progress + { Token = this.Token + Value = serialize param } } member this.Report(?cancellable, ?message, ?percentage) = async { diff --git a/src/CSharpLanguageServer/Lsp/Workspace.fs b/src/CSharpLanguageServer/Lsp/Workspace.fs index ec774fdb..a47fc41a 100644 --- a/src/CSharpLanguageServer/Lsp/Workspace.fs +++ b/src/CSharpLanguageServer/Lsp/Workspace.fs @@ -421,8 +421,8 @@ let workspaceDocumentSymbol let workspaceDocumentVersion workspace uri = uri |> workspace.OpenDocs.TryFind |> Option.map _.Version -let workspaceWithSolutionsLoaded (settings: ServerSettings) (lspClient: ILspClient) workspace = async { - let progressReporter = ProgressReporter lspClient +let workspaceWithSolutionsLoaded (settings: ServerSettings) (lspClient: ILspClient) (clientCapabilities: ClientCapabilities) workspace = async { + let progressReporter = ProgressReporter(lspClient, clientCapabilities) let beginMessage = sprintf "Loading workspace (%d workspace folders)" workspace.Folders.Length diff --git a/src/CSharpLanguageServer/State/ServerState.fs b/src/CSharpLanguageServer/State/ServerState.fs index 1bd35566..929dce63 100644 --- a/src/CSharpLanguageServer/State/ServerState.fs +++ b/src/CSharpLanguageServer/State/ServerState.fs @@ -583,7 +583,7 @@ let processServerEvent (logger: ILogger) state postSelf msg : Async match solutionReloadDeadline < DateTime.Now with | true -> - let! updatedWorkspace = workspaceWithSolutionsLoaded state.Settings state.LspClient.Value state.Workspace + let! updatedWorkspace = workspaceWithSolutionsLoaded state.Settings state.LspClient.Value state.ClientCapabilities state.Workspace return { state with diff --git a/tests/CSharpLanguageServer.Tests/ProgressReporterTests.fs b/tests/CSharpLanguageServer.Tests/ProgressReporterTests.fs index f49d5c80..4f050cc7 100644 --- a/tests/CSharpLanguageServer.Tests/ProgressReporterTests.fs +++ b/tests/CSharpLanguageServer.Tests/ProgressReporterTests.fs @@ -6,20 +6,23 @@ open Ionide.LanguageServerProtocol open Ionide.LanguageServerProtocol.Types open Ionide.LanguageServerProtocol.JsonRpc open CSharpLanguageServer.Lsp +open CSharpLanguageServer.Tests.Tooling + +/// Client stub that tracks whether WindowWorkDoneProgressCreate was called +type TrackingLspClientStub() = + let mutable createCalled = false + + member _.WasCreateCalled = createCalled -/// A test stub that throws an exception when WindowWorkDoneProgressCreate is called, -/// simulating a client that doesn't support this method. -type ThrowingLspClientStub() = interface System.IDisposable with member _.Dispose() = () interface ILspClient with member _.WindowWorkDoneProgressCreate(_: WorkDoneProgressCreateParams) = - async { return raise (Exception("Method not supported")) } + createCalled <- true + async { return LspResult.Ok() } member _.Progress(_: ProgressParams) = async { return () } - - // Other required interface members return default values member _.WindowShowMessage(_) = async { return () } member _.WindowLogMessage(_) = async { return () } member _.TelemetryEvent(_) = async { return () } @@ -41,25 +44,57 @@ type ThrowingLspClientStub() = return LspResult.Ok { Applied = false; FailureReason = None; FailedChange = None } } +let capabilitiesWithWorkDoneProgress (supported: bool): ClientCapabilities = + let windowCaps: WindowClientCapabilities = + { WorkDoneProgress = Some supported + ShowMessage = None + ShowDocument = None } + + { Workspace = None + TextDocument = None + NotebookDocument = None + Window = Some windowCaps + General = None + Experimental = None } + [] -let ``ProgressReporter Begin does not throw when client throws exception`` () = - let client = new ThrowingLspClientStub() - let reporter = new ProgressReporter(client) +let ``ProgressReporter does not call WindowWorkDoneProgressCreate when capability not supported`` () = + let client = new TrackingLspClientStub() + let reporter = new ProgressReporter(client, emptyClientCapabilities) - // Should not throw - exception should be caught internally - Assert.DoesNotThrowAsync(fun () -> - reporter.Begin("Test Title") |> Async.StartAsTask :> System.Threading.Tasks.Task - ) + reporter.Begin("Test Title") |> Async.RunSynchronously + + Assert.That(client.WasCreateCalled, Is.False, "WindowWorkDoneProgressCreate should not be called when capability is not supported") + +[] +let ``ProgressReporter does not call WindowWorkDoneProgressCreate when WorkDoneProgress is false`` () = + let client = new TrackingLspClientStub() + let caps = capabilitiesWithWorkDoneProgress false + let reporter = new ProgressReporter(client, caps) + + reporter.Begin("Test Title") |> Async.RunSynchronously + + Assert.That(client.WasCreateCalled, Is.False, "WindowWorkDoneProgressCreate should not be called when WorkDoneProgress is false") + +[] +let ``ProgressReporter calls WindowWorkDoneProgressCreate when capability is supported`` () = + let client = new TrackingLspClientStub() + let caps = capabilitiesWithWorkDoneProgress true + let reporter = new ProgressReporter(client, caps) + + reporter.Begin("Test Title") |> Async.RunSynchronously + + Assert.That(client.WasCreateCalled, Is.True, "WindowWorkDoneProgressCreate should be called when WorkDoneProgress is true") [] -let ``ProgressReporter Report and End are no-ops after failed Begin`` () = - let client = new ThrowingLspClientStub() - let reporter = new ProgressReporter(client) +let ``ProgressReporter Report and End are no-ops when capability not supported`` () = + let client = new TrackingLspClientStub() + let reporter = new ProgressReporter(client, emptyClientCapabilities) - // Begin with a throwing client + // Begin with unsupported capability reporter.Begin("Test Title") |> Async.RunSynchronously - // Report and End should not throw (they should be no-ops since canReport is false) + // Report and End should not throw Assert.DoesNotThrowAsync(fun () -> reporter.Report(message = "Progress") |> Async.StartAsTask :> System.Threading.Tasks.Task ) diff --git a/tests/CSharpLanguageServer.Tests/Tooling.fs b/tests/CSharpLanguageServer.Tests/Tooling.fs index af8f8dc2..882f7632 100644 --- a/tests/CSharpLanguageServer.Tests/Tooling.fs +++ b/tests/CSharpLanguageServer.Tests/Tooling.fs @@ -88,6 +88,11 @@ let defaultClientCapabilities = ResolveSupport = None HonorsChangeAnnotations = None CodeActionLiteralSupport = Some { CodeActionKind = { ValueSet = Array.empty } } } } + Window = + Some + { WorkDoneProgress = Some true + ShowMessage = None + ShowDocument = None } Experimental = {| csharp = {| metadataUris = true |} |} |> serialize |> Some } let defaultClientProfile =