search

ocaml-ppx / ppx_deriving

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

Regression test for incorrect behavior when deriving on mutually recursive types #273

Closed shonfeder closed 7 months ago

shonfeder commented 8 months ago

Adds a regression test for #272

Note that is this on top of #263 and only 7dbaf97f797d127cd6d23aa11863e480febac289 is relevant currently.

During the ppxlib dev meeting, @pitag-ha and @NathanReb asked me to see if #263 would fix the problem here. The failing test here unfortunately demonstrates that it does not fix the problem as is:

File "src_test/make/test_deriving_make.ml", lines 63-64, characters 2-19:
63 | ..and secondary_recursive_type = string
64 |   [@@deriving show]
Error: make can be derived only for record types

See https://ocaml.ci.dev/github/ocaml-ppx/ppx_deriving/commit/7dbaf97f797d127cd6d23aa11863e480febac289/variant/alpine-3.19-4.14_opam-2.1#L238-241

NathanReb commented 8 months ago

Just to clarify what the bug is and what is the expected fix.

What happens here is that it tries to derive make for all types which it can't. What you would like instead is that it would only derive make for the type declarations in the recursive type decl that have the [@@deriving make] right?

NathanReb commented 8 months ago

It would be good to have expect tests there instead, that way we could have merged this PR with a green CI and update the test along with the bug fix.

I'll look into merging #263 once the 5.02 compat in ppxlib is dealt with and will come back to this afterward! Sorry for the delay!

shonfeder commented 8 months ago

Just to clarify what the bug is and what is the expected fix.

What happens here is that it tries to derive make for all types which it can't. What you would like instead is that it would only derive make for the type declarations in the recursive type decl that have the [@@deriving make] right?

That's right. It's deriving for declarations which are not annotated, which then breaks when these are underivable. Instead, it should only derive for declarations that are annotated.

shonfeder commented 8 months ago

It would be good to have expect tests there instead, that way we could have merged this PR with a green CI and update the test along with the bug fix.

I'll look into merging #263 once the 5.02 compat in ppxlib is dealt with and will come back to this afterward! Sorry for the delay!

No worries! I am not blocked by this.

NathanReb commented 8 months ago

We finally merged #263 so I'll be able to look into this properly. Thanks for your patience @shonfeder !

NathanReb commented 7 months ago

I'm closing this as I added your test as part of #281 !

Thanks reporting and reproducing this bug!