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

Recursive Json.Decode.Decoder types cause `temp._0 is not a function` runtime error #361

Closed
jamesmacaulay opened this Issue Aug 20, 2015 · 16 comments

Comments

Projects
None yet
7 participants
@jamesmacaulay

jamesmacaulay commented Aug 20, 2015

I'm playing around with Json.Decoder trying to build a generic decoder which produces union types of any possible JSON value, and something about the recursive nature of the decoder is causing a runtime error when attempting to decode object or array values:

http://share-elm.com/sprout/55d556ece4b06aacf0e8c185
https://gist.github.com/jamesmacaulay/ea28b7fdb108d53ddeb7

Introducing some kind of indirection can allow the decoder to work correctly with object values at one level of nesting, but some variations of nested objects and arrays always seems to cause trouble.

I might not be doing this all quite right, but I'm working under the assumption that any runtime error in Elm is a bug ;)

I have a feeling this may actually be a problem with the compiler, but I can't easily think of how I might be able to reproduce it in another situation with other types and functions.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Aug 20, 2015

Contributor

It's this issue, isn't it? See the list of related issues referencing that one over there, for similar errors in different situations.

Contributor

jvoigtlaender commented Aug 20, 2015

It's this issue, isn't it? See the list of related issues referencing that one over there, for similar errors in different situations.

@jamesmacaulay

This comment has been minimized.

Show comment
Hide comment
@jamesmacaulay

jamesmacaulay Aug 20, 2015

You might very well be right, I hadn't seen that issue. I don't fully understand the root causes of that issue; however, the indirection I'm introducing makes it so that the top-level decoder is only referenced by the object and array decoders within a function that is only be invoked when needed...I would have thought that this would get around any recursive definition issues. What are the constraints on recursion in Elm?

jamesmacaulay commented Aug 20, 2015

You might very well be right, I hadn't seen that issue. I don't fully understand the root causes of that issue; however, the indirection I'm introducing makes it so that the top-level decoder is only referenced by the object and array decoders within a function that is only be invoked when needed...I would have thought that this would get around any recursive definition issues. What are the constraints on recursion in Elm?

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Aug 20, 2015

Contributor

I'm not actually sure it is exactly the same as the linked issue. It just struck me as likely in view of the similarity to other examples of something like this I've seen (with parser combinators). Also, I hadn't seen that your final example code actually "protects" all the recursive occurrences under lambdas.

It does, so one might expect there to not be any problems. Nevertheless, it could still be the same root issue. Whether you run into the undefinedness depends on when stuff gets run. At initialization time of the module? Or only later? Maybe your Js.dict (Js.succeed True)Js.andThen(\_ -> stuff-to-protect) is not actually effective enough in terms of providing protection by delaying evaluation. Since Js.succeed does not consume any input, it (and then also the dangerous stuff) might run at module initialization time all the same as if you had directly written stuff-to-protect at top-level.

Inspecting the generated JavaScript might help.

Contributor

jvoigtlaender commented Aug 20, 2015

I'm not actually sure it is exactly the same as the linked issue. It just struck me as likely in view of the similarity to other examples of something like this I've seen (with parser combinators). Also, I hadn't seen that your final example code actually "protects" all the recursive occurrences under lambdas.

It does, so one might expect there to not be any problems. Nevertheless, it could still be the same root issue. Whether you run into the undefinedness depends on when stuff gets run. At initialization time of the module? Or only later? Maybe your Js.dict (Js.succeed True)Js.andThen(\_ -> stuff-to-protect) is not actually effective enough in terms of providing protection by delaying evaluation. Since Js.succeed does not consume any input, it (and then also the dangerous stuff) might run at module initialization time all the same as if you had directly written stuff-to-protect at top-level.

Inspecting the generated JavaScript might help.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Aug 20, 2015

Contributor

Changing your example slightly by making object and array be functions with a () argument (see here) leads again to an undefinedness error, and the relevant JavaScript is:

   var decoder = $Json$Decode.oneOf(_L.fromArray([object({ctor: "_Tuple0"})
                                                 ,array({ctor: "_Tuple0"})
                                                 ,string
                                                 ,$int
                                                 ,$float
                                                 ,bool
                                                 ,$null]));
   var array = function (_v4) {
      return function () {
         switch (_v4.ctor)
         {case "_Tuple0":
            return A2($Json$Decode.andThen,
              $Json$Decode.array($Json$Decode.succeed(true)),
              function (_v6) {
                 return function () {
                    return $Json$Decode.map(Array)($Json$Decode.array(decoder));
                 }();
              });}
         _U.badCase($moduleName,
         "on line 57, column 3 to 82");
      }();
   };

