Skip to content

feat(Async+Task+ValueTask): consistent helper modules#19844

Open
bartelink wants to merge 9 commits into
dotnet:mainfrom
bartelink:atvt
Open

feat(Async+Task+ValueTask): consistent helper modules#19844
bartelink wants to merge 9 commits into
dotnet:mainfrom
bartelink:atvt

Conversation

@bartelink

@bartelink bartelink commented May 28, 2026

Copy link
Copy Markdown

Adds consistent helper modules for Async, Task and ValueTask.

Resolves fsharp/fslang-suggestions#1466

Checklist

@github-actions

github-actions Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

✅ No release notes required

@bartelink bartelink force-pushed the atvt branch 2 times, most recently from 5850539 to 7ad7946 Compare May 28, 2026 14:34
@bartelink bartelink marked this pull request as ready for review May 28, 2026 15:25
@bartelink bartelink requested a review from a team as a code owner May 28, 2026 15:25
Copilot AI review requested due to automatic review settings May 28, 2026 15:25

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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds new camelCase helpers for Async, Task, and ValueTask in FSharp.Core, along with unit tests and surface area/release note updates.

Changes:

  • Introduced result, map, bind, ignore, catchWith, catch, empty for Async, Task, ValueTask (+ Task.ofValueTask, ValueTask.ofTask where available).
  • Added unit tests covering success/failure flows for the new helpers.
  • Updated netstandard surface area baselines and release notes.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/TaskModuleFunctions.fs New tests for Task/ValueTask camelCase helpers.
tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncModuleFunctions.fs New tests for Async camelCase helpers.
tests/FSharp.Core.UnitTests/FSharp.Core.UnitTests.fsproj Includes the new test files in the test project.
tests/FSharp.Core.UnitTests/FSharp.Core.SurfaceArea.netstandard21.release.bsl Surface area baseline updated for new APIs.
tests/FSharp.Core.UnitTests/FSharp.Core.SurfaceArea.netstandard21.debug.bsl Surface area baseline updated for new APIs.
tests/FSharp.Core.UnitTests/FSharp.Core.SurfaceArea.netstandard20.release.bsl Surface area baseline updated for new APIs.
tests/FSharp.Core.UnitTests/FSharp.Core.SurfaceArea.netstandard20.debug.bsl Surface area baseline updated for new APIs.
src/FSharp.Core/tasks.fsi Public signatures/docs for new Task/ValueTask modules.
src/FSharp.Core/tasks.fs Implementation of new Task/ValueTask helpers.
src/FSharp.Core/async.fsi Public signatures/docs for new Async camelCase helpers.
src/FSharp.Core/async.fs Implementation of new Async camelCase helpers.
docs/release-notes/.FSharp.Core/11.0.100.md Release notes entry for the new APIs.

Comment thread src/FSharp.Core/async.fsi Outdated
Comment thread src/FSharp.Core/tasks.fs Outdated
Comment thread src/FSharp.Core/tasks.fs

### Added

