roc-lang / roc

A fast, friendly, functional language.
https://roc-lang.org
Universal Permissive License v1.0
4.46k stars 314 forks source link

weird error/ crash #5519

Open salarii opened 1 year ago

salarii commented 1 year ago

I compiled it using :
roc dev reprod.roc --linker=legacy


    app "exp"
    packages { pf: "https://github.com/roc-lang/basic-cli/releases/download/0.3.1/97mY3sUwo433-pcnEQUlMhn-sWiIf_J9bPhcAFZoqY4.tar.br" }
    imports [pf.Stdout] 
    provides [main] to pf

makeSquare = \ x, y, val ->
    makeSquareRec x y val []

makeSquareRec = \ x, y, val, list  ->
    if y == 0 then
        list
    else
        makeSquareRec x (y - 1) val (List.append  list (List.repeat  val  x ))

makeCubeRec = \ x, y, z, val ,list  ->
    if z == 0 then
        list
    else
        makeCubeRec  x y  (z - 1) val (List.append list (makeSquare x y val ) )

makeCube = \ x, y, z, val  ->
    makeCubeRec x y z val []

increasedList = \ cnt, list -> 
        if cnt == 0 then
            list
        else
            increasedList (cnt - 1) (List.prepend  list (cnt - 1))

shiftList  = \ center, list ->
        List.map list (\ elem ->  elem + center )

circle = \ x, y, z, r ->
    size = 2*r +1
    list = increasedList size []
    listX = shiftList -r  list
    listY = shiftList -r  list
    listZ = shiftList -r  list
    List.walk listX  [] ( 
        \ stateX, idx -> List.walk listY stateX (
            \ stateY, idy -> List.walk listY stateY  (
                \ stateZ, idz ->
                    if ( Num.powInt idx 2 ) + ( Num.powInt idy 2 ) + ( Num.powInt idz 2 )  <= ( Num.powInt r 2 ) then  
                        List.append  stateZ  {x : idx + x ,y: idy + y,z : idz + z}
                    else 
                        stateZ ))) 

getListFromList = \ list, cnt  ->
    when List.get list cnt  is 
        Ok val -> val
        Err _ -> []   

modifyFieldCube  = \ cube, x, y, z, val ->
    getListFromList cube z
    |> getListFromList y
    |> List.replace x val 
    |> (\ updated -> (List.replace cube z ( List.replace (getListFromList cube z) y updated.list  ).list   ).list  )

setShape = \ cube, shape, val ->
    List.walk shape cube  (
        \ cb, point -> 
            modifyFieldCube cb point.x point.y point.z val )          

main =

    x = 10
    y = 16
    z = 10
    delta  = 2
    modCub  =
            makeCube x y z 0 

    sl  = circle  5  9  4   1

    slice =   
        makeCube x y z 0
        |> setShape sl  1
    Stdout.line  "imho, is  should work "
AjaiKN commented 1 year ago

The reason you're getting an integer overflow here is because Roc is inferring that most of the numbers you're using are Nats, which are unsigned integers. So when you try to subtract 0 - 3, instead of producing a negative number, you get a crash because Nats aren't allowed to be negative.

In this case, it seems like the reason Roc is inferring that everything's a Nat is because you're passing numbers to certain List functions that expect a Nat. So if you insert Num.toNat in a few places, Roc will no longer assume everything's a Nat:

app "exp"
    packages { pf: "https://github.com/roc-lang/basic-cli/releases/download/0.3.1/97mY3sUwo433-pcnEQUlMhn-sWiIf_J9bPhcAFZoqY4.tar.br" }
    imports [pf.Stdout] 
    provides [main] to pf

makeSquare = \ x, y, val ->
    makeSquareRec x y val []

makeSquareRec = \ x, y, val, list  ->
    if y == 0 then
        list
    else
        makeSquareRec x (y - 1) val (List.append  list (List.repeat  val  (Num.toNat x) ))

makeCubeRec = \ x, y, z, val ,list  ->
    if z == 0 then
        list
    else
        makeCubeRec  x y  (z - 1) val (List.append list (makeSquare x y val ) )

makeCube = \ x, y, z, val  ->
    makeCubeRec x y z val []

increasedList = \ cnt, list -> 
        if cnt == 0 then
            list
        else
            increasedList (cnt - 1) (List.prepend  list (cnt - 1))

shiftList  = \ center, list ->
        List.map list (\ elem ->  elem + center )

circle = \ x, y, z, r ->
    size = 2*r +1
    list = increasedList size []
    listX = shiftList -r  list
    listY = shiftList -r  list
    listZ = shiftList -r  list
    List.walk listX  [] ( 
        \ stateX, idx -> List.walk listY stateX (
            \ stateY, idy -> List.walk listY stateY  (
                \ stateZ, idz ->
                    if ( Num.powInt idx 2 ) + ( Num.powInt idy 2 ) + ( Num.powInt idz 2 )  <= ( Num.powInt r 2 ) then  
                        List.append  stateZ  {x : idx + x ,y: idy + y,z : idz + z}
                    else 
                        stateZ ))) 

getListFromList = \ list, cnt  ->
    when List.get list (Num.toNat cnt)  is 
        Ok val -> val
        Err _ -> []   

modifyFieldCube  = \ cube, x, y, z, val ->
    getListFromList cube z
    |> getListFromList y
    |> List.replace (Num.toNat x) val 
    |> (\ updated -> (List.replace cube (Num.toNat z) ( List.replace (getListFromList cube z) (Num.toNat y) updated.list  ).list   ).list  )

setShape = \ cube, shape, val ->
    List.walk shape cube  (
        \ cb, point -> 
            modifyFieldCube cb point.x point.y point.z val )          

main =

    x = 10
    y = 16
    z = 10
    delta  = 2
    modCub  =
            makeCube x y z 0 

    sl  = circle  5  9  4   1

    slice =   
        makeCube x y z 0
        |> setShape sl  1
    Stdout.line  "imho, is  should work "

You can make this change more robust by adding type annotations to make sure Roc knows you want everything to be a signed integer (in the following example, I128):

app "exp"
    packages { pf: "https://github.com/roc-lang/basic-cli/releases/download/0.3.1/97mY3sUwo433-pcnEQUlMhn-sWiIf_J9bPhcAFZoqY4.tar.br" }
    imports [pf.Stdout] 
    provides [main] to pf

makeSquare : I128, I128, I128 -> List (List I128)
makeSquare = \ x, y, val ->
    makeSquareRec x y val []

makeSquareRec = \ x, y, val, list  ->
    if y == 0 then
        list
    else
        makeSquareRec x (y - 1) val (List.append  list (List.repeat  val  (Num.toNat x) ))

makeCubeRec = \ x, y, z, val ,list  ->
    if z == 0 then
        list
    else
        makeCubeRec  x y  (z - 1) val (List.append list (makeSquare x y val ) )

makeCube : I128, I128, I128, I128 -> List (List (List I128))
makeCube = \ x, y, z, val  ->
    makeCubeRec x y z val []

increasedList : I128, List I128 -> List I128
increasedList = \ cnt, list -> 
        if cnt == 0 then
            list
        else
            increasedList (cnt - 1) (List.prepend  list (cnt - 1))

shiftList : I128, List I128 -> List I128
shiftList  = \ center, list ->
        List.map list (\ elem ->  elem + center )

circle : I128, I128, I128, I128 -> List { x: I128, y: I128, z: I128 }
circle = \ x, y, z, r ->
    size = 2*r +1
    list = increasedList size []
    listX = shiftList -r  list
    listY = shiftList -r  list
    listZ = shiftList -r  list
    List.walk listX  [] ( 
        \ stateX, idx -> List.walk listY stateX (
            \ stateY, idy -> List.walk listY stateY  (
                \ stateZ, idz ->
                    if ( Num.powInt idx 2 ) + ( Num.powInt idy 2 ) + ( Num.powInt idz 2 )  <= ( Num.powInt r 2 ) then  
                        List.append  stateZ  {x : idx + x ,y: idy + y,z : idz + z}
                    else 
                        stateZ ))) 

getListFromList : List (List a), I128 -> List a
getListFromList = \ list, cnt  ->
    when List.get list (Num.toNat cnt)  is 
        Ok val -> val
        Err _ -> []   

modifyFieldCube : List (List (List I128)), I128, I128, I128, I128 -> List (List (List I128))
modifyFieldCube  = \ cube, x, y, z, val ->
    getListFromList cube z
    |> getListFromList y
    |> List.replace (Num.toNat x) val 
    |> (\ updated -> (List.replace cube (Num.toNat z) ( List.replace (getListFromList cube z) (Num.toNat y) updated.list  ).list   ).list  )

setShape : List (List (List I128)), List { x: I128, y: I128, z: I128 }, I128 -> List (List (List I128))
setShape = \ cube, shape, val ->
    List.walk shape cube  (
        \ cb, point -> 
            modifyFieldCube cb point.x point.y point.z val )          

main =

    x = 10
    y = 16
    z = 10
    delta  = 2
    modCub  =
            makeCube x y z 0 

    sl  = circle  5  9  4   1

    slice =   
        makeCube x y z 0
        |> setShape sl  1
    Stdout.line  "imho, is  should work "

For a high-level language like Roc without automatic conversion between integer types, I personally think it's a little unfortunate that we're using unsigned integers in so many places. I understand that it's better for efficiency, but I think this is a good example of how it can be unnecessarily confusing for beginners.

bhansconnect commented 1 year ago

Looks like the root issues is that we allowing negating Nats and unsigned numbers. The original code probably should fail to type check.

0 - 3nat works as expected and crashes. In this case 0 is positive and so is 3. They are both unsigned and underflow.

The issue with the example above is when you have negate instead of subtraction: -(3nat) in the repl is the minimal repro. This should fail to type check. Instead, it returns 18446744073709551613 : Nat.

Edit: -(3nat) maybe should type check, but if it does, it definitely should panic. The reason it maybe should type check is that -(0nat) is technically valid.

salarii commented 1 year ago

ok thx for hints, I think it would be difficult to figure this on my own especially when dbg showed me very confusing information (or it was crashing showing nothing ). Now I understand depending on functions I chained together it was inferring different types, I considered this as dbg bug but it was reality. I think I will start put type information on top to avoid problems. I wanted to use Nat but I designed function circle in such a way that int should be used instead and I was screwed, but still it was very little indication of coming problems. I will try to isolate int only to circle and leave Nat in all other places, and val is F32 btw

rtfeldman commented 1 year ago

Edit: -(3nat) maybe should type check, but if it does, it definitely should panic. The reason it maybe should type check is that -(0nat) is technically valid.

Yeah, it should panic if given a nonzero unsigned number - I think that's the fix we want! πŸ‘

bhansconnect commented 1 year ago

it should panic if given a nonzero unsigned number

Are you sure? That seems like a lot less pleasant of an experience in Roc. For example, with the code written above, the user will just get a panic and likely will not know why. Anytime a user is not typing functions, they may accidentally have an unsigned integer type without realizing it. This panic will essentially always be hit. What do we gain from making negate on unsigned numbers panic instead of just never type check? Even if the unsigned number is zero, there is no gain from calling negate, it is a no-op. I think failing to type check would be a much nicer user experience.

AjaiKN commented 1 year ago

It looks like this bug is part of #5197

rtfeldman commented 1 year ago

What do we gain from making negate on unsigned numbers panic instead of just never type check?

Short answer: numeric type simplicity!

Today Num.neg can be Num a -> Num a. What would the type of Num.neg be if we wanted it to accept these:

I8, I16, I32, I64, I128 F32, F64, Dec

...but reject these?

U8, U16, U32, U64, U128, Nat

It couldn't be Num a anymore; we'd need something like NotUnsignedInt a.

Keep in mind how numbers are implemented in order to be able to have type aliases like Int a and Frac a and Num aβ€”for example, considering this is the type of Int and this is the type of U8, what do those types have to change to in a world where we have a type representing "not an unsigned integer, but otherwise could be either an Int or a Frac" (which only Num.neg and Num.abs would use, presumably)?

I'm not saying we couldn't do it (I sketched it out years ago; it involved records in the types of Int and U8), but my conclusion after thinking that through was "yeah, not worth the type system complexity just for this one Num.neg and Num.abs edge case." πŸ˜„