Improving error messaging

[patricoferris]

Feature Request

Error handling in the deriver is not great which was helpfully discovered this thanks to @guptadiksha307.

The biggest improvement would be to provide better error messages when the wrong type of Yaml value is uncovered. For example this code could be a lot better and at least described what it was expecting to find https://github.com/patricoferris/ppx_deriving_yaml/blob/main/lib/value.ml#L162 !

An example

Consider the following little program:

type place = {
  name : string;
  population : string;
} [@@deriving yaml]

type t = {
  places : place list
} [@@deriving yaml]

let yml = {|
places:
  - name: Belfast
    population: 634594|}

let () = 
  match Yaml.of_string_exn yml |> of_yaml with
    | Ok _ -> print_endline "Parsed Nicely :)"
    | Error (`Msg m) -> print_endline m

In the data (the yml string) you can the population of Belfast is given as a number 634594 rather than a string "634594". The type of population in place is a string however so the generated of_yaml function will fail because it will try to read a string and instead find a number!

We might expect the error message to tell us something informative like Was expecting a string but got a <x>, but instead we get the following useful error message!

err

If it is useful the code above was compiled with:

(executable
 (name main)
 (libraries yaml)
 (preprocess
  (pps ppx_deriving_yaml)))

[AniketARS]

Hi, I've read that this is good first issue, would like to give it a try. Please assign me this issue :)

[patricoferris]

Hi @AniketARS,

Thanks for the interest in working on this. Is this part of Outreachy? Before assigning issues it would be good to have the project set up (opam installed and the project building and being tested with dune build and dune runtest respectively).

[AniketARS]

Hi @AniketARS,

Thanks for the interest in working on this. Is this part of Outreachy? Before assigning issues it would be good to have the project set up (opam installed and the project building and being tested with dune build and dune runtest respectively).

Yes, I'm in contribution period of outreachy. Sure, after commenting on this issue I did setup ocaml and tried to learn some of ocaml. It would be helpful to me if you can share some resources to learn more of ocaml specially about syntax.

[pitag-ha]

Welcome @AniketARS and nice that you want to contribute!

It would be helpful to me if you can share some resources to learn more of ocaml specially about syntax.

Have you already had a look at the tutorials on the ocaml.org site? I personally also like the online book Real World OCaml a lot, particular the first chapter Language Concepts gives a good overview over the commonly used aspects of the language, but it might be a bit long to just get started. There is also a fun-mooc course on Functional Programming in OCaml.

By the way, what's your background? Do you already have some experience with functional programming?

[AniketARS]

By the way, what's your background? Do you already have some experience with functional programming?

I will consider myself as a beginner in this context because I have basic experience with functional programming but with python.

[patricoferris]

All ok @AniketARS, is there anything I can help with if you are still working on this issue ?

[AniketARS]

All ok @AniketARS, is there anything I can help with if you are still working on this issue ?

Yes i found the line responsible for this issue:

let mk_pat_match ~loc cases =
  let cases = cases @ [ ([%pat? _], [%expr Error (`Msg "err")]) ] in
  Exp.function_ (List.map (fun (pat, exp) -> Exp.case pat exp) cases)

But what i want is to pass argument and expr_arg to above function from below:

