[Bug fix] Handle arithmetic overflow in BigDecimal#div

[kellydanma]

Closes #10502

• 1. handle overflow
• 2. write tests

[straight-shoota]

I think the implementation could be much simplerl Just don't use a variable for scale and replace with the expression directly.

There are two unrelated reads from the variable:

• We can be sure that @scale >= other.scale in the remainder == ZERO branch (otherwise the remainder couldn't be zero). So there is no further test required and we can just use @scale - other.@scale.
• In the final expression of the method's main branch, scale + i can be expanded and restructured to avoid an intermediary negative value: @scale + i - other.scale

[kellydanma]

We can be sure that @scale >= other.scale in the remainder == ZERO branch (otherwise the remainder couldn't be zero)

I thought so too, but consider https://play.crystal-lang.org/#/r/auvc

[straight-shoota]

Oh, right.

Taking some more inspiration from https://github.com/akubera/bigdecimal-rs/blob/fc797c302f4947e429cc331febc66fe3c7badc5e/src/lib.rs#L1273 wouldn't be a bad idea. It seems there's actually a lot missing in our implementation.
And there are only a hand full of specs which don't even go near any edge cases.

[kellydanma]

Taking some more inspiration from https://github.com/akubera/bigdecimal-rs/blob/fc797c302f4947e429cc331febc66fe3c7badc5e/src/lib.rs#L1273 wouldn't be a bad idea.

I was also thinking this. But it seems out of scope for a bug fix. If there's an open issue to rewrite #div, I would be happy to work on it. I will have a lot of free time next weekend!

Do you think the current implementation is sufficient for the bug fix?

[straight-shoota]

I suppose it's probably fine to merge this as is. But I'd prefer to overhaul the entire method's implementation, not just fix a single bug. There's likely more.
I don't think there's an issue to track this, but that's fine. If you want to go ahead with this, just push more changes to this PR. I'm very happy to wait for a bigger refactor.

[kellydanma]

Personally, I'd prefer for this PR to only handle the bug fix.

I haven't had time to dig into lib.rs yet. I'm new to Crystal and would appreciate limiting the scope here to the original issue so there's no pressure for me to rewrite the method 😅

[straight-shoota]

Well my fear is that this bug fix isn't much of a help. Because it's pretty certain that many other things are broken in that method. Maybe in other methods, too. Hence I'd like a complete overhaul.

You should not feel pressured at! Why don't you try things out, see how far you get? You can ask for help here or better in chat. And if you can't get it done, someone else can take over.

[asterite]

Could we change it from UInt64 to Int64 like in Rust at least? I think that would solve most of the issues, right?

[straight-shoota]

The unsigned overflow should already be fixed with the current state of this PR.

[kellydanma]

I think this bug fix, while small, still addresses the original issue entirely. I would feel more comfortable merging a small bug fix as my first contribution, to be honest. I don't understand why a #div rewrite has to be done in a single PR. Let me know if this is possible! 😄

That doesn't mean I won't help with the BigDecimal overhaul in the future though; even changing UInt64 to Int64 is beyond the scope of a #div rewrite. I would be happy to contribute more actively once school is done.

[asterite]

I think this bug fix, while small, still addresses the original issue entirely

Me too. I don't think there's a need to wait to do an entire refactor for possible bugs, if those bugs are yet unknown. And this PR already fixes one of them, while waiting for a refactor won't.