Presence of return is misleading about fsharp.control.asyncseq HOT 17 OPEN

So if I understand correctly we should just remove the Return from the builder in this repo (and accept the breaking change if people are using return explicitly in asyncSeq)

from fsharp.control.asyncseq.

As a workaround to stop the pain now, I've added this to our project to define a version of the CE builder without the Return:

namespace FSharp

open FSharp.Control

/// Computation builder that allows creating of asynchronous sequences using the 'asyncSeq { ... }' syntax
/// This is a "fixed" version without the value-ignoring Return
type FixedAsyncSeqBuilder() =
    let builder = AsyncSeq.AsyncSeqBuilder()

    member __.Bind (source, body) = builder.Bind (source, body)
    member __.Combine (seq1, seq2) = builder.Combine (seq1, seq2)
    member __.Delay f = builder.Delay f
    member __.For (source: 'a seq, action) = builder.For (source, action)
    member __.For (source: 'a AsyncSeq, action) = builder.For (source, action)
    member __.TryFinally (body, comp) = builder.TryFinally (body, comp)
    member __.TryWith (body, handler) = builder.TryWith (body, handler)
    member __.Using (resource, binder) = builder.Using (resource, binder)
    member __.While (guard, body) = builder.While (guard, body)
    member __.Yield value = builder.Yield value
    member __.YieldFrom source = builder.YieldFrom source
    member __.Zero () = builder.Zero ()

module Control =
    // shadow the builder from FSharp.Control
    let asyncSeq = FixedAsyncSeqBuilder()

from fsharp.control.asyncseq.

I'll try do an updated summary because i got same issue ( #92 ), it's really error prone :D more so migrating from v1 to v2

So the compiler feature from @mexx was released in F# 4.1

NOTE

• the FSharp.Core version doesnt matter (it's ok we continue to target >= v3), it matter just the compiler version. Right? /cc @mexx @dsyme
• Until support for compiler builtin in VS 2015 is dropped by asyncseq, it's not possibile to use the new feature.

Q: Can be useful to do it just for netstandard2.0? that will add an #ifdef NETSTANDARD2_0 to remove the Return from asyncsec builder (if i understand right the change required by the feature).

• PRO fix the issue for netstandard, because it require a F# 4.1+ compiler so we can expect to have that feature
• PRO doesnt change anything for users targeting net only (no api surface changes), for older or newer compiler
• CONS1 if a code multitarget both net and netstandard (or add netstandard after a migration), any usage of return will fail to compile (well, will be ok in net, but fails in netstandard).
• CONS2 this change the current api surface for netstandard2.0, so it's anyway a breaking change -> fsharp.control.asyncseq v3**. but user with v2 can meanwhile start change the code to remove it, so override to v3 will work anyway.

I think the CONS1 is instead a nice to have.
Will break some compilation, but usage of return directly is 99% a bug afaik.
I cannot think why someone should use it directly, the desugar of while is done by compiler.
So doing that will surface a bug in the user implementation.

any code like this

let a = asyncSeq {
   yield 1
   return 2 // this compile, but 2 is ignored, so a bug without error
   }

will need to be changed to

let a = asyncSeq {
   yield 1
   }

who does the same thing.

@eulerfx @mexx @dsyme make sense?

from fsharp.control.asyncseq.

Yes, IIRC this is an F# language limitation where it is problematic to express that a method takes a single parameter of type unit, rather than no parameters. It's almost never needed but the computation expression translation uses both foo.Return(()) and foo.Return() so we need a method that can accept either. If you can find an alternative workaround it would be much appreciated.

from fsharp.control.asyncseq.

@dsyme I've tried a patched version of compiler and could successfully remove the Return method from the builder.

As stated in the initial comment, the call Zero() instead of the Return() allows constructs like

asyncSeq {
    yield 1
    do! Async.Sleep 2000
    yield 30
}

to be processed without the need for Return method.

Should I create a PR with the compiler change in visualfsharp for further discussion?

from fsharp.control.asyncseq.

Hi @maxx

We wouldn't change Return to Zero - that would be a breaking change. We might however remove the limitation that Return gets called with both () (zero) and (()) (one) arguments, which if IIRC is the cause of the problem.

However even that change might not meeting our no-breaking-change policy, and we might have to resort to just emitting a warning of some kind.

from fsharp.control.asyncseq.

I think my concern is not clear. In the current asyncSeq one can use return whatever and the value of whatever expression is not processed any further by the asyncSeq. Furthermore the asyncSeq { return [1] } is an empty sequence. This is not intuitive and do not correspond to the behavior of other computation expressions. So IMHO the root cause is the presence of return.

Don, you are right about the breaking change if we simply replace the return () with zero. But at the time we inject the behavior we know about what methods are present on the builder and could inject Zero when there is no Return. This would allow us to write AsyncSeqBuilder without having the Return method on it and the asyncSeq { return whatever } would become illegal at compile time.

from fsharp.control.asyncseq.

@dsyme I've updated my patch to fallback to Zero if Return is not present, you can find it here.

from fsharp.control.asyncseq.

That approach looks good! Please add an fslang.uservoice.com entry for it - I'll mark it as approved for some future release - and submit the PR to Microsoft/visualfsharp.

It will take a long while before libraries like AsyncSeq can rely on this language change though, since it basically currently targets FSharp.Core 4.3.0.0 and supports use by F# 3.0+

from fsharp.control.asyncseq.

UserVoice and PR

from fsharp.control.asyncseq.

@dsyme
While implementing the tests for the PR on visualfsharp repo, I could define the implementation for return as Return (()) = empty which will be picked correctly for the unit parameter, and therefore allow only unit valued expressions to be used with return keyword. It doesn't work when you define Return() = empty. Is it intended to work this way?

It's very weird, actually the distinguishing problem only arises from the usage of signature file.
Only in the signature file I didn't found a way to define that there should be one parameter of type unit. Is there a way to define such signature in the .fsi file?

from fsharp.control.asyncseq.

@mexx following up on this, is there still some action to take on this library or has focused shifted elsewhere?

from fsharp.control.asyncseq.

The next version of F# compiler will allow to use Zero without the need to provide Return.
With this version this library could get rid of the Return method on the builder. However this would be a breaking change for users of the library.

from fsharp.control.asyncseq.

I was extremely confused by this as well. My use case was yielding a single value in the sequence given an Async<T>. I used return, which type checked, so I was expecting the async seq to have this one value produced by the async expression, but got nothing. The crux of this issue is that the builder's return accepts any parameter and ignores it.

The adjustment to the compiler is nice, since I ran into that do! issue with my own CE as well - but I'd like to suggest a simple workaround in the meantime that does not cause this confusion: restrict Return's parameter type to unit. This allows you to continue to use do! and return () but not return 1 or any other arbitrary value. This would cause code written against this library to fail to compile if the code returns non-unit values, but this can only be a good thing since such code was probably written under the misconception that something was being done with the value.

from fsharp.control.asyncseq.

I tried to create a pull request for the above, but encountered a problem with specifying the signature in the fsi file. The compiler required me to define the Return method as member x.Return (_: unit) = empty, but there doesn't seem to be a way to define the signature for that. member Return : unit -> AsyncSeq<'T> gives the error Module 'FSharp.Control.AsyncSeq' requires a value 'member AsyncSeq.AsyncSeqBuilder.Return : unit -> AsyncSeq<'T>'. This isn't an option unless we exclude this member from the signature file.

from fsharp.control.asyncseq.

I was really confused by this.
Found this thread and the other issue only after posting my own solution here

from fsharp.control.asyncseq.

This issue confused two of my teammates again last week (we're a team of only 6, so that really hurts). I've submitted a PR with my best effort to fix this issue.

from fsharp.control.asyncseq.