Embed errors in the AST instead of raising

[panglesd]

Currently, when ppx_fields_conv encounters an error, it uses the raise_errorf function to raise a located error.

The exception is caught by ppxlib, which in this case:

• Catch the error,
• stops the rewriting process
• add the error (as a [%%%ocaml.error ...] extension node) to the last valid ast
• Use the resulting AST

The interruption of the rewriting is quite bad for the user experience! The implication for the users are:

• Since ppx_fields_conv 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")
• Only one (meaningful) error from your PPX is reported at a time.

Example

For instance:

type invalid1 = int [@@deriving fields]

type invalid2 = int [@@deriving fields]

type valid = { i : int } [@@deriving fields]
let _ = i

would report several errors:

• Unsupported use of fields (you can only use it on records for invalid1: the right error
• 'Unbound value i`

The error for invalid2 is not shown, since the rewriting is cancelled when the exception is raised.

You can find more information about error reporting in PPXs in this section of the ppxlib manual.

❓ Would you be willing to accept contributions to this issue? I'm considering assigning its resolution as part of an outreachy internship: see more information here.

[panglesd]

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 can 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, the example could be rewritten as:

type invalid = { i : int; create : int; fold : int } [@@deriving fields]

let _ = i

which, when raising instead of embedding errors, would not list both errors (both create and fold are forbidden as field names) ; and would not generate i while it could.

[Burnleydev1]

Hello @panglesd, please I can I work on this issue.

[Burnleydev1]

Hello @panglesd, please I am facing some difficulties, I used the steps you listed in the other issue on metaquot, here is the code I added

     Location.Error.createf ~loc "nonrec is not compatible with the `fields' preprocessor"
      |> Location.Error.to_extension
      |> Ast_builder.Default.ptyp_extension ~loc 

to replace

Location.raise_errorf ~loc "nonrec is not compatible with the `fields' preprocessor"

but I get this error
This expression has type core_type but an expression was expected of type unit because it is in the left-hand side of a sequence
I also get errors like
This expression has type core_type but an expression was expected of type unit because it is in the result of a conditional with no else branch
in the second part of the code

    let is_record td =
      match td.ptype_kind with
      | Ptype_record _ -> true
      | _ -> false
    in
    if not (List.exists tds ~f:is_record)
    then
      Location.Error.createf ~loc
        (match tds with
        | [ _ ] -> "Unsupported use of fields (you can only use it on records)."
        | _ -> "'with fields' can only be applied on type definitions in which at least one type definition is a record")
      |> Location.Error.to_extension
      |> Ast_builder.Default.ptyp_extension ~loc

it seems like an else branch is missing but i dont know the condition to apply if i am to include one, or how to work around this error without including an else branch.

I will do a draft pull request containing the most recent changes