* Added camelCase module-level functions `result`, `map`, `bind`, `ignore`, `catchWith`, `catch`, and `empty` in `module`s `Async`,`Task` and `ValueTask`, plus `Task.ofValueTask` and `ValueTask.ofTask`. ([LanguageSuggestion #1466](https://github.com/fsharp/fslang-suggestions/issues/1466), [PR #19844](https://github.com/dotnet/fsharp/pull/19844))

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.

Still open: `module`smodules, plus comma-space → modules Async, Task, ValueTask``.

@bartelink bartelink Jul 13, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

reworded in 6708b8f to make format more consistent if there's any other functions added any time soon.

Comment thread src/FSharp.Core/async.fs
@github-actions github-actions Bot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label May 28, 2026
@T-Gro

T-Gro commented Jun 12, 2026

Copy link
Copy Markdown
Member

@bartelink :

Hi Ruben, just to let you know - I am waiting for when we stop flowing into .NET 10 releases (10.0.400).
Once we branch off to do NET11 only from main, I will have another look (but looks ready) at all your PRs and merge if no objections 👍 .

@bartelink

bartelink commented Jun 12, 2026

Copy link
Copy Markdown
Author

Thanks @T-Gro; will be ready when the times come.

Some open questions for when you have a minute to scan:

@bartelink

bartelink commented Jun 30, 2026

Copy link
Copy Markdown
Author

@T-Gro Two more notes

@T-Gro T-Gro left a comment

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.

Focused on the consistency premise across Async/Task/ValueTask. Two decisions already settled: module suffix (compiled TaskModule/ValueTaskModule) and cancellation propagates everywhere (first-class, not caught like a normal exception — applies to both catch and catchWith). Inline comments have repros + proposed fixes.

Comment thread src/FSharp.Core/tasks.fs
Comment thread src/FSharp.Core/tasks.fs
return handler e
}

[<CompiledName("Catch")>]

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.

catch demotes cancellation to Error — also ValueTask.catch (:860). Reverses the shipped Error(cancelled) test; decision is to treat cancellation as first-class, consistent with Async.

Task.FromCanceled<int>(CancellationToken true) |> Task.catch
  • result is Error (TaskCanceledException)
  • Async.catch propagates; Error should be reserved for genuine faults

Proposed fix:

TaskBuilder.task {
    try
        let! v = task
        return Ok v
    with
    | :? OperationCanceledException as e -> return raise e   // stays Canceled
    | e -> return Error e
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

(corrected; pushed for Task, but not yet for ValueTask)
Thanks for the catch, the proposed fix works (though I wonder if the stack trace is optimal and/or whether that's the canonical way to bail on cancellation)

@T-Gro I guess map, bind, catch, catchWith should each have xmldoc covering the pinned behavior? i.e. I'm thinking that it should allude to the fact that catch will let a TaskCanceledException escape so it's not 100% 1:1 equivalent to task { try let! r = task in Ok r with e -> Error e }

Comment thread src/FSharp.Core/tasks.fs
[<CompiledName("Map")>]
let inline map ([<InlineIfLambda>] mapping: 'T -> 'U) (task: Task<'T>) : Task<'U> =
if task.Status = TaskStatus.RanToCompletion then
result (mapping task.Result)

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.

map/bind throw synchronously on already-completed input — also bind (:751) and the ValueTask equivalents (:809/:822).

let boom (_: int) : int = failwith "boom"
tcs.Task       |> Task.map boom     // pending
Task.result 21 |> Task.map boom     // completed
  • pending input → faulted Task
  • completed input → raises at the call site, no Task returned
  • same call, exception delivered two different ways depending on timing

Proposed fix:

if task.Status = TaskStatus.RanToCompletion then
    try result (mapping task.Result)
    with e -> Task.FromException<'U> e

@bartelink bartelink Jul 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@T-Gro I get the point/concern on the catch variants - easy to test etc and will fix.

same call, exception delivered two different ways depending on timing

But can you clarify the exact desire re cancellation handling please?

This code will use the task { leg in the canceled or faulted case and passes my tests:

if task.Status = TaskStatus.RanToCompletion then
    result (mapping task.Result)
else
    task {
        let! v = task
        return mapping v
    }

Recall task.Status = TaskStatus.RanToCompletion is a ns2.0 compatible equivalent of .IsCompletedSuccessfully, so AIUI there can't be a throw from that leg, so the proposed fix would never hit its catch?

Changing Status = TaskStatus.RanToCompletion fast path check to to instead use IsCompleted to force a given handling would entail switching to:

if task.IsCompleted then // includes Canceled or Faulted states
     try result (task.GetAwaiter().GetResult() |> mapping) // Result would surface AggregateException
     with e -> Task.FromException<'U>(e)
else
    TaskBuilder.task {
            let! v = task
            return mapping v
        }

Current (passing with above impls) test semantics:

[<Fact>]
let ``Task.map flows Cancellation (sync)`` () =
    use cts = new CancellationTokenSource()
    cts.Cancel()
    let t = Task.FromCanceled<int>(cts.Token) |> Task.map (fun x -> x * 2)
    task {
        let! e = Assert.ThrowsAsync<TaskCanceledException>(fun () -> t)
        Assert.Equal(cts.Token, e.CancellationToken)
    }
    
[<Fact>]
let ``Task.map flows Cancellation (async)`` () =
    let tcs = TaskCompletionSource<int>()
    let t = tcs.Task |> Task.map (fun x -> x * 2)
    use cts = new CancellationTokenSource()
    tcs.SetCanceled cts.Token
    task {
        let! e = Assert.ThrowsAsync<TaskCanceledException>(fun () -> t)
        Assert.Equal(cts.Token, e.CancellationToken)
    }
    
[<Fact>]
let ``Task.map propagates exception (sync)`` () =
    let t = Task.FromException<int>(Exception "boom") |> Task.map (fun x -> x * 2)
    task {
        let! e = Assert.ThrowsAnyAsync<exn>(fun () -> t)
        Assert.Equal("boom", e.Message)
    }

[<Fact>]
let ``Task.map propagates exception (async)`` () =
    let tcs = TaskCompletionSource<int>()
    let t = tcs.Task |> Task.map (fun x -> x * 2)
    tcs.SetException(exn "boom")
    task {
        let! e = Assert.ThrowsAnyAsync<exn>(fun () -> t)
        Assert.Equal("boom", e.Message)
    }

How would one specify/validate the precise semantics you seek? Is it about being able to set a breakpoint?

Anything worth borrowing from prior art?:

https://github.com/fsprojects/FSharpPlus/blob/master/src/FSharpPlus/Extensions/Task.fs#L15-L28
https://github.com/fsprojects/FSharpPlus/blob/master/src/FSharpPlus/Extensions/Task.fs#L85-L96
https://github.com/demystifyfp/FsToolkit.ErrorHandling/blob/master/src/FsToolkit.ErrorHandling/Task.fs#L8-L36

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

cc @TheAngryByrd @gusty If either of you have time to throw a set of eyes over the impl and the test suite to see if there are any gaps that FsToolkit and/or FSharpPlus cover which should be considered?

https://github.com/bartelink/fsharp/blob/atvt/src/FSharp.Core/tasks.fs#L729-L804
https://github.com/bartelink/fsharp/blob/atvt/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/TaskModuleFunctions.fs#L11-L274

The aim is for a balance of:

  • idiomatic / terse impl (optimization can come later (though each function has a sync/completed fast path)
  • thorough test suite that provides coverage of all intended behaviors, no matter how esoteric (i.e. if this provides bad stack traces and/or usage of return raise e throws away stack traces and the test suite should call that out, I'm interested!

Bottom line it would be good to rule out footguns like egregious AggregateException wrapping or catch/catchWith trapping cancellation from the off

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@T-Gro Task test suite reviewed, expanded and polished
catch/catchWith Cancellation handling is corrected

NOTE ValueTask impl and tests are still unchanged - I'll port those when we're happy with Task.

Comment thread src/FSharp.Core/tasks.fsi

[<Fact>]
let ``ValueTask.map transforms value (async)`` () =
let vt = ValueTask<int>(Task.FromResult 21) |> ValueTask.map (fun x -> x * 2)

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.

"(async)" tests run the sync path — also bind (:203).

ValueTask<int>(Task.FromResult 21).IsCompletedSuccessfully   // true
  • input already completed → fast path; the slow (Task-allocating) branch is untested
  • also why the map/bind timing issue has no failing test

Proposed fix:

let tcs = TaskCompletionSource<int>()
let vt = ValueTask<int>(tcs.Task) |> ValueTask.map (fun x -> x * 2)
Assert.False vt.IsCompletedSuccessfully
tcs.SetResult 21
Assert.Equal(42, vt.Result)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Apologies for the lack of quality control - will review the suite and make Task vs ValueTask more consistent before forcing it on human eyes again ☹️
(ignore has the same issue, IsCompletedSuccessfully should be IsCompleted etc etc)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Comment thread src/FSharp.Core/async.fsi Outdated
/// let readFile filename numBytes =
/// async {
/// use file = System.IO.File.OpenRead(filename)
/// do! file.AsyncRead(numBytes) |> Async.ignore&lt;int&gt;

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.

Doc example doesn't type-check.

file.AsyncRead(numBytes) |> Async.ignore<int>
  • AsyncRead : int -> Async<byte[]>, so ignore<int>FS0001

Proposed fix:

file.AsyncRead(numBytes) |> Async.ignore<byte array>

@bartelink bartelink Jul 13, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fixed (but using byte[])

@T-Gro

T-Gro commented Jul 11, 2026

Copy link
Copy Markdown
Member

@bartelink I noticed I was owing a few replies here, I apologize for taking a longer time.

Any thoughts on whether other more fringe things can/should make it in e.g. Task.parallelLimit … extending the base impl to avoid redundant Task instances and/or canceling in-flight work once an exception is raised

Let's keep it out of this PR. It adds throttling + cancel-on-first-exception + aggregation and overlaps Parallel.ForEachAsync. The cancel-in-flight story (no clean BCL-idiomatic path, as you note) is exactly why it deserves its own treatment.

Wondering if you see the value/point in including a [Task|Async|ValueTask].wait/runSync … also addresses the ask for a syncTask … (#1459)

The ergonomics are real, but a blessed runSync/wait is a policy call (sync-over-async, deadlock footguns) that these pure combinators don't carry. Let's not fold it in here.

ICYMI I have a StartTaskImmediate stacked in the wings, awaiting Async.Await

Noted — happy to review once that lands.


I believe that clears everything I had outstanding — if I've missed a question anywhere in the thread, or any of the above needs more detail, flag it and I'll follow up.

@bartelink

bartelink commented Jul 14, 2026

Copy link
Copy Markdown
Author

Thanks for catches - this is ready for re-review from my perspective

Checklist:

  • port final Task impl and test suite to ValueTask
  • xmldoc re Cancellation semantics for map, bind, catch, catchWith ?
  • any follow-ups from @TheAngryByrd @gusty
  • followups from anyone else - please feel free to refer anyone with relevant expertise (and time!) by atting them in...
  • remove empty ?

Comment thread src/FSharp.Core/tasks.fs

[<CompiledName("Ignore")>]
[<RequiresExplicitTypeArguments>]
let inline ignore<'T> (task: Task<'T>) : Task<unit> =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
let inline ignore<'T> (task: Task<'T>) : Task<unit> =
let inline ignore (task: Task) : Task<unit> =

ignore doesn't accept Task (non generic).

The implementation in F#+ allows to convert a Task to a Task<unit> and I would say in 50% of the scenarios is used in this way. I think we already discussed this.

@bartelink bartelink Jul 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hm problem is that people want RequiresExplicitTypeArguments [and hence generic] here
also it has to be a module so no overloading
Would not want to do a one-off ugly name unless there are other cases...
Maybe naming it Task.ofTask (as the inital proposal suggested) would work
But I'm also betting that in the kind of codebase requiring heavy TPL interop this would be only the start of the conversion helpers that would be useful but deep in the long tail in terms of usage count?

(Best discussed in fslang-sugggestion)

Comment thread src/FSharp.Core/tasks.fs
}

[<CompiledName("Catch")>]
let catch (task: Task<'T>) : Task<Result<'T, exn>> =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we kind of agreed that this function shouldn't be included for the reasons mentioned in the discussion.

I'm not pretending to restart the discussions, but just looking at the votes. There are 3 "really don't want" against 1 "really want", plus 2 "I'm ok with it", there are more against that in favor.

@bartelink bartelink Jul 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

empty was more lukewarm and I'd venture higher priority to debate...
As my vote indicates, I'd personally prefer it not to be there, but there are people who think it's an important thing to have in the box... The place to debate this this is probably out in the fslang-suggestion though

Comment thread src/FSharp.Core/tasks.fs

[<CompiledName("Ignore")>]
[<RequiresExplicitTypeArguments>]
let inline ignore<'T> (task: ValueTask<'T>) : ValueTask<unit> =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same comment as for the Task version

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

Labels

AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

Async/Task/ValueTask standard functions

4 participants