mk_pat_match ~loc
        [ ([%pat? `Float [%p argument]], [%expr Ok [%e expr_arg]]) ]

So that i can produce error stating Type-Mismatch: Given <x> but needed <y> to parse

[patricoferris]

Sounds plausible to me. The Given <x> part is going to be difficult I think because it could literally be anything (that's a polymorphic variant), so Expected a <y> might be easier.

[DeborahLanyero]

Hello @patricoferris and @pitag-ha , Am an outreachy applicant.
I have looked at this issue and it's a good first issue, please assign it to me.
Am in the process of setting up my opam environment , however I have errors building. I however , went back to the installation guide and am following the process once again.

[pitag-ha]

Hi @DeborahLanyero, nice that you're interested in contributing to OCaml. Welcome!

We usually ask our applicants first to set up a working project environment and, once that's done, to sign up for an issue. Would that be ok for you?

[DeborahLanyero]

Yes am okay with it. Thank you.

[DeborahLanyero]

Hello @pitag-ha , I have completed setting up the environment. and am currently going through the resources availed to get familiar with the language.

[ayc9]

Nice @DeborahLanyero ! You can start working on the issue now. I'm Aya one of the co-mentors, definitely let us know of any questions 🙂

[lekha06]

Hi @pitag-ha and @ayc9 , I am lekha, outreachy applicant, I completed setting up the environment and went through some of the resources shared and I would like to start working on this.

[ayc9]

Hey @lekha06 ! It seems Deborah is looking into this issue already.

Have you looked at the other issues listed on the contributions period info page?

For example, this is one of the easy first issues from that list: NathanReb/ppx_yojson#27 You can start with that one instead.

[ayc9]

Hey @DeborahLanyero are you still working on this issue? What do you have so far?

patricoferris added some info on how to run the tests on this page, which might be helpful to you.

Also, you might want to check out the explanations here on how the %t and %p work. For example, on this line, we are checking if typ is a string. If it is, we are calling this function mk_pat_match that adds the error we want to change.

The next step is probably to have the error message say what is the expected type only, like: "Error: expected type." Do you already have some ideas on how to make that change? Let us know what you are trying and if there is something we can help you with!

[DeborahLanyero]

Hello @ayc9 , yes am still working on this issue. I don't know how to add the message also I have been reading about exception handling using the materials provided.
(~loc) returns the location and the expression(in my case the error message?)

[DeborahLanyero]

        | Rtag (name, false, [ t ]) ->
            Exp.case
              [%pat? `O [ ([%p pstring ~loc name.txt], `A [ x ]) ]]
              [%expr
                [%e of_yaml_type_to_expr None t] x >>= fun x ->
                Result.Ok [%e Exp.variant name.txt (Some (evar ~loc "x"))]]
        | _ -> Exp.case [%pat? _] [%expr Error (`Msg "Not implemented")])

Basing on that example, did you mean that i can just change the message from "err" to something like "Error: expected type "string" "?

For the different cases int, float , string, boolean?

[DeborahLanyero]

Hey @ayc9 ,@pitag-ha If so, that I can change the message from "err" to a more desirable one like "Error: expected a string"
The function (mk_pat_match) is storing the message inside (expr_arg).([https://github.com/patricoferris/ppx_deriving_yaml/blob/f07aa2bdc733269d400906d9e4c1439960c477bc/lib/value.ml#L175])

My question is should I remove the expression(expr_arg) and change the cases to:
match typ with
| [%type: int] ->
mk_pat_match ~loc
[
([%pat? Float [%p argument]], [%expr Ok (int_of_float [%expr Error (Msg "error: Expected type Float")]) ] )]);
]
| [%type: float] ->
mk_pat_match ~loc
[ ([%pat? Float [%p argument]], [%expr Ok [%expr Error (Msg "error: Expected type Float")]) ] ]) ]
| [%type: string] ->
mk_pat_match ~loc
[ ([%pat? String [%p argument]], [%expr Ok [%expr Error (Msg "error:Expected type String")]) ] ]) ]
| [%type: bool] ->
mk_pat_match ~loc
[ ([%pat? Bool [%p argument]], [%expr Ok [%expr Error (Msg "error: Expected type Boolean")]) ] ]) ]

[ayc9]

Hi @DeborahLanyero -- expr_arg is not what you should change. The lines you are quoting (see here) are doing something unrelated to detecting errors: they are putting a pattern and an expression into a list, then they are passing that list to the mk_pat_match function.

On line 162, mk_pat_match is what is adding an error node at the end of that list using the @ concatenating operator (see append on this page). And then on line 163 it's doing something else. Remembering our goal: we want to have the error message say what the expected type is. Since we only know what the expected type is at lines 181-197, we need to either

• Add the error message to the list before we go to mk_pat_match, which means we need to add an element within the brackets for each of the cases in lines 181-197.
• Or, we need to pass the name of the type to mk_pat_match and add it to the message on line 162

Do these options make sense? Let us know which one you try and how it works out!

[DeborahLanyero]

Yes please, it does make sense , let me try it out and give feedback accordingly

[prosper74]

hi @DeborahLanyero are you still working on this issue?

If not, I'd like to look into it...

[patricoferris]

I think it's okay for you to work on this @prosper74 if you still want to ^^

[patricoferris]

Fixed in #32