fsharp / fslang-suggestions

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

Allow awaiting tasks in async CEs without Async.AwaitTask #1142

Open cmeeren opened 2 years ago

cmeeren commented 2 years ago

I propose we allow let!/do!/match!/return! in async CEs with Task values.

Additionally, if possible (if there are no hidden gotchas and it's easy to implement), we could consider doing the same for ValueTask and/or other awaitables (anything that has a suitable GetAwaiter method?), though Task is the most important one.

The existing way of approaching this problem in F# is to use Async.AwaitTask (and additionally, for ValueTask, calling AsTask() first).

Pros and Cons

The advantages of making this adjustment to F# are:

The disadvantages of making this adjustment to F# are: None that I can think of.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): XS (Just add a suitable Bind overload to the async CE)

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.

jwosty commented 2 years ago

There is a con: it encourages using a construct which may kill performance, as switching between async and task is not free. That's not to say that this isn't worth doing though.

EDIT: Didn't realize you could already await async inside task. Nevermind.

baronfel commented 2 years ago

One personal hurdle I've had with this easy bridging mechanism is that it makes it very easy to lose the CancellationToken for the async in the translation. The explicit call serves as a reminder to me to check if my Task-generating function accepts a CancellationToken, and if so extract it from the async context before creating the Task.

jwosty commented 2 years ago

One personal hurdle I've had with this easy bridging mechanism is that it makes it very easy to lose the CancellationToken for the async in the translation

Yeah, it would be really great to solve this in some way. Would be really nice if the compiler could emit a warning when a) you're inside an async context and b) you call a task-returning method/function with an optional cancellation token parameter and c) you haven't supplied said optional token. But that would be another language suggestion.

baronfel commented 2 years ago

that would be another language suggestion.

Yeah, and in the general case that would be something better served by an Analyzer IMO. Keep the compiler core itself focused, and push best-practices stuff outside the core.

cmeeren commented 2 years ago

The explicit call serves as a reminder to me to check if my Task-generating function accepts a CancellationToken, and if so extract it from the async context before creating the Task.

Sure, though if its only real utility is acting as a reminder for checking something elsewhere (in your task-generating function), then I'd argue it's not worth keeping. As you say above, an analyzer or similar is better for that.

jwosty commented 2 years ago

The explicit call serves as a reminder to me to check if my Task-generating function accepts a CancellationToken, and if so extract it from the async context before creating the Task.

Sure, though if its only real utility is acting as a reminder for checking something elsewhere (in your task-generating function), then I'd argue it's not worth keeping. As you say above, an analyzer or similar is better for that.

I was referring to something more like the following:

// given:
//type File =
//    static member ReadAllTextAsync: path: string *
//                                    ?cancellationToken: Threading.CancellationToken
//                                    -> Task<string>

let doWork file = async {
    // Could generate warning to pass in CancellationToken here, since it is statically knowable
    let! text = Async.AwaitTask (File.ReadAllTextAsync file)
    printfn "file contents: %A" text
}

And if we used a function instead of ReadAllText which doesn't take a cancellation token, it wouldn't emit said warning. So it would be a sure-fire "you forgot to pass the cancellation token, dummy." However as has already been said one can make the argument that it should be in an analyzer, not the compiler

edgarfgp commented 2 years ago

This took me sometime to figure out and would be awesome to have a more friendly approach

jamil7 commented 2 years ago

Estimated cost (XS, S, M, L, XL, XXL): XS (Just add a suitable Bind overload to the async CE)

I tried doing this a while ago, and found that overloading _.Bind() didn't work so well when I was mixing Asyncs and Tasks in the CE. It broke type inference quickly, and I had to annotate all the variables in the CE manually, which was annoying.

So I used the hide and seek champion _.Source() instead (page 70 here). I'm not sure if it was intended to be used like this, but here is what I ended up with, which works fine in my personal experience when it comes to type inference.