tweag / nickel

Better configuration for less
https://nickel-lang.org/
MIT License
2.43k stars 93 forks source link

Manually defined parametric contract throws `error: non serializable term` error when used without parameters #1460

Open DrRuhe opened 1 year ago

DrRuhe commented 1 year ago

Describe the bug Given this Contract that takes a parameter the non-serializable term error will appear pointing to the definition of the contract, which is unrelated to the actual place where the contract is used incorrectly:

let SomeParametricContract = fun parameter label value =>value
in
{
    without_error | SomeParametricContract "param" = {},
    with_error | SomeParametricContract = {},
}
error: non serializable term
  ┌─ assets/fruits.ncl:1:30
  │
1 │ let SomeParametricContract = fun parameter label value =>value
  │                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  │
  = Nickel only supports serlializing to and from strings, booleans, numbers, enum tags, `null` (depending on the format), as well as records and arrays of serializable values.
  = Functions and special values (such as contract labels) aren't serializable.
  = If you want serialization to ignore a specific value, please use the `not_exported` metadata.

Expected behavior Instead of throwing an unrelated non-serializable term error, the CLI should report something like this:

error: invalid contract
  ┌─ assets/fruits.ncl:1:30
  │
5 │with_error | SomeParametricContract = {},
  │             ^^^^^^^^^^^^^^^^^^^^^^
  │
  = Note: are you missing a parameter?
yannham commented 1 year ago

Hi, thank for reporting! I concur that this is not a rare error to make, and the message is not helping.

I currently don't have a good rationale to throw an error though, we'll have to think about it more. What happens is that a contract annotation like this ends up as something like with_error = SomeParametricContract label {}, which is indeed still a function that is not serializable. The issue is that we can't just error out when a contract application evaluates to a function: typically, a function contract like f | Number -> String turns into $arrow $number $string f, and should evaluate to a function (a version of f wrapped with additional checks).

Maybe we can backtrack on such a non-serializable error and check this case in an ad-hoc way (does the problematic value have a contract attached).

Another improvement, independently from this contract error, should be to show which field was being evaluated/serialized when the error occurred; that would at least give a minimum amount of error locality.