ppx_deriving_yojson icon indicating copy to clipboard operation
ppx_deriving_yojson copied to clipboard

Improve error messages

Open NathanReb opened this issue 5 years ago • 3 comments

It would be nice to improve the error reporting of the derived of_yojson functions as it would really ease debugging errors in the parsed json or in hand written parsers involved.

Some of them can be improved quite easily:

[%of_yojson: int] `Null

is

Error ""

I think in general it would also be a huge improvement to be able to track the error location through the json structure.

For example, if we consider the following:

type t =
  { a : A.t
  ; b : B.t
  }
  [@@deriving of_yojson]

it would be helpful to concatenate "in field a: " to the A.of_yojson error message when the parsing of a fails.

I don't have a clear idea of what would be the best way to handle those but would be happy to discuss it with you and to provide a PR if you feel like that should be improved.

NathanReb avatar Oct 30 '18 15:10 NathanReb

The requirement/need/wish seems very sensible to me. Two remarks:

  • The error-reporting scheme should not hurt performance too much. I am highly confident that it's possible to have both at the same time (your idea of recursively passing an "access path" to the parser to report where the problem is seems mostly free), but I would expect any change to come with summary benchmarks to measure this.

  • ppx_deriving_yojson is supposed to desugar into straightforward calls to the Yojson library, not implement elaborate logic itself. Ideally we want all "interesting" logic to be implemented at the Yojson level, and simply used from the deriver. In the present case, however, I am not completely sure how to do this: error-reporting may require transporting information as we traverse data structures, so we need to do something on our end. I would still expect most of the logic to be added/improved at the Yojson level.

gasche avatar Nov 03 '18 10:11 gasche

(Agreed on both counts.)

whitequark avatar Nov 05 '18 03:11 whitequark

Just improving error messages in these case (without adding location) is a nice quick win. I just got bit by [%of_yojson: string] returning Error "" and had no idea where to start :)

emillon avatar Mar 19 '19 10:03 emillon