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

Removes thunk in decoder, causes infinite loop #318

Open
agj opened this issue Aug 24, 2024 · 1 comment
Open

Removes thunk in decoder, causes infinite loop #318

agj opened this issue Aug 24, 2024 · 1 comment
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@agj
Copy link

agj commented Aug 24, 2024

Hello! I've found a corner case when simplifying decoders.

image

The above simplification looks fine at first glance. The problem is that it actually changes the meaning of the code. There's a reason this decoder was written this way, using a thunk: this snippet of code lives inside the compose function, and after simplification, it will cause the function to recursively call itself in an infinite loop.

@jfmengels jfmengels added bug Something isn't working help wanted Extra attention is needed labels Aug 24, 2024
@jfmengels
Copy link
Owner

jfmengels commented Aug 24, 2024

Hey 👋

Thank you for reporting 🙏
I agree that this is a bug. On top of this recursion issue, this can also have a performance impact, where you might want to evaluate an expensive computation the lambda lazily. I have seen this mostly for Parser.map.

I think adding an exception (not applying map f (succeed x) -> succeed (f x)) to some of the map functions where the value can be evaluated lazily. For the others, I don't think it changes anything to apply the change. For instance, Maybe.map f (Just x) will evaluate eagerly anyway.

So I think the exception should be made for:

  • Json.Decode.map
  • Json.Decode.mapN? 🤔
  • Parser.map (and Parser.Advanced.map) (though this is not implemented yet anyway)
  • Task.map
  • Task.mapError? 🤔
  • Platform.Cmd.map
  • Platform.Sub.map
  • Random.map? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants