Embed errors in the generated AST instead of raising.

[NathanReb]

Embedding errors directly into the AST using the special [%ocaml.error "Some error msg"] is the preferred way of reporting errors in ppxlib based ppx-es so it would be nice to migrate from raising through Location.raise_error* to embedding errors directly.

This would allow reporting several errors at once for a given ppx_yojson extension.

[ayc9]

Hello! Giving this issue a shot for the Outreachy contribution period of Oct 2021.

[desirekaleba]

Hi @NathanReb, I am an Outreachy intern willing to contribute to Expand OCaml's library of standard derivers. Can I be assigned to this, please?

[pitag-ha]

Hi @desirekaleba, nice that you want to contribute to OCaml! Do you already have OCaml up and running on your computer?

[pitag-ha]

Hi @pitag-ha, Yes I have Ocaml up and running on my computer. I followed this to achieve that.

Cool! So yes: perfect to start working on an issue now! Which one do you prefer? This one here or the one on ppx_deriving_yaml that you've expressed interest in?

[pitag-ha]

Just for future reference: @desirekaleba is working on the ppx_deriving_yaml issue now, so this issue here is still open to be picked up by anyone.

[Anu-123-gif]

Hi @pitag-ha ! I am Anubhuti, an Outreachy applicant. I would like to work on this issue. Can this be assigned to me, please?
Thanks!

[pitag-ha]

Hey @Anu-123-gif, welcome! Do you already have ocaml up and running?

[Anu-123-gif]

@pitag-ha Yes, I have ocaml up and running and I have also cloned this repository. I would really appreciate it if you could help me with the next steps, thanks!

[pitag-ha]

Yes, I have ocaml up and running

Cool! So yes: you can go ahead and work on this issue.

I would really appreciate it if you could help me with the next steps, thanks!

The next steps will be to improve the error reporting as asked for in the issue description and, once that's done, to open a PR. There's a very detailed new section for the Ppxlib manual (not merged yet) about ppx error reporting, which explains exactly what's asked for in this issue: https://github.com/ocaml-ppx/ppxlib/pull/318/files

Let me know if you have any further questions!

[Anu-123-gif]

Thanks! I'll reach out to you with any additional questions that I have along the way.

[Anu-123-gif]

Hey @pitag-ha, so far I've understood that the errors are raised using Raise.unsupported_payload ~loc (for example) and in the raise.ml we find Location.raise_errorf ~loc "ppx_yojson reason for error" which contains the message to be displayed.
In the issue description above Nathan mentioned using the special [%ocaml.error "Some error msg"] would be a better approach.
Right now what I'm understanding is that we should replace Raise.unsupported_payload ~loc with [%ocaml.error "Unsupported Payload"] wherever it's mentioned. I wanted to clarify if I am on the right path or not. Please correct me wherever I may have gone wrong in understanding.

[ayc9]

Hi @Anu-123-gif -- you're right that the main task is to move from raising errors (like the payload example you gave), to errors as embedded nodes in the AST. You're also right to look at raise.ml to find all the error messages you will need to use.

Now the question is how to embed the errors: pitag shared with you a link that's a PR diff that is in something similar to markdown. I recommend you copy that into a md editor to be able to read it more easily. At the start of that documentation, they list functions that raise errors and the corresponding function that would embed the error instead. The most relevant one is raise_errorf which would be replaced with error_extensionf

The documentation pitag linked above explains the usage of error_extensionf in more detail, and also has examples. I hope this helps you get started, let us know if there's something from the documentation that is not clear!

[Anu-123-gif]

Hey @ayc9 , thank you for your response. I had read the Pr linked by pitag but now after the information you shared it'll be more clear to me. I'll start working on that and get back to you. Thanks again!

[Anu-123-gif]

Hey, @ayc9 @pitag-ha I'm having a little trouble understanding how to test the changes I have made. Basically, I have figured out where I'm supposed to write the command test/lib/dune is one of the locations. I am not able to decipher how to write the required for example I want to test the changes I made in the expression.ml file ...for this instance what command should I write in the tests or can you please direct me to some resource which might get me a clearer perspective regarding this.I would really appreciat your help. Thanks a lot!

[ayc9]

Hi @Anu-123-gif -- after making the changes and running dune runtest, you should be able to see what the tests expected (in red) and what the code actually outputted (in green).

The first thing to check is if the new things in green are indeed the changes you want (in this case, you want to see [%ocaml.error "Some error msg"]). If it is what you need, you run dune promote to update the tests to the changes you made, and you will not need to do much to the tests beyond that.

Let me know if this makes sense!

Edit: You can check out this page to read more about how the tests work!

[Anu-123-gif]

Hey @ayc9 thanks for the reply.

Update: I went through the intro to OCaml ppx ecosystem and now I understand the workings of each section clearly. I have now started working on the issue with more detailed knowledge. My doubt is cleared now :)
P.S: I found the link to intro to ocmal ppx ecosystem from your intern blog. It was really helpful Thanks for that :)

[Anu-123-gif]

Hey @ayc9, while going throught the PR linked by pitag I found a piece of code pexp_extension ~loc @@ Location.error_extensionf ~loc "Default value can only be derived for int, float, and string." which might be useful for this issue. But I'm having trouble understanding this statement,like the usage of pexp_extension.
Could you please help me out here,thanks!

[ayc9]

Ppxlib provides a function in its API to create error extension nodes: error_extensionf. This function creates an extension node, which then has to be transformed in[to] the right kind of node using [other] functions[,] such as for instance pexp_extension from the Default module of Ast_builder.

This is saying that:

1. First, we use error_extensionf to create an error extension node
2. Then we (usually) use pexp_extension on that node we created. (the @@ is just passing the node into pexp_extension)

The documentation then goes on to talk about whole-file rewriters and context free transformations, but you can skip those details because I don't think they are needed here. For your case specifically, you should search the repo for places where errors are raised (raise_errorf) and replace them with nodes as described above. You should only be changing .ml and .mli files.

After you make some changes, you can try dune runtest to see how the output of the code has changed. Then, you should see what used to happen in red, and you should see something like [%ocaml.error "Some error msg"] in green. If that is the case, dune promote, if you don't see the error nodes like that, or if the code doesn't compile, then you need to investigate further to make it work. I hope this clarifies explicitly your steps! @Anu-123-gif Let us know what you try and if you are stuck!

[Anu-123-gif]

Thank you for your response @ayc9!
I am actually having an issue with the syntax I suppose.
For example: I replaced let unsupported_payload ~loc = Location.raise_errorf ~loc "ppx_yojson: unsupported payload"
with
let unsupported_payload ~loc = pexp_extension ~loc @@ Location.error_extensionf ~loc "ppx_yojson: unsupported payload"
I was shown an error:
Error: Unbound value pexp_extension which I am not able to debug :(
Hope you can help me out here, really thankfull for all your help so far!:heart:

[ayc9]

This error is saying the compiler doesn't know where to get the function from. pexp_extension is from the Default module of the Ast_builder library, so using Ast_builder.Default.pexp_extension should make it work!

[Anu-123-gif]

Thanks a lot @ayc9 it worked when I used this piece of code. I just have one doubt since we don't want to raise errors, what alternative should be used in for example expressions.ml, where Raise.unsupported_payload ~loc is being used.
Also using the above code in .ml, it does not match with the expected declaration code in .mli. That is another area where I'm confused.

[pitag-ha]

Hey @Anu-123-gif, nice to see that you're making progress!

I just have one doubt since we don't want to raise errors, what alternative should be used in for example expressions.ml, where Raise.unsupported_payload ~loc is being used.

I'm not sure I understand your question here, but let's see if the following helps: as a first step, I think it could help to simply not use the Raise module inside expression.ml, but instead to directly embed the error inside expression.ml; i.e. replace the use of Raise.unsupported_payload ~loc by the error extension node you've already sorted out with Aya. Does that make sense?

Also using the above code in .ml, it does not match with the expected declaration code in .mli. That is another area where I'm confused.

Are you referring to expression.ml/expression.mli here or to raise.ml/raise.mli?

[Anu-123-gif]

Hey @pitag-ha! thank you for your reply. I had tried to directly embed the error commands as discussed with aya above. However, doing this led to a lot of errors. For example: File "lib/expression.ml", line 110, characters 34-48: 110 | Expander.expand_int ~loc ~pexp_extension s ^^^^^^^^^^^^^^ Error: Unbound value pexp_extension File "lib/pattern.ml", line 9, characters 21-29: 9 | let expand_int ~loc ~ppat_loc s = ^^^^^^^^ Error (warning 27 [unused-var-strict]): unused variable ppat_loc.
This is the latest error I encountered after trying to minimize the errors by doing different experiments with the code.

Another error which I can't rid of is Error: Signature mismatch: Values do not match: val expand_int : loc:location -> pexp_extension:'a -> string -> expression is not included in val expand_int : loc:location -> pexp_loc:location -> string -> expression The type loc:location -> pexp_extension:'a -> string -> expression is not compatible with the type loc:location -> pexp_loc:location -> string -> expression Type pexp_extension:'a -> string -> expression is not compatible with type pexp_loc:location -> string -> expression This error is encountered a lot by me.
I have figured out that the above error is due to predefined declarations at the top of expression.ml but I'm not able to understand what kind of changes will sort my issue for me.
P.S: I am using pexp_extension on the nodes created everywhere, hopefully, that isn't causing this issue.

Are you referring to expression.ml/expression.mli here or to raise.ml/raise.mli?

At the time I was using Location.error_extensionf command in raise.ml, hence raise.mli was showing a different declaration should have been used. Now that I dropped the idea of using the raise command itself, this will not be an issue as I'm trying to directly embed the error_extensionf command in expressions.ml.

Thanks again!

[ayc9]

Hey @Anu-123-gif -- for the first set of errors: it looks like you are using a function expand_int. When a parameter to a function has a tilde ~ it means the parameter is labelled and you need to make sure to use the right label for that parameter each time you use the function (see this page for examples). The first error message is saying that you are giving a ~pexp_extension labelled argument to expand_int, but the function does not expect such an argument with that label. The second error message is saying that expand_int has a labelled argument ~ppat_loc that is never used in the function body. Hope this clarifies what you need to fix!

As for the signature error mismatch: this is due to the change you made to expand_int's function declaration. The compiler is expecting a parameter with label ~pexp_loc, but it's getting ~pexp_extension. If you need pexp_extension, then you can just call Ast_builder.Default.pexp_extension anytime -- it does not need to be passed as a parameter. Try to keep the changes to function inputs to a minimum, and try to restrict your changes to just the lines that were raising errors.

Hope this helps you understand the compiler's error messages a bit better!