ocaml-ppx / ppx_deriving

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

Fix ord on variants conflicting with `Result` #260

Open sim642 opened 2 years ago

sim642 commented 2 years ago

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 commented 2 years ago

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: https://github.com/ocaml/ocaml/issues/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.