Working with exceptions is too easy to get wrong

[matthid]

I'd like to refine the suggestions while the discussion emerges so I'll start with the reasoning:

• I often see code like with _ -> or with exn ->, even the compiler and here
  The problem is that code like this makes debugging and finding errors extremely difficult when some parts are moving and different (at the time of writing unexpected) errors are thrown. Basically this style catches exceptions you didn't intend to catch.
• Especially critical I think is code like with _ -> None, often used in a collect
• failwith is wrong in most locations: example. In .NET you should always include the reason for the error in the Inner exception field. So the correct code is most of the time with e -> raise <| MyException("string", e) which is both a lot more ugly and harder to write than with _ -> failwith "string" . Again not following this makes finding the root cause unnecessary complex and difficult for the developer.
• failwith shouldn't be used as it throws a new System.Exception(msg) which shouldn't be done accoding to the .NET Guidelines. Its usage basically forces callers to catch general exceptions, which again you shoudn't (see first point).
• The exception keyword makes it practically impossible to follow best practices in .NET exception handling.

I propose

• we introduce a warning when code like the above is used, for example when the exception object is not used in a general System.Exception handler.
• we introduce a easier syntax for re-throwing exception which looks a bit nicer than raise <| MyException(..., e). Currently I don't have a good idea maybe
  • failwithf "text" , exn to allow setting an "InnerException" when using failwithf
  • raisef<MyException> "text", exn to allow raising a custom exception via the string * exn constructor.
  • failwithe : exn -> string -> 't @rmunn
  • failwithe : exn -> StringFormat<'a,_> -> 'a @piaste
  • rethrowf : (string -> 'a -> #exn) -> 'a -> StringFormat<'c,_> -> 'c @piaste
  • rethrowf : (string -> #exn) -> StringFormat<'c,_> -> 'c @piaste
• Change code-generation of the exception keyword in a way to allow setting the Message and InnerException properties (ie there should be constructors calling the base constructors and language primitives to use them, for example createExn<'e> : exn -> msg -> 'e).
  • There exists a workaround for Message by using override.

To be honest those are not very well thought through...

The existing way of approaching this problem in F# is doing it the wrong way.

Pros and Cons

The advantages of making this adjustment to F# are

• make it easier to do the correct thing.
• improved C#/.NET interop
• better tooling and debugging experience

The disadvantages of making this adjustment to F# are

• it's work
• possibly a larger API surface

Extra information

Estimated cost (XS, S, M, L, XL, XXL): L

Related suggestions: I have not found anything here on github

Affidavit (please submit!)

Please tick this by placing a cross in the box:

• This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
• I have searched both open and closed suggestions on this site and believe this is not a duplicate
• This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

• This is not a breaking change to the F# language design
• I or my company would be willing to help implement and/or test this

I'd like it to be non-breaking if possible. I don't think the above are actually breaking

/discuss

[rmunn]

How about failwithe exn "My text"? The exn parameter comes before the string so that one can do something like failwithe exn <| sprintf "Exception in line %s: %s" __LINE__ "message".

Maybe there could also be a raiseChained<'e> function, of type exn -> msg -> 'e. Usage:

exception MyCustomExn of innerException : exn * message : string

try
    1 / 0
with e ->
    raiseChained<MyCustomExn> e "Something went wrong"

which would create, and then raise, a new instance of MyCustomExn with InnerException pointing to the divide-by-zero exception, and message "Something went wrong".

I'd like to see some way of writing raiseChained without having to specify the type as a type parameter, but I don't see an obvious way of achieving that. The InnerException property is read-only, so it's not feasible to write something like raiseChained e MyCustomExn("Something went wrong"), since after the time the exception instance is constructed it's too late to set the InnerException property.

Wait, I just had an idea. If I was writing in Lisp, this is the point where I'd be reaching for macros, to reach into the MyCustomExn("Something went wrong") expression and modify it before it's evaluated. And now that #259 is in F# 4.0, we might be able to leverage that:

let raiseChained innerExn ([<ReflectedDefinition>]outerExn : exn * expr : Expr<exn>) =
    // ...

Then raiseChained would match the expr name against a valid exception-constructor expression -- and would then inject the innerExn into that constructor. A little bit "black-magic"ish, but it would make for a very simple syntax for users:

try
    1 / 0
with e ->
    raiseChained e MyCustomExn("Something went wrong")

Thoughts?

[matthid]

@rmunn Thanks for the suggestions. Ok let me go into this one by one:

• failwithe exn <| sprintf "Exception in line %s: %s" __LINE__ "message" - Yes it's not ideal but in combination with my suggested warning that probably would be an improvement. I don't like that you need to use <| but it's a bit better.

• exception MyCustomExn of innerException : exn * message : string. To be honest that just opens up another thing which is too easy to get wrong. It generates:

  [CompilationMapping(SourceConstructFlags.Exception)]
  [Serializable]
  public class MyCustomExn : Exception, IStructuralEquatable
  {
  	internal Exception innerException@;
  
  	internal string message@;
  
  	[CompilationMapping(SourceConstructFlags.Field, 0)]
  	public Exception innerException
  	{
  		get
  		{
  			return this.innerException@;
  		}
  	}
  
  	[CompilationMapping(SourceConstructFlags.Field, 1)]
  	public string message
  	{
  		get
  		{
  			return this.message@;
  		}
  	}
  
  	public MyCustomExn(Exception innerException, string message)
  	{
  		this.innerException@ = innerException;
  		this.message@ = message;
  	}
  
  	public MyCustomExn()
  	{
  	}
  
  	protected MyCustomExn(SerializationInfo info, StreamingContext context) : base(info, context)
  	{
  	}
  
          // Equals and GetHashCode...
  }

  As you can see there is no way to set the "real" "InnerException" or "Message" property. I personally never use it for that reason. So we either talk about the exception keyword in another suggestion or I'll add it to the list above. My current recommendation is to just pretend it doesn't exist. Usually I'll write exception similar to this

• raiseChained<'e> function, of type exn -> msg -> 'e

  So it would be more like raiseChained<'e,'t> : exn -> msg -> 't which makes usage cumbersome again. Or we have some createExn<'e> : exn -> msg -> 'e and then use raise <| createExn<MyCustom> e "msg". Again: Quite non-ideomatic to use. Not even thinking about custom formats...

• raiseChained e MyCustomExn("Something went wrong") probably doesn't work because the parens are not "bound" to the MyCustomExn constructor but seen as a new argument of raiseChained therefore it needs to be raiseChained e (MyCustomExn "error"). To be honest I'd love to see a syntax which gets rid of parens and <| for the default case. Because otherwise raise <| MyCustomExn("error", inner) almost always is better (because of less magic)

Thinking about this as a whole, I'll probably add the exception keyword to my list above, because if we add primitives like createExn : exn -> msg -> 'e we should think about how we could improve interop with exception (ie. make it work in combination with it) as well.

[matthid]

Modified my initial post and changed it to "L" because of different code-generation of exception (which might move into a different suggestion later, depending on how this evolves)

[piaste]

@matthid

I don't like that you need to use <| but it's a bit better. [..] To be honest I'd love to see a syntax which gets rid of parens and <| for the default case.

You could avoid it with ksprintf, in the same way failwithf does:

let rethrowf e fmt =
    ksprintf (fun msg -> raise <| Exception(message = msg, innerException = e)) fmt
    
// usage
let foo =  
    let e = Exception("foo")
    if true then 15 else rethrowf e "The error was generated in %s" "line 2"

and if you wanted to specify the type of exception, you could replace Exception in the definition with a string -> exn -> exn parameter and then pass in the constructor as a first-class function.

[matthid]

@piaste

open System
open Microsoft.FSharp.Core.Printf
let rethrowa f fmt =
    ksprintf (fun msg -> raise <| f msg) fmt
let rethrowb f inner fmt =
    ksprintf (fun msg -> raise <| f(msg,inner)) fmt
type MyCustomExn = Exception
let param = "some text"
let inner = Exception("foo")
rethrowa (fun msg -> MyCustomExn(msg, inner)) "some message using %s" param  
rethrowb MyCustomExn inner "some message using %s" param

that doesn't look too bad. It might look a bit awkward when the exception has custom parameters (ie not only string*exn), but it's actually a good start, thanks!

[cartermp]

I like the warnings idea, and think that they are definitely doable as a small-to-medium effort. What kind of messages did you have in mind for the antipatterns you described above?

[matthid]

We first need to discuss when we expect the warning as we could make this as "intelligent" as we want:

• warn when _ is used or the variable is not used (might conflict with the unused variable warning)
• warn even when the exception object is used, but we detect that only the Message property was used
• warn when there exists a code-path where the exception object is ignored
• On which exception types this warning should be enabled? Currently I'm thinking System.Exception only?

The message could be

• General message

  FS ____: Exception information should not be ignored. Either wrap the exception object in a new exception object or alternatively log the exception.

• Or if we detect a failwith (and add rethrowf to FSharp.Core):

  FS ____: Exception information should not be ignored. Use rethrowf Exception exn "sd" instead of failwith "sd"

  Where exn is the name of the exception object in the code and failwith "sd" the exact code used in the with

• Or if we detect a None / fallback value:

  FS ____: Exception information should not be ignored. Consider catching a more specific exception type here or alternatively log the exception.

[matthid]

Just wanted to say that

let rethrowf f inner fmt =
    ksprintf (fun msg -> raise <| f(msg,inner)) fmt

works really nice in practice because every exception should have that constructor.
However, I'm a bit hesitant to suggest adding it because it would be nice to at least think about how that could work in combination with the exception keyword.

So, I'm still missing a good idea how we could glue everything together nicely and not have half of it feel wrong.

[matthid]

Just some ideas about the exception keyword @dsyme :

• Extend exception codegen to include constructor to set message and innerexception, this constructor could be transparent for F# code if we want
• Add primitives to use the new constructor:

type Record =
  | Sub1
  | Sub2
  
exception SimpleException
exception MyException of Record
exception MyException2 of test:string * r:Record
let inner = exn("Inner Exception")

try
    createSimpleExn MyException Sub1 inner "TestString" //  (string * exn -> 'exn) -> exn -> string -> 'exn
    createSimpleExnf MyException Sub1 inner "TestString %s" "format" // (string * exn -> 'exn) -> exn -> format...
    createExn MyException Sub1 inner "TestString"
    createExn MyException2 ("test", Sub1) inner "TestString" // ('a -> 'exn) -> 'a -> exn -> string -> 'exn
    createExnf MyException2 ("test", Sub1) inner "TestString %s" "format"
    raiseExnf MyException2 ("test", Sub1) inner "TestString %s" "format" // ('a -> 'exn) -> 'a -> exn -> format ...
    raiseSimpleExn exn inner "TestString" // (string*exn -> 'exn) -> exn -> string -> 't
    raiseSimpleExnf exn inner "TestString %s" "format" // (string*exn -> 'exn) -> exn -> string -> 't
    raise <| MyException(Sub1)
with
| MyException(Sub1) -> ...

Imho we need a function for creating an exception object and ideally a convenience function to raise.
What I don't like here is that this only works for simple (string*exn constructor) and F# exceptions but not generically for all exceptions. But maybe I just want too much.

So raiseSimpleExn is exactly the rethrowf from above while raiseExn is the analog one for F# exceptions.

In a perfect world we would not need the *simple functions but somehow unify them. But I have no idea how that could work.

We probably would extend the warning I suggested above with hints to those new primitives when we detect that an F# exception is involved.

[dsyme]

@matthid Did you consider supporting settable properties in exception construction? e.g.

SimpleException(InnerException=inner)
SimpleException(InnerException=inner, Message=sprintf "hello")
MyException(Sub1,InnerException=inner)

(given your declarations above)

That feels more orthogonal from a language perspective? All settable properties on the Exception type are automatically covered, and there would be no additions to FSharp.Core?

[matthid]

@dsyme While that would work technically, I feel like the simple default use case needs to be simplified a lot. It should not involve parenthesis <|, raise and nested sprintf.

raise <| SimpleException(InnerException=inner, Message=sprintf "hello")
With this I feel like people just keep using failwithf ... And I cannot really blame them.

It definitely would be the correct first step to make it POSSIBLE to do the right thing (which your suggestion enables). I'd prefer a solution which PUSHES you into doing the right thing. And bonus points if it looks as nice as failwith ;)

[dsyme]

@matthid OK, I see. I think that's a hard goal to achieve through an API which would itself basically be optional to use. The warning you recommend would help though.

[matthid]

Is there any subset where a PR has a chance of being accepted?

@dsyme regarding your last suggestion I feel like InnerException=inner is a bit "unusual" given that InnerException and Message are not settable in System.Exception so that would overload the syntax. I just wanted to point that out, but given the current state, I'd rather implement this as it would already improve the situation. Maybe going with this in addition with the warnings and fixing the exception code-gen is enough in practice (maybe we just cannot have nice code given the current state - ie. correct exception code just needs to look ugly in F#. At least we can address the correctness a bit).

[charlesroddie]

@matthid let me try to simplify this.
We have failwith: string -> 'T.
But sometimes we want to include inner exceptions.
So we need another keyword, such as failwithinner: string -> exn -> 'T.
Is there anything missing in this summary?

[matthid]

Is there anything missing in this summary?

@charlesroddie Indeed: you shouldn't throw or catch plain "Exception"s according to .NET design guidelines. So, in fact, you shouldn't use failwith or failwithinner at all.

[cartermp]

Yeah, failwith is generally not preferred. It's the equivalent of throw new Exception(msg) and that's discouraged here: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/using-standard-exception-types

We also adopt a similar guideline in the F# docs: https://docs.microsoft.com/en-us/dotnet/fsharp/style-guide/conventions#use-exceptions-when-errors-cannot-be-represented-with-types

[dsyme]

I think in practice people use failwith in many situations, e.g. rapid prototyping, data scripting and unreachable code. So the guidelines don't really match F# practice. Even in FSharp.Core we have 57 uses of failwith.... They should be scrubbed - but it's an indication that F# practice is not exactly the same as .NET Component guidelines.

[matthid]

I think in practice people use failwith in many situations, e.g. rapid prototyping, data scripting and unreachable code.

Yes. But I don't see how that changes anything:

• rapid prototyping can become production or library code. Now we are all probably too lazy to remove failwith later on. Which makes proper exception handling higher in the stack impossible (best you can do is to general exceptions and parse the message).
• unreachable code can become reachable by adding new cases and forgotten about. This will just be frustrating for users if a new Exception("Unreachable") is thrown, because they don't know what is unreachable or how to workaround. This is especially true if not the direct caller is throwing this but a transitive dependency. This leads to bad bug reports and to more work hours for everyone involved.
• People learn the language by the above use-cases and later do it the wrong way.

In general I feel like using failwith is just being lazy or not knowing better. (Yes there are exceptions like test-code or scripting, but question is what we optimize for)

but it's an indication that F# practice is not exactly the same as .NET Component guidelines.

Yes maybe, but I'm not sure I agree on the exception part of this. The guidelines makes sure writer and users of code agree on how exceptions are used. As we have C# users using F# libraries and the other way around we should agree on exception usage. Otherwise F# libraries force C# authors to catch general exceptions (System.Exception) which is generally seen as a very bad thing to do as this unintentionally might catch other exceptions you didn't want to catch (NullReferenceException for example).

Even in FSharp.Core we have 57 uses of failwith.... They should be scrubbed

Yes they most likely should be all revisited. Same goes for the compiler IMHO. But that is out of scope for this suggestion. Maybe we should warn on every instance of failwith ;)

[cartermp]

but it's an indication that F# practice is not exactly the same as .NET Component guidelines.

I don't think that the guidelines should reflect what people do in practice, but rather what people probably should do in practice. As an example, many F# developers avoid exceptions like the plague, but that doesn't mean it isn't valuable to recommend using them (and the diagnostic data they provide) in exceptional circumstances.

[dsyme]

I don't think that the guidelines should reflect what people do in practice, but rather what people probably should do in practice.

Yes, I think in this case the guidance is too strong. Using failwith is fine in many circumstances, even including production code - though there's certainly nothing wrong with removing it and using more refined types. I'm not jumping to my keyboard to fix those uses of failwith even in FSharp.Core, though it would be an improvement if it were done.

So the guidance should probably be refined to be more nuanced.

[matthid]

Maybe it looks like the suggestion has shifted from "rethrowing" and wrong exception handling to not using failwith. But it really has not. Those are basically the same thing: If you use failwith you have to catch general exceptions and might catch exceptions you didn't intend to catch. This is very well known in C#/.NET world.
In general you now have two bugs: One actual bug and one exception handling bug hiding the actual bug. It's just a matter of time until an actual bug occurs and someone is trying to find it.

I think I'll add that point to the initial post.

[charlesroddie]

P(need to query an exception to work out what to do) = P(exception) * P(handle exception rather than crashing | exception) * P(need to query the exception to work out what to do | handling exception), which is a product of three small numbers, so is very small. So failwith shouldn't be disparaged as it makes sense not to put in a lot of effort for very small gain. This is the wrong line of attack IMO if you want to get things approved.

OTOH making it easier to use System.SomeSpecificException instead of System.Exception and making inner exceptions easier to include does make sense.

[cartermp]

@dsyme I think the docs are pretty clear about it: https://docs.microsoft.com/en-us/dotnet/fsharp/style-guide/conventions#use-exceptions-when-errors-cannot-be-represented-with-types

Since the recommendation overall is to follow .NET guidelines for exceptions, and failwith doesn't give a specific exception, it should generally be avoided. If we feel that the .NET guidelines for exceptions do not apply to F#, then this guidance could perhaps be adjusted. But I really only see this as an OCAML holdover, where exceptions are fundamentally different to .NET.

[dsyme]

Since the recommendation overall is to follow .NET guidelines for exceptions, and failwith doesn't give a specific exception, it should generally be avoided. If we feel that the .NET guidelines for exceptions do not apply to F#, then this guidance could perhaps be adjusted.

The distinction is roughly between script programming, app programming and component programming. In (all) script and (most) app programming, failwith is totally fine. In component programming it's not really fine - though some use is tolerated especially for unreachable code and for F#-to-F# code in unpolished components.

The .NET guidelines were written from the "enterprise component" perspective in the great era of enterprise coding and do not carry over completely to either F# or C# script and app programming. If you're doing script or (some) app programming in C# you should not go around defining new exception types - though you should use an existing refined exception type where it makes sense.

But I really only see this as an OCAML holdover, where exceptions are fundamentally different to .NET.

It wasn't not really a holdover - we embraced it as convenient, and so its inclusion was intentional (failwithf isn't included in OCaml, so when we added it that was a sign we were embracing some use of failwith).

