Closed panglesd closed 1 year ago
Note that we decided that we will change the ppxlib behaviour regarding the handling of exceptions, to match the current use of raise_errorf
in PPXs.
Catching an exception will no longer stop the rewriting process. So, the example I gave in the original issue is not relevant any more.
However, embedding errors still have advantages: It allows reporting multiple errors, while still outputting valid AST for the part that were successful. In the case of this PPX, an example where embedding errors is better could written as:
let invalid = [%expr [%e? _] [%e? _]]
which, when raising instead of embedding errors, would not list both errors (both [%e? _]
raise "Expression expected").
Hello @panglesd, I would love to work on this issue but I am still new to ocaml language and would like to please request for your guidance in working on this task.
Hello @Burnleydev1 !
In order to work on this issue, you will have to:
In order to understand metaquot itself, in addition to the resources that you should have read in the outreachy issue https://github.com/ocaml-ppx/ppxlib/issues/389, you can read the chapter of ppxlib on metaquot: https://ocaml.org/p/ppxlib/latest/doc/generating-code.html#metaquot and https://ocaml.org/p/ppxlib/latest/doc/matching-code.html#metaquot
Finally, to turn raising into embeddings, you can look at ppxlib's example: here is the version raising: https://github.com/ocaml-ppx/ppxlib/blob/427f96e126d306538eb541ac591f71b2c68e5dd4/examples/simple-extension-rewriter/ppx_get_env.ml and the same file where the raising has been turned into error embedding: https://github.com/ocaml-ppx/ppxlib/blob/main/examples/simple-extension-rewriter/ppx_get_env.ml
Hi @panglesd, thanks for the pointers. I’ll get on it right away.
Hello @panglesd, I did went through the resources and did some more research, and I would love to ask if changing the code
let cast ext =
match snd ext with
| PPat (p, None) -> p
| PPat (_, Some e) ->
Location.raise_errorf ~loc:e.pexp_loc "guard not expected here"
| _ -> Location.raise_errorf ~loc:(loc_of_extension ext) "pattern expected"
end)
in ppx_metaquot file
let loc = loc_of_extension ext in
match snd ext with
| PPat (p, None) -> p
| PPat (_, Some e) ->
Location.Error.createf ~loc:e.pexp_loc "guard not expected here"
|> Location.Error.to_extension
|> Extension_constructor.create "Error" "guard_not_expected_here" []
| _ ->
Location.Error.createf ~loc "pattern expected"
|> Location.Error.to_extension
|> Extension_constructor.create "Error" "pattern_expected" []
is a good start to solving the issue,
I learned that using result type
is also another method but I'm not sure if these will actually work yet,
and also I wish to ask if I need to change the Raise_errorf
function in `Location too.
thanks
Hello @panglesd, I think I understood this steps:
Search your code for all uses of [raise_errorf](https://ocaml.org/p/ppxlib/latest/doc/Ppxlib/Location/index.html#val-raise_errorf), using grep, for instance. For each of them, turn them into functions returning a (_, extension) result type, using [error_extensionf](https://ocaml.org/p/ppxlib/latest/doc/Ppxlib/Location/index.html#val-error_extensionf) to generate the Error.
but I have difficulties understanding the next steps about propagating the results (using maps and bind), will this be done in another file or it will be done in each file containing raise_errorf
That is a very good start!
The result type is when you cannot embed the error directly in a part of AST. In the present case, you can directly return an AST node, as you've tried to do it in the code sample you sent me, so you don't need to use the result type.
There are still errors in the code you sent me (the Extension_constructor
module does not exists).
You have three steps in your code:
Location.Error.createf
)Extension_constructor.create
, I'm not sure what you meant here.You need to replace 3. with:
Ast_builder
module, with the _extension
suffix`.)It would be good if you can try the code before sending it for help! Could you successfully setup and compile the project? In which case, were you able to make your modifications, get an error from the compiler, and understand the error message?
You need to replace 3. with:
- Turn the extension node into an AST node of the right type (using functions from the Ast_builder module, with the _extension suffix`.)
@panglesd thanks a lot, I will get on in right away
It would be good if you can try the code before sending it for help! Could you successfully setup and compile the project? In which case, were you able to make your modifications, get an error from the compiler, and understand the error message?
I actually created a new project using dune init project <project name>
and started working from there to test the code but I was thinking I will need to create a rewrite I think or edit the ppxlib_metaquot
file in opam/mirage/lib/ppxlib/metaquot/ppxlib_metaquot.ml
that is where I am confused on how to implement the changes since I am scared of modifying a file that can break my current ocaml compiler.
Also, I would like to say that the code in ppxlib_metaquot.ml gives lots of errors when I open it in vs code, please Are there some dependencies I will have to install to have it function normally?
I actually created a new project using dune init project
and started working from there to test the code but I was thinking I will need to create a rewrite I think or edit the ppxlib_metaquot file in opam/mirage/lib/ppxlib/metaquot/ppxlib_metaquot.ml that is where I am confused on how to implement the changes since I am scared of modifying a file that can break my current ocaml compiler. Also, I would like to say that the code in ppxlib_metaquot.ml gives lots of errors when I open it in vs code, please Are there some dependencies I will have to install to have it function normally?
I think I have understood how to do this
I will try to apply the corrections you suggested.
In which case, were you able to make your modifications, get an error from the compiler, and understand the error message?
@panglesd Please I wish to ask if you mean the error message I get when I run dune build
?
I think I have understood how to do this
I assume you were able to clone the ppxlib repo (this one), install the dependencies if needed, and run dune build
, before making modifications to the code. (If not, that's the first step!)
Once you have successfully run dune build
(without your modifications), VS Code should not show any error! And you can start trying to modify the code.
The file in opam/mirage/lib/ppxlib/metaquot/ppxlib_metaquot.ml
has been downloaded when installing some other package, you should not modify it directly! The file you should modify is the one in this repository.
I think I have understood how to do this
I assume you were able to clone the ppxlib repo (this one), install the dependencies if needed, and run
dune build
, before making modifications to the code. (If not, that's the first step!)Once you have successfully run
dune build
(without your modifications), VS Code should not show any error! And you can start trying to modify the code.The file in
opam/mirage/lib/ppxlib/metaquot/ppxlib_metaquot.ml
has been downloaded when installing some other package, you should not modify it directly! The file you should modify is the one in this repository.
Hello @panglesd, thank you for this information.
I tried using the ast_builder.default.extension
but I got the error Unbound value Ast_builder.Default.extension
but there is already open Ast_builder.default
so wish to ask for help solving this issue,
I did research and tried adding let loc = loc_of_extension in
but i get the error Syntax error after unclosed struct, expecting
end'` as well but the ohter error above is no longer displayed
|> Ast_builder.Default.extension
(Location.mknoloc (Longident.Lident "Error.pattern_expected")) []
You should try as much as you can to read the error message, they are already informing you what is wrong!
let... in
: "toplevel" let bindings (let
that are not inside other let
s) don't have a in
at the end: let ... in ...
are for local definitions.Ast_builder.Default
, and there is no mention of an extension
value.Hello @panglesd, I have been trying the various values from the Ast_builder.Default
API and saw the
extension_condtructor
but I don't know how exactly to call it as I am still getting the Unbound module Ast_builder.Default.Extension_constructor
, and I tried calling it using the method |> Ast_builder.Default.Extension_constructor.create "Error" "pattern_expected" []
please I need help grabbing how to use the values in the Ast_builder.Default
API
Hello @panglesd, I created a Pull request to this issue here, I decide to use the Ppat_extension since i kept getting the error This expression should not be a constructor, the expected type is Ppxlib.pattern
when I used other functions.
I think you were misled by the extension constructor thing. Extension constructors are for the "extensive sum types": see here and here.
Even though you don't need extension_constructor
, be careful that the API says that it is a value. You are using it as a module, that's why OCaml complains that it is unbound.
ppat_extension
is the right function to use: it builds a pattern from an extension node.
I now understand why extension_constructor
was not working.
Fixed in #397.
Currently, when
ppxlib.metaquot
encounters an error, it uses theraise_errorf
function to raise a located error.The exception is caught by
ppxlib
, which in this case:[%%%ocaml.error ...]
extension node) to the last valid astThe interruption of the rewriting is quite bad for the user experience! The implication for the users are:
ppxlib.metaquotl
runs at the "context-free" phase, the "last valid AST" is before the context-free phase. So, no other derivers/extenders get run, which generates a lot of noise in the errors (such as "uninterpreted extensions" or "unbound identifiers")Example
For instance:
would report several errors:
expression expected
forinvalid1
(the right error, but not super explicit!)for
invalid1,
invalid2and
invalid3`.The right error for
invalid2
("pattern expected", again it could be more precise) is not shown, and similarly the error forinvalid3
("guard not expected here") is not displayed. Moreover, the "uninterpreted extension" errors add a lot of noise!Since
ppxlib.metaquot
can (in my opinion) be confusing with antiquote, it would be nice if the error reporting were user-friendly and precise!This issue is part of an outreachy internship: see more information here.