janestreet / janestreet/ppx_fields_conv
Embed errors in the AST instead of raising
- Dominant language
- OCaml
- Stars
- 63
- Forks
- 12
- PR merge metrics
- No merged PRs in 30d
Description
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:
```ocaml
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](https://ocaml.org/p/ppxlib/latest/doc/good-practices.html#handling_errors) of the ppxlib manual.
:question: Would you be willing to accept contributions to this issue? I'm considering assigning its resolution as part of an [outreachy](https://www.outreachy.org/) internship: see more information [here](https://github.com/ocaml-ppx/ppxlib/issues/389).
Contributor guide
Research direction
Start by reading the ppxlib manual's error-handling section and inspect how ppx_fields_conv currently uses raise_errorf. Use the invalid1/invalid2/valid example as the behavioral check: errors from both invalid declarations should be preserved, later derivers should still run, and unrelated noise should be avoided.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- ocaml
- Domain
- compilers
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100