janestreet / bin_prot

Binary protocol generator
MIT License
73 stars 21 forks source link

Incompatible types in generated functions for complex types #18

Closed pbiggar closed 5 years ago

pbiggar commented 6 years ago

I'm getting a weird error with a type I want to derive bin_prot functions from. I've been using bin_prot for a few months on all my types without problems otherwise. This is a fairly minimized version. The problem come from (int abc) in N, but only with the ndt and dt definitions. Removing almost anything will make the code compile fine.

type 'x abc = M
              | N of (int abc) * 'x abc
              [@@deriving bin_io]

and ndt = O [@@deriving bin_io]
and dt = ndt abc [@@deriving bin_io]

The error is:

Error: This expression has type ndt -> int
       but an expression was expected of type
         Core_kernel__.Import.int Bin_prot.Size.sizer =
           Core_kernel__.Import.int -> int
       Type ndt is not compatible with type Core_kernel__.Import.int = int

I'm using ocaml 4.06.0, with Core 0.11 (same problem in Core 0.10, I upgraded to avoid the problem). Building with jbuilder 1.0+beta19.1, core, core_extended, ppx_bin_prot all 0.11, ppx_deriving 4.2.1.

Any ideas what's going wrong?

bmillwood commented 6 years ago

Seems likely to be a bug. I get the same with this:

type 'a t = C of int t * string t [@@deriving bin_io];;
yminsky commented 6 years ago

This is likely because OCaml doesn't support polymorphic recursion without some extra type annotations. Funny that we haven't already encountered and fixed this ourselves! I guess such types don't come up that often. If you can get jbuilder to emit the desugared code (which you should at least be able to do with @@deriving_inline), you can then play around with it and see if you can figure out whether you can get it to work by adding type annotations to enable polymorphic recursion.

y

On Wed, Mar 28, 2018 at 2:36 AM, Ben Millwood notifications@github.com wrote:

Seems likely to be a bug. I get the same with this:

type 'a t = C of (int t * string t) option [@@deriving bin_io];;

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/janestreet/bin_prot/issues/18#issuecomment-376777820, or mute the thread https://github.com/notifications/unsubscribe-auth/AArqJg6nu63oilw2X5icoAjIvmDBXl_Nks5tiy9egaJpZM4S-C1X .

pbiggar commented 6 years ago

Yes, that does indeed seem to be the problem @yminsky.

The generated code is:

let rec bin_size_abc _size_of_x =
  function
  | N (v1, v2) ->
      let size = 1 in
      let size = Pervasives.(+) size (bin_size_abc bin_size_string v1) in
      Pervasives.(+) size (bin_size_abc _size_of_x v2)
  | M -> 1
and bin_size_ndt = function | O -> 1
and bin_size_dt v = bin_size_abc bin_size_ndt v

I changed it to be as follows and it compiled:

let rec bin_size_abc : 'a. 'a -> _ =
  fun _size_of_x ->
  function
  | N (v1, v2) ->
      let size = 1 in
      let size = Pervasives.(+) size (bin_size_abc bin_size_string v1) in
      Pervasives.(+) size (bin_size_abc _size_of_x v2)
  | M -> 1
and bin_size_ndt = function | O -> 1

With identical changes to bin_write_abc and bin_read_abc.

yminsky commented 6 years ago

OK. So, seems like a straightforward thing to fix. We'll likely get to it eventually, but if you're feeling adventurous I'm sure a PR with a working implementation would speed our way towards a fix.

pbiggar commented 6 years ago

I took a look in https://github.com/janestreet/ppx_bin_prot/blob/master/src/ppx_bin_prot.ml to try and fix this, but it looks way beyond my abilities at the moment :(

yminsky commented 6 years ago

Well, as a short-term hack, you can get the code with deriving_inline, and then fix it, and use that fixed code in your project. Not pretty, but effective.

jonathan-laurent commented 6 years ago

I submitted a PR fixing this issue.

pbiggar commented 5 years ago

Thanks for the help y'all!