Skip to content

refactor(CancellableTasks): Delegate to Task.sequential/parallelLimit - #20695

Open
bartelink wants to merge 3 commits into
dotnet:mainfrom
bartelink:apply-parallel-limit
Open

bartelink wants to merge 3 commits into
dotnet:mainfrom
bartelink:apply-parallel-limit

Conversation

@bartelink

@bartelink bartelink commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Follow up to #20128 as noted in #20128 (comment) - Replace bodies of CancellableTask.whenAllThrottled and sequential with Task.parallelLimit and sequential

Copilot AI balanced review requested due to automatic review settings October 3, 2026 23:09
@bartelink
bartelink requested a review from a team as a code owner October 3, 2026 23:09
@bartelink bartelink changed the title refactor(CancellableTasks): Apply Task.sequential/parallelLimit refactor(CancellableTasks): Delegate to Task.sequential/parallelLimit Oct 3, 2026
@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

❗ Release notes required

You can open this PR in browser to add release notes: open in github.dev

@bartelink,

Caution

No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

`* . (PR #XXXXX)`

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

Change path Release notes path Description
`src/FSharp.Core` docs/release-notes/.FSharp.Core/11.0.200.md No release notes found or release notes format is not correct

✅ Found changes and release notes in following paths:

Change path Release notes path Description
`vsintegration/src` docs/release-notes/.VisualStudio/18.vNext.md

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The sequential delegation can continue queued work after cancellation.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Refactors editor cancellable-task helpers to reuse FSharp.Core task combinators.

Changes:

  • Delegates throttled parallel and sequential execution to Task helpers.
  • Adds the follow-up PR to Visual Studio release notes.
File Description
CancellableTasks.fs Replaces custom scheduling implementations.
18.vNext.md Adds the follow-up PR reference.

Comment thread vsintegration/src/FSharp.Editor/Common/CancellableTasks.fs Outdated
@@ -1104,32 +1104,7 @@ module CancellableTasks =
let inline whenAllThrottled maxDegreeOfParallelism (tasks: CancellableTask<'a> seq) =
cancellableTask {

@bartelink bartelink Oct 4, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could collapse this to fun ct -> Task.parallelLimit maxDegreeOfParallelism ct tasks (and same for sequential); no idea if it desugars to the same ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes pls collapse it, lets use the FSharp.Core variants where possible.

@bartelink bartelink Oct 5, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a call each of whenAllThrottled and sequential at present (neither has a ct locally so it makes sense for them to remain using it indirectly via the CancellableTask module)
I've removed the wrapping within the body to make them a one-liner.
Will eventually get IcedTasks to match it as part of the demystifyfp/FsToolkit.ErrorHandling#373 work

@bartelink
bartelink force-pushed the apply-parallel-limit branch 9 times, most recently from b2b5901 to cfe0ac4 Compare October 5, 2026 00:16
@bartelink
bartelink force-pushed the apply-parallel-limit branch from 5ac18b9 to dfe59e9 Compare October 5, 2026 08:53

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

3 participants