fsharp / fsharp/fslang-suggestions
Working with exceptions is too easy to get wrong
- Dominant language
- No language data
- Stars
- 373
- Forks
- 21
- PR merge metrics
- No merged PRs in 30d
Description
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](https://www.google.de/?gws_rd=ssl#q=%22with+_+-%3E%22+site::https://github.com/Microsoft/visualfsharp/blob/master) and [here](https://www.google.de/?gws_rd=ssl#q=%22with+exn+-%3E%22+site::https://github.com/Microsoft/visualfsharp/blob/master)
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](https://github.com/Microsoft/visualfsharp/blob/de67934a8c64f57ee18356f4d94a1edc2df7fdca/src/fsharp/symbols/Exprs.fs#L725). 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](https://github.com/fsharp/fslang-suggestions/issues/591#issuecomment-514657081) as it throws a `new System.Exception(msg)` which shouldn't be done accoding to the [.NET Guidelines](https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/using-standard-exception-types#exception-and-systemexception). Its usage basically forces callers to catch general exceptions, which again you shoudn't (see first point).
- The `exception` keyword makes it [practically impossible](https://github.com/fsharp/fslang-suggestions/issues/591#issuecomment-312997756) 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 "text", exn` to allow raising a custom exception via the `string * exn` constructor.
* [`failwithe : exn -> string -> 't` @rmunn](https://github.com/fsharp/fslang-suggestions/issues/591#issuecomment-312992081)
* [`failwithe : exn -> StringFormat<'a,_> -> 'a` @piaste ](https://github.com/fsharp/fslang-suggestions/issues/591#issuecomment-313940242)
* [`rethrowf : (string -> 'a -> #exn) -> 'a -> StringFormat<'c,_> -> 'c` @piaste](https://github.com/fsharp/fslang-suggestions/issues/591#issuecomment-313940242)
* [`rethrowf : (string -> #exn) -> StringFormat<'c,_> -> 'c` @piaste](https://github.com/fsharp/fslang-suggestions/issues/591#issuecomment-313940242)
- 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`](https://github.com/Microsoft/visualfsharp/issues/3327#issuecomment-315025498) 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:
* [x] This is not a question (e.g. like one you might ask on [stackoverflow](http://stackoverflow.com)) and I have searched stackoverflow for discussions of this issue
* [x] I have [searched both open and closed suggestions on this site](http://github.com/fsharp/fslang-suggestions/issues) and believe this is not a duplicate
* [x] 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:
* [x] This is not a breaking change to the F# language design
* [x] 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
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.