The problem here is obvious: decoder is not a function, just a variable whose value is to be computed at module initialization time. But that computation depends on the array function, which is introduced only afterwards.

Some of the discussion in some of the many issues referenced at the end of this was precisely about order of introducing top-level definitions (and about separating declaration and initialization of variables, first introducing some stub that is later overwritten but effectively prevents undefinedness errors at module loading...). Here, if array came before decoder (in the JavaScript), the specific runtime error would not have occurred. Maybe your original example has the same cause for failure.

(Oh, and of course, since the above JavaScript was taken from what http://share-elm.com spit out, it is not clear whether it accurately reflects what the newest version of elm-compiler does.)

Contributor

jvoigtlaender commented Aug 20, 2015

Changing your example slightly by making object and array be functions with a () argument (see here) leads again to an undefinedness error, and the relevant JavaScript is:

   var decoder = $Json$Decode.oneOf(_L.fromArray([object({ctor: "_Tuple0"})
                                                 ,array({ctor: "_Tuple0"})
                                                 ,string
                                                 ,$int
                                                 ,$float
                                                 ,bool
                                                 ,$null]));
   var array = function (_v4) {
      return function () {
         switch (_v4.ctor)
         {case "_Tuple0":
            return A2($Json$Decode.andThen,
              $Json$Decode.array($Json$Decode.succeed(true)),
              function (_v6) {
                 return function () {
                    return $Json$Decode.map(Array)($Json$Decode.array(decoder));
                 }();
              });}
         _U.badCase($moduleName,
         "on line 57, column 3 to 82");
      }();
   };

The problem here is obvious: decoder is not a function, just a variable whose value is to be computed at module initialization time. But that computation depends on the array function, which is introduced only afterwards.

Some of the discussion in some of the many issues referenced at the end of this was precisely about order of introducing top-level definitions (and about separating declaration and initialization of variables, first introducing some stub that is later overwritten but effectively prevents undefinedness errors at module loading...). Here, if array came before decoder (in the JavaScript), the specific runtime error would not have occurred. Maybe your original example has the same cause for failure.

(Oh, and of course, since the above JavaScript was taken from what http://share-elm.com spit out, it is not clear whether it accurately reflects what the newest version of elm-compiler does.)

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Aug 20, 2015

Contributor

This was the discussion I was referring to: elm/compiler#999.

Contributor

jvoigtlaender commented Aug 20, 2015

This was the discussion I was referring to: elm/compiler#999.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Aug 20, 2015

Contributor

Unsurprisingly, making also decoder be a function with a dummy () argument lets your program work as desired: see here. Your previous protection against recursive dependencies was not pervasive enough: The expression Js.dict (Js.succeed True)Js.andThen(\_ -> stuff-to-protect)`` is indeed strictly depending onstuff-to-protect, despite that(_ ->`.

I'm reasonably certain now that what you ran into here is indeed just another instance of elm/compiler#873.

I guess you have the following options:
a) Wait until that issue is fixed and hope the fix is general enough to cover your case.
b) Introduce dummy arguments by hand, quite pervasively.
c) Try to get something like the recursively-combinator from https://github.com/Dandandan/parser into Json.Decode.

Contributor

jvoigtlaender commented Aug 20, 2015

Unsurprisingly, making also decoder be a function with a dummy () argument lets your program work as desired: see here. Your previous protection against recursive dependencies was not pervasive enough: The expression Js.dict (Js.succeed True)Js.andThen(\_ -> stuff-to-protect)`` is indeed strictly depending onstuff-to-protect, despite that(_ ->`.

I'm reasonably certain now that what you ran into here is indeed just another instance of elm/compiler#873.

I guess you have the following options:
a) Wait until that issue is fixed and hope the fix is general enough to cover your case.
b) Introduce dummy arguments by hand, quite pervasively.
c) Try to get something like the recursively-combinator from https://github.com/Dandandan/parser into Json.Decode.

@jamesmacaulay

This comment has been minimized.

Show comment
Hide comment
@jamesmacaulay

jamesmacaulay Aug 20, 2015

Ah, interesting! I had previously gone down that path of making those functions take a () argument, but without also using the andThen trick, so it just ended up blowing the stack.

This solves my problem perfectly, I can define the ()-taking functions privately and only expose single instances of e.g. decoder = buildDecoder () outside the module.

I'll let @evancz or someone else decide if this issue should be left open for the sake of tracking the runtime error. I still don't completely understand if this is something that Json.Decode "should" be able to do without this workaround.

Thanks!

jamesmacaulay commented Aug 20, 2015

Ah, interesting! I had previously gone down that path of making those functions take a () argument, but without also using the andThen trick, so it just ended up blowing the stack.

This solves my problem perfectly, I can define the ()-taking functions privately and only expose single instances of e.g. decoder = buildDecoder () outside the module.

I'll let @evancz or someone else decide if this issue should be left open for the sake of tracking the runtime error. I still don't completely understand if this is something that Json.Decode "should" be able to do without this workaround.

Thanks!

@jamesmacaulay

This comment has been minimized.

Show comment
Hide comment
@jamesmacaulay

jamesmacaulay Aug 20, 2015

Also I will check out that other parser combinator library as well, thanks for the pointer.

jamesmacaulay commented Aug 20, 2015

Also I will check out that other parser combinator library as well, thanks for the pointer.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Aug 21, 2015

Member

Thank you for working through this together! I'm going to close this one and add it to the compiler issue @jvoigtlaender pointed out. I will make an explicit list of issues we need to revisit after we do that fix, so we can confirm that it covers all the cases.

So I'm going to close this for now, and we will reopen if it is not solved when we do the revisit.

Member

evancz commented Aug 21, 2015

Thank you for working through this together! I'm going to close this one and add it to the compiler issue @jvoigtlaender pointed out. I will make an explicit list of issues we need to revisit after we do that fix, so we can confirm that it covers all the cases.

So I'm going to close this for now, and we will reopen if it is not solved when we do the revisit.

@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Jan 6, 2016

@jamesmacaulay have you actually implemented a work around for your general json decoder, because we would be really interested in how you did it?

awalterschulze commented Jan 6, 2016

@jamesmacaulay have you actually implemented a work around for your general json decoder, because we would be really interested in how you did it?

@jamesmacaulay

This comment has been minimized.

Show comment
Hide comment
@jamesmacaulay

jamesmacaulay Jan 10, 2016

@awalterschulze I updated the gist with some code that I haven't tried in a while now but I think it does work:

https://gist.github.com/jamesmacaulay/ea28b7fdb108d53ddeb7#file-anydecoderfixed-elm

edit: yup, it works: http://share-elm.com/sprout/5691dd48e4b070fd20da8431

jamesmacaulay commented Jan 10, 2016

@awalterschulze I updated the gist with some code that I haven't tried in a while now but I think it does work:

https://gist.github.com/jamesmacaulay/ea28b7fdb108d53ddeb7#file-anydecoderfixed-elm

edit: yup, it works: http://share-elm.com/sprout/5691dd48e4b070fd20da8431

@awalterschulze

This comment has been minimized.

Show comment
Hide comment
@awalterschulze

awalterschulze Jan 11, 2016

Thank you very much :)

awalterschulze commented Jan 11, 2016

Thank you very much :)

@noprompt

This comment has been minimized.

Show comment
Hide comment
@noprompt

noprompt Jan 27, 2016

I actually ran into this identical issue yesterday. Thanks for sharing the solution @jamesmacaulay.

@evancz Having a generic decoder like this seems like it would be a fairly useful "out of the box" addition to Json. It's quite reusable.

noprompt commented Jan 27, 2016

I actually ran into this identical issue yesterday. Thanks for sharing the solution @jamesmacaulay.

@evancz Having a generic decoder like this seems like it would be a fairly useful "out of the box" addition to Json. It's quite reusable.

@zachmay

This comment has been minimized.

Show comment
Hide comment
@zachmay

zachmay Jan 29, 2016

@noprompt Did @jamesmacaulay's solution work for you under (presumably) 0.16.0? I've got a similar issue and it doesn't seem to be working for me.

zachmay commented Jan 29, 2016

@noprompt Did @jamesmacaulay's solution work for you under (presumably) 0.16.0? I've got a similar issue and it doesn't seem to be working for me.

@noprompt

This comment has been minimized.

Show comment
Hide comment
@noprompt

noprompt Feb 3, 2016

@zachmay I can verify this works correctly (see here). Everything is effectively the same except for stylistic and naming differences (which were present before I discovered @jamesmacaulay's solution).

noprompt commented Feb 3, 2016

@zachmay I can verify this works correctly (see here). Everything is effectively the same except for stylistic and naming differences (which were present before I discovered @jamesmacaulay's solution).

@narkisr

This comment has been minimized.

Show comment
Hide comment
@narkisr

narkisr Mar 9, 2016

Had the same issue, this is pretty hard to debug

narkisr commented Mar 9, 2016

Had the same issue, this is pretty hard to debug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment