janet-lang / janet

A dynamic language and bytecode vm
https://janet-lang.org
MIT License
3.48k stars 225 forks source link

if-let macro behavior changed? #1319

Closed 5thWall closed 11 months ago

5thWall commented 11 months ago

Hello, I recently picked up Janet and I came across this usage in junk-drawer which is currently throwing a compile error on 1.32.1 with: compile error: unknown symbol new-elapsed.

(if-let [new-elapsed (+ elapsed 1)
            complete? (> new-elapsed duration)]
  (remove-entity wld tween-ent)
  (put tween-data :elapsed-time new-elapsed))))

There's nothing in the documentation about this, so I'm wondering if this is the intended behavior?

sogaiu commented 11 months ago

Possibly related https://github.com/janet-lang/janet/issues/1189

Also, may be you meant 1.32.1 and not 3.32.1?

bakpakin commented 11 months ago

So I have bisected this change to Janet version 1.27.0, but I think the new behavior is the correct behavior. The bindings inside if-let are short-circuiting, meaning that in the false case, it is possible that some of the bindings do not have values at all. While we could initialize things to nil, that doesn't sit right with me.

The above code makes sense since new-elapsed will always be truthy, but really should be written:

(let [new-elapsed (+ elapsed 1)]
  (if (> new-elapsed duration)
    (remove-entity wld tween-ent)
    (put tween-data :elapsed-time new-elapsed)))
AlecTroemel commented 10 months ago

Ya letting the false branch of the if-let use variables defined could definitely lead to some confusing code :laughing:, since you don't necessarily know which definition in the let was false.

Hopefully I did depend on the old behavior anywhere else, otherwise I'll be kicking myself