Fsharp: CE try catch with exception filters should use EDI instead of throw ex

[NinoFloris]

CE try catch with exception filters should use EDI instead of throw ex.
Came across this while improving exception handling in Ply.

open System

let foo() = async {
    try
        invalidOp "ex"
    with
    // This isn't catching InvalidOperationException, so the stacktrace should stay the same
    | :? ArgumentException -> return ()
}

Catch lambda is compiled to

  [CompilerGenerated]
    internal static FSharpAsync<Unit> catchHandler@1(Exception _arg1)
    {
        ArgumentException ex = _arg1 as ArgumentException;
        if (ex != null)
        {
            return AsyncPrimitives.MakeAsync(new foo@8-2());
        }
        throw _arg1; 
    }

A rethrow isn't possible there but ExceptionDispatchInfo.Throw is.

Doing a throw causes cut off stacktraces

[NinoFloris]

@cartermp could you tag this as a bug?

[cartermp]

I'd need @dsyme to consider this one. The F# 4.5 work to improve async stack traces was wrought with all kinds of problems where 1 step forward in one area would lead to 1 step backwards in another.

[NinoFloris]

That was about removing stack frames, this is about fixing broken codegen

[NinoFloris]

So to clarify because I was short in my issue description:

• This affects all CEs, when there are exception filters in play.
• Normal (outside CE) exception filters don't have this behavior.
• CEs that only use try ... with ex -> don't have this behavior either.
• Using throw here instead of rethrow/EDI.throw means exceptions that don't match any of the filters get their stacktraces cut off, starting from the position in the stack where the CE filters are applied.

This is a bug.

[dsyme]

cc @eiriktsarpalis

[dsyme]

The relevant code is here in PatternMatchCompilation.fs

                | Throw ->
                    // We throw instead of rethrow on unmatched try-catch in a computation expression. But why?
                    // Because this isn't a real .NET exception filter/handler but just a function we're passing
                    // to a computation expression builder to simulate one.
                    mkThrow matchm resultTy (exprForVal matchm origInputVal)

So the proposal is to change this to emit a

 (ExceptionDispatchInfo.Capture exn).Throw()

@abelbraaksma note this is a just a bug w.r.t. loss of stack traces, nothing else.

[NinoFloris]

Alright I see two options here, the easy fix and the semantically correct one

1. Replacing throw ex; with EDI.Throw(exn) inside catchHandler@1. This gets us 90% of the wins, the * only * downside is that this causes the trace to be augmented with the catchHandler@1 frame even though semantically this frame shouldn't exist.
   When an exception is thrown in the catch handler it means the exception didn't match any filters, and just like a native/IL try catch an exception should actually bubble upwards unchanged, which brings us to 2.

2. To remove the extra frame an additional recognized overload for TryWith should be added. This would take a catch function catch : EDI -> M<'T> instead of catch : exn -> M<'T>. The EDI can then be captured in the implementer's try with and edi.Throw() could then be called by the catch handler, note the use of the passed in instance here. If the handler matches, the exception can be retrieved from the EDI via EDI.SourceException, if it doesn't match the catchHandler@1 frame would be invisible due to the Capture predating the frame, as it semantically should.

I'll get to work on 1 but we may want to put 2 in as a language design suggestion.

[dsyme]

THis is right. We should treat (1) as a bug fix and (2) as a lang design suggestion.

[abelbraaksma]

@abelbraaksma note this is a just a bug w.r.t. loss of stack traces, nothing else.

@dsyme, I understand, but I wasn't part of this thread, did you mean to copy me in? There's an SO question around where I did ask about EDI and use it with CE's, but that's years ago ;)

[NinoFloris]

Hopefully this helps to show the positive impact of stacktrace differences before and after the codegen change:

let origin (): unit = invalidOp "Generic error message."
let caller() = task {
    try
        return origin()
    with
    | :? ArgumentNullException -> return ()
    | :? ArgumentException -> return ()
    // This isn't catching InvalidOperationException, so the stacktrace should stay the same
}

[<EntryPoint>]
let main argv =
    try caller().GetAwaiter().GetResult() |> ignore
    with ex -> Console.WriteLine(ex)
    0

Before (note PlyProgram.origin is not in this trace at all):

System.InvalidOperationException: Generic error message.
   at [email protected](Exception _arg1) in /Users/nfloris/Projects/Crowded/Ply/Program.fs:line 15
   at Ply.TplPrimitives.tryWith[u](FSharpFunc`2 continuation, FSharpFunc`2 catch) in /Users/nfloris/Projects/Crowded/Ply/Ply.fs:line 307
   at [email protected](Unit unitVar) in /Users/nfloris/Projects/Crowded/Ply/Program.fs:line 11
   at Ply.TplPrimitives.ContinuationStateMachine`1.RunContinuation() in /Users/nfloris/Projects/Crowded/Ply/Ply.fs:line 84
   at Ply.TplPrimitives.ContinuationStateMachine`1.System-Runtime-CompilerServices-IAsyncStateMachine-MoveNext() in /Users/nfloris/Projects/Crowded/Ply/Ply.fs:line 101
--- End of stack trace from previous location where exception was thrown ---
   at PlyProgram.main(String[] argv) in /Users/nfloris/Projects/Crowded/Ply/Program.fs:line 21

You can imagine that in an actual application the traces above could leave out numerous calls that could have provided exactly the information needed to fix the issue.

After:

System.InvalidOperationException: Generic error message.
   at PlyProgram.origin() in /Users/nfloris/Projects/Crowded/Ply/Program.fs:line 9
   at [email protected](Unit unitVar) in /Users/nfloris/Projects/Crowded/Ply/Program.fs:line 12
   at Ply.TplPrimitives.tryWith[u](FSharpFunc`2 continuation, FSharpFunc`2 catch) in /Users/nfloris/Projects/Crowded/Ply/Ply.fs:line 280
--- End of stack trace from previous location where exception was thrown ---
   at [email protected](Exception _arg1) in /Users/nfloris/Projects/Crowded/Ply/Program.fs:line 15
   at Ply.TplPrimitives.tryWith[u](FSharpFunc`2 continuation, FSharpFunc`2 catch) in /Users/nfloris/Projects/Crowded/Ply/Ply.fs:line 307
   at [email protected](Unit unitVar) in /Users/nfloris/Projects/Crowded/Ply/Program.fs:line 11
   at Ply.TplPrimitives.ContinuationStateMachine`1.RunContinuation() in /Users/nfloris/Projects/Crowded/Ply/Ply.fs:line 84
   at Ply.TplPrimitives.ContinuationStateMachine`1.System-Runtime-CompilerServices-IAsyncStateMachine-MoveNext() in /Users/nfloris/Projects/Crowded/Ply/Ply.fs:line 101
--- End of stack trace from previous location where exception was thrown ---
   at PlyProgram.main(String[] argv) in /Users/nfloris/Projects/Crowded/Ply/Program.fs:line 21

Here we see the origin of the exception in the trace 🎉

Attached is the workaround in Ply, I'm very happy to remove it after the change ships.

    let tryWith(continuation : unit -> Ply<'u>) (catch : exn -> Ply<'u>) =
        try
            let next = continuation()
            if next.IsCompletedSuccessfully then next else 
                let mutable awaitable = next.awaitable
                Ply(await = { new Awaitable<'u>() with 
                    override __.Await(csm) = awaitable.Await(&csm)
                    override this.GetNext() = 
                        try 
                            let next = awaitable.GetNext()
                            if next.IsCompletedSuccessfully then next else
                                awaitable <- next.awaitable
                                Ply(await = this)
                        with ex ->
                            let edi = ExceptionDispatchInfo.Capture(ex)
                            try catch ex
                            // 'Fix' for https://github.com/dotnet/fsharp/issues/8529 hopefully this can soon be removed.
                            // It is much less common for user code to raise the same exception again with the intent of obscuring traces
                            // than it is to have exception filters where it's expected the trace stays intact if it doesn't match any filter.
                            // Yet we have no way of discriminating correct user code doing `raise ex` from the incorrect compiler generated call.
                            // Therefore we also examine ex.Data[forceThrowEx] to opt out of our own incorrect (but preferable) behavior.
                            with catchEx when obj.ReferenceEquals(catchEx, ex) && not <| ex.Data.Contains(forceThrowEx) ->
                                edi.Throw()
                                defaultof<_>
                })
        with ex ->
            let edi = ExceptionDispatchInfo.Capture(ex)
            try catch ex
            // 'Fix' for https://github.com/dotnet/fsharp/issues/8529 hopefully this can soon be removed.
            // It is much less common for user code to raise the same exception again with the intent of obscuring traces
            // than it is to have exception filters where it's expected the trace stays intact if it doesn't match any filter.
            // Yet we have no way of discriminating correct user code doing `raise ex` from the incorrect compiler generated call.
            // Therefore we also examine ex.Data[forceThrowEx] to opt out of our own incorrect (but preferable) behavior.
            with catchEx when obj.ReferenceEquals(catchEx, ex) && not <| ex.Data.Contains(forceThrowEx) ->
                edi.Throw()
                defaultof<_>

[eiriktsarpalis]

Tagging a slightly related issue: https://github.com/fsharp/fslang-suggestions/issues/613