Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

shonfeder
Copy link
Contributor

@shonfeder shonfeder commented Feb 24, 2024

Adds a regression test for #272

Note that is this on top of #263 and only 7dbaf97 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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

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

@NathanReb
Copy link
Collaborator

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

Thanks reporting and reproducing this bug!

@NathanReb NathanReb closed this Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants