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

gencopy fails to build on OCaml 4.13.1 #118

Closed
rwmjones opened this issue Oct 5, 2021 · 6 comments
Closed

gencopy fails to build on OCaml 4.13.1 #118

rwmjones opened this issue Oct 5, 2021 · 6 comments

Comments

@rwmjones
Copy link

rwmjones commented Oct 5, 2021

+ dune build -j48
File "tools/gencopy.ml", line 79, characters 43-74:
79 |     Pat.construct ?loc ?attrs (lid ?loc s) (may_tuple ?loc Pat.tuple args)
                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: This expression has type Parsetree.pattern option
       but an expression was expected of type
         (str list * Parsetree.pattern) option
       Type Parsetree.pattern is not compatible with type
         str list * Parsetree.pattern 
@rwmjones
Copy link
Author

rwmjones commented Oct 5, 2021

This patch is what we're carrying in Fedora. It allows compilation but I've no idea if it's correct:

https://src.fedoraproject.org/rpms/ocaml-migrate-parsetree/blob/rawhide/f/0001-tools-gencopy.ml-Fix-compilation.patch

@pitag-ha
Copy link
Member

pitag-ha commented Oct 5, 2021

Ok, thanks for reporting!

It would still be good to keep on using may_tuple since the variant constructor might have no or only one parameter (whereas if you use Pat.tuple always, you're assuming that it has more than one parameter), so I think something like

Pat.construct ?loc ?attrs (lid ?loc s) (Option.map (fun tuple -> ([], tuple)) (may_tuple ?loc Pat.tuple args))

would work.

If this isn't urgent now, I'll look into it in more detail when we need to generate the migration modules for 4.14. We'll need to generate them for ppxlib directly, not for OMP anymore, but we'll still use this same tooling.

@rwmjones
Copy link
Author

rwmjones commented Oct 5, 2021

Sure, no problem at the moment. I have enough of a workaround for now.

@pitag-ha
Copy link
Member

pitag-ha commented Oct 6, 2021

Cool! By the way, out of curiosity: What do you need the gencopy for?

@rwmjones
Copy link
Author

rwmjones commented Oct 6, 2021

We just package it in Fedora. I don't have any insight into whether anyone uses it.

@pitag-ha
Copy link
Member

pitag-ha commented Dec 2, 2021

This has been fixed in #119.

@pitag-ha pitag-ha closed this as completed Dec 2, 2021
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

No branches or pull requests

2 participants