Do not panic in libraries

[StabbyCutyou]

I noticed in several places, you guys are panicking - with good intentions.

The problem is, this makes it a lot harder to "trust" the library will not cause problems when handed improper inputs.

The better solution to this is to return an error, which must be handled by the user expressly. While it's more code involved to work with the library, it's also a significantly safer approach to providing functionality for others to consume.

If this is a PR you'd accept, I would happily make this change.

[vadimg]

Most of the time, we panic when something that should never happen occurs. There are 2 times where we panic for other reasons:

• Creating a Decimal from a float when the float is Nan or +/-Inf (I agree this should return an err instead)
• Overflow of the exponent (returning an err would make the APIs very ugly and prevent method chaining)

Unfortunately, even for the creation of a Decimal from a float, I don't think I want to change the API since that would break all clients if anyone decides to upgrade. I might revise this decision in the future, though.

[RadhiFadlillah]

@victorquinn can you look at this again ?

I'm fine with the library panicking on cases that mentioned by @vadimg, however I don't think library should panic when decimal divided by zero. Sure I can check the divisor for zero before running Div, but I think it would be nice if the library not panic in this common case.

Overflow of the exponent (returning an err would make the APIs very ugly and prevent method chaining)

If you are concerned with method chaining, you can create two APIs like sqlx do :

• Div will return Decimal and error, and won't panic when the divisor is zero.
• MustDiv will only return Decimal, and will panic when the divisor is zero.

[victorquinn]

Not saying that I disagree with you @RadhiFadlillah however even in normal math a divide by 0 will panic in Go. See this example.

So if the intent is to have this library behave similarly to the Go behavior without this library then a panic would actually make sense here.

This library was modeled heavily after the core Go big int library which also panics in divide by zero scenarios.

That said, I am happy to revisit the broader question of whether we should ever panic. The trend with most Go libraries has been to not panic as it's difficult to rely on a library that could potentially take down your entire application (as @StabbyCutyou astutely put it above). So I am open to considering that change assuming we find a good way of dealing with those errors.

[kekoav]

Found this issue after experiencing a panic 😅 . First, thanks for this library it has been extremely useful, and works well for my use cases.

I think the divide by zero has the better case for panicking but the other panics (I counted 5 in total) are entirely unexpected. The one I encountered was the overflow panic in QuoRem, which is very unfortunate and surprising.

I would suggest following the more recent pattern of returning errors from methods that may error. This does make using the functions perhaps less "elegant" but enables less brittle code since users will know to handle errors. The panics sit there like a ticking time bomb, for those rare combinations of inputs that we could recover from (without panic recovery).

Another argument against panic for a library like this is a Decimal is treated as an opaque data type, and we don't use it like a built-in or numeric type. The syntax is already cumbersome compared to arithmetic with built-ins, but this isn't C++ (no operator overloading) anyway, so I've already let go of expecting perfect readable syntax.

Other decimal libraries like Intel's DFP(written in C) always return error "flags" so the user always anticipates checking for an error after any operation. It does make for more code rife with boilerplate error checking(sounds like Go 😉 ), but provides a high level of confidence that a bad value from a computation doesn't propagate further, and certainly it won't crash the program.

While I like the syntactic sugar that chaining operations provides, I personally would rather be able to provide confidence against panics, and encourage good error handling. I think we could come up with some helper functions or patterns to make chaining easier, but also provide error handling. I also like the suggestion by @RadhiFadlillah for separate functions that panic.

Alternatively, work could be done to remove these panics by adding features: implementing exponents that don't overflow, supporting +/- Infinity as a valid Decimal value, or not converting a float64to string and then using NewFromString() (which may error), etc. . This would make me happy too. Every library is going to have its limits, I think errors at least help to manage that expectation by communicating that an exceptional situation is possible when calling a method.

I would gladly help with some code if this direction is something that would be considered. It seems like the concerns of breaking backward compatibility and creating an "ugly" API may not make this type of a change palatable at this time. Since one of the primary use cases for this type of library is financial computations, an error in the wrong place could actually be pretty costly.

[victorquinn]

I would gladly help with some code if this direction is something that would be considered. It seems like the concerns of breaking backward compatibility and creating an "ugly" API may not make this type of a change palatable at this time. Since one of the primary use cases for this type of library is financial computations, an error in the wrong place could actually be pretty costly.

I'd gladly consider a PR that safely implemented errors as you describe! I don't want to guarantee I'll accept it before I see any code but I'll accept any reasonable implementation.

I'd like to understand and ensure it does so safely and in a mostly backward compatible way, but don't feel strongly that it needs to be 100% backwards compatible (we can tag the current state as 1.0 and consider that 2.0).

So yes if you would like to take that on @kekoav I'd gladly work with you to end at a positive outcome that either minimizes or flat out removes all panics from this library :)

[mwoss]

Thanks all for the conversation over this topic. There's nothing more I could add, all concerns, comments are valid in my opinion. Unfortunately, the change would break backward compatibility. Such a change in lib API could be introduced in a major release. As we still have a lot of work before we could move to v2, we will revisit this issue after we close all other pending issues. Stay tuned.