From c55fdf334ef536b090e4e2756b9b0bffe1e1d75e Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Mon, 31 Oct 2022 22:19:41 +0100 Subject: [PATCH] Remove ReturnFrom and with it, the `return!`-enabled tailcalls, we should look into tailcalling `yield!` instead --- src/FSharpy.TaskSeq.Test/TaskSeq.Tests.CE.fs | 15 -- src/FSharpy.TaskSeq/TaskSeqBuilder.fs | 265 ++++++------------- 2 files changed, 86 insertions(+), 194 deletions(-) diff --git a/src/FSharpy.TaskSeq.Test/TaskSeq.Tests.CE.fs b/src/FSharpy.TaskSeq.Test/TaskSeq.Tests.CE.fs index 159e6f14..a102ef4d 100644 --- a/src/FSharpy.TaskSeq.Test/TaskSeq.Tests.CE.fs +++ b/src/FSharpy.TaskSeq.Test/TaskSeq.Tests.CE.fs @@ -110,21 +110,6 @@ let ``CE taskSeq with nested deeply yield! perf test 8521 nested tasks`` () = ta data |> should equal (List.ofSeq expected) // cannot compare seq this way, so, List.ofSeq it is } -[] -let ``CE taskSeq with several return!`` () = task { - // TODO: should we even support this? Traditional 'seq' doesn't. - let tskSeq = taskSeq { - return! Gen.sideEffectTaskSeq 10 - return! Gen.sideEffectTaskSeq 5 - } - - let! data = tskSeq |> TaskSeq.toListAsync - - // FIXME!!! This behavior is *probably* not correct - data |> should equal [ 1..10 ] -} - - [] let ``CE taskSeq with mixing yield! and yield`` () = task { let tskSeq = taskSeq { diff --git a/src/FSharpy.TaskSeq/TaskSeqBuilder.fs b/src/FSharpy.TaskSeq/TaskSeqBuilder.fs index e9f03eac..5a7430e1 100644 --- a/src/FSharpy.TaskSeq/TaskSeqBuilder.fs +++ b/src/FSharpy.TaskSeq/TaskSeqBuilder.fs @@ -94,10 +94,6 @@ type TaskSeqStateMachineData<'T>() = [] val mutable boxedSelf: TaskSeq<'T> - /// If set, used for tailcalls using 'return!', contains the target. - [] - val mutable tailcallTarget: TaskSeq<'T> option - member data.PushDispose(disposer: unit -> Task) = if isNull data.disposalStack then data.disposalStack <- null @@ -110,7 +106,6 @@ type TaskSeqStateMachineData<'T>() = and [] TaskSeq<'T>() = - abstract TailcallTarget: TaskSeq<'T> option abstract MoveNextAsyncResult: unit -> ValueTask /// Initializes the machine data on 'self' @@ -164,96 +159,51 @@ and [] TaskSeq<'Machine, 'T data.builder <- AsyncIteratorMethodBuilder.Create() machine.Data <- data - - member internal this.hijack() = - let res = this._machine.Data.tailcallTarget - - match res with - | Some tg -> - // We get here only when there are multiple ReturnFroms - // which allows us to do tailcalls. - - // This recurses itself, e.g. tg.TailcallTarget calls this.hijack(). - match tg.TailcallTarget with - | None -> res - | Some tg2 as res2 -> - // Cut out chains of tailcalls - this._machine.Data.tailcallTarget <- Some tg2 - res2 - | None -> res - // Note: Not entirely clear if this is needed, everything still compiles without it interface IValueTaskSource with member this.GetResult(token: int16) = - match this.hijack () with - | Some tg -> (tg :> IValueTaskSource).GetResult(token) - | None -> - this._machine.Data.promiseOfValueOrEnd.GetResult(token) - |> ignore + this._machine.Data.promiseOfValueOrEnd.GetResult(token) + |> ignore - member this.GetStatus(token: int16) = - match this.hijack () with - | Some tg -> (tg :> IValueTaskSource).GetStatus(token) - | None -> this._machine.Data.promiseOfValueOrEnd.GetStatus(token) + member this.GetStatus(token: int16) = this._machine.Data.promiseOfValueOrEnd.GetStatus(token) member this.OnCompleted(continuation, state, token, flags) = - match this.hijack () with - | Some tg -> (tg :> IValueTaskSource).OnCompleted(continuation, state, token, flags) - | None -> this._machine.Data.promiseOfValueOrEnd.OnCompleted(continuation, state, token, flags) + this._machine.Data.promiseOfValueOrEnd.OnCompleted(continuation, state, token, flags) // Needed for MoveNextAsync to return a ValueTask interface IValueTaskSource with - member this.GetStatus(token: int16) = - match this.hijack () with - | Some tg -> (tg :> IValueTaskSource).GetStatus(token) - | None -> this._machine.Data.promiseOfValueOrEnd.GetStatus(token) + member this.GetStatus(token: int16) = this._machine.Data.promiseOfValueOrEnd.GetStatus(token) member this.GetResult(token: int16) = - match this.hijack () with - | Some tg -> + try log - "Getting result for token on hijacked branch, status: %A" - ((tg :> IValueTaskSource).GetStatus(token)) - - (tg :> IValueTaskSource).GetResult(token) - | None -> - try - log - "Getting result for token on normal branch, status: %A" - (this._machine.Data.promiseOfValueOrEnd.GetStatus(token)) + "Getting result for token on normal branch, status: %A" + (this._machine.Data.promiseOfValueOrEnd.GetStatus(token)) - let x = this._machine.Data.promiseOfValueOrEnd.GetResult(token) - this._machine.Data.promiseOfValueOrEnd.Reset() - x - with e -> - // FYI: an exception here is usually caused by the CE statement (user code) throwing an exception - // We're just logging here because the following error would also be caught right here: - // "An attempt was made to transition a task to a final state when it had already completed." - log "Error '%s' for token: %i" e.Message token - reraise () + let x = this._machine.Data.promiseOfValueOrEnd.GetResult(token) + this._machine.Data.promiseOfValueOrEnd.Reset() + x + with e -> + // FYI: an exception here is usually caused by the CE statement (user code) throwing an exception + // We're just logging here because the following error would also be caught right here: + // "An attempt was made to transition a task to a final state when it had already completed." + log "Error '%s' for token: %i" e.Message token + reraise () member this.OnCompleted(continuation, state, token, flags) = - match this.hijack () with - | Some tg -> (tg :> IValueTaskSource).OnCompleted(continuation, state, token, flags) - | None -> this._machine.Data.promiseOfValueOrEnd.OnCompleted(continuation, state, token, flags) + this._machine.Data.promiseOfValueOrEnd.OnCompleted(continuation, state, token, flags) interface IAsyncStateMachine with /// The MoveNext method is called by builder.MoveNext() in the resumable code member this.MoveNext() = - match this.hijack () with - | None -> - log "at IAsyncStateMatchine.MoveNext" - - try - moveNextRef &this._machine - with e -> - log "at IAsyncStatemachine EXCEPTION!!!" - log "%A" e + log "at IAsyncStateMatchine.MoveNext" - | Some tg -> - // jump to the hijacked method - (tg :> IAsyncStateMachine).MoveNext() + try + moveNextRef &this._machine + with e -> + log "at IAsyncStatemachine EXCEPTION!!!" + log "%A" e /// SetStatemachine is (currently) never called @@ -290,87 +240,67 @@ and [] TaskSeq<'Machine, 'T interface System.Collections.Generic.IAsyncEnumerator<'T> with member this.Current = - match this.hijack () with - | Some tg -> - // recurse, but not really: we jump to a different instance of our taskSeq - // in case there's a tail call target. - (tg :> IAsyncEnumerator<'T>).Current - - | None -> - match this._machine.Data.current with - | ValueSome x -> x - | ValueNone -> - // Returning a default value is similar to how F#'s seq<'T> behaves - // According to the docs, behavior is Unspecified in case of a call - // to Current, which means that this is certainly fine, and arguably - // better than raising an exception. - Unchecked.defaultof<'T> + match this._machine.Data.current with + | ValueSome x -> x + | ValueNone -> + // Returning a default value is similar to how F#'s seq<'T> behaves + // According to the docs, behavior is Unspecified in case of a call + // to Current, which means that this is certainly fine, and arguably + // better than raising an exception. + Unchecked.defaultof<'T> member this.MoveNextAsync() = - match this.hijack () with - | Some tg -> - // recurse, but not really: we jump to a different instance of our taskSeq - // in case there's a tail call target. - (tg :> IAsyncEnumerator<'T>).MoveNextAsync() - - | None -> - log "MoveNextAsync..." + log "MoveNextAsync..." - if this._machine.ResumptionPoint = -1 then // can't use as IAsyncEnumerator before IAsyncEnumerable - log "at MoveNextAsync: Resumption point = -1" + if this._machine.ResumptionPoint = -1 then // can't use as IAsyncEnumerator before IAsyncEnumerable + log "at MoveNextAsync: Resumption point = -1" - ValueTask() + ValueTask() - elif this._machine.Data.completed then - log "at MoveNextAsync: completed = true" + elif this._machine.Data.completed then + log "at MoveNextAsync: completed = true" - // return False when beyond the last item - this._machine.Data.promiseOfValueOrEnd.Reset() - ValueTask() + // return False when beyond the last item + this._machine.Data.promiseOfValueOrEnd.Reset() + ValueTask() - else - log "at MoveNextAsync: normal resumption scenario" + else + log "at MoveNextAsync: normal resumption scenario" - let data = this._machine.Data - data.promiseOfValueOrEnd.Reset() - let mutable ts = this + let data = this._machine.Data + data.promiseOfValueOrEnd.Reset() + let mutable ts = this - log "at MoveNextAsync: start calling builder.MoveNext()" + log "at MoveNextAsync: start calling builder.MoveNext()" - data.builder.MoveNext(&ts) + data.builder.MoveNext(&ts) - log "at MoveNextAsync: done calling builder.MoveNext()" + log "at MoveNextAsync: finished calling builder.MoveNext()" - // If the move did a hijack then get the result from the final one - match this.hijack () with - | Some tg -> tg.MoveNextAsyncResult() - | None -> this.MoveNextAsyncResult() + this.MoveNextAsyncResult() /// Disposes of the IAsyncEnumerator (*not* the IAsyncEnumerable!!!) member this.DisposeAsync() = - match this.hijack () with - | Some tg -> (tg :> IAsyncDisposable).DisposeAsync() - | None -> + task { log "DisposeAsync..." - task { - match this._machine.Data.disposalStack with - | null -> () - | _ -> - let mutable exn = None + match this._machine.Data.disposalStack with + | null -> () + | _ -> + let mutable exn = None - for d in Seq.rev this._machine.Data.disposalStack do - try - do! d () - with e -> - if exn.IsNone then - exn <- Some e + for d in Seq.rev this._machine.Data.disposalStack do + try + do! d () + with e -> + if exn.IsNone then + exn <- Some e - match exn with - | None -> () - | Some e -> raise e - } - |> ValueTask + match exn with + | None -> () + | Some e -> raise e + } + |> ValueTask override this.MoveNextAsyncResult() = @@ -402,8 +332,6 @@ and [] TaskSeq<'Machine, 'T // assume it's a possibly new, not yet supported case, treat as default ValueTask(this, version) // uses IValueTaskSource<'T> - override cr.TailcallTarget = cr.hijack () - and TaskSeqCode<'T> = ResumableCode, unit> and TaskSeqStateMachine<'T> = ResumableStateMachine> and TaskSeqResumptionFunc<'T> = ResumptionFunc> @@ -445,25 +373,17 @@ type TaskSeqBuilder() = else // Goto request - match sm.Data.tailcallTarget with - | Some tg -> - log $"at Run.MoveNext, hijack" - - let mutable tg = tg - moveNextRef &tg + log $"at Run.MoveNext, await" - | None -> - log $"at Run.MoveNext, await" + // don't capture the full object in the next closure (won't work because: byref) + // but only a reference to itself. + let boxed = sm.Data.boxedSelf - // don't capture the full object in the next closure (won't work because: byref) - // but only a reference to itself. - let boxed = sm.Data.boxedSelf - - sm.Data.awaiter.UnsafeOnCompleted( - Action(fun () -> - let mutable boxed = boxed - moveNextRef &boxed) - ) + sm.Data.awaiter.UnsafeOnCompleted( + Action(fun () -> + let mutable boxed = boxed + moveNextRef &boxed) + ) with exn -> log "Exception dump:" @@ -556,10 +476,13 @@ type TaskSeqBuilder() = member inline _.TryFinallyAsync(body: TaskSeqCode<'T>, compensation: unit -> Task) : TaskSeqCode<'T> = ResumableCode.TryFinallyAsync( + TaskSeqCode<'T>(fun sm -> sm.Data.PushDispose(fun () -> compensation ()) body.Invoke(&sm)), + ResumableCode<_, _>(fun sm -> + sm.Data.PopDispose() let mutable __stack_condition_fin = true let __stack_vtask = compensation () @@ -583,6 +506,7 @@ type TaskSeqBuilder() = Task.CompletedTask) body.Invoke(&sm)), + ResumableCode<_, _>(fun sm -> sm.Data.PopDispose() compensation () @@ -595,11 +519,15 @@ type TaskSeqBuilder() = body: #IDisposable -> TaskSeqCode<'T>, ?priority: IPriority2 ) : TaskSeqCode<'T> = + + // FIXME: what about priority? ignore priority + // A using statement is just a try/finally with the finally block disposing if non-null. this.TryFinally( (fun sm -> (body disp).Invoke(&sm)), (fun () -> + // yes, this can be null from time to time if not (isNull (box disp)) then disp.Dispose()) ) @@ -610,7 +538,10 @@ type TaskSeqBuilder() = body: #IAsyncDisposable -> TaskSeqCode<'T>, ?priority: IPriority1 ) : TaskSeqCode<'T> = + + // FIXME: what about priorities? ignore priority + // A using statement is just a try/finally with the finally block disposing if non-null. this.TryFinallyAsync( (fun sm -> (body disp).Invoke(&sm)), @@ -727,27 +658,3 @@ type TaskSeqBuilder() = sm.Data.awaiter <- awaiter sm.Data.current <- ValueNone false) - - // TODO: using return! for tailcalls is wrong. We should use yield! and have F# - // desugar to a different builder method when in tailcall position - // - // Because of this using return! from non-tailcall position e.g. in a try-finally or try-with will - // giv incorrect results (escaping the exception handler - 'close up shop and draw results from somewhere else') - member inline b.ReturnFrom(other: IAsyncEnumerable<'T>) : TaskSeqCode<'T> = - TaskSeqCode<_>(fun sm -> - match other with - | :? TaskSeq<'T> as other -> - // in cases that this is null - other.InitMachineDataForTailcalls(sm.Data.cancellationToken) - - // set 'self' to point to the 'other', and unset Current - sm.Data.tailcallTarget <- Some other - sm.Data.awaiter <- null - sm.Data.current <- ValueNone - - // For tailcalls we return 'false' and re-run from the entry (trampoline) - false - - | _ -> - // other types of IAsyncEnumerable, just yield - b.YieldFrom(other).Invoke(&sm))