elm/core

NaN argument to the mod operator (%) causes unbounded recursion

Closed this issue · 3 comments

mch commented

Running this simple program causes a JavaScript exception:

module Main exposing (..)

import Html exposing (Html, text, div)


main : Html a
main =
    let
        nanFloat : Float
        nanFloat = sqrt -1
        
        nanInt : Int
        nanInt = ceiling nanFloat
        
        recursionHere : Int
        recursionHere = 6 % nanInt

    in
        div [] 
            [ div [] [ text (toString recursionHere) ]
            ]

Resulting error:

Uncaught RangeError: Maximum call stack size exceeded
    at mod (VM98 javascript:1117)
    at mod (VM98 javascript:1124)
    at mod (VM98 javascript:1124)
    at mod (VM98 javascript:1124)
    ...

On Ellie: https://ellie-app.com/36bpc9T7v5ba1/5

The implementation should check for NaN in the arguments, and likely return NaN. In addition, it should not be possible to get an Int containing a value like Nan or Infinity.

Browser: Chrome 58.0.3029.96 (64-bit)
OS: macOS 10.12.4
Elm: 0.18.0

Thanks for the issue! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

Judging by the type signature of both isNaN and isInfinite the problem rather lies in the fact that you were able to get a NaN typed as an Int.

One possible fix would be to add a check to round, floor, ceiling and truncate.

However, some research led me to this disccussion on ghc's trac. Of particular interest is tristes_tigres suggestion to separate rounding and conversion functions, to avoid the performance hit of checking undefined values.

This could be implemented with the addition of a toInt function (or fromFloat to avoid a possible collision with String.toInt). The new type signatures would look like:

round: Float -> Float
ceiling: Float -> Float
floor: Float -> Float
truncate: Float -> Float
toInt: Float -> Int
toFloat: Int -> Float  -- unchanged

Current uses of round, ceiling, floor and truncate would need to be composed with toInt

Whatever seems more appropriate, I would be willing to tackle this issue

Tracking this in #721. Gathering all the JS math oddities may help clarify how to address it in a coherent way that does not have really troubling performance implications.