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

Fix ord on variants conflicting with Result #260

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sim642
Copy link
Contributor

@sim642 sim642 commented Mar 18, 2022

Closes #254. Besides Error also applies to Ok, coming from Ppx_deriving_runtime.

Conceptually the fix is simple: just add a type constraint to the to_int function generated into the wildcard_case of variant comparison. This forces type-directed construction resolution to correctly use the constructor from our type instead of Result.

The tricky part was not breaking other tests, where a new variant type is defined with the same name as a standard one, e.g. bool. In that case the to_int type constraint inside the comparator must still refer to the outer bool type being declared, not the one from Ppx_deriving_runtime, which happens to be opened at that point due to sanitize.
A local module definition Ppx_deriving_ord_helper is used to capture the type being declared outside of sanitize, such that inside it, we can refer to the correct bool. Hopefully such type-only local module doesn't have any runtime cost (?), otherwise I'll have to make it conditional to only happen for variant types (currently it's generated unconditionally and otherwise just unused).

@sim642
Copy link
Contributor Author

sim642 commented Mar 19, 2022

I'm guessing my fix only works with OCaml 4.11 and up due to the following point in OCaml 4.11.0 release notes:

  • #6673, #1132, #9617: Relax the handling of explicit polymorphic types. This improves error messages in some polymorphic recursive definition, and requires less polymorphic annotations in some cases of mutually-recursive definitions involving polymorphic recursion. (Leo White, review by Jacques Garrigue and Gabriel Scherer)

I tried slapping Ppx_deriving.strong_type_of_type around the to_int type constraint, but that revealed a very confusing error message from the typechecker: ocaml/ocaml#11129.

Changing that to avoid the Ptyp_poly when there are no free variables leads to a problem with the actually polymorphic cases:

Error: This expression should not be a function, the expected type is
       'a. [ `Cons of 'a | `Nil ] -> Ppx_deriving_runtime.int

I hopefully managed to make it all work by also inserting Pexp_newtypes in the to_int expression, but it's surprisingly involved and I'll have to clean it up before I push it here.

sim642 added a commit to sim642/ppx_deriving that referenced this pull request Mar 19, 2022
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.

Deriving ord on a variant containing Error
1 participant