The exception definition is largely an OCaml holdover (as the OP says, we never really made those fully .NET-friendly). Or, more precisely, it's good enough to work in purely F# code but again probably not for use in components, especially those designed for use from both F# and C#.

I don't disagree with the OP for component programming, but have misgivings about applying component programming rules to script and app programming. There are other examples of this distinction - for example consider warning 1182 (unused variables), which is not on by default for script programming (and I wouldn't want it on) - though I'd be happy to have it on in our templates for component (libraries), and perhaps also apps.

[cartermp]

The distinction is roughly between script programming, app programming and component programming. In (all) script and (most) app programming, failwith is totally fine.

The distinction between short-lives scripts and long-lived applications is probably fine, but I honestly don't see too much value in defining best practices for this w.r.t error-handling. If the guideline is, "if you're not doing something critical then you don't need to think too hard about error handling, then that guideline is probably better left unstated. "Do what's convenient if correctness and maintainability doesn't matter" isn't very helpful.

In component programming it's not really fine - though some use is tolerated especially for unreachable code and for F#-to-F# code in unpolished components.

Guidelines should not apply for unpolished/in-progress components. The point of them is that you follow this sort of stuff when you're looking to polish components.

[dsyme]

Guidelines should not apply for unpolished/in-progress components. The point of them is that you follow this sort of stuff when you're looking to polish components.

