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

Force bindings inside let-flow to be unique #201

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tanzoniteblack
Copy link
Contributor

Without this, the following code can randomly pick "cat" or "dog" (though "dog" is far more common, making this bug hard to detect or reproduce).

@(let-flow [a "cat"
            a "dog"]
   a)

Note: There's a likelyhood that users updating to this code will need to update their code base due to it failing to compile. I (Ryan Smith) find this acceptable as the code is already broken, even if the user hasn't realized it yet.

Without this, the following code can randomly pick "cat" or "dog" (though "dog" is far more common, making this bug hard to detect or reproduce).

```clj
@(let-flow [a "cat"
            a "dog"]
   a)
```

Note: There's a likelyhood that users updating to this code will need to update their code base due to it failing to compile. I (Ryan Smith) find this acceptable as the code is already broken, even if the user hasn't realized it yet.
@tanzoniteblack tanzoniteblack requested a review from slipset as a code owner July 19, 2021 18:21
@tanzoniteblack
Copy link
Contributor Author

This bug caused one of the most headache inducing bugs that I've ever encountered. Running API servers at Yummly returned different results using the exact same code and data. And to make it worse, when running the code in a repl & adding logging statements to try and debug, each time the code was recompiled it had the potential to randomly switch the results from expected to non.

@ztellman
Copy link
Collaborator

I'm sorry for the headache, but I think the correct thing to do here is to let variables be shadowed the way they would be in a normal let form, such that a in the body is whatever the last-bound a is. I'm assuming this is what you originally expected?

@tanzoniteblack
Copy link
Contributor Author

I'm sorry for the headache, but I think the correct thing to do here is to let variables be shadowed the way they would be in a normal let form, such that a in the body is whatever the last-bound a is. I'm assuming this is what you originally expected?

This would be the desired behavior. The reason I went with this approach when we found the bug last year was a simple "I couldn't figure out a sane way to do this" inside the macro. I'm confident it's possible, it just wasn't something that I came up with a solution for in a reasonable period of time.

@youngilchoo
Copy link

We can make the bindings unique by turning them into a map, then back again. So, add

bindings2   (flatten
    (map
      (fn [[k v]] [(symbol k) v])
      (into {} (map
                 (fn [[s v]] [(keyword s) v])
                 (partition 2 bindings)))))

and use this in the definition for vars and vals'.

@tanzoniteblack
Copy link
Contributor Author

@youngilchoo , there's an easier way to force the property of removing duplicate bindings to make only distinct names without throwing an exception (bindings (distinct bindings)). The problem with this method is that it doesn't cause shadowing, but just replaces.

@(let-flow [a (future "cat")
            b (future a)
            a "dog"]
   [a b])
=> ["cat" "cat"]

compare with let, which is the desired behavior:

(let [a "cat"
      b a
      a "dog"]
  [a b])
=> ["dog" "cat"]

@youngilchoo
Copy link

Good point. Not used to using let bindings like a sequence of assignments!

@KingMob KingMob self-requested a review as a code owner December 6, 2022 06:03
@KingMob
Copy link
Collaborator

KingMob commented Feb 25, 2023

Probably need to do something like rename the bindings based on static single assignment. It should be simpler, since there's no need for φ functions, since there's no branching in a let.

@KingMob KingMob removed the request for review from slipset March 9, 2023 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants