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

show path in JSON/TOML error reporting #3938

Closed
zimbatm opened this issue Aug 16, 2020 · 3 comments
Closed

show path in JSON/TOML error reporting #3938

zimbatm opened this issue Aug 16, 2020 · 3 comments
Labels
error-messages Confusing messages and better diagnostics stale

Comments

@zimbatm
Copy link
Member

zimbatm commented Aug 16, 2020

Is your feature request related to a problem? Please describe.

When importing JSON or TOML files in Nix code, it's not obvious where the data is coming from whenever an error is happening.

$ nix-instantiate --eval demo.nix 
error: --- JSONParseError ---------------------------------------------------- nix-instantiate
[json.exception.parse_error.101] parse error at line 5, column 13: syntax error while parsing array - unexpected string literal; expected ']'

It's great that the error shows in which line of the JSON the error is happening, but the user doesn't know from which file it's happening. To find that out, they have to dig into the Nix code.

A general principle in error reporting is to measure how much effort the user has to go through to find a resolution to the problem. How many hoops they have to jump through. Right now it is not completely straight-forward.

./demo.nix

let
  importJSON = file: builtins.fromJSON (builtins.readFile file);
in
  importJSON ./demo.json

./demo.json

{
  "users": [
    "eelco",
    "grahamc"
    "zimbatm"
  ]
}

Describe the solution you'd like

As we see in the example, the readFile path has long been forgotten when fromJSON is being called.

I can see multiple potential solutions:

  1. Add more builtins. Implement importJSON and importTOML (and importYAML?). That makes it trivial to report the path since the function has access to both data. The downside is that it adds more builtins.

  2. Extend the string context to contain the source of the data. And change fromJSON to take advantage of the extra data during error reporting. The advantage is that this change could be done transparently without changing any of the APIs. Users that upgrade Nix will automatically get the improved error messages.

  3. Re-implement importJSON with tryEval to extend the error message. I haven't really thought of the implications of this one as it seems like the clunkiest approach (it sort of breaks the lazy evaluation).

Solution 2 is my preferred one as it allows to disconnect the read from the eval.

@zimbatm
Copy link
Member Author

zimbatm commented Aug 16, 2020

/cc @domenkozar

@domenkozar domenkozar added the error-messages Confusing messages and better diagnostics label Aug 17, 2020
@stale
Copy link

stale bot commented Feb 13, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Feb 13, 2021
@domenkozar
Copy link
Member

This is fixed:

$ /nix/store/hbwdvww1nqdav95iggp4zpgnpcwk037b-nix-2.6.0pre19700101_59a5f35/bin/nix-instantiate foo.nix --show-trace
error: [json.exception.parse_error.101] parse error at line 5, column 13: syntax error while parsing array - unexpected string literal; expected ']'

       … while decoding a JSON string

       at /home/domen/dev/nixpkgs/foo.nix:2:22:

            1| let
            2|   importJSON = file: builtins.fromJSON (builtins.readFile file);
             |                      ^
            3| in

       … while evaluating 'importJSON'

       at /home/domen/dev/nixpkgs/foo.nix:2:16:

            1| let
            2|   importJSON = file: builtins.fromJSON (builtins.readFile file);
             |                ^
            3| in

       … from call site

       at /home/domen/dev/nixpkgs/foo.nix:4:3:

            3| in
            4|   importJSON ./demo.json
             |   ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error-messages Confusing messages and better diagnostics stale
Projects
None yet
Development

No branches or pull requests

2 participants