ocaml-ppx / ppx_deriving

Type-driven code generation for OCaml
MIT License
466 stars 89 forks source link

Fix a bug with [@@deriving make] on type declarations sets #281

Closed NathanReb closed 7 months ago

NathanReb commented 7 months ago

Fixes #272.

There was a bug with [@@deriving make] that caused errors if it was used on a set of type declarations, recursive or non recursive, that contained a non record type.

The deriver was applied to all type declarations in the set, regardless of where the attributes was attached and failed on the ones that weren't record types.

There is no particularly nice way to handle this in ppxlib, I assume the design decision was that one would only attach a single [@@deriving ...] attribute per set and the individual derivers would be responsible to decide which type declarations are relevant. The driver does allow attaching multiple [@@deriving ...] attributes in the same set but performs a merge of all of them so it can lead to confusing situations for the end user.

With all that in mind, there is currently no proper way to tell to which individual type declarations the deriver was attached and even though there might be a hacky one, I'm not sure it would be desirable to use it as it's unlikely that other derivers do so and we probably don't want to have ppx_deriving.make behave differently to the rest of the ppx universe in that regard.

The fix will derive make for all record types in the set if there is at least one, or will report errors about a misuse of the deriver otherwise.

The PR contains the initial regression test added in #273 by @shonfeder along with a small refactoring to properly embed errors. This allows us to treat regular errors, for example errors coming from misuse of [@main] or [@split], and the make can only be derived for record types error differently which we use for the actual fix.

I also updated the README to document how to use ppx_deriving.make with type declaration sets.

NathanReb commented 7 months ago

@sim642 I'm pinging you in case you're interested in reviewing this. I'd recommend the per-commit view here, although I'm happy to split out the error handling bits if it makes the review easier.

I was hoping to get the fix in before hitting a new 6.0 release.