Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

if-let macro behavior changed? #1319

Closed
5thWall opened this issue Oct 26, 2023 · 3 comments
Closed

if-let macro behavior changed? #1319

5thWall opened this issue Oct 26, 2023 · 3 comments
Labels
not a bug This is expected behvaior, not a bug wontfix Would be nice if this were fixed, but for <reason> we won't

Comments

@5thWall
Copy link

5thWall commented Oct 26, 2023

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
Copy link
Contributor

sogaiu commented Oct 26, 2023

Possibly related #1189

Also, may be you meant 1.32.1 and not 3.32.1?

@bakpakin bakpakin added bug This is not expected behavior, and needs fixing and removed bug This is not expected behavior, and needs fixing labels Oct 28, 2023
@bakpakin
Copy link
Member

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)))

@bakpakin bakpakin added wontfix Would be nice if this were fixed, but for <reason> we won't not a bug This is expected behvaior, not a bug labels Oct 29, 2023
@AlecTroemel
Copy link
Contributor

Ya letting the false branch of the if-let use variables defined could definitely lead to some confusing code 😆, 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not a bug This is expected behvaior, not a bug wontfix Would be nice if this were fixed, but for <reason> we won't
Projects
None yet
Development

No branches or pull requests

4 participants