F# exceptions should be formatted properly in stack traces

vasily-kirichenko commented 7 years ago

exception Foo of string
let x: int = raise (Foo "message one")

> FSI_0002+Foo: Exception of type 'FSI_0002+Foo' was thrown.
   at <StartupCode$FSI_0005>.$FSI_0005.main@()

"message one" is not shown in the stack trace, which is awful.

Compare to an ordinary exception:

> let x: int = raise (Foo("message one"))
FSI_0006+Foo: message one
>    at <StartupCode$FSI_0007>.$FSI_0007.main@()

It makes F#-style exceptions basically unusable.

vasily-kirichenko commented 7 years ago

A "workaround" is to override `ToString":

exception Foo of string with override this.Message = this.Data0

:)

matthid commented 7 years ago

To be honest the exception keyword is unusable for several reasons, see https://github.com/fsharp/fslang-suggestions/issues/591. Thanks for the workaround with override, nice trick. Sad that this doesn't work for InnerException

eiriktsarpalis commented 7 years ago

I should add that code generation for ISerializable is also broken for exception types.

vasily-kirichenko commented 7 years ago

On a positive note, code generation for exceptions should be quite isolated in the codebase and safe to adjust (just thinking aloud).

dsyme commented 7 years ago

I should add that code generation for ISerializable is also broken for exception types.

The link for this issue is here: https://github.com/Microsoft/visualfsharp/issues/878. A related issue is whether serialization of exceptions is supported or configurable in .NET Core https://github.com/dotnet/coreclr/issues/2715.

It makes F#-style exceptions basically unusable.

I'd be interested for someone to advocate the more radical solution: simply deprecating F#-style exception declarations. I'm not necessarily pushing for that, but I've often considered whether we should just be asking people to write the class declaration. How much is the feature buying us if we have to codegen our way out of trouble all the time?

forki commented 7 years ago

Automatic code gen is nice - if it works properly ;-)

I think biggest user is compiler itself. From what I can see sometimes only to pattern match on it ;-)

eiriktsarpalis commented 7 years ago

I'd be interested for someone to advocate the more radical solution: simply deprecating F#-style exception declarations.

Unless there is intention of radically overhauling their design, I'd favor deprecation. That said, writing class-based exceptions in F# is very painful boilerplate and hard to get right. A properly written exception must consume two different constructors of its base class (the regular one and the ISerializable one) which means that it cannot be defined using default constructor syntax. There's certainly something missing in this space.

Also. FWIW F# exceptions cannot be generic

dsyme commented 7 years ago

BTW I think "unusable" is too strong - and it's best to avoid hyperbole in technical discussions. F#-defined exceptions are used in compiler and elsewhere and have long been usable for that purpose. But I agree they are far from perfect.

matthid commented 7 years ago

I think "unusable" is too strong

I guess that really depends on the personal opinion on where your point of no return is.

F#-defined exceptions are used in compiler

Tbh. the compiler is probably not a good example on how to do exceptions. The compiler-API is especially painful to use when some internal stuff fails. The compiler will not tell you what's wrong (only "internal-error") and will hide internal error details pretty hard from you.

In my FAKE netcore update this really did cost countless hours of debugging and custom FCS builds providing more debugging information. But that is a different discussion.

Imho we can deprecate exception as a short term solution. But I'd love to see the code-gen fixed long term and provide a robust cross-situational-implementation. Really using exception should be a "pit of success" just like everything else in F#. If we cannot provide a implementation that works everywhere we need to give users the tools and guides to customize as required.

dsyme commented 7 years ago

Really using exception should be a "pit of success" just like everything else in F#.

I agree with this.

Tbh. the compiler is probably not a good example on how to do exceptions. The compiler-API is especially painful to use when some internal stuff fails. The compiler will not tell you what's wrong (only "internal-error") and will hide internal error details pretty hard from you.

The normal technique is to turn on first-chance exception debugging in the debugger (e.g. Visual Studio) to show the exception at the point of it being raised. At least that works well for me. By it's nature a compiler tends to have catch-all exception handlers at its entry point

But yes, it can be painful, I agree

In my FAKE netcore update this really did cost countless hours of debugging and custom FCS builds providing more debugging information. But that is a different discussion.

Yes, debugging FAKE is particularly brutal. I think the problem there was much more that the FCS API for scripting was not initially well-designed for reporting exceptions - more than the "exception" definitions in F#

Imho we can deprecate exception as a short term solution.

We would only deprecate if class-based exception declarations were deemed a suitable replacement (which they aren't, as Eirik has pointed out). And we would only do it as a permanent solution.

We can follow up on https://github.com/fsharp/fslang-suggestions/issues/591, which I basically agree with.

matthid commented 7 years ago

The normal technique is to turn on first-chance exception debugging in the debugger (e.g. Visual Studio) to show the exception at the point of it being raised

We should just keep in mind that there are environments where this is just not possible. At that early point in time this was not possible in netcore and it even is not by now for F# (but I could help myself with a C# app I guess). And there always will be platforms and early adopters where this is not an option at all. But yeah, I agree it would have helped. It's already the first thing I turn on, but I'm always more happy if I can figure out the bug without it - either because of awesome errors/exceptions or logging ;)

were deemed a suitable replacement (which they aren't, as Eirik has pointed out)

They are hard to get right either way so I don't understand the point there. One could argue that using exception it's a lot harder because you cannot really change what the compiler generates (which is already broken). But I get the point about making it permanent.

So, to get back to this issue: I think it would be a small step into the correct direction to change the message to default to the same string as a union-case would generate? Or allow to mark fields as the message or make it by convention?

exception of message:string * other:Data would automatically set the Message property? Or as I suggested in https://github.com/fsharp/fslang-suggestions/issues/591 expect the Message and InnerException properties to be "always" there and have new primitives to set them on new instances.

@vasily-kirichenko What would be the expected behavior or the "bugfix" you suggest?

dsyme commented 7 years ago

. At that early point in time this was not possible in netcore and it even is not by now for F#

Do you know what the technical issue is there? Do you mean with VS 2017?

Always populating "Message" with the sprintf %A sounds totally reasonable

eiriktsarpalis commented 7 years ago

Always populating "Message" with the sprintf %A sounds totally reasonable

:+1: it can always be overridden if something different is required

abelbraaksma commented 7 years ago

This is now marked "feature request", but really also is a bug ;), see below.

I'm very glad this discussion is being held. I don't think the DU-style exceptions should be taken out or deprecated, I use them a lot and I'd argue that I won't be the only one.

Some of the comments made here (like @matthid _"would automatically set the Message property?") suggest that it is possible to create a Message property with F#-style exceptions.

I raised a bug 9 months ago about that, it is currently broken and raises pretty weird errors by the compiler, or none when it should (it totally confuses the system, but that was VS2015, not sure about 2017), see #1602.

To summarize:

exception SomeException of Message: string compiles, but wrongly
- it adds a Message_get, which shadows the parent Exception.Message (missing override)
- after casting to System.Exception (or catching it), the Message property will be empty
- when trying to overcome this with an extra override this.Message = this.Message you'll get FS0023
- same problem with other fields (Data for instance)

Current workarounds

What I needed for my project was:

simple pattern matching on exceptions (working with .NET exceptions natively is a hassle)
make the exception types exportable, that is, catchable and usable outside of F#
simple definitions (they need to be generated from a RESX file with messages)

I eventually did this for each exception, which makes it exportable and still usable in pattern matching try/with:

module Exc =
    exception SomeException of info: string with
        override this.Message = this.info

My preference

Any solution that can seamlessly cast to and fro System.Exception seems appropriate. I don't know what the effort is to fix the current behavior in the F# compiler, but I'd hope that something like the following minimum wouldn't be too hard:

add the override to prevent the shadowing in cases where Message or Data is used in the DU fields (this may be extended to work the same for other DU's, see what currently happens in #1602).
keep current pattern-matching behavior (really, don't deprecate!)
auto-gen override this.Message when the first argument is string and unnamed, exception X of string
get rid of the compile errors mentioned in #1602.

Of course, there is much more that can be done, but this would solve the issue raised here and adds a lot of usability to the exception types.

mmc41 commented 5 years ago

Being new to F#, poor printing of F# exceptions was one of my biggest disappointments. None of the literature mentioned how awkward it would be when debugging F# exceptions. I expected them to simply work.

dsyme commented 2 years ago

A possible fix for the original problem reported here is in #12736. It adds a Message override if none has been provided, using %A to format all the data in the exception object. This seems reasonable.

We will likely gate it by language version since it could be a breaking change (people doing silly things like searching inside the text of an exception message)

dsyme commented 2 years ago

Fixed up above

abelbraaksma commented 2 years ago

Neat! So this fixes the shadowing of the Message_get in the generated code? I.e., the main thing of the causes I mentioned:

it adds a Message_get, which shadows the parent Exception.Message (missing override)

abelbraaksma commented 2 years ago

Oh wait, you already mentioned that above, cool!

It adds a Message override if none has been provided, using %A to format all the data in the exception object. This seems reasonable.

dotnet / fsharp