fsharp / fslang-suggestions

The place to make suggestions, discuss and vote on F# language and core library features
346 stars 21 forks source link

Add `Async.AwaitTask` overloads which helps with CancellationToken passing, and a new warning #1284

Open jwosty opened 1 year ago

jwosty commented 1 year ago

I propose we add two Async.AwaitTask overloads:

I also propose that an accompanying compiler warning be added. In situations where the task method could have been provided a CancellationToken (either via an optional parameter or a separate overload), the compiler should emit a warning.

The warning could say something like: "This task-returning method can accept a CancellationToken (through an overload or optional parameter), but is not provided one. Please either use Async.AwaitTask(makeTask: CancellationToken -> Task<'a>), or capture and provide a CancellationToken manually."

Surely there are better wordings for such a warning; improvements are welcome.

For example, the following, which forgets to pass a CancellationToken, would now emit a warning:

let doWorkBad () = async {
    do! Async.AwaitTask (File.WriteAllTextAsync ("file.txt", "Hello, world!"))
}

To make the warning go away, you could use the new overload:

let doWorkGood () = async {
    do! Async.AwaitTask (fun ct -> File.WriteAllTextAsync ("file.txt", "Hello, world!", ct))
}

Or capture and pass it yourself:

let doWork () = async {
    let! ct = Async.CancellationToken
    do! Async.AwaitTask (File.WriteAllTextAsync ("file.txt", "Hello, world!", ct))
}

The existing way of approaching this problem in F# is ...

For the new method, you can write your own extensions today:

type Async =
    static member AwaitTask (makeTask: CancellationToken -> Task<'a>) = async {
        let! ct = Async.CancellationToken
        let! result = Async.AwaitTask (makeTask ct)
        return result
    }
    static member AwaitTask (makeTask: CancellationToken -> Task) = async {
        let! ct = Async.CancellationToken
        let! result = Async.AwaitTask (makeTask ct)
        return result
    }

Pros and Cons

The advantages of making this adjustment to F# are:

The disadvantages of making this adjustment to F# are:

Extra information

Estimated cost (XS, S, M, L, XL, XXL): S

Related suggestions: (put links to related suggestions here)

Affidavit (please submit!)

Please tick this by placing a cross in the box:

Please tick all that apply:

For Readers

If you would like to see this issue implemented, please click the :+1: emoji on this issue. These counts are used to generally order the suggestions by engagement.

vzarytovskii commented 1 year ago

I personally don't think that emitting warning from compiler is a good idea, it makes it aware of a very specific type as well as twisting the promise of optional parameters (i.e. Optional parameters are optional, except CancellationToken).

This, however, should be a great analyzer for when we will be working on analyzers sdk.

jwosty commented 1 year ago

I personally don't think that emitting warning from compiler is a good idea, it makes it aware of a very specific type as well as twisting the promise of optional parameters (i.e. Optional parameters are optional, except CancellationToken).

This, however, should be a great analyzer for when we will be working on analyzers sdk.

What is your opinion of just the additional overloads? Those could be added without the warning.

bartelink commented 1 year ago

TL;DR the Async.AwaitTask semantics are problematic, and adding more operations that implicitly couple to those semantics is therefore something that should not proceed until that's resolved.


I arrived at the same thing, with the name Async.call: https://github.com/jet/propulsion/blob/master/src/Propulsion/Internal.fs#L102-L107 https://github.com/jet/equinox/blob/master/src/Equinox.Core/Infrastructure.fs#L54-L58

I don't love the name call, but on balance, I feel it's bad news to overload the AwaitTask name, especially given its problematic semantics. For me, the best way to resolve the forces is to first get corrected AwaitTask operators (as described in https://github.com/fsprojects/FSharp.Control.TaskSeq/issues/141) into a library before doing this work. Because the semantics differ from the AwaitTask ones, it will likely end up with a new name. Then, when that's done, the 'run a Task, passing the CT' operation can be implemented in terms of that.

Also, as noted in https://github.com/fsprojects/FSharp.AWS.DynamoDB/issues/65, anything that can assist with making correct propagation of CTs the unobtrusive non-issue it is in F# app code bases would be very welcome (Even considering making a CT argument mandatory for the Task layer in FSharp.AWS.DynamoDB in order to force the issue). However I'd say that in practice, the actual calls of Task-returning methods are many levels deep into libraries (probably in an area that is using the task builder) - and hence forwarding the CTs through the layers can be a messy change that's rarely as neat as the presented 'to make the warning go away' example. Based on that, I'd also agree that any solution should be a language agnostic thing that does not belong in any compiler

Frassle commented 1 year ago

There's a long standing issue in the fsharp repo about this problem: https://github.com/dotnet/fsharp/issues/2127