A polished script or polished app should follow coding style guidelines (e.g. formatted properly, but can use failwith etc.).

A polished component (i.e. library, nuget package) should follow both coding and style component guidelines. So this is a component guideline.

For example, I am fine with a polished TicTacToe or FabulousContacts app using "failwith" for unexpected error conditions or unreachable code in utility code. I'm not ok with 53 examples of "failwith" in FSharp.Core, but I'd take them on a case by case basis and apply the component recommendations. For unreachable code I'd still expect a few as we have no other recommendation for that.

[dsyme]

To get back to the concrete suggestions

• I like all the suggestions that allow setting InnerException when raising exceptions, e.g. failwithe and the like. We should just do those.

• I like any adjustments to exception ABC codegen that get the defaults right, allow InnerException to be set and so on, though it's more work

[abelbraaksma]

@dsyme, does this mean it's approved in principle? If so, could you mark it such, so that someone can start making an RFC? Or should we discuss further until more details are sieved out?

[dsyme]

Yes, the above portions are approved-in-principle. The detail may be contentious but please go ahead with an RFC for those bits

[Tarmil]

Now that class constructors can be fully treated as first-class functions (ie we can pipe into them and so on), we could even treat exception constructors exactly like class constructors, without loss of functionality. I think this would make it feel more idiomatic to have overloaded constructors with additional message and/or innerException arguments. I believe auto-generating these would not be a breaking change even when an exception is constructed with inferred arguments.