gren-lang / core

Gren's core package
https://packages.gren-lang.org/package/gren-lang/core/version/latest/overview
Other
32 stars 8 forks source link

modBy crashes if given a zero #85

Open boxed opened 4 months ago

boxed commented 4 months ago

modBy 0 x crashes with a Debug.crash (https://github.com/gren-lang/core/blob/098cca86e948f018cea9ca3b10a256b8c89917ba/src/Gren/Kernel/Math.js#L14).

I suggest that there be two functions:

modBy : Int -> Int -> Maybe Int

which returns Nothing on a zero input, and:

modByWithDefault : Int -> Int -> Int -> Int
modByWithDefault modulus default x  =

which does returns a default value on zero.

The reason for the second method is that there is some overhead from the Maybe handling which would be a problem in performance sensitive code, as @robinheghan pointed out on zulip. The docs should point out the performance characteristics.

I think this is a fairly easy change, but I am a bit scared of touching kernel code of course :P

robinheghan commented 4 months ago

I started a discussion on the elm #language-design channel about this. It seems several other languages specialcase n / 0 == 0. Would that be an option? modBy would then follow the same behaviour.

I was also made aware that Roc has Checked versions of such functions. So divChecked or modByChecked, which would return a Maybe. Would that work?

Are there math-functions, other than div and modBy, that we should look at as well?

boxed commented 4 months ago

I find it very annoying that I get crashes for divide by zero in python but I also think that just returning zero would cause even more subtle bugs that would be difficult to track down.

I would for example also like to be able to set it so that any production of NaN in python would raise instead. Much nicer than a garbage value that pollutes the execution flow dowstream some arbitrary amount until it is (hopefully!) caught.

Same here with divide by zero and mod. Yea it's annoying but the right thing and the easy thing should be aligned. That's why I would be against "checked" versions: it makes it opt in to do the right thing. You could call that broken by default too.

GabrielBoehme13 commented 3 months ago

I would like to propose the following…


intDivBy : Int -> Int -> Maybe Int

modBy : Int -> Int -> Maybe Int

remainderBy : Int -> Int -> Maybe Int

unsafeIntDivBy : Int -> Int -> Int

unsafeModBy : Int -> Int -> Int

unsafeRemainderBy : Int -> Int -> Int

…with the unsafe variants available for use in performance-sensitive code. This correctly promotes the safe behavior as the default, and highlights the unsafe nature of the high-performance options.

The unsafe variants would either crash upon division by zero (my preference), or return an undocumented integer result for performance reasons. Either approach (program crash, data corruption) seems reasonable, since the programmer is opting into the unsafe behavior.

It seems several other languages specialcase n / 0 == 0. Would that be an option?

Please, no. I was bitten by an unexpected n // 0 == 0 in an Elm project. I would prefer to have Gren crash on any integer divide-by-zero operation (or even have the // operator removed from the language), rather than getting unexpected zeroes silently corrupting my results. 🙂