Repository navigation
Improve background progress reporting - #1545
TheAngryByrd wants to merge 6 commits into
Conversation
…hared progress reporter
…g in typechecking
There was a problem hiding this comment.
🟡 Changes recommended
Cancellation propagation, configuration handling, duplicate-operation tracking, and one test assertion are currently incorrect.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces shared LSP progress reporting for typechecking, analyzers, project loading, and dependent-file checks.
Changes:
- Adds shared, cancellable batch progress infrastructure.
- Integrates reporters into server workflows.
- Adds concurrency helpers and reporter tests.
File summaries
| File | Description |
|---|---|
FSharpLspClient.fs |
Implements shared progress reporting. |
FSharpLspClient.fsi |
Exposes the reporter API. |
AdaptiveServerState.fs |
Integrates shared progress and cancellation. |
Utils.fs |
Adds percentage-based parallelism helpers. |
Utils.fsi |
Declares new parallelism helpers. |
SharedTypecheckProgressReporterTests.fs |
Tests reporter behavior. |
Program.fs |
Registers the new tests. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 8
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let simpleName = IO.Path.GetFileName fileName | ||
| activeFiles <- Set.add simpleName activeFiles |
| let projectLoadingProgressReporter = | ||
| new SharedTypecheckProgressReporter("Loading Projects", fun () -> progressLookup.CreateProgressReport(lspClient)) |
| opts, | ||
| shouldCache = shouldCache | ||
| ) | ||
| |> Async.withCancellation progressCt |
| fileToken, | ||
| progressReporter.CancellationToken | ||
| ) | ||
| CancellationTokenSource.CreateLinkedTokenSource(rootToken, fileToken, progressCt) |
|
|
||
| if config.Notifications.BackgroundServiceProgress then | ||
| do! progress.Begin($"Checking for unnecessary parentheses {fileName}...", message = filePathUntag) | ||
| use! _progress = analyzerProgressReporter.Begin($"Unnecessary parens - {fileName}") |
| // Second batch — counters should have been reset | ||
| let! batchDisp2 = | ||
| reporter.BeginBatch ([| "C:/src/C.fs"; "C:/src/D.fs" |]) CancellationToken.None | ||
| |> Async.AwaitTask | ||
|
|
||
| do! settle () | ||
|
|
||
| // The Begin for second batch should show 0% (0/2) | ||
| let callsArr = calls.ToArray() | ||
|
|
||
| let hasZeroPercentage = | ||
| callsArr | ||
| |> Array.exists (fun c -> | ||
| match c with | ||
| | Begin(_, _, Some 0u) -> true | ||
| | Report(_, Some 0u) -> true | ||
| | _ -> false) |
| Expect.isTrue (ct = CancellationToken.None) "CancellationToken should be None after all files end" | ||
| } | ||
|
|
||
| testCaseAsync "Multiple sequential files reuse the same report cycle" |
| let normalizedFiles = files |> Array.map normalizePath |> Array.distinct | ||
| batchTotal <- batchTotal + normalizedFiles.Length | ||
| batchFiles <- Set.union batchFiles (normalizedFiles |> Set.ofArray) |
69e74d3 to
d7e1661
Compare
nojaf
left a comment
nojaf
left a comment
There was a problem hiding this comment.
Thanks for this! Folding the per-file "Typechecking X" / "Checking unused opens X" notifications into one progress item is a real UX improvement, and it also saves the window/workDoneProgress/create round-trip per typecheck. I'd like to get it in, but a few things need another look first.
Behaviour changes
1. Most users would no longer see typecheck and project-load progress.
On main, "Typechecking Foo.fs", "Typechecking Dependent F# files" and "Loading N Projects" are always reported. This PR gates all three behind BackgroundServiceProgress, which defaults to false (LspHelpers.fs), and Ionide doesn't expose FSharp.notifications.backgroundServiceProgress in its settings. So for nearly every VS Code user, project loading and typechecking would go silent. If that's intentional, could you call it out in the description and changelog? Personally I'd keep at least "Loading Projects" always on.
2. Dependent-file checks go from parallel75 to parallel25.
On a 4-core machine that means one file at a time, so typechecking dependents after a save gets roughly 3x slower. This feels unrelated to progress reporting. Could it move to its own PR with some numbers behind it? parallel50 is also added but unused.
3. Cancelling now affects every operation, and keeps affecting new ones for a while.
Because the report is shared, the client's cancel button now cancels every typecheck in flight, including the one for the file being edited. Before, it only cancelled that one file. The cancelled report also stays in progressReport until all scopes are done, so any typecheck that starts in that window gets an already-cancelled token from GetCancellationToken() and fails straight away. Maybe Begin could drop a report whose CTS has fired and create a fresh one?
4. The project-loading percentage doesn't move.
BeginBatchSync starts every project up front and ends them all in Dispose, so the user sees "0/N completed" with a list of .fsproj names for the whole load, followed by a jump to the end. Real progress would need per-project events from the loader (loader.Notifications). Otherwise I'd keep the old "Loading N Projects" message without a percentage.
Smaller things
- Cancellation now escapes from the dependent-file check. With the new
whenfilter, cancellation ofrootToken/progressCtescapesbypassAdaptiveAndCheckDependenciesForFile, andSaveDocumentawaits that. A quick second save or edit now ends the save handler with an exception thatmainswallowed. Worth checking that it gets logged quietly rather than as an error. - The lock is held during a client round-trip.
StartFileholds the lock whilereport.Beginwaits for the client's response toworkDoneProgress/create, so other typechecks queue behind it. It only matters when the setting is on, but it's on the hot path. GetCancellationToken()takes the lock even when reporting is disabled (the default), on every typecheck. It could returnCancellationToken.Noneearly whenisEnabled()is false.BeginBatchSyncdoesn't await its tasks. TheStartFile/EndFile/EndBatchtasks are discarded, so any exceptions (e.g.ObjectDisposedExceptionat shutdown) go unobserved. It also sends 2N$/progressmessages for N projects.
Nitpicks
- The name is too narrow.
SharedTypecheckProgressReporteris also used for analyzers and project loading.SharedProgressReporterwould be more accurate. - Labels are passed where paths are expected.
Begin("Unused opens - Foo.fs")passes a label throughPath.GetFullPath/GetFileName, which only works by accident. A separate label parameter, or a non-path key, would be cleaner. - The
Utils.fschange dropped a doc comment. Theparallel75doc comment is gone (the.fsicopy stays), and there's a stray extra blank line. - The test mock parses JSON by hand. Its
IndexOf/Substringcode could be replaced withp.Value.["kind"],p.Value.["message"]etc., sincep.Valueis already aJToken.
Summary
FSharp.notifications.backgroundServiceProgressfor all shared progress reportersTesting
dotnet fantomas --check build.fsx srcdotnet build test/FsAutoComplete.Tests.Lsp/FsAutoComplete.Tests.Lsp.fsproj -c Release -f net10.0 -p:ImportDirectoryPackagesProps=false --nologodotnet run -c Release -f net10.0 --no-build --project test/FsAutoComplete.Tests.Lsp/FsAutoComplete.Tests.Lsp.fsproj -- --filter-test-list "SharedTypecheckProgressReporter"— 17 passedThe full solution build reached the changed projects, but nested TestExplorer sample builds imported a parent
Directory.Packages.propsand failed withNU1008. The focused LSP build completed with zero warnings and errors.