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

Runtime error when using Json.Decode #448

Closed
dasch opened this Issue Nov 25, 2015 · 10 comments

Comments

Projects
None yet
5 participants
@dasch

dasch commented Nov 25, 2015

I'm running into a runtime error when writing a recursive JSON decoder.

I have a few tests: some that verify that simple Avro schemas can be parsed and a single one that tries to verify that a complex record schema can be parsed. The latter fails with:

parse record type: FAILED. Expected: Ok (AvroRecord ([{ name = "name", type$ = AvroString },{ name = "employees", type$ = AvroInt }])); got: Err ("expecting one of the following:\n    expecting a String but got {\"type\":\"record\",\"name\":\"company\",\"fields\":[{\"type\":\"string\",\"name\":\"name\"},{\"type\":\"int\",\"name\":\"employees\"}]}\n    unknown primitive type record\n    callback is not a function")

The problem is the last part, callback is not a function. If I avoid the recursive reference to avroType, instead inlining the definition of avroType, everything works.

I haven't been able to reproduce it with a minimal example, so I'll post the full code here:

-- Avro.elm
module Avro (AvroType (..), parse) where


import Json.Decode as Json exposing (..)


type alias Field = { name : String, type' : AvroType }

type AvroType
    = AvroRecord (List Field)
    | AvroString
    | AvroInt


parse str =
  decodeString avroType str

avroType =
  oneOf
    [ string `andThen` primitive
    , ("type" := string) `andThen` primitive
    , ("type" := string) `andThen` complex
    ]

primitive type' =
  case type' of
    "string" -> succeed AvroString
    "int" -> succeed AvroInt
    _ -> fail <| "unknown primitive type " ++ type'

complex type' =
  case type' of
    "record" -> record
    _ -> fail <| "unknown complex type " ++ type'

record =
  object1 AvroRecord ("fields" := list field)

field =
  object2 Field
    ("name" := string)
    ("type" := avroType)
-- Test.elm
import ElmTest exposing (..)
import Avro exposing (..)

tests : Test
tests = 
  let
      schema = """
        {
          "type": "record",
          "name": "company", 
          "fields": [
            {
              "type": "string",
              "name": "name"
            },
            {
              "type": "int",
              "name": "employees"
            }
          ]
        }
      """

      assertAvroType expectedType schema =
        assertEqual (Ok expectedType) (Avro.parse schema)
  in
    suite "Avro Schemas"
      [ test "parse record type" <|
          assertAvroType (AvroRecord [
              { name = "name", type' = AvroString },
              { name = "employees", type' = AvroInt }
            ]) (schema)
      , test "parse int type" <|
          assertAvroType AvroInt """ { "type": "int" } """
      , test "parse int type" <|
          assertAvroType AvroInt """ "int" """
      ]

main = 
  elementRunner tests
@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Nov 25, 2015

Contributor

Sounds very much like another instance of this issue: elm/compiler#873.

For some discussion of a case specifically for Json.Decode stuff, see https://github.com/elm-lang/core/issues/361.

On the core/library level, a remedy could be to add something like this or this combinator to Json.Decode.

Contributor

jvoigtlaender commented Nov 25, 2015

Sounds very much like another instance of this issue: elm/compiler#873.

For some discussion of a case specifically for Json.Decode stuff, see https://github.com/elm-lang/core/issues/361.

On the core/library level, a remedy could be to add something like this or this combinator to Json.Decode.

@dasch

This comment has been minimized.

Show comment
Hide comment
@dasch

dasch Nov 25, 2015

:-(

Is there a target release for fixing this?

dasch commented Nov 25, 2015

:-(

Is there a target release for fixing this?

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Nov 25, 2015

Contributor

I don't know. @evancz?

Contributor

jvoigtlaender commented Nov 25, 2015

I don't know. @evancz?

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Nov 25, 2015

Member

I agree with @jvoigtlaender's assessment.

Is there a target release for fixing this?

Depends what you mean by "this". At some point I will sit down and fix the root compiler issue, but it's not my highest priority. It is definitely a bad experience that it leads to a crash, but it is an immediate crash so no goofy stuff happens 10 minutes in to execution.

So that bug is not blocking you, it is just annoying. The compiler just less helpful than normal in this case.

In terms of getting your code working, I'd read this where someone is trying to do pretty much the same thing as you. Then this to learn how to represent self-recursive data nicely.

If you are still struggling to get things working, I'd say ask about it on elm-discuss. There are a ton of folks who can add in the necessary lazy calls to get this working and they are happy to help!

Member

evancz commented Nov 25, 2015

I agree with @jvoigtlaender's assessment.

Is there a target release for fixing this?

Depends what you mean by "this". At some point I will sit down and fix the root compiler issue, but it's not my highest priority. It is definitely a bad experience that it leads to a crash, but it is an immediate crash so no goofy stuff happens 10 minutes in to execution.

So that bug is not blocking you, it is just annoying. The compiler just less helpful than normal in this case.

In terms of getting your code working, I'd read this where someone is trying to do pretty much the same thing as you. Then this to learn how to represent self-recursive data nicely.

If you are still struggling to get things working, I'd say ask about it on elm-discuss. There are a ton of folks who can add in the necessary lazy calls to get this working and they are happy to help!

@evancz evancz closed this Nov 25, 2015

@dasch

This comment has been minimized.

Show comment
Hide comment
@dasch

dasch Nov 26, 2015

I guess it's something that there are workarounds for, but it very much went against my expectations of how to use Elm – and I don't seem to be the only one. I think it's fine if it's not top priority, I just wanted to post this as feedback. I originally posted the code into the Slack channel where I got help, but the people there asked my to file it as a bug.

dasch commented Nov 26, 2015

I guess it's something that there are workarounds for, but it very much went against my expectations of how to use Elm – and I don't seem to be the only one. I think it's fine if it's not top priority, I just wanted to post this as feedback. I originally posted the code into the Slack channel where I got help, but the people there asked my to file it as a bug.

@dasch

This comment has been minimized.

Show comment
Hide comment
@dasch

dasch Nov 26, 2015

Thanks for taking your time to look at this!

dasch commented Nov 26, 2015

Thanks for taking your time to look at this!

@johannth

This comment has been minimized.

Show comment
Hide comment
@johannth

johannth May 6, 2017

I just managed to run into this issue by writing a recursive json decoder. My app compiled but crashed immediately at startup with a rather unhelpful error message, not obviously related to recursion. Definitely my first unfriendly experience with the otherwise super-friendly Elm compiler.

I'm unsure how common this scenario so I understand if this has low priority but just wanted to +1 on providing a compile time error if possible.

johannth commented May 6, 2017

I just managed to run into this issue by writing a recursive json decoder. My app compiled but crashed immediately at startup with a rather unhelpful error message, not obviously related to recursion. Definitely my first unfriendly experience with the otherwise super-friendly Elm compiler.

I'm unsure how common this scenario so I understand if this has low priority but just wanted to +1 on providing a compile time error if possible.

@1602

This comment has been minimized.

Show comment
Hide comment
@1602

1602 Aug 9, 2017

This is one of the features damaging reputation of elm for more than two years (actually, Just One Issue I know about) and as far as I could tell everyone learning elm facing this issue and overall impression is like "they lied to me, it actually fails in runtime".

Just my two pence on prioritisation of this issue: even though it fails immediately, it makes learning elm harder than it should be.

1602 commented Aug 9, 2017

This is one of the features damaging reputation of elm for more than two years (actually, Just One Issue I know about) and as far as I could tell everyone learning elm facing this issue and overall impression is like "they lied to me, it actually fails in runtime".

Just my two pence on prioritisation of this issue: even though it fails immediately, it makes learning elm harder than it should be.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Aug 9, 2017

Contributor

@1602, this will supposedly be fixed with the upcoming release of Elm 0.19. See elm/compiler#1591.

Contributor

jvoigtlaender commented Aug 9, 2017

@1602, this will supposedly be fixed with the upcoming release of Elm 0.19. See elm/compiler#1591.

@1602

This comment has been minimized.

Show comment
Hide comment
@1602

1602 Aug 9, 2017

Thanks @jvoigtlaender!

I will try to test it on my code to confirm that it works with this fix, it seems related because after some code reorganisation I'm getting TypeError: Cannot read property 'tag' of undefined bug, which also referenced in elm/compiler#1591

1602 commented Aug 9, 2017

Thanks @jvoigtlaender!

I will try to test it on my code to confirm that it works with this fix, it seems related because after some code reorganisation I'm getting TypeError: Cannot read property 'tag' of undefined bug, which also referenced in elm/compiler